1638127 - Linux applying :-moz-focusring to buttons even without keyboard interaction feels off

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect, P3)

Description

[:Gijs (he/him)]

On Windows, we only start showing focus rings once the user uses the keyboard to move focus in the document. That feels like a more natural choice.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Just a quick note, because I'm going to bed so not gonna write the patch right now: GTK behavior seems to be (from playing with gtk3-demo and gkt3-widget-factory) to show focus rings only after keyboard interaction.

So I think this is a matter of a pref flip. There's probably more stuff to be cleaned up afterwards though.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Gijs, are you sure of comment 0?

data:text/html,<style>:-moz-focusring { outline: 10px solid red }</style><button style="background: blue">foo

Shows a red outline on windows... Whether it should is another matter I guess, but...

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Huh, I lie, that's from browserstack apparently...

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1638127 - Make focus rings not always shown on Linux. r=stransky,karlt

This makes us closer to the GTK behavior (not showing outlines until
we've navigated with the keyboard).

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1638127 - Minor cleanup around nsWindowRoot. r=edgar

Depends on D75501

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1638127 - Move the decision of whether to draw focus rings based on the theme further down. r=stransky,karlt,mstange

This is useful for non-native theme too, and shouldn't change behavior
in other themes.

This is so that we don't need to pile up ifdefs in nsNativeTheme, and so
that the GTK theme doesn't change behavior with the following patches.

Instead of hackily remove the FOCUS bit, make the theme check for the
FOCUSRING bit, which is the right thing to check for anyway.

Depends on D75502

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1638127 - Remove nsGlobalWindowInner::mShowFocusRingForContent. r=edgar

Instead move the check to the focus manager, more similar to how
focus-visible works.

Now nsGlobalWindowInner::ShouldShowFocusRing means "Should we show focus
ring for anything in this window", that is: Have we keyboard-navigated
in this window, or do we have a pref that says that we should always
show focus rings.

Fix some callers appropriately (some of them that were not properly
accounting for the element being focused in the first place...).

Depends on D75503

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1638127 - Make Linux focus ring behavior match Windows, rather than Mac. r=stransky,karlt

The Mac focus model is a bit different (mouse doesn't focus form
controls for example).

This matches GTK3 to my knowledge, where outlines are not shown until
you've navigated with the keyboard.

We should maybe consider changing Android as well (and maybe all
platforms, actually), but that's a bit of a bigger endeavour.

Depends on D75504

Comment 9

[Emilio Cobos Álvarez (:emilio)]

ok, so apparently whether we always show focus rings on windows depends on some system setting, which is default false. I got a bit driven away cleaning up this code but I think the end state is better.

Comment 10

[Emilio Cobos Álvarez (:emilio)]

This is also useful work for :focus-visible and such.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d85cc9055caf
Minor cleanup around nsWindowRoot. r=edgar

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a8850a0ffab1
Remove nsGlobalWindowInner::mShowFocusRingForContent. r=edgar

Comment 13

[Natalia Csoregi [:nataliaCs]]

Backed out for failures on test_focusrings.xhtml

backout: https://hg.mozilla.org/integration/autoland/rev/8baffa98bf72c951e2bb20769bb9fc3d82199722

push: failure appeared on a subsequent push https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedTaskRun=RG_f0ohwQTywzlxs0zt1xQ-0&revision=b2881b76bd3b7acc8f76cb059a0dd216d899bce6&searchStr=os%2Cx%2C10.14%2Cdebug%2Cmochitests%2Cwithout%2Ce10s%2Ctest-macosx1014-64%2Fdebug-mochitest-chrome-1proc-2%2Cm-1proc%28c2%29

failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=302507483&repo=autoland&lineNumber=109982

[task 2020-05-15T21:09:41.617Z] 21:09:41 INFO - TEST-START | dom/tests/mochitest/general/test_focusrings.xhtml
[task 2020-05-15T21:09:42.716Z] 21:09:42 INFO - GECKO(2047) | [2047, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004005 (NS_ERROR_FAILURE): file /builds/worker/checkouts/gecko/dom/base/nsContentUtils.cpp, line 3734
[task 2020-05-15T21:09:42.807Z] 21:09:42 INFO - GECKO(2047) | [2047, Main Thread] WARNING: NS_ENSURE_TRUE(presShell) failed: file /builds/worker/checkouts/gecko/dom/base/nsGlobalWindowOuter.cpp, line 4263
[task 2020-05-15T21:09:42.838Z] 21:09:42 INFO - TEST-INFO | started process screencapture
[task 2020-05-15T21:09:42.957Z] 21:09:42 INFO - TEST-INFO | screencapture: exit 0
[task 2020-05-15T21:09:42.957Z] 21:09:42 INFO - Buffered messages logged at 21:09:42
[task 2020-05-15T21:09:42.957Z] 21:09:42 INFO - must wait for load
[task 2020-05-15T21:09:42.958Z] 21:09:42 INFO - TEST-PASS | dom/tests/mochitest/general/test_focusrings.xhtml | unfocused shows no ring
[task 2020-05-15T21:09:42.958Z] 21:09:42 INFO - Buffered messages finished
[task 2020-05-15T21:09:42.958Z] 21:09:42 INFO - TEST-UNEXPECTED-FAIL | dom/tests/mochitest/general/test_focusrings.xhtml | focus shows ring
[task 2020-05-15T21:09:42.958Z] 21:09:42 INFO - SimpleTest.ok@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:299:16
[task 2020-05-15T21:09:42.958Z] 21:09:42 INFO - runTest@chrome://mochitests/content/chrome/dom/tests/mochitest/general/test_focusrings.xhtml:48:7
[task 2020-05-15T21:09:42.958Z] 21:09:42 INFO - SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:918:23
[task 2020-05-15T21:09:42.958Z] 21:09:42 INFO - TEST-PASS | dom/tests/mochitest/general/test_focusrings.xhtml | initial appearance
[task 2020-05-15T21:09:42.958Z] 21:09:42 INFO - TEST-PASS | dom/tests/mochitest/general/test_focusrings.xhtml | mouse click on <button id='elem'>Button</button>
[task 2020-05-15T21:09:42.959Z] 21:09:42 INFO - TEST-PASS | dom/tests/mochitest/general/test_focusrings.xhtml | mouse click on <button id='elem'>Button</button> ring

Comment 14

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/d85cc9055caf

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/701570151436
Remove nsGlobalWindowInner::mShowFocusRingForContent. r=edgar

Comment 16

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/701570151436

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bdfe90f8088e
Make focus rings not always shown on Linux. r=stransky
https://hg.mozilla.org/integration/autoland/rev/72afaabad8ca
Make Linux focus ring behavior match Windows, rather than Mac. r=karlt

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/acf55c958a83
Fix a typo in an ifdef on a CLOSED TREE.

Comment 19

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/bdfe90f8088e
https://hg.mozilla.org/mozilla-central/rev/72afaabad8ca
https://hg.mozilla.org/mozilla-central/rev/acf55c958a83