Closed Bug 1474110 Opened 2 years ago Closed 2 years ago

Choose browserAction text color among white and black, maximizing contrast

Categories

(WebExtensions :: Frontend, enhancement)

enhancement
Not set
normal

Tracking

(firefox63 fixed)

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: Oriol, Assigned: Oriol)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [design-decision-approved][browserAction])

Attachments

(1 file)

This was one of the proposals in bug 1424620 comment 3:

   By default, the text badge color should be chosen among white and black
   in order to maximize contrast.

   So if I use browser.browserAction.setBadgeBackgroundColor({color: "black"})
   then the text color should be white.
   And if I use browser.browserAction.setBadgeBackgroundColor({color: "white"})
   then the text color should be black.

   I don't think transparency can be properly taken into account, so I would
   just ignore the alpha channel and assume the background is fully opaque.

   https://www.w3.org/TR/WCAG20-TECHS/G18.html#G18-procedure can be used to
   choose between white and black. If L is the background luminance according
   to that algorithm, if 1.05*0.05 < (L+0.05)**2 then we should prefer black.

Was already approved in bug 1424620 comment 13.
Comment on attachment 8993146 [details]
Bug 1474110 - Choose browserAction text color among white and black, maximizing contrast

https://reviewboard.mozilla.org/r/257960/#review266346

Given I'm understanding the calculation I pointed out below, the addition of a comment, and some instruction for QA to give some visual verfication, I'm good with this.

::: browser/components/extensions/parent/ext-browserAction.js:637
(Diff revision 1)
> +        return channel / 12.92;
> +      }
> +      return ((channel + 0.055) / 1.055) ** 2.4;
> +    });
> +    let luminance = 0.2126 * r + 0.7152 * g + 0.0722 * b;
> +    let channel = 1.05 * 0.05 < (luminance + 0.05) ** 2 ? 0 : 255;

I'm getting a touch lost right here as this isn't (or doesn't seem to be) part of the explanation in the referenced document.

Specifically, what does "1.05 \* 0.05" represent in terms of luminance?

I take it as a luminance midpoint, and if the luminance of the color "(luminance + 0.05) \*\* 2" is higher than midpoint, the color is lighter, so we choose black for text.

Add a code comment to explain.
Attachment #8993146 - Flags: review?(mixedpuppy) → review+
Comment on attachment 8993146 [details]
Bug 1474110 - Choose browserAction text color among white and black, maximizing contrast

https://reviewboard.mozilla.org/r/257960/#review266356

::: browser/components/extensions/parent/ext-browserAction.js:637
(Diff revision 1)
> +        return channel / 12.92;
> +      }
> +      return ((channel + 0.055) / 1.055) ** 2.4;
> +    });
> +    let luminance = 0.2126 * r + 0.7152 * g + 0.0722 * b;
> +    let channel = 1.05 * 0.05 < (luminance + 0.05) ** 2 ? 0 : 255;

The luminance of black is 0, the luminance of white is 1, let `L` be the luminance of the background. Black will never be lighter than the background, so its contrast is `(L + 0.05) / 0.05`. White will never be darker than the background, so its contrast is `1.05 / (L + 0.05)`. That's all according to the linked algorithm. Then we want to maximize contrast, so black is chosen if `(L + 0.05) / 0.05 > 1.05 / (L + 0.05)`, i.e. `(L + 0.05)**2 > 1.05 * 0.05`.
Also cached white as the default text color for the default [0xd9, 0, 0, 255] background.
This avoids contrast calculations for add-ons that don't use setBadgeBackgroundColor.
Comment on attachment 8993146 [details]
Bug 1474110 - Choose browserAction text color among white and black, maximizing contrast

https://reviewboard.mozilla.org/r/257960/#review266388

Thanks!
(255, 'comparing with ssh://hg.mozilla.org/integration/autoland\nremote: Permission denied (publickey).', 'abort: no suitable response from remote hg!')
Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b8bdb82da4ac
Choose browserAction text color among white and black, maximizing contrast r=mixedpuppy
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b8bdb82da4ac
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Is manual testing required on this bug? If yes, please provide some STR and the proper extension(if required) or set the “qe-verify -“ flag.

Thanks!
Flags: needinfo?(oriol-bugzilla)
Covered by automated tests
Flags: needinfo?(oriol-bugzilla) → qe-verify-
I've added a note in https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/browserAction/setBadgeBackgroundColor, which seemed to be the best place to mention this. I didn't think it was really appropriate to make a change in the compat data here, but please let me know if you do!
Flags: needinfo?(oriol-bugzilla)
I would also mention this in getBadgeTextColor once it is documented.
The compat data for setBadgeBackgroundColor doesn't seem necessary with the note. But maybe I would include it in the compat data for getBadgeTextColor
Flags: needinfo?(oriol-bugzilla)
Thanks! I've mentioned this in https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/browserAction/getBadgeTextColor. I haven't added it the compat data, since I think the notes in the page should be enough.
You need to log in before you can comment on or make changes to this bug.