Closed Bug 1740652 Opened 3 years ago Closed 2 years ago

LW Theme affects the entire UI regardless of the OS dark/light setting

Categories

(Thunderbird :: Theme, defect)

Thunderbird 96
defect

Tracking

(thunderbird_esr91 unaffected, thunderbird_esr102 unaffected)

RESOLVED FIXED
96 Branch
Tracking Status
thunderbird_esr91 --- unaffected
thunderbird_esr102 --- unaffected

People

(Reporter: t.matsuu, Assigned: Paenglab)

References

(Blocks 1 open bug)

Details

(Keywords: regression, regressionwindow-wanted)

Attachments

(6 files, 2 obsolete files)

I usually use "Fall Foggy Forest" theme.
If I set default app mode with dark on Windows OS, dark mode is applied to menu and massage list pane.
However folder pane and message header pane shows dark theme text-color (white) with light theme background (white).

Attached image Screenshot (deleted) —

Tb theme: "Fall Foggy Forest"
Windows theme: dark

Second line in message list pane is the selected mail.
Third one is the mail with mouse cursor hovered.

Attached image Screenshot (deleted) —

Tb theme: "System theme — auto"
Windows theme: dark

Second line in message list pane is the selected mail.
Third one is the mail with mouse cursor hovered.

This is because of the automatic system dark theme usage which was introduced in M-C. I'm on a patch but this needs some time because I have to remove the special Windows treechildren styling.

Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
Attached patch 1740652-dark-theme-LW-theme.patch (obsolete) (deleted) — Splinter Review

This fixes the most issues when using the dark system theme and a dark LW-theme.

On Mac there are mostly issues when TB don't have the focus.
On Windows I had to remove the special (Explorer) treechildren appearance. I hope we can go away from the old tree widget soon.

There where also some issues with not using the correct colours

Attachment #9250622 - Flags: review?(alessandro)

The Events views do still use the default (white) theme. I plan to fix this in a separate bug.

Is this in anyway related to the patch that emilio did for us on bug 1740089?

I'd say no. It happens already in 95. This is because the widgets now use the dark system theme colours. In 91 the system colours are always the one from the light theme on Mac/Windows. Now they behave more like the Linux theme did it since long time.

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

This is because of the automatic system dark theme usage which was introduced in M-C. I'm on a patch but this needs some time because I have to remove the special Windows treechildren styling.

Do you know which bug causes this regression?

Unfortunately, no.

Version: unspecified → Thunderbird 96
Comment on attachment 9250622 [details] [diff] [review] 1740652-dark-theme-LW-theme.patch Review of attachment 9250622 [details] [diff] [review]: ----------------------------------------------------------------- On Windows 10 with dark OS theme there are a few problems: - The reply/forward/etc icons in the message thread are dark when the row is selected. They should be white. - Same problem for the attachments icon in the message row. - The twisty icon to open/close the message thread is dark grey instead of light grey when the message is not selected. Everything else looks pretty good.
Attachment #9250622 - Flags: review?(alessandro) → feedback+
Attached patch 1740652-dark-theme-LW-theme.patch (obsolete) (deleted) — Splinter Review

This should fix both issues and additionally the IMIP bar.

Attachment #9250622 - Attachment is obsolete: true
Attachment #9250808 - Flags: review?(alessandro)
Comment on attachment 9250808 [details] [diff] [review] 1740652-dark-theme-LW-theme.patch Review of attachment 9250808 [details] [diff] [review]: ----------------------------------------------------------------- Very good work as this fixes almost all problems. I just have a few questions. The selected/hovered attachment item doesn't look good, but you can do that in a follow up patch if you want. ::: calendar/base/themes/osx/today-pane.css @@ +27,5 @@ > #today-pane-panel:not(:-moz-lwtheme):-moz-window-inactive { > background-color: hsl(0, 0%, 97%); > } > > +@media not (prefers-color-scheme: dark) { Is this correct? Don't we need the inactive color state also when not in dark variation? ::: mail/themes/shared/mail/messenger.css @@ +306,5 @@ > +@media (prefers-color-scheme: dark) { > +:root:-moz-lwtheme-brighttext .themeable-brighttext { > + --toolbarbutton-header-bordercolor: var(--lwt-toolbarbutton-hover-background, > + rgba(255, 255, 255, .25)); > + } messed up indentation? ::: mail/themes/windows/mail/messenger.css @@ -427,5 @@ > - treechildren::-moz-tree-row(tagged, selected, focus) { > - border-color: rgba(0,0,0,0.25); > - } > - > - @media (-moz-os-version: windows-win7), Are we sure we don't need these anymore? Just to be sure we won't start getting bug reports from win7 users
Attachment #9250808 - Flags: review?(alessandro) → feedback+

Another thing.
The fixes you did for the icons in the message thread work on Windows but not entirely on macOS as the twisty keys are still grey with the dark variation.

The commit message of the patch should also be changed since the changesets affect every platform and not just Windows.

Blocks: tb-dark-mode

(In reply to Alessandro Castellani [:aleca] from comment #12)

Comment on attachment 9250808 [details] [diff] [review]
1740652-dark-theme-LW-theme.patch

Review of attachment 9250808 [details] [diff] [review]:

Very good work as this fixes almost all problems.
I just have a few questions.

The selected/hovered attachment item doesn't look good, but you can do that
in a follow up patch if you want.

Should be fixed now.

::: calendar/base/themes/osx/today-pane.css
@@ +27,5 @@

#today-pane-panel:not(:-moz-lwtheme):-moz-window-inactive {
background-color: hsl(0, 0%, 97%);
}

+@media not (prefers-color-scheme: dark) {

Is this correct?
Don't we need the inactive color state also when not in dark variation?

Check the @media not (). The inactive background is only on light themes applied. With the dark TB theme there is also no inactive background colour.

::: mail/themes/shared/mail/messenger.css
@@ +306,5 @@

+@media (prefers-color-scheme: dark) {
+:root:-moz-lwtheme-brighttext .themeable-brighttext {

  • --toolbarbutton-header-bordercolor: var(--lwt-toolbarbutton-hover-background,
  •                                        rgba(255, 255, 255, .25));
    
  • }

messed up indentation?

It's aligned to be behind the var( like some lines above.

::: mail/themes/windows/mail/messenger.css
@@ -427,5 @@

  • treechildren::-moz-tree-row(tagged, selected, focus) {
  • border-color: rgba(0,0,0,0.25);
  • }
  • @media (-moz-os-version: windows-win7),

Are we sure we don't need these anymore?
Just to be sure we won't start getting bug reports from win7 users

I think it's better to use the same colours as we use for the other elements, like for example the new tree widget, when they are selected.

Attachment #9250808 - Attachment is obsolete: true
Attachment #9250950 - Flags: review?(alessandro)
Comment on attachment 9250950 [details] [diff] [review] 1740652-dark-theme-LW-theme.patch / landed in comment 16 Review of attachment 9250950 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, thanks. Let's mark this bug as leave open so you can add the follow up here for the calendar view.
Attachment #9250950 - Flags: review?(alessandro) → review+
Target Milestone: --- → 96 Branch

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/b1b7101781cd
Make the dark LW-themes work with the dark system themes again. r=aleca

BuildID 20211117104658 seems to work fine.

However 20211118105235 shows black text on white background even though message list pane. 😢
Theme image is applied but UI is same as light Tb theme.

Most likely the new issue is due to the changes in bug 1740089.

Emilio, was this done on purpose? A LW theme with OS dark mode not letting the rest of the UI follow the OS dark themeing?

Flags: needinfo?(emilio)
Summary: Color is broken with "Fall Foggy Forest" theme in some area → LW Theme affects the entire UI regardless of the OS dark/light setting

It seems the that LW theme now is the ruler of the whole UI.
So if the used LW Theme has a light toolabr, color contrast, or whatnot, the whole UI remains white. Using a LW Theme with a dark toolbar (like dark fox), forces the dark UI, even if the OS theme is set to light.

Is this on purpose?

(In reply to Alessandro Castellani [:aleca] from comment #19)

It seems the that LW theme now is the ruler of the whole UI.
So if the used LW Theme has a light toolabr, color contrast, or whatnot, the whole UI remains white. Using a LW Theme with a dark toolbar (like dark fox), forces the dark UI, even if the OS theme is set to light.

Ah, now I confirmed.
For "Fall Foggy Forest", dark UI is applied on Firefox, and light UI is applied on Thunderbird which is independent of the OS theme.

Yes, a dark UI theme will affect prefers-color-scheme everywhere, that was done a while ago (bug 1529323), but now we fall back to the frame color (in bug 1740089), which in that theme is "colors": { "frame": "#aca8a9", "tab_background_text": "#ffffff" }.

So it's expected that prefers-color-scheme: dark matches after that patch, what I don't understand is why the tree is light. I guess it's because all your selectors are using this attribute instead to determine a bunch of colors to use or what not?

Does a patch like this fix it? That'd fall back to the regular text color to compute the lwt-tree-* thing:

diff --git a/mail/themes/ThemeVariableMap.jsm b/mail/themes/ThemeVariableMap.jsm
--- a/mail/themes/ThemeVariableMap.jsm
+++ b/mail/themes/ThemeVariableMap.jsm
@@ -162,16 +162,17 @@ const ThemeVariableMap = [
     {
       lwtProperty: "sidebar",
     },
   ],
   [
     "--sidebar-text-color",
     {
       lwtProperty: "sidebar_text",
+      fallbackProperty: "textcolor",
       processColor(rgbaChannels, element) {
         if (!rgbaChannels) {
           element.removeAttribute("lwt-tree");
           element.removeAttribute("lwt-tree-brighttext");
           Services.prefs.setIntPref("browser.theme.toolbar-theme", 2);
           return null;
         }

However I'm confused, because I don't seem to be able to reproduce comment 0 on trunk. Is it just about installing that theme having OS dark mode? I get light UI here... what am I missing?

Flags: needinfo?(emilio) → needinfo?(alessandro)

(In reply to Emilio Cobos Álvarez (:emilio) from comment #21)

However I'm confused, because I don't seem to be able to reproduce comment 0 on trunk. Is it just about installing that theme having OS dark mode? I get light UI here... what am I missing?

Comment 0 is fixed with the patch landed in this bug.

Now that all system colours follow the system theme, like Linux does since long time, the lwt-default-theme-in-dark-mode on Mac and Windows could be dropped?

Attachment #9250950 - Attachment description: 1740652-dark-theme-LW-theme.patch → 1740652-dark-theme-LW-theme.patch / landed in comment 16

However I'm confused, because I don't seem to be able to reproduce comment 0 on trunk. Is it just about installing that theme having OS dark mode? I get light UI here... what am I missing?

The original report of this issue was fixed.

I think I'm missing something here. What is the correct approach and expected outcome of a LW theme?

Previous behavior
LW Theme only affects the toolbar and other chrome decoration, the content (message pane, sidebar, etc) is not affected by it and it follows the OS dark/light mode.

Current behavior
LW Theme affects everything, enforcing light/dark color variations regardless of the OS theming.

Which behavior should we support?
If we go with the latter, wouldn't be jarring for users used to use a LW theme that comes with lighter tones but with the dark content coming from the OS theming?

Flags: needinfo?(alessandro)

LW Theme affects everything, enforcing light/dark color variations regardless of the OS theming.

To be clear it does not enforce much. It changes prefers-color-scheme, and on chrome pages it changes the default color-scheme used as well, but you can override it using the color-scheme CSS property.

That said, that is the expected behavior by default, at least in terms of prefers-color-scheme. If the given theme has different ntp_text_color (newtab page text color) in a way that the new tab page should be dark, then it should use that to determine the content theme. The code is here, it can be tweaked if thunderbird has different needs somehow. However I'm still confused about this:

If we go with the latter, wouldn't be jarring for users used to use a LW theme that comes with lighter tones but with the dark content coming from the OS theming?

This should never happen? A light theme should end up with light content by default.

This should never happen? A light theme should end up with light content by default.

I'm with you on this in terms of confusion, since TB always behaved that way, meaning that no matter what LW theme was used, the tab content always ignored it and followed the OS theming, which it was always a pain to make it right.

Looks good with today's Daily. 😊

Regressions: 1742567

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/faa1e99a2c1d
Remove the scrollbar-color style which is no more needed. r=aleca

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/1de142e49940
Use the dark theme with dark system theme and dark LW-theme. r=aleca

Little CSS error with a wrongly written selector: :root:-moz-lwtheme-brighttext] treecol {
Here: https://searchfox.org/comm-central/rev/691d42940642ff075d8d9769c87f53e8e9c8e825/mail/themes/shared/mail/messenger.css#1040

Pushed by alessandro@thunderbird.net: https://hg.mozilla.org/comm-central/rev/f7faf67e6b54 Follow up fix typo in CSS selector. rs=bustage-fix DONTBUILD
Regressions: 1743440

Can this be closed?

Flags: needinfo?(mkmelin+mozilla)

I think so, but Richard is more on top of this.

Flags: needinfo?(mkmelin+mozilla) → needinfo?(richard.marti)

Yes, this bug should be done.

Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Flags: needinfo?(richard.marti)
Keywords: leave-open
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: