Closed Bug 1709050 Opened 3 years ago Closed 3 years ago

Focus ring displayed by default on C2A for some about:pages

Categories

(Firefox :: Keyboard Navigation, defect, P2)

defect

Tracking

()

VERIFIED FIXED
90 Branch
Tracking Status
firefox90 --- verified

People

(Reporter: RT, Assigned: mconley)

References

(Blocks 1 open bug)

Details

(Whiteboard: [proton-foundations] [priority:2a])

Attachments

(2 files)

Attached image Sans titre.png

The following internal pages display a focus ring by default on the main click to action whereas the focus ring should only be displayed if the page was accessed through keyboard navigation:

  • existing user onboarding
  • about:config
  • page displayed when accessing a domain that does not exist
  • page displayed after refreshing Firefox
  • page displayed after entering an invalid URL
  • page displayed after session restore failed

Some of this might overlap with bug 1708261.

Assignee: nobody → mconley
See Also: → 1708261

I believe most of the about: pages where we're showing the ring is because we have autofocus="true" on the button, and so when the autofocus runnable fires, it focuses that button, and then we enter ShouldMatchFocusVisible here: https://searchfox.org/mozilla-central/rev/aec7c53cdbbff65305d41c9d805a70efc0e902ed/dom/base/nsFocusManager.cpp#1232

and then we hit this branch: https://searchfox.org/mozilla-central/rev/aec7c53cdbbff65305d41c9d805a70efc0e902ed/dom/base/nsFocusManager.cpp#1275

which delegates the decision making to the value of the mUnknownFocusMethodShouldShowOutline boolean on the window, which defaults to true, and notably has this comment:

  // .. The initial value of this is true (so as to show
  // outlines for stuff like html autofocus, or initial programmatic focus
  // without any other user interaction).

Hey emilio, shouldn't we only ever match :focus-visible if the user has been navigating / switching focus with the key? Why would autofocus or initial programmatic focus default to showing the ring? Is this part of the web specification for :focus-visible or something?

Flags: needinfo?(emilio)

It's not part of any specification, and we can change it if we want / need to... But how would a user know where the focus is if we don't show the focusring?

Flags: needinfo?(emilio)

We have an escape hatch to prevent the outline as needed for chrome (FocusOptions.preventFocusRing)

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

It's not part of any specification, and we can change it if we want / need to... But how would a user know where the focus is if we don't show the focusring?

(More to the point, the behavior matches other browsers, so if we want to change it we probably want a good reason for it)

But how would a user know where the focus is if we don't show the focusring?

I think in the cases for this bug, it's implied by the styling of the primary button. The preventFocusRing escape hatch works for the programmatic case, but what about the autofocus case?

Status: NEW → ASSIGNED

(In reply to Mike Conley (:mconley) (:⚙️) (Catching up on needinfos) from comment #6)

But how would a user know where the focus is if we don't show the focusring?

I think in the cases for this bug, it's implied by the styling of the primary button. The preventFocusRing escape hatch works for the programmatic case, but what about the autofocus case?

There's no escape hatch that I know for autofocus. Is it too annoying to focus the button programmatically instead?

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

Is it too annoying to focus the button programmatically instead?

No, it's reasonably straight-forward, but it's also something we'll need to do for every in-content page. I guess I'm just looking to see if there's something here that could help front-end from needing to remember to do that every time. I guess it's not practical to have about: pages default mUnknownFocusMethodShouldShowOutline to false?

It's possible to do that I guess, in InitDocumentDependentState, you could check the document URI and set the bit to false. Though gotta be careful with about:blank, and it is slightly unfortunate to have that behavior difference between about: pages and other pages.

Alright, well, given that the list of about: pages listed in comment 0 is finite and quite small, maybe let's just go with the escape hatch for now, and not use autofocus on those.

So one of the issues I'm running into here is that about:neterror doesn't qualify for the [ChromeOnly] preventFocusRing optional argument to focus(). Is there a more lax directive for in-product unprivileged about: pages like about:neterror, emilio?

Flags: needinfo?(emilio)

Yeah, we should be able to implement something like that quite easily.

Looking at stuff that those pages use like <link rel=localization>, I see we special-case them for l10n... Christoph, do you know why error pages don't run with the system principal?

Flags: needinfo?(ckerschb)
No longer blocks: 1710756
Depends on: 1710756

I filed bug 1710756 to expose this to error pages.

Flags: needinfo?(emilio)
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/13b220eff7d8
Suppress the focus ring in the upgrade dialog and some about: pages when they first load. r=Mardak,Gijs
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ec36559d6b92
Suppress the focus ring in the upgrade dialog and some about: pages when they first load. r=Mardak,Gijs
Regressions: 1711530
Flags: needinfo?(mconley)
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7a7f6f210624
Suppress the focus ring in the upgrade dialog and some about: pages when they first load. r=Mardak,Gijs
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 90 Branch

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

Looking at stuff that those pages use like <link rel=localization>, I see we special-case them for l10n... Christoph, do you know why error pages don't run with the system principal?

Sorry for the lag here - clearing out my ni? queue right now. To answer your question, the rule of thumb is: Grant system privileges when you have to, but try to grant only the lowest needed privileges possible. It seems error pages don't need system privileges.

Flags: needinfo?(ckerschb)
Flags: qe-verify+

I have reproduced the issue on an affected Nightly build 90.0a1 (2021-05-03)
The fix was verified on 90.0b2 (2021-06-01). Tests were performed on Ubuntu 20.04, Windows 10 and macOS 10.15.
Thanks!

Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: