1359808 - Animating translateY and the page's scroll position makes the translation stutter

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect, P3)

Description

[u482975]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:54.0) Gecko/20100101 Firefox/54.0
Build ID: 20170418004027

Steps to reproduce:

I'm animating both the documentElement.scrollTop and the translateY on an element. And it is causing a stutter in the animated translate.

I've made a jsfiddle to demonstrate: jsfiddle.net/tt1z8ddg/10/

I happen to have an older version of Firefox installed as well, version 44. And on that version is does animate smoothly.

Comment 1

[Nicolas Silva [:nical]]

kats: could this be APZ-related?

Comment 2

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

It looks smooth to me. Can you post a video of what you're seeing? I'm assuming based on your UA in comment 0 that you're on Windows and using 54.0 release.

Comment 3

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

Oh actually I can reproduce on Windows. Doesn't happen on OS X. I'll investigate.

Comment 4

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

Attachment: Standalone test case (click on page to trigger)

Comment 5

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

Interestingly, the standalone test case works fine if I load it as-is. If I open the devtools panel it lags.

Comment 6

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

Probably something to do with empty transactions and the APZ code.

Comment 7

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

I looked into this a bit more. I can actually reproduce it consistently across platforms now, not sure why I couldn't before. It looks like what happens is that the page first changes the transform. This can be done using an empty transaction - nsIFrame::TryUpdateTransformOnly returns true [1]. Then the page updates the scroll position which the APZ paint-skipping code [2] also computes as being doable with an empty transaction. However, the problem is that the transform is computed assuming the "old" scroll offset, and so is not adjusted properly. In fact, TryUpdateTransformOnly has code to catch this scenario [3] but it doesn't trigger because the scroll position change happens *after* the transform change. If, in the test page, I reverse the order of the transform and scroll position updates, the problem goes away because we start scheduling full paints instead.

The obvious solution is to modify the APZ paint-skipping code so that if it detects a pending transform on the layer, it aborts the empty transaction and schedules a full paint instead. However there might be multiple layers for the scrollframe, and we'd have to check all the descendants of all of those layers, so it's a little less clean than I'd like. Also if we add more things schedule empty transactions, we'd have to update all these pieces of code to check for every other piece of code which doesn't really scale well. It might be better to just set a flag somewhere on the layer manager that says "an empty transaction was scheduled for X reason" and then if we try to set the flag with two different values of X we abort and schedule a full paint instead.

[1] http://searchfox.org/mozilla-central/rev/5dadcbe55b4ddd1e448c06c77390ff6483aa009b/layout/generic/nsFrame.cpp#6656
[2] http://searchfox.org/mozilla-central/rev/5dadcbe55b4ddd1e448c06c77390ff6483aa009b/layout/generic/nsGfxScrollFrame.cpp#2883
[3] http://searchfox.org/mozilla-central/rev/5dadcbe55b4ddd1e448c06c77390ff6483aa009b/layout/generic/nsFrame.cpp#6621

Comment 8

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

P3 and wontfix for 55 since this is a fairly rare situation I think. And it can be easily worked around by swapping the order of the scroll position and transform updates in JS.

Comment 9

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

Attachment: Bug 1359808 - Don't do empty transactions for scroll updates if there are already pending transforms in the layer tree.

The pending transforms must have been computed using the older scroll offset
values, which means that updating the scroll offsets without recomputing the
transforms will make them wrong. If we do an empty transaction for the scroll
offset updates, the transforms will not get computed. This patch catches this
scenario and schedules a full paint instead of the empty transaction instead.
The case where the scroll offset is modified *before* the transform is already
handled by code in nsIFrame::TryUpdateTransformOnly.

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

Comment 10

[Markus Stange [:mstange]]

Comment on attachment 8885753 [details]
Bug 1359808 - Don't do empty transactions for scroll updates if there are already pending transforms in the layer tree.

https://reviewboard.mozilla.org/r/156552/#review164298

Looks good, sorry for the delay.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b5294562d3f5
Don't do empty transactions for scroll updates if there are already pending transforms in the layer tree. r=mstange

Comment 12

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

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