Closed Bug 1668989 Opened 1 year ago Closed 11 months ago

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

Categories

(Thunderbird :: Theme, defect)

Desktop
macOS
defect

Tracking

(thunderbird_esr78+ fixed, thunderbird83+ fixed)

RESOLVED FIXED
84 Branch
Tracking Status
thunderbird_esr78 + fixed
thunderbird83 + fixed

People

(Reporter: TheOne, Assigned: aleca)

References

(Depends on 1 open bug, Regression)

Details

(Keywords: regression)

Attachments

(2 files, 2 obsolete files)

Attached image 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.

Component: Folder and Message Lists → Theme

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.

Flags: needinfo?(alessandro)

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

Assignee: nobody → alessandro
Flags: needinfo?(alessandro)
Regressed by: 1638233

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

Depends on: tb78found

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?

Flags: needinfo?(awagner)
See Also: → 1668991

(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.

Flags: needinfo?(awagner)

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.

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

(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.

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.

Status: NEW → RESOLVED
Closed: 11 months ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1668896

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

Status: RESOLVED → REOPENED
Flags: needinfo?(alessandro)
Resolution: DUPLICATE → ---

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

Flags: needinfo?(awagner)

I am using the default theme.

Flags: needinfo?(awagner)

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.

Flags: needinfo?(alessandro)
Severity: -- → S3
Duplicate of this bug: 1668991

(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.

Attached patch 1668989-theme-switch.diff (obsolete) — Splinter Review

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?

Attachment #9183383 - Flags: ui-review?(richard.marti)
Attachment #9183383 - Flags: review?(mkmelin+mozilla)
Flags: needinfo?(awagner)
Comment on attachment 9183383 [details] [diff] [review]
1668989-theme-switch.diff

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

Code wise looks reasonable.
Attachment #9183383 - Flags: review?(mkmelin+mozilla) → review+

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.

Attachment #9183383 - Flags: ui-review?(richard.marti) → ui-review-

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.

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.

Attached patch 1668989-theme-switch.diff (obsolete) — Splinter Review

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.

Attachment #9183383 - Attachment is obsolete: true
Attachment #9183505 - Flags: ui-review?(richard.marti)
Attachment #9183505 - Flags: review+

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

Attachment #9183505 - Attachment is obsolete: true
Attachment #9183505 - Flags: ui-review?(richard.marti)
Attachment #9183550 - Flags: ui-review?(richard.marti)
Attachment #9183550 - Flags: review+

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

Many thanks!!

Attachment #9183550 - Flags: ui-review?(richard.marti) → ui-review+
Target Milestone: --- → 84 Branch

Pushed by mkmelin@iki.fi:
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

Status: REOPENED → RESOLVED
Closed: 11 months ago11 months ago
Resolution: --- → FIXED

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.

Attachment #9183550 - Flags: approval-comm-beta?

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

[Triage Comment]
Approved for beta

Attachment #9183550 - Flags: approval-comm-beta? → approval-comm-beta+

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

Flags: needinfo?(awagner)

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.

Attachment #9183550 - Flags: approval-comm-esr78?
Duplicate of this bug: 1674167

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

(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.

Duplicate of this bug: 1665772

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

[Triage Comment]
Approved for esr78

Attachment #9183550 - Flags: approval-comm-esr78? → approval-comm-esr78+
Duplicate of this bug: 1666642

Unfortunately, this is still happening in 78.4.1

Status: RESOLVED → REOPENED
Resolution: FIXED → ---

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.

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

Status: REOPENED → RESOLVED
Closed: 11 months ago11 months ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.