Allow basic keyboard navigation in panelmultiview subviews

VERIFIED FIXED in Firefox 55

Status

()

enhancement
P1
normal
VERIFIED FIXED
2 years ago
10 months ago

People

(Reporter: mikedeboer, Assigned: mikedeboer)

Tracking

(Depends on 1 bug, Blocks 4 bugs, {access})

Trunk
Firefox 55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 verified)

Details

(Whiteboard: [photon-structure])

Attachments

(1 attachment)

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.
Blocks: 1354146
Flags: qe-verify+
Priority: -- → P2
QA Contact: gwimberly
Whiteboard: [photon] → [photon-structure]
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Priority: P2 → P1
Iteration: --- → 55.5 - May 15
Iteration: 55.5 - May 15 → 55.6 - May 29
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...
Attachment #8867680 - Flags: review?(gijskruitbosch+bugs)
(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.
Flags: needinfo?(gijskruitbosch+bugs)
I can see how it works now.
Flags: needinfo?(gijskruitbosch+bugs)
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? :-)
Attachment #8867680 - Flags: review?(gijskruitbosch+bugs)
(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 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? :-)
Attachment #8867680 - Flags: review?(gijskruitbosch+bugs) → review+
Blocks: 1366205
Blocks: 1366207
Pushed by mdeboer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/585abbb3d188
add support for keyboard navigation inside panel views. r=Gijs
https://hg.mozilla.org/mozilla-central/rev/585abbb3d188
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Verified on Windows, Ubuntu, and Mac.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Depends on: 1378016
Keywords: access
Blocks: 1387512
Blocks: 1418973
No longer blocks: 1425972
Depends on: 1425972
Depends on: 1425981
Depends on: 1489974
You need to log in before you can comment on or make changes to this bug.