transform translateX animation with requestAnimationFrame is jittery
Categories
(Core :: CSS Transitions and Animations, defect, P3)
Tracking
()
People
(Reporter: ksenia, Unassigned)
References
()
Details
(Whiteboard: [layout:backlog:quality])
Attachments
(2 files, 2 obsolete files)
1.77 KB,
text/html
|
Details | |
1.98 KB,
patch
|
Details | Diff | Splinter Review |
This was initially reported here:
https://github.com/webcompat/web-bugs/issues/50046
To reproduce open the attached test case in Firefox Preview or Preview Nightly.
Expected:
Timer bar decreases smoothly
Actual:
Timer bar jumps forward and back
Reporter | ||
Comment 1•5 years ago
|
||
Comment 2•5 years ago
|
||
The testcase uses requestAnimationFrame to animate transform: translateX(p%) one percentage point per second, and relies on transition to "fill in the gaps". Due to timing inaccuracies and rounding, the RAF callback
- does not always trigger at the right time and,
- might skip or repeat percentage values for translate.
Brian: is this combination of transforms and transitions "supported" with animations?
Updated•5 years ago
|
Comment 3•5 years ago
|
||
Sorry for the massive delay here, this arrived just as I went on PTO.
I think there is something broken about the way we update transitions running on the compositor.
If we explicitly flush styles in the animate()
function in the test case the problem goes away. That suggests that we're updating the transition on the compositor with old styles from the main thread (and likely we're throttling updates on the main thread too making the style older still).
We have code specifically to avoid this that we introduced for Firefox OS but I guess it's not working correctly here.
We've seen other cases where updating transitions on the compositor was not behaving as expected (there was one Xidorn filed but I can't find it right now) so this is probably a good opportunity to debug what is going on here.
Perhaps Boris or Hiro is interested in digging into this?
Comment 4•5 years ago
|
||
(In reply to Brian Birtles (:birtles) from comment #3)
We have code specifically to avoid this that we introduced for Firefox OS but I guess it's not working correctly here.
I checked this very quickly by some logs. It seems this function works well sometimes (i.e. it calculates the correct start value and then replaces the transition well). However, the jittery happens when its computedTiming.mProgress
is null, so we don't calculate the start value. I will keep checking what happens.
Comment 5•5 years ago
|
||
If we still want to keep using the tweak, I'd suggest that we do the tweak on the compositor not on the main thread.
Comment 6•5 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) from comment #5)
If we still want to keep using the tweak, I'd suggest that we do the tweak on the compositor not on the main thread.
Filed bug 1626165. That should fix this issue.
Comment 7•5 years ago
|
||
Moving into CSS Transitions and Animations.
Updated•5 years ago
|
Comment 8•5 years ago
|
||
I'm curious why the start positions get so out of sync here. I thought we did a mini-flush before triggering transitions to bring the main thread up to date when it was throttled? Looking at the test case though it seems like the main thread value is very old.
Comment 9•5 years ago
|
||
The component has been changed since the backlog priority was decided, so we're resetting it.
For more information, please visit auto_nag documentation.
Updated•5 years ago
|
Comment 10•5 years ago
|
||
For what it's worth, I think this bug (or bug 1626165 if we fix it there), is fairly high priority. This has come up a number of times and I've personally had to work around it a few times in Web apps by adding extra style flushes. I think it's even more noticeable than bug 1540906 so it's probably on par with that for priority.
Updated•5 years ago
|
Comment 11•5 years ago
|
||
We set mReplacedTransition only when it has current effect (i.e. in active
phase). However, when we replace the start value with mReplacedTransition,
the current time stamp might be a little bit late and the progress of the
mReplacedTransition might be just finished, so it's in after phase.
We have to handle the case of after phase case, so we send the up-to-date
transition into the compositor.
Comment 12•5 years ago
|
||
I could find a problem here in nsTransitionManager::ConsiderInitiatingTransition. We get the start value for the new transition from |aOldStyle|, but at the moment, the transform style has been being throttling, so the |aOldStyle| is pretty much stale. As Brian suspected this is a regression by Stylo.
When we call this ConsiderInitiatingTransition
it's in a sequential task after a normal style traversal, we skipped the first animation restyling before this normal traversal. It's hard to recall why I did avoid making the element dirty when the transform style is changed...
Comment 13•5 years ago
|
||
Dropping bug 1626165 from the dependency list since this issue is not related to the start time tweak machinery.
Comment 14•5 years ago
|
||
This is a patch I wrote before for bug 1607723. I noticed this patch also fixes this issue.
An alternative way I can think of to fix this issue requires the third animation only restyling, which sounds awful. :) FWIW, the alternative way is, 1) defer creating a new transition if we are going to replace the old transition (i.e. defer discarding the old transition) 2) post restyle::layer for the target element 3) restyle the throttled animation in the second animation restyling 4) trigger a sequential task to create a new transition and discard the old transition somehow 5) restyle the new transition in the third animation restyling. I understand this sounds....
Another hacky way I can think of is to use the timeline time when we replace the old transition instead of TimeStamp::Now() if TimeStamp::Now() - the timeline time < vsync interval
.
Comment 15•5 years ago
•
|
||
(In reply to Hiroyuki Ikezoe (:hiro) from comment #14)
Created attachment 9140703 [details] [diff] [review]
A patch to fix this issueThis is a patch I wrote before for bug 1607723. I noticed this patch also fixes this issue.
After applying this patch, my log looks pretty different. In most cases, the original transition is correct, so we don't have to replace the start value (i.e. computedTiming.mProgress.IsNull()
is true in CSSTransition::UpdateStartValueFromReplacedTransition()
). And even if we replace the start value, the original start value is pretty close to the new start value. This behavior looks like much close to what hiro mentioned in Riot: UpdateStartValueFromReplacedTransition()
is for the low-spec device (or busy main thread case), not for the general case.
It seems we are close to the root cause.
Updated•5 years ago
|
Comment 16•4 years ago
|
||
Because this bug's Severity has not been changed from the default since it was filed, and it's Priority is P3
(Backlog,) indicating it has been triaged, the bug's Severity is being updated to S3
(normal.)
Updated•4 years ago
|
Comment 17•4 years ago
|
||
(In reply to Firefox Bug Husbandry Bot from comment #16)
Because this bug's Severity has not been changed from the default since it was filed, and it's Priority is
P3
(Backlog,) indicating it has been triaged, the bug's Severity is being updated toS3
(normal.)
Is it weird that the Firefox Bug Husbandry Bot should reset the assignee?
Comment 18•4 years ago
|
||
Oops, I should have noted about the patch in comment 14.
I know it's suboptimal, because it's flushes unrelated throttling animations, that said it should work in the wild. But there is a fundamental issue in the patch, that is that it breaks our reftest-no-flush machinery. When we use reftest-no-flush, we also use reftest-wait on the root document element, which means when the reftest-wait is removed, the document element gets dirty, which means we flush all throttled animations in the document. That's quite unfortunate.
Updated•3 years ago
|
Updated•2 years ago
|
Description
•