1741255 - Make sure scroll animations go into Play/Pause state from PlayPending/PausePending

Status: RESOLVED FIXED

(Core :: CSS Transitions and Animations, defect, P3)

Description

[Boris Chiou [:boris]]

Bug 1676791 makes scroll-linked animations work via CSS animation. However, animation-play-state:paused doesn't work after this.

Not sure if we have to update PendingAnimationTracker, or we just miss some spec updates. Let's address this issue in this bug.

Comment 1

[Boris Chiou [:boris]]

The root cause is that we rely on Animation::mPendingReadyTime to change the animation's state from pending to non-pending. However, this is for time-based animation. In order to fix this, a better long-term way is to introduce the progress-based animation (spec issue: https://github.com/w3c/csswg-drafts/issues/4862, chrome issue: [1]). For example, in current Chrome Canary (version 98.xxx), animation.currentTime returns a percentage value, instead of a time value (so this doesn't match the current draft spec).

Using a fake duration may work but it doesn't match the spec issue. Besides, I noticed we shouldn't use a fixed specified timing for scroll animations. For example, in Chrome, delay works by calculating the percentage of delay and duration. e.g. if delay is 1s, duration is 5s, the scroll animation starts to move after we scroll 20% of scroll-range. And fill should work, based on the animation-fill property.

Besides, I also noticed there is some magic for handling duration, e.g. if duration is inf, Chrome doesn't create the scroll-linked animation [2]. This may be related to the comment in the spec issue.

So in this bug, a short-term way is: we may have to update how we handle timing first, and then use some fake time value to make the animation change the pending status. (Obviously, the paused scroll animations are always in paused pending, so we never really pause the animation. And we have the similar issue for PlayPending and Play.)

I'm not planning to introduce the progress-based animation until we get more information from the spec issue.

[1] https://bugs.chromium.org/p/chromium/issues/detail?id=1140602
[2] https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/animation/animation.cc;l=202-238;drc=fde2db9853cab5681586ea71d877f4cefd5eee13

Comment 2

[Boris Chiou [:boris]]

Attachment: Bug 1741255 - Don't put the scroll animations into PendingAnimationTracker.

We should update the pending state once the scroll animation has a
valid progress (i.e. current time). So the animation goes into play or
pause, instead of forever play-pending or pause-pending.

Comment 3

[Boris Chiou [:boris]]

Attachment: Bug 1741255 - Fix playing finished scroll animations on reversing scrolling.

Based on the step 1 in [1], we have to define unconstrainted current time
and use it to make sure we can change the direction of timeline
(especially for playing scroll animations).

Besides, we shouldn't remove the scroll-animations at finished state
because it is still possible to scroll it on reverse direction.

[1] https://drafts.csswg.org/web-animations-1/#updating-the-finished-state

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Boris, I'd expect this bug to add a couple of print reftests. What I have in my mind so far is;

1. a scroll linked animation linked to the root scroller
2. a scroll linked animation linked to an element
3. a scroll linked animation linked to the root scroller of an iframe

I presume only 3) works fine as of now though.

Comment 5

[Boris Chiou [:boris]]

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

Boris, I'd expect this bug to add a couple of print reftests. What I have in my mind so far is;

1. a scroll linked animation linked to the root scroller
2. a scroll linked animation linked to an element
3. a scroll linked animation linked to the root scroller of an iframe

I presume only 3) works find as of now though.

I believe 2) is not supported now for scroll-linked animation because we always use root scroller now.

Besides, do you have any print reftest example to me? I guess we have some but not sure which folder. Thanks.

Comment 6

[Hiroyuki Ikezoe (:hiro)]

(In reply to Boris Chiou [:boris] (PTO after 12/20) from comment #5)

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

Boris, I'd expect this bug to add a couple of print reftests. What I have in my mind so far is;

1. a scroll linked animation linked to the root scroller
2. a scroll linked animation linked to an element
3. a scroll linked animation linked to the root scroller of an iframe

I presume only 3) works find as of now though.

I believe 2) is not supported now for scroll-linked animation because we always use root scroller now.

Right, but it's worth adding them altogether.

Besides, do you have any print reftest example to me? I guess we have some but not sure which folder. Thanks.

you can find them in testing/web-platform/tests, wpt print reftests have -print suffix.

Comment 7

[Hiroyuki Ikezoe (:hiro)]

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

Boris, I'd expect this bug to add a couple of print reftests. What I have in my mind so far is;

1. a scroll linked animation linked to the root scroller

Now I think this should probably work fine (the machinery Emily introduced should handle this properly).

Comment 8

[Boris Chiou [:boris]]

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

you can find them in testing/web-platform/tests, wpt print reftests have -print suffix.

I see. There is a doc for wpt print test: https://web-platform-tests.org/writing-tests/print-reftests.html.

Comment 9

[Boris Chiou [:boris]]

Attachment: Bug 1741255 - Add wpt print tests for basic scroll animations.

For the following basic cases:

1. a scroll linked animation linked to the root scroller
2. a scroll linked animation linked to an element
3. a scroll linked animation linked to the root scroller of an iframe

Comment 10

[Phabricator Automation]

Comment on attachment 9253542 [details]
Bug 1741255 - Fix playing finished scroll animations on reversing scrolling.

Revision D132751 was moved to bug 1746101. Setting attachment 9253542 [details] to obsolete.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2540bece2242
Don't put the scroll animations into PendingAnimationTracker. r=hiro
https://hg.mozilla.org/integration/autoland/rev/96e35a880162
Add wpt print tests for basic scroll animations. r=hiro

Comment 12

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/32076 for changes under testing/web-platform/tests

Comment 13

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/2540bece2242
https://hg.mozilla.org/mozilla-central/rev/96e35a880162

Comment 14

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot