Nothing Special   »   [go: up one dir, main page]

Closed Bug 1741255 Opened 3 years ago Closed 3 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
97 Branch
Tracking Status
firefox97 --- fixed

People

(Reporter: boris, Assigned: boris)

References

(Blocks 3 open bugs)

Details

Attachments

(2 files, 1 obsolete file)

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.

Summary: Make animation-play-state:paused work properly → Make animation-play-state:paused work properly for scroll-linked animations
Assignee: nobody → boris.chiou

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

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.

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

Summary: Make animation-play-state:paused work properly for scroll-linked animations → Make sure scroll animations go into Play/Pause state from PlayPending/PausePending
Attachment #9253542 - Attachment description: Bug 1741255 - Fix playing finished scroll animations on reverse scrolling. → Bug 1741255 - Fix playing finished scroll animations on reversing scrolling.

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.

(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.

(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.

(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).

Status: NEW → ASSIGNED

(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.

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
Attachment #9253541 - Attachment description: Bug 1741255 - Fix the update of pending state for scroll animations. → Bug 1741255 - Don't put the scroll animations into PendingAnimationTracker.
Blocks: 1746101

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.

Attachment #9253542 - Attachment is obsolete: true
Blocks: 1746107
No longer blocks: 1746101
Depends on: 1746101
Blocks: 1746116
Pushed by bchiou@mozilla.com:
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
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/32076 for changes under testing/web-platform/tests
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 97 Branch
Upstream PR merged by moz-wptsync-bot
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: