1298435 - Emphasized highlight does not follow browser resize

Status: VERIFIED FIXED

(Toolkit :: Find Toolbar, defect)

Description

[Alice0775 White]

Attachment: screenshot

Build Identifier:
https://hg.mozilla.org/mozilla-central/rev/a65b35c8e5b17c2585968974aef1da67a8c56642
Mozilla/5.0 (Windows NT 10.0; WOW64; rv:51.0) Gecko/20100101 Firefox/51.0 ID:20160826063819

Steps To reproduce:
1. Enable new findbar
  user_pref("findbar.highlightAll", true);
  user_pref("findbar.modalHighlight", true);
2. Open any web page
3. Open Find Bar (Ctrl+F)
4. Perform find with any term
5. Resize browser

Actual Results:
Emphasized highlight  is wrongly positioned

Expected Results:
Properly positioned

Comment 1

[Mike de Boer [:mikedeboer]]

Attachment: Bug 1298435 - upon browser window resize, update all the rects we track for each range we found and correctly reposition all oulines atop the mask.

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

Comment 2

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

Comment on attachment 8785933 [details]
Bug 1298435 - upon browser window resize, update all the rects we track for each range we found and correctly reposition all oulines atop the mask.

https://reviewboard.mozilla.org/r/74950/#review72978

::: toolkit/modules/FinderHighlighter.jsm:1018
(Diff revision 1)
>     *   {Boolean} scrollOnly     TRUE when the page has scrolled in the meantime,
>     *                            which means that the fixed positioned elements
>     *                            need to be repainted.
>     */
> -  _scheduleRepaintOfMask(window, { contentChanged, scrollOnly } = { contentChanged: false, scrollOnly: false }) {
> +  _scheduleRepaintOfMask(window, { contentChanged, scrollOnly, updateAllRanges } =
> +                                 { contentChanged: false, scrollOnly: false, updateAllRanges: false }) {

Please add documentation here for updateAllRanges.

::: toolkit/modules/FinderHighlighter.jsm:1035
(Diff revision 1)
>  
>      // When we request to repaint unconditionally, we mean to call
>      // `_repaintHighlightAllMask()` right after the timeout.
>      if (!dict.unconditionalRepaintRequested)
>        dict.unconditionalRepaintRequested = !contentChanged || repaintFixedNodes;
> +    // Some events, like a resize, call for recalculation all the rects of all ranges.

recalculation of all the rects of all ranges.

Comment 3

[Mike de Boer [:mikedeboer]]

Comment on attachment 8785933 [details]
Bug 1298435 - upon browser window resize, update all the rects we track for each range we found and correctly reposition all oulines atop the mask.

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

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bf5b8a794ab0
upon browser window resize, update all the rects we track for each range we found and correctly reposition all oulines atop the mask. r=jaws

Comment 5

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 6

[roxleitan]

Verified on the latest Nightly Build ID: 20160913030425 on Windows 7 x64 and Windows 10 x64 with https://www.reddit.com/ page , searching for "ago" term and managed to reproduce it.

Comment 7

[Mike de Boer [:mikedeboer]]

Regression, I have a patch ready.

Comment 8

[Mike de Boer [:mikedeboer]]

Attachment: Follow-up patch: make sure the modal highlight outline is always inserted in the top-most window

Comment 9

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

Comment on attachment 8791608 [details] [diff] [review]
Follow-up patch: make sure the modal highlight outline is always inserted in the top-most window

Review of attachment 8791608 [details] [diff] [review]:
-----------------------------------------------------------------

Should this have been caught by one of our tests?

Comment 10

[Mike de Boer [:mikedeboer]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #9)
> Should this have been caught by one of our tests?

Well, I think it'd be quite simple to add window-object checking to browser_FinderHighlighter.jsm, even though I think that it's highly unlikely that the same bug will be introduced again.
I will work on this asap in another bug.

Comment 11

[Mike de Boer [:mikedeboer]]

https://hg.mozilla.org/integration/fx-team/rev/d8aad5dec43e6e962cc39d2a78c1002c1872d1a7
Bug 1298435 - make sure the modal highlight outline is always inserted in the top-most window. r=jaws

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 13

[roxleitan]

This bug is still reproducible on build Nightly 52.0a1, build ID: 20160920030429. 
Bug is reproducible on Windows 10 x64 on https://www.washingtonpost.com/ page when the browser is resized multiple times.
Reopening the bug.

Comment 14

[Mike de Boer [:mikedeboer]]

(In reply to [e-mail] from comment #13)
> Bug is reproducible on Windows 10 x64 on https://www.washingtonpost.com/
> page when the browser is resized multiple times.
> Reopening the bug.

This page is causing so many reflows when resizing, it's insane. However, that doesn't mean we shouldn't support it, just that this is simply the best I can do (using the Nightly-to-be for Sept 28th) for now. You notice, for example, that after resizing some more the rectangles do correct themselves. But sometimes ranges simply disappear, because you resize to a low-width/ mobile size and the layout switches. Detection for that is happening in bug 1302470, so that'll make the behavior a _lot_ better.
Any other optimizations to be more correct would be a P4 bug IMO.
I think it'd better to resolve this particular bug, file a new one that depends on bug 1302470 and once that's resolved, we'll see what's left to work on.

Comment 15

[roxleitan]

Hi Mike,

I agree that the initial issue reported in this bug is resolved, now the issue is reproducible only after multiple browser resize.
I filed bug 1305725 to cover the multiple resize issue from Comment 13.

Comment 16

[Mike de Boer [:mikedeboer]]

Other bug covers the remaining bug here, calling this fixed.

Comment 17

[roxleitan]

Based on comment 15 and comment 16 I will mark this as Verified-Fixed