Closed Bug 1282070 Opened 3 years ago Closed 3 years ago

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

Categories

(Toolkit :: Find Toolbar, defect)

defect
Not set
Points:
3

Tracking

()

VERIFIED FIXED
mozilla51
Iteration:
50.4 - Aug 1
Tracking Status
firefox49 --- unaffected
firefox50 --- disabled
firefox51 --- verified

People

(Reporter: bj, Assigned: mikedeboer)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(2 files)

Attached image 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.
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!
Blocks: 384458
Feature has been backed out so not tracking these bugs for 50.
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.
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Iteration: --- → 50.4 - Aug 1
Points: --- → 3
Flags: qe-verify+
Flags: firefox-backlog+
OS: Linux → All
Hardware: x86_64 → All
Blocks: 1291278
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 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?
Attachment #8779357 - Flags: review?(jaws) → review+
(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.
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
Pushed by mdeboer@mozilla.com:
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
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
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)
Docshell leaks also appear to have returned on the relanding: https://treeherder.mozilla.org/logviewer.html#?job_id=11163887&repo=fx-team
Blocks: 1295759
Flags: needinfo?(mdeboer)
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
https://hg.mozilla.org/mozilla-central/rev/4d92551f854b
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Depends on: 1302939
Verified as fixed on Windows 10 on the latest Nightly 52.0a1 Build ID: 20160920030429
Status: RESOLVED → VERIFIED
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
Flags: needinfo?(mdeboer)
Flags: needinfo?(mdeboer)
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.
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.