1747909 - Auto labels use white text over default GUI background until configured

Status: VERIFIED FIXED

(Thunderbird :: Message Reader UI, defect)

Description

[Léa Gris]

Attachment: unreadable.png

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:95.0) Gecko/20100101 Firefox/95.0

Steps to reproduce:

When X-Mozilla-Keys: key words are displayed in message header bar, these renders white text over light grey background in Linux default system colour theme.

To reproduce:
In Thunderbird Linux Ubuntu default theme using system colours:

1. Subscribe to RSS feed https://stackoverflow.com/feeds/tag?tagnames=bash&sort=newest
   When viewing RSS entries/posts, the labels in header like:
   X-Mozilla-Keys: linux bash oracle
   Keywords: linux, bash, oracle
   these are displayed as unreadable white text over light grey background.

Actual results:

Keywords/Keys displayed as unreadable white text over light grey background.

See attached screenshot portion.

Expected results:

Use readable contract like the default text colour.

Comment 1

[Léa Gris]

When a label has not been configured with a colour, the rendering of the text is considered with the background is RGB #000000 black and use a white colour for the text, but the background of the label is rendered the default GUI background tint which is usually light grey.

This is indeed a bug affecting all automatically created labels without a user configured colour.

This makes automatically created labels, totally illegible. (IMAPv4 or ATOM XML RSS tags or labels).

Comment 2

[Léa Gris]

Generated HTML code for an automatically-generated label

<label xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
  value="database"
  class="tagvalue"
  style="color: white; background-color: ;"
  aria-label="Étiquettes: database"
/>

Generated inline style-sheet

color: white;
background-color: ;

As can be seen, newly generated labels without a specifically configured colour, defines no background-color, but still programmatically sets a white text colour.

Could be fixed with:

A proper behaviour change would be to not set the CSS color when background-color is not set either.

So that when label colour is not defined, it renders with the default GUI's text and background colours.

Comment 3

[Richard Marti (:Paenglab)]

Attachment: 1747909-tag-black-text.patch

This fixes the issue by checking in isColorContrastEnough() if there is a background colour set. If not the default text colour (black) is used.

Comment 4

[Magnus Melin [:mkmelin]]

Comment on attachment 9257172 [details] [diff] [review]
1747909-tag-black-text.patch

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

::: mail/modules/TagUtils.jsm
@@ +108,5 @@
>  function isColorContrastEnough(aColor) {
> +  // Is a color set? If not, return "true" to use the default color.
> +  if (!aColor) {
> +    return true;
> +  }

This function is a bit funny, returning a Color() and not boolean. If anything it should return null for false. 
It's always checked for a falsy value though, so not clear to me how it fixes the bug.

Comment 5

[Richard Marti (:Paenglab)]

But all caller of isColorContrastEnough() seems to check for boolean, or not? See https://searchfox.org/comm-central/search?q=isColorContrastEnough&case=false&regexp=false&path=

Comment 6

[Léa Gris]

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

But all caller of isColorContrastEnough() seems to check for boolean, or not? See https://searchfox.org/comm-central/search?q=isColorContrastEnough&case=false&regexp=false&path=

The function is not funny at all, it always returns a boolean.

The other return point is this:

   return new Color(cR, cG, cB).isContrastRatioAcceptable(
     new Color(0, 0, 0),
     "AAA"
   );

It creates a new Color object instance from the R,G,B components of the label colour (which due to the 0 padding, makes it black when the background-color is not defined).
From this Color object instance, it calls the isContrastRatioAcceptable() method with a black Color object. The methods returns a boolean, so does the isColorContrastEnough method as well.

Comment 7

[Magnus Melin [:mkmelin]]

Comment on attachment 9257172 [details] [diff] [review]
1747909-tag-black-text.patch

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

Oh I see now. Makes sense! r=mkmelin

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/b221272a0b63
Use in Tags black text when no background color is set. r=mkmelin

Comment 9

[Richard Marti (:Paenglab)]

Comment on attachment 9257172 [details] [diff] [review]
1747909-tag-black-text.patch

[Approval Request Comment]
User impact if declined: unreadable RSS auto tags
Testing completed (on c-c, etc.): on c-c
Risk to taking this patch (and alternatives if risky): low

Comment 10

[Wayne Mery (:wsmwk)]

Comment on attachment 9257172 [details] [diff] [review]
1747909-tag-black-text.patch

[Triage Comment]
Approved for beta
Approved for esr91

Comment 11

[Rob Lemley [:rjl]]

Thunderbird 96.0b4:
https://hg.mozilla.org/releases/comm-beta/rev/06340a7526cd

Comment 12

[Léa Gris]

Attachment: Screenshot confirms successful fix as intended

This screenshot from Thunderbird Daily 97.0a1 (2022-01-04) (64-bit) just confirms the issue has been resolved with the intended result.

Comment 13

[Rob Lemley [:rjl]]

Thunderbird 91.5.0:
https://hg.mozilla.org/releases/comm-esr91/rev/5746d3089492