Closed Bug 1359808 Opened 3 years ago Closed 2 years ago

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

Categories

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

54 Branch
Unspecified
Windows
defect

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox54 --- wontfix
firefox55 --- wontfix
firefox56 --- fixed

People

(Reporter: MartijnJoosstens, Assigned: kats)

References

()

Details

(Whiteboard: [gfx-noted])

Attachments

(2 files)

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.
Component: Untriaged → Graphics: Layers
Product: Firefox → Core
kats: could this be APZ-related?
Flags: needinfo?(bugmail)
Priority: -- → P3
Whiteboard: [gfx-noted]
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.
Flags: needinfo?(bugmail) → needinfo?(MartijnJoosstens)
OS: Unspecified → Windows
Probably something to do with empty transactions and the APZ code.
Status: UNCONFIRMED → NEW
Component: Graphics: Layers → Panning and Zooming
Ever confirmed: true
Priority: P3 → --
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
Attachment #8883548 - Attachment description: Standalone test case → Standalone test case (click on page to trigger)
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.
Assignee: nobody → bugmail
Priority: -- → P3
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.
Attachment #8885753 - Flags: review?(mstange) → review+
Pushed by kgupta@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/b5294562d3f5
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.