1699259 - Focus rings show up on buttons when showing tab-modal dialogs (and proton content / window modal ones)

Status: RESOLVED FIXED

(Toolkit Graveyard :: Notifications and Alerts, defect, P1)

Description

[:Gijs (he/him)]

Somehow opening these dialogs seems to sometimes trigger :-moz-focusring, which is not expected. In proton this shows quite "heavy" focus rings around buttons which shouldn't happen for mouse users.

I can intermittently reproduce this even when the tab-modal dialogs were initially implemented back in April 2020, before they used a separate window. However, I'm struggling to work out what is tripping the flag on the window to start showing focus rings.

Emilio, I'm hoping that with your experience of focus rings and pernosco / rr this should be easy for you to figure out - can you please help identify how that is happening? For STR on current nightly, just alert('hi') from a web console seems to do it.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

So this is IMO more of a feature rather than a bug, but...

The focus comes from here.

Since the dialog has never had any focused element, then programmatic focus shows a ring. There's some discussion in this CSSWG issue.

So for most pages that open dialogs this is not an issue because the dialog is opened via click, which focuses the button etc, and then we don't show rings because the button didn't use to match :focus-visible...

I'd argue that autofocus should actually show the focus rings. Anyhow if you don't want to probably we could add a special codepath to the focus manager to forcibly not match focus-visible in this case or something.

Comment 2

[:Gijs (he/him)]

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

So this is IMO more of a feature rather than a bug, but...

The focus comes from here.

Since the dialog has never had any focused element, then programmatic focus shows a ring. There's some discussion in this CSSWG issue.

So for most pages that open dialogs this is not an issue because the dialog is opened via click, which focuses the button etc, and then we don't show rings because the button didn't use to match :focus-visible...

I'm confused - what kind of "dialog" are you referring to here? Are you suggesting that this problem doesn't happen if a website calls alert() in response to a mouse click (and if so, why does that make a difference here - we still load commonDialog...)? Or are you talking about html:dialog use inside websites (which AIUI is still preffed off in gecko on release) ? Does the presence or absence of a focused element in the webpage before the alert() shows up make a difference to the :focus-visible matching in the alert() dialog document, even though they're in different processes as well as different browsing context trees (ie the alert dialog is not a subframe of the web content document)? Or something else?

Comment 3

[Emilio Cobos Álvarez (:emilio)]

I meant for regular websites where the dialog (as in html:dialog or something else) is in the same document, sorry.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1699259 - Add a ChromeOnly FocusOptions.preventFocusRing to opt out of focus ring heuristics. r=edgar!,Gijs!

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Would something like the above work for you?

Comment 6

[:Gijs (he/him)]

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

Would something like the above work for you?

Maybe, about to head for lunch - is the same thing not already possible from frontend code if we use the focus manager directly and pass the right flags, instead of relying on the Element.focus() abstraction? I don't know at what level the heuristics are implemented...

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Well, there was no flag to trump these heuristics to not show the ring. There's a flag to always show a ring, but that's obvs not what you want.

I could just add the focus manager code and tweak the dialog to use Services.focus directly if you want, but seemed in line with preventScroll being in FocusOptions, and it's pretty trivial to do.

Comment 8

[:Gijs (he/him)]

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

Well, there was no flag to trump these heuristics to not show the ring. There's a flag to always show a ring, but that's obvs not what you want.

I could just add the focus manager code and tweak the dialog to use Services.focus directly if you want, but seemed in line with preventScroll being in FocusOptions, and it's pretty trivial to do.

OK, then let's go the element.focus() route. We might need to update some other focusing code, I guess we'll find out when this lands. I'll test this a bit on my Windows machine later, but I'm going to need to do a full build so it'll take a while.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f588378316fd
Add a ChromeOnly FocusOptions.preventFocusRing to opt out of focus ring heuristics. r=edgar,Gijs

Comment 10

[Alexandru Michis [:malexandru]]

Backed out changeset f588378316fd (Bug 1699259) for causing mochitest failures in test_focusrings.xhtml
Backout link: https://hg.mozilla.org/integration/autoland/rev/5db45840697073fd14a407137d92be2f5e173a00
Push with failures, failure log.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/01e72f4f6021
Add a ChromeOnly FocusOptions.preventFocusRing to opt out of focus ring heuristics. r=edgar,Gijs

Comment 12

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/01e72f4f6021