Closed
Bug 1474110
Opened 6 years ago
Closed 6 years ago
Choose browserAction text color among white and black, maximizing contrast
Categories
(WebExtensions :: Frontend, enhancement)
WebExtensions
Frontend
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 hidden (mozreview-request) |
Comment 2•6 years ago
|
||
mozreview-review |
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+
Assignee | ||
Comment 3•6 years ago
|
||
mozreview-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`.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 6•6 years ago
|
||
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 7•6 years ago
|
||
mozreview-review |
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!
Assignee | ||
Updated•6 years ago
|
Keywords: checkin-needed,
dev-doc-needed
Comment 8•6 years ago
|
||
(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
Comment 10•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b8bdb82da4ac
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox63:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Comment 11•6 years ago
|
||
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)
Assignee | ||
Comment 12•6 years ago
|
||
Covered by automated tests
Flags: needinfo?(oriol-bugzilla) → qe-verify-
Comment 13•6 years ago
|
||
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)
Assignee | ||
Comment 14•6 years ago
|
||
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)
Comment 15•6 years ago
|
||
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.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•