1289170 - toolbar keyboard navigation works strangely

Status: RESOLVED FIXED

(DevTools :: Framework, defect, P3)

Description

[Tom Tromey :tromey]

As part of bug 1278473, I experimented a bit with keyboard navigation in the
toolbar.  This was added in bug 1242852.

I'm using Linux.  STR:

* Open devtools
* Move mouse over "Inspector" tab on toolbar
* Press right arrow
=> Focus moves to "Console" tab, good.
* Press left arrow once
=> Back to "Inspector", good.
* Press left arrow again
=> Focus disappears.  I was expecting it to move to the "Pick an element from the page"
   button; or, failing that, perhaps to somewhere else on the toolbar

* Press right arrow a number of times
=> Focus moves smoothly through toolbar tabs.  Good so far...
* ... but keep pressing it and, for me, it eventually disappears.

It works fine through the "normal" tabs: Inspector, Console, Debugger,
Style Editor, Canvas, Performance, Memory, Network Monitor.
Then it disappears.
If I keep pressing the right arrow key while the focus is gone, I
eventually see the focus reappear on the Toolbox Options button.
If I press right arrow again after this, it disappears again for good.


I was expecting this to either work a bit more like Tab, or to just cycle
through the various items on the toolbar.

Also, a similar toolbar in the inspector also allows arrow navigation, but
works differently.  With the top-level toolbar, moving focus between the
buttons with the arrow key has no other effect; whereas in the rule view,
moving between Rules/Computed/Animations/Forms using the arrow keys has
the side effect of displaying the content corresponding to that tab.
I don't know which is preferable but I do think they ought to work the same.

Comment 1

[Brian Grinstead [:bgrins]]

Yura, any opinions on the expected behavior here?

Additional context: we are hoping to remove and/or shim Services.focusManager.moveFocus to make it work without chrome privileges

Comment 2

[Yura Zenevich [:yzen] (please use "needinfo")]

(In reply to Brian Grinstead [:bgrins] from comment #1)
> Yura, any opinions on the expected behavior here?
> 
> Additional context: we are hoping to remove and/or shim
> Services.focusManager.moveFocus to make it work without chrome privileges

Yes, this is something I wanted to work on: moving from a model where toolbar controls are selected with focus to one where selection is handled with aria-activedescendant

Comment 3

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: Bug 1289170 - improving keyboard accessibility for dev tools tabbar.

Review commit: https://reviewboard.mozilla.org/r/68978/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/68978/

Comment 4

[Yura Zenevich [:yzen] (please use "needinfo")]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cbb96ef5158d

Comment 5

[Brian Grinstead [:bgrins]]

Comment on attachment 8777401 [details]
Bug 1289170 - improving keyboard accessibility for dev tools tabbar.

https://reviewboard.mozilla.org/r/68978/#review66080

This looks great! Fixes behavior, removes chrome API, and a simpler implementation

::: devtools/client/framework/toolbox.js:886
(Diff revision 1)
> -      if (controlID) {
> -        control = this.doc.getElementById(controlID);
> -      }
> -      if (rangeParent || !control) {
> -        // If range parent is present, the focused is moved within the toolbar,
> -        // simply updating aria-activedescendant. Or if aria-activedescendant is
> +   * results in navigating away from the tabbar container.
> +   * @param  {FocusEvent} event
> +   */
> +  _onTabbarFocus: function (event) {
> +    this.tabbarFocusableElms.forEach(elm =>
> +      elm.setAttribute("tabindex", event.target == elm ? "0" : "-1"));

Nit: ===

::: devtools/client/framework/toolbox.js:897
(Diff revision 1)
> +   * @param  {KeyboardEvent} event
> +   */
> +  _onTabbarArrowKeypress: function (event) {
> -      let { key, target } = event;
> +    let { key, target } = event;
> -      let win = this.win;
> -      let elm, type;
> +    let focusableElms = this.tabbarFocusableElms;
> +    let curIndex = focusableElms.indexOf(target);

What if curIndex is -1?  I imagine that could happen if an addon injects DOM or if we add a new non-button element but forget to change the selector.  I'm thinking we should be defensive in that case and return early without throwing

::: devtools/client/framework/toolbox.js:917
(Diff revision 1)
> -        // Ignore all other keys.
> -        return;
> +      return;
> -      }
> +    }
> +
> +    focusableElms.forEach(elm =>
> +      elm.setAttribute("tabindex", newTarget == elm ? "0" : "-1"));

Nit: ===

Comment 6

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 8777401 [details]
Bug 1289170 - improving keyboard accessibility for dev tools tabbar.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68978/diff/1-2/

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5d2e3f233977
improving keyboard accessibility for dev tools tabbar. r=bgrins

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5d2e3f233977