1334213 - Highlighted findbar results do not scroll with the page, on Twitter threads, Gmail, Google Groups

Status: RESOLVED FIXED

(Toolkit :: Find Toolbar, defect)

Description

[Daniel Holbert [:dholbert]]

STR:
 1. Visit https://twitter.com/vanaman/status/824668733201276929 (or another twitter thread that requires scrolling)
 2. Ctrl+F and type "that" (or some other word that appears multiple times)
 3. Scroll the page (with mousewheel or keyboard)

ACTUAL RESULTS:
 - The highlighted overlay stays in place despite the page scrolling underneath it. So the highlighted areas end up being around bogus locations.  Sometimes they "snap" back into place, but often not for *seconds*.

EXPECTED RESULTS:
 - Highlighted overlay should stay (approximately) over the find results.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: screencast of bug

Here's a screencast showing the bug (with mousewheel scrolling).  This is taken in a fresh profile, latest Nightly, 54.0a1 (2017-01-26) (64-bit)

It looks like maybe the overlay doesn't realize that it needs to repaint until I hover near its results with my mouse cursor, or something like that.

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I'm seeing this in GMail as well.

Comment 3

[Daniel Holbert [:dholbert]]

Confirmed, this affects Gmail too. (I can reproduce this there on the whole-inbox-view -- the default page you get to after logging in -- as well as on a long scrollable email-thread view.)

Also affects Google Groups, e.g. at https://groups.google.com/forum/?fromgroups#!forum/mozilla.dev.platform

Comment 4

[Mike de Boer [:mikedeboer]]

Alright, will fix this asap. Thanks for the updates!

Comment 5

[Mike de Boer [:mikedeboer]]

On OSX, I can see that the yellow outline fails to scroll with the page, but the white boxes are correcting as expected.
My guess is that on other platforms the 'scroll' event doesn't fire for mousewheel usage? Sounds unlikely to me, but Daniel can you see if you can reproduce it using other means of scrolling, like the scrollbars?

Comment 6

[Mike de Boer [:mikedeboer]]

I can see this issue in my Linux VM as it seems to fire much less scroll events... they feel tethered somehow. Especially with the 'bounce' effect on Ubuntu...

I'll do some printf-debugging...

Comment 7

[Daniel Holbert [:dholbert]]

(In reply to Mike de Boer [:mikedeboer] from comment #5)
> On OSX, I can see that the yellow outline fails to scroll with the page, but
> the white boxes are correcting as expected.

(I'm on Linux, BTW.)

* If I use the arrowkeys to scroll, then I see the behavior that you've described here. (yellow box fails to scroll, but white boxes move correctly, albeit with a small delay)

* If I drag the scrollbar:
 - while I'm dragging it, I see behavior as I described in comment 0 (all highlight-boxes misplaced, *occasionally* with some white boxes snapping into the correct spot)
 - after I stop dragging, the white boxes all snap to the right spots, but the yellow box is still misplaced.

Comment 8

[Mike de Boer [:mikedeboer]]

Attachment: Bug 1334213 - listen to scroll events at the capturing phase to make sure to catch them when event propagation is stopped by a script in the current document. Also make sure to always update the yellow range outline position.

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

Comment 9

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

Comment on attachment 8832035 [details]
Bug 1334213 - listen to scroll events at the capturing phase to make sure to catch them when event propagation is stopped by a script in the current document. Also make sure to always update the yellow range outline position.

https://reviewboard.mozilla.org/r/108452/#review110024

::: toolkit/modules/FinderHighlighter.jsm:1282
(Diff revision 1)
>        () => dict.busySelecting = true
>      ];
>      let target = this.iterator._getDocShell(window).chromeEventHandler;
>      target.addEventListener("MozAfterPaint", dict.highlightListeners[0]);
>      target.addEventListener("resize", dict.highlightListeners[1]);
> -    target.addEventListener("scroll", dict.highlightListeners[2]);
> +    target.addEventListener("scroll", dict.highlightListeners[2], true);

Please use a passive event listener. See https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener#Improving_scrolling_performance_with_passive_listeners

Comment 10

[Mike de Boer [:mikedeboer]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #9)
> Please use a passive event listener. See
> https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/
> addEventListener#Improving_scrolling_performance_with_passive_listeners

Woa, thanks for teaching me this! Patch updated.

Comment 11

[Mike de Boer [:mikedeboer]]

Comment on attachment 8832035 [details]
Bug 1334213 - listen to scroll events at the capturing phase to make sure to catch them when event propagation is stopped by a script in the current document. Also make sure to always update the yellow range outline position.

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

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f2f8cc19b856
listen to scroll events at the capturing phase to make sure to catch them when event propagation is stopped by a script in the current document. Also make sure to always update the yellow range outline position. r=jaws

Comment 13

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

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