Closed Bug 1536521 Opened 5 months ago Closed 5 months ago

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)

defect

Tracking

()

VERIFIED FIXED
Firefox 68
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)

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:

  1. focus the Library button and hit enter or space to open the Library button's menu
  2. arrow down to the Bookmarks menu item
  3. right arrow to go to the Bookmarks sub-menu
  4. left arrow to get back to the main Library menu
  5. 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.

Flags: needinfo?(jteh)
Priority: -- → P2

This occurs because:

  1. This panel opens as a child of the toolbar.
  2. 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
  3. 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.

Flags: needinfo?(jteh) → needinfo?(mdeboer)
Duplicate of this bug: 1538639

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

This occurs because:

  1. This panel opens as a child of the toolbar.
  2. 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
  3. 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...).

Blocks: 1418973

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.:

  1. open a webpage
  2. click in url bar
  3. click on hamburger button (while url bar is still focused)
  4. hit down arrow.

I dunno if we really need to keep supporting this, though I guess it'd be nice if we could...

(Please forgive the comment spree...)

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

This occurs because:

  1. This panel opens as a child of the toolbar.
  2. 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
  3. 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.

Flags: needinfo?(mdeboer) → needinfo?(jteh)

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.

Assignee: nobody → jteh
Status: NEW → ASSIGNED
Flags: needinfo?(jteh)
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/1610f4d6f9a3
Use a capturing listener for PanelMultiView's keydown handler. r=Gijs

Jamie++ for the great documentation you added. Thanks!

Status: ASSIGNED → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 68

I'm guessing we want to uplift this, unless we intend to turn off the keyboard navigation stuff for 67 again?

Flags: needinfo?(jteh)

Verified fixed in Firefox 68.0a1 build 20190326214944 Windows 64 bit.

Flags: needinfo?(jteh)
Status: RESOLVED → VERIFIED

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:
Attachment #9053499 - Flags: approval-mozilla-beta?

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.

Attachment #9053499 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
https://hg.mozilla.org/projects/ash/rev/1610f4d6f9a38a7f0fcff07aac4f7d04e7069fe5
Bug 1536521: Use a capturing listener for PanelMultiView's keydown handler. r=Gijs
Flags: qe-verify+
QA Whiteboard: [qa-triaged]

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.

QA Whiteboard: [qa-triaged]
Flags: qe-verify+
No longer blocks: 1436086
Flags: in-testsuite+
Regressed by: 1436086
You need to log in before you can comment on or make changes to this bug.