Closed Bug 1678029 Opened 4 years ago Closed 4 years ago

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

Categories

(Thunderbird :: Theme, defect, P1)

Unspecified
Linux

Tracking

(thunderbird_esr78? fixed, thunderbird84? fixed)

RESOLVED FIXED
85 Branch
Tracking Status
thunderbird_esr78 ? fixed
thunderbird84 ? fixed

People

(Reporter: porrumentzio, Assigned: aleca)

References

Details

(Keywords: access, regression)

Attachments

(3 files, 2 obsolete files)

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.

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.

Flags: needinfo?(richard.marti)
Keywords: access
OS: Unspecified → Linux
Summary: [ACCESSIBILITY] New bluish icons (v.78.4.3) not nicely visible with dark theme → New bluish icons (v.78.4.3) not nicely visible with dark theme on Manjaro Linux

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.

Flags: needinfo?(richard.marti) → needinfo?(alessandro)

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

Flags: needinfo?(alessandro)

Yes, also on Daily.

Assignee: nobody → alessandro
Status: UNCONFIRMED → ASSIGNED
Type: enhancement → defect
Ever confirmed: true
Priority: -- → P1

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.

Flags: needinfo?(alice0775)

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.

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

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!

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

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.

Flags: needinfo?(ntim.bugs)
Attached patch 1678029-linux-dark.diff (obsolete) — Splinter Review

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

Attachment #9188710 - Flags: ui-review?(richard.marti)
Attachment #9188710 - Flags: review?(mkmelin+mozilla)

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

Flags: needinfo?(ntim.bugs)
Attached patch 1678029-linux-dark.diff (obsolete) — Splinter Review

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
Attachment #9188710 - Attachment is obsolete: true
Attachment #9188710 - Flags: ui-review?(richard.marti)
Attachment #9188710 - Flags: review?(mkmelin+mozilla)
Attachment #9188726 - Flags: ui-review?(richard.marti)
Attachment #9188726 - Flags: review?(mkmelin+mozilla)
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
Attachment #9188726 - Flags: review?(mkmelin+mozilla) → review+

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.

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

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.

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

Attachment #9188726 - Attachment is obsolete: true
Attachment #9188953 - Flags: ui-review+
Attachment #9188953 - Flags: review+

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/9b90c7c2dc15
Fix dark theme variation for Linux OS. r=mkmelin, ui-r=Paenglab

Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 85 Branch
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

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

Attachment #9189054 - Flags: ui-review?(richard.marti)
Attachment #9189054 - Flags: review?(geoff)

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

LGTM.

Attachment #9189054 - Flags: review?(geoff) → review+
Attachment #9189054 - Flags: ui-review?(richard.marti) → ui-review+
Attachment #9188953 - Attachment filename: 1678029-linux-dark.diff → 1678029-linux-dark.diff [LANDED ON C-C]
Attachment #9188953 - Attachment description: 1678029-linux-dark.diff → 1678029-linux-dark.diff [LANDED ON C-C]
Attachment #9188953 - Attachment filename: 1678029-linux-dark.diff [LANDED ON C-C] → 1678029-linux-dark.diff

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/1218cc68c9ba
Follow up: fix performance issue. r=darktrojan

Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED

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.

Attachment #9188953 - Attachment description: 1678029-linux-dark.diff [LANDED ON C-C] → 1678029-linux-dark.diff
Attachment #9188953 - Flags: approval-comm-esr78?
Attachment #9188953 - Flags: approval-comm-beta?

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.

Attachment #9189054 - Flags: approval-comm-esr78?
Attachment #9189054 - Flags: approval-comm-beta?

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

[Triage Comment]
Approved for beta

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

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

[Triage Comment]
Approved for beta

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

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

[Triage Comment]
Approved for esr78

Attachment #9189054 - Flags: approval-comm-esr78? → approval-comm-esr78+

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

[Triage Comment]
Approved for esr78

Attachment #9188953 - Flags: approval-comm-esr78? → approval-comm-esr78+

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!

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: