1668989 - Wrong text color after switching from dark mode to light mode leads to invisible text

Status: RESOLVED FIXED

(Thunderbird :: Theme, defect)

Description

[Andreas Wagner [:TheOne] [use NI]]

Attachment: list.png

When switching from dark mode to light mode, the text color in the message list stays white for unread emails and for the selected email. This makes the entry barely or completely unreadable.

Comment 1

[Andreas Wagner [:TheOne] [use NI]]

This might be a regression from bug 1638233. Aleca, would you mind checking? This is a significant issue for mac users that use automatic os theme switching.

Comment 2

[Andreas Wagner [:TheOne] [use NI]]

Mozregression narrowed it down to https://hg.mozilla.org/comm-central/pushloghtml?fromchange=caf553dec882c3644cc382d8fe35642ab4394376&tochange=355771da53468a4daaa4b4fb1678cb7855010bb5

Comment 3

[Andreas Wagner [:TheOne] [use NI]]

Removing the lwt-tree-brighttext attribute from the window element fixes the issue.

Comment 4

[Alessandro Castellani [:aleca]]

Thanks for the report and for identifying the regression.
I'll take care of it.

Comment 5

[Alessandro Castellani [:aleca]]

When switching from dark mode to light mode

Does this happen when switching from Dark to Default?
Does the issue persist if you relaunch Thunderbird?

Comment 6

[Andreas Wagner [:TheOne] [use NI]]

(In reply to Alessandro Castellani (:aleca) from comment #5)

When switching from dark mode to light mode

Does this happen when switching from Dark to Default?

So, I first noticed that when macOS switched from dark mode to light mode (I have the default theme set in Tb).
I can also reproduce switching from the Tb Dark theme to Light theme
I can't really test switching from Dark to Default because, when doing so, Tb stays in Dark mode even though macOS is set to Light mode.

Does the issue persist if you relaunch Thunderbird?

No.

Comment 7

[Michael Herf]

Came to report same bug on Windows 10, so I assume the fix is the same. But please test on both?

When switching Windows app theme from "dark" to "light" bolded text (unread) is drawn in white.

Comment 8

[Alessandro Castellani [:aleca]]

Would you be able to test the latest beta release?
This issue is fixed there and bug 1668896 should be responsible for it.

Comment 9

[Andreas Wagner [:TheOne] [use NI]]

(In reply to Alessandro Castellani (:aleca) from comment #8)

Would you be able to test the latest beta release?
This issue is fixed there and bug 1668896 should be responsible for it.

It looks like this is indeed fixed in 82.0b2.

Comment 10

[Alessandro Castellani [:aleca]]

Great, I'm gonna mark this bug and bug 1668991 as duplicates of bug 1668896, since both are different symptoms of the same issue.
Thanks for checking.

Comment 12

[Andreas Wagner [:TheOne] [use NI]]

Looks like this was not a duplicate after all. I upgraded to 78.4 and the issue is still occurring.

Comment 13

[Richard Marti (:Paenglab)]

Andreas, do you use a LW-theme with bright text in the toolbar?

Comment 14

[Andreas Wagner [:TheOne] [use NI]]

I am using the default theme.

Comment 15

[Alessandro Castellani [:aleca]]

WTH! There's something totally broken now in 78 when switching between themes.
Selecting Dark theme doesn't change the ui.systemUsesDarkTheme, and switching back to Default or Light triggers is.
Uff, this is driving me crazy.
I'll deal with it, thanks for the feedback.

Comment 17

[Andreas Wagner [:TheOne] [use NI]]

(In reply to Alessandro Castellani (:aleca) from comment #15)

WTH! There's something totally broken now in 78 when switching between themes.
Selecting Dark theme doesn't change the ui.systemUsesDarkTheme, and switching back to Default or Light triggers is.
Uff, this is driving me crazy.
I'll deal with it, thanks for the feedback.

Yes, see comment 2. It's likely https://hg.mozilla.org/comm-central/rev/eb5bf594244d26e7b80efc36bf8642cdb39af411 and I believe I remember that when I stepped through that code _isTextColorDark didn't return the right thing when switching from (os level) dark mode to light mode.

Comment 18

[Alessandro Castellani [:aleca]]

Attachment: 1668989-theme-switch.diff

I don't want to jinx it, but I think I got it.

As I suspected, it's all a race problem that causes random outcomes.
The --sidebar-text-color property is changed before the "extensions.activeThemeID" PREF was changed, so our whole theme luminance and system dark shenanigans calculation was happening a bit too soon, and we were ending up with a sidebar color that didn't match the actual selected theme.

It was totally clunky and hard to manage everything in the same location, so I decided to create an observer for dealing with the theme change and the ui.systemUsesDarkTheme, and don't interfere with the processColor() nor the inferFromText() method.

So, here's a quick overview of what I did:

• I created an observer that gets triggered only when the theme actually changes. The observer will properly update the ui.systemUsesDarkTheme pref accordingly, without clashing with the theme color changing process.
• I simplified the processColor() to only handle the element attribute.
• I rewrote the inferFromText() to match Firefox, since we now have an handy observer called "windowlwthemeupdate" which can help us trigger the toolbox brighttext attribute change at the right time. I also copied their implementation of a local caching system for the luminance matching to prevent running that method when not necessary.

Here's a try run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=7aa6c6ac54c88aa31268cea6c35beb762e3c1b2d

Andreas, would you be able to load this patch, or test the try-build (once finished) with a separated profile to confirm all the issues have been fixed?

Richard, could you test this on Windows to be sure it respects the OS theme?

Magnus, what do you think about that observer? Is in the correct location or do you recommend moving it somewhere else more appropriate?

Comment 19

[Magnus Melin [:mkmelin]]

Comment on attachment 9183383 [details] [diff] [review]
1668989-theme-switch.diff

Review of attachment 9183383 [details] [diff] [review]:
-----------------------------------------------------------------

Code wise looks reasonable.

Comment 20

[Richard Marti (:Paenglab)]

Comment on attachment 9183383 [details] [diff] [review]
1668989-theme-switch.diff

This doesn't fix on Windows the issue of the dark LW-themes which don't set the sidebar colours:

• When I switch from the default or light theme to such a dark LW-theme, Dark Fox in my case, all content pages switch to the dark theme and the folder pane icons are white on the white background.
• After restarting TB, the folder pane icons are correctly dark. But the in-content pages are still dark. The main tab content is light except the themed toolbars.
• When I disable the LW-theme, the default theme is now active, the icons are again white until I restart TB.
• All other combinations seem to work correctly, also the change from LW-theme to default when enabling the default theme.

Mac has the same issue with the white folder pane icons when switching to a dark LW-theme. But a restart doesn't fix it. They are always white.

On Linux I only see the light folder pane icons when I switch from dark TB theme to a dark LW-theme. A TB restart fixes the icon colour. All other cases are working.

On Mac and Windows with the dark system theme, when using a dark LW-theme, the main content switches to light. This should then, if possible, use the dark theme somehow.

Comment 21

[Alessandro Castellani [:aleca]]

Darn, we're so close.
What's the problem with a "Dark LW-Theme"? What's the uniqueness of those type of themes?
Which dark LW-theme are you using on mac? Dark Fox?

It seems that the only remaining issue is related to dark LW-themes. If I can manage to fix that edge case we should be good to go.

Comment 22

[Richard Marti (:Paenglab)]

I'm using on all platforms the same themes. For the dark LW-themes Dark Fox. This themes set only these colours:
"colors": {"frame": "#000000", "tab_background_text": "#ffffff"}.
It's the tab_background_text that is used to make the toolbar text white. And the frame colour sets the window background to black. This LW-themes don't set any colour in the content. They skin only the toolbars and statusbar.

Comment 23

[Alessandro Castellani [:aleca]]

Attachment: 1668989-theme-switch.diff

This patch simplifies even more things.
We're gonna rely on the theme itself to pass the right color value, and if no color is specified, it means it's only a LW theme that doesn't affect the content color.

Since the theme change event is triggered after the processColor()run, we consistently have the lwt-tree-brighttext attribute, true or false, depending on what the theme requires.
I'm using that attribute as a condition for the in-content dark/light mode, so no luminance calculation needed.

Comment 24

[Alessandro Castellani [:aleca]]

Attachment: 1668989-theme-switch.diff

Small fix for when using light themes on a OS dark mode.

Comment 25

[Richard Marti (:Paenglab)]

Comment on attachment 9183550 [details] [diff] [review]
1668989-theme-switch.diff

Many thanks!!

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/a44b22f0e2e1
Fix UI not properly updating when switching between Dark, Light, and Default theme. r=mkmelin, ui-r=Paenglab DONTBUILD

Comment 27

[Alessandro Castellani [:aleca]]

Comment on attachment 9183550 [details] [diff] [review]
1668989-theme-switch.diff

[Approval Request Comment]
Regression caused by (bug #): bug 1638233
User impact if declined: Inconsistent interface when switching between dark, light, and default themes.
Testing completed (on c-c, etc.): on c-c
Risk to taking this patch (and alternatives if risky): Low as this patch drastically improves and (potentially) fixes all our current themes problem by implementing an observer to properly calculate the luminance when a theme is switched.

Comment 28

[Wayne Mery (:wsmwk)]

Comment on attachment 9183550 [details] [diff] [review]
1668989-theme-switch.diff

[Triage Comment]
Approved for beta

Comment 29

[Rob Lemley [:rjl]]

Thunderbird 83.0b2:
https://hg.mozilla.org/releases/comm-beta/rev/2de8adaa6e28

Comment 30

[Andreas Wagner [:TheOne] [use NI]]

Thank you for working on this!
Do we want to request uplift to the next point release as well?

Comment 31

[Alessandro Castellani [:aleca]]

Comment on attachment 9183550 [details] [diff] [review]
1668989-theme-switch.diff

[Approval Request Comment]
Regression caused by (bug #): bug 1638233
User impact if declined: Inconsistent interface when switching between dark, light, and default themes.
Testing completed (on c-c, etc.): on c-c and beta
Risk to taking this patch (and alternatives if risky): Low as this patch drastically improves and (potentially) fixes all of our current themeing problems by implementing an observer to properly calculate the luminance when a theme is switched.

Comment 33

[Cheryl Pace]

So I am not a programmer. What is the solution?

Comment 34

[Richard Marti (:Paenglab)]

(In reply to Cheryl Pace from comment #33)

So I am not a programmer. What is the solution?

Disable the LW-theme until the fix is in TB 78.

Comment 36

[Wayne Mery (:wsmwk)]

Comment on attachment 9183550 [details] [diff] [review]
1668989-theme-switch.diff

[Triage Comment]
Approved for esr78

Comment 37

[Rob Lemley [:rjl]]

Thunderbird 78.4.1:
https://hg.mozilla.org/releases/comm-esr78/rev/ff7f8b9141a5

Comment 39

[Andreas Wagner [:TheOne] [use NI]]

Unfortunately, this is still happening in 78.4.1

Comment 40

[Richard Marti (:Paenglab)]

Yes, I see still an issue with TB 78.4.1. I compared the the files ThemeVariableMap.jsm and there is a line missing which was added by bug 1659282.

Comment 41

[Rob Lemley [:rjl]]

Reclosing this bug as there is nothing more to do here. Uplift was requested on the dependency.