1678029 - New bluish icons (v.78.4.3) not nicely visible with dark theme on Manjaro Linux

Status: RESOLVED FIXED

(Thunderbird :: Theme, defect, P1)

Description

[Porrumentzio]

Attachment: thunderbird-dark-icons.png

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:83.0) Gecko/20100101 Firefox/83.0

Steps to reproduce:

After upgrading Thunderbid to v.78.4.3 on Manjaro Linux, which ships dark theme for Thunderbird by default

Actual results:

Now the new dark-bluish icons for each email folder is not easily visible, as it does very little contrast with the dark background.
Also, those brighter icons for each account is not as easy as before.

Expected results:

Those icons should have more contrast, having a brighter color or a white border.

Comment 1

[Thomas D. (:thomas8)]

Richard, I believe the low-contrast icons are coming from a Linux distro, is that correct?
Don't think we are shipping that as Linux dark theme.

Comment 2

[Richard Marti (:Paenglab)]

Alessandro, something must have regressed the dark system theme detection. The folder icons are no more light with the dark system theme. Tried with TB78 and Daily on Ubuntu and Mint.

The last time I tested this it worked.

Comment 3

[Alessandro Castellani [:aleca]]

Is this 78.5?
78.4 was missing a line removeAttribute("brigthtext") from the theme switcher.
Does this happen also on Daily?

Comment 4

[Richard Marti (:Paenglab)]

Yes, also on Daily.

Comment 5

[Alessandro Castellani [:aleca]]

Pinging Alice hoping for some help in finding the regression window.

Set the OS on dark theme, launch TB = Thunderbird doesn't properly trigger the brighttext attribute.

This worked at least until a month ago and now it's broken on both daily and 78, so something must have been uplifted as well.

Comment 6

[Richard Marti (:Paenglab)]

Not sure Alice can test this on Linux as this is a Linux problem. Windows and Mac use the TB dark theme when the system theme is set to dark.

Comment 7

[Alessandro Castellani [:aleca]]

We handle the dark/light theme detection and variation in 3 places, which trigger at specific times during the loading and theme switching cycle.

When the sidebar text color changes due to OS change: https://searchfox.org/comm-central/rev/e3363365eed24f91277ac9946d8e61d357d8547d/mail/base/modules/ThemeVariableMap.jsm#158

When the theme changes: https://searchfox.org/comm-central/rev/e3363365eed24f91277ac9946d8e61d357d8547d/mail/base/content/msgMail3PaneWindow.js#481

When the window state and toolbar visibility changes: https://searchfox.org/comm-central/rev/e3363365eed24f91277ac9946d8e61d357d8547d/mail/base/content/toolbarIconColor.js#12

Comment 8

[Alice0775 White]

Ubuntu20.04, Settings > Appearance > Window color: Dark
Regression window:
https://hg.mozilla.org/comm-central/pushloghtml?fromchange=a888cc00988ff7b4a21dbd05178cd0d2a055b063&tochange=fa46aff614580410f276eee0fecf0013e11a670e
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=d95c612379c34bd08c24846a13f698c2f7ea3ebe&tochange=34d71b4a00863e7615463592662dfe362c4a993e

Comment 9

[Alessandro Castellani [:aleca]]

Thank you so much Alice.

I found the issue but not what caused it, yet.

Here: https://searchfox.org/comm-central/rev/e3363365eed24f91277ac9946d8e61d357d8547d/mail/base/modules/ThemeVariableMap.jsm#158
On macOs and Windows in dark mode, the rgbaChannels variable is defined, therefore we properly process the luminance and set the attributes.

On Linux, that variable is completely empty!

Comment 10

[Thomas D. (:thomas8)]

Great teamwork everyone following my needinfo, keep going! You can fix this! ;-)

Comment 11

[Alessandro Castellani [:aleca]]

I have a patch that seems to fix this, but it's not optimal as I'm running checks at every inferFromText() interaction.
I'd like to ping Tim hoping he can shed some light on some of my doubts.

Tim, do you know if there's an event we can observe on Linux when the theme changes at OS level? I was expecting windowlwthemeupdate to be triggered but I was wrong.
It also seems that the lwt-default-theme-in-dark-mode attribute is not set on Linux when the OS is dark but the theme is default.

The issue reported here was fixed in bug 1638233, and the mechanism was improved in bug 1668989, just for context.

Comment 12

[Alessandro Castellani [:aleca]]

Attachment: 1678029-linux-dark.diff

This patch fixes it, but I'm not sure this is the right approach as it seems a bit cumbersome.

Comment 13

[Tim Nguyen :ntim]

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

I have a patch that seems to fix this, but it's not optimal as I'm running checks at every inferFromText() interaction.
I'd like to ping Tim hoping he can shed some light on some of my doubts.

Tim, do you know if there's an event we can observe on Linux when the theme changes at OS level?

I would really recommend not using custom colors but rather system colors on Linux for the default browser theme, this ensures you'll get contrasting colors regardless the system theme.

Otherwise you could try watching the activate event, since it's probably the most common case to manually change the OS theme after blurring the application: https://searchfox.org/mozilla-central/rev/7b40f0b246ad0b54975b1525811f2ad599b95f33/browser/base/content/browser.js#8709 .

I was expecting windowlwthemeupdate to be triggered but I was wrong.

This event triggers when add-on manager theme changes, not when the OS one does. Both are sort of linked on macOS/Windows, but not Linux.

It also seems that the lwt-default-theme-in-dark-mode attribute is not set on Linux when the OS is dark but the theme is default.

Yes, this is only set when @media (prefers-color-scheme: dark) matches, but it doesn't match intentionally on Linux, because web pages have a certain expectation about dark themes (e.g. dark background, light text). Linux themes can be quite random (e.g. dark background & dark text, etc.), making good heuristics isn't really easy, so prefers-color-scheme is always considered as light or no-preference for compatibility.

Comment 14

[Alessandro Castellani [:aleca]]

Attachment: 1678029-linux-dark.diff

Thank you so much for the detailed explanation.

I updated the patch to only run those extra conditions on Linux, when the inferFromText() is activated, and if the sidebar text color actually changed.

These are all the covered scenarios:

• Default TB Theme + OS Dark theme = lwt-tree-brighttext + ui.systemUsesDarkTheme
• Default TB Theme + OS Light theme = nothing
• Dark or Light TB Theme + Any OS theme = TB Theme has priority

Comment 15

[Magnus Melin [:mkmelin]]

Comment on attachment 9188726 [details] [diff] [review]
1678029-linux-dark.diff

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

Looks ok to me, but didn't try it.

::: mail/base/content/toolbarIconColor.js
@@ +132,5 @@
> +    // the sidebar icons and properly update the brighttext attribute.
> +    if (
> +      reason == "activate" &&
> +      AppConstants.platform == "linux" &&
> +      Services.prefs.getCharPref("extensions.activeThemeID") ==

not sure this pref always is set, so probably best to add , ""  as default so it doesn't throw?

::: mail/base/modules/ThemeVariableMap.jsm
@@ +170,5 @@
> +          // themes, but in the case of a dark GTK theme, we need to detect the
> +          // text luminance to properly update the attributes.
> +          if (
> +            AppConstants.platform == "linux" &&
> +            Services.prefs.getCharPref("extensions.activeThemeID") ==

here too

Comment 16

[Richard Marti (:Paenglab)]

Comment on attachment 9188726 [details] [diff] [review]
1678029-linux-dark.diff

Works for me. When changing the system theme from dark to light the icons are wrong until the TB window is activated, no problem.

Comment 17

[Porrumentzio]

Hi, I'm the original author of the post! What a surprise to have so many responses!

The OS I am using is Manjaro Linux (an Arch-derived distro) with MATE (continuation of GNOME2) graphical environment.

Comment 18

[Alessandro Castellani [:aleca]]

(In reply to Porrumentzio from comment #17)

Hi, I'm the original author of the post! What a surprise to have so many responses!

Thanks for reporting this issue.
We have a patch that should land soon, meanwhile you can temporarily solve this problem by activating the Thunderbird dark theme from the Add-ons > Themes option page.

Comment 19

[Alessandro Castellani [:aleca]]

Attachment: 1678029-linux-dark.diff

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/9b90c7c2dc15
Fix dark theme variation for Linux OS. r=mkmelin, ui-r=Paenglab

Comment 21

[Alessandro Castellani [:aleca]]

Possible impact on performance: https://treeherder.mozilla.org/jobs?repo=comm-central&group_state=expanded&fromchange=5b69deb258bec457a5fd4f0df4b1c0a443e9c543&selectedTaskRun=M-XAaPE-QH2lGWagicqk6w.0
I'm investigating.

Comment 22

[Alessandro Castellani [:aleca]]

Attachment: 1678029-performance-test.diff

This follow up avoids the performance issue by storing a local cached value of the sidebar color, preventing unnecessary luminance calculation of UI pref changes if nothing changed.

I'm asking another ui-r to Richard just to be sure all the usual scenarios are still correct.

Try run: https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=4280084c949730b8c65f0803acefbc2a364951f3

Comment 23

[Geoff Lankow (:darktrojan)]

Comment on attachment 9189054 [details] [diff] [review]
1678029-performance-test.diff

LGTM.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/1218cc68c9ba
Follow up: fix performance issue. r=darktrojan

Comment 26

[Alessandro Castellani [:aleca]]

Comment on attachment 9188953 [details] [diff] [review]
1678029-linux-dark.diff

[Approval Request Comment]
Regression caused by (bug #): -
User impact if declined: Thunderbird UI doesn't follow the Operating System dark variation. Only Linux users are affected.
Testing completed (on c-c, etc.): on c-c
Risk to taking this patch (and alternatives if risky): low as it only affects Linux users and is necessary to guarantee proper contrast and readability.

Asking approvals for beta and 78 at the same time.

Comment 27

[Alessandro Castellani [:aleca]]

Comment on attachment 9189054 [details] [diff] [review]
1678029-performance-test.diff

[Approval Request Comment]
Same as above.
This follow up patch fixes a performance issue.

Comment 28

[Wayne Mery (:wsmwk)]

Comment on attachment 9188953 [details] [diff] [review]
1678029-linux-dark.diff

[Triage Comment]
Approved for beta

Comment 29

[Wayne Mery (:wsmwk)]

Comment on attachment 9189054 [details] [diff] [review]
1678029-performance-test.diff

[Triage Comment]
Approved for beta

Comment 30

[Dave Miller [:justdave]]

Thunderbird 84.0b2
https://hg.mozilla.org/releases/comm-beta/rev/8a341c45b167
https://hg.mozilla.org/releases/comm-beta/rev/5ebb024f8a66

Comment 31

[Wayne Mery (:wsmwk)]

Comment on attachment 9189054 [details] [diff] [review]
1678029-performance-test.diff

[Triage Comment]
Approved for esr78

Comment 32

[Wayne Mery (:wsmwk)]

Comment on attachment 9188953 [details] [diff] [review]
1678029-linux-dark.diff

[Triage Comment]
Approved for esr78

Comment 33

[Rob Lemley [:rjl]]

Thunderbird 78.5.1:
https://hg.mozilla.org/releases/comm-esr78/rev/fe220621a9a2
https://hg.mozilla.org/releases/comm-esr78/rev/7e4ec7705f6e

Comment 34

[juraj.masiar]

Hello,
Is this bug responsible for the new "blue" icon for the unread email folder (instead of normal bold) and "dark blue" for the new unread email folder?
https://i.imgur.com/hUfkdhQ.png
The change just landed in 78.5.1 and I'm trying to find the reason behind...
Thanks!

Comment 35

[Richard Marti (:Paenglab)]

Yes.