1899741 - Allow focus to move into and out of feature callouts

Status: RESOLVED WONTFIX

(Firefox :: Messaging System, defect, P1)

Description

[Contact team at [e-mail]]

Attachment: bugESC.mp4

[Affected versions]:

• Firefox build version >=123

[Affected Platforms]:

• Windows 10 x64
• macOS Ventura 13.6.2
• Ubuntu 22.04 x64

[Prerequisites]:

• Have a Firefox build version >=123.
• Have a browser client enrolled in a Nimbus experiment that enables the “Discovery Search” message:
  https://experimenter.services.mozilla.com/nimbus/discovery-of-search-sap-early-day-en/summary

[Steps to reproduce]:

1. Open the browser from the prerequisites
2. Open a new tab.
3. Open a new tab.
4. Close the message using the “ESC” key.

[Expected result]:

• The message is closed using the “ESC” key when there is more than one opened tab

[Actual result]:

• The message cannot be closed using the “ESC” key when there is more than one opened tab

[Notes]:

• A video of this issue is attached.
• We noticed that this bug is related to the fact that the “Discovery Search” message cannot be focused when there is more than one opened tab.
• This issue is occurring for early day users and existing users, too, and for all the branches.

Comment 2

[Shane Hughes [:aminomancer]]

I guess the real issue here is that you can't tab into the callout. It's expected that the callout must be focused for the Escape key to close it. But it's not desired for it to be impossible to tab into or out of the callout, since that makes it impossible for keyboard-only users to close the callout if it somehow loses focus. (I suppose it's unlikely for it to lose focus for a keyboard-only user, though, since focus also gets trapped within the callout panel. Still, not optimal for accessibility.)

This might have always been the case since we adopted panels, or the issue might be that I made the callout panel unfocusable (removing tabIndex) in bug 1898346, because we didn't like having a non-interactive element focusable, especially the focus ring. The content inside the panel should be focusable, but I guess that can't happen for some reason. Focus also gets trapped in the callout.

It's like the panel has its own separate focus context, but I'm not sure where that is implemented. I know most panels in the browser use TreeWalkers to handle focus, but this particular panel does not use panelUI or PanelMultiView. It's just a <panel> created over here.

It can be tested like this:

1. Set browser.newtabpage.activity-stream.asrouter.devtoolsEnabled to true
2. Go to about:asrouter
3. Search for TEST_FEATURE_TOUR with the findbar, click the "Show" button for its row

Gijs or Emilio, if you have some time to spare, could you help us understand how focus behavior works for panels? How does focus get "stuck" inside the panel, and what prevents focus from landing inside the panel when tabbing from outside it? Thank you.

Comment 3

[Emilio Cobos Álvarez [:emilio]]

(In reply to Shane Hughes [:aminomancer] from comment #2)

Gijs or Emilio, if you have some time to spare, could you help us understand how focus behavior works for panels? How does focus get "stuck" inside the panel, and what prevents focus from landing inside the panel when tabbing from outside it? Thank you.

This is long-standing / since forever, see this comment: https://searchfox.org/mozilla-central/rev/b476ffaef761ff85c012e2d93050cf444ff7be34/dom/base/nsFocusManager.cpp#4246-4255

Now whether we should avoid it... I think it might be worth trying, or worth adding a CSS opt-in / opt out for that? Maybe we should only trap focus for autohide panels?

Sean has been looking at making this code use the DOM tree, and maybe has thoughts?

Comment 4

[Shane Hughes [:aminomancer]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #3)

This is long-standing / since forever, see this comment: https://searchfox.org/mozilla-central/rev/b476ffaef761ff85c012e2d93050cf444ff7be34/dom/base/nsFocusManager.cpp#4246-4255

Great, thank you!

Now whether we should avoid it... I think it might be worth trying, or worth adding a CSS opt-in / opt out for that? Maybe we should only trap focus for autohide panels?

That sounds reasonable. The ctrlTab code might need to be adjusted, but other than that I can't think of are any other non-autohide panels besides ours.

ni?sefeng for feedback on Emilio's suggestion, thanks!

Comment 5

[Sean Feng [:sefeng211]]

Sorry, I don't have much thoughts on this. I think what Emilio suggested make sense. I guess I'd prefer Maybe we should only trap focus for autohide panels? for better consistency.

Comment 6

[Shane Hughes [:aminomancer]]

I found that update notifications (AppMenuNotifications) also disable autohide. So that may be kind of an a11y problem, since you can't focus into those update notifications using just a keyboard.

Comment 7

[Shane Hughes [:aminomancer]]

I've noticed that some of this logic which prevents tabbing into/out of a popup frame also applies to popover frames. But I don't think popovers have the same autohide behavior. At least, I haven't been able to find any documentation of autohide behavior for popovers. And this is actually directly relevant to this bug, because we plan to start using popover for feature callouts in HTML content.

So, what should we do about popovers? Perhaps instead of branching on noautohide, we should add a new attribute (trapfocus?) that directly controls this behavior. However, it seems like there's a big difference between changing behavior for our proprietary XUL elements and changing behavior for an HTML element that implements a web standard. Would I have permission to add a nonstandard feature to popover? Or should I just leave the behavior as-is for popover elements?

Comment 8

[Shane Hughes [:aminomancer]]

Attachment: Bug 1899741 - Allow focus to move into and out of noautohide popups. r=emilio

Comment 9

[Emilio Cobos Álvarez [:emilio]]

I haven't been able to find any documentation of autohide behavior for popovers.

https://html.spec.whatwg.org/#popover-light-dismiss?

So, what should we do about popovers?

If the focus behavior of popovers is not sufficient or not sufficiently controllable, then we should file a spec issue. We could propose and prototype a solution for that if / as needed.

Comment 10

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

There is an r+ patch which didn't land and no activity in this bug for 2 weeks.
:aminomancer, could you have a look please?
If you still have some work to do, you can add an action "Plan Changes" in Phabricator.
For more information, please visit BugBot documentation.

Comment 11

[Emily McMinn :emcminn]

We hit an issue while landing an addons callout in m-c; where the existing focus behaviour was called out for pulling focus from the URL bar when opening a new tab.

Just noting here that the original focus behaviour was added partially as an a11y feature for VoiceOver on Mac. Putting focus in the callout allows the screen reader to announce the callout; though this workaround is only necessary for MacOs; Windows users using either NVDA or Narrator should hear the callout announced due to the role=alertdialog property.

Comment 12

[Emily McMinn :emcminn]

Closing in favour of fix in https://bugzilla.mozilla.org/show_bug.cgi?id=1892417