[web-animations-1] Initialize start time of scroll animations to zero (#2075) #4842

ogerchikov · 2020-03-06T22:47:04Z

Initializing start time of scroll animations to zero is required to have animation effect progress in sync with the timeline progress.

Changes made:

Removed responding to a newly inactive timeline section ([web-animations-1] Remove "responding to a newly inactive timeline" section? #2080).
Updated playing/pausing animation procedures to initialize start time to zero for finite timelines. This makes animation current time equal to scroll timeline current time or unresolved if the timeline is inactive.
Updated pausing animation procedure to execute animation pending pause task when inactive timeline becomes active. This makes ready time resolved at the time of the execution and, as a result, produces resolved hold_time.
Updated definition of idle play state to include check for unresolved start time. This is required so
playing animations attached to inactive timeline return "running" state. Without this change play pending animations attached to inactive timeline return "running" state, while playing animations attached to inactive timeline return "idle" state.

birtles · 2020-03-12T06:02:38Z

web-animations-1/Overview.bs

+    :   If |animation|'s <a>current time</a> is <a>unresolved</a>,
+        <a>start time</a> is <a>unresolved</a>, the <var>auto-rewind</var>
+        flag is true and <em>either</em>:
+        *   the |animation| has no associated <a>timeline</a>, or
+        *   the associated <a>timeline</a> is
+            <a lt="inactive timeline">inactive</a>


I'm afraid I don't quite understand the reasoning behind these conditions. I thought we discussed setting the start time to zero for scroll-based timelines? But they won't necessarily have no timeline, right?

Setting start time to zero and hold time to null produces unresolved current time. This is to ensure no animation effect is applied when the timeline is inactive.

Just to be clear regarding your comment about zero start time initialization for scroll timeline, I am planning to make this change later, but as part of a separate PR. The goal of this PR is just handling inactive/null timelines.

I see. So the tricky part is making an inactive timeline produce an unresolved current time in the animation whilst also still allowing animations without a timeline to be seeked to a particular time.

This is complicated because we might have Web compat issues here given that Chrome and Firefox both ship the Animation() constructor so the behavior of calling play() on animations without a timeline is already observable.

That said, we probably don't commonly encounter inactive timelines, so I wonder if we can make this change simply apply to inactive timelines?

Or is it a bit odd that calling play() on an animation without a timeline behaves differently from calling play() on an animation attached to an inactive timeline? From my understanding, both would make the animation "running" and pending, but in the first case the currentTime would be zero, but in the second case the currentTime would be null.

With #4899 proposal, if accepted, play/pause procedures will be updated to initialize start time to zero for both active and inactive finite timelines. Event without #4899, we probably should be initializing start time to 0 for finite timeline linked animations regardless of the timeline activeness.

Having said this, we need to decide which behavior we expect from animations without a timeline. In #2066 the "half" agreement was to treat a null timeline as inactive timeline, meaning animations don't expose animation effect. If this indeed the desired behavior, perhaps we can find a way to phaseout already shipped behavior in favor of the desired one.

Changed to initialize start time of finite timeline animations to 0. No changes are made for the animations that don't have a timeline. Note, in this change we initialize start time to 0 regardless of the timeline activeness. It achieves the desired effect when the timeline is active (anim.currentTime==timeline.currentTime) and when the timeline is inactive (animation.currentTime = null).

birtles · 2020-03-12T06:03:48Z

web-animations-1/Overview.bs


    * |animation|'s [=hold time=] is [=unresolved=], and
+    * |animation|'s [=start time=] is [=unresolved=], and


I'm not quite sure I follow this but I think it probably relates back to my comment here.

This is required to avoid early exit if start time is set.

I'm not sure about this. Won't this mean that calling play() on an already running animation makes it become pending? (Since the hold time is unresolved in that case but the start time is not -- previous we would abort the procedure in that case but now we won't.)

We could probably fix the above by simply adding some extra flag to this procedure to indicate if we made a change that means that we need to wait for a pending task to run. This might be a more elegant way, however.

Added the flag.

birtles · 2020-03-12T06:04:33Z

web-animations-1/Overview.bs

+    :   If |animation|'s <a>start time</a> is <a>unresolved</a> and
+        <em>either</em>:
+        *   the |animation| has no associated <a>timeline</a>, or
+        *   the associated <a>timeline</a> is
+            <a lt="inactive timeline">inactive</a>


As with my first comment, I'm afraid I don't follow why we have these particular conditions.

The explanation is the same as in the first comment.

Please see the first comment.

ogerchikov · 2020-03-27T22:01:19Z

I repurposed this change to initialize start time of scroll animations to 0 instead of handling inactive timelines.

web-animations-1/Overview.bs

ogerchikov · 2020-03-27T22:14:19Z

@majido @birtles - can you please take a look at these changes? I believe we are in agreement that start time of scroll animations should be initialized to 0.

…inite.

birtles

Sorry for the delay on this. I have a couple of comments that I think are worth considering.

web-animations-1/Overview.bs

birtles · 2020-04-09T05:18:13Z

web-animations-1/Overview.bs


    * |animation|'s [=hold time=] is [=unresolved=], and
+    * |animation|'s [=start time=] is [=unresolved=], and


I'm not sure about this. Won't this mean that calling play() on an already running animation makes it become pending? (Since the hold time is unresolved in that case but the start time is not -- previous we would abort the procedure in that case but now we won't.)

birtles · 2020-04-09T05:18:59Z

web-animations-1/Overview.bs

@@ -1324,6 +1304,9 @@ follows:
    perform the steps according to the first matching condition from below:

    <div class="switch">
+    :   If |animation|'s <a>start time</a> is <a>unresolved</a> and the
+        associated <a>timeline</a> is not [=monotonically increasing=]
+    ::  Let <var>animation</var>'s <a>start time</a> be zero.


(Presumably whatever we decide for the play procedure will also apply here.)

Changed similarly to Play.

ogerchikov · 2020-04-09T23:11:38Z

@birtles - thanks for your feedback! All your comments are addressed, please take a look. Note, I intentionally don't change null timelines behaviors. I am happy to do so but not in this PR.

birtles

Thanks for all your work on this. I'm really nervous about changing these very complicated procedures but hopefully this works! Thanks again!

web-animations-1/Overview.bs

birtles · 2020-04-10T07:33:07Z

web-animations-1/Overview.bs

 ::  The animation has a resolved <a>current time</a> that changes on each
-    <a>animation frame</a> (provided the [=playback rate=] is not zero).
+    <a>animation frame</a> (provided the [=playback rate=] is not zero
+    and the <a>timeline</a> is [=monotonically increasing=]). If the
+    <a>timeline</a> is not [=monotonically increasing=], the
+    <a>current time</a> may not change on each <a>animation frame</a> or
+    is unresolved if the <a>timeline</a> is
+    <a lt="inactive timeline">inactive</a>. In the latter case
+    the animation has no effect.


Perhaps this could be simplified to something like...

The animation has a resolved current time that changes on each animation frame (provided the playback rate is not zero and the animation has an associated timeline that is active and monotonically increasing).

Changed per your suggestion. However I still feel that we are not explaining behavior of animations attached to active/inactive finite timelines in terms of current time update and having effect.

Yes, that's true. But I think this is non-normative text so perhaps it doesn't need to describe everything?

web-animations-1/Overview.bs

birtles · 2020-04-10T07:35:28Z

web-animations-1/Overview.bs

@@ -1953,6 +1996,8 @@ condition from the following:
 :   <em>All</em> of the following conditions are true:
    *   The <a>current time</a> of <var>animation</var> is <a>unresolved</a>,
        <em>and</em>
+    *   the <a>start time</a> of <var>animation</var> is <a>unresolved</a>,
+        <em>and</em>


I'm a little bit nervous about changing this part since I'm afraid it might have unintended consequences that I haven't been able to anticipate. However, if we can make this change and none of the existing WPT break, then I guess it's ok.

Without this change, having current time unresolved automatically means start time being unresolved. That's how I convinced myself it should not be any backward compatibility problems.

Without this change, having current time unresolved automatically means start time being unresolved. That's how I convinced myself it should not be any backward compatibility problems.

Is that true? If you have no timeline or an inactive timeline, I think you can still have a resolved start time with an unresolved current time?

After you mentioned null timeline, the case I can think of is when running animation.timeline is set to null. However in Chrome animation.timeline is read only, so hopefully user scripts don't rely on this behavior.

In regards to inactive timelines, I don't think DocumentTimeline is expected to become newly inactive. Even if it does become inactive, the spec says we should use previous resolved time. "Responding to newly inactive timelines" is not implemented in Chrome, so, again, hopefully user scripts don't rely on this behavior.

I think the major impact of this change is that we are changing the mental model of animation play state. Before, 'idle'/'running' implied activeness of a timeline the animation is attached to, while now 'idle' play state can only be achieved by user actions - cancelling a running animation or not calling play/pause.

After you mentioned null timeline, the case I can think of is when running animation.timeline is set to null. However in Chrome animation.timeline is read only, so hopefully user scripts don't rely on this behavior.

I think Chrome is shipping the Animation() constructor, however, which allows creating animations without a timeline so I think it's possible it's already being relied on. It does seem unlikely, however.

ogerchikov · 2020-04-10T18:14:40Z

@birtles - thanks again for reviewing and providing great feedback. All your comments are addressed.

birtles · 2020-04-11T08:24:56Z

Looks great to me. I'll be interested to hear your answer to the one question I added but feel free to merge this (or do you not have access to do it?).

Implemented web-animations-1 spec changes introduces in [1]. - Update play and pause procedures to initialize start time of scroll animations to zero. - Updated calculate play state procedure to return "running" state for animations that has start time resolved. - Added/modified tests reflecting spec changes. [1] w3c/csswg-drafts#4842 Bug: 1070637 Change-Id: Ic83995899b2f3f8d8f985f84b8a2b438bbad7c35

Implemented web-animations-1 spec changes introduces in [1]. - Update play and pause procedures to initialize start time of scroll animations to zero. - Updated calculate play state procedure to return "running" state for animations that has start time resolved. - Added/modified tests reflecting spec changes. [1] w3c/csswg-drafts#4842 Bug: 1070637 Change-Id: Ic83995899b2f3f8d8f985f84b8a2b438bbad7c35 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2150687 Commit-Queue: Olga Gerchikov <gerchiko@microsoft.com> Reviewed-by: Majid Valipour <majidvp@chromium.org> Reviewed-by: Kevin Ellis <kevers@chromium.org> Cr-Commit-Position: refs/heads/master@{#761974}

…ns to zero., a=testonly Automatic update from web-platform-tests Initialize start time of scroll animations to zero. Implemented web-animations-1 spec changes introduces in [1]. - Update play and pause procedures to initialize start time of scroll animations to zero. - Updated calculate play state procedure to return "running" state for animations that has start time resolved. - Added/modified tests reflecting spec changes. [1] w3c/csswg-drafts#4842 Bug: 1070637 Change-Id: Ic83995899b2f3f8d8f985f84b8a2b438bbad7c35 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2150687 Commit-Queue: Olga Gerchikov <gerchiko@microsoft.com> Reviewed-by: Majid Valipour <majidvp@chromium.org> Reviewed-by: Kevin Ellis <kevers@chromium.org> Cr-Commit-Position: refs/heads/master@{#761974} -- wpt-commits: 46978682c5d3e2ff08468be05564307a9a75e06b wpt-pr: 23145

…ns to zero., a=testonly Automatic update from web-platform-tests Initialize start time of scroll animations to zero. Implemented web-animations-1 spec changes introduces in [1]. - Update play and pause procedures to initialize start time of scroll animations to zero. - Updated calculate play state procedure to return "running" state for animations that has start time resolved. - Added/modified tests reflecting spec changes. [1] w3c/csswg-drafts#4842 Bug: 1070637 Change-Id: Ic83995899b2f3f8d8f985f84b8a2b438bbad7c35 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2150687 Commit-Queue: Olga Gerchikov <gerchikomicrosoft.com> Reviewed-by: Majid Valipour <majidvpchromium.org> Reviewed-by: Kevin Ellis <keverschromium.org> Cr-Commit-Position: refs/heads/master{#761974} -- wpt-commits: 46978682c5d3e2ff08468be05564307a9a75e06b wpt-pr: 23145 UltraBlame original commit: 8082006b84318e9f8564abb4265107ddf769774c

…ns to zero., a=testonly Automatic update from web-platform-tests Initialize start time of scroll animations to zero. Implemented web-animations-1 spec changes introduces in [1]. - Update play and pause procedures to initialize start time of scroll animations to zero. - Updated calculate play state procedure to return "running" state for animations that has start time resolved. - Added/modified tests reflecting spec changes. [1] w3c/csswg-drafts#4842 Bug: 1070637 Change-Id: Ic83995899b2f3f8d8f985f84b8a2b438bbad7c35 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2150687 Commit-Queue: Olga Gerchikov <gerchikomicrosoft.com> Reviewed-by: Majid Valipour <majidvpchromium.org> Reviewed-by: Kevin Ellis <keverschromium.org> Cr-Commit-Position: refs/heads/master{#761974} -- wpt-commits: 46978682c5d3e2ff08468be05564307a9a75e06b wpt-pr: 23145 UltraBlame original commit: e563517d3d930c71fde7bffe58aa64a68dce2d15

…ns to zero., a=testonly Automatic update from web-platform-tests Initialize start time of scroll animations to zero. Implemented web-animations-1 spec changes introduces in [1]. - Update play and pause procedures to initialize start time of scroll animations to zero. - Updated calculate play state procedure to return "running" state for animations that has start time resolved. - Added/modified tests reflecting spec changes. [1] w3c/csswg-drafts#4842 Bug: 1070637 Change-Id: Ic83995899b2f3f8d8f985f84b8a2b438bbad7c35 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2150687 Commit-Queue: Olga Gerchikov <gerchikomicrosoft.com> Reviewed-by: Majid Valipour <majidvpchromium.org> Reviewed-by: Kevin Ellis <keverschromium.org> Cr-Commit-Position: refs/heads/master{#761974} -- wpt-commits: 46978682c5d3e2ff08468be05564307a9a75e06b wpt-pr: 23145 UltraBlame original commit: 8082006b84318e9f8564abb4265107ddf769774c

…ns to zero., a=testonly Automatic update from web-platform-tests Initialize start time of scroll animations to zero. Implemented web-animations-1 spec changes introduces in [1]. - Update play and pause procedures to initialize start time of scroll animations to zero. - Updated calculate play state procedure to return "running" state for animations that has start time resolved. - Added/modified tests reflecting spec changes. [1] w3c/csswg-drafts#4842 Bug: 1070637 Change-Id: Ic83995899b2f3f8d8f985f84b8a2b438bbad7c35 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2150687 Commit-Queue: Olga Gerchikov <gerchikomicrosoft.com> Reviewed-by: Majid Valipour <majidvpchromium.org> Reviewed-by: Kevin Ellis <keverschromium.org> Cr-Commit-Position: refs/heads/master{#761974} -- wpt-commits: 46978682c5d3e2ff08468be05564307a9a75e06b wpt-pr: 23145 UltraBlame original commit: e563517d3d930c71fde7bffe58aa64a68dce2d15

…ns to zero., a=testonly Automatic update from web-platform-tests Initialize start time of scroll animations to zero. Implemented web-animations-1 spec changes introduces in [1]. - Update play and pause procedures to initialize start time of scroll animations to zero. - Updated calculate play state procedure to return "running" state for animations that has start time resolved. - Added/modified tests reflecting spec changes. [1] w3c/csswg-drafts#4842 Bug: 1070637 Change-Id: Ic83995899b2f3f8d8f985f84b8a2b438bbad7c35 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2150687 Commit-Queue: Olga Gerchikov <gerchikomicrosoft.com> Reviewed-by: Majid Valipour <majidvpchromium.org> Reviewed-by: Kevin Ellis <keverschromium.org> Cr-Commit-Position: refs/heads/master{#761974} -- wpt-commits: 46978682c5d3e2ff08468be05564307a9a75e06b wpt-pr: 23145 UltraBlame original commit: 8082006b84318e9f8564abb4265107ddf769774c

…ns to zero., a=testonly Automatic update from web-platform-tests Initialize start time of scroll animations to zero. Implemented web-animations-1 spec changes introduces in [1]. - Update play and pause procedures to initialize start time of scroll animations to zero. - Updated calculate play state procedure to return "running" state for animations that has start time resolved. - Added/modified tests reflecting spec changes. [1] w3c/csswg-drafts#4842 Bug: 1070637 Change-Id: Ic83995899b2f3f8d8f985f84b8a2b438bbad7c35 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2150687 Commit-Queue: Olga Gerchikov <gerchikomicrosoft.com> Reviewed-by: Majid Valipour <majidvpchromium.org> Reviewed-by: Kevin Ellis <keverschromium.org> Cr-Commit-Position: refs/heads/master{#761974} -- wpt-commits: 46978682c5d3e2ff08468be05564307a9a75e06b wpt-pr: 23145 UltraBlame original commit: e563517d3d930c71fde7bffe58aa64a68dce2d15

…w3c#2075) (w3c#4842) Initialize start time of scroll animations to zero.

… and Pause(). r=hiro Based on w3c/csswg-drafts#4842, we define "has finite timeline", which is a timeline that's not monotonically increasing. We need this to update start time and hold time for scroll-timeline, so we play scroll-linked animations as we expected, e.g. GetLocalTime() returns the correct time value from GetCurrentTimeAsDuration(). Known issue: we still have bugs when setting "animation-play-state:paused". Will do that in Bug 1741255. Differential Revision: https://phabricator.services.mozilla.com/D131168

Implemented web-animations-1 spec changes introduces in [1]. - Update play and pause procedures to initialize start time of scroll animations to zero. - Updated calculate play state procedure to return "running" state for animations that has start time resolved. - Added/modified tests reflecting spec changes. [1] w3c/csswg-drafts#4842 Bug: 1070637 Change-Id: Ic83995899b2f3f8d8f985f84b8a2b438bbad7c35 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2150687 Commit-Queue: Olga Gerchikov <gerchiko@microsoft.com> Reviewed-by: Majid Valipour <majidvp@chromium.org> Reviewed-by: Kevin Ellis <kevers@chromium.org> Cr-Commit-Position: refs/heads/master@{#761974} Former-commit-id: 6829205c8c8643df09b0352b506ba290dbdd7565

Initial change.

1fd6277

dlibby- requested review from birtles, flackr and majido March 9, 2020 17:40

birtles requested changes Mar 12, 2020

View reviewed changes

Initialize start time of scroll animations to zero.

fc80ef2

ogerchikov changed the title ~~[web-animations-1] Updating animation play/pause procedure for inactive timelines (#2066)~~ [web-animations-1] Initialize start time of scroll animations to zero (#2066) Mar 27, 2020

ogerchikov changed the title ~~[web-animations-1] Initialize start time of scroll animations to zero (#2066)~~ [web-animations-1] Initialize start time of scroll animations to zero (#2075) Mar 27, 2020

ogerchikov commented Mar 27, 2020

View reviewed changes

web-animations-1/Overview.bs Outdated Show resolved Hide resolved

ogerchikov mentioned this pull request Apr 2, 2020

[web-animations-1] Make animations become idle when they have an inactive timeline #2066

Closed

ogerchikov added 2 commits April 2, 2020 14:36

Use monotonically increasing generalization of timelines instead of f…

3df0083

…inite.

Update play states.

441a771

birtles reviewed Apr 9, 2020

View reviewed changes

Addressed code review comments.

5d11616

birtles approved these changes Apr 10, 2020

View reviewed changes

Addressed code review comments.

4806500

ogerchikov merged commit 37f0682 into w3c:master Apr 13, 2020

chromium-wpt-export-bot mentioned this pull request Apr 21, 2020

Initialize start time of scroll animations to zero. web-platform-tests/wpt#23145

Merged

ogerchikov mentioned this pull request May 11, 2020

[web-animations-1] Fixed play animation procedure for scroll animations (#2075) #5059

Merged

birtles added the web-animations-1 label May 12, 2020

JTensai pushed a commit to JTensai/csswg-drafts that referenced this pull request May 13, 2020

[web-animations-1] Initialize start time of scroll animations to zero (…

5e10730

…w3c#2075) (w3c#4842) Initialize start time of scroll animations to zero.

ogerchikov mentioned this pull request Jun 2, 2020

[scroll-animations] Limit start time of progress-based finite timeline linked animations to 0 #4899

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[web-animations-1] Initialize start time of scroll animations to zero (#2075) #4842

[web-animations-1] Initialize start time of scroll animations to zero (#2075) #4842

ogerchikov commented Mar 6, 2020 •

edited

birtles Mar 12, 2020

ogerchikov Mar 12, 2020

ogerchikov Mar 12, 2020

birtles Mar 17, 2020

ogerchikov Mar 25, 2020

ogerchikov Mar 27, 2020

birtles Mar 12, 2020

ogerchikov Mar 27, 2020

birtles Apr 9, 2020

birtles Apr 9, 2020

ogerchikov Apr 9, 2020

birtles Mar 12, 2020

ogerchikov Mar 12, 2020

ogerchikov Mar 27, 2020

ogerchikov commented Mar 27, 2020

ogerchikov commented Mar 27, 2020

birtles left a comment

birtles Apr 9, 2020

birtles Apr 9, 2020

ogerchikov Apr 9, 2020

ogerchikov commented Apr 9, 2020

birtles left a comment

birtles Apr 10, 2020

ogerchikov Apr 10, 2020

birtles Apr 11, 2020

birtles Apr 10, 2020

ogerchikov Apr 10, 2020

birtles Apr 11, 2020

ogerchikov Apr 13, 2020

birtles Apr 14, 2020

ogerchikov commented Apr 10, 2020

birtles commented Apr 11, 2020


		* \|animation\|'s [=hold time=] is [=unresolved=], and
		* \|animation\|'s [=start time=] is [=unresolved=], and

[web-animations-1] Initialize start time of scroll animations to zero (#2075) #4842

[web-animations-1] Initialize start time of scroll animations to zero (#2075) #4842

Conversation

ogerchikov commented Mar 6, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ogerchikov commented Mar 27, 2020

ogerchikov commented Mar 27, 2020

birtles left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ogerchikov commented Apr 9, 2020

birtles left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ogerchikov commented Apr 10, 2020

birtles commented Apr 11, 2020

ogerchikov commented Mar 6, 2020 •

edited