1473748 - PanelMultiView.jsm keyboard navigation can potentially be simplified

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, enhancement, P1)

Description

[Johann Hofmann [:johannh]]

In bug 1467385 we had a few follow-up tasks/questions (which we did not further explore due to wanting to uplift the patch):

- Remove the "subviewkeynav" class again and just have a better way to figure out which elements are navigable (buttons, menuitems, etc.).
- Refactoring _getNavigableElements into a lazy getter that performs the initialization currently done by "keyNavigation"
- Figure out whether the setup done by "keyNavigation" is necessary

These may actually be connected in some ways, so I'm filing one bug to look into simplifying this code.

Comment 1

[:Paolo Amadini]

Attachment: Bug 1473748 - Part 1 - Use the type of element instead of the "subviewbutton" and "subviewkeynav" classes to initialize the list of navigable elements.

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

Comment 2

[:Paolo Amadini]

Attachment: Bug 1473748 - Part 2 - Simplify how navigable elements are initialized.

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

Comment 3

[:Paolo Amadini]

It seems that setting "tabindex" is still necessary to make at least the "toolbarbutton" elements tabbable and show the focus rings.

Comment 4

[:Gijs (he/him)]

Comment on attachment 8994154 [details]
Bug 1473748 - Part 1 - Use the type of element instead of the "subviewbutton" and "subviewkeynav" classes to initialize the list of navigable elements.

https://reviewboard.mozilla.org/r/258768/#review265710

::: browser/components/customizableui/PanelMultiView.jsm:1368
(Diff revision 1)
>     *
>     * @return {Array}
>     */
>    _getNavigableElements() {
>      let buttons = Array.from(this.node.querySelectorAll(
> -      ".subviewbutton:not([disabled]), .subviewkeynav:not([disabled])"));
> +      ":-moz-any(button,toolbarbutton,menulist,.text-link):not([disabled])"));

This is expensive. See the "Notes" section in https://developer.mozilla.org/en-US/docs/Web/CSS/:matches .

Comment 5

[:Gijs (he/him)]

Comment on attachment 8994155 [details]
Bug 1473748 - Part 2 - Simplify how navigable elements are initialized.

https://reviewboard.mozilla.org/r/258770/#review265714

::: browser/components/customizableui/test/browser_panel_keyboard_navigation.js:12
(Diff revision 1)
>  const {PanelView} = ChromeUtils.import("resource:///modules/PanelMultiView.jsm", {});
>  const kHelpButtonId = "appMenu-help-button";
>  
> +function getEnabledNavigableElementsForView(panelView) {
> +  return Array.from(panelView.querySelectorAll(
> +    "button,toolbarbutton,menulist,.text-link"

This doesn't check if elements are disabled, which suggests something is broken with elements that are disabled dynamically.

Comment 6

[:Paolo Amadini]

(In reply to :Gijs (he/him) from comment #5)
> > +  return Array.from(panelView.querySelectorAll(
> > +    "button,toolbarbutton,menulist,.text-link"
> 
> This doesn't check if elements are disabled, which suggests something is
> broken with elements that are disabled dynamically.

It's checked below in the filter function.

Note that things are indeed broken when elements are enabled and disabled dynamically, it's an existing issue. The problem is that the code that handles navigation doesn't take into account disabled elements after wrapping around the array. This is probably why the CSS selector excludes disabled items. I don't think this occurs anywhere in production though, and it's better to look into fixing this as part of bug 1477673, rather than spending time rewriting code that looks like is trying to mirror something we already have in the platform.

(In reply to :Gijs (he/him) from comment #4)
> > -      ".subviewbutton:not([disabled]), .subviewkeynav:not([disabled])"));
> > +      ":-moz-any(button,toolbarbutton,menulist,.text-link):not([disabled])"));
> 
> This is expensive. See the "Notes" section in
> https://developer.mozilla.org/en-US/docs/Web/CSS/:matches .

I'm aware that when processing a CSS file this selector would be put in the category of universal rules, but I'm not sure that this matters at all when querySelector is used, especially since this is a small DOM subtree. This code is also only used during keyboard navigation, not when the view is opened, so it may not be as performance sensitive.

Anyways, I'm not interested in micro-profiling this, so I'm fine with anything that the reviewer feels is a good solution. Speaking of which, I'll ask you for review since you've already made some comments and Johann is going away soon.

Comment 7

[:Gijs (he/him)]

Comment on attachment 8994154 [details]
Bug 1473748 - Part 1 - Use the type of element instead of the "subviewbutton" and "subviewkeynav" classes to initialize the list of navigable elements.

https://reviewboard.mozilla.org/r/258768/#review265750

r=me with the selector fixed to be less expensive.

::: browser/components/customizableui/PanelMultiView.jsm:1368
(Diff revision 1)
>     *
>     * @return {Array}
>     */
>    _getNavigableElements() {
>      let buttons = Array.from(this.node.querySelectorAll(
> -      ".subviewbutton:not([disabled]), .subviewkeynav:not([disabled])"));
> +      ":-moz-any(button,toolbarbutton,menulist,.text-link):not([disabled])"));

Well, seeing as we're already doing this in the test, and it's at least as readable, just use `"button,toolbarbutton,..."` as the selector and check for `[disabled]` in the filter function?

Comment 8

[:Gijs (he/him)]

Comment on attachment 8994155 [details]
Bug 1473748 - Part 2 - Simplify how navigable elements are initialized.

https://reviewboard.mozilla.org/r/258770/#review265746

It's a bit sad that this has gotten so muddled... I think ideally we shouldn't use this solution for the identity popup at all. We have no choice in the other menus because they contain <toolbarbuttons> (and kind of have to because they can be customized there from the toolbars), but the identity popup contains <button>s and they are tab-focusable and usable just fine - don't need any special code for that - and arrow-based navigation isn't expected and doesn't really make sense. There's already an attribute you can set on panels to disable keyboard navigation, and we could have just set that on the popup in question and avoided all of this.

::: browser/components/customizableui/PanelMultiView.jsm:1364
(Diff revision 1)
> -   * @return {Array}
> +   * This list is cached until the view is closed, so elements that become
> +   * enabled later may not be navigable.

To be fair, that's only the view, not the popup. Under what circumstances do we actually first disable items and then enable them *after* the view shows?

::: browser/components/customizableui/PanelMultiView.jsm:1408
(Diff revision 1)
> -    this.selectedElement = this._getNavigableElements()[0];
> +    this.selectedElement = this._navigableElements[0];
>      this.focusSelectedElement();

Why doesn't this just call keyNavigation with a fake event? That could have been done in bug 1467385...

Comment 9

[:Paolo Amadini]

(In reply to :Gijs (he/him) from comment #8)
> It's a bit sad that this has gotten so muddled... I think ideally we
> shouldn't use this solution for the identity popup at all. We have no choice
> in the other menus because they contain <toolbarbuttons> (and kind of have
> to because they can be customized there from the toolbars), but the identity
> popup contains <button>s and they are tab-focusable and usable just fine -
> don't need any special code for that - and arrow-based navigation isn't
> expected and doesn't really make sense. There's already an attribute you can
> set on panels to disable keyboard navigation, and we could have just set
> that on the popup in question and avoided all of this.

We have two <toolbarbuttons> in the Site Information panel, but I think setting "tabindex" on them might be enough to enable navigation with the TAB key. I agree we should reserve arrow navigation for panels that resemble menus.

> To be fair, that's only the view, not the popup. Under what circumstances do
> we actually first disable items and then enable them *after* the view shows?

I'm not aware of any at the moment. They would definitely behave incorrectly.

> Why doesn't this just call keyNavigation with a fake event? That could have
> been done in bug 1467385...

Sure, that would've been another way of implementing it.

Comment 10

[:Paolo Amadini]

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

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2f77b09b34fb
Part 1 - Use the type of element instead of the "subviewbutton" and "subviewkeynav" classes to initialize the list of navigable elements. r=johannh
https://hg.mozilla.org/integration/mozilla-inbound/rev/88f0905f8d8c
Part 2 - Simplify how navigable elements are initialized. r=johannh

Comment 12

[James Teh [:Jamie]]

(In reply to :Paolo Amadini from comment #9)
> We have two <toolbarbuttons> in the Site Information panel, but I think
> setting "tabindex" on them might be enough to enable navigation with the TAB
> key.

tabindex="0" isn't supported for XUL elements that aren't normally focusable. In my POC for bug 1436086, I had to use style -moz-user-focus: normal; to make toolbarbuttons focusable.

Comment 13

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/2f77b09b34fb
https://hg.mozilla.org/mozilla-central/rev/88f0905f8d8c