1282070 - New Find locates strings on partially loaded pages and doesn't notice when the position changes

Status: VERIFIED FIXED

(Toolkit :: Find Toolbar, defect)

Description

[B.J. Herbison]

Attachment: Screenshot of the issue

See the attached screenshot.

I visited https://www.washingtonpost.com/news/monkey-cage/wp/2016/06/24/britain-has-voted-for-brexit-heres-what-happens-next/ and started searching. Matches were found (and positioned correctly) but then the page adjusted. The matches moved but Find didn't notice.

The yellow and white represent the previous locations. The blue is the current location of the yellow match.

Possibly related issues:
* The general misplacement of matches.
* Use Find on a page, close the find toolbar, change the page, find again. The old locations are used (but some actions cause the locations to be recalculated).

I believe I reported the second issue previously but I can't find the bug number right now.

Comment 1

[Mike de Boer [:mikedeboer]]

I believe this is likely a dupe of one of the open bugs, but that'll sort itself out in due time. Thanks for reporting!

Comment 2

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

[Tracking Requested - why for this release]: Regression

Comment 3

[Marcia Knous [:marcia]]

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

Comment 4

[Mike de Boer [:mikedeboer]]

We can fix this by listening to the MozAfterPaint event on the content document and schedule a repaint then to update the boxes.
This will resolve many other 'painting issues', or at least provides the ground work necessary to fix them.
The painting scheduler of the highlighter already does deferred execution, chunking and protects against parallel runs, so the work here 'should' be relatively simple.

Comment 5

[Mike de Boer [:mikedeboer]]

Attachment: Bug 1282070 - repaint the modal highlight mask when the page resizes or changes size due to added/ removed content.

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

Comment 6

[Mike de Boer [:mikedeboer]]

This patch does quite a few things:

 - It fixes bug 1279652 and bug 1282766.
 - It makes sure to re-count and re-highlight when 'Case Sensitive' or 'Whole Words' are toggled.
 - It also fixes the find count when the page updates whilst the findbar is open.

Comment 7

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

Comment on attachment 8779357 [details]
Bug 1282070 - repaint the modal highlight mask when the page resizes or changes size due to added/ removed content.

https://reviewboard.mozilla.org/r/70342/#review68106

::: toolkit/modules/Finder.jsm:375
(Diff revision 1)
>          controller.scrollLine(true);
>          break;
>      }
>    },
>  
> -  _notifyMatchesCount: function(result) {
> +  _notifyMatchesCount: function(result = this._currentMatchesCountResult) {

I haven't seen `this` referenced in a default argument, but I tested it out and it refers to the same value that `this` is set to within the function. TIL.

::: toolkit/modules/Finder.jsm:377
(Diff revision 1)
>      }
>    },
>  
> -  _notifyMatchesCount: function(result) {
> +  _notifyMatchesCount: function(result = this._currentMatchesCountResult) {
> +    // The `_currentFound` property is only used for internal bookkeeping.
> +    delete result._currentFound;

Instead of deleting the property just set it to false.

::: toolkit/modules/Finder.jsm:446
(Diff revision 1)
>  
> -      this._notifyMatchesCount(result);
> -    });
> +  onIteratorRestart({ word, linksOnly }) {
> +    this.requestMatchesCount(word, this._currentMatchLimit, linksOnly);
>    },
>  
> +  // END of FinderIterator listener

I generally think comments like this are a code smell meaning that blocks are either too long or there is not good enough separation through whitespace or separate files.

How do you think you could rearrange this code so this type of comment wouldn't be necessary?

::: toolkit/modules/FinderHighlighter.jsm:235
(Diff revision 1)
>      }
>  
> -    return found;
> +    return this._found;
>    }),
>  
> +  // Start of FinderIterator listener

Ditto. In this case, I would remove the newlines between all of the function declarations below and then get rid of the "// Start of FinderIterator listener" and "// End of FinderIterator listener", and replace them with one comment at the start saying "FinderIterator listener implementation"

::: toolkit/modules/FinderIterator.jsm:85
(Diff revision 1)
>      if (!finder)
>        throw new Error("Missing required option 'finder'");
>      if (!word)
>        throw new Error("Missing required option 'word'");
> -    if (typeof onRange != "function")
> -      throw new TypeError("Missing valid, required option 'onRange'");
> +    if (typeof listener != "object")
> +      throw new TypeError("Missing valid, required option 'listener'");

"Missing required option 'listener'"

::: toolkit/modules/FinderIterator.jsm:165
(Diff revision 1)
> +    if (!iterParams.word)
> +      return;

Per line 82, isn't 'word' required?

Comment 8

[Mike de Boer [:mikedeboer]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #7)
> Instead of deleting the property just set it to false.

Well, with deleting the property I'm able to pass this object back cleanly to findbar.xml, so that's why I do it.

Comment 9

[Mike de Boer [:mikedeboer]]

https://hg.mozilla.org/integration/fx-team/rev/bc160abe215baed95384cb10e17118b168caa05a
Bug 1282070 - repaint the modal highlight mask when the page resizes or changes size due to added/ removed content. r=jaws

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/bc160abe215b
repaint the modal highlight mask when the page resizes or changes size due to added/ removed content. r=jaws

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for docshell leakage:

https://hg.mozilla.org/integration/fx-team/rev/52fa735c7cec7333737999f62a43856e7306f6ba

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=bc160abe215baed95384cb10e17118b168caa05a
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=11153272&repo=fx-team

Comment 12

[Mike de Boer [:mikedeboer]]

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

Comment 13

[Mike de Boer [:mikedeboer]]

https://hg.mozilla.org/integration/fx-team/rev/af4cc6c04573fca495450725de702f7188dab6ea
Bug 1282070 - repaint the modal highlight mask when the page resizes or changes size due to added/ removed content. r=jaws

Comment 14

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

I had to back this out for mochitest(oth) failures like https://treeherder.mozilla.org/logviewer.html#?job_id=11159066&repo=fx-team

https://hg.mozilla.org/integration/fx-team/rev/049fe094cf32

(Failures were present for the initial landing and went away in the backout, then came back with the relanding)

Comment 15

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

Docshell leaks also appear to have returned on the relanding: https://treeherder.mozilla.org/logviewer.html#?job_id=11163887&repo=fx-team

Comment 16

[Mike de Boer [:mikedeboer]]

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

Comment 17

[Mike de Boer [:mikedeboer]]

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

Comment 18

[Mike de Boer [:mikedeboer]]

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

Comment 19

[Mike de Boer [:mikedeboer]]

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

Comment 20

[Mike de Boer [:mikedeboer]]

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

Comment 21

[Mike de Boer [:mikedeboer]]

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

Comment 22

[Mike de Boer [:mikedeboer]]

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

Comment 23

[Mike de Boer [:mikedeboer]]

https://hg.mozilla.org/integration/fx-team/rev/4d92551f854b80764c7b010c76043920c28c8e35
Bug 1282070 - repaint the modal highlight mask when the page resizes or changes size due to added/ removed content. r=jaws

Comment 24

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

https://hg.mozilla.org/mozilla-central/rev/4d92551f854b

Comment 25

[roxleitan]

Verified as fixed on Windows 10 on the latest Nightly 52.0a1 Build ID: 20160920030429

Comment 26

[Sylvestre Ledru [:Sylvestre]]

Mike, as this is marked as impacting 50 (maybe it has been disabled), do you want to uplift that? 
If yes, could you fill the uplift request? Thanks

Comment 27

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

I reproduced this issue on Fx 50.0a1 (20160624030212).
I can confirm this issue is no longer reproducible, I verified using Fx 51.0a2 (20161113004006), on Windows 10 x64, Ubuntu 14.04 LTS and Mac OS X 10.11.