1725917 - Incompatible native text foreground and background colors in chrome due to alt theme with ui.systemUsesDarkTheme flipped.

Status: VERIFIED FIXED

(Core :: Widget: Gtk, defect)

Description

[Karl Tomlinson (:karlt)]

color:fieldtext and appearance:textfield appear to sometimes derive from different themes, which can lead to scenarios where both are light colors and text is unreadable.

This can be reproduced with
mozregression --launch 2021-08-15 --pref ui.systemUsesDarkTheme:1
Version 93 has layout.css.prefers-color-scheme.content-override:0, which provides a replacement for legacy ui.systemUsesDarkTheme usage, but ui.systemUsesDarkTheme indicates whether or not a GTK window has a dark background. Presumably the same bug exists for themes with dark window backgrounds but light text entry backgrounds.

Reverting widget.gtk.alt-theme.dark to false avoids the issue.
This was switched in https://hg.mozilla.org/mozilla-central/rev/0e6c67113ca7#l1.28

STR:
Manage Bookmarks -> Bookmarks Toolbar -> Get Involved -> select part of invisible URL.

Comment 1

[Karl Tomlinson (:karlt)]

Attachment: invisible entry text in Library

Comment 2

[Karl Tomlinson (:karlt)]

Attachment: Same bug exists with buttons and their text

Comment 3

[Karl Tomlinson (:karlt)]

Was the alt theme intended for use in a situation where there are no native appearance widgets?

Comment 4

[Emilio Cobos Álvarez (:emilio)]

No. this is expected to happen if you change ui.systemUsesDarkTheme to a value that actually doesn't match your system, because we'd choose system colors for chrome content based on that l&f int. We could work-around this by not looking at the pref here, and looking at the underlying system theme value.

But afaict people were flipping that pref to change prefers-color-scheme on content pages, and thus I think adding layout.css.prefers-color-scheme.content-override was the right solution for that.

Presumably the same bug exists for themes with dark window backgrounds but light text entry backgrounds.

No, it shouldn't. The issue here is that we draw widgets with one theme, but due to the pref flip we pick system colors from another theme. If you don't flip the pref we should use system colors from the right theme (and thus text should be visible).

Does that seem like a reasonable explanation? I could add a workaround for legacy ui.systemUsesDarkTheme usage, but I'd rather not to unless it's totally needed.

Comment 5

[Karl Tomlinson (:karlt)]

Thanks for helping me understand.

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

No. this is expected to happen if you change ui.systemUsesDarkTheme to a value that actually doesn't match your system, because we'd choose system colors for chrome content based on that l&f int. We could work-around this by not looking at the pref here, and looking at the underlying system theme value.

In one sense, that would make ColorSchemeForDocument()s decisions consistent with
nsLookAndFeel::LightTheme()/DarkTheme()

However, doing so may be ignoring the pref more than necessary/appropriate and I'm not clear on the potential consequences should LookAndFeel::SystemColorScheme() and so ColorSchemeForDocument() be inconsistent with Document::PrefersColorScheme().

But afaict people were flipping that pref to change prefers-color-scheme on content pages, and thus I think adding layout.css.prefers-color-scheme.content-override was the right solution for that.

Yes, I agree that having a dedicated (and appropriately named) pref for that is the right thing to do.

Presumably the same bug exists for themes with dark window backgrounds but light text entry backgrounds.

No, it shouldn't. The issue here is that we draw widgets with one theme, but due to the pref flip we pick system colors from another theme. If you don't flip the pref we should use system colors from the right theme (and thus text should be visible).

Ah, I see (and verified) we don't have the same issue there because then ColorSchemeForDocument() and gtk/nsLookAndFeel have a consistent concept of whether or not the system theme is dark.

However, there's no good reason AFAICT to pick system colors from one theme and draw with another. We can fix that.

Does that seem like a reasonable explanation? I could add a workaround for legacy ui.systemUsesDarkTheme usage, but I'd rather not to unless it's totally needed.

It is a little difficult to guess exactly what legacy users wanted when they set ui.systemUsesDarkTheme and the legacy behavior is not necessarily what one would have expected from the name, so I don't feel a need to continue to support legacy behavior through this pref.

Having ui.systemUsesDarkTheme == 1 make Firefox chrome choose a dark theme seems quite reasonable to me.
My main concern is with different themes being used for foreground and background.

Arguably a ui.systemUsesDarkTheme override might mean either

1. A user wants the default theme to be considered as a dark theme (for when dark theme detection fails, for example) and so the alt theme should be a light theme, or
2. A user wants a dark theme irrespective of what the default GTK theme is.

Currently, interpretation 2 is used for system colors but interpretation 1 for drawing.

The interpretation for drawing comes from logic in MatchFirefoxThemeIfNeeded(), which is inconsistent with that in ColorSchemeForDocument() for system colors.

I'll change the logic in MatchFirefoxThemeIfNeeded() to be consistent with the choice for system colors.

We could consider separately whether we want to change from interpretation 2 to interpretation 1, but I'm fine with interpretation 2 as long as it is consistent.

Comment 6

[Karl Tomlinson (:karlt)]

Attachment: Bug 1725917 use the same logic for chrome ColorScheme in MatchFirefoxThemeIfNeeded() as in ColorSchemeForDocument() r?emilio

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e40234e5ffde
use the same logic for chrome ColorScheme in MatchFirefoxThemeIfNeeded() as in ColorSchemeForDocument() r=emilio

Comment 8

[Iulian Moraru]

Backed out for causing linux build bustages on nsLookAndFeel.cpp.

Push with failures

Failure log

Backout link

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/48fda625ea93
use the same logic for chrome ColorScheme in MatchFirefoxThemeIfNeeded() as in ColorSchemeForDocument() r=emilio

Comment 10

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/48fda625ea93

Comment 11

[Timea Zsoldos [:zstimi/tzsoldos], Desktop QA]

I have reproduced this issue using Firefox 93.0a1 build from 2021-08-15 on Ubuntu 20.04 x64 with pref ui.systemUsesDarkTheme:1.
I can confirm this issue is fixed, I verified using Firefox 94.0a1 and Firefox 93.0b7 on Ubuntu 20.04 x64 with pref ui.systemUsesDarkTheme:1 added, the issue not reproducible.