1546633 - Can't open buttons with subviews in overflow panel

Status: RESOLVED FIXED

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

Description

[:Gijs (he/him)]

Looking for a regression window, but basically, str:

1. pin library button in the overflow menu
2. tab to overflow buton
3. hit space/enter
4. navigate to library button if necessary (with tab/arrow)
5. hit space/enter/right-arrow

ER:
open subview

AR:
no subview

Comment 1

[:Gijs (he/him)]

Looks like bug 1477673.

Comment 2

[James Teh [:Jamie]]

Curiously, this doesn't happen for the Forget button (panic-button), but does for the Library button.

Comment 3

[James Teh [:Jamie]]

Ug. I almost wish I hadn't opened this can of keyboard worms. :(

This didn't happen before bug 1477673 because we didn't use PMV key nav for the overflow panel before then.

I think this happens because:

1. library-button explicitly defines onmousedown and onkeypress events, but not oncommand. (I guess it needs to do mousedown to support drag, and because of that, it needed keypress to manage setting panel focus properly, since we're not relying on CustomizableUI to do that.)
2. PMV key nav (which runs on keydown, thus before library-button's keypress) simulates a command event and a click event. It does not simulate a mousedown event.

The intuitive fix is to change library-button's keypress handler to keydown so it runs before PMV keydown. Unfortunately, that would mean focus wouldn't be set properly when opening this from the overflow menu with the keyboard, since that needs to be handled within PMV key nav.

Another option is to change PanelMultiView.showSubView to take a boolean isKeyPress argument (or an event argument) and have PanelUI.showSubView pass that. This would be another way to trigger the initial focus behaviour for sub-views.

Gijs, thoughts?

Comment 4

[James Teh [:Jamie]]

Oh, a third option is to add an oncommand handler to library-button. However, I'm not sure how that would interact with the existing mousedown and keypress handlers. In particular, the command event handles keyup for space, not keydown or keypress, which might cause some pain; I ran into a bunch of trouble with that when working on toolbar key nav.

Comment 5

[James Teh [:Jamie]]

(In reply to James Teh [:Jamie] from comment #3)

Another option is to change PanelMultiView.showSubView to take a boolean isKeyPress argument (or an event argument) and have PanelUI.showSubView pass that. This would be another way to trigger the initial focus behaviour for sub-views.

I realised that only works if you change library-button's keypress handler to a keydown handler. And:

The intuitive fix is to change library-button's keypress handler to keydown so it runs before PMV keydown.

PMV keydown is a capturing handler, so it doesn't matter what we do, library-button's handlers will never run first.

Comment 6

[James Teh [:Jamie]]

Attachment: Bug 1546633: PanelMultiView: Send mousedown event when activating a button via the keyboard.

Previously, we sent a command event and a click event.
Normally, the command event executes the action, then the click event closes the menu.
However, in some cases (e.g. the Library button), there is no command event handler and the mousedown event executes the action instead.

Comment 7

[:Gijs (he/him)]

I've r+'d the patch so I think I can clear this, let me know if I missed something.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/041741ce1646
PanelMultiView: Send mousedown event when activating a button via the keyboard. r=Gijs

Comment 9

[Andrei Ciure[:aciure]]

Backed out 2 changesets (bug 1545766, bug 1546633) for causing browser_PanelMultiView_keyboard.js to perma fail

push that caused the backout: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel%2Crunnable&selectedJob=243835504&revision=041741ce164674137968c39aab0a9c80e84c99db

failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&selectedJob=243835504&searchStr=os%2Cx%2C10.10%2Cshippable%2Copt%2Cmochitests%2Ctest-macosx64-shippable%2Fopt-mochitest-browser-chrome-e10s-2%2Cm%28bc2%29

backout: https://hg.mozilla.org/integration/autoland/rev/47ea779dc66b06fdcc4a247fbe29bf9770cefa8b

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f6903c331545
PanelMultiView: Send mousedown event when activating a button via the keyboard. r=Gijs

Comment 11

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/f6903c331545