1852050 - [Caret browsing] Caret is misaligned on checkboxes in about:preferences

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect)

Description

[Dão Gottwald [:dao]]

So I thought the custom checkbox styling messed with the caret positioning when caret browsing is enabled in about:preferences. But from what I've seen, removing the custom styling doesn't fix the caret, so there seems to be something broken on a more fundamental level.

(Anecdotally, caret browsing mode is something that users end up enabling unintentionally, which is a separate problem that we should probably look into at some point.)

Comment 1

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

The severity field is not set for this bug.
:hiro, could you have a look please?

For more information, please visit BugBot documentation.

Comment 2

[Hiroyuki Ikezoe (:hiro)]

Dão, would you mind attaching an example to see the issue? Honestly I don't quite understand what the problem is. And is the problem related to scroll or overflow?

Comment 3

[Dão Gottwald [:dao]]

Attachment: Screenshot from 2023-10-30 15-01-45.png

(In reply to Hiroyuki Ikezoe (:hiro) from comment #2)

Dão, would you mind attaching an example to see the issue? Honestly I don't quite understand what the problem is. And is the problem related to scroll or overflow?

Here's an screenshot of the issue with the "Open previous windows and tabs" checkbox in about:preferences.

STR:

1. open about:preferences
2. enable caret browsing (F7)
3. put the caret somewhere close to the "Open previous windows and tabs" checkbox
4. move the caret to the beginning of the "Open previous windows and tabs" label with arrow keys
5. press the left arrow key one more time

Comment 4

[Hiroyuki Ikezoe (:hiro)]

As :dholbert found out (and told to me in #layout Matrix) there's an empty <image class="checkbox-icon"> (which is a zero width element). I guess we don't properly handle such kind of empty elements somewhere in focus or DOM selection area?

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Also note that :dholbert tried to set pointer-event:none or -moz-user-focus:none to the element, but it doesn't fix the issue. So probably our focus handling is unrelated to this bug, I would say.

Comment 6

[Daniel Holbert [:dholbert]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #4)

As :dholbert found out (and told to me in #layout Matrix) there's an empty <image class="checkbox-icon"> (which is a zero width element). I guess we don't properly handle such kind of empty elements somewhere in focus or DOM selection area?

Also note: despite its name, that element is not the thing that draws the checkbox icon. I can set display:none on that node or delete it from the DOM entirely, and the checkbox still looks/functions just the same, as far as I can tell.

Looks like that element comes from this JS, written as part of bug 1590573:
https://searchfox.org/mozilla-central/rev/a1827d62f97d9721993ba489f4af7dea422d418f/toolkit/content/widgets/checkbox.js#15

The author Emma isn't active anymore, but emilio reviewed and has poked at this XUL-emulated-in-flex stuff more than anyone -- emilio ,do you know if that element served/serves a purpose?

Comment 7

[Daniel Holbert [:dholbert]]

Er, I guess that commit was just moving code around, and really <image class="checkbox-icon"> came from bug 1455433 which was just migrating from old/analogous XBL which had <xul:image class="checkbox-icon" xbl:inherits="src"/>. Still: emilio is as likely as anyone to know about weird XUL stuff, so I'll leave his needinfo open. :)

I'm not sure at what point in the history this element lost its function, but it's worth exploring whether we can just remove it from the custom-element code, or display:none it away if we need it in some contexts but not in this context...

Comment 8

[Dão Gottwald [:dao]]

checkbox-icon is being used in about:preferences#home and about:preferences#sync (in the "Choose What To Sync" dialog).

(In reply to Daniel Holbert [:dholbert] from comment #7)

display:none it away if we need it in some contexts but not in this context...

Yeah, that seems reasonable.

Moving this back to Firefox::Settings UI for now. Thanks!

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1852050 - Do not display empty checkbox-icons. r=#settings

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/02097332c55c
Do not display empty checkbox-icons. r=settings-reviewers,Gijs

Comment 11

[Dão Gottwald [:dao]]

(In reply to Pulsebot from comment #10)

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/02097332c55c
Do not display empty checkbox-icons. r=settings-reviewers,Gijs

This will probably break the sync icons as they don't use the src attribute: https://searchfox.org/mozilla-central/rev/eadfec923e2b9c927ade8d0dd4f08a82da50a8a9/browser/themes/shared/preferences/preferences.css#851-889

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Thanks, asked for a backout... I'll try to figure out a better solution, do those not want the inline margin?

Comment 13

[Dão Gottwald [:dao]]

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

Thanks, asked for a backout... I'll try to figure out a better solution, do those not want the inline margin?

They set their own margin: https://searchfox.org/mozilla-central/rev/eadfec923e2b9c927ade8d0dd4f08a82da50a8a9/browser/themes/shared/preferences/preferences.css#821,823
We could just override display there... not great but probably good enough unless this is a pattern we'd want to repeat often.
We could also set the src attribute on those checkboxes.

Comment 14

[Cristina Horotan [:chorotan]]

Backed out on request
Backout: https://hg.mozilla.org/integration/autoland/rev/ee800512cfe548550dbf9b33655d86f0e41eda26

Comment 15

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1852050 - Do not display empty checkbox-icons. r=#settings

Comment 16

[Emilio Cobos Álvarez (:emilio)]

(In reply to Dão Gottwald [:dao] from comment #13)

We could just override display there... not great but probably good enough unless this is a pattern we'd want to repeat often.

Yeah I'll do that for now I think. I think <checkbox> being a "leaf" / BaseText element is rather weird fwiw. I think longer term it might be worth it to switch these to use shadow dom, so that we could slot the icon only if needed or what not. But I guess for now this is fine.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/adbd0d773fe7
Do not display empty checkbox-icons. r=settings-reviewers,desktop-theme-reviewers,dao

Comment 18

[Narcis Beleuzu [:NarcisB]]

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

Comment 19

[Cristian Baica [:cbaica], Release Desktop QA]

I've managed to reproduce the issue using Fx119.0a1.
The issue is verified fixed using Fx 122.0b1, Fx123.0a1 and Fx121.0RC on Windows 10 and Ubuntu 20.04. The caret is no longer going that low between text and checkboxes across different sections of the preferences.

Please note that while this has been fixed, checkbox caret positions still don't seem to be aligned with their respective text caret position.