1228407 - [e10s] `element.scroll({behavior: 'smooth'})` has different behavior when multi-process is enabled

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Simon Lydell]

Attachment: Smooth scrolling implemented using `element.scroll({behavior: 'smooth'})` on keydown

1. Open the attched test.html in a clean profile in Firefox with multi-process (e10s) enabled. The test page allows to scroll the page by using for instance `document.documentElement.scrollBy({top: 60, behavior: 'smooth'})` on each 'keydown' event for the 'k' key.
2. Hold down the 'k' key. This scrolls the page downwards smoothly as expected, but after a short while it stops, and then scrolls down in occasional outbursts. (The same thing happens when using the other scrolling keys mentioned in the test page.)
3. Turn off multi-process (e10s).
4. Hold the 'k' key on the test page again. Now the page scrolls downwards smoothly as expected, and keeps doing so in a constant speed for as long as 'k' is held.

I’d expect `element.scroll({behavior: 'smooth'})` to have the same behavior when called repeatedly, before the previous call has finished scrolling, regardless of whether multi-process is enabled or not. Preferably, the current non-multi-process behavior.

I’m a developer of the [VimFx][1] add-on, which uses the technique shown in the test page to implement the scrolling keyboard shortcuts of the add-on. While that is my primary use case, I think it is important that DOM methods that can be used by web page content (as demonstrated in the test page) behave the same regardless of whether multi-process is enabled or not.

If multi-process-by-default would be shipped to everyone tomorrow, I’d have to replace `element.scroll({behavior: 'smooth'})` with a custom implementation, which is a shame since it works so well in non-multi-process, and would replace one line of code with, say, a hundred or so for no reason.

[1]: https://github.com/akhodakivskiy/VimFx

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Hey Simon,

Do you still see the bug with APZ disabled? You can disable APZ by setting layers.async-pan-zoom.enabled to false in about:config.

Comment 2

[Simon Lydell]

I don’t see the bug with APZ disabled. Thanks for the workaround.

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Hey kats, this sounds like an APZ bug. I've marked this as blocking apz-desktop... is there anything else I need to do to make sure this bug is in the right place?

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Thanks for the test case! I can reproduce this as well. It seems very similar to bug 1217399, but is easier for me to reproduce. I'm changing it to block all-aboard-apz instead of apz-desktop because this is something we should fix before we ship APZ.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

I can start looking into this.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

I looked into this, and this is what I believe is happening when you hold down the 'k' key on the test case:

- the first smooth scroll request comes in to APZ and it starts the smooth scroll animation. the content-side scroll offset doesn't get updated yet
- additional smooth scroll requests come in to APZ with the same destination (which is based on the same content-side scroll offset). these can cancel the existing animation and start new ones. i have a patch locally that just updates the existing animation with the new destination, it doesn't matter.
- eventually the APZ animation comes to an end and it tries to send a repaint request to the main thread with the new final scroll offset
- however, every time the repaint request goes out, it gets rejected by the APZCCH code because the scroll generation number is stale, because the main thread is continually creating new smooth scroll requests and advancing the generation number
- this prevents the main thread from ever seeing the new scroll offset at the end of the animation, and so it keeps sending smooth scroll requests with the same destination.

Effectively the difference between the main-thread scrolling implementation and APZ for this use case is that the scroll generation number is perpetually rendered stale, and so APZ can't sync its new scroll offset to the main thread. The non-APZ implementation doesn't have this problem, so it works fine.

Trying to figure out what a good fix here is...

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Attachment: WIP

This fixes part of the problem at least. It stops the main thread from flooding APZ with the same request over and over unnecessarily. More importantly it stops increasing the scroll generation counter so APZ can actually send sync its scroll offset back to the main thread. The behaviour is better but still much worse compared to the up/down keys. I'll keep looking.

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Attachment: WIP part 2

And this should do the other bit. The remaining problem was that we kept restarting the compositor smooth scroll animation rather than just updating it to the new destination. Overall the scroll speed is still slower than the main-thread version but it's smooth at least.

Comment 9

[Simon Lydell]

I currently adjust the layout.css.scroll-behavior.spring-constant pref to change scroll speed. Are there any APZ prefs that make a difference as well?

Also, thanks for looking into this so quickly!

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 1 - Prevent layout from flooding APZ with smooth-scroll requests

Comment 11

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 2 - Rearrange some code/extract helper/update docs

This is a little scary given how brittle the code is but I think this makes the code a bit clearer as to why things are happening in the order that they are.

Comment 12

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 3 - Update smooth scroll animation destination rather than restarting it

Comment 13

[Kartikaya Gupta (email:[e-mail])]

(In reply to Simon Lydell from comment #9)
> I currently adjust the layout.css.scroll-behavior.spring-constant pref to
> change scroll speed. Are there any APZ prefs that make a difference as well?
> 
> Also, thanks for looking into this so quickly!

The layout.css.scroll-behavior.damping-ratio pref works with layout.css.scroll-behavior.spring-constant to define the smooth-scrolling physics, and should affect both APZ and non-APZ versions. I don't think that any other APZ prefs will affect the speed of a single smooth-scroll.

Comment 14

[Botond Ballo [:botond]]

Comment on attachment 8696469 [details] [diff] [review]
Part 1 - Prevent layout from flooding APZ with smooth-scroll requests

Review of attachment 8696469 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsGfxScrollFrame.cpp
@@ +2121,5 @@
>          }
>  
>          if (nsLayoutUtils::AsyncPanZoomEnabled(mOuter)) {
> +          if (mApzSmoothScrollDestination == mDestination &&
> +              mScrollGeneration == sScrollGenerationCounter) {

Couldn't sScrollGenerationCounter be incremented by a different scroll frame?

Comment 15

[Botond Ballo [:botond]]

Comment on attachment 8696471 [details] [diff] [review]
Part 2 - Rearrange some code/extract helper/update docs

Review of attachment 8696471 [details] [diff] [review]:
-----------------------------------------------------------------

The main change here seems to be that AcknowledgeScrollUpdate() is now called before CancelAnimation(). Is that significant?

Comment 16

[Botond Ballo [:botond]]

Comment on attachment 8696472 [details] [diff] [review]
Part 3 - Update smooth scroll animation destination rather than restarting it

Review of attachment 8696472 [details] [diff] [review]:
-----------------------------------------------------------------

This looks fine, but I'd like Kip to verify that calling SetDestination() on the axis models is sufficient and nothing else needs to be recomputed.

Comment 17

[Kartikaya Gupta (email:[e-mail])]

(In reply to Botond Ballo [:botond] from comment #14)
> Couldn't sScrollGenerationCounter be incremented by a different scroll frame?

Hm, good point. Yeah it could but if that were to happen it would just fall back to the old behaviour so at least this patch doesn't make things worse. In order for the other scroll frame to severely impact the behaviour here, the two scrollframes would have to be getting scrolled while perfectly interleaved (e.g. the content is doing alternating calls to scrollBy on two different scrollframes). It doesn't seem like an entirely implausible scenario so we should probably handle it. I'll see if I can update the patch to do so.

(In reply to Botond Ballo [:botond] from comment #15)
> 
> The main change here seems to be that AcknowledgeScrollUpdate() is now
> called before CancelAnimation(). Is that significant?

Yes, sending the AcknowledgeScrollUpdate early means that any subsequent repaint requests (such as the one sent in CancelAnimation) will take effect on the main thread rather than getting rejected because of a stale generation counter. This new organization of code is basically another way to fix bug 1216355. The advantage is that the CancelAnimation/StartSmoothScroll (which are logically one operation) are grouped together. It's also easier to enforce the brittle requirement of not sending repaint requests between the CopySmoothScrollInfo and the scroll update acknowledgement (again, because they are grouped logically in the code).

(In reply to Botond Ballo [:botond] from comment #16)
> This looks fine, but I'd like Kip to verify that calling SetDestination() on
> the axis models is sufficient and nothing else needs to be recomputed.

Sure. For the record that's what the main thread code does as well - https://dxr.mozilla.org/mozilla-central/rev/59bc3c7a83de7ffb611203912a7da6ad84535a5a/layout/generic/nsGfxScrollFrame.cpp#2161

Comment 18

[:kip (Kearwood Gilbert)]

Comment on attachment 8696472 [details] [diff] [review]
Part 3 - Update smooth scroll animation destination rather than restarting it

Review of attachment 8696472 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good

Comment 19

[Botond Ballo [:botond]]

Comment on attachment 8696469 [details] [diff] [review]
Part 1 - Prevent layout from flooding APZ with smooth-scroll requests

Review of attachment 8696469 [details] [diff] [review]:
-----------------------------------------------------------------

Feel free to leave the handling of multiple scroll frames being scrolled in an interleaved fashion, to a follow-up (maybe add a comment to mention it in that case).

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3ddca5b65d9b
https://hg.mozilla.org/integration/mozilla-inbound/rev/32df7136da75
https://hg.mozilla.org/integration/mozilla-inbound/rev/9474cf8b8d18

Comment 21

[Kartikaya Gupta (email:[e-mail])]

Backed out for test failures in https://hg.mozilla.org/integration/mozilla-inbound/rev/407e7946bee4

There were at least some M-e10s-2 Linux x64 debug and B2G Emulator ICS opt M-8 tests failing, possibly more.

Comment 22

[Kartikaya Gupta (email:[e-mail])]

I think I figured out the problem, the acknowledgement wasn't getting sent in the isFirstPaint case. Try push to see if there's other problems:

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

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f9a6158722a3
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a8a815d5a1c
https://hg.mozilla.org/integration/mozilla-inbound/rev/d55129069a1f

Comment 24

[Kartikaya Gupta (email:[e-mail])]

Got r+ from botond for the incremental change at https://hg.mozilla.org/try/rev/75c97cf1dfd0, re-landed the patches with that.

Comment 25

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/f9a6158722a3
https://hg.mozilla.org/mozilla-central/rev/7a8a815d5a1c
https://hg.mozilla.org/mozilla-central/rev/d55129069a1f