Summary

Make file attachment thumbnail actions keyboard accessible

Review Request #10898 — Created Feb. 8, 2020 and updated March 20, 2020, 5:56 p.m.

Information

Owner

hxqlin

Repository

Review Board

Branch

master

Bugs

Depends On

Commit

cd88d22...

Reviewers

Groups

reviewboard

People

Description

This patch enables users who are not using a mouse to take advantage of the
actions attached to file attachment thumbnails in a review request. When
a file attachment is tabbed to, the menu appears and the actions in the
menu can be tabbed back and forth using tab & shift+tab. An action can be
taken using the enter key.

The implementation of the file attachment menu was slightly changed to
accomodate the new behavior. Rather than using display: none and
display: block for the file actions container, we use opacity: 0 and
opacity: 1 to show and hide the container. When we use display: none,
this prevents the children of the container from being tabbed to. In addition,
using padding for the shadow causes the focus ring to be wider than necessary.
By using margin, we can get a tighter and clearer focus ring with the same
shadow.

Testing Done

Manual testing:
- using tab & shift+tab to navigate to the file attachments, verifying that the
file actions menu is opened and that each item can be selected using the enter key
and the actions menu is closed once the thumbnail loses focus

Unit tests:
- testing that the hover event is fired when a mouse is hovered over the thumbnail
- testing that the focus event is fired when the thumbnail is tabbed to

Files

Issues

Description	From	Last Updated
This RB enables users, Should RB be "patch" or "review request"?	mike_conley	March 2, 2020, 1:21 p.m.
We can combine this with the definition of this._$file. Note that in ES6 we also now prefer the native function.bind() …	david	Feb. 24, 2020, 10:46 a.m.
This should have an "Args" section documenting what e is.	david	Feb. 24, 2020, 10:46 a.m.
This needs an "Args:" section.	david	Feb. 24, 2020, 10:46 a.m.
What listens for this?	mike_conley	March 20, 2020, 5:55 p.m.

flake8 passed.

JSHint passed.

Change Summary:

Finish implementation of keyboard nav in the file actions. Attach videos of previous behaviour + new behaviour and update RB description.

Summary:

-	[WIP] Show file actions on file attachment focus
+	Show file actions on file attachment focus

Description:

~		This RB enables users who are not using a mouse to take advantage of the actions attached to file attachment thumbnails in a review request. When a file attachment is tabbed to, the menu appears but individual options at the moment can't be selected. The styling/positioning of the menu is also a little off.
	~	This RB enables users who are not using a mouse to take advantage of the actions attached to file attachment thumbnails in a review request. When a file attachment is tabbed to, the menu appears and the actions in the menu can be tabbed back and forth using tab & shift+tab. An action can be taken using the enter key.
	+
	+	The implementation of the file attachment menu was slightly changed to accomodate the new behavior. Rather than using `display: none` and `display: block` for the file actions container, we use `opacity: 0` and `opacity: 1` to show and hide the container. When we use `display: none`, this prevents the children of the container from being tabbed to. In addition, using padding for the shadow causes the focus ring to be wider than necessary. By using margin, we can get a tighter and clearer focus ring with the same shadow.

Commit:

-	36cf04319223581ab943c99d4c08d31b4dec40b7
+	700c1d4614431d447a2f291cf50f18f6fb1b946d

Diff:

Revision 2 (+36 -21)

Show changes

	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/static/rb/js/views/fileAttachmentThumbnailView.es6.js

Removed Files:

Added Files:

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Summary:

-	Show file actions on file attachment focus
+	Make file attachment thumbnail actions keyboard accessible

Change Summary:

Add tests

Commit:

-	700c1d4614431d447a2f291cf50f18f6fb1b946d
+	84a721fde5df4e7be4d10d462f03493b7d0b05c9

Diff:

Revision 3 (+73 -21)

Show changes

	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/static/rb/js/views/fileAttachmentThumbnailView.es6.js
	reviewboard/static/rb/js/views/tests/fileAttachmentThumbnailViewTests.es6.js

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Testing Done:

~		Manual testing only so far.
	~	Manual testing & new unit tests.

Change Summary:

Fix description & testing sections

Description:

~		This RB enables users who are not using a mouse to take advantage of the actions attached to file attachment thumbnails in a review request. When a file attachment is tabbed to, the menu appears and the actions in the menu can be tabbed back and forth using tab & shift+tab. An action can be taken using the enter key.
	~	This RB enables users who are not using a mouse to take advantage of the
	+	actions attached to file attachment thumbnails in a review request. When
	+	a file attachment is tabbed to, the menu appears and the actions in the
	+	menu can be tabbed back and forth using tab & shift+tab. An action can be
	+	taken using the enter key.

~		The implementation of the file attachment menu was slightly changed to accomodate the new behavior. Rather than using `display: none` and `display: block` for the file actions container, we use `opacity: 0` and `opacity: 1` to show and hide the container. When we use `display: none`, this prevents the children of the container from being tabbed to. In addition, using padding for the shadow causes the focus ring to be wider than necessary. By using margin, we can get a tighter and clearer focus ring with the same shadow.
	~	The implementation of the file attachment menu was slightly changed to
	+	accomodate the new behavior. Rather than using `display: none` and
	+	`display: block` for the file actions container, we use `opacity: 0` and
	+	`opacity: 1` to show and hide the container. When we use `display: none`,
	+	this prevents the children of the container from being tabbed to. In addition,
	+	using padding for the shadow causes the focus ring to be wider than necessary.
	+	By using margin, we can get a tighter and clearer focus ring with the same
	+	shadow.

Testing Done:

~		Manual testing & new unit tests.
	~	Manual testing:
	+	- using tab & shift+tab to navigate to the file attachments, verifying that the
	+	file actions menu is opened and that each item can be selected using the enter key
	+	and the actions menu is closed once the thumbnail loses focus
	+
	+	Unit tests:
	+	- testing that the hover event is fired when a mouse is hovered over the thumbnail
	+	- testing that the focus event is fired when the thumbnail is tabbed to

reviewboard/static/rb/js/views/fileAttachmentThumbnailView.es6.js (Diff revision 3)

The issue has been resolved. Show all issues

We can combine this with the definition of this._$file. Note that in ES6 we also now prefer the native function.bind() over the underscore _.bind() method.

this._$file = this.$('.file')
    .focusin(this._onHoverOrFocusIn.bind(this))
    .focusout(this._onHoverOrFocusOut.bind(this));

reviewboard/static/rb/js/views/fileAttachmentThumbnailView.es6.js (Diff revision 3)
The issue has been resolved. Show all issues
```
This should have an "Args" section documenting what e is.
```
reviewboard/static/rb/js/views/fileAttachmentThumbnailView.es6.js (Diff revision 3)
The issue has been resolved. Show all issues
```
This needs an "Args:" section.
```

Change Summary:

Fix issues from review.

Commit:

-	84a721fde5df4e7be4d10d462f03493b7d0b05c9
+	cd88d227751a9f9b688e72e37ad5bf7373dc3a9a

Diff:

Revision 4 (+81 -21)

Show changes

	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/static/rb/js/views/fileAttachmentThumbnailView.es6.js
	reviewboard/static/rb/js/views/tests/fileAttachmentThumbnailViewTests.es6.js

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Hi Hannah,

This looks pretty good! Watching the videos, I'll note that the selection highlight is missing behind the right and bottom edge of the menu when the download item is selected. Do we know what's happening there and why?

hxqlin March 2, 2020, 1:39 p.m.

That selection highlight is something that's part of RB itself. According to Christian: we clip the edges to try to make them flush against the top border of the review request box (you can observe the same behavior by tabbing through the "Download Diff," "Review," etc. buttons at the top of the review request).

The issue has been resolved. Show all issues

This RB enables users,

Should RB be "patch" or "review request"?

hxqlin March 2, 2020, 1:21 p.m.

woops you're right! using "RB" was an old habit from LinkedIn - they call review requests "RBs" :P

Description:

~		This RB enables users who are not using a mouse to take advantage of the
	~	This patch enables users who are not using a mouse to take advantage of the
		actions attached to file attachment thumbnails in a review request. When
		a file attachment is tabbed to, the menu appears and the actions in the
		menu can be tabbed back and forth using tab & shift+tab. An action can be
		taken using the enter key.

		The implementation of the file attachment menu was slightly changed to
		accomodate the new behavior. Rather than using `display: none` and
		`display: block` for the file actions container, we use `opacity: 0` and
		`opacity: 1` to show and hide the container. When we use `display: none`,
		this prevents the children of the container from being tabbed to. In addition,
		using padding for the shadow causes the focus ring to be wider than necessary.
		By using margin, we can get a tighter and clearer focus ring with the same
		shadow.

Thanks, Hannah! So I gave this patch a spin, and I noticed that it works fine on Chrome, but on Firefox, I'm unable to tab-focus within the attachment menu. Any idea what's going on there?

hxqlin March 9, 2020, 10:57 p.m.

Hm that's super interesting; thanks for investigating that! I tested that out myself and found the same thing. This wasn't isolated to just this feature though - I noticed I couldn't tab to any link/anchor tag on the page! I wasn't sure if this was a Review Board problem or a browser problem, so I mocked up a super basic webpage with a list and some anchor tags.

Turns out I couldn't tab to any of those links in Firefox either, but I could in Chrome. I did some digging and it turns out that it's actually a Mac problem: https://stackoverflow.com/questions/11704828/how-to-allow-keyboard-focus-of-links-in-firefox Mozilla treats the Mac system preferences differently so anchor tags aren't focusable by default. I got it working on Firefox by doing step 2 at the Stack Overflow link, or going to "about:config" in Firefox and creating a new accessibility.tabfocus preference and setting it to 7.

reviewboard/static/rb/js/views/fileAttachmentThumbnailView.es6.js (Diff revision 4)

The issue has been dropped. Show all issues

What listens for this?

hxqlin March 9, 2020, 10:57 p.m.

I'm not entirely sure what you mean here, but I think you're asking if there's some other kind of component kind of watching for this event? In that case, I think it's a no. I was following the pattern of the hover event, which did this.trigger('hoverIn', this.$el);. I guess another goal of including those lines was to make sure the default hover and focus events are fired on top of the custom behavior.

mike_conley March 20, 2020, 5:55 p.m.

I guess another goal of including those lines was to make sure the default hover and focus events are fired on top of the custom behavior.

I believe no matter what, the default events will fire (unless you use preventDefault / stopPropagation on them). What this does is fires a custom focusIn event through the BackboneJS mechanisms. I guess this is a way to stay consistent however, even though nobody is listening for this one.

Ship it!

```
This looks good to me! Thank you!
```