Add full keyboard interaction in new review request page
Review Request #10852 — Created Jan. 24, 2020 and updated
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 backwardsUnit 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, … |
|
|
class names are usually kebab case and i think don't typically include view in them. also did you mean collection … |
|
|
iirc there's a constant defined for the enter key code already - i think $.ui.keyCode.ENTER ? also i believe which … |
|
|
not sure which key this corresponds to, but same comment above about using an already defined constant and possibly favoring … |
|
|
can you do this.views[this._currentElement].focus(); ? same thing as below |
|
|
since this is the class name of the view, i think $('.branchesSelectionView') could be replaced with this.$el |
|
|
same issue about which and using an already defined constant here. also, if this supports the tab key (keycode 9) … |
|
|
i think this._currentElement could be better named as this._activeCommitIndex or something similar; the name is kinda confusing. when i saw … |
|
|
same comment about keyCode and using a constant for the numeric value |
|
|
does this fn need a check to make sure .rb-c-search-field__input is actually the active element before moving focus to the … |
|
|
i think Handling should be lowercase to be consistent with the style in other tests |
|
|
i think CommitsView should be camel cased? |
|
|
you could put this into a beforeEach |
|
|
could you use toHaveBeenCalledWith(...) here to check the arguments? |
|
|
toHaveBeenCalledWith(...) ? |
|
|
toHaveBeenCalledWith(...) could be used here too i think |
|
|
toHaveBeenCalledWith(...) ? |
|
|
i think Handling should be lowercase |
|
|
could view.render(); and view.delegateEvents(); be put into a beforeEach? |
|
|
toHaveBeenCalledWith(...) ? |
|
|
toHaveBeenCalledWith(...) ? |
|
|
toHaveBeenCalledWith(...) ? |
|
|
maybe toHaveBeenCalledWith(...) here? |
|
|
same comment about which and using the ui constant |
|
|
same comment about possibly replacing $('.branchesSelectionView') with this.$el |
|
|
Do we need to stop propagation/prevent default here? |
|
|
Add a blank line between this block and the next |
|
|
Please wrap to 80 columns. |
|
|
Add a blank line above this one. |
|
|
Please wrap to 80 columns. |
|
|
Does this work even though view is never attached to the DOM? |
|
|
This should be written in the imperative mood ("Handle a keydown event") |
|
|
Imperative mood. |
|
|
Imperative mood. |
|
|
We don't need the inner parens on this first part. |
|
|
Let's align all of this with the opening (. We also don't need the inner parens: } else if (event.which … |
|
|
Imperative mood. |
|
|
Imperative mood. |
|
|
Imperative mood. |
|

Description: |
|
---|

Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 2 (+39) |
Checks run (2 succeeded)

Summary: |
|
||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
||||||||||||||||||||||||
Commit: |
|
||||||||||||||||||||||||
Diff: |
Revision 3 (+122) |
Checks run (2 succeeded)

Description: |
|
||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Commit: |
|
||||||||||||||||||
Diff: |
Revision 4 (+115) |
Checks run (2 succeeded)

Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 5 (+117) |
Checks run (2 succeeded)

Description: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
Commit: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 6 (+261 -1) |
Checks run (2 succeeded)

Description: |
|
||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Commit: |
|
||||||||||||||||||
Diff: |
Revision 7 (+263 -1) |
Checks run (2 succeeded)

Summary: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Testing Done: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Commit: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 8 (+272 -1) |
Checks run (2 succeeded)

Checks run (2 succeeded)

Testing Done: |
|
---|
-
-
reviewboard/static/rb/js/newReviewRequest/views/branchesView.es6.js (Diff revision 9) class names are usually kebab case and i think don't typically include
view
in them. also did you meancollection
here and notselection
? i might be misunderstanding that part but basically i think the class name should bebranches-collection
-
reviewboard/static/rb/js/newReviewRequest/views/commitView.es6.js (Diff revision 9) 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!
-
reviewboard/static/rb/js/newReviewRequest/views/commitView.es6.js (Diff revision 9) iirc there's a constant defined for the enter key code already - i think
$.ui.keyCode.ENTER
? also i believewhich
is preferred overkeyCode
in jQuery - https://stackoverflow.com/questions/4471582/keycode-vs-which -
reviewboard/static/rb/js/newReviewRequest/views/commitsView.es6.js (Diff revision 9) not sure which key this corresponds to, but same comment above about using an already defined constant and possibly favoring
which
instead ofkeyCode
-
reviewboard/static/rb/js/newReviewRequest/views/commitsView.es6.js (Diff revision 9) can you do
this.views[this._currentElement].focus();
? same thing as below -
reviewboard/static/rb/js/newReviewRequest/views/commitsView.es6.js (Diff revision 9) since this is the class name of the view, i think
$('.branchesSelectionView')
could be replaced withthis.$el
-
reviewboard/static/rb/js/newReviewRequest/views/commitsView.es6.js (Diff revision 9) 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) -
reviewboard/static/rb/js/newReviewRequest/views/commitsView.es6.js (Diff revision 9) i think
this._currentElement
could be better named asthis._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 -
-
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? -
-
-
-
-
-
-
-
-
-
-
-
-
-
reviewboard/static/rb/js/newReviewRequest/views/tests/repositorySelectionViewTests.es6.js (Diff revision 9) maybe
toHaveBeenCalledWith(...)
here? -
reviewboard/static/rb/js/views/uploadDiffView.es6.js (Diff revision 9) same comment about
which
and using the ui constant -
reviewboard/static/rb/js/views/uploadDiffView.es6.js (Diff revision 9) same comment about possibly replacing
$('.branchesSelectionView')
withthis.$el

-
-
reviewboard/static/rb/js/newReviewRequest/views/commitsView.es6.js (Diff revision 9) 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! -
reviewboard/static/rb/js/newReviewRequest/views/commitsView.es6.js (Diff revision 9) 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. -
reviewboard/static/rb/js/newReviewRequest/views/commitsView.es6.js (Diff revision 9) 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
-
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 otherkeydown
handlers in the other files. -
-
view.render()
can be put in the beforeEach but notview.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. -
reviewboard/static/rb/js/views/uploadDiffView.es6.js (Diff revision 9) mentioned using
$('.branches-collection')
in the other comment

Description: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Commit: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 10 (+365 -1)
|
Checks run (2 succeeded)
-
-
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.

Commit: |
|
||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 11 (+370 -1)
|
||||||||||||||||||
Added Files: |
Checks run (2 succeeded)
-
-
reviewboard/static/rb/js/newReviewRequest/views/commitView.es6.js (Diff revision 11) Do we need to stop propagation/prevent default here?
-
reviewboard/static/rb/js/newReviewRequest/views/commitsView.es6.js (Diff revision 11) Add a blank line between this block and the next
-
reviewboard/static/rb/js/newReviewRequest/views/commitsView.es6.js (Diff revision 11) Please wrap to 80 columns.
-
reviewboard/static/rb/js/newReviewRequest/views/commitsView.es6.js (Diff revision 11) Add a blank line above this one.
-
reviewboard/static/rb/js/newReviewRequest/views/commitsView.es6.js (Diff revision 11) Please wrap to 80 columns.
-

Commit: |
|
||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 12 (+377 -1)
|
Checks run (2 succeeded)
-
I haven't actually tried running it, but this is looking really solid. Just a few style nits left.
-
reviewboard/static/rb/js/newReviewRequest/views/branchesView.es6.js (Diff revision 12) This should be written in the imperative mood ("Handle a keydown event")
-
reviewboard/static/rb/js/newReviewRequest/views/commitView.es6.js (Diff revision 12) Imperative mood.
-
reviewboard/static/rb/js/newReviewRequest/views/commitsView.es6.js (Diff revision 12) Imperative mood.
-
reviewboard/static/rb/js/newReviewRequest/views/commitsView.es6.js (Diff revision 12) We don't need the inner parens on this first part.
-
reviewboard/static/rb/js/newReviewRequest/views/commitsView.es6.js (Diff revision 12) 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) {
-
-
-

Commit: |
|
||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 13 (+377 -1)
|