Closed Bug 1378320 Opened 4 years ago Closed 4 years ago

Use Windows 10 accent color in the title bar when in the foreground

Categories

(Thunderbird :: Theme, enhancement)

All
Windows 10
enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 56.0

People

(Reporter: Paenglab, Assigned: Paenglab)

Details

Attachments

(1 file, 9 obsolete files)

Now with bug 1344910 and bug 1344917 checked-in, we can set the Windows accent color to the titlebar when draw in titltebar is enabled.
Attached patch Win10titlebarColor.patch (obsolete) — Splinter Review
With this patch, when in the Windows Prefs the accent color for windows titlebars is enabled, TB uses this color instead of the light blue background color in the titlebar when drawintitlebar is enabled.
Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
Attachment #8883555 - Flags: review?(jorgk)
Attached patch Win10titlebarColor.patch (obsolete) — Splinter Review
Huh, completely wrong patch. Now the correct one.
Attachment #8883555 - Attachment is obsolete: true
Attachment #8883555 - Flags: review?(jorgk)
Attachment #8883557 - Flags: review?(jorgk)
Only for W10? Then the review will have to wait a week for when I'm back on my W10 machine ;-)
Yes, Win10 only. No problem, as long as it's in TB 59. ;-)
Attached patch Win10titlebarColor.patch (obsolete) — Splinter Review
I moved some code to make it simpler. Please can you test it also under Win7 as this reorganization also touches Win7?

FX also uses the accent color background with normal titlebar enabled. I'm following this now. There's now a bug open (bug 1378855) to revert this. When this happens, I'll update this patch with this small patch.
Attachment #8883557 - Attachment is obsolete: true
Attachment #8883557 - Flags: review?(jorgk)
Attachment #8884167 - Flags: review?(jorgk)
On Windows 7 I don't see any difference. That's intended, right?
Yes, then the reorganization changes are okay. In Spain you should then see the menubar and tabs toolbar in the same colour as the window titlebars. But this only when you have at least Win10 1607.
Attached patch Win10titlebarColor.patch (obsolete) — Splinter Review
As discussed through email apply the accent colour only when drawintitlebar is enabled.
Attachment #8884167 - Attachment is obsolete: true
Attachment #8884167 - Flags: review?(jorgk)
Attachment #8884528 - Flags: review?(jorgk)
OK, looking at this on Windows 7 now.

In my "basic" theme I see no difference. I can see that title bar, menu and tabbar all have the same light blue colour. Is this really desired? See below.

Switching to an Aero theme, I see that the patch makes a difference. Without the patch, the active tab is (almost) white and the menu below is light blue. With the patch, the active tab and the menu below have the same colour and it looks nicer. Title bar (transparent), tabbar (inactive tabs) and menu have different colours, which (from private e-mail) I understand is also what we aim for in Windows 10.

So r+ for the Win7 behaviour. Or wait ...

As I said above, in a Win 7 basic theme title bar, menu, and tabbar (inactive tabs) all have the same light blue colour. Which is what Firefox does. But do we need to follow this? Wouldn't different colours like for Windows 10 or Windows 7 Aero look so much nicer also for Windows 7 basic?
Attached patch Win10titlebarColor.patch (obsolete) — Splinter Review
How about this? It makes on Win7 and 8 the menu- and tabbar background grey when drawintitlebar is false like on Win10.
Attachment #8884528 - Attachment is obsolete: true
Attachment #8884528 - Flags: review?(jorgk)
Attachment #8884570 - Flags: review?(jorgk)
That looks better for Windows 7 with a basic theme. Aero theme unchanged, right?

Do you want me to test Win10 later or land it now?
Oh, I forgot: Do you feel responsible for this:
get/windows/nsLookAndFeel.cpp, line 818
[5580] WARNING: Using fallback for accent color - UI code failed to use the -moz-windows-accent-color-applies media query properly: file c:/mozilla-source/comm-central/mozilla/widget/windows/nsLookAndFeel.cpp, line 271
(In reply to Jorg K (GMT+2) from comment #11)
> That looks better for Windows 7 with a basic theme. Aero theme unchanged,
> right?

Correct, Aero stays transparent.

> Do you want me to test Win10 later or land it now?

We can land it and you check it later.

(In reply to Jorg K (GMT+2) from comment #12)
> Oh, I forgot: Do you feel responsible for this:
> get/windows/nsLookAndFeel.cpp, line 818
> [5580] WARNING: Using fallback for accent color - UI code failed to use the
> -moz-windows-accent-color-applies media query properly: file
> c:/mozilla-source/comm-central/mozilla/widget/windows/nsLookAndFeel.cpp,
> line 271

Me? Not at all. Do you see this during build or when running TB? When latter, do you see the same with FX? I'll attach a patch with a small change to a media query to test if it's caused by me.
Attached patch Win10titlebarColor.patch (obsolete) — Splinter Review
This should be the safer way for the media query.

If this still happens, it's not my fault:
(In reply to Jorg K (GMT+2) from comment #12)
> Oh, I forgot: Do you feel responsible for this:
> get/windows/nsLookAndFeel.cpp, line 818
> [5580] WARNING: Using fallback for accent color - UI code failed to use the
> -moz-windows-accent-color-applies media query properly: file
> c:/mozilla-source/comm-central/mozilla/widget/windows/nsLookAndFeel.cpp,
> line 271
Attachment #8884570 - Attachment is obsolete: true
Attachment #8884570 - Flags: review?(jorgk)
Attachment #8884576 - Flags: review?(jorgk)
Sorry, still the same message in the debug console.

Jonathan, what are we doing wrong to trigger this warning?
Flags: needinfo?(jwatt)
It means that this function is returning a failure code:

https://dxr.mozilla.org/mozilla-central/rev/13716663a04056df2777a19082b16bbcf05654b9/widget/windows/nsLookAndFeel.cpp#774

Most likely that's because the 'AccentColor' and 'ColorPrevalence' keys are not being found in the registry, which presumably means you're trying to use the accent color on a version of Windows prior to Windows 10. You can't do that because older versions of Windows don't have the concept of accent color, hence the warning.
Flags: needinfo?(jwatt)
Thanks, Jonathan. So we should only access those features on Windows 10 and later.
Actually from a cursory look it seems you are using the -moz-windows-accent-color-applies media query which should prevent you from getting this warning. I'm not sure why that's not working (or what you're doing differently to the Firefox UI). I'm afraid I won't be able to debug what's going on until I get back later this month though.
All the accentcolor code in my last patch is inside a Win10 and default-theme media queries and then in the -moz-windows-accent-color-applies media query. So it should never apply on Win7.

Jörg, are you sure you tested the last patch?
(In reply to Richard Marti (:Paenglab) from comment #19)
> Jörg, are you sure you tested the last patch?
I've just reimported it and I can see this
 @media (-moz-os-version: windows-win10) and (-moz-windows-default-theme) {
+  @media (-moz-windows-accent-color-applies) {

and this

@media (-moz-os-version: windows-win10) {
...
+    @media (-moz-windows-default-theme) {
+      @media (-moz-windows-accent-color-applies) {

but I still get:
[3160] WARNING: Using fallback for accent color - UI code failed to use the -moz-windows-accent-color-applies media query properly: file c:/mozilla-source/comm-central/mozilla/widget/windows/nsLookAndFeel.cpp, line 271
What happens when you remove the -moz-windows-accent-color-applies media query rules with their content and leave the rest of the patch?
Attached patch Win10titlebarColor.patch - v7 (obsolete) — Splinter Review
Updated the patch after landing of bug 1379938. But this one has a problem, the accent color is no more shown.

Jörg, with the M-C patch it should no more show a warning on Win7 as this is removed: https://hg.mozilla.org/mozilla-central/rev/5a5d8de65b70#l7.12
Attachment #8884576 - Attachment is obsolete: true
Attachment #8884576 - Flags: review?(jorgk)
Attachment #8886258 - Flags: review?(jorgk)
So I guess there is nothing to review here until the NI in bug 1379938 is resolved.
Comment on attachment 8886258 [details] [diff] [review]
Win10titlebarColor.patch - v7

Please let me know when I should review this. Clearing the request for now.
Attachment #8886258 - Flags: review?(jorgk)
Attached patch Win10titlebarColor.patch (obsolete) — Splinter Review
This patch is compatible with the actual toolkit query implementation. I think we can use this until the query renaming lands, which isn't clear when this happens as since the backout there is no activity in it.

About your warning you saw, there is now bug 1381496 for it.
Attachment #8886258 - Attachment is obsolete: true
Attachment #8887125 - Flags: review?(jorgk)
Well, I'll wait a little. Clearly bug 1379938 will rename the -moz-windows-accent-color-applies, so then we have to follow again.
Attached patch Win10titlebarColor.patch (obsolete) — Splinter Review
Sorry to spam you but I found during working on bug 1381833 that on Win8 the menu text isn't white when darkframe is true.

My added code is:

+  @media (-moz-windows-default-theme) {
+    #messengerWindow[darkwindowframe="true"]:not(:-moz-lwtheme):not(:-moz-window-inactive) >
+      #navigation-toolbox {
+      color: white;
+    }
+  }

which FX has likewise implemented.
Attachment #8887125 - Attachment is obsolete: true
Attachment #8887125 - Flags: review?(jorgk)
Attachment #8887530 - Flags: review?(jorgk)
(In reply to Richard Marti (:Paenglab) from comment #27)
> Sorry to spam you but I found during working on bug 1381833 that on Win8 the
> menu text isn't white when darkframe is true.
I'm not reviewing this until we get to version 10. This is only version 9 so far.
Duplicate of this bug: 1382039
Bug 1379938 landed, so I'm ready for version 10 ;-)
Okay, version 10.
Attachment #8887530 - Attachment is obsolete: true
Attachment #8887530 - Flags: review?(jorgk)
Attachment #8888106 - Flags: review?(jorgk)
Attachment #8886258 - Attachment description: Win10titlebarColor.patch → Win10titlebarColor.patch - v7
Comparing with v7
https://bugzilla.mozilla.org/attachment.cgi?oldid=8886258&action=interdiff&newid=8888106&headers=1
I noticed that you moved one block from primaryToolbar.css to mailWindow1.css. Why?

Also, how do I test this, what do I look for? I recently upgraded to W10 1703.
(In reply to Jorg K (GMT+2) from comment #32)
> Also, how do I test this, what do I look for? I recently upgraded to W10
> 1703.
I compared an older Daily to a local build with this patch and saw no difference, neither with or without tabs in the titlebar.
(In reply to Jorg K (GMT+2) from comment #32)
> Comparing with v7
> https://bugzilla.mozilla.org/attachment.
> cgi?oldid=8886258&action=interdiff&newid=8888106&headers=1
> I noticed that you moved one block from primaryToolbar.css to
> mailWindow1.css. Why?

It's to place the corresponding rules together.

> Also, how do I test this, what do I look for? I recently upgraded to W10
> 1703.

In the Windows prefs go to the color settings and tick "Apply accent color to title bars" (I have a German Windows, not sure how it's spelled in English Windows). Then, when you have set mail.tabs.drawInTitlebar to true, the titlebar should get the accent color. With mail.tabs.drawInTitlebar set to false the menu/tabbar should be always grey.

With unticked "Apply accent color to title bars" and mail.tabs.drawInTitlebar true the menubar/tabbar shoUld be light blue
Comment on attachment 8888106 [details] [diff] [review]
Win10titlebarColor.patch

(In reply to Richard Marti (:Paenglab) from comment #34)
> In the Windows prefs go to the color settings and tick "Apply accent color
> to title bars" (I have a German Windows, not sure how it's spelled in
> English Windows). Then, when you have set mail.tabs.drawInTitlebar to true,
> the titlebar should get the accent color.
You mean: The menu/tabbar should get the accent colour, right? In this case there is no titlebar, so the colour moves to the next bar down.

It does that and looks pretty ugly ;-)
Attachment #8888106 - Flags: review?(jorgk) → review+
(In reply to Jorg K (GMT+2) from comment #35)
> 
> It does that and looks pretty ugly ;-)

Yeah, depending on the accent color. On Win8 from beginning it's looking the same. Also on Win7 Aero it's the same, the only difference is the Glass effect.
Keywords: checkin-needed
Actually the 55.0b2 problem exists also in W7 including the glass/transparency bug here (W7SP1 on 4k Aero). Espicially bad when TB menu is out of focus for people with limited eyesight: they can not identify any menu any more. The focus idea is bad anyway. No serious applications does that.

I am asking myself: why would a TB relase ever need to override a personalized css? This seems to be the core of the problem. I really would love to see a policy: "do not change look and feel if there is a previous working setting that the user has deliberately chosen". (In many cases on purpose and with a lot of effort.)

Many users feel uncomfortable when a TB release ignores their preferences and forces them to use something else out of the blue sky. No wonder some users are so frustrated, e.g. when they have limited eyesight and put in hours to customize TB, just to find that some freaky release has killed all the efforts - again. Sadly this annoyance is deeply rooted in the history of TB releases since 1.0 so I have no hopes for change in the core policy.
https://hg.mozilla.org/comm-central/rev/0406339439ffb98b60c17f41a1529fc1f8fb5073
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 56.0
You need to log in before you can comment on or make changes to this bug.