Make file attachment thumbnail actions keyboard accessible

Review Request #10898 — Created Feb. 8, 2020 and updated

Information

Review Board
master
cd88d22...

Reviewers

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.

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


Description From Last Updated

This RB enables users, Should RB be "patch" or "review request"?

mike_conleymike_conley

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

daviddavid

This should have an "Args" section documenting what e is.

daviddavid

This needs an "Args:" section.

daviddavid

What listens for this?

mike_conleymike_conley
hxqlin
hxqlin
hxqlin
hxqlin
hxqlin
david
  1. 
      
  2. 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));
    
  3. Show all issues

    This should have an "Args" section documenting what e is.

  4. Show all issues

    This needs an "Args:" section.

  5. 
      
hxqlin
mike_conley
  1. 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?

    1. 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).

  2. Show all issues

    This RB enables users,

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

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

  3. 
      
hxqlin
Review request changed
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.

mike_conley
  1. 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?

    1. 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.

  2. Show all issues

    What listens for this?

    1. 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.

    2. 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.

  3. 
      
mike_conley
  1. This looks good to me! Thank you!

  2.