keyboard navigating to a sub menu and back to the main menu breaks selection in main menu in toolbar menu buttons
Categories
(Firefox :: Keyboard Navigation, defect, P2)
Tracking
()
Tracking | Status | |
---|---|---|
firefox-esr60 | --- | unaffected |
firefox66 | --- | unaffected |
firefox67 | --- | verified |
firefox68 | --- | verified |
People
(Reporter: asa, Assigned: Jamie)
References
(Blocks 1 open bug, Regression)
Details
(Keywords: regression)
Attachments
(1 file)
47 bytes,
text/x-phabricator-request
|
pascalc
:
approval-mozilla-beta+
|
Details | Review |
Keyboard navigating to a sub menu with right arrow and back to the main menu breaks selection in main menu in toolbar menu buttons.
Steps to Reproduce:
- focus the Library button and hit enter or space to open the Library button's menu
- arrow down to the Bookmarks menu item
- right arrow to go to the Bookmarks sub-menu
- left arrow to get back to the main Library menu
- now arrow down and notice that the next item down the menu gets skipped.
Note: if you use enter instead of right arrow to get to the sub menu, everything seems to work.
Reporter | ||
Updated•6 years ago
|
Updated•6 years ago
|
Assignee | ||
Comment 1•6 years ago
|
||
This occurs because:
- This panel opens as a child of the toolbar.
- ToolbarKeyboardNavigator adds a keypress handler to the toolbar, but PanelMultiView adds a keydown handler to the window instead of the panel:
https://searchfox.org/mozilla-central/source/browser/components/customizableui/PanelMultiView.jsm#1117 - Because the PanelMultiView keydown listener is on an ancestor of the toolbar, ToolbarKeyboardNavigator gets there first.
Mike, do you recall (or have any idea) why the keydown handler is added to the window instead of the panel? (This was originally implemented by you in bug 1354144.) I tried changing it to the panel and it seems to work, but I might be missing some edge case.
Assignee | ||
Updated•6 years ago
|
Comment 3•6 years ago
|
||
(In reply to James Teh [:Jamie] from comment #1)
This occurs because:
- This panel opens as a child of the toolbar.
- ToolbarKeyboardNavigator adds a keypress handler to the toolbar, but PanelMultiView adds a keydown handler to the window instead of the panel:
https://searchfox.org/mozilla-central/source/browser/components/customizableui/PanelMultiView.jsm#1117- Because the PanelMultiView keydown listener is on an ancestor of the toolbar, ToolbarKeyboardNavigator gets there first.
Mike, do you recall (or have any idea) why the keydown handler is added to the window instead of the panel? (This was originally implemented by you in bug 1354144.) I tried changing it to the panel and it seems to work, but I might be missing some edge case.
Are the buttons inside the panel focusable now when they weren't at the time that patch landed? That might explain why - IIRC Paolo refactored some of this somewhat recently (I should really remember because I think I reviewed all of the patches we're talking about...).
Comment 4•6 years ago
|
||
Ah, so, it didn't use to be possible to open the menus with the keyboard. So focus would never enter the panel, so the listener needed to be outside of the panel in order for that to even happen. You can repro this by e.g.:
- open a webpage
- click in url bar
- click on hamburger button (while url bar is still focused)
- hit down arrow.
I dunno if we really need to keep supporting this, though I guess it'd be nice if we could...
Comment 5•6 years ago
|
||
(Please forgive the comment spree...)
(In reply to James Teh [:Jamie] from comment #1)
This occurs because:
- This panel opens as a child of the toolbar.
- ToolbarKeyboardNavigator adds a keypress handler to the toolbar, but PanelMultiView adds a keydown handler to the window instead of the panel:
https://searchfox.org/mozilla-central/source/browser/components/customizableui/PanelMultiView.jsm#1117- Because the PanelMultiView keydown listener is on an ancestor of the toolbar, ToolbarKeyboardNavigator gets there first.
We could maybe just make the panel handler capturing? Then any preventDefault/stopPropagation would prevent the toolbar one from interfering, and preserve the behavior from comment #4. Or we could decide we don't care about that case and just move the listener.
Assignee | ||
Comment 6•6 years ago
|
||
PanelMultiView adds the keydown handler on the window so that it handles key presses when a panel appears but doesn't get focus, as happens when a button to open a panel is clicked with the mouse.
However, this means the listener is on an ancestor of the panel, which means that handlers such as ToolbarKeyboardNavigator are deeper in the tree.
Previously, PanelMultiView used a bubbling (default) listener.
This meant that ToolbarKeyboardNavigator handled the event first, causing it to interfere when a panel opened within the toolbar; e.g. the Library menu.
To fix this, use a capturing listener for PanelMultiView so it gets the event first.
Updated•6 years ago
|
Comment 8•6 years ago
|
||
Jamie++ for the great documentation you added. Thanks!
Comment 9•6 years ago
|
||
bugherder |
Comment 10•6 years ago
|
||
I'm guessing we want to uplift this, unless we intend to turn off the keyboard navigation stuff for 67 again?
Assignee | ||
Comment 11•6 years ago
|
||
Verified fixed in Firefox 68.0a1 build 20190326214944 Windows 64 bit.
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 12•6 years ago
|
||
Comment on attachment 9053499 [details]
Bug 1536521: Use a capturing listener for PanelMultiView's keydown handler.
Beta/Release Uplift Approval Request
- Feature/Bug causing the regression: Bug 1436086.
- User impact if declined: Using the right/left arrows in panels attached to the toolbar (e.g. the Library menu) will cause strange behaviour such as items not being focused as expected.
- Is this code covered by automated tests?: Yes
- Has the fix been verified in Nightly?: Yes
- Needs manual test from QE?: No
- If yes, steps to reproduce:
- List of other uplifts needed: None
- Risk to taking this patch: Low
- Why is the change risky/not risky? (and alternatives if risky): Simple patch with clearly understood behaviour covered by automated tests.
- String changes made/needed:
Comment 13•6 years ago
|
||
Comment on attachment 9053499 [details]
Bug 1536521: Use a capturing listener for PanelMultiView's keydown handler.
Fix for a 67 keyboard navigation regression, well understood patch with tests and fix verified on Nightly, uplift approved for 67 beta 6, thanks.
Comment 14•6 years ago
|
||
bugherder uplift |
Comment 15•6 years ago
|
||
Updated•6 years ago
|
Updated•6 years ago
|
Comment 16•6 years ago
|
||
Hi, I managed to reproduce this issue in Firefox Beta 67.0b4 but it no longer occurs in 67.0b6 on Windows 10, Ubuntu and Mac OsX.
Updated•6 years ago
|
Updated•3 years ago
|
Description
•