Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 6 commits into from Apr 13, 2020
Merged

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

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1fd6277
Initial change.
ogerchikov Mar 6, 2020
fc80ef2
Initialize start time of scroll animations to zero.
ogerchikov Mar 27, 2020
3df0083
Use monotonically increasing generalization of timelines instead of f…
ogerchikov Apr 2, 2020
441a771
Update play states.
ogerchikov Apr 8, 2020
5d11616
Addressed code review comments.
ogerchikov Apr 9, 2020
4806500
Addressed code review comments.
ogerchikov Apr 10, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
55 changes: 24 additions & 31 deletions web-animations-1/Overview.bs
View file
Open in desktop
Expand Up @@ -828,31 +828,6 @@ follows:
<var>animation</var> with the <var>did seek</var> flag set to false, and
the <var>synchronously notify</var> flag set to false.

<h4 id="responding-to-a-newly-inactive-timeline">Responding to a newly inactive
timeline</h4>

Issue(2080): With the set of timelines defined in this level of this
specification, this situation is not expected to occur. As
a result, this section will likely be moved to a subsequent level
of this specification.

When the <a>timeline</a> associated with an <a>animation</a>,
<var>animation</var>, becomes newly <a lt="inactive timeline">inactive</a>,
if <var>animation</var>'s <a>previous current time</a> is <a
lt="unresolved">resolved</a>, the procedure to <a>silently set the current
time</a> of <var>animation</var> to <a>previous current time</a> is run.

<div class="note">

This step makes the behavior when an <a>animation</a>'s <a>timeline</a> becomes
<a lt="inactive timeline">inactive</a> consistent with when it is
disassociated with a <a>timeline</a>.
Furthermore, it ensures that the only occasion on which an <a>animation</a>
becomes <a lt="idle play state">idle</a>, is when the procedure to
<a>cancel an animation</a> is performed.

</div>

<h4 id="setting-the-associated-effect" oldids="setting-the-target-effect">Setting the associated effect of an animation</h4>

The procedure to <dfn>set the associated effect of an animation</dfn>,
Expand Down Expand Up @@ -1150,7 +1125,14 @@ as CSS Animations [[CSS-ANIMATIONS-1]].
condition from the following, if any:

<div class="switch">

: 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>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


:: Set <var>animation</var>'s <a>start time</a> to zero.
: If |animation|'s [=effective playback rate=] &gt; 0,
the <var>auto-rewind</var> flag is true and <em>either</em>
<var>animation</var>'s:
Expand Down Expand Up @@ -1185,9 +1167,10 @@ as CSS Animations [[CSS-ANIMATIONS-1]].
1. Cancel that task.
1. Set <var>has pending ready promise</var> to true.

1. If the following three conditions are <em>all</em> satisfied:
1. If the following four conditions are <em>all</em> satisfied:

* |animation|'s [=hold time=] is [=unresolved=], and
* |animation|'s [=start time=] is [=unresolved=], and
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the flag.

* |aborted pause| is false, and
* |animation| does <em>not</em> have a [=pending playback rate=],

Expand Down Expand Up @@ -1324,6 +1307,12 @@ 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
<em>either</em>:
* the |animation| has no associated <a>timeline</a>, or
* the associated <a>timeline</a> is
<a lt="inactive timeline">inactive</a>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The explanation is the same as in the first comment.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see the first comment.

:: Let <var>animation</var>'s <a>start time</a> be zero.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed similarly to Play.


: If <var>animation</var>'s [=playback rate=] is &ge; 0,
:: Let <var>animation</var>'s <a>hold time</a> be zero.
Expand All @@ -1345,10 +1334,14 @@ follows:
1. If <var>has pending ready promise</var> is false,
set <var>animation</var>'s <a>current ready promise</a> to
<a>a new promise</a> in the <a>relevant Realm</a> of <var>animation</var>.
1. Schedule a task to be executed at the first possible moment after
the user agent has performed any processing necessary to suspend
the playback of
<var>animation</var>'s <a>associated effect</a>, if any.
1. Schedule a task to be executed at the first possible moment where
<em>both</em> of the following conditions are true:
* the user agent has performed any processing necessary to suspend
the playback of <var>animation</var>'s <a>associated effect</a>, if any.

* the animation is associated with a <a>timeline</a> that is not
<a lt="inactive timeline">inactive</a>.

The task shall perform the following steps:

1. Let <var>ready time</var> be the time value of the timeline associated
Expand Down