1279652 - Find-Again highlights same word twice after scrolling

Status: REOPENED

(Toolkit :: Find Toolbar, defect)

Description

[Justin Dolske [:Dolske]]

First - the new find-in-page UI is awesome. \o/

But noticed a bug on Reddit homepage:

1) Search for "ago" (in every submission, "submitted x hours ago".
2) Command-G to find & highlight next occurrence
3) Repeat #2 until it highlights the word at the bottom of the window
4) Once more - scrolls the view and correctly highlights the next word
5) Once more - instead of advancing to the next word, it just animates the find over the same word from step 4.
6) Further Command-G works until you reach the bottom of the window again.

The same issue occurs when the search wraps around (and scrolls) to the top of the page.

This also seems to happen when you switch to another tab and then return (without having dismissed the find UI) -- Command-G will re-animate the already-highlighted word. I'm not really sure if that's a bug or not, though.

Comment 1

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

[Tracking Requested - why for this release]: Regression

Comment 2

[Marcia Knous [:marcia]]

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

Comment 3

[Andrew Overholt [:overholt]]

Bulk update to find bar bugs that won't be in 50 (according to https://bugzilla.mozilla.org/show_bug.cgi?id=1279695#c4 and local testing).

Comment 4

[Mike de Boer [:mikedeboer]]

This was fixed by bug 1279695.

Comment 5

[Brindusa Tot, Desktop QA]

This bug is still reproducible on build Nightly 51.0a1, build ID: 20160913030425. 
Bug is reproducible on Windows 7, Ubuntu 16.04.

Reopening the bug.

Comment 6

[Mike de Boer [:mikedeboer]]

(In reply to Brindusa Tot[:brindusat] from comment #5)
> This bug is still reproducible on build Nightly 51.0a1, build ID:
> 20160913030425. 
> Bug is reproducible on Windows 7, Ubuntu 16.04.
> 
> Reopening the bug.

I'm confused, because in my Nightly reddit.com doesn't render 'ago', but it does so in Fx Beta for me. Weird.
How were you able to reproduce and what is it you saw?

Comment 7

[Brindusa Tot, Desktop QA]

Attachment: screencast reproducing the issue

Mike, 
Tried again this scenario on latest Nightly 52.0a2, build id 20160920030429, on Windows 7.

Here are the results:
Following the steps from bug description, at step 2:  "Repeat #2 until it highlights the word at the bottom of the window", when page is scrolled down, by hitting the CTRL+G, to find the next occurrence, the highlights of the 'ago' are cleared. 
Next, when it gets to the bottom of the page, hit again CTRL+G, then the page is scrolled to the top and first 'ago' is highlighted for 1 second. IF hit once more CTRL+G - instead of advancing to the next 'ago', it just animates the same 'ago'(first from the page). Please see attached video.

Note: Sometimes the last occurrence of the finding word from the page is also animated two times, but this is intermittent. (had luck and get it on the screencast)

Comment 8

[Mike de Boer [:mikedeboer]]

I was finally able to reproduce this by clearing my cookies and localStorage for .reddit.com!

Comment 9

[Mike de Boer [:mikedeboer]]

Attachment: Bug 1279652 - reddit.com invalidates all references in the TextLayer, resulting in zero ClientRects for ranges that we have in the FinderHighlighter cache. This fix makes sure to re-fetch all the ranges when this is detected.

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

Comment 10

[Mike de Boer [:mikedeboer]]

I have to note here, though, that this patch only fixes the fact that all the highlights disappear and that the highlighter is able to restore itself after subsequent usage of the findbar (CTRL+G further).
This does not fix the real cause of this issue, which seems to be that valid ranges may return ZERO ClientRects for `range.getClientRects()` when the page does something I can't detect at the moment.

Comment 11

[Mike de Boer [:mikedeboer]]

Comment on attachment 8797580 [details]
Bug 1279652 - reddit.com invalidates all references in the TextLayer, resulting in zero ClientRects for ranges that we have in the FinderHighlighter cache. This fix makes sure to re-fetch all the ranges when this is detected.

Bad patch, introduces regressions.

Comment 12

[Mike de Boer [:mikedeboer]]

Comment on attachment 8797580 [details]
Bug 1279652 - reddit.com invalidates all references in the TextLayer, resulting in zero ClientRects for ranges that we have in the FinderHighlighter cache. This fix makes sure to re-fetch all the ranges when this is detected.

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

Comment 13

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

Comment on attachment 8797580 [details]
Bug 1279652 - reddit.com invalidates all references in the TextLayer, resulting in zero ClientRects for ranges that we have in the FinderHighlighter cache. This fix makes sure to re-fetch all the ranges when this is detected.

https://reviewboard.mozilla.org/r/83254/#review82870

::: toolkit/modules/FinderHighlighter.jsm
(Diff revision 2)
> -    if (dict.brightText)
> -      dict.modalHighlightAllMask.setAttributeForElement(kMaskId, "brighttext", "true");

Why does this patch remove the brighttext code?

Comment 14

[Mike de Boer [:mikedeboer]]

(In reply to Jared Wein [:jaws] (overloaded with reviews / please needinfo? me) from comment #13)
> Why does this patch remove the brighttext code?

It doesn't have to do anything about this patch, but the thing is that this code is obsolete (ie. not doing anything) since we're not using stylesheets anymore, but inline styles.

Comment 15

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

Comment on attachment 8797580 [details]
Bug 1279652 - reddit.com invalidates all references in the TextLayer, resulting in zero ClientRects for ranges that we have in the FinderHighlighter cache. This fix makes sure to re-fetch all the ranges when this is detected.

https://reviewboard.mozilla.org/r/83254/#review83174

::: toolkit/modules/FinderHighlighter.jsm:1212
(Diff revision 2)
>        let pageContentChanged = (Math.abs(previousWidth - width) > kContentChangeThresholdPx ||
>                                  Math.abs(previousHeight - height) > kContentChangeThresholdPx);
>        // When the page has changed significantly enough in size, we'll restart
>        // the iterator with the same parameters as before to find us new ranges.
>        if (pageContentChanged)
>          this.iterator.restart(this.finder);

It seems this patch and the lines highlighted here serve similar purposes. Can these get merged?

Comment 16

[Mike de Boer [:mikedeboer]]

Comment on attachment 8797580 [details]
Bug 1279652 - reddit.com invalidates all references in the TextLayer, resulting in zero ClientRects for ranges that we have in the FinderHighlighter cache. This fix makes sure to re-fetch all the ranges when this is detected.

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

Comment 17

[Mike de Boer [:mikedeboer]]

Comment on attachment 8797580 [details]
Bug 1279652 - reddit.com invalidates all references in the TextLayer, resulting in zero ClientRects for ranges that we have in the FinderHighlighter cache. This fix makes sure to re-fetch all the ranges when this is detected.

https://reviewboard.mozilla.org/r/83254/#review83174

> It seems this patch and the lines highlighted here serve similar purposes. Can these get merged?

Nice catch! I merged them, it was the right thing to do.

Comment 18

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

Comment on attachment 8797580 [details]
Bug 1279652 - reddit.com invalidates all references in the TextLayer, resulting in zero ClientRects for ranges that we have in the FinderHighlighter cache. This fix makes sure to re-fetch all the ranges when this is detected.

https://reviewboard.mozilla.org/r/83254/#review83490

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5bccb629be60
reddit.com invalidates all references in the TextLayer, resulting in zero ClientRects for ranges that we have in the FinderHighlighter cache. This fix makes sure to re-fetch all the ranges when this is detected. r=jaws

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5e187221dbe8
reddit.com invalidates all references in the TextLayer, resulting in zero ClientRects for ranges that we have in the FinderHighlighter cache. This fix makes sure to re-fetch all the ranges when this is detected. r=jaws

Comment 21

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d768ef11f948
Backed out changeset 5bccb629be60 for leaking docshells, e.g. in browser_Finder_hidden_textarea.js and browser_findbar.js. r=backout

Comment 22

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

Sorry, had backed out the wrong changeset but relanded. Backout was https://hg.mozilla.org/integration/autoland/rev/d768ef11f948f3ed4ed91fe791b46d73ecd04404

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5e187221dbe8

Comment 24

[roxleitan]

Attachment: 2016-10-18.mp4

The issue is still reproducible on the latest Nightly 52.0a1 (Build ID: 20161017030209) on Ubuntu 16.04 and Windows 10 and Mac OS X 10.11.
The yellow highlight disappears when navigates through matches using Ctrl+G or F3 keys and the page is scrolled down, then a match is highlighted with yellow background twice or more. 

Reopening the bug.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Setting status-firefox52 to disabled since this feature isn't riding the 52 train.

Comment 26

[Julien Cristau [:jcristau]]

disabled in bug 1355596

Comment 27

[Mike de Boer [:mikedeboer]]

Roxana, I'd like to get a detailed list of defects here so that I can get a good angle on what's still TODO here.
Please compare the each defect _without_ 'findbar.modalHighlight' set to true, but _with_ 'findbar.highlightAll' set to true and if it occurs in both modes, it's a WONTFIX defect.

Thanks!!

Comment 28

[roxleitan]

Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:58.0) Gecko/20100101 Firefox/58.0
Build ID: 20171015220106

Hi Mike,

Tested using the latest Nightly 58.0a1 on Windows 10 x64.

The issue is reproducible in both cases:
-'findbar.modalHighlight' set to true and 'findbar.highlightAll' set to false
-'findbar.modalHighlight' set to false and 'findbar.highlightAll' set to true

At some point, when navigating through matches the highlight disappears. "F3" re-animates the already-highlighted word.

Comment 29

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.