Closed Bug 1534360 Opened 3 years ago Closed 3 years ago

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

Categories

(Thunderbird :: Folder and Message Lists, defect)

defect
Not set
normal

Tracking

(thunderbird67 fixed, thunderbird68 fixed)

RESOLVED FIXED
Thunderbird 68.0
Tracking Status
thunderbird67 --- fixed
thunderbird68 --- fixed

People

(Reporter: allar, Assigned: jorgk-bmo)

References

Details

(Whiteboard: [regression:TB66?])

Attachments

(3 files, 1 obsolete file)

Attached image 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.

Summary: Tags text color → Tags: Black text color on blue background is hard to read

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?

Flags: needinfo?(richard.marti)

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?

Flags: needinfo?(richard.marti)

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.

Flags: needinfo?(mdeboer)
Duplicate of this bug: 1539245
Whiteboard: [regression:TB66?]
Duplicate of this bug: 1541018

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.

Flags: needinfo?(mdeboer)

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.

Attached patch 1534360-contrast.patch (obsolete) — Splinter Review
Assignee: nobody → jorgk
Attachment #9055196 - Flags: feedback?(richard.marti)
Attachment #9055196 - Flags: feedback?(mdeboer)
Attachment #9055196 - Flags: feedback?(honzab.moz)
Comment on attachment 9055196 [details] [diff] [review]
1534360-contrast.patch

Can you provide a test build perhaps?

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 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.
Attachment #9055196 - Flags: feedback?(richard.marti)
Comment on attachment 9055196 [details] [diff] [review]
1534360-contrast.patch

Yes, not very convincing. It must be related to the `217`.
Attachment #9055196 - Flags: feedback?(honzab.moz)

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

Flags: needinfo?(jaws)

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

Flags: needinfo?(jaws)

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.

Can you also check that the Win8 stuff still works.

Attachment #9055196 - Attachment is obsolete: true
Attachment #9055196 - Flags: feedback?(mdeboer)
Attachment #9055530 - Flags: review?(richard.marti)
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
Attachment #9055530 - Flags: review?(richard.marti) → review+

For example - colors in TB ver 59 attached.

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

Flags: needinfo?(mdeboer)

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

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

Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED

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

Target Milestone: --- → Thunderbird 68.0

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

Flags: needinfo?(mdeboer)

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

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

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.

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.

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

Of course we use the landed patch with B/G reversed.
Attachment #9055530 - Flags: approval-comm-beta+
You need to log in before you can comment on or make changes to this bug.