1306234 - Dimmed background is inconsistent after clicking "Highlight All"

Status: VERIFIED FIXED

(Toolkit :: Find Toolbar, defect)

Description

[Simona Badau, Desktop QA]

Attachment: reversedBackground.mp4

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

[Affected versions]:
- Nightly 52

[Affected platforms]:
- Windows 10, Ubuntu 14.04, Mac OS X 10.11

[Steps to reproduce]:
1. Launch Firefox
2. Log into https://bugzilla.mozilla.org/userprefs.cgi?tab=settings
3. Select "Bugzilla's general appearance (skin)" to "Classic"  in "User Interface" section and Submit changes
4. Open the Find Bar
5. Search for "bugzilla"
6. Click twice on "Highlight All" button
  
[Expected result]:
In steps 5 and 6, the dimmed background should be the same.

[Actual result]:
Inconsistent dimmed background, after step 6, the dimmed background is reversed. Please see the attached scree-cast for more details.

Comment 1

[Mike de Boer [:mikedeboer]]

This is also reproducable on https://developer.mozilla.org/en-US/, so I'll take that as an easier to reproduce example.

This is what happens on 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.

The other thing, when Highlight All is clicked repeatedly, is another matter and will be fixed with the item above.

Comment 2

[Mike de Boer [:mikedeboer]]

Attachment: Bug 1306234 - stop flickering when the background color of the findbar dimmed highlighting mode changes and make sure the color changes if needed when all the ranges have been found as well.

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

Comment 3

[Mike de Boer [:mikedeboer]]

Comment on attachment 8797558 [details]
Bug 1306234 - stop flickering when the background color of the findbar dimmed highlighting mode changes and make sure the color changes if needed when all the ranges have been found as well.

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

Comment 4

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

Comment on attachment 8797558 [details]
Bug 1306234 - stop flickering when the background color of the findbar dimmed highlighting mode changes and make sure the color changes if needed when all the ranges have been found as well.

https://reviewboard.mozilla.org/r/83242/#review82850

So what's happening here is that the dark background is used because the first match is on a dark background, but then when the highlight all is toggled the other matches were taken into account and the code had determined not to darken the page as much?

::: toolkit/modules/FinderHighlighter.jsm:77
(Diff revision 2)
>    ],
>    maskNode: [
>      ["background", "rgba(0,0,0,.35)"],
>      ["pointer-events", "none"],
>      ["position", "absolute"],
> +    ["transition", "background .5s ease-in"],

This transition is a bit long here. Can you lower this to .2s ? I'm also a bit worried that this will degrade performance. Have you tested this on large pages?

Comment 5

[Mike de Boer [:mikedeboer]]

Comment on attachment 8797558 [details]
Bug 1306234 - stop flickering when the background color of the findbar dimmed highlighting mode changes and make sure the color changes if needed when all the ranges have been found as well.

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

Comment 6

[Mike de Boer [:mikedeboer]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #4)
> This transition is a bit long here. Can you lower this to .2s ?

Done!

> I'm also a
> bit worried that this will degrade performance. Have you tested this on
> large pages?

Good point! I made a small adjustment so that the transition is only enabled when the rects are drawn as well (meaning that the initial, first time render will not trigger a transition).

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a4c3d4d2588c
stop flickering when the background color of the findbar dimmed highlighting mode changes and make sure the color changes if needed when all the ranges have been found as well. r=jaws

Comment 8

[Carsten Book [:Tomcat]]

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

Comment 9

[roxleitan]

Verified fixed on the latest Nightly 52.0a1 (Build ID:20161013030204) on Ubuntu 16.04 and Windows 10 and Mac OS X 10.11
I can still see a short reverse for the dimmed background after "Highlight All" button is pressed.For this I filed bug 1310161.

Considering the above, setting the status verified fixed.