1760222 - Consider introducing fine-grained scroll linked effects detector

Status: RESOLVED FIXED

(Core :: Panning and Zooming, enhancement)

Description

[Hiroyuki Ikezoe (:hiro)]

ScrollLinkedEffectDetector is per document basis, once after it detected, we consider it persists.

When I was working on bug 1571758, I thought it should be per element basis, but given that all users' cases in bug 1759017 and bug 1759147 look like scrolling in the same scroll linked target scroller, so that's not an option here I have in my mind.

Rather what I have in my mind are;

1. Don't consider having scroll linked effect if changing properties values are not going to be changed, we don't currently check whether the value is changed or not.
2. Consider having scroll linked effect per refresh driver's most recent time stamp

I am not sure whether this will be a better result for users, because these will introduce another source of scroll jittering. So for example, if we detected a scroll linked effect in a vsync frame then we will use either the one-frame delayed offset or the latest one depending on how much time the frame spent on building display list etc. And if we don't detect scroll linked effect in a frame, we will always use the one-frame delayed offset.

Anyways, what I noticed in a couple of sites, google search results, phabricator, those sites are applicable in this case.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Here is a try build; https://treeherder.mozilla.org/jobs?repo=try&revision=494ac18c1458b7583156441fcd127598cbb6cba5&selectedTaskRun=ADaEXebdTBGh7wgO-KH1Gw.0

Michael and G, can you guys please volunteer to try this build to see whether it improves the situation or not? I am also curious it has some negative impacts on the test case in bug 1571758 comment 0. Thanks!

Comment 2

[Gregory Pappas [:gregp]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #1)

Here is a try build; https://treeherder.mozilla.org/jobs?repo=try&revision=494ac18c1458b7583156441fcd127598cbb6cba5&selectedTaskRun=ADaEXebdTBGh7wgO-KH1Gw.0

Michael and G, can you guys please volunteer to try this build to see whether it improves the situation or not? I am also curious it has some negative impacts on the test case in bug 1571758 comment 0. Thanks!

I do not see any lag or stutter in that try build

Comment 3

[Gregory Pappas [:gregp]]

(In reply to G from comment #2)

(In reply to Hiroyuki Ikezoe (:hiro) from comment #1)

Here is a try build; https://treeherder.mozilla.org/jobs?repo=try&revision=494ac18c1458b7583156441fcd127598cbb6cba5&selectedTaskRun=ADaEXebdTBGh7wgO-KH1Gw.0

Michael and G, can you guys please volunteer to try this build to see whether it improves the situation or not? I am also curious it has some negative impacts on the test case in bug 1571758 comment 0. Thanks!

I do not see any lag or stutter in that try build

I also tested this test case https://bug1571758.bmoattachments.org/attachment.cgi?id=9083374
It feels the same in v98 stable, the current nightly, and your try build. So it seems to have not regressed

Comment 4

[Michael Marley]

I tried it too and it is a dramatic improvement over the existing behavior, but it still seems to stutter at the moment the scroll-induced actions occur. For example, on the Google search results page, it stutters when you start scrolling down and the search bar overlay appears from the top.

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Thank you both! Great to hear!

(In reply to Michael Marley from comment #4)

I tried it too and it is a dramatic improvement over the existing behavior, but it still seems to stutter at the moment the scroll-induced actions occur. For example, on the Google search results page, it stutters when you start scrolling down and the search bar overlay appears from the top.

Yeah, that's exactly what the site does. It does change the header position style from absolute to fixed and mutating top style in response to scrolling. Once after scroll position reached to a point, they do just repeatedly set the top style value to 0px. Thus the new scroll-linked effect detector stop detecting it because the top value isn't changed. I am super surprised that you guys are very very sensitive in such kind of situations. :)

I've finished writing mochitests for this change; https://treeherder.mozilla.org/jobs?repo=try&revision=d8ff52a64208c4ca933963c0fd0c46cda2db83dc . I'd hope the test is stable on try server.

Comment 6

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #5)

I've finished writing mochitests for this change; https://treeherder.mozilla.org/jobs?repo=try&revision=d8ff52a64208c4ca933963c0fd0c46cda2db83dc . I'd hope the test is stable on try server.

There was an unused variable error. :/ A new try; https://treeherder.mozilla.org/#/jobs?repo=try&revision=45c5738a57eb7fceee9879d5f6570c1e2994f037

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1760222 - Add an API for JS to tell whether the document has scroll-linked effects or not. r?botond

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1760222 - Detect scroll-linked effects only in the same refresh driver's timestamp. r?botond

With this change, has-scroll-linked-effect flag won't persist so that we can
avoid choosing either the one-frame delayed scroll offset or the latest scroll
offset in cases where there's no longer effective scroll linked effect. It will
mitigate scroll jittering.

Depends on D141456

Comment 9

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1760222 - Test cases for `Ignore unchanged property value for scroll-linked effect detector`. r?botond

Depends on D141457

Comment 10

[Michael Marley]

Thanks for the fix!

Yeah, I'm ultra-sensitive to stuttery graphics. It's both a blessing and a curse.

Comment 11

[vladkosi]

I have tested the windows build from comment #6 and the stuttering seems to be gone.

Comment 12

[tsebrenko]

I have tested linux_x64 build from comment #6 and there is no stuttering.

Comment 13

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1760222 - Ignore unchanged property for scroll-linked effect detector. r=hiro

I think this is cleaner.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f65d2d719277
Add an API for JS to tell whether the document has scroll-linked effects or not. r=botond
https://hg.mozilla.org/integration/autoland/rev/e9b3e3f52aec
Detect scroll-linked effects only in the same refresh driver's timestamp. r=botond
https://hg.mozilla.org/integration/autoland/rev/aff6bf37365d
Ignore unchanged property for scroll-linked effect detector. r=hiro
https://hg.mozilla.org/integration/autoland/rev/1bf5e1ca3746
Test cases for `Ignore unchanged property value for scroll-linked effect detector`. r=botond

Comment 15

[Cristian Tuns]

Backed out for causing reftest failures on disable-apz-for-sle-pages.html

• Backout link
• Push with failures
• Failure Log
• Failure line: REFTEST TEST-UNEXPECTED-FAIL | layout/reftests/async-scrolling/disable-apz-for-sle-pages.html == layout/reftests/async-scrolling/disable-apz-for-sle-pages-ref.html | image comparison, max difference: 255, number of differing pixels: 788500

Comment 16

[Hiroyuki Ikezoe (:hiro)]

That's a very interesting reftest, it was originally for making sure apz.disable_for_scroll_linked_effects works as expected, indeed it did catch this behavior change. I will figure out a way to make it work with this behavior change.

Comment 17

[Michael Marley]

I hate to say it, but I'm also seeing the behavior where if I leave the browser running long enough, the jerky scrolling on SLE pages comes back. :(

Comment 18

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1760222 - Make disable-apz-for-sle-pages.html work with the new ScrollLinkedEffectDetector. r?botond

In subsequent commits, our ScrollLinkedEffectDetector is changed to;

1. Don't consider having scroll linked effect if changing properties values
   are not going to be changed, we don't currently check whether the value is
   changed or not.
2. Consider having scroll linked effect per refresh driver's most recent time
   stamp

With these changes, a reftest, disable-apz-for-sle-pages.html will no longer
work as it is. Fortunately our new ScrollLinkedEffectDetector is still per
document basis. So to make it work as expected, we need to;

1. Add a new invisible scrollable element
2. Keep listening scroll events for the element
3. Invoke scrolling the element
4. Change one of scroll linked effect CSS properties in the listener

Then our new ScrollLinkedListener still considers the document has a scroll
linked effect all the time.

This revised reftest fails with apz.disable_for_scroll_linked_effects=false,
which means it works as expected.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f8d5412ac6a6
Make disable-apz-for-sle-pages.html work with the new ScrollLinkedEffectDetector. r=botond
https://hg.mozilla.org/integration/autoland/rev/8943c31d926f
Add an API for JS to tell whether the document has scroll-linked effects or not. r=botond
https://hg.mozilla.org/integration/autoland/rev/e8c084bb09ea
Detect scroll-linked effects only in the same refresh driver's timestamp. r=botond
https://hg.mozilla.org/integration/autoland/rev/d06689014c21
Ignore unchanged property for scroll-linked effect detector. r=hiro
https://hg.mozilla.org/integration/autoland/rev/5b80a12c035b
Test cases for `Ignore unchanged property value for scroll-linked effect detector`. r=botond

Comment 20

[Hiroyuki Ikezoe (:hiro)]

(In reply to Michael Marley from comment #17)

I hate to say it, but I'm also seeing the behavior where if I leave the browser running long enough, the jerky scrolling on SLE pages comes back. :(

Two possibilities; One is probably the pages in question keep listening scroll events and having scroll linked effect impacts. The other is there's still something we haven't noticed yet.

Comment 21

[Cristina Cozmuta (:CrissCozmuta)]

https://hg.mozilla.org/mozilla-central/rev/f8d5412ac6a6
https://hg.mozilla.org/mozilla-central/rev/8943c31d926f
https://hg.mozilla.org/mozilla-central/rev/e8c084bb09ea
https://hg.mozilla.org/mozilla-central/rev/d06689014c21
https://hg.mozilla.org/mozilla-central/rev/5b80a12c035b

Comment 22

[Michael Marley]

The way I reproduced the jerkiness coming back was to start your test build from a few days back, go to a page that had previously been jerky (https://www.wunderground.com/weather/us/nc/cary is a good example) and confirm that it wasn't jerky anymore. Then, I closed that tab but left the browser running on the new tab page all day while I was at work. When I got home, I went to https://www.wunderground.com/weather/us/nc/cary again and saw that it was jerky again. Is there any way I can collect more information that might help figure out what is going on with that?

Comment 23

[Gregory Pappas [:gregp]]

(In reply to Michael Marley from comment #22)

The way I reproduced the jerkiness coming back was to start your test build from a few days back, go to a page that had previously been jerky (https://www.wunderground.com/weather/us/nc/cary is a good example) and confirm that it wasn't jerky anymore. Then, I closed that tab but left the browser running on the new tab page all day while I was at work. When I got home, I went to https://www.wunderground.com/weather/us/nc/cary again and saw that it was jerky again. Is there any way I can collect more information that might help figure out what is going on with that?

Since this bug has already been closed I suggest filing a new one with a title like "Scrolling becomes jerky on wunderground.com after leaving Firefox open for a long time"

Comment 24

[Hiroyuki Ikezoe (:hiro)]

Yeah, filing a new bug would be nice with some information, two profile results; one is during the jerky is happening, the other is not jerky one. If both have no heavy task appeared, it's probably due to bug 1571758. That said, even if bug 1571758 is the cause, it may be possible that the site is actually doing scroll linked effect at the moment.

Comment 25

[Michael Marley]

I was trying to reproduce that with the latest nightly build so I could open another report, but I wasn't able to. I guess something is different now.

Comment 26

[Alexey104]

For me scrolling is much better in the latest Nightly compared to 98 build, but stutters didn't go completely. Still observe lags. My good and bad profiles are here:
https://bugzilla.mozilla.org/show_bug.cgi?id=1761812