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

Status: RESOLVED FIXED

(Thunderbird :: Theme, enhancement)

Description

[Richard Marti (:Paenglab)]

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.

Comment 1

[Richard Marti (:Paenglab)]

Attachment: Win10titlebarColor.patch

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.

Comment 2

[Richard Marti (:Paenglab)]

Attachment: Win10titlebarColor.patch

Huh, completely wrong patch. Now the correct one.

Comment 3

[Jorg K (CEST = GMT+2)]

Only for W10? Then the review will have to wait a week for when I'm back on my W10 machine ;-)

Comment 4

[Richard Marti (:Paenglab)]

Yes, Win10 only. No problem, as long as it's in TB 59. ;-)

Comment 5

[Richard Marti (:Paenglab)]

Attachment: Win10titlebarColor.patch

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.

Comment 6

[Jorg K (CEST = GMT+2)]

On Windows 7 I don't see any difference. That's intended, right?

Comment 7

[Richard Marti (:Paenglab)]

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.

Comment 8

[Richard Marti (:Paenglab)]

Attachment: Win10titlebarColor.patch

As discussed through email apply the accent colour only when drawintitlebar is enabled.

Comment 9

[Jorg K (CEST = GMT+2)]

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?

Comment 10

[Richard Marti (:Paenglab)]

Attachment: Win10titlebarColor.patch

How about this? It makes on Win7 and 8 the menu- and tabbar background grey when drawintitlebar is false like on Win10.

Comment 11

[Jorg K (CEST = GMT+2)]

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?

Comment 12

[Jorg K (CEST = GMT+2)]

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

Comment 13

[Richard Marti (:Paenglab)]

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

Comment 14

[Richard Marti (:Paenglab)]

Attachment: Win10titlebarColor.patch

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

Comment 15

[Jorg K (CEST = GMT+2)]

Sorry, still the same message in the debug console.

Jonathan, what are we doing wrong to trigger this warning?

Comment 16

[Jonathan Watt [: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.

Comment 17

[Jorg K (CEST = GMT+2)]

Thanks, Jonathan. So we should only access those features on Windows 10 and later.

Comment 18

[Jonathan Watt [:jwatt]]

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.

Comment 19

[Richard Marti (:Paenglab)]

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?

Comment 20

[Jorg K (CEST = GMT+2)]

(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

Comment 21

[Richard Marti (:Paenglab)]

What happens when you remove the -moz-windows-accent-color-applies media query rules with their content and leave the rest of the patch?

Comment 22

[Richard Marti (:Paenglab)]

Attachment: Win10titlebarColor.patch - v7

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

Comment 23

[Jorg K (CEST = GMT+2)]

So I guess there is nothing to review here until the NI in bug 1379938 is resolved.

Comment 24

[Jorg K (CEST = GMT+2)]

Comment on attachment 8886258 [details] [diff] [review]
Win10titlebarColor.patch - v7

Please let me know when I should review this. Clearing the request for now.

Comment 25

[Richard Marti (:Paenglab)]

Attachment: Win10titlebarColor.patch

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.

Comment 26

[Jorg K (CEST = GMT+2)]

Well, I'll wait a little. Clearly bug 1379938 will rename the -moz-windows-accent-color-applies, so then we have to follow again.

Comment 27

[Richard Marti (:Paenglab)]

Attachment: Win10titlebarColor.patch

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.

Comment 28

[Jorg K (CEST = GMT+2)]

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

Comment 30

[Jorg K (CEST = GMT+2)]

Bug 1379938 landed, so I'm ready for version 10 ;-)

Comment 31

[Richard Marti (:Paenglab)]

Attachment: Win10titlebarColor.patch

Okay, version 10.

Comment 32

[Jorg K (CEST = GMT+2)]

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.

Comment 33

[Jorg K (CEST = GMT+2)]

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

Comment 34

[Richard Marti (:Paenglab)]

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

[Jorg K (CEST = GMT+2)]

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 ;-)

Comment 36

[Richard Marti (:Paenglab)]

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

Comment 37

[Peer Tavori]

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.

Comment 38

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/0406339439ffb98b60c17f41a1529fc1f8fb5073