Fix PanelMultiView keyboard navigation to better handle dynamic updates, etc.

RESOLVED FIXED in Firefox 68

Status

()

enhancement
P3
normal
RESOLVED FIXED
11 months ago
2 months ago

People

(Reporter: Paolo, Assigned: Jamie)

Tracking

(Blocks 2 bugs)

unspecified
Firefox 68
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox68 fixed)

Details

Attachments

(1 attachment)

Reporter

Description

11 months ago
Keyboard navigation in PanelMultiView uses a custom mechanism that creates a static list of navigable elements when the first navigation key is pressed. This means that elements that are initially disabled and get enabled later may not become navigable at the same time.

It might be possible to use a mechanism similar to usual TAB key navigation instead, so the static list becomes unnecessary and dynamic updates are considered.

Comment 1

11 months ago
I'm a bit confused, because I'm pretty sure this used to work. Did it get broken?

(I'm also surprised that the dep bug is in "Menus", and this isn't, but OK...)
Flags: needinfo?(paolo.mozmail)
Priority: -- → P3
Reporter

Comment 2

11 months ago
I think this has always been broken in the way I mentioned in bug 1473748 comment 6.
Flags: needinfo?(paolo.mozmail)
Assignee

Comment 3

4 months ago

The problem with standard focus handling is that it doesn't handle the arrow keys in the way we want. We could just override the arrow keys to advance focus in the right direction, but that wouldn't solve the problem of getting trapped in the search box when it is added to the overflow menu or getting trapped in the permission menulist in the Site Identity panel. We also override the arrow keys even when a menulist has focus; see bug 1522092.

Instead of using a static list that is generated once, I propose to use a TreeWalker similar to that used for toolbar keyboard navigation (see bug 1436086). This will handle dynamic updates correctly, as well as simplifying the code which deals with buttons that are disabled during navigation. (I'm pretty sure the code that deals with this currently is broken in multiple ways; e.g. it only searches for toolbarbuttons, but there can be other selectable controls now.)

In addition:

  1. The arrow keys (but not tab) will skip over menulists and text boxes. The user will need to use tab to access these. This still allows the panel to feel like a menu where appropriate without the risk of the user getting trapped should they use the arrow keys where such a control is present.
  2. The arrow keys will not be overridden in menulists/text boxes.

I already have a patch for this, though it needs some polish and tests.

Gijs, Johann, I'd really appreciate your thoughts on this.

Flags: needinfo?(jhofmann)
Flags: needinfo?(gijskruitbosch+bugs)

Comment 4

4 months ago

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

The problem with standard focus handling is that it doesn't handle the arrow keys in the way we want. We could just override the arrow keys to advance focus in the right direction, but that wouldn't solve the problem of getting trapped in the search box when it is added to the overflow menu or getting trapped in the permission menulist in the Site Identity panel. We also override the arrow keys even when a menulist has focus; see bug 1522092.

Instead of using a static list that is generated once, I propose to use a TreeWalker similar to that used for toolbar keyboard navigation (see bug 1436086). This will handle dynamic updates correctly, as well as simplifying the code which deals with buttons that are disabled during navigation. (I'm pretty sure the code that deals with this currently is broken in multiple ways; e.g. it only searches for toolbarbuttons, but there can be other selectable controls now.)

In addition:

  1. The arrow keys (but not tab) will skip over menulists and text boxes. The user will need to use tab to access these. This still allows the panel to feel like a menu where appropriate without the risk of the user getting trapped should they use the arrow keys where such a control is present.
  2. The arrow keys will not be overridden in menulists/text boxes.

I already have a patch for this, though it needs some polish and tests.

Gijs, Johann, I'd really appreciate your thoughts on this.

I'm not super keen on hand-writing yet another set of traversal code, but if there's no better way, sure.

Note that in terms of the arrow keys, the way macOS deals with this in the "Help" menu (which has a search control right at the top) is to just make up/down arrow move to the next/previous item, instead of navigating in the text input. I think that's a legitimate route we could take here, too, but I don't know if that's harder for reasons I haven't considered.

Flags: needinfo?(gijskruitbosch+bugs)
Assignee

Comment 5

4 months ago

(In reply to :Gijs (he/him) from comment #4)

Note that in terms of the arrow keys, the way macOS deals with this in the "Help" menu (which has a search control right at the top) is to just make up/down arrow move to the next/previous item, instead of navigating in the text input. I think that's a legitimate route we could take here, too, but I don't know if that's harder for reasons I haven't considered.

I think the up/down arrows might navigate suggestions in the search box? That's certainly an option, though it wouldn't solve the problem for menulists. That said, it's reasonable to keep the Identity Panel as something that's only tabbable if we think menulists will never end up in some menu-like panel.

Comment 6

4 months ago

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

I think the up/down arrows might navigate suggestions in the search box?

Ah, hm, yeah, I forgot about that. Then this likely doesn't really work, so let's go with your suggestion...

One thing I should mention is that UX is currently doing a (not so) slight redesign of the control center that will make it look and behave much more menu-like, i.e. much more similar to the main menu (but it will still have the menulists, duh).

Other than that I agree with Gijs on this:

I'm not super keen on hand-writing yet another set of traversal code, but if there's no better way, sure.

Thanks :)

Flags: needinfo?(jhofmann)
Assignee

Comment 8

3 months ago

Adjusting summary to reflect that standard focus handling might not be the approach we end up taking here, as per comment 3.

Summary: Use standard focus handling in PanelMultiView keyboard navigation → Fix handling of dynamic updates in PanelMultiView keyboard navigation
Assignee

Updated

3 months ago
Blocks: 1539984
Assignee

Updated

3 months ago
Blocks: 1539976
Assignee

Updated

3 months ago
Blocks: 1489874
Assignee

Updated

3 months ago
Assignee: nobody → jteh
Status: NEW → ASSIGNED
Assignee

Updated

3 months ago
Summary: Fix handling of dynamic updates in PanelMultiView keyboard navigation → Fix PanelMultiView keyboard navigation to better handle dynamic updates, etc.
Assignee

Comment 9

3 months ago

Previously, this code cached a list of controls on first use and used that for navigation.
This refactor addresses several issues:

  1. There is now a separate focus order for tab/shift+tab and down/up arrows.
    This allows menulists, textboxes, etc. which use the arrow keys themselves to be focused with tab, but skipped with the arrows.
    This means the user won't fall into these controls when using the up/down arrow keys and be confused by the subsequent arrowing behaviour.

  2. When a menulist, textbox, etc. is focused, the arrow keys, space and enter are now passed to the control.
    This is a better fix for handling of the arrow keys by menulists (bug 1522092).
    It also fixes left arrow in a textarea moving to the previous view instead of moving the caret (bug 1489874).

  3. This improves handling of dynamic updates to the panel.
    For example, elements that are initially disabled and enabled later will be navigable.
    This is because the next element is determined dynamically by the TreeWalker, rather than using a cached list.

  4. The interim fix for bug 1522092 disabled PanelMultiView keyboard navigation.
    This caused some regressions, including arrow keys/activation on the Site Identity Report a Problem link (bug 1539976) and some controls not being navigable if the Site Identity panel is opened using the mouse (bug 1539984).
    With the above fixes, we can now re-enable PanelMultiView keyboard navigation in the Site identity panel and thus fix these regressions.

  5. Previously, PanelMultiView keyboard navigation was disabled in the main toolbar overflow menu.
    This is because the search box can be added to the overflow menu, which previously caused problems for the arrow keys.
    With the above fixes, we can now safely enable PanelMultiView keyboard navigation in the overflow menu.

  6. PanelMultiView keyboard tests have been added.
    Previously, we relied on tests specific to various panels to exercise this functionality.

Assignee

Updated

3 months ago
Blocks: 1454865
Assignee

Updated

3 months ago
Blocks: 1541787
Assignee

Updated

3 months ago
Blocks: 1542975

Comment 10

3 months ago
Pushed by jteh@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/69db665d8263
Refactor PanelMultiView keyboard navigation to use a TreeWalker. r=Gijs,johannh

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&revision=eca8a6e641c0248dc064050b2767d87aa9f1b4c6&selectedJob=239024893

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=239024893&repo=autoland&lineNumber=14642
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=239019743&repo=autoland&lineNumber=7110
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=239035167&repo=autoland&lineNumber=4156

Backout link: https://hg.mozilla.org/integration/autoland/rev/a8e6fd651ef20b6940782870c2764a66949a542e

11:11:25 INFO - TEST-PASS | browser/components/customizableui/test/browser_PanelMultiView_keyboard.js | menulist still focused after ArrowDown -
11:11:25 INFO - Buffered messages finished
11:11:25 INFO - TEST-UNEXPECTED-FAIL | browser/components/customizableui/test/browser_PanelMultiView_keyboard.js | menulist value 2 after ArrowDown - Got 1, expected 2
11:11:25 INFO - Stack trace:
11:11:25 INFO - chrome://mochikit/content/browser-test.js:test_is:1325
11:11:25 INFO - chrome://mochitests/content/browser/browser/components/customizableui/test/browser_PanelMultiView_keyboard.js:testArrowsMenulist:184
11:11:25 INFO - chrome://mochikit/content/browser-test.js:Tester_execTest/<:1116
11:11:25 INFO - chrome://mochikit/content/browser-test.js:Tester_execTest:1144
11:11:25 INFO - chrome://mochikit/content/browser-test.js:nextTest/<:1005
11:11:25 INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<:803

Flags: needinfo?(jteh)

Comment 12

2 months ago
Pushed by jteh@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8329ed3a30b9
Refactor PanelMultiView keyboard navigation to use a TreeWalker. r=Gijs,johannh
Assignee

Updated

2 months ago
Flags: needinfo?(jteh)

Comment 13

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

Updated

2 months ago
Regressions: 1545766

Updated

2 months ago
Regressions: 1546633

Updated

2 months ago
Duplicate of this bug: 1546602
You need to log in before you can comment on or make changes to this bug.