1687074 - about:preferences findInPage highlight is very difficult to see when combining dark mode with custom highlight / text colours using ui.* prefs

Status: NEW

(Firefox :: Settings UI, defect, P2)

Description

[Shane Hughes [:aminomancer]]

Attachment: aboutPrefHighlight.png

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

Steps to reproduce:

• Enable chromeUI dark mode
• Go to about:preferences and type something in the searchbox
• findInPage.js calls this:

getFindSelection(win) {
// Yuck. See bug 138068.
let docShell = win.docShell;

let controller = docShell
    .QueryInterface(Ci.nsIInterfaceRequestor)
    .getInterface(Ci.nsISelectionDisplay)
    .QueryInterface(Ci.nsISelectionController);

let selection = controller.getSelection(
    Ci.nsISelectionController.SELECTION_FIND
);
selection.setColors("currentColor", "#ffe900", "currentColor", "#003eaa");

return selection;

}

Actual results:

See the attached image. #ffe900 is ultra-bright yellow, 100% saturation 50% lightness. Works fine when currentColor is black, but in dark mode the font color is white. In that case, the text is almost completely illegible. I've tried rec709, rec2020, and DCI-P3 and it's really difficult to read in every case unless gamma is set very low. I don't think it's likely that the average user's color space is configured in such a way that this works.

Expected results:

I think findInPage.js should either read a pref or do a media query, and set a property _highlightColor accordingly. Then when it calls selection.setColors() it can pass "currentColor", highlightColor. I did test it myself with a pref. I think that would be the cheapest way to make the color dynamic but I'm not any expert on the selection controller. However, I think it would be cleaner to use something like:
_highlightColor = window.matchMedia("(prefers-color-scheme: dark)").matches ? "#7d7309" : #ffe900

That way it's not necessary to make any new prefs. But idk what other considerations there might be. Maybe it's more desirable to just find a neutral highlight color that is legible whether the text is black or white. In my experience though, I don't think all users would be happy with a single color. I find that washed out purple is a pretty neutral color that's still bold enough to stick out as a "highlight" color, but not every user wants purple highlights. In this case I think it might actually make sense to make a new pref, since changing highlight colors is a popular user modification. iirc, macOS has let you choose between several highlight colors for like 15 or 20 years.

That said, some prefs already exist for highlight colors. I don't think they're that great, but there are prefs like ui.textHighlightBackground and ui.textSelectBackground and a few more. These change the color of highlights created by the findbar. These prefs could be used but they work in a weird way that I can't really describe. You'd probably have to test them to see what I mean.

Anyway, there also needs to be a CSS rule for the arrow tooltips, e.g.
@media (prefers-color-scheme: dark) {
.search-tooltip {
background-color: #7d7309;
color: #fff;
border: 1px solid #635607;
}
}

Comment 1

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

Bugbug thinks this bug should belong to this component, but please revert this change in case of error.

Comment 2

[:Gijs (he/him)]

(In reply to aminomancer from comment #0)

• Enable chromeUI dark mode

Can you add more detail about what you mean here, so this is reproducible from a clean profile? I can't reproduce the issue when using ui.systemUsesDarkTheme to get the prefs to be dark: highlights stay blue and so the issue does not appear. I tried both the default and "dark" Firefox themes, on Windows 10 (which I guess is what you're on based on the UA at the top of your report, though you later mention macOS - clarifying that would be helpful too). Thank you!

Comment 3

[Shane Hughes [:aminomancer]]

Thanks for the response. I figured out the general issue. It's actually caused by the prefs I mentioned earlier:
ui.textHighlightBackground: #7755FF
ui.textHighlightForeground: #FFFFFF
ui.textSelectBackground: #FFFFFF
ui.textSelectBackgroundAttention: #FF3388
ui.textSelectForegroundAttention: #000000

These are the only way I know of to change the regular findbar highlight colors without overwriting the source code, but unfortunately they mess up a lot of other stuff as well. And like I said in the OP, the exact logic behind their operation eludes me. They seem to use some kind of blend modes, like 2 colors are being applied at one time. Because depending on the other prefs, a given pref like ui.textSelectBackground seems to govern the text color, but if you change the other prefs, it instead seems to govern the background color. Like if you try flipping some of those #FFFFFF values to #000000 you'll see what I mean.

At first I thought maybe #000000 in this case means 0 alpha, but now it really seems like they are layering on top of each other for some reason. I couldn't find any documentation on how this works unfortunately. But in any case. ui.textSelectBackground is the one that messes up the highlight color on about:preferences. And it does also mess up the highlight color in some non-content windows, like in the "show all bookmarks" places menu's search bar. I guess I can't expect much since I'm the one going out of my way changing obscure preferences, but the default findbar highlight colors are pretty dang ugly. And findbar.modalHighlight is really buggy, it draws a transparent white overlay on top of the window which then recedes as I scroll down a browser page. So the overlay doesn't seem to be drawing correctly, it's somehow mistaken about the scroll height of the page.

In the meantime I think I'll try writing a script to just overwrite the findbar highlight function, so I can get rid of these prefs.

Comment 4

[:Gijs (he/him)]

Jared (as you seem to have reviewed the work in bug 1360500) and/or Emilio, can you clarify some of the confusion here, given comment #3? I'm not familiar with this code and it's not clear to me if there's a bug to be fixed (either in the prefs code calling setColors, and/or in the layout code that apparently uses background/foreground colours for the opposite thing per comment #3 ).

Comment 5

[Emilio Cobos Álvarez (:emilio)]

The weird thing you're describing in comment 0 wrt the prefs' behavior are probably the code to ensure sufficient contrast here:

https://searchfox.org/mozilla-central/rev/c54c71ecbd9e64cafc0df3b596e206ac4072cb91/layout/generic/nsTextFrame.cpp#4063
https://searchfox.org/mozilla-central/rev/c54c71ecbd9e64cafc0df3b596e206ac4072cb91/layout/generic/nsTextFrame.cpp#3893

The contrast is computed using luminance, and if the contrast is not enough with the background we swap the colors:

https://searchfox.org/mozilla-central/rev/c54c71ecbd9e64cafc0df3b596e206ac4072cb91/layout/generic/nsTextFrame.cpp#3849-3852

So the heuristic is working as intended (this landed before bug 333659, so quite a lot ago) but maybe we can tweak the heuristic a bit more nowadays? Or maybe we don't want to do that for highlights...

Comment 6

[Shane Hughes [:aminomancer]]

Thanks for the feedback, I guess I never noticed a swap since I use an addon to make everything dark. But I'm wondering why the prefs should affect the about:preferences page specifically. I get why it should choose between 2 colors (the yellow and blue defined in findInPage.js) based on a contrast calculation, but why are these prefs having any effect on it, since they do not actually change the yellow/blue colors? The colors are not read from any pref or variable somewhere, they're just passed as string parameters, totally transient. Seems like the highlight prefs shouldn't matter in this case since their colors are never being drawn to begin with.

But anyway the about:preferences issue is a pretty minor complaint, I just posted the report since I thought it was a more straightforward bug. Now I realize this probably affects very few users since it only comes up if you edit these obscure highlight color prefs. However I still think the weird behavior of the ui.text* prefs is worth looking at. This is really more of a feature request but it's connected to the about:preferences highlight thing, which still seems like unintended behavior. I guess the behavior of these prefs is internally consistent, but even if it's working as intended, it makes for a very confusing user configuration. It's very hard to predict what a given pref is going to do since it changes how the other ones manifest, and I've never been able to find any real documentation or even a description in the source code.

I don't know how to fix something this low level but even a new bool pref to disable the mSufficientContrast code would at least make it possible to set the colors consistently. Anyone using it would have to carefully pick a combination of colors that is legible in every situation, but if they're changing these prefs in the first place that's probably what they're trying to do. (at least that's why I changed them) I know it's a super niche application, but it wouldn't need to have any performance impact, e.g. only read during startup?

Otherwise, all of that effort could be obviated if the default highlight colors were just modified. The default colors are highly visible for sure, but not very appealing. I think if the ability to change them wasn't buried in such esoteric prefs, a lot of users would probably take advantage of it. Or if the findbar color system was overhauled and some simple color picker was placed in the about:preferences page, so it's visually clear what to expect.

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

Since we are setting a hard-coded background color we should not be using currentColor here. I will upload a patch that replaces currentColor with #000 and #fff for selection text color and alternate selection text color.

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Bug 1687074 - Use a fixed foreground color on the preferences' findInPage since we are setting a fixed bg color.

Comment 9

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

The severity field for this bug is set to S3. However, the accessibility severity is higher, [access-s2].
:jaws, could you consider increasing the severity?

For more information, please visit auto_nag documentation.

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

I'm going to leave severity unchanged here since this also requires changing the color mode. I would like to see this get fixed though.