Closed
Bug 1279717
Opened 8 years ago
Closed 8 years ago
Highlight all is low contrast in certain text/background color case
Categories
(Toolkit :: Find Toolbar, defect)
Tracking
()
Tracking | Status | |
---|---|---|
firefox49 | --- | unaffected |
firefox50 | --- | verified |
People
(Reporter: alice0775, Assigned: mikedeboer)
References
Details
(Keywords: polish, regression)
Attachments
(3 files, 1 obsolete file)
Steps to reproduce: 1. Open attached 2. Ctrl+F 3. Input Mozilla Actual Results: Highlight all is low contrast Expected Results: Highlight all should be highly noticeable
Reporter | ||
Updated•8 years ago
|
Summary: Highlight all is low contrast in certain case → Highlight all is low contrast in certain text/background color case
Comment 1•8 years ago
|
||
[Tracking Requested - why for this release]: Regression
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Iteration: --- → 50.1
Points: --- → 2
Flags: qe-verify+
Flags: firefox-backlog+
Assignee | ||
Comment 2•8 years ago
|
||
Attachment #8763049 -
Flags: review?(jaws)
Comment 3•8 years ago
|
||
Comment on attachment 8763049 [details] [diff] [review] Patch v1: inverse the mask to use a white background on pages with bright text color Review of attachment 8763049 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/modules/FinderHighlighter.jsm @@ +520,5 @@ > + * > + * @param {String} cssColor RGB color value in the default format rgb[a](r,g,b) > + * @return {Boolean} > + */ > + _isTextColorBright(cssColor) { We shouldn't be implementing this here. We have similar code that computes brighttext for themes. Please find a way to share that code. See http://searchfox.org/mozilla-central/rev/970569ad57ac4436ff31aa2ac63f38ed3ee2932d/browser/base/content/browser.js#7811
Attachment #8763049 -
Flags: review?(jaws) → review-
Assignee | ||
Comment 4•8 years ago
|
||
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #3) > See > http://searchfox.org/mozilla-central/rev/ > 970569ad57ac4436ff31aa2ac63f38ed3ee2932d/browser/base/content/browser.js#7811 That's where I blatantly copied it from! :) I'll put it in a module and change the code there too to use it.
Comment 5•8 years ago
|
||
Comment on attachment 8763049 [details] [diff] [review] Patch v1: inverse the mask to use a white background on pages with bright text color Review of attachment 8763049 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/modules/RemoteFinder.jsm @@ +309,5 @@ > this._finder.requestMatchesCount(data.searchString, data.matchLimit, data.linksOnly); > break; > > case "Finder:ModalHighlightChange": > + this._finder.onModalHighlightChange(data.useModalHighlight); What's up with this rename from .ModalHighlightChange to .onModalHighlightChange? I don't see any other renames for this in this patch.
Assignee | ||
Comment 6•8 years ago
|
||
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #5) > What's up with this rename from .ModalHighlightChange to > .onModalHighlightChange? I don't see any other renames for this in this > patch. Well, this is a syntax error: .ModalHighlightChange doesn't exist! .onModalHighlightChange does... The fact that I opted to fix this here is co-incidental.
Comment 7•8 years ago
|
||
(In reply to Mike de Boer [:mikedeboer] from comment #6) > (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #5) > > What's up with this rename from .ModalHighlightChange to > > .onModalHighlightChange? I don't see any other renames for this in this > > patch. > > Well, this is a syntax error: .ModalHighlightChange doesn't exist! > .onModalHighlightChange does... The fact that I opted to fix this here is > co-incidental. Are we missing a test here? Ideally this syntax error would have been hit through automated testing.
Assignee | ||
Comment 8•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/59448/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/59448/
Attachment #8763119 -
Flags: review?(jaws)
Attachment #8763120 -
Flags: review?(jaws)
Assignee | ||
Comment 9•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/59450/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/59450/
Assignee | ||
Updated•8 years ago
|
Attachment #8763049 -
Attachment is obsolete: true
Comment 10•8 years ago
|
||
Comment on attachment 8763119 [details] Bug 1279717 - introduce a Color.jsm module that implements common color math operations in a single place. https://reviewboard.mozilla.org/r/59448/#review56528
Attachment #8763119 -
Flags: review?(jaws) → review+
Comment 11•8 years ago
|
||
Comment on attachment 8763120 [details] Bug 1279717 - inverse the mask to use a white background on pages with bright text color. https://reviewboard.mozilla.org/r/59450/#review56530 r=me IFF you can say that you added a test for the syntax error :) ::: toolkit/modules/FinderHighlighter.jsm:525 (Diff revision 1) > + * Checks whether a CSS RGB color value can be classified as being 'bright'. > + * > + * @param {String} cssColor RGB color value in the default format rgb[a](r,g,b) > + * @return {Boolean} > + */ > + _isTextColorBright(cssColor) { s/\_isTextColorBright/\_isColorBright/ Can you create a member on Color.jsm that is: .isBright(), which internally returns `relativeLuminance > 0.7` ? ::: toolkit/modules/RemoteFinder.jsm:312 (Diff revision 1) > case "Finder:ModalHighlightChange": > - this._finder.ModalHighlightChange(data.useModalHighlight); > + this._finder.onModalHighlightChange(data.useModalHighlight); Did you add a test that would fail if this syntax error still existed?
Attachment #8763120 -
Flags: review?(jaws) → review+
Comment 12•8 years ago
|
||
https://reviewboard.mozilla.org/r/59448/#review56532 ::: browser/base/content/browser.js:1018 (Diff revision 1) > if (window.matchMedia("(-moz-os-version: windows-win8)").matches && > window.matchMedia("(-moz-windows-default-theme)").matches) { > - let windowFrameColor = Cu.import("resource:///modules/Windows8WindowFrameColor.jsm", {}) > - .Windows8WindowFrameColor.get(); > - > - // Formula from W3C's WCAG 2.0 spec's color ratio and relative luminance, > + let windowFrameColor = new Color(...Cu.import("resource:///modules/Windows8WindowFrameColor.jsm", {}) > + .Windows8WindowFrameColor.get()); > + // Default to black for foreground text. > + if (windowFrameColor.contrastRatio(new Color(0, 0, 0)) < 3) { This less-than-three check is quite specific to color ratio and relative luminance. It would be better placed as a method on Color.jsm. What do you think about a simple method called, "isContrastRatioAcceptable(color)", that internally calls .contrastRatio(color) and returns true if greater than 3. (This if-condition would need to be negated with that change)
Assignee | ||
Comment 13•8 years ago
|
||
Comment on attachment 8763119 [details] Bug 1279717 - introduce a Color.jsm module that implements common color math operations in a single place. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59448/diff/1-2/
Assignee | ||
Comment 14•8 years ago
|
||
Comment on attachment 8763120 [details] Bug 1279717 - inverse the mask to use a white background on pages with bright text color. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59450/diff/1-2/
Assignee | ||
Updated•8 years ago
|
Attachment #8763119 -
Flags: review+ → review?(jaws)
Assignee | ||
Updated•8 years ago
|
Attachment #8763120 -
Flags: review+ → review?(jaws)
Assignee | ||
Comment 15•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2e57e2d18d25
Updated•8 years ago
|
Attachment #8763119 -
Flags: review?(jaws) → review+
Comment 16•8 years ago
|
||
Comment on attachment 8763119 [details] Bug 1279717 - introduce a Color.jsm module that implements common color math operations in a single place. https://reviewboard.mozilla.org/r/59448/#review56768 ::: browser/base/content/browser.js:1018 (Diff revisions 1 - 2) > if (window.matchMedia("(-moz-os-version: windows-win8)").matches && > window.matchMedia("(-moz-windows-default-theme)").matches) { > let windowFrameColor = new Color(...Cu.import("resource:///modules/Windows8WindowFrameColor.jsm", {}) > .Windows8WindowFrameColor.get()); > // Default to black for foreground text. > - if (windowFrameColor.contrastRatio(new Color(0, 0, 0)) < 3) { > + if (!windowFrameColor.isContrastRatioAcceptable(new Color(0, 0, 0))) { Sorry for the churn here, but I think this should be isContrastRatioAccessible. I couldn't think of a better name until today, but "accessible" explains the goal here better than some vague "acceptable" term. ::: toolkit/modules/Color.jsm:46 (Diff revisions 1 - 2) > + get isBright() { > + return this.relativeLuminance > 0.7; Might be good to explain where 0.7 comes from. ::: toolkit/modules/Color.jsm:72 (Diff revisions 1 - 2) > + * Biased method to check if the contrast ratio between two colors is high > + * enough to be discernable. > + * > + * @param {Color} otherColor Color instance to calculate the contrast with > + * @return {Boolean} > + */ > + isContrastRatioAcceptable(otherColor) { > + return this.contrastRatio(otherColor) > 3; Might be good to explain where "3" comes from.
Comment 17•8 years ago
|
||
Comment on attachment 8763120 [details] Bug 1279717 - inverse the mask to use a white background on pages with bright text color. https://reviewboard.mozilla.org/r/59450/#review56772 ::: toolkit/modules/tests/browser/file_FinderSample.html:2 (Diff revision 2) > +<!DOCTYPE html> > +<!DOCTYPE html> nit, two doctypes
Attachment #8763120 -
Flags: review?(jaws) → review+
Assignee | ||
Comment 18•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=eb4cbd1456ef
Assignee | ||
Comment 19•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7eca9cc57979
Assignee | ||
Comment 20•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d8e666acb968
Assignee | ||
Comment 21•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0eeb33c16b48
Assignee | ||
Comment 22•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ffa48d81204f
Assignee | ||
Comment 23•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/aa3e5b818a71c843464421deaa495d0a399ca513 Bug 1279717 - introduce a Color.jsm module that implements common color math operations in a single place. r=jaws https://hg.mozilla.org/integration/fx-team/rev/229e285859a7349b5958157b0f0ae60a7b2a574c Bug 1279717 - inverse the mask to use a white background on pages with bright text color. r=jaws
Assignee | ||
Comment 24•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/8ec0039f5a778f6afe8f6e063ea56cf887e55e7d Bug 1279717 - disable browser_FinderHighlighter.js mochitest on linux due to frequent intermittents. r=test-only
Comment 25•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/aa3e5b818a71 https://hg.mozilla.org/mozilla-central/rev/229e285859a7 https://hg.mozilla.org/mozilla-central/rev/8ec0039f5a77
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Updated•8 years ago
|
QA Contact: brindusa.tot
Comment 26•8 years ago
|
||
Feature has been backed out so not tracking these bugs for 50.
tracking-firefox50:
? → ---
Reporter | ||
Comment 27•8 years ago
|
||
Reproduced on Nightly51.0a1
Status: RESOLVED → REOPENED
status-firefox51:
--- → affected
tracking-firefox51:
--- → ?
Resolution: FIXED → ---
Reporter | ||
Comment 28•8 years ago
|
||
I will filed new bug
No longer blocks: 1291278
Status: REOPENED → RESOLVED
Closed: 8 years ago → 8 years ago
status-firefox51:
affected → ---
tracking-firefox51:
? → ---
Resolution: --- → FIXED
Reporter | ||
Comment 29•8 years ago
|
||
I have filed Bug 1302207
Comment 30•8 years ago
|
||
I've managed to reproduce this issue on an older Nightly build 50.0a1 from 2016-06-11. This bug is verified fixed on 50.0b1-build2 (20160920155715), using Windows 10 x64, Mac OS 10.11 and Ubuntu 16.04 x64 LTS.
You need to log in
before you can comment on or make changes to this bug.
Description
•