Closed Bug 1279717 Opened 3 years ago Closed 3 years ago

Highlight all is low contrast in certain text/background color case

Categories

(Toolkit :: Find Toolbar, defect)

50 Branch
defect
Not set
Points:
2

Tracking

()

VERIFIED FIXED
mozilla50
Iteration:
50.1 - Jun 20
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
Summary: Highlight all is low contrast in certain case → Highlight all is low contrast in certain text/background color case
Depends on: 1279742
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Iteration: --- → 50.1
Points: --- → 2
Flags: qe-verify+
Flags: firefox-backlog+
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-
(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 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.
(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.
(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.
Attachment #8763049 - Attachment is obsolete: true
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 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+
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)
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/
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/
Attachment #8763119 - Flags: review+ → review?(jaws)
Attachment #8763120 - Flags: review+ → review?(jaws)
Attachment #8763119 - Flags: review?(jaws) → review+
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 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+
Blocks: 1279616
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
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
QA Contact: brindusa.tot
Feature has been backed out so not tracking these bugs for 50.
Reproduced on Nightly51.0a1
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Blocks: 1291278
I will filed new bug
No longer blocks: 1291278
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
I have filed Bug 1302207
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.