-moz-win-accentcolortext isn't always right

RESOLVED FIXED in Firefox 56

Status

()

Core
Widget: Win32
P1
normal
RESOLVED FIXED
a month ago
2 days ago

People

(Reporter: Benjamin Smedberg, Assigned: dao)

Tracking

({regression})

unspecified
mozilla56
regression
Points:
---
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox54 unaffected, firefox55 unaffected, firefox56 fixed)

Details

(Whiteboard: [photon-visual][p2])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

a month ago
Created attachment 8884395 [details]
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?
(Reporter)

Updated

a month ago
Whiteboard: [photon-visual]
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.
Flags: needinfo?(jwatt)
Whiteboard: [photon-visual] → [photon-visual][triage]
(Assignee)

Comment 2

a month ago
(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
(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.
Flags: needinfo?(jwatt)
(Assignee)

Comment 4

a month ago
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.
(Assignee)

Comment 5

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

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

Comment 6

a month ago
Created attachment 8884567 [details] [diff] [review]
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.
(Reporter)

Comment 7

a month ago
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.
Duplicate of this bug: 1379855
Duplicate of this bug: 1379853
(Assignee)

Updated

a month ago
Component: Theme → Widget: Win32
Flags: qe-verify+
Product: Firefox → Core
Summary: With Windows 10 accent color tabs, tab titles are hard to read → -moz-win-accentcolortext isn't always right
Whiteboard: [photon-visual][triage] → [photon-visual][p2]

Updated

a month ago
Priority: -- → P2
QA Contact: brindusa.tot

Comment 10

a month ago
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
(Assignee)

Comment 11

a month ago
(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.
(Assignee)

Updated

a month ago
Attachment #8884567 - Attachment is obsolete: true
(Assignee)

Updated

a month ago
Assignee: nobody → dao+bmo
Status: NEW → ASSIGNED
Priority: P2 → P1
Comment hidden (mozreview-request)

Updated

a month ago
Iteration: --- → 56.3 - Jul 24

Comment 13

a month ago
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

29 days ago
mozreview-review
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
Attachment #8887446 - Flags: review?(jmathies) → review+

Comment 15

29 days ago
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ed51d9dc55c3
Tweak -moz-win-accentcolortext formula to better match native title bars. r=jimm
(Assignee)

Comment 16

29 days ago
(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

28 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ed51d9dc55c3
Status: ASSIGNED → RESOLVED
Last Resolved: 28 days ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
status-firefox54: --- → unaffected
status-firefox55: --- → unaffected
status-firefox-esr52: --- → unaffected
QA Contact: brindusa.tot → ovidiu.boca
You need to log in before you can comment on or make changes to this bug.