1534360 - Tags: Black text color on blue background is hard to read

Status: RESOLVED FIXED

(Thunderbird :: Folder and Message Lists, defect)

Description

[allar]

Attachment: colors.png

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:65.0) Gecko/20100101 Firefox/65.0

Steps to reproduce:

Selected e-mail to add tag or select multiple emails with different tagas and colors.

Actual results:

Black text is not visible on colored background.

Expected results:

Text should be white (image example attached), selected green tag text is white as it should be. Other items are still black.

Comment 1

[Jorg K (CEST = GMT+2)]

Well, we have a contrast calculation here:
https://searchfox.org/comm-central/rev/1ed80b150864c0b6e953d3d76ca6e45b6753f3c6/mail/base/modules/TagUtils.jsm#88
but that has been somewhat suboptimal on various occasions.

I noticed that plain green, #FF0000, creates a white text, which is also not desirable.

Richard, can you play with this a bit?

Comment 2

[Richard Marti (:Paenglab)]

When I set https://searchfox.org/comm-central/rev/1ed80b150864c0b6e953d3d76ca6e45b6753f3c6/mail/base/modules/TagUtils.jsm#104 between 235 and 254 the green background gets a black foreground and the light background colours stay on black text. Only when I set blue (#0000FF) the text colour is still black, like already before.

You are mathematician, could we somehow weight the different channels to give blue a white text colour?

Comment 3

[Jorg K (CEST = GMT+2)]

Hmm, I need to look at the maths behind it, yes. As I said, the calculation comes from
https://searchfox.org/comm-central/rev/1ed80b150864c0b6e953d3d76ca6e45b6753f3c6/mail/base/modules/TagUtils.jsm#88
and here:
https://searchfox.org/mozilla-central/rev/358f816f63da072145c593e9e2ac36b7250ecd25/toolkit/modules/Color.jsm#80

Mike, do you know anything about this magic? And where does the 217 come from?
https://searchfox.org/mozilla-central/rev/358f816f63da072145c593e9e2ac36b7250ecd25/browser/modules/Windows8WindowFrameColor.jsm#41

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

When I set https://searchfox.org/comm-central/rev/1ed80b150864c0b6e953d3d76ca6e45b6753f3c6/mail/base/modules/TagUtils.jsm#104
between 235 and 254 the green background gets a black foreground and the light
background colours stay on black text. Only when I set blue (#0000FF) the text
colour is still black, like already before.

Well, in the original patch, you had 255. Back then, I looked at when the text goes white on a dark background. With 255 you get black text on very dark background which is not legible, with 217 is switches to white text even on not so dark background, see bug 1497041 comment #49.

Comment 6

[Honza Bambas (:mayhemer)]

There can be some CSS tricks: https://css-tricks.com/switch-font-color-for-different-backgrounds-with-css/
And there can be a JS calculation: https://stackoverflow.com/questions/3942878/how-to-decide-font-color-in-white-or-black-depending-on-background-color

Comment 7

[Mike de Boer [:mikedeboer]]

So yeah, I think using relativeLuminance and the strict WCAG guidelines as explained in the StackOverflow answer would be the way to go here.

In code:

Color.prototype = {
  //...
  get useBrightText() {
    return this.relativeLuminance <= 0.179;
  },
  //...
}

function isColorContrastEnough(aColor) {
  let bgColor = getColor(aColor);
  return !(new Color(...bgColor).useBrightText);
}

I named it 'useBrightText', because that's verbiage I see used in the context of Themes. You could change it to get useDarkText() this.relativeLuminance > 0.179, to match the StackOverflow post.

Comment 8

[Mike de Boer [:mikedeboer]]

Also, feel to use and cache Math.sqrt(1.05 * 0.05) - 0.05, instead of 0.179. Note that the other hard-coded values used here are already approximations.

Comment 9

[Jorg K (CEST = GMT+2)]

Attachment: 1534360-contrast.patch

Like so? I still get black on rather dark grey for RGB=(90,90,90). Going down to (89,89,89) gives me white foreground.

Mike, can you explain the 217 here:
https://searchfox.org/comm-central/rev/9bc5aee7e03b167f154af5011a9f6ab286ad7bf0/mozilla/browser/modules/Windows8WindowFrameColor.jsm#41

We copied Windows8WindowFrameColor and then refactored it:
https://searchfox.org/comm-central/rev/1ed80b150864c0b6e953d3d76ca6e45b6753f3c6/mail/base/modules/TagUtils.jsm#104
https://searchfox.org/comm-central/rev/9bc5aee7e03b167f154af5011a9f6ab286ad7bf0/mail/base/modules/Windows8WindowFrameColor.jsm#33

Comment 10

[Honza Bambas (:mayhemer)]

Comment on attachment 9055196 [details] [diff] [review]
1534360-contrast.patch

Can you provide a test build perhaps?

Comment 11

[Jorg K (CEST = GMT+2)]

I can, but I think we're still discussing it. This still doesn't appear to be right. Honza, thanks for being on the beta program and for providing the inspiration to get this bug rolling again. I had it on my to-do list. I'll be away on Wednesday, but we'll get it fixed for TB 67 beta 2.

Comment 12

[Richard Marti (:Paenglab)]

Comment on attachment 9055196 [details] [diff] [review]
1534360-contrast.patch

I see no change on my test colours. On a blue (#0000ff) background it's still black text.

Comment 13

[Jorg K (CEST = GMT+2)]

Comment on attachment 9055196 [details] [diff] [review]
1534360-contrast.patch

Yes, not very convincing. It must be related to the `217`.

Comment 14

[Mike de Boer [:mikedeboer]]

(In reply to Jorg K (GMT+2) from comment #9)

Mike, can you explain the 217 here:
https://searchfox.org/comm-central/rev/9bc5aee7e03b167f154af5011a9f6ab286ad7bf0/mozilla/browser/modules/Windows8WindowFrameColor.jsm#41

No I can't, but I'm sure Jared can! (He wrote it in bug 1004576.)

Comment 15

[Jared Wein [:jaws] (please needinfo? me)]

The 217 is mentioned in https://bugzilla.mozilla.org/show_bug.cgi?id=1004576#c13.

This is the RGB value (same value for all components) of the default window frame color on Windows 8. I don't have a local Windows 8 install anymore, but this screenshot shows it: https://techreport.com/r.x/2012_5_20_Windows_8_Desktop_to_ditch_Aero/win8-desktop-new.jpg

Using the Inspector tool on the background window's frame, I get #d9d9d9, which converts to rgb(217,217,217).

Comment 16

[Jorg K (CEST = GMT+2)]

Hmm, how can we move this forward:

Initially there was:
https://searchfox.org/mozilla-central/rev/14dc5b7d8a6da1854b2f9f33f1da77a97368cd54/browser/modules/Windows8WindowFrameColor.jsm#11
Three parameters are looked at there:
customizationColor which is used to calculate fgR, fgG, fgB, then there is colorizationColorBalance (magically 78 by default) which is used to calculate alpha and then there is frameBaseColor. All those three thrown together calculate some other colour.

Refactored it looks like this:
https://searchfox.org/comm-central/rev/1ed80b150864c0b6e953d3d76ca6e45b6753f3c6/mail/base/modules/TagUtils.jsm#104
https://searchfox.org/comm-central/rev/9bc5aee7e03b167f154af5011a9f6ab286ad7bf0/mail/base/modules/Windows8WindowFrameColor.jsm#33

Now, the refactored getColor() is also used in our contrast calculation, and there the magic 217 comes into play again. I guess that's wrong.

Comment 17

[Jorg K (CEST = GMT+2)]

Attachment: 1534360-contrast.patch (v2)

Can you also check that the Win8 stuff still works.

Comment 18

[Richard Marti (:Paenglab)]

Comment on attachment 9055530 [details] [diff] [review]
1534360-contrast.patch (v2)

Review of attachment 9055530 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with my proposed change.

You reverted Windows8WindowFrameColor.jsm to pre bug 1497041, correct? Tested on Win 8 and it works.

::: mail/base/modules/TagUtils.jsm
@@ +86,5 @@
> +  // Zero-pad the number just to make sure that it is 8 digits.
> +  let colorHex = ("00000000" + aColor).substr(-8);
> +  let colorArray = colorHex.match(/../g);
> +  let [, cR, cG, cB] = colorArray.map(val => parseInt(val, 16));
> +  return new Color(cR, cG, cB).relativeLuminance > 0.179;

When you change the line to:

return new Color(cR, cB, cG).relativeLuminance > 0.179;

it works with my blue tag too.

The cG and the cB need to be exchanged because Color.jsm uses not the normal RGB order:
https://searchfox.org/mozilla-central/rev/14dc5b7d8a6da1854b2f9f33f1da77a97368cd54/toolkit/modules/Color.jsm#32

Comment 19

[allar]

Attachment: Selected colors in Thunderbird 59

For example - colors in TB ver 59 attached.

Comment 20

[Jared Wein [:jaws] (please needinfo? me)]

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

The cG and the cB need to be exchanged because Color.jsm uses not the normal
RGB order:
https://searchfox.org/mozilla-central/rev/
14dc5b7d8a6da1854b2f9f33f1da77a97368cd54/toolkit/modules/Color.jsm#32

Wow, that is really unexpected. Mike, was there a reason you used rbg here?

Comment 21

[Jorg K (CEST = GMT+2)]

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

When you change the line to:
return new Color(cR, cB, cG).relativeLuminance > 0.179;
it works with my blue tag too.

Thanks for the review. The Color constructor takes RGB:
https://searchfox.org/mozilla-central/rev/14dc5b7d8a6da1854b2f9f33f1da77a97368cd54/toolkit/modules/Color.jsm#18
so this is the order I must pass them in.

There appears to be a bug in the luminance calculation:
https://searchfox.org/mozilla-central/rev/14dc5b7d8a6da1854b2f9f33f1da77a97368cd54/toolkit/modules/Color.jsm#32

We'll see what Mike has to say.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/24aecfef93d1
Restore Windows8WindowFrameColor back to the M-C version and fix contrast calculation. r=Paenglab

Comment 23

[Jorg K (CEST = GMT+2)]

We'll have to follow-up when M-C fixes the colour order. I've added a comment:
https://hg.mozilla.org/comm-central/rev/24aecfef93d1#l1.40

Comment 24

[Mike de Boer [:mikedeboer]]

(In reply to (behind on needinfos) Jared Wein [:jaws] (please needinfo? me) from comment #20)

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

The cG and the cB need to be exchanged because Color.jsm uses not the normal
RGB order:
https://searchfox.org/mozilla-central/rev/
14dc5b7d8a6da1854b2f9f33f1da77a97368cd54/toolkit/modules/Color.jsm#32

Wow, that is really unexpected. Mike, was there a reason you used rbg here?

No, that's a bug! And a glaring one at that... /me goes stand in a corner.

Comment 25

[Mike de Boer [:mikedeboer]]

Jorg, btw, you should always feel free to just fix the shared modules - I would've been happy to take it ;-)

Comment 26

[Jorg K (CEST = GMT+2)]

Hmm, I'm not sure what you're saying. I should have provided a patch for Color.jsm? Can you please fix it and I'll handle the C-C follow-up. We also need to have our fix here go to beta. Getting the M-C fix onto M-B is even more administrative work (not to say hassle).

Comment 27

[Mike de Boer [:mikedeboer]]

Suresure, I'm not saying I want to push more work onto your shoulders! I was merely trying to say that if you were at all hesitant to touch it, I'd gladly reaffirm that it's not a high bar. I'm going to file the bug and fix it though, because I want to make sure that its consumers also get the right behavior.

Comment 28

[Jorg K (CEST = GMT+2)]

Oh, we're fearless and never hesitant, but also pragmatic. For now, we fixed our contrast issue, and I prefer to leave the M-C work to the M-C people :-) - Thanks in advance.

Comment 29

[Jorg K (CEST = GMT+2)]

Comment on attachment 9055530 [details] [diff] [review]
1534360-contrast.patch (v2)

Of course we use the landed patch with B/G reversed.

Comment 30

[Jorg K (CEST = GMT+2)]

TB 67 beta 2:
https://hg.mozilla.org/releases/comm-beta/rev/9168a98c354379dda05d5f56cc6bf0584b8c611e