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

[ogerchikov]

#4842 is missing updates required to handle reverse() API and re-playing finished animations:

• If scroll animation is reversed and start time is set as part of play animation procedure, pending playback rate needs to be applied to ensure correct current time calculation while the animation is in pending state.
• When re-playing finished scroll animation hold_time needs to be set to unresolved to ensure start_time stays resolved.

[birtles]

• If scroll animation is reversed and start time is set as part of play animation procedure, pending playback rate needs to be applied to ensure correct current time calculation while the animation is in pending state.

Do you have an example (or test?) for this? It seems odd that we sometimes apply the pending playback rate immediately and sometimes apply it in the ready task.

It would be nice to see an example/test for the second point too so I can understand it better.

[ogerchikov]

Here are the failing tests you requested.

• reverse() example:

promise_test(async t => {
  const animation = createScrollLinkedAnimation(t); // duration: 1000
  // Wait for new animation frame which allows the timeline to compute new
  // current time.
  await waitForNextFrame();
  animation.play();
  animation.currentTime = 2000;
  // play() invoked by reverse() sets start time to 1000 based on the
  // effective playback rate, which is -1.
  animation.reverse(); 
  // This fails. Current time is calculated based on playback rate, which is 1.
  assert_equals(animation.currentTime, 1000, 
    'reverse() should start playing from the animation effect end ' +
    'if the playbackRate > 0 and the currentTime > effect end');
}, 'Reversing an animation when playbackRate > 0 and currentTime > ' +
   'effect end should make it play from the end');

• Re-playing finished animation

promise_test(async t => {
  const animation = createScrollLinkedAnimation(t); // duration: 1000
  // Wait for new animation frame  which allows the timeline to compute new
  // current time.
  await waitForNextFrame();
  animation.play();
  animation.finish();
  assert_time_equals_literal(animation.currentTime, 1000);
  animation.play();
  // This fails. Finished animation has both start and hold time set. 
  // Start time is initialized to zero, but later it's cleared since the hold time is set.
  assert_time_equals_literal(animation.currentTime, 0); 
}, 'Playing a finished animation seeks back to the start');

[birtles]

* reverse() example:

promise_test(async t => {
  const animation = createScrollLinkedAnimation(t); // duration: 1000
  // Wait for new animation frame which allows the timeline to compute new
  // current time.
  await waitForNextFrame();
  animation.play();
  animation.currentTime = 2000;
  // play() invoked by reverse() sets start time to 1000 based on the
  // effective playback rate, which is -1.
  animation.reverse(); 
  // This fails. Current time is calculated based on playback rate, which is 1.
  assert_equals(animation.currentTime, 1000, 
    'reverse() should start playing from the animation effect end ' +
    'if the playbackRate > 0 and the currentTime > effect end');
}, 'Reversing an animation when playbackRate > 0 and currentTime > ' +
   'effect end should make it play from the end');

Thank you!

Thinking about this, I think this is just an artifact of the fact that we are seeking using the start time rather than the hold time -- hence the effect doesn't take place until the animation is ready.

But thinking about this further, currently the whole point of the pending task is typically to resolve a suitable start time. That's why setting the start time cancels any pending tasks.

Do we now want animations to remain pending if they don't have an active timeline? If so, we need to update the procedure to set the start time (and possibly other places). If not, we could possibly just call that procedure here instead.

[ogerchikov]

But thinking about this further, currently the whole point of the pending task is typically to resolve a suitable start time. That's why setting the start time cancels any pending tasks.

I was also thinking that perhaps initializing start time in play() should work the same as setting the start time regardless of timeline activeness. However I convinced myself otherwise to keep scroll and time linked animations behave similarly in terms of timing of resolving Ready promise and the value of pending flag.

In addition, calling play() on pause pending animation has the same effect - hold time unresolved, start time is resolved and Ready promise is resolved asynchronously.

Do we now want animations to remain pending if they don't have an active timeline? If so, we need to update the procedure to set the start time (and possibly other places). If not, we could possibly just call that procedure here instead.

Are you referring to finite inactive timelines or in general?
If just finite - there is no need to keep the animation pending as calculated current time is unresolved and the animation doesn't have an effect.

[birtles]

But thinking about this further, currently the whole point of the pending task is typically to resolve a suitable start time. That's why setting the start time cancels any pending tasks.

I was also thinking that perhaps initializing start time in play() should work the same as setting the start time regardless of timeline activeness. However I convinced myself otherwise to keep scroll and time linked animations behave similarly in terms of timing of resolving Ready promise and the value of pending flag.

In addition, calling play() on pause pending animation has the same effect - hold time unresolved, start time is resolved and Ready promise is resolved asynchronously.

There's a parallel though because both play() and pause() update the start time after the animation is ready (although in the latter case, it is updated to an unresolved time). With finite timelines, however, we're updating it synchronously.

My concern is that it feels odd that we are sometimes running the procedure to apply the pending playback rate synchronously and sometimes asynchronously. Don't you think that feels a bit odd? A bit unpredictable?

If we could do the whole thing synchronously by invoking the set the start time procedure, that would at least be more consistent.

But I'm actually starting to wonder if the issue needs to be fixed in the first place. Could we just say that the current time is out of date until the animation is ready? (As opposed to the start time being out of date.)

For the second test, clearing the hold time probably makes sense (I need to double-check). Sorry, it takes me a long time each time to try and remember how this all fits together.

[ogerchikov]

My concern is that it feels odd that we are sometimes running the procedure to apply the pending playback rate synchronously and sometimes asynchronously. Don't you think that feels a bit odd? A bit unpredictable?

Yes, I agree that it's inconsistent and odd. But perhaps it can be explained that while scroll animation is pending, calculating current time requires up to date playback_rate.

If we could do the whole thing synchronously by invoking the set the start time procedure, that would at least be more consistent.

I prototyped this in Chromium and ran all the timing tests. I didn't find any failures with the exception of pending flag being false after play() and timing of ready promise. Note that update an animations finished state invoked from setting the start time (see the last bullet) should be called with did seek flag set to false. Alternatively play task can be called synchronously from play().

This is probably the right way to go since there is no really pending actions that can not be accomplished synchronously. I am just worried that it can complicate user scripts when both scroll and time linked animations are involved.

But I'm actually starting to wonder if the issue needs to be fixed in the first place. Could we just say that the current time is out of date until the animation is ready? (As opposed to the start time being out of date.)

What does it mean the current time is out of date? Unresolved and no effect? If so it's inconsistent with time linked animations in pending state. Perhaps I am missing something?

[birtles]

If we could do the whole thing synchronously by invoking the set the start time procedure, that would at least be more consistent.

I prototyped this in Chromium and ran all the timing tests. I didn't find any failures with the exception of pending flag being false after play() and timing of ready promise. Note that update an animations finished state invoked from setting the start time (see the last bullet) should be called with did seek flag set to false. Alternatively play task can be called synchronously from play().

This is probably the right way to go since there is no really pending actions that can not be accomplished synchronously. I am just worried that it can complicate user scripts when both scroll and time linked animations are involved.

Yes, that's a very valid concern.

However, I think by calling the procedure to set the start time the Promise resolution procedure will mean those callbacks get called at the next microtask checkpoint so those code will still act more-or-less the same in both cases.

The only difference is that the pending state is false immediately after calling play() for the scroll-timeline case. However, that is already true if, for example, you call play() on an already running animation attached to a document timeline--so in that sense we're being consistent with the idea that we only go pending if there is pending work to do.

(And the more I think about it, the more I wonder if the pending playback rate actually needs to be applied asychronously for play() anyway? I'm having trouble thinking of a scenario where it would make a difference?)

While we're touching this procedure, I wonder if it would be more neat to drop the performed seek flag and set a local seek time variable. Then, if that value was set, before step 6 we'd either update the hold time or call the set the start time procedure? (And possibly clear the hold time value there too as per this PR?)

Thanks for all your work on this. Unfortunately I think at this time it's only you and I who are really familiar with these procedures and they're very complicated. I wish we could make them all simpler somehow!

[majido]

I am not the right reviewer here. Perhaps @kevers-google has some opinion on the matter. He implemented most of the pending play/pause implementation in Chromium and knows the procedures in much more depth.

[kevers-google]

Looking at the first failure (will address second in followup post):

// Schedule animation to start playing.
animation.play();

// Update the hold time since we don’t yet have a start time. Pending pauses are
// synchronously resolved, perhaps pending plays should be as well.
animation.currentTime = 2000;

// Change pending playback rate to -1 and trigger the steps to play an animation
// with auto-rewind set to true. Here we have a difference between finite
// and monotonic timelines. Finite sets the start time, whereas monotonic sets the
// hold time. Setting either hold or start time does not unresolve its counterpart.
// It appears we can get into a state with both hold time and start time resolved,
// which is normally only possible when in the finished state. If hold time = 2000
// but start time = 1000, then current time = 2000.
animation.reverse();

Ensuring that hold time is cleared whenever start time is set should resolve this problem. Since we update the finished state as the last step in the process, we don't need to worry about clearing the hold time if the animation is finished (it'll simply be re-resolved).

[kevers-google]

Problem 2 looks like it has a common route cause as setting the start time, but not clearing the hold time leaves the animation in a weird state. Looks like tweaking step 5 in:

https://drafts.csswg.org/web-animations-1/#playing-an-animation-section

to reset the hold time when setting the start time could resolve both failures.

[ogerchikov]

(And the more I think about it, the more I wonder if the pending playback rate actually needs to be applied asychronously for play() anyway? I'm having trouble thinking of a scenario where it would make a difference?)

Applying pending playback rate asynchronously makes a difference when reversing or calling updatePlaybackRate() on running animation. In these cases play pending task needs both the current and pending playback rates to recalculate start time to match the current time.

anim.play();
...
anim.reverse(); // or anim.updatePlaybackRate(-1)

I was thinking more about diverging play procedure for scroll animations to call set start time. I feel like this inconsistency is "more inconsistent" than applying pending playback rate.

How about we generalize updating playback rate asynchronously only when it's needed? In this case the condition will be:

• If performed seek is true or hold time is resolved,
  • Apply any pending playback rate on animation.

[birtles]

Ok, please help me try to understand. I believe Kevin is suggesting that simply resetting the hold time when setting the start time is enough to fix both problems? Is that correct? Olga, does that work?

(And the more I think about it, the more I wonder if the pending playback rate actually needs to be applied asychronously for play() anyway? I'm having trouble thinking of a scenario where it would make a difference?)

Applying pending playback rate asynchronously makes a difference when reversing or calling updatePlaybackRate() on running animation. In these cases play pending task needs both the current and pending playback rates to recalculate start time to match the current time.

Oh, good point.

I was thinking more about diverging play procedure for scroll animations to call set start time. I feel like this inconsistency is "more inconsistent" than applying pending playback rate.

I'm not quite sure I follow. Does this mean that you prefer we don't call the "set start time" procedure but just directly apply the pending playback rate? Is there anything in particular that you feel is better about that?

(I'm wondering if calling the "set start time" procedure is better since we skip the pending state, and that's better since the UA doesn't actually have any time values to resolve asynchronously?)

How about we generalize updating playback rate asynchronously only when it's needed? In this case the condition will be:

• If performed seek is true or hold time is resolved,
  • Apply any pending playback rate on animation.

That's interesting. What would that help with?

In any case, I still want to refactor this to make step 5 simply set a local seek time variable, and then, as a separate step, update either the hold time or start time based on the value of finite timeline. Once we do that, it would be easier to clear the pending playback rate there too if we decide to do that.

[ogerchikov]

In any case, I still want to refactor this to make step 5 simply set a local seek time variable, and then, as a separate step, update either the hold time or start time based on the value of finite timeline. Once we do that, it would be easier to clear the pending playback rate there too if we decide to do that.

Second commit has the refactoring per your suggestion, both play and pause procedures. It's much cleaner now.

Ok, please help me try to understand. I believe Kevin is suggesting that simply resetting the hold time when setting the start time is enough to fix both problems? Is that correct? Olga, does that work?

I spoke to Kevin offline and his point was that clearing hold time is required but not sufficient for the reverse() case.

I was thinking more about diverging play procedure for scroll animations to call set start time. I feel like this inconsistency is "more inconsistent" than applying pending playback rate.

I'm not quite sure I follow. Does this mean that you prefer we don't call the "set start time" procedure but just directly apply the pending playback rate? Is there anything in particular that you feel is better about that?

Yes, I prefer we don't call "set start time" from play. I also spoke to Kevin about it and his preference is alike. He is concerned that differences in timing of ready promise resolution might complicate user scrips. In the example below, pause() resolves new promise for scroll animations, versus the one from play() procedure for the time based animations (see Pausing an animation step 7).

anim.play();
anim.pause();

How about we generalize updating playback rate asynchronously only when it's needed? In this case the condition will be:

• If performed seek is true or hold time is resolved,
  • Apply any pending playback rate on animation.

That's interesting. What would that help with?

This is my attempt to generalize when playback rate doesn't need to be updated asynchronously so animation type is not a factor in the logic.

In general, it feels like differences in timing of updating playback rate is a "lesser evil" than having pretty much different play procedure for scroll animations. However I'll trust your judgement since you are the expert and my opinion is more based on intuition rather than real data.

[birtles]

That latest patch looks really really good.

I'd still be curious to hear if you have answers to the questions from my previous comment. It would be good to get @kevers-google's review too.

[ogerchikov]

Thanks @birtles for reviewing. I thought I answered all the questions. Did I miss anything?

[birtles]

Sorry, this is really really odd, but I promise I looked for your answer before posting my question and it wasn't there (maybe I had a cached version of this page?). I see it now. Thank you!

[kevers-google]

Very nice. Much easier to follow the flow.