1074630 - Implement player finishing behavior

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Brian Birtles (:birtles)]

This includes:
* Making sure AnimationPlayer.currentTime stops when it gets to the end of the source content (or 0 in reverse)
* Responding to changes to length of the source content
* Returning the right value from AnimationPlayer.playState
* Adding the finished promise and resolving it at the appropriate moments

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 2

[Jonathan Watt [:jwatt]]

Attachment: tests

Comment 3

[Jonathan Watt [:jwatt]]

Attachment: patch

Address IRC comments.

Comment 4

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 5

[Brian Birtles (:birtles)]

Comment on attachment 8580368 [details] [diff] [review]
patch

Review of attachment 8580368 [details] [diff] [review]:
-----------------------------------------------------------------

r=birtles with comments addressed and assuming this actually passes the tests.

I'm particularly concerned about the change to AnimationPlayer::ComposeStyle breaking something. The code for updating animation style is still very brittle so it seems like whenever we touch it something breaks. Our test suite is gradually getting better though so if everything passes I guess we're ok.

::: dom/animation/AnimationPlayer.cpp
@@ +68,5 @@
>      mReady->MaybeResolve(this);
>    }
>  
> +  // Call UpdateFinishedState before UpdateSourceContent because the former
> +  // can change the current time, which is used by the latter.

I don't know if we need to repeat this every time?

Better still, make an UpdateTiming function that calls both UpdateFinishedState and UpdateSourceContent and put the comment there?

@@ +215,5 @@
> +  } else {
> +    // Unfortunately there's some weirdness in the spec at the moment where if
> +    // you're finished and paused, the playState is paused. This prevents us
> +    // from just checking |PlayState() == AnimationPlayState::Finished| here,
> +    // and we need this much more messy check to see if we're finished.

See my comment on UpdateFinishedState. I think we should temporarily factor out a utility method for checking if we're *actually* finished and call that here and there (and perhaps in PlayState() too?).

@@ +224,5 @@
> +         currentTime.Value().ToMilliseconds() >= SourceContentEnd()) ||
> +        (mPlaybackRate < 0.0 &&
> +         currentTime.Value().ToMilliseconds() <= 0.0);
> +      if (finished) {
> +        mFinished->MaybeResolve(this);

If determining if we're finished or not is expensive then we should probably only do this step when mFinished is initially null. Currently it seems like it's probably ok (although it does require calculating all the timing of the source content).

@@ +459,4 @@
>  }
>  
>  void
> +AnimationPlayer::DoPlay(bool aForPlayStateChange)

What do you think about using an two-valued enum here? LimitBehavior::AutoRewind / LimitBehavior::Continue?
I think that would be easier to understand.

@@ +632,5 @@
> +      mHoldTime.SetNull();
> +    }
> +  }
> +
> +  bool currentFinishedState = PlayState() == AnimationPlayState::Finished;

I think the spec is probably wrong here. I think we should be comparing whether we are *actually* finished, as opposed to what the playState says.

We should probably factor out a method AreWeActuallyFinished as a temporary step and use that.

::: dom/animation/AnimationPlayer.h
@@ +221,5 @@
>  
>  protected:
> +  // Flag to pass to DoPlay to indicate that it should not carry out finishing
> +  // behavior (reset the current time to the beginning of the active duration).
> +  static const bool ForPlayStateChange = true;

As suggested elsewhere, I think we should use an enum class here with values that are less to do with the context in which we're called and more to do with the desired behaviour (i.e. auto-rewind or not when we've reached the end)

@@ +235,5 @@
>    // as necessary. The caller is responsible for resolving or aborting the
>    // mReady promise as necessary.
>    void CancelPendingPlay();
>  
> +  void UpdateFinishedState();

Stick this above UpdateSourceContent()?

@@ +259,3 @@
>    // A Promise that is replaced on each call to Play() (and in future Pause())
>    // and fulfilled when Play() is successfully completed.
>    // This object is lazily created by GetReady.

Put the link at the end of the comment?

@@ +260,5 @@
>    // and fulfilled when Play() is successfully completed.
>    // This object is lazily created by GetReady.
>    nsRefPtr<Promise> mReady;
>  
> +  // See http://w3c.github.io/web-animations/#current-finished-promise

Likewise here.

@@ +262,5 @@
>    nsRefPtr<Promise> mReady;
>  
> +  // See http://w3c.github.io/web-animations/#current-finished-promise
> +  // A Promise that is replaced with a new (pending) Promise object whenever we
> +  // leave the active duration.

This is not right. It's resolved when we reach the end of the source content or 0 when playing backwards. And it's not replaced until we leave that state.

@@ +276,5 @@
>    bool mIsRunningOnCompositor;
>    // Indicates whether we were in the finished state during our
>    // most recent unthrottled sample (our last ComposeStyle call).
> +  // If Promise::IsPending() or Promise::mState was public we could check our
> +  // finished Promise's state instead of having this member.

We probably couldn't though since we only lazily create the mFinished promise so we couldn't actually use it for tracking state.

Comment 6

[Brian Birtles (:birtles)]

Comment on attachment 8580092 [details] [diff] [review]
tests

Review of attachment 8580092 [details] [diff] [review]:
-----------------------------------------------------------------

We need the following tests:

* Anim duration is 200s
  Seek anim to 100s
  Change anim duration to 50s
  Anim remains at 100s and is finished (promise resolves etc.)
  Change anim duration to 200s
  Anim resumes from 100s with new unresolved finished promise

  It would also be worth doing a further variation on this where after seeking to 100s we also wait a frame before updating the anim duration. The reason is that we might be calling UpdateFinishedState() from SetCurrentTime() and storing the previous current time there. However, we should *also* call UpdateFinishedState from Tick() and if we fail to do that the current time will stay at 100s (not, say, 100.1s) so we'll be able to detect that and fail here.

* Tests for playbackRate == 0
  e.g. set playbackRate = 0 then seek past the end of the source content
  This should not lead to resolving the finished promise

* Tests that setting animation-play-state: running on a CSS Animations that has finished does *not* trigger the auto-rewind behaviour

* Tests for currentTime
  e.g. seek to just before the end of the animation, wait a frame or two (until the real world time puts us *past* the end of the animation) and then check that the currentTime doesn't advance past the end

* Tests for playbackRate < 0
  e.g. let an animation play backwards and check that the currentTime sticks to 0.
  e.g. (2) something similar to the first test described above but in the reverse direction

That's all that comes to mind now but there are probably lots of other cases to check. Would you mind doing a quick skim of the code changes and check that we have a test covering each of the them?

::: dom/animation/test/css-animations/test_animation-player-finished.html
@@ +12,5 @@
> +<script>
> +'use strict';
> +
> +const ANIM_PROP_VAL = 'abc 100s';
> +const ANIM_DURATION = 100000; // ms

I find these constants at the top of the file make reading the test a little harder. What do you think about dropping them and putting the animation inline?
e.g.

  var div = addDiv(t, { style: 'animation abc 100s' });

@@ +21,5 @@
> +  var player = div.getAnimationPlayers()[0];
> +
> +  var previousFinishedPromise = player.finished;
> +  
> +  player.ready.then(function() {

I thought we needed to do:

  player.ready.then(t.step_func(function() {

Or have I been misunderstanding testharness.js all along?

@@ +32,5 @@
> +                  'Finished promise does not change when pausing (for now)');
> +    player.play();
> +    assert_not_equals(player.finished, previousFinishedPromise,
> +                      'Finished promise object identity differs after calling'
> +                      + ' play()');

This doesn't seem right. Maybe I'm forgetting something or maybe it's a spec bug but the only time I'd expect play() to trigger a new finished promise is when we have already finished and play() triggers auto-rewind behaviour.

@@ +38,5 @@
> +    previousFinishedPromise = player.finished;
> +    player.currentTime = ANIM_DURATION;
> +  });
> +
> +  player.finished.then(function() {

Why not just make the first block return player.finished and chain this step as in:

  player.currentTime = ANIM_DURATION;
  return player.finished;
})).then(t.step_func(function() {
  assert_equals...

(The extra brace is based on my comment above.)

@@ +44,5 @@
> +                  'Finished promise is the same object when playing completes');
> +    t.done();
> +  });
> +}, 'A new finished promise is created each time play() is called'
> +   + ' the animation property');

This sentence seems to be missing a word or two.

@@ +46,5 @@
> +  });
> +}, 'A new finished promise is created each time play() is called'
> +   + ' the animation property');
> +
> +/* XXXjwatt - is this desired behavior, since we create a new *ready* promise

No, this is not the desired behavior. We can create several new ready promises while keeping the same finished promise (e.g. if we pause and resume several times).

@@ +73,5 @@
> +                  'Finished promise has same object identity after redundant call'
> +                  + ' to play()');
> +    t.done();
> +  });
> +}, 'Redundant calls to play() do not generate new finished promise objects');

I'm not sure if this is completely needed but doesn't hurt. However, the case where we finish something twice is probably needed. For example, seek to the end of the animation (finished promise resolves etc.), then seek past the end of the animation and check we still have the same finished promise.

Also interesting would be if we seek to the end of the animation, then set playbackRate to -1 and seek to 0. I guess that should create a new finished promise according to the spec.

@@ +87,5 @@
> +                  'Object identity of player passed to Promise callback'
> +                  + ' matches the player object owning the Promise');
> +    t.done();
> +  });
> +}, 'The finished promise is fulfilled with its AnimationPlayer');

This is great. I like these really focussed tests.

@@ +95,5 @@
> +
> +  // Set up pending animation
> +  div.style.animation = ANIM_PROP_VAL;
> +  var player = div.getAnimationPlayers()[0];
> +  assert_equals(player.playState, 'pending', 'Player is initially pending');

I don't think we need to test that here.

@@ +106,5 @@
> +  })).catch(t.step_func(function(err) {
> +    assert_equals(err.name, 'AbortError',
> +                  'finished promise is rejected with AbortError');
> +    assert_equals(player.playState, 'idle',
> +                  'Player is idle after animation was cancelled');

It's probably worth checking the identity of the finished promise has changed here too?

@@ +123,5 @@
> +  var div = addDiv(t);
> +
> +  // As before, but this time instead of removing all animations, simply update
> +  // the list of animations. At least for Firefox, updating is a different
> +  // code path.

Is it still a different code path if the new animation-name doesn't match a keyframes rule? (In this case I don't think 'def' matches anything.)

::: dom/animation/test/css-transitions/test_animation-player-finished.html
@@ +8,5 @@
> +'use strict';
> +
> +async_test(function(t) {
> +  var div = addDiv(t);
> +  div.style.transform = 'translate(0px)';

Combine this into one line?

  var div = addDiv(t, { style: 'transform: translate(0px)' })

@@ +28,5 @@
> +                  'Finished promise does not change when pausing (for now)');
> +    player.play();
> +    assert_not_equals(player.finished, originalFinishedPromise,
> +                      'Finished promise object identity differs after calling'
> +                      + ' play()');

As with the animations test, I don't think we should get a new Promise here.

@@ +32,5 @@
> +                      + ' play()');
> +    t.done();
> +  }));
> +}, 'A new finished promise is created each time play() is called'
> +   + ' the animation property');

Again, grammatically something is off here.

@@ +45,5 @@
> +  div.style.transform = 'translate(10px)';
> +  window.getComputedStyle(div).transform;
> +
> +  var player = div.getAnimationPlayers()[0];
> +  assert_equals(player.playState, 'pending', 'Player is initially pending');

As before, I don't think we need to test this here.

@@ +77,5 @@
> +  div.style.marginLeft = '100px';
> +  window.getComputedStyle(div).marginLeft;
> +
> +  var player = div.getAnimationPlayers()[0];
> +  assert_equals(player.playState, 'pending', 'Player is initially pending');

Ditto.

Comment 7

[Jonathan Watt [:jwatt]]

Comment on attachment 8580368 [details] [diff] [review]
patch

For the webidl change.

Comment 8

[Brian Birtles (:birtles)]

Attachment: part 2 - Remove some unneeded startTime tests

I'm not sure if you'll like this one. Tell me what you think

Comment 9

[Brian Birtles (:birtles)]

Attachment: part 2 - Remove some unneeded startTime tests

Now that we have separate tests for checking the initial state of startTime we
can remove these checks from tests for setting the startTime.

Also, while we're at it, we needn't check the playState and animationPlayState
since these should be covered by other tests.

Comment 10

[Brian Birtles (:birtles)]

Comment on attachment 8580540 [details] [diff] [review]
part 2 - Remove some unneeded startTime tests

Oops, wrong bug

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8580368 [details] [diff] [review]
patch

(I assume some patch will rename AnimationPlayer to Animation)

Comment 12

[Jonathan Watt [:jwatt]]

(In reply to Brian Birtles (:birtles) from comment #6)
> We need the following tests:
> 
> * Anim duration is 200s
>   Seek anim to 100s
>   Change anim duration to 50s
>   Anim remains at 100s and is finished (promise resolves etc.)
>   Change anim duration to 200s
>   Anim resumes from 100s with new unresolved finished promise
> 
>   It would also be worth doing a further variation on this where after
> seeking to 100s we also wait a frame before updating the anim duration. The
> reason is that we might be calling UpdateFinishedState() from
> SetCurrentTime() and storing the previous current time there. However, we
> should *also* call UpdateFinishedState from Tick() and if we fail to do that
> the current time will stay at 100s (not, say, 100.1s) so we'll be able to
> detect that and fail here.

It seems this latter variation would essentially just be checking that currentTime advances as we get ticks after setting currentTime to a given value. The finishing tests don't seem to be the appropriate place to test that, since I don't see what that has to do with finishing.

> * Tests for currentTime
>   e.g. seek to just before the end of the animation, wait a frame or two
> (until the real world time puts us *past* the end of the animation) and then
> check that the currentTime doesn't advance past the end

I don't think this belongs in the finishing tests. The currentTime tests should do this.

> * Tests for playbackRate < 0
>   e.g. let an animation play backwards and check that the currentTime sticks
> to 0.
>   e.g. (2) something similar to the first test described above but in the
> reverse direction

I'll add this to the playbackRate bug's tests.

> > +const ANIM_PROP_VAL = 'abc 100s';
> > +const ANIM_DURATION = 100000; // ms
> 
> I find these constants at the top of the file make reading the test a little
> harder. What do you think about dropping them and putting the animation
> inline?

I'd rather keep them because I find it makes it easier in one regard. The advantage is when looking at the code and seeing that you're setting say, the current time to the duration, you know that it's being set to the end of the active interval. Readers don't need to look up to see what the duration or delay was set to, or figure out what the numbers mean in relation to those values. It also makes the tests less error prone, and easier to change all the subtests all at one go if we decide to change the values (say at some point we decide the duration is not long enough).

> I thought we needed to do:
> 
>   player.ready.then(t.step_func(function() {
> 
> Or have I been misunderstanding testharness.js all along?

You're right. We should be doing that to get error tidier messages. (Seems that the existing ready promise tests are doing this wrong too.)

Comment 13

[Jonathan Watt [:jwatt]]

Attachment: tests

Comment 14

[Jonathan Watt [:jwatt]]

Attachment: CSS animations tests

Comment 15

[Brian Birtles (:birtles)]

(In reply to Jonathan Watt [:jwatt] from comment #12)
> (In reply to Brian Birtles (:birtles) from comment #6)
> > We need the following tests:
> > 
> > * Anim duration is 200s
> >   Seek anim to 100s
> >   Change anim duration to 50s
> >   Anim remains at 100s and is finished (promise resolves etc.)
> >   Change anim duration to 200s
> >   Anim resumes from 100s with new unresolved finished promise
> > 
> >   It would also be worth doing a further variation on this where after
> > seeking to 100s we also wait a frame before updating the anim duration. The
> > reason is that we might be calling UpdateFinishedState() from
> > SetCurrentTime() and storing the previous current time there. However, we
> > should *also* call UpdateFinishedState from Tick() and if we fail to do that
> > the current time will stay at 100s (not, say, 100.1s) so we'll be able to
> > detect that and fail here.
> 
> It seems this latter variation would essentially just be checking that
> currentTime advances as we get ticks after setting currentTime to a given
> value. The finishing tests don't seem to be the appropriate place to test
> that, since I don't see what that has to do with finishing.

We need to test that Tick() calls UpdateFinishedState somehow, but I don't mind how we do that or which test file it goes in.

> > * Tests for currentTime
> >   e.g. seek to just before the end of the animation, wait a frame or two
> > (until the real world time puts us *past* the end of the animation) and then
> > check that the currentTime doesn't advance past the end
> 
> I don't think this belongs in the finishing tests. The currentTime tests
> should do this.

Yes, it can go in the currentTime tests but it's still part of finishing behaviour. One way or another it needs to get tested.

> > * Tests for playbackRate < 0
> >   e.g. let an animation play backwards and check that the currentTime sticks
> > to 0.
> >   e.g. (2) something similar to the first test described above but in the
> > reverse direction
> 
> I'll add this to the playbackRate bug's tests.

Great, thanks!

> > I thought we needed to do:
> > 
> >   player.ready.then(t.step_func(function() {
> > 
> > Or have I been misunderstanding testharness.js all along?
> 
> You're right. We should be doing that to get error tidier messages. (Seems
> that the existing ready promise tests are doing this wrong too.)

Yes, I noticed that myself just yesterday.

Comment 16

[Brian Birtles (:birtles)]

Comment on attachment 8585871 [details] [diff] [review]
CSS animations tests

>+@keyframes abc {
>+  to { transform: translate(10px) }
>+}
>+@keyframes def {}

Nit: We should probably rename this to 'anim' and 'empty' or something a little more descriptive. But this a problem with the existing tests (where we sometimes use 'anim' sometimes use 'abc') so we can fix it later and do them all at once.

>+async_test(function(t) {
>+  var div = addDiv(t);
>+  div.style.animation = ANIM_PROP_VAL;
>+  var player = div.getAnimationPlayers()[0];
>+
>+  var previousFinishedPromise = player.finished;
>+
>+  player.ready.then(t.step_func(function() {
>+    assert_equals(player.finished, previousFinishedPromise,
>+                  'Finished promise is the same object when playing starts');
>+    player.pause();
>+    assert_equals(player.finished, previousFinishedPromise,
>+                  'Finished promise does not change when pausing');
>+    player.play();
>+    assert_equals(player.finished, previousFinishedPromise,
>+                  'Finished promise does not change when play() unpauses');
>+
>+    player.currentTime = ANIM_DURATION;
>+  }));
>+
>+  player.finished.then(t.step_func(function() {
>+    assert_equals(player.finished, previousFinishedPromise,
>+                  'Finished promise is the same object when playing completes');
>+    t.done();
>+  }));
>+}, 'Test pausing then playing does not change the finished promise');

This is fine, but I think it's more idiomatic to chain the two calls together so that the middle part becomes:

    ...
    player.currentTime = ANIM_DURATION;
    return player.finished;
  })).then(t.step_func(function() {
    ...

What do you think? Is there any reason to prefer the current arrangement?

>+async_test(function(t) {
>+  var div = addDiv(t);
>+  div.style.animation = ANIM_PROP_VAL;
>+  var player = div.getAnimationPlayers()[0];
>+
>+  var previousFinishedPromise = player.finished;
>+
>+  player.currentTime = ANIM_DURATION;
>+
>+  player.finished.then(t.step_func(function() {
>+    player.currentTime = ANIM_DURATION + 1000;
>+    assert_equals(player.finished, previousFinishedPromise,
>+                  'Finished promise is unchanged jumping passed end of ' +

Super nit: 'past end' ;)

>+async_test(function(t) {
>+  var div = addDiv(t);
>+  div.style.animation = ANIM_PROP_VAL;
>+  var player = div.getAnimationPlayers()[0];
>+
>+  const HALF_DUR = ANIM_DURATION / 2;
>+  const QUARTER_DUR = ANIM_DURATION / 4;
>+
>+  player.currentTime = HALF_DUR;
>+  div.style.animationDuration = QUARTER_DUR + 'ms'; // finished

'finished' is a bit cryptic. Perhaps we can move this comment to the following line and make it "Player should now be finished"?

>+
>+  player.finished.then(t.step_func(function() {
>+    assert_equals(player.currentTime, ANIM_DURATION / 2,
>+                  'currentTime should be unchanged when duration shortened');

Why don't we compare to HALF_DUR here?

Also, I'm not sure how to test this, but I *think* what we want to test is that actually setting the duration causes us to resolve the finished promise right? It's possible we might fail to do that, but that Tick() calls UpdateFinishedState() and so we resolve it then and this test passes anyway? Does that sound right?

If so, I wonder if it would be valid to flush the style change, schedule a rAF callback and ensure the promise callback was resolved before the rAF callback ran? As I understand it, resolved promises cause microtasks to be scheduled which should be executed before revisiting the event loop and therefore before the next frame. Does that seem right to you?

>+async_test(function(t) {
>+  var div = addDiv(t);
>+  div.style.animation = ANIM_PROP_VAL;
>+  var player = div.getAnimationPlayers()[0];
>+
>+  player.ready.then(function() {
>+    player.playbackRate = 0;
>+    player.currentTime = ANIM_DURATION + 1000;
>+    return waitForTwoAnimationFrames();
>+  }).then(t.step_func(function() {
>+    t.done();
>+  }));
>+
>+  player.finished.then(t.step_func(function() {
>+    assert_unreached('finished promise should not resolve when playbackRate ' +
>+                     'is zero');
>+  }));
>+}, 'Test finished promise changes when playbackRate == 0');

This is great. We should probably do the same for playbackRate < 0 right?

>+async_test(function(t) {
>+  var div = addDiv(t);
>+  div.style.animation = ANIM_PROP_VAL;
>+  var player = div.getAnimationPlayers()[0];
>+
>+  var previousFinishedPromise = player.finished;
>+
>+  player.currentTime = ANIM_DURATION;
>+
>+  player.finished.then(function() {
>+    div.style.animationPlayState = 'running';
>+    return waitForTwoAnimationFrames();
>+  }).then(t.step_func(function() {
>+    assert_equals(player.finished, previousFinishedPromise,
>+                  'Should not replay when animation-play-state changes to ' +
>+                  '"running" on finished animation');
>+    t.done();
>+  }));
>+}, 'Test finished promise changes when animationPlayState set to running');

This is great too. Do you think it makes sense to check the currentTime hasn't gone backwards as well? Perhaps in the currentTime tests?

The rest of the tests look really really good. Thanks!

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e610227b9163
https://hg.mozilla.org/mozilla-central/rev/60460c0baf49