1302035 - Inverse dimmed highlight is sometimes shown on light pages when the first match has bright text

Status: VERIFIED FIXED

(Toolkit :: Find Toolbar, defect)

Description

[Alice0775 White]

Steps To Reproduce:
1. Logged in https://bugzilla.mozilla.org/
2. Select "Bugzilla's general appearance (skin)" to "Classic"  in "User Interface" section of https://bugzilla.mozilla.org/userprefs.cgi?tab=settings
3. Open https://bugzilla.mozilla.org/ or any BMO page in New tab
4. Find "bug"

Actual Results:
Dimmed background is not shown though emphasized text is there

Expected Results:
Dimmed background is shown

Comment 1

[Alice0775 White]

Attachment: screenshot

Comment 3

[Mike de Boer [:mikedeboer]]

What needs to be done here is to take a sample of the found ranges (I recommend 5, randomly picked from the set with even spread) and determine the text brightness from them.
Only if the majority of these ranges have bright colored text, inverse the mask color.

Comment 4

[Mike de Boer [:mikedeboer]]

Attachment: Bug 1302035 - implement an algorithm to better detect whether a page is dark or bright and use that to toggle the dimming mode of the find toolbar.

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

Comment 5

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

Comment on attachment 8793376 [details]
Bug 1302035 - implement an algorithm to better detect whether a page is dark or bright and use that to toggle the dimming mode of the find toolbar.

https://reviewboard.mozilla.org/r/80118/#review79156

::: toolkit/modules/FinderHighlighter.jsm:738
(Diff revision 1)
> +        ranges.push(dict.currentFoundRange);
> +        ++sampleSize;
> +      } else {
> +        --sampleSize;
> +      }
> +    }
> +    let brightCount = 0;
> +    for (let i = 0; i < sampleSize; ++i) {
> +      let range = ranges[Math.floor((rangesCount / sampleSize) * i)];

ranges can have an item appended to it but rangesCount will now be incorrect.

Comment 6

[Mike de Boer [:mikedeboer]]

Comment on attachment 8793376 [details]
Bug 1302035 - implement an algorithm to better detect whether a page is dark or bright and use that to toggle the dimming mode of the find toolbar.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/80118/diff/1-2/

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ee337e54f0e5
implement an algorithm to better detect whether a page is dark or bright and use that to toggle the dimming mode of the find toolbar. r=jaws

Comment 8

[Iris Hsiao [:ihsiao]]

https://hg.mozilla.org/mozilla-central/rev/ee337e54f0e5

Comment 9

[Simona Badau, Desktop QA]

Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Firefox/52.0
Build ID: 20160927030200

The dimmed highlight still inverses  when "Highlight All" button is clicked twice. Reproduced with the Classic Bugzilla theme when searching for "bugzilla": 
https://bugzilla.mozilla.org/userprefs.cgi?tab=settings

Moreover, the issue is reproducible without clicking on the "Highlight All" button if the search match is bright text on dark background (to reproduce follow the STR from dupe bug 1304313).

Reproduced the above issues on Windows 10 using the latest Nightly 52 version (Build ID: 20160927030200).

Comment 10

[Mike de Boer [:mikedeboer]]

Simona, can you file a new bug for this new issue?

Comment 11

[Simona Badau, Desktop QA]

Sure Mike, filed bug 1306234. 

Since this issues is no longer reproducible (when following the STR from the Description) and a follow up bug was filed for the remaining issues, marking the status of this bug to Verified fixed (verified on Ubuntu 16.04, Mac OS X 10.11 and Windows 10 using the latest Nightly 52 - Build ID: 20160928030201).

Also, I'm setting status-firefox52 to verified and status-firefox51 to disabled (the new toolbar feature is disabled by default on Firefox 51).

Comment 12

[Mike de Boer [:mikedeboer]]

(In reply to Simona B [:simonab ] from comment #9)
> Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Firefox/52.0
> Build ID: 20160927030200
> 
> The dimmed highlight still inverses  when "Highlight All" button is clicked
> twice. Reproduced with the Classic Bugzilla theme when searching for
> "bugzilla": 
> https://bugzilla.mozilla.org/userprefs.cgi?tab=settings
> 
> Moreover, the issue is reproducible without clicking on the "Highlight All"
> button if the search match is bright text on dark background (to reproduce
> follow the STR from dupe bug 1304313).
> 
> Reproduced the above issues on Windows 10 using the latest Nightly 52
> version (Build ID: 20160927030200).

This is what happens on, for example, https://developer.mozilla.org/en-US/:
1. 'm' has about 80 matches, most of them are in the part of the page that has a light background, thus the black dim is shown.
2. 'mo' has about 20 matches, the same scenario as 1.
3. 'moz' has about 10 matches, most of them are in the part of the page that has a dark background, thus the white dim is shown.

For legibility, this is all desired behavior IMO.
I can do something about the flickering, however, when the background color changes - I'll transition it from black to white and vice-versa to make it easier on the eyes. I've tested this locally and looks very nice.