Summary

Make collapse diff buttons keyboard accessible

Review Request #10925 — Created Feb. 27, 2020 and updated March 31, 2020, 10:58 a.m.

Information

Owner

hxqlin

Repository

Review Board

Branch

master

Bugs

Depends On

Blocks

10951

Commit

8679dc4...

Reviewers

Groups

reviewboard

People

Description

Previously, the collapse diff button on reviewable diffs and diffs in
comment fragments could not be used with the keyboard as shown in the
attached video collapse-btn-old.mov, unlike the expand diff button. This
review request makes the collapse diff button keyboard accessible. The new
behavior is documented in the attached video collapse-btn-new.mov. There
is also a side-by-side screenshot comparison of the old and new button.

The collapse button used to be a <div>, which is not focusable by
default. The fix is to change the <div> to an <a>, which made the button
focusable but did not invoke the collapse diff action. (Using an <a> is
consistent with the expand diff button.) By adding the href=# attribute,
both clicking on the element and using the enter key while the element is
focused invokes the click handler to collapse the diff.

Testing Done

Manual testing on reviewable diffs and comment fragment diffs:
- used tab & shift+tab to check the collapse diff button can be tabbed to
- used enter key to invoke the collapse diff action and re-expand diff
using only the keyboard

Files

Issues

Description	From	Last Updated
A few notes on the change description: The text is wrapping far too early. Can you wrap at ~75 characters? …	chipx86	March 31, 2020, 10:50 a.m.
Can you show a side-by-side screenshot of an old collapse button and a new one? There's some fundamental changes to …	chipx86	March 31, 2020, 10:51 a.m.
The attributes aren't aligned correctly.	chipx86	March 31, 2020, 10:57 a.m.
The attributes aren't aligned correctly. Since you're updating this line anyway to wrap, let's move the data-... attributes to the …	chipx86	March 31, 2020, 10:57 a.m.

flake8 passed.

JSHint passed.

The issue has been resolved. Show all issues

A few notes on the change description:

The text is wrapping far too early. Can you wrap at ~75 characters?
HTML tags should become string literals (backticks)
The Asana link isn't going to be useful to outside contributors, and this change description will be used when landing. You can just remove that from here.

The issue has been resolved. Show all issues

Can you show a side-by-side screenshot of an old collapse button and a new one? There's some fundamental changes to the display type that's been brought in through this, and I want to make sure it's going to be consistent.

reviewboard/templates/diffviewer/diff_file_fragment.html (Diff revision 1)
The issue has been resolved. Show all issues
```
The attributes aren't aligned correctly.
```

reviewboard/templates/reviews/diff_comment_fragment.html (Diff revision 1)

The issue has been resolved. Show all issues

The attributes aren't aligned correctly.
Since you're updating this line anyway to wrap, let's move the data-... attributes to the second line, keep things under our line length limits.
And fix our variable references to be {{comment.id}} instead of {{ comment.id }} (the latter must have snuck in).
These comments apply to both modified lines in the file.

Change Summary:

Fix issues from review

Description:

~		Previously, the collapse diff button on reviewable diffs and
~		diffs in comment fragments could not be used with the keyboard
~		as shown in the attached video `collapse-btn-old.mov`, unlike
~		the expand diff button. This review request makes the collapse
~		diff button keyboard accessible. The new behavior is documented
~		in the attached video `collapse-btn-new.mov`.
	~	Previously, the collapse diff button on reviewable diffs and diffs in
	~	comment fragments could not be used with the keyboard as shown in the
	~	attached video `collapse-btn-old.mov`, unlike the expand diff button. This
	~	review request makes the collapse diff button keyboard accessible. The new
	~	behavior is documented in the attached video `collapse-btn-new.mov`. There
	~	is also a side-by-side screenshot comparison of the old and new button.

~		The collapse button used to be a <div>, which is not focusable
~		by default. The fix is to change the <div> to an <a>, which made
~		the button focusable but did not invoke the collapse diff action.
~		(Using an <a> is consistent with the expand diff button.) By
~		adding the `href=#` attribute, both clicking on the element and
~		using the enter key while the element is focused invokes the
	~	The collapse button used to be a `<div>`, which is not focusable by
	~	default. The fix is to change the `<div>` to an `<a>`, which made the button
	~	focusable but did not invoke the collapse diff action. (Using an `<a>` is
	~	consistent with the expand diff button.) By adding the `href=#` attribute,
	~	both clicking on the element and using the enter key while the element is
	~	focused invokes the `click` handler to collapse the diff.
-		`click` handler to collapse the diff.
-
-		Asana link: https://app.asana.com/0/40883733502513/1157098340882771/f

Testing Done:

		Manual testing on reviewable diffs and comment fragment diffs:
~		- used tab & shift+tab to check the collapse diff button can be
~		tabbed to
~		- used enter key to invoke the collapse diff action and re-expand
	~	- used tab & shift+tab to check the collapse diff button can be tabbed to
	~	- used enter key to invoke the collapse diff action and re-expand diff
	~	using only the keyboard
-		diff using only the keyboard

Commit:

1c430fa229dd65539005593b50cd3a8ae087b4b4

8679dc402cabef2f347d59d2fc2afc49360e024c

Diff:

Revision 2 (+9 -11)

Show changes

	reviewboard/static/rb/css/pages/diffviewer.less
	reviewboard/templates/diffviewer/diff_file_fragment.html
	reviewboard/templates/reviews/diff_comment_fragment.html

Added Files:

Checks run (2 succeeded)

flake8 passed.

JSHint passed.