1354144 - Allow basic keyboard navigation in panelmultiview subviews

Status: VERIFIED FIXED

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

Description

[Mike de Boer [:mikedeboer]]

It should be possible to use the arrow keys to move up and down the list of menu items inside the subview that's currently active.

When the associated command of a selected item references another subview, hitting <ENTER> or the <R-ARROW> key will open that subview. In RTL-mode this should change to be the <L-ARROW> key.

Comment 1

[Mike de Boer [:mikedeboer]]

Attachment: Bug 1354144 - add support for keyboard navigation inside panel views.

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

Comment 2

[:Gijs (he/him)]

Comment on attachment 8867680 [details]
Bug 1354144 - add support for keyboard navigation inside panel views.

https://reviewboard.mozilla.org/r/139266/#review143058

Neat! The pseudolock approach is interesting, I think we should do that bit slightly differently, but otherwise this looks very promising.

Should we add the shortcut back (conditional on the pref) that we added in bug 881937 and then backed out?

::: browser/components/customizableui/PanelMultiView.jsm:213
(Diff revision 1)
> +  get _du() {
> +    if (this.__du)
> +      return this.__du;
> +    return this.__du = Cc["@mozilla.org/inspector/dom-utils;1"].getService(Ci.inIDOMUtils);
> +  }

So... I expect that in order to expose this correctly to a11y, we will either need to actually move focus, or set aria attributes to indicate the active item.

We could decide to do both that and the :hover pseudoclass stuff, or we could decide to use the added attributes (or :focus, as appropriate) for styling and get rid of the pseudo class locking.

Personally, I would prefer the latter, but it's up to you.

::: browser/components/customizableui/PanelMultiView.jsm:232
(Diff revision 1)
>        this.panelViews.currentView = panel;
>      else
>        this.__currentSubView = panel;
>      return panel;
>    }
> +  get _keyNavigationMap() {

Should this and the other getter self-delete/override? Or do we ever delete the map/`_du`?

::: browser/components/customizableui/PanelMultiView.jsm:734
(Diff revision 1)
> +    let navMap = this._keyNavigationMap.get(view);
> +    if (!navMap) {
> +      navMap = {};
> +      this._keyNavigationMap.set(view, {});
> +    }
> +    let buttons = this._getNavigableElements(view);

This feels like it should ideally be cached per view rather than re-fetched for every keypress. Is there a way to do so and clear the list when the view is hidden, maybe? Or is there a case I'm missing where the set of available buttons can change while the panel is up, maybe? I suppose, if you open the menu while a page is loading and that enables/disables e.g. the character encoding item...

::: browser/components/customizableui/PanelMultiView.jsm:743
(Diff revision 1)
> +    let stop = () => {
> +      event.stopPropagation();
> +      event.preventDefault();
> +    };
> +    let du = this._du;
> +    let idx;

Nit: more descriptive name please, maybe 'buttonIndex', or at least just 'index' ?

Also, we could potentially just declare this locally in the 2 blocks that need this?

::: browser/components/customizableui/PanelMultiView.jsm:783
(Diff revision 1)
> +            this.goBack(view.backButton);
> +          break;
> +        }
> +        // Fall-through...
> +      }
> +      case event.DOM_VK_RETURN: {

If we use focus to move between items, we get this for free, I believe.

::: browser/components/customizableui/PanelMultiView.jsm:790
(Diff revision 1)
> +        if (!buttons[idx])
> +          break;
> +        stop();
> +        du.removePseudoClassLock(buttons[idx], ":hover");
> +        delete navMap.selected;
> +        buttons[idx].click();

I don't think a 'forward' arrow should activate a button unless a subview is attached, especially not in the case of e.g. the `[cut][copy][paste]` buttons where the user could easily think that horizontal arrow keys would navigate between them.

In the case where this opens another subview, I think we should reset the navmap for that new subview to start at index 0. Would that make sense? I don't see code doing that off-hand...

::: browser/components/customizableui/PanelMultiView.jsm:814
(Diff revision 1)
> +
> +    let idx = navMap.selected;
> +    if (!buttons[idx])
> +      return;
> +    this._du.removePseudoClassLock(buttons[idx], ":hover");
> +    this._keyNavigationMap.clear();

You already do this 12 lines up...

Comment 3

[Mike de Boer [:mikedeboer]]

(In reply to :Gijs from comment #2)
> Should we add the shortcut back (conditional on the pref) that we added in
> bug 881937 and then backed out?

You're talking about 'just' the shortcut, or also the tabindex things?

> So... I expect that in order to expose this correctly to a11y, we will
> either need to actually move focus, or set aria attributes to indicate the
> active item.
> 
> We could decide to do both that and the :hover pseudoclass stuff, or we
> could decide to use the added attributes (or :focus, as appropriate) for
> styling and get rid of the pseudo class locking.
> 
> Personally, I would prefer the latter, but it's up to you.

Hmm, I was liking the pseudo-class locking, because it gives us quite a bit of control without needing to sprinkle selectors for it.
The bigger thing for me at the moment is that `toolbarbutton.focus()` does nothing interesting for me at the moment. There must be something I'm missing - is that the tabindex=0 thing?

> Should this and the other getter self-delete/override? Or do we ever delete
> the map/`_du`?

If I `delete this._du` right here, it'll throw and complain I need to add a setter for that. I can do that, but is more code than we have now.

> This feels like it should ideally be cached per view rather than re-fetched
> for every keypress. Is there a way to do so and clear the list when the view
> is hidden, maybe? Or is there a case I'm missing where the set of available
> buttons can change while the panel is up, maybe? I suppose, if you open the
> menu while a page is loading and that enables/disables e.g. the character
> encoding item...

No you're right, it's perfectly fine to cache them.

> If we use focus to move between items, we get this for free, I believe.

I tried moving focus to the toolbarbuttons, but it didn't work using `.focus()`. Should I use some nsIDOMWindowUtils trickery?

> I don't think a 'forward' arrow should activate a button unless a subview is
> attached, especially not in the case of e.g. the `[cut][copy][paste]`
> buttons where the user could easily think that horizontal arrow keys would
> navigate between them.
> 
> In the case where this opens another subview, I think we should reset the
> navmap for that new subview to start at index 0. Would that make sense? I
> don't see code doing that off-hand...

Agreed. The latter paragraph coincides with the caching nodes per view code.

Comment 4

[Mike de Boer [:mikedeboer]]

I can see how it works now.

Comment 5

[Mike de Boer [:mikedeboer]]

Comment on attachment 8867680 [details]
Bug 1354144 - add support for keyboard navigation inside panel views.

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

Comment 6

[Mike de Boer [:mikedeboer]]

Comment on attachment 8867680 [details]
Bug 1354144 - add support for keyboard navigation inside panel views.

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

Comment 7

[:Gijs (he/him)]

Comment on attachment 8867680 [details]
Bug 1354144 - add support for keyboard navigation inside panel views.

https://reviewboard.mozilla.org/r/139266/#review144040

I tried this out and there's a few bugs that I think we should fix...

1) if you open about:config, then click the menu panel, then hit down arrow, the selection of rows in about:config moves, too. I expect we need to call stopPropagation() and preventDefault() ?
2) if you open the menu panel, hit down arrow a few times, then hit *right* arrow a few times, the selection keeps moving down. But now if you hit up arrow, you go back to where you switched from down to right arrow. That seems wrong - not entirely sure what's causing that.
3) if you open the menu panel, hit down arrow once, then keep hitting right arrow, you will cycle through the entire menu but you never enter a submenu
4) if you go back from a subview to mainview, the selected item in there is lost. I think we should forget 'forward' selected items (so if you re-enter the same subview, you should start at the top again) but not the selected item in the 'previous' view.
5) my impression (and I haven't checked this carefully) is that we clear the selected item immediately when entering a subview, and I think this should behave the same way as clicks - keep showing the item as selected until the subview has been entered.

I wonder if we should keep the dashed arrow styling in addition to the :hover-like styling. Personally I think the :hover styling is sufficient, but perhaps you had a specific reason for doing that? :-)

Comment 8

[Mike de Boer [:mikedeboer]]

(In reply to :Gijs from comment #7)
> 4) if you go back from a subview to mainview, the selected item in there is
> lost. I think we should forget 'forward' selected items (so if you re-enter
> the same subview, you should start at the top again) but not the selected
> item in the 'previous' view.

That would be neat, BUT a potential bug pit: onviewshowing there's the potential that a view changes its contents when going from hidden to visible. It's highly unlikely, and in the case of the main menu it never happens, but clearing the entire map seems like the safest option for now.
In other words: how about making this follow-up fodder to get it to behave that way?

Comment 9

[Mike de Boer [:mikedeboer]]

Comment on attachment 8867680 [details]
Bug 1354144 - add support for keyboard navigation inside panel views.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139266/diff/3-4/

Comment 10

[:Gijs (he/him)]

Comment on attachment 8867680 [details]
Bug 1354144 - add support for keyboard navigation inside panel views.

https://reviewboard.mozilla.org/r/139266/#review144112

Great! Let's not forget the followup though. :-)

::: browser/components/customizableui/PanelMultiView.jsm:791
(Diff revision 4)
> +          if (view.getAttribute("title"))
> +            this.goBack(view.backButton);

Is this the getAttribute check there to check if going back makes sense (ie to check if this is the main view)? Can we use another check that is more obvious?

::: browser/components/customizableui/PanelMultiView.jsm:806
(Diff revision 4)
> +      case "Enter": {
> +        let button = buttons[navMap.selected];
> +        if (!button)
> +          break;
> +        stop();
> +        // Unfortunately, 'tabindex' doesn't not execute the default action, so

Negation for you! Negation for you! Negation for everyone!

</oprah>


;-)

::: browser/themes/shared/customizableui/panelUI.inc.css:15
(Diff revision 4)
>  % Basically, the 0.1px is there to avoid CSS rounding errors causing buttons to wrap.
>  % For gory details, refer to https://bugzilla.mozilla.org/show_bug.cgi?id=963365#c11
>  % There's no calc() here (and therefore lots of calc() where this is used) because
>  % we don't support nested calc(): https://bugzilla.mozilla.org/show_bug.cgi?id=968761
>  %define menuPanelButtonWidth (@menuPanelWidth@ / 3 - 0.1px)
> -%define buttonStateHover :not(:-moz-any([disabled],[open],:active)):hover
> +%define buttonStateHover :not(:-moz-any([disabled],[open],:active)):-moz-any(:hover,:focus)

OK, but now the default focus ring gets shown on the button... Can we hide that, given that we provide an alternative indication of where keyboard focus is? :-)

Comment 11

[Mike de Boer [:mikedeboer]]

Comment on attachment 8867680 [details]
Bug 1354144 - add support for keyboard navigation inside panel views.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139266/diff/4-5/

Comment 12

[Mike de Boer [:mikedeboer]]

Comment on attachment 8867680 [details]
Bug 1354144 - add support for keyboard navigation inside panel views.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139266/diff/5-6/

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/585abbb3d188
add support for keyboard navigation inside panel views. r=Gijs

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/585abbb3d188

Comment 15

[u554753]

Verified on Windows, Ubuntu, and Mac.