New bluish icons (v.78.4.3) not nicely visible with dark theme on Manjaro Linux
Categories
(Thunderbird :: Theme, defect, P1)
Tracking
(thunderbird_esr78? fixed, thunderbird84? fixed)
People
(Reporter: porrumentzio, Assigned: aleca)
References
Details
(Keywords: access, regression)
Attachments
(3 files, 2 obsolete files)
7.38 KB,
image/png
|
Details | |
6.71 KB,
patch
|
aleca
:
review+
aleca
:
ui-review+
wsmwk
:
approval-comm-beta+
wsmwk
:
approval-comm-esr78+
|
Details | Diff | Splinter Review |
2.94 KB,
patch
|
darktrojan
:
review+
Paenglab
:
ui-review+
wsmwk
:
approval-comm-beta+
wsmwk
:
approval-comm-esr78+
|
Details | Diff | Splinter Review |
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•4 years ago
•
|
||
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.
Updated•4 years ago
|
Comment 2•4 years ago
|
||
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.
Assignee | ||
Comment 3•4 years ago
|
||
Is this 78.5?
78.4 was missing a line removeAttribute("brigthtext")
from the theme switcher.
Does this happen also on Daily?
Comment 4•4 years ago
|
||
Yes, also on Daily.
Assignee | ||
Updated•4 years ago
|
Assignee | ||
Comment 5•4 years ago
|
||
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•4 years ago
|
||
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.
Assignee | ||
Comment 7•4 years ago
|
||
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•4 years ago
|
||
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
Assignee | ||
Comment 9•4 years ago
|
||
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•4 years ago
|
||
Great teamwork everyone following my needinfo, keep going! You can fix this! ;-)
Updated•4 years ago
|
Assignee | ||
Comment 11•4 years ago
|
||
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.
Assignee | ||
Comment 12•4 years ago
|
||
This patch fixes it, but I'm not sure this is the right approach as it seems a bit cumbersome.
Comment 13•4 years ago
|
||
(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.
Assignee | ||
Comment 14•4 years ago
|
||
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•4 years ago
|
||
Comment 16•4 years ago
|
||
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.
Reporter | ||
Comment 17•4 years ago
|
||
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.
Assignee | ||
Comment 18•4 years ago
|
||
(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.
Assignee | ||
Comment 19•4 years ago
|
||
Assignee | ||
Updated•4 years ago
|
Comment 20•4 years ago
|
||
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
Updated•4 years ago
|
Assignee | ||
Comment 21•4 years ago
|
||
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.
Assignee | ||
Updated•4 years ago
|
Assignee | ||
Comment 22•4 years ago
|
||
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.
Comment 23•4 years ago
|
||
Comment on attachment 9189054 [details] [diff] [review]
1678029-performance-test.diff
LGTM.
Updated•4 years ago
|
Assignee | ||
Updated•4 years ago
|
Assignee | ||
Updated•4 years ago
|
Assignee | ||
Updated•4 years ago
|
Assignee | ||
Updated•4 years ago
|
Comment 25•4 years ago
|
||
Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/1218cc68c9ba
Follow up: fix performance issue. r=darktrojan
Assignee | ||
Comment 26•4 years ago
|
||
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.
Assignee | ||
Comment 27•4 years ago
|
||
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•4 years ago
|
||
Comment on attachment 9188953 [details] [diff] [review]
1678029-linux-dark.diff
[Triage Comment]
Approved for beta
Comment 29•4 years ago
|
||
Comment on attachment 9189054 [details] [diff] [review]
1678029-performance-test.diff
[Triage Comment]
Approved for beta
Comment 30•4 years ago
|
||
bugherder uplift |
Comment 31•4 years ago
|
||
Comment on attachment 9189054 [details] [diff] [review]
1678029-performance-test.diff
[Triage Comment]
Approved for esr78
Comment 32•4 years ago
|
||
Comment on attachment 9188953 [details] [diff] [review]
1678029-linux-dark.diff
[Triage Comment]
Approved for esr78
Comment 33•4 years ago
|
||
bugherder uplift |
Comment 34•4 years ago
|
||
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•4 years ago
|
||
Yes.
Description
•