1379266 - -moz-win-accentcolortext isn't always right

Status: VERIFIED FIXED

(Core :: Widget: Win32, defect, P1)

Description

[Benjamin Smedberg]

Attachment: toolbar-active-inactive.png

After the landing of bug 1196266, my tabs coloring has several usability/polish problems. I'm going to file two separate bugs.

This one is about the readability of the tab contents. In particular:

1) the tab text
2) the tab "highlight" (the color that shows when a background tab title has changed)
3) the container tab highlight above the tab (this is least important because I don't think we're shipping container tabs as part of 56/57).

Screenshot attached.

I personally keep the main titlebar always visible so that I can drag windows properly, but the same basic problems here occur with or without the main titlebar.

I suggest that we need some combination of the following:
* make inactive tabs blend to grey rather than completely the background color: this is what Chrome does for example, and I find it reasonable but it it might not work as well in Firefox without a titlebar where the space between the top of the window and the tab is almost nonexistent.
* use the windows titlebar text color to draw the tab text.
* do something with the highlight coloring: make it light for dark colors and dark for light colors, maybe?

Comment 1

[Johann Hofmann [:johannh]]

Jonathan, the Windows accent color text in this case should be white. I can confirm that sometimes the accent color text is not switching correctly in Firefox, can you try debugging this? It's fairly easy to reproduce by randomly switching accent colors and watching the Firefox background tab text color.

As for the other issues I would defer that to the new Photon tab implementation, we shouldn't fix up the old Australis tabs.

Comment 2

[Dão Gottwald [::dao]]

(In reply to Johann Hofmann [:johannh] from comment #1)
> Jonathan, the Windows accent color text in this case should be white. I can
> confirm that sometimes the accent color text is not switching correctly in
> Firefox, can you try debugging this?

I don't think there's anything to debug. We don't get the text color from the OS but compute it ourselves:
https://hg.mozilla.org/mozilla-central/annotate/a4d028c197ef/widget/windows/nsLookAndFeel.cpp#l833

Comment 3

[Johann Hofmann [:johannh]]

(In reply to Dão Gottwald [::dao] from comment #2)
> (In reply to Johann Hofmann [:johannh] from comment #1)
> > Jonathan, the Windows accent color text in this case should be white. I can
> > confirm that sometimes the accent color text is not switching correctly in
> > Firefox, can you try debugging this?
> 
> I don't think there's anything to debug. We don't get the text color from
> the OS but compute it ourselves:
> https://hg.mozilla.org/mozilla-central/annotate/a4d028c197ef/widget/windows/
> nsLookAndFeel.cpp#l833

Oh, I just assumed this was a system value. Well, that's good. We just need to find the exact luminance threshold Windows applies and set it in our code.

Comment 4

[Dão Gottwald [::dao]]

In attachment 8884395 [details] the background is #0099BC and the calculated luminance is 123 (48%) so we pick black as the text color. http://serennu.com/colour/hsltorgb.php says #0099BC has a luminance of 37% so perhaps we should use a different formula.

Comment 5

[Dão Gottwald [::dao]]

Apparently this is the formula to get the HSL luminance for an RGB color:

(max(r, g, b) + min(r, g, b)) / 2;

Comment 6

[Dão Gottwald [::dao]]

Attachment: straw man patch

(In reply to Dão Gottwald [::dao] from comment #5)
> Apparently this is the formula to get the HSL luminance for an RGB color:
> 
> (max(r, g, b) + min(r, g, b)) / 2;

I tried this with a threshold of 110 rather than 127 and results seem worse, e.g. this picks white text on a yellow background.

Comment 7

[Benjamin Smedberg]

I spent a few minutes poking around MSDN and the web to see if we could get Windows to tell us what text color they are using, but I couldn't find anything definitively useful.

Comment 10

[Mark Straver]

After asking around the Windows dev forum at Microsoft, they do have a guideline up[1] for this calculation, but apparently there have been reports (MSDN/stack overflow) that it's not very accurate.

The guideline specifies:
bool colorIsDark = (5 * c.G + 2 * c.R + c.B) <= 8 * 128;

Maybe someone on Win 10 can see how well this measures up to what was added in Bug 1344910 in nsLookAndFeel::GetAccentColorText?

[1] https://docs.microsoft.com/en-us/windows/uwp/style/color

Comment 11

[Dão Gottwald [::dao]]

(In reply to Mark Straver from comment #10)
> After asking around the Windows dev forum at Microsoft, they do have a
> guideline up[1] for this calculation, but apparently there have been reports
> (MSDN/stack overflow) that it's not very accurate.
> 
> The guideline specifies:
> bool colorIsDark = (5 * c.G + 2 * c.R + c.B) <= 8 * 128;
> 
> Maybe someone on Win 10 can see how well this measures up to what was added
> in Bug 1344910 in nsLookAndFeel::GetAccentColorText?
> 
> [1] https://docs.microsoft.com/en-us/windows/uwp/style/color

Thanks! Just tested this, it makes us pick the same text colors as used in native title bars for all predefined accent colors.

Comment 12

[Dão Gottwald [::dao]]

Attachment: Bug 1379266 - Tweak -moz-win-accentcolortext formula to better match native title bars.

Review commit: https://reviewboard.mozilla.org/r/158282/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/158282/

Comment 13

[Mark Straver]

This might also apply to Windows 8's "darkwindowframe" calculation in browser.js.

Unfortunately, Bug 1279717 has split that out to color.jsm which is also used by other functions, meaning that if you'd change that formula to Microsoft's less relative-contrast-accurate, simple calculation, it would apply to everything that uses color.jsm -- probably not something you'd want?

Comment 14

[Jim Mathies [:jimm]]

Comment on attachment 8887446 [details]
Bug 1379266 - Tweak -moz-win-accentcolortext formula to better match native title bars.

https://reviewboard.mozilla.org/r/158282/#review164842

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ed51d9dc55c3
Tweak -moz-win-accentcolortext formula to better match native title bars. r=jimm

Comment 16

[Dão Gottwald [::dao]]

(In reply to Mark Straver from comment #13)
> This might also apply to Windows 8's "darkwindowframe" calculation in
> browser.js.

I won't bother changing anything there. It would be nice if we could get rid of that code and use -moz-win-accentcolor / -moz-win-accentcolortext on Windows 8.

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ed51d9dc55c3

Comment 18

[Brindusa Tot[:brindusat]]

Verified as fixed on Windows 10 x64 with latest Nightly 57.0a1 from 09/11/2017, as follows: I verified that for different accent colors, menu bar and tabs bar get the same color as the one set for Windows, and the text color on menu bar and tabs not active have the same text colors as used for native title bar windows (like file explorer window).

Comment 19

[Brindusa Tot[:brindusat]]

Sorry, updated the status-firefox56 by mistake.