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

Status: VERIFIED FIXED

(Toolkit :: Find Toolbar, defect)

Description

[Alice0775 White]

Attachment: body_style=_background-color_black_color_white.html

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

Comment 1

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/message/reply/question/etc.)]

[Tracking Requested - why for this release]: Regression

Comment 2

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1: inverse the mask to use a white background on pages with bright text color

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

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

Comment 4

[Mike de Boer [:mikedeboer]]

(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

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 6

[Mike de Boer [:mikedeboer]]

(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

[Jared Wein [:jaws] (please needinfo? me)]

(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.

Comment 8

[Mike de Boer [:mikedeboer]]

Attachment: Bug 1279717 - introduce a Color.jsm module that implements common color math operations in a single place.

Review commit: https://reviewboard.mozilla.org/r/59448/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/59448/

Comment 9

[Mike de Boer [:mikedeboer]]

Attachment: Bug 1279717 - inverse the mask to use a white background on pages with bright text color.

Review commit: https://reviewboard.mozilla.org/r/59450/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/59450/

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

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

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

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?

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

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 13

[Mike de Boer [:mikedeboer]]

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 14

[Mike de Boer [:mikedeboer]]

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/

Comment 15

[Mike de Boer [:mikedeboer]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2e57e2d18d25

Comment 16

[Jared Wein [:jaws] (please needinfo? me)]

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

[Jared Wein [:jaws] (please needinfo? me)]

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

Comment 18

[Mike de Boer [:mikedeboer]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=eb4cbd1456ef

Comment 19

[Mike de Boer [:mikedeboer]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7eca9cc57979

Comment 20

[Mike de Boer [:mikedeboer]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d8e666acb968

Comment 21

[Mike de Boer [:mikedeboer]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0eeb33c16b48

Comment 22

[Mike de Boer [:mikedeboer]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ffa48d81204f

Comment 23

[Mike de Boer [:mikedeboer]]

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

Comment 24

[Mike de Boer [:mikedeboer]]

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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/aa3e5b818a71
https://hg.mozilla.org/mozilla-central/rev/229e285859a7
https://hg.mozilla.org/mozilla-central/rev/8ec0039f5a77

Comment 26

[Marcia Knous [:marcia]]

Feature has been backed out so not tracking these bugs for 50.

Comment 27

[Alice0775 White]

Reproduced on Nightly51.0a1

Comment 28

[Alice0775 White]

I will filed new bug

Comment 29

[Alice0775 White]

I have filed Bug 1302207

Comment 30

[Ciprian Georgiu, Desktop QA]

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.