1975799 - IP Protection Panel cannot be accessed and focused on with the keyboard

Status: VERIFIED FIXED

(Firefox :: IP Protection, defect)

Description

[kpatenio]

Although we're still in the early stages of the panel, I'm noticing that I can't navigate from the toolbar to our panel with the keyboard. I was able to trace down the focus logic here and eventually here, but for some reason it can’t focus on this.selectedElement because it ends up being null.

There's also this code specifically after pressing the arrow or tab key. I get an error saying:

TypeError: can't access property "focus", button is null PanelMultiView.sys.mjs:1846:9
    keyNavigation resource:///modules/PanelMultiView.sys.mjs:1846
    handleEvent resource:///modules/PanelMultiView.sys.mjs:1230

which links to this line.

Comment 1

[Fred Chasen [:fchasen]]

I think the issue here is our custom element's shadow dom, which the treewalker doesn't handle in https://searchfox.org/mozilla-central/rev/96b60eb2246121c09203eaba74f9b3e8f626f44c/browser/components/customizableui/PanelMultiView.sys.mjs#1573

There is a deep tree walker that can handle shadow dom, but will need to see if it's a simple replacement.

• https://bugzilla.mozilla.org/show_bug.cgi?id=1834968
• https://searchfox.org/mozilla-central/rev/ada05ac4faf1f79944a35a9eb79083a684a98975/devtools/server/actors/inspector/document-walker.js#58-65

Comment 2

[Fred Chasen [:fchasen]]

Attachment: Bug 1975799 - Allow PanelMultiView TreeWalker filter to accept elements with shadowRoots that delegate focus. r=#ip-protection-reviewers,#recomp-reviewers

Comment 3

[James Teh [:Jamie]]

I recognise that this is in early stages, but this would be an access s2 if this were to ship, so I'm triaging it as such. If there is no clear plan to ship this any time soon, you can use the no-plan-to-ship keyword to prevent this from showing up on s2 dashboards, bot nags, etc., but note that this keyword should be removed once there is a clear plan to ship this.

Comment 4

[BugBot [:suhaib / :marco/ :calixte]]

The severity field for this bug is set to S3. However, the accessibility severity is higher, .
:fchasen, could you consider increasing the severity?

For more information, please visit BugBot documentation.

Comment 5

[Fred Chasen [:fchasen]]

This isn't shipping yet so I am going to leave the severity here the same but I also think the accessibility level is warranted (and this goes beyond our specific elements).

I put up a patch that should solve the issues with navigation in the IP Protection panel, but also tries to be as general as possible for custom elements. It updates the TreeWalker filter to accept elements that have a shadowDom that is set to delagateFocus allowing focus to enter into the shadowDom even if the TreeWalker can not.

Since our content element has several interactive elements that should get focus, I also needed to add a captureFocus attribute to keep the key events in the element instead of being handled by PanelMultiView. (But with this set focus can't leave that element unless there is another element in the panel that doesn't have tabindex set as -1)

:Jamie, I wanted to see if you had any context about why we are setting most of the nodes that TreeWalker accepts to have a tabindex of -1 in https://searchfox.org/mozilla-central/source/browser/components/customizableui/PanelMultiView.sys.mjs#1608 ?

I think the focus capturing attribute wouldn't be needed if this wasn't set with focus just exiting the an elements shadowDom to the next element as needed.

Comment 6

[James Teh [:Jamie]]

PanelMultiView keyboard navigation has a few fundamental requiements:

1. It needs to support toolbarbuttons and the like which aren't focusable by default. At the time, setting tabindex explicitly was much simpler than the more risky change of making toolbarbuttons focusable and the more complicated change of making all the individual toolbarbuttons in all the relevant panels focusable.
2. It needs to support arrow key navigation (for panels which are more like menus such as the Firefox hamburger menu and other similar menus), as well as tab key navigation. However, the set of elements reachable with the arrow keys needs to be different from the set of elements reachable with the tab key. This is because the arrow keys can't be allowed to enter text boxes, combo boxes, etc., since otherwise, the user could land in one of these and wouldn't be able to get out with the arrow keys, making arrow key navigation asymmetric. This is why we set tabindex="-1" and handle the keys (arrow keys, tab, etc.) directly, rather than relying on Gecko's default tab key handling.
3. It needs to handle correct auto focusing when a panel appears, when a sub-panel is opened, etc., rather than each panel needing to implement this itself.
4. Eventually, we probably want to support jumping to panel items reachable with the arrow keys by typing the first letter (or few letters) of the item's name. The current lack of this functionality makes our hamburger menus very inefficient for keyboard users. Potential features like this are another reason the PanelMultiView code "manages" focus itself, rather than relying on Gecko.

As I understand it (I haven't dug into or tested your patch), your patch will allow a user to move focus inside a custom element. However, once there, PanelMultiView won't be able to handle key presses any more. This means that arrow key navigation (2) and possibly other things in future (4) won't work. My other concern is that a user might move into the custom element with the arrow keys, but thereafter, the arrow keys will stop working, making arrow key navigation asymmetric. Unless you're explicitly only allowing entry into these elements with the tab key?

Anyway, since we explicitly manage focus (per 2), we could set tabindex="0" instead of "-1" if that helps. While PanelMultiView is managing focus with its key handler, this won't make any difference; we just set -1 because setting 0 might lead to the incorrect perception that we're allowing DOM to handle tab key navigation. But I'm a bit concerned about this approach (particularly long term), which is why I think panelMultiView ultimately needs to be able to handle elements inside shadow DOM itself as suggested in bug 1834968.

Comment 7

[Fred Chasen [:fchasen]]

Thanks :Jamie for that context, that all makes sense now.

I’ve updated the patch based on using tabindex 0 which simplifies things by allowing focus to escape from an elements shadow dom on tab (but not from a panel view).

This will by default only support tab and using the arrow keys to navigate to an element that delegates focus. This works well for most of the reusable components that only have a single focusable interactive element in them or use slots.

For larger components, like the IP Protection one that contains all the UI for a panel body, tab works correctly for multiple interactive elements this way but as you mentioned there is no up/down arrow support within the element. I was thinking that is probably ok, as that UI isn’t menu like.

For components that should be navigated with arrows, setting tabNavigationOnly to explicitly be “false” will let the component handle the arrow up and down events by itself. I’ll need to make an example of this but using the arrow keys to move the Services.focus forwards and backwards worked well in my testing. (But handling could be as complex as needed for a component and will have to handle skipping tab only elements itself unfortunately)

I had looked into using the deep tree walker linked to reach these elements and that seems like a good approach but I found some issues that made me want to handle it this way for now:

• There is no filter for that walker so we’d have to recreate much of the TreeWalker functionality. The dev tools implementation has most of it but not all.
• Custom elements are likely to use the moz input lit elements or other components so the actual input can be nested deeper than the walker would go. We’d still have to add all those elements to be accepted or have an attribute to identify them.

Comment 8

[Fred Chasen [:fchasen]]

Attachment: Bug 1975799 - Capture focus events in the IP Protection panel to allow keyboard navigation. r=#ip-protection-reviewers

• Updates the PanelMultiView TreeWalker filter to accept elements with data-captures-focus="true" and handle those elements like iframes, delagting focus handling to the element.
• Captures focus for ipprotection-content and adds a focus method to make sure the connection toggle is the first element focused.
• Adds a key handler in ipprotection-content to allow arrow navigation.
• Delegate focus in ipprotection-signedout to allow focusing its contents.

Comment 9

[Fred Chasen [:fchasen]]

Trying to make the solution generic was making this much more confusing, so will continue that in https://bugzilla.mozilla.org/show_bug.cgi?id=1831348 instead.

For this panel we are:

• Moving from having separate header and body panel elements to a single element in https://bugzilla.mozilla.org/show_bug.cgi?id=1978486
• Adding a data attribute to make PanelMultiView handle an element like an iframe and delegate focus event to that
• Adding arrow key handling to our custom element

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/d4a826a44cd0
https://hg.mozilla.org/integration/autoland/rev/50b17f073d7f
Capture focus events in the IP Protection panel to allow keyboard navigation. r=ip-protection-reviewers,rking,kpatenio

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/14d7dd148e0a
https://hg.mozilla.org/integration/autoland/rev/32c00062882c
Revert "Bug 1975799 - Capture focus events in the IP Protection panel to allow keyboard navigation. r=ip-protection-reviewers,rking,kpatenio" for causing bc failures @ browser_ipprotection_keyboard_navigation.js

Comment 12

[Sandor Molnar[:smolnar]]

Backed out for causing bc failures @ browser_ipprotection_keyboard_navigation.js

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

Push with failures

Failure log -> TEST-UNEXPECTED-FAIL | browser/components/ipprotection/tests/browser/browser_ipprotection_keyboard_navigation.js

INFO - TEST-PASS | browser/components/ipprotection/tests/browser/browser_ipprotection_keyboard_navigation.js | vpn-support-link focused after Tab pressed - 
[task 2025-07-24T17:29:44.804+00:00] 17:29:44     INFO - Waiting for focus on upgrade-vpn-button
[task 2025-07-24T17:29:44.804+00:00] 17:29:44     INFO - Buffered messages finished
[task 2025-07-24T17:29:44.805+00:00] 17:29:44     INFO - TEST-UNEXPECTED-FAIL | browser/components/ipprotection/tests/browser/browser_ipprotection_keyboard_navigation.js | Test timed out - 
[task 2025-07-24T17:29:44.805+00:00] 17:29:44     INFO - GECKO(6663) | Completed ShutdownLeaks collections in process 6663
[task 2025-07-24T17:29:44.806+00:00] 17:29:44     INFO - TEST-START | Shutdown
[task 2025-07-24T17:29:44.809+00:00] 17:29:44     INFO - Browser Chrome Test Summary
[task 2025-07-24T17:29:44.809+00:00] 17:29:44     INFO - Passed:  32
[task 2025-07-24T17:29:44.809+00:00] 17:29:44     INFO - Failed:  1
[task 2025-07-24T17:29:44.809+00:00] 17:29:44     INFO - Todo:    0
[task 2025-07-24T17:29:44.809+00:00] 17:29:44     INFO - Mode:    e10s
[task 2025-07-24T17:29:44.809+00:00] 17:29:44     INFO - *** End BrowserChrome Test Results ***
[task 2025-07-24T17:29:44.809+00:00] 17:29:44     INFO - GECKO(6663) | Exiting due to channel error.
[task 2025-07-24T17:29:44.809+00:00] 17:29:44     INFO - GECKO(6663) | Exiting due to channel error.
[task 2025-07-24T17:29:44.810+00:00] 17:29:44     INFO - GECKO(6663) | Exiting due to channel error.
[task 2025-07-24T17:29:44.810+00:00] 17:29:44     INFO - GECKO(6663) | Exiting due to channel error.
[task 2025-07-24T17:29:44.811+00:00] 17:29:44     INFO - GECKO(6663) | Exiting due to channel error.
[task 2025-07-24T17:29:44.811+00:00] 17:29:44     INFO - GECKO(6663) | Exiting due to channel error.
[task 2025-07-24T17:29:44.812+00:00] 17:29:44     INFO - GECKO(6663) | Exiting due to channel error.
[task 2025-07-24T17:29:44.812+00:00] 17:29:44     INFO - TEST-INFO | Main app process: exit 0
[task 2025-07-24T17:29:44.812+00:00] 17:29:44     INFO - TEST-UNEXPECTED-FAIL | browser/components/ipprotection/tests/browser/browser_ipprotection_keyboard_navigation.js | Application shut down (without crashing) in the middle of a test!
[task 2025-07-24T17:29:44.812+00:00] 17:29:44     INFO - TEST-INFO took 44797ms
[task 2025-07-24T17:29:44.813+00:00] 17:29:44     INFO - runtests.py | Application ran for: 0:00:50.565732
[task 2025-07-24T17:29:44.813+00:00] 17:29:44     INFO - zombiecheck | Reading PID log: /tmp/tmpinb1dkikpidlog
[task 2025-07-24T17:29:44.813+00:00] 17:29:44     INFO - ==> process 6663 launched child process 6745
[task 2025-07-24T17:29:44.814+00:00] 17:29:44     INFO - zombiecheck | Checking for orphan process with PID: 6745
[task 2025-07-24T17:29:44.814+00:00] 17:29:44     INFO - Stopping web server
[task 2025-07-24T17:29:44.814+00:00] 17:29:44     INFO - Server shut down.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/618aa771139e
https://hg.mozilla.org/integration/autoland/rev/4c749838be35
Capture focus events in the IP Protection panel to allow keyboard navigation. r=ip-protection-reviewers,rking,kpatenio

Comment 14

[Cristina Horotan [:chorotan]]

https://hg.mozilla.org/mozilla-central/rev/4c749838be35

Comment 15

[Valentina Virlics [:vvalentina], Ecosystem QA]

I have verified that this issue is no longer reproducible using the latest Firefox Nightly 144.0a1 and Firefox Beta 143.0b1, installed on Windows 11 x64.
Accessing and navigation the/in IP connection panel using the keyboard is possible. Each element can be selected and opended including turning the VPN ON/OFF.
It is worth mentioning that the "?" Help option is focused last, after all elements from the panel. Is this an issue? Should we track it separately?

Comment 16

[Fred Chasen [:fchasen]]

:vvalentina Sorry for not mentioning this earlier, the help option focusing last is the intended behavior so that we can focus the toggle initially.

Comment 17

[Valentina Virlics [:vvalentina], Ecosystem QA]

Based on above comments and verifications we can mark this verified-fixed.