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

Status: VERIFIED FIXED

(Firefox :: Keyboard Navigation, defect, P2)

Description

[Romain Testard [:RT]]

Attachment: 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

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Some of this might overlap with bug 1708261.

Comment 2

[Mike Conley (:mconley) (:⚙️)]

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?

Comment 3

[Emilio Cobos Álvarez (: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?

Comment 4

[Emilio Cobos Álvarez (:emilio)]

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

Comment 5

[Emilio Cobos Álvarez (:emilio)]

(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)

Comment 6

[Mike Conley (:mconley) (:⚙️)]

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?

Comment 7

[Emilio Cobos Álvarez (:emilio)]

(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?

Comment 8

[Mike Conley (:mconley) (:⚙️)]

(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?

Comment 9

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 10

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 11

[Mike Conley (:mconley) (:⚙️)]

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?

Comment 12

[Emilio Cobos Álvarez (: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?

Comment 13

[Emilio Cobos Álvarez (:emilio)]

I filed bug 1710756 to expose this to error pages.

Comment 14

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1709050 - Suppress the focus ring in the upgrade dialog and some about: pages when they first load. r?Mardak!,Gijs!

Comment 15

[Pulsebot]

Pushed by [e-mail]:
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

Comment 16

[Dorel Luca [:dluca]]

Backed out changeset 13b220eff7d8 (bug 1709050) for Browser-chrome failures in browser/base/content/test/about/browser_aboutCertError.js. CLOSED TREE

Log:
https://treeherder.mozilla.org/logviewer?job_id=339904676&repo=autoland&lineNumber=2334

Push with failures:
https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&revision=13b220eff7d8797ee5a62f77ff892b4129285517

Backout:
https://hg.mozilla.org/integration/autoland/rev/a09512d83e9b666a3bb673e3afea8dcb73c716d2

Comment 17

[Pulsebot]

Pushed by [e-mail]:
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

Comment 18

[Atila Butkovits]

Backed out for causing failure at browser_aboutNetError.js.

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

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&revision=ec36559d6b9210d519e732bf07fe90684f5868a3&searchStr=linux%2C18.04%2Cx64%2Ctsan%2Copt%2Cmochitests%2Ctest-linux1804-64-tsan%2Fopt-mochitest-browser-chrome-e10s%2Cbc3&selectedTaskRun=XhKYYXLRT_OIhOp7_R8Dsw.0

Failure log: https://treeherder.mozilla.org/logviewer?job_id=339928501&repo=autoland&lineNumber=2412

Comment 19

[Pulsebot]

Pushed by [e-mail]:
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

Comment 20

[Sandor Molnar]

https://hg.mozilla.org/mozilla-central/rev/7a7f6f210624

Comment 21

[Christoph Kerschbaumer [:ckerschb]]

(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.

Comment 22

[Giorgia Nichita, Release Desktop QA]

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!