1636998 - no focus ring on radios and checkboxes in some themes.

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect)

Description

[Bill Goldstein [:aja] (UTC-6)]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Firefox/78.0

Steps to reproduce:

Non-reduced test case: https://jsfiddle.net/jnwbysoq/
scroll down to the Forms section

Actual results:

No focus outline on radios and checkboxes
Probable regression with landing of bug 1311444, though haven't run a mozregression on it.

Expected results:

Focus outline on radios and checkboxes

Comment 1

[Bill Goldstein [:aja] (UTC-6)]

(In reply to Bill Goldstein [:aja] (UTC-6) from comment #0)

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Firefox/78.0

Steps to reproduce:

Non-reduced test case: https://jsfiddle.net/jnwbysoq/
scroll down to the Forms section

Actual results:

No focus outline on radios and checkboxes
Probable regression with landing of bug 1311444, though haven't run a mozregression on it.

Expected results:

Focus outline on radios and checkboxes

On STYLED radios/checkboxes, native or non-native. (On win, if that's relevant).

Comment 2

[Bill Goldstein [:aja] (UTC-6)]

With a lot of extraneous stuff removed: https://jsfiddle.net/2f5yeLz9/

Comment 3

[Emilio Cobos Álvarez (:emilio)]

With non-native, you mean widget.disable-native-theme-for-content=true, right?

If so, I can see it only if the chedckbox is not checked.

Comment 4

[Bill Goldstein [:aja] (UTC-6)]

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

With non-native, you mean widget.disable-native-theme-for-content=true, right?

If so, I can see it only if the chedckbox is not checked.

Correct, widget.disable-native-theme-for-content=true pref.
I see no focus ring on either radio or checkbox, checked or not.
I do, however, see some indication of a focus state on unchecked checkbox: its border remains and its background changes.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

(In reply to Bill Goldstein [:aja] (UTC-6) from comment #4)

Correct, widget.disable-native-theme-for-content=true pref.
I see no focus ring on either radio or checkbox, checked or not.
I do, however, see some indication of a focus state on unchecked checkbox: its border remains and its background changes.

Yeah, that's what should ideally happen. If there's focus indicator from the theme then we shouldn't paint the focus ring.

Comment 6

[Bill Goldstein [:aja] (UTC-6)]

I could maybe see having radios/checkboxes default to outline:none, though I'd think that'd be suboptimal.

But ignoring them when styled with outline-* properties entirely?
Certainly that'd be counter-intuitive and a web compat issue since blink/edgium do render the outline-* properties on checkboxes and checked radios when :focus / :focus-visible apply.

Am I missing something here?

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1636998 - Only suppress auto-style outlines for widgets that paint their own focus indicator. r=jfkthame

Turns out we did have a hook for this already! But it is used to draw or
not inner button styles, so not quite equivalent.

I had to expand the amount of things it applies to because buttons and
such do paint focus indicators in all widgets. This patch could cause
some undesired outlines in some widgets. I hope not (I tried to audit to
the best of my knowledge), but in that case they'd be just more values
to add to the list.

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1636998 - Make ::-moz-focus-outer a no-op, and remove it on Nightly. r=jwatt

See https://bugzilla.mozilla.org/show_bug.cgi?id=932410#c2 for the
context for which this pseudo-element was added.

In the previous patch, I had to special-case range appearance because of
this pseudo-class, but that patch makes this pseudo-class completely
redundant, as now all form controls, themed and unthemed, display
outlines, unless the native theme displays a focus indicator on its own.

Remove the special case, and make ranges use outlines like everything
else rather than this bespoke pseudo-element.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

(In reply to Bill Goldstein [:aja] (UTC-6) from comment #6)

I could maybe see having radios/checkboxes default to outline:none, though I'd think that'd be suboptimal.

But ignoring them when styled with outline-* properties entirely?
Certainly that'd be counter-intuitive and a web compat issue since blink/edgium do render the outline-* properties on checkboxes and checked radios when :focus / :focus-visible apply.

Am I missing something here?

Yeah, what Chrome at least does is quite similar to what I'm proposing. It makes outline-style: auto on a themed checkbox do something different than on a <div> (something theme-dependent). That's the focus indicator bit. If the author uses other outline style then we'd honor it just normally.

Comment 10

[Firefox Bug Husbandry Bot]

Because this bug's Severity is normal and has not been changed, and this bug's priority is -- (none,) indicating it has has not been previously triaged, the bug's Severity is being updated to -- (default, untriaged.)

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f5546bfc9604
Only suppress auto-style outlines for widgets that paint their own focus indicator. r=jfkthame

Comment 12

[Narcis Beleuzu [:NarcisB]]

Backed out for reftest failures on 1174332-1.html

Backout link: https://hg.mozilla.org/integration/autoland/rev/6a0cd004fa71b854f321889b13c047e387851247
Log link: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=302150479&repo=autoland&lineNumber=11781

Comment 13

[Bill Goldstein [:aja] (UTC-6)]

(In reply to Pulsebot from comment #11)

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f5546bfc9604
Only suppress auto-style outlines for widgets that paint their own focus
indicator. r=jfkthame

Tested this (backed-out) autoland build (on win10/64), and now get outline showing on checkboxes and checked radios with non-auto/none outline-styles.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/18afbaa4a006
Only suppress auto-style outlines for widgets that paint their own focus indicator. r=jfkthame

Comment 15

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/18afbaa4a006

Comment 16

[Mathew Hodson]

I think bug 1311427 was accidentally added o the Regressed by field. It's a duplicate so shouldn't be there.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/62ad26fbfaf8
Make ::-moz-focus-outer a no-op, and remove it on Nightly. r=jwatt

Comment 18

[Natalia Csoregi [:nataliaCs]]

Backed out for reftest failures on 1174332-1.html

backout: https://hg.mozilla.org/integration/autoland/rev/f7b43d8ff137c5b8be1ffe3d3539644c7ff64ba2

push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=62ad26fbfaf8cd6ee331b5d79d6933fa10723bf2&searchStr=reftest&selectedTaskRun=V488H9TsQVSV3E_QM-GjWQ-0

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

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/70102e3f7b28
Make ::-moz-focus-outer a no-op, and remove it on Nightly. r=jwatt

Comment 20

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

Set release status flags based on info from the regressing bug 1311444

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/40d09276fc4f
Mark a known-failing test as random.

Comment 22

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/70102e3f7b28
https://hg.mozilla.org/mozilla-central/rev/40d09276fc4f