Closed Bug 1638233 Opened 5 years ago Closed 4 years ago

[Linux] Thunderbird side panel icons are broken with system dark theme

Categories

(Thunderbird :: Theme, defect, P1)

Tracking

(thunderbird_esr78 fixed, thunderbird78 wontfix, thunderbird79 wontfix, thunderbird80 fixed)

RESOLVED FIXED
81 Branch
Tracking Status
thunderbird_esr78 --- fixed
thunderbird78 --- wontfix
thunderbird79 --- wontfix
thunderbird80 --- fixed

People

(Reporter: asdfghrbljzmkd, Assigned: aleca)

References

(Blocks 1 open bug)

Details

(Keywords: regression, Whiteboard: [TM:78.2.0])

Attachments

(6 files, 2 obsolete files)

Attached image Dark theme (deleted) —

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

Steps to reproduce:

Launched TB 77 Beta with a fresh profile, and added one of my email addresses.

System is Xubuntu 20.04, tested with Adwaita/Adwaita Dark themes.

Actual results:

Icons in side pane are barely visible (dark blue on black)

Expected results:

Icons should be visible (maybe light blue for dark systems themes?)

Unrelated, but not sure if it's worth a new bug report or if it's already known, but the color in the active tab is missing until you switch tabs.

Attached image Light theme (icons are visible) (deleted) —

Image of light theme (icons are visible).

Component: Untriaged → Theme

This comes from https://searchfox.org/comm-central/rev/af886b93f1a520cb98cb6c0e3eda612a1caf2560/mail/themes/shared/mail/folderPane.css#6. It's a light black colour for light backgrounds. But Linux themes and Windows high contrast themes can have dark backgrounds without trigger [lwt-tree-brighttext]. I think we should only use a opacity here to make the icons less dark/light.

What do you think, Alessandro?

I think we should only use a opacity here to make the icons less dark/light.

I'm not sure that would be a good solution as we risk to lose too much contrast and the user won't be able to use the colors he wants with a fixed imposed opacity below 0.8

How are we handling the text color? Can we implement CSS variations in order to cover every scenario?

We use on trees the system colour -moz-FieldText.

On toolbars we detect the text colour through toolbarIconColor.js. Maybe something similar could be possible for the tree.

I see that the status of this bug is still unconfirmed. I can confirm that this bug still exist in Thunderbird 78.0 on Linux (Ubuntu with Adwaita dark).

Richard, is this something you might be able to tackle?

I tried it but wasn't successful.

I'll get a stab at this.

Status: UNCONFIRMED → NEW
Ever confirmed: true

The issue goes a little bit deeper as also some parts of the calendar are not properly styled.

The quick solution would be to tell the user to go to the Add-ons section and select the dark theme, but we should that automatically.
I have a dirty patch we could use as a starting point, which tackles also bug 1592210.

Assignee: nobody → alessandro
Status: NEW → ASSIGNED
Attached patch 1638233-folders-dark.diff (obsolete) (deleted) — Splinter Review

This is more a proof of concept than an actual patch.

We currently have a method in the ThemeVariableMap.jsm which runs at startup and whenever a theme is changed.
https://searchfox.org/comm-central/rev/eb69eff3ca16509af6cf8f2dd29be396bebbc14d/mail/base/modules/ThemeVariableMap.jsm#162

We could use it and check for the colour contrast of the first toolbar we pick.
There's a small problem sometimes where the method runs before the entire UI switched back to light, therefore failing the contrast check and still returning a dark UI.

I think, and this is just speculation, that we might improve this approach by forcing a theme switch to the dark variation in case the user has the default theme selected, in order to cover these scenarios:

  • The OS has a dark theme -> the theme is on default (not edited by the user) -> we add the brighttext attribute and the ui.systemUsesDarkTheme.
  • The OS has a dark theme -> the theme is not default (edited by the user) -> we don't do anything.
  • The user switches from default to dark -> we add the brighttext attribute and the ui.systemUsesDarkTheme.

Thoughts?

P.S.: I noticed that the observer in the toolbarIconColor.js basically runs on every window interaction (clicks, window drag, resize, etc.). We should look into that and limit it as I don't think that's good for performance.

Attachment #9165802 - Flags: feedback?(richard.marti)
Attachment #9165802 - Flags: feedback?(mkmelin+mozilla)
Comment on attachment 9165802 [details] [diff] [review] 1638233-folders-dark.diff Great that it changes the ui.systemUsesDarkTheme depending on the theme. But there are issues: - with the default Ubuntu theme, which has dark menu/tab toolbars but the rest light, it uses the dark setting. - opening the AB or the Composer shows errors and it resets to light.
Attachment #9165802 - Flags: feedback?(richard.marti) → feedback+

with the default Ubuntu theme, which has dark menu/tab toolbars but the rest light, it uses the dark setting.

We need an element that we can safely use to detect the color, do you have anything in mind?

opening the AB or the Composer shows errors and it resets to light.

Ah, I guess that method runs for every new window that is opened, which it shouldn't. Could we detect if we're dealing with the main window or a dialog?

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

with the default Ubuntu theme, which has dark menu/tab toolbars but the rest light, it uses the dark setting.

We need an element that we can safely use to detect the color, do you have anything in mind?

Maybe we should use the #folderTree. Check a toolbar text colour could be dangerous because when a dark LW-theme is active, the toolbar text could be light but the normal TB content uses still black text. The only toolbar we could use would be the #folderPane-toolbar but this isn't always shown. Also the window itself can't be used because of the LW-themes applying to it.

Attached patch 1638233-folders-dark.diff (obsolete) (deleted) — Splinter Review

Here's another try for this.
I did a bunch of changes, so here's a recap of the reasons why I implemented those things:

Remove extra calls
I explored the code a bit more thoroughly and discovered that the inferFromText() was getting called multiple times.
From here: https://searchfox.org/mozilla-central/rev/c6676771df58c6e0098574bc6b11517acbf264cf/toolkit/components/extensions/parent/ext-theme.js#398
And here: https://searchfox.org/comm-central/rev/caacc41c6d79e8e4015698bd23a60dbc5a957f1c/mail/base/content/msgMail3PaneWindow.js#2004

If I'm not wrong, all those calls are not necessary since we're relying on the "lightweight-theme-styling-update" observer triggered from the ext-theme.js.
I removed the extra call and I don't see any UI regression, but I might have missed something.

Prevent unnecessary attribute changes
The inferFromText() method runs a lot, like at every click, move, resize, or window opening, and it runs 2 or 3 times for every call (I'm still trying to understand why and if I can prevent that).
I implemented a kCurrentColor variable to store the most recent color from the main tabmail. If that color didn't change since the last time we called this method, I don't thing we should run it again because, hopefully, nothing changed and we don't need to update the attributes on all our toolbars.

Switching to ui.systemUsesDarkTheme
I'm using the ThemeVariableMap.jsm to detect the current color and update the attributes accordingly.
The processColor() method runs only at startup, or when a new window is opened, so if the user changes the OS theme to a dark theme, he'll need to restart TB to see all the changes properly applying.

This is not necessary if the user changes the TB built-in theme to Dark, fixing bug 1592210.

Let me know if you find anything sketchy, or if the performance gets affected by these changes.

Attachment #9165802 - Attachment is obsolete: true
Attachment #9165802 - Flags: feedback?(mkmelin+mozilla)
Attachment #9166371 - Flags: ui-review?(richard.marti)
Attachment #9166371 - Flags: review?(geoff)
Comment on attachment 9166371 [details] [diff] [review] 1638233-folders-dark.diff Work very good except when changing from a dark/light theme to a LW-theme. Then it uses the `ui.systemUsesDarkTheme` that the other theme has set before and not the system default. After a TB restart it's correct again. And when I change under Linux the system theme from bright to dark, the colours change but not `ui.systemUsesDarkTheme`.
Attachment #9166371 - Flags: ui-review?(richard.marti) → ui-review+

Yeah, that's a limitation of this implementation, as I wrote in comment 17:

if the user changes the OS theme to a dark theme, he'll need to restart TB to see all the changes properly applying.

I'm not sure if I can find a way to let it update dynamically without restart and without hogging the performance.

It's not so bad when a restart is needed. I don't think, changing the system theme is a regularly thing the user does.

Comment on attachment 9166371 [details] [diff] [review] 1638233-folders-dark.diff Review of attachment 9166371 [details] [diff] [review]: ----------------------------------------------------------------- This change looks okay to me, although my understanding of the code around it is a bit limited. ::: mail/base/modules/ThemeVariableMap.jsm @@ +175,5 @@ > + if (!tabmail) { > + return null; > + } > + > + let rgb = element.ownerDocument.ownerGlobal[0] This is weird. 1. You should probably use `tabmail` instead of `element`, although that would be the same because we know they're in the same document. 2. `.ownerDocument.ownerGlobal` is the same as just `.ownerGlobal`. 3. The `[0]` means you're actually using a *different* window from the one containing tabmail – the content of the first iframe inside it. I'm a little surprised that works.
Attachment #9166371 - Flags: review?(geoff) → review+
Attached patch 1638233-folders-dark.diff (deleted) — Splinter Review

Thanks for the review.
I still have 1 issue I'm not sure how to solve.

Switching from the built-in TB dark theme to default, doesn't properly update the ui.systemUsesDarkTheme, since the UI colors are updated slightly after the processColor() finished running, so it still detects the previous color scheme.
I'm not sure this is an acceptable behaviour.

The theme fixes itself after a restart.

Attachment #9166371 - Attachment is obsolete: true
Attachment #9166660 - Flags: review+
Attachment #9166660 - Flags: feedback?(richard.marti)
Attachment #9166660 - Flags: feedback?(geoff)
Comment on attachment 9166660 [details] [diff] [review] 1638233-folders-dark.diff I think, we can go with this because it is already a big improvement and deal with this issue in a follow-up bug. It would be great when we can eliminate the issue. A workaround would be when going from dark theme to default to use the light theme and then switch to default theme.
Attachment #9166660 - Flags: feedback?(richard.marti) → feedback+

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

Switching from the built-in TB dark theme to default, doesn't properly update the ui.systemUsesDarkTheme, since the UI colors are updated slightly after the processColor() finished running, so it still detects the previous color scheme.

If I understand correctly, what it boils down to is trying to find the CSS value of -moz-dialogtext. Is there some other way to do this? One way would be to find an element or property that doesn't change with the theme, but there may be some less hacky way.

(In reply to Richard Marti (:Paenglab) from comment #23)

A workaround would be when going from dark theme to
default to use the light theme and then switch to default theme.

I think this effectively happens if you disable the current theme to go back to the default, as opposed to enabling the default. Which is a bit weird and probably not at all useful, but I thought I'd mention it.

Comment on attachment 9166660 [details] [diff] [review] 1638233-folders-dark.diff I agree with Richard. Might as well ship this and tackle the other problem later.
Attachment #9166660 - Flags: feedback?(geoff) → feedback+

All right, thanks for the feedback.

I think this effectively happens if you disable the current theme to go back to the default.

Correct, when disabling the dark theme, TB first switches to light and then to default. We could force the same workflow when a user selects default.

If I understand correctly, what it boils down to is trying to find the CSS value of -moz-dialogtext. Is there some other way to do this? One way would be to find an element or property that doesn't change with the theme, but there may be some less hacky way.

I tried to find a way but I couldn't, probably due to my lack of knowledge of this area.

Since this issue affects 78, should we uplift?
Maybe we should let it sit on beta for a while and keep an eye on for any regression, and use the time to explore improvements.

Target Milestone: --- → 81 Branch

IMO this definitely needs uplift as it's a regression from the previous ESR, breaking TB out-of-the-box for many users (myself included).

As a temporary workaround you can set the Theme to dark, until this patch lands in the next release.

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/eb5bf594244d
Fix default color of folder icons with Adwaita Dark theme. r=darktrojan

Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED

Alessandro, do you plan to uplift this bug.

Flags: needinfo?(alessandro)

He's away this and next week, but yes we should uplift.

Comment on attachment 9166660 [details] [diff] [review]
1638233-folders-dark.diff

[Approval Request Comment]
User impact if declined: some parts of the openpgp UI unreadable, see bug 1658695 too

Flags: needinfo?(alessandro)
Attachment #9166660 - Flags: approval-comm-esr78?
Attachment #9166660 - Flags: approval-comm-beta?

Comment on attachment 9166660 [details] [diff] [review]
1638233-folders-dark.diff

[Triage Comment]
Approved for beta
Approved for esr78

Because this hasn't gone through beta - we need this explicitly tested with 78 candidate when it becomes available - will post on tb-planning (CCing some likely suspects)

Flags: needinfo?(wls220spring)
Flags: needinfo?(mkmelin+mozilla)
Flags: needinfo?(benc)
Attachment #9166660 - Flags: approval-comm-esr78?
Attachment #9166660 - Flags: approval-comm-esr78+
Attachment #9166660 - Flags: approval-comm-beta?
Attachment #9166660 - Flags: approval-comm-beta+
Flags: needinfo?(mkmelin+mozilla)

I updated my TB 80.0b3 to 80.0b4 on Ubuntu 18.04.5 LTS with the Adwaita OS theme.
Changed the TB Dark theme I prefer to the Default theme.
Restarted Thunderbird.
Opened Tweaks and changed to the Adwaita Dark theme.
TB 80.0b4 looked like the reporter's screenshot in the bug description.
Restarted TB 80.0b4 and all returned to looking good.

I think I was beginning to test this in the release candidate when I discovered the other theme bugs I filed and quit testing.
Will test again with the 78.2.0 rc.

Attached image debian-gnome-adwaita-dark.png (deleted) —

Dark theme looks OK on the build sent out via the mailing list (about window reports "78.2.0").
High contrast theme looks fine too.
(This is running under Gnome on Debian, switching the "Applications" appearance via gnome-tweaks)

There was an issue where switching between themes while Thunderbird was running left it a little confused - like the background colours changed, but a bunch of the icons and text seemed wrong.
Worked fine when you switch themes while thunderbird isn't running.

(all a bit alien to an I3 user like me :-) Themes and desktops are usually just things that happen to other people)

Hope this is what you were looking for - let me know if not and I can try some other stuff.

Flags: needinfo?(benc)

Oh - forgot to mention that the message display pane seems unaffected by theme changes. I guess HTML emails would be problematic, but I'd have thought text emails would follow the theme (ie light text on dark background).

The dark message display was reverted because of too much issues with HTML messages. Differ between plain text and HTML messages would also lead to reactions to: "Why are some messages dark and others not?" for users that don't understand the difference between plain and HTML.

Yes, a better solution for the message pane is tracked in bug 1657833.

The Folder Pane icons are still not visible with the Thunderbird Default theme, then just applying the Adwaita-dark operating system theme with Thunderbird open using:

Name Thunderbird
Version 78.2.0
Build ID 20200814165000

Also TB 80.0b5.

A restart of Thunderbird and they become visible.

Switching back to my default Adwaita theme with Thunderbird open, I lost all Folder pane and Thread Pane content and had to restart to recover the content.
Appears to work fine if you change operating system themes with Thunderbird closed.

Status: RESOLVED → VERIFIED
Flags: needinfo?(wls220spring)

Based on the results in comment 46 I'm going to reopen this bug.

I still see the same testing the current 78.2.0 release candidate build (20200821101218) on Ubuntu 18.04.5 LTS.

Status: VERIFIED → REOPENED
Resolution: FIXED → ---

(In reply to WaltS48 [:walts48] from comment #47)

Based on the results in comment 46 I'm going to reopen this bug.

I still see the same testing the current 78.2.0 release candidate build (20200821101218) on Ubuntu 18.04.5 LTS.

This has been reported in comment 22, in this quote:

Switching from the built-in TB dark theme to default, doesn't properly update the ui.systemUsesDarkTheme, since the UI colors are updated slightly after the processColor() finished running, so it still detects the previous color scheme.
I'm not sure this is an acceptable behaviour.
The theme fixes itself after a restart.

Both Geoff and Richard approved this and it's an expected behaviour that will be fixed in a follow up bug.

Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
Regressions: 1661229
Regressions: 1668896
Regressions: 1668989
Regressions: 1668991
Blocks: tb-dark-mode
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: