Add full keyboard interaction in new review request page

Review Request #10852 — Created Jan. 24, 2020 and updated

Information

Review Board
master
dd35797...

Reviewers

The new review request page before this change had a couple issues
when navigating the page through just the keyboard itself.
On Chrome for example, the user was able to tab through most input
fields exclusing the commitsView list and side bar. For Firefox,
and Safari, you could only tab through the repository filter search
bar, the select diff file, and branchesSelect view.

Revamped the keyboard usability experience within the new review
request page so that the user can fully tab through the page going
forwards and can shift+tab backwards excluding the repository selection
side bar to be done in a future project. The user will first tab
through to the repository search field in the side bar, the RBTools tip,
the select file diff button, the branchesSelect view dropdown, and
finally the commitsView list where the user can navigate through each
commit using the up/down/tab keys. Press enter to open the dialog to
create a new review request.

Note that you are unable to interact with the dialog if triggered by the
keyboard and currently the user can still tab through the commitsView
in the background even if the modal is open. A link is attached for
reference and will be fixed in a future request.

Manual tests:
- Full keyboard usability end to end from the repo search bar,
to RBTools tip, upload diff button, branchesView dropdown, and
finally to the commitsView on Chrome, Firefox, and Safari
- Tabbing in the commitsView, focusing on the correct commit
- Using the up/down arrow keys in the commitsView, focusing on
the correct commit
- Pressing the up key on the first commit brings focus back to the
branchesView
- A random combination of up/down/tab keys in commitsView,
move focus to an element outside the commitsView, and return focus
back to the first commit and all commits are navigated in order
with whatever up/down/tab key combination
- Shift+Tab full user flow starting from CommitsView going backwards

Unit Tests:
- All tests pass before these new changes and all tests still pass
after the addition of new tests
- Tabbing from the repo search bar in the side bar to RBTools tip link
- Tabbing from the RBTools tip to the upload diff button
- Tabbing from the upload diff button to branchesView
- Pressing the up key on the first commit brings focus back to the
branchesView
- Pressing the up/down arrow key presses within the commitsView making
sure we are on the correct commit
- Pressing the down/tab keys to get to the end of the commitsView list
making sure that focus is still within the view
- Shift+Tab for each element and that it can tab back to the previous
element


Description From Last Updated

This is great stuff, Monica. I loved seeing the demo. Something that stood out is that, due to the layout, …

chipx86chipx86

class names are usually kebab case and i think don't typically include view in them. also did you mean collection …

hxqlinhxqlin

iirc there's a constant defined for the enter key code already - i think $.ui.keyCode.ENTER ? also i believe which …

hxqlinhxqlin

not sure which key this corresponds to, but same comment above about using an already defined constant and possibly favoring …

hxqlinhxqlin

can you do this.views[this._currentElement].focus(); ? same thing as below

hxqlinhxqlin

since this is the class name of the view, i think $('.branchesSelectionView') could be replaced with this.$el

hxqlinhxqlin

same issue about which and using an already defined constant here. also, if this supports the tab key (keycode 9) …

hxqlinhxqlin

i think this._currentElement could be better named as this._activeCommitIndex or something similar; the name is kinda confusing. when i saw …

hxqlinhxqlin

same comment about keyCode and using a constant for the numeric value

hxqlinhxqlin

does this fn need a check to make sure .rb-c-search-field__input is actually the active element before moving focus to the …

hxqlinhxqlin

i think Handling should be lowercase to be consistent with the style in other tests

hxqlinhxqlin

i think CommitsView should be camel cased?

hxqlinhxqlin

you could put this into a beforeEach

hxqlinhxqlin

could you use toHaveBeenCalledWith(...) here to check the arguments?

hxqlinhxqlin

toHaveBeenCalledWith(...) ?

hxqlinhxqlin

toHaveBeenCalledWith(...) could be used here too i think

hxqlinhxqlin

toHaveBeenCalledWith(...) ?

hxqlinhxqlin

i think Handling should be lowercase

hxqlinhxqlin

could view.render(); and view.delegateEvents(); be put into a beforeEach?

hxqlinhxqlin

toHaveBeenCalledWith(...) ?

hxqlinhxqlin

toHaveBeenCalledWith(...) ?

hxqlinhxqlin

toHaveBeenCalledWith(...) ?

hxqlinhxqlin

maybe toHaveBeenCalledWith(...) here?

hxqlinhxqlin

same comment about which and using the ui constant

hxqlinhxqlin

same comment about possibly replacing $('.branchesSelectionView') with this.$el

hxqlinhxqlin

Do we need to stop propagation/prevent default here?

daviddavid

Add a blank line between this block and the next

daviddavid

Please wrap to 80 columns.

daviddavid

Add a blank line above this one.

daviddavid

Please wrap to 80 columns.

daviddavid

Does this work even though view is never attached to the DOM?

daviddavid

This should be written in the imperative mood ("Handle a keydown event")

daviddavid

Imperative mood.

daviddavid

Imperative mood.

daviddavid

We don't need the inner parens on this first part.

daviddavid

Let's align all of this with the opening (. We also don't need the inner parens: } else if (event.which …

daviddavid

Imperative mood.

daviddavid

Imperative mood.

daviddavid

Imperative mood.

daviddavid
bui1
bui1
bui1
bui1
bui1
bui1
bui1
bui1
bui1
bui1
bui1
kpatenio
  1. 
      
  2. Has this been manually tested with other browsers? (i.e. Safari, Firefox, Edge, etc.)

    1. Yes! It was testing with chrome, firefox, and safari. I'd like the mentors to chime in with testing with edge/IE as I'm also curious about that.

  3. 
      
hxqlin
  1. 
      
  2. Show all issues

    class names are usually kebab case and i think don't typically include view in them. also did you mean collection here and not selection? i might be misunderstanding that part but basically i think the class name should be branches-collection

  3. i was thinking about doing this for my work but i didn't get around to looking up how yet - glad i spotted it here!

  4. Show all issues

    iirc there's a constant defined for the enter key code already - i think $.ui.keyCode.ENTER ? also i believe which is preferred over keyCode in jQuery - https://stackoverflow.com/questions/4471582/keycode-vs-which

  5. Show all issues

    not sure which key this corresponds to, but same comment above about using an already defined constant and possibly favoring which instead of keyCode

  6. Show all issues

    can you do this.views[this._currentElement].focus(); ? same thing as below

  7. Show all issues

    since this is the class name of the view, i think $('.branchesSelectionView') could be replaced with this.$el

  8. Show all issues

    same issue about which and using an already defined constant here. also, if this supports the tab key (keycode 9) i think this should also support shift+tab to go in the opposite direction (same behaviour as up arrow key)

  9. Show all issues

    i think this._currentElement could be better named as this._activeCommitIndex or something similar; the name is kinda confusing. when i saw the constant defined at the top of the file, i thought it was referring to a DOM/jQuery node and not just the index of one

  10. Show all issues

    same comment about keyCode and using a constant for the numeric value

  11. Show all issues

    does this fn need a check to make sure .rb-c-search-field__input is actually the active element before moving focus to the rbtools tip?

  12. Show all issues

    i think Handling should be lowercase to be consistent with the style in other tests

  13. Show all issues

    i think CommitsView should be camel cased?

  14. Show all issues

    you could put this into a beforeEach

  15. oh yeah these were the ui key code constants i was referring to above! :D

  16. Show all issues

    could you use toHaveBeenCalledWith(...) here to check the arguments?

  17. Show all issues

    toHaveBeenCalledWith(...) ?

  18. Show all issues

    toHaveBeenCalledWith(...) could be used here too i think

  19. Show all issues

    toHaveBeenCalledWith(...) ?

  20. Show all issues

    i think Handling should be lowercase

  21. Show all issues

    could view.render(); and view.delegateEvents(); be put into a beforeEach?

  22. Show all issues

    toHaveBeenCalledWith(...) ?

  23. Show all issues

    toHaveBeenCalledWith(...) ?

  24. Show all issues

    toHaveBeenCalledWith(...) ?

  25. Show all issues

    maybe toHaveBeenCalledWith(...) here?

  26. Show all issues

    same comment about which and using the ui constant

  27. Show all issues

    same comment about possibly replacing $('.branchesSelectionView') with this.$el

  28. 
      
bui1
  1. 
      
  2. this.views[this._currentElement].$el.focus(); ended up working for me so we can treat that view as a jquery object but thanks for the suggestion!

  3. Attempted that but then I couldn't key press back up to the branch dropdown itself. I think it needs an explicit element to focus back to so I left it as $('.branches-collection').focus(); instead.

  4. Ah I didn't know that shift+tab was a thing haha thats a cool tidbit I learned today from you then :)

    Now that's something I'll implement for sure not just for this view but for the others as well

  5. This event will only occur if there is a keydown on the search field input itself specified by 'keydown .rb-c-search-field__input': '_onKeyDown',. Essentially this event happens when the element is in focus and the correct event occurs. The focus check happens implicitly I believe unless I'm interpreting this wrong. Could argue that this case would also apply to the other keydown handlers in the other files.

  6. I left it has pascal cased as the other describe test suites suggest this

  7. view.render() can be put in the beforeEach but not view.delegateEvents because the spies need to be setup first before this function is called. Essentially we want the spy wrapper callback to be called when the proper DOM event happens otherwise the tests will say the spy was not called.

  8. mentioned using $('.branches-collection') in the other comment

  9. 
      
bui1
chipx86
  1. 
      
  2. Show all issues

    This is great stuff, Monica. I loved seeing the demo.

    Something that stood out is that, due to the layout, the focus indicator the browser uses isn't super helpful. It just appears as a line of focus, and it's not super clear which thing it's supposed to be indicating.

    I think what we should do for the keyboard-based navigation is remove the focus outline (outline: 0) and instead set a background color (we can match the top bar's color maybe), so that it's very clear what item is currently the selected item.

  3. 
      
bui1
david
  1. 
      
  2. Show all issues

    Do we need to stop propagation/prevent default here?

  3. Show all issues

    Add a blank line between this block and the next

  4. Show all issues

    Please wrap to 80 columns.

  5. Show all issues

    Add a blank line above this one.

  6. Show all issues

    Please wrap to 80 columns.

  7. Show all issues

    Does this work even though view is never attached to the DOM?

    1. Correct this still does work. The unit tests itself pass and I can confirm with debugger breakpoints that code is being hit within the view.

  8. 
      
bui1
david
  1. I haven't actually tried running it, but this is looking really solid. Just a few style nits left.

  2. Show all issues

    This should be written in the imperative mood ("Handle a keydown event")

  3. Show all issues

    Imperative mood.

  4. Show all issues

    Imperative mood.

  5. Show all issues

    We don't need the inner parens on this first part.

  6. Show all issues

    Let's align all of this with the opening (. We also don't need the inner parens:

    } else if (event.which === $.ui.keyCode.DOWN ||
               event.which === $.ui.keyCode.TAB) {
    
  7. Show all issues

    Imperative mood.

  8. Show all issues

    Imperative mood.

  9. Show all issues

    Imperative mood.

  10. 
      
bui1
Review request changed