Allow basic keyboard navigation in panelmultiview subviews

VERIFIED FIXED in Firefox 55

Status

()

enhancement
P1
normal
VERIFIED FIXED
2 years ago
7 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)

(Assignee)

Description

2 years ago
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.
(Assignee)

Updated

2 years ago
Blocks: 1354146
Flags: qe-verify+
Priority: -- → P2
QA Contact: gwimberly
Whiteboard: [photon] → [photon-structure]
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Priority: P2 → P1
Iteration: --- → 55.5 - May 15
Iteration: 55.5 - May 15 → 55.6 - May 29

Comment 2

2 years ago
mozreview-review
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)
(Assignee)

Comment 3

2 years ago
(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)
(Assignee)

Comment 4

2 years ago
I can see how it works now.
Flags: needinfo?(gijskruitbosch+bugs)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 7

2 years ago
mozreview-review
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)
(Assignee)

Comment 8

2 years ago
(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 hidden (mozreview-request)

Comment 10

2 years ago
mozreview-review
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+
(Assignee)

Updated

2 years ago
Blocks: 1366205
(Assignee)

Updated

2 years ago
Blocks: 1366207
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 13

2 years ago
Pushed by mdeboer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/585abbb3d188
add support for keyboard navigation inside panel views. r=Gijs

Comment 14

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/585abbb3d188
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55

Comment 15

2 years ago
Verified on Windows, Ubuntu, and Mac.
Status: RESOLVED → VERIFIED
Flags: qe-verify+

Updated

2 years ago
Depends on: 1378016
Keywords: access

Updated

2 years ago
Blocks: 1387512

Updated

a year ago
Blocks: 1418973
No longer blocks: 1425972
Depends on: 1425972
Depends on: 1425981

Updated

7 months ago
Depends on: 1489974
You need to log in before you can comment on or make changes to this bug.