1150808 - Implement Animation.reverse()

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect)

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Attachment: Implament Animation.reverse()

https://treeherder.mozilla.org/#/jobs?repo=try&revision=97c1914298f0

Comment 2

[Brian Birtles (:birtles)]

Comment on attachment 8630331 [details] [diff] [review]
Implament Animation.reverse()

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

This looks great. I think it's just the rewinding behavior we need to change. r=me with that change and the additional tests.

::: dom/animation/Animation.cpp
@@ +281,5 @@
> +    return;
> +  }
> +
> +  SilentlySetPlaybackRate(-mPlaybackRate);
> +  Play(aRv, LimitBehavior::Continue);

I this this should be LimitBehavior::AutoRewind.

For example if you have,

  // Assume playbackRate > 0 and an animation that is 10s long
  anim.currentTime = 20000;
  anim.reverse();
  // This should start playback from anim.currentTime = 10000

We should add a test for this too.

::: dom/animation/test/css-animations/file_animation-reverse.html
@@ +38,5 @@
> +  })).then(t.step_func_done(function() {
> +    assert_equals(animation.playState, 'running',
> +                  'Animation.playState should be "running" after reverse()');
> +  }));
> +}, 'reverse() starts to play when pausing animation');

I think I would like to add these tests too:

1. reverse() from idle starts playing the animation

   e.g.
   elem.style.animation = "anim 100s";
   var anim = elem.getAnimations()[0];
   elem.style.animation = ""; // cancels animation
   flushComputedStyle(elem); // this is defined in testcommon.js and triggers the update
   // anim.currentTime should be null now
   anim.reverse();
   // anim.currentTime should be 100000 now

2. reverse() maintains the same currentTime (except when we do the rewinding)

   e.g.
   elem.style.animation = "anim 100s";
   var anim = elem.getAnimations()[0];
   anim.currentTime = 50000;
   anim.reverse();
   // anim.currentTime should still be 50000

As per my other comment:

3. reverse() when playbackRate > 0 and currentTime > effect end
4. reverse() when playbackRate > 0 and currentTime < 0
5. reverse() when playbackRate < 0 and currentTime < 0
6. reverse() when playbackRate < 0 and currentTime > effect end
7. reverse() when playbackRate == 0

::: dom/webidl/Animation.webidl
@@ +42,1 @@
>  };

The WebIDL changes need DOM peer review (e.g. smaug, bz)

Comment 3

[Brian Birtles (:birtles)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #1)
> Created attachment 8630331 [details] [diff] [review]
> Implament Animation.reverse()

Nit: Implement

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Thanks for kindly reviewing!

(In reply to Brian Birtles (:birtles) from comment #2)
> 3. reverse() when playbackRate > 0 and currentTime > effect end
> 4. reverse() when playbackRate > 0 and currentTime < 0
> 5. reverse() when playbackRate < 0 and currentTime < 0
> 6. reverse() when playbackRate < 0 and currentTime > effect end

I've added these test (and others). I think you mentioned all of these cases are against finite animations. Should we also test infinite animations? Some of tests should throw InvalidStateError per spec.

> 7. reverse() when playbackRate == 0

In this case, attachment 8630331 [details] [diff] [review] returns -0. Should we set 0 instead of -0 in this case? Chromium does return 0 in this case.

Comment 5

[Brian Birtles (:birtles)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #4)
> Thanks for kindly reviewing!
> 
> (In reply to Brian Birtles (:birtles) from comment #2)
> > 3. reverse() when playbackRate > 0 and currentTime > effect end
> > 4. reverse() when playbackRate > 0 and currentTime < 0
> > 5. reverse() when playbackRate < 0 and currentTime < 0
> > 6. reverse() when playbackRate < 0 and currentTime > effect end
> 
> I've added these test (and others). I think you mentioned all of these cases
> are against finite animations. Should we also test infinite animations? Some
> of tests should throw InvalidStateError per spec.

Oh yes, good idea!

> > 7. reverse() when playbackRate == 0
> 
> In this case, attachment 8630331 [details] [diff] [review] returns -0.
> Should we set 0 instead of -0 in this case? Chromium does return 0 in this
> case.

Oh, that's interesting! I hadn't though of that!

I think playbackRate should probably return 0.

We should probably:

  1. Skip the step to change the playbackRate if playbackRate == 0.0 (I think this covers
     the case when playbackRate == -0.0 since 0.0 == -0.0 right)
  2. If playbackRate == -0.0, set it to 0.0
  3. Update the spec

I can do (3) if you agree.

For (2), if you do:

  anim.playbackRate = -0;
  anim.reverse();
  console.log(anim.playbackRate); // Does this print -0 or 0?

I was thinking it should return 0. What do you think?

Comment 6

[Brian Birtles (:birtles)]

I just checked what Chrome do, and they just skip the method altogether if playbackRate == 0.[1]

[1] https://chromium.googlesource.com/chromium/blink/+/master/Source/core/animation/Animation.cpp#570

Comment 7

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #5)
> > > 7. reverse() when playbackRate == 0
> > 
> > In this case, attachment 8630331 [details] [diff] [review] returns -0.
> > Should we set 0 instead of -0 in this case? Chromium does return 0 in this
> > case.
> 
> Oh, that's interesting! I hadn't though of that!
> 
> I think playbackRate should probably return 0.
> 
> We should probably:
> 
>   1. Skip the step to change the playbackRate if playbackRate == 0.0 

I've also checked blink codes. They also skip following steps. i.e. silently setting playback rate and playing.

> (I think this covers
>      the case when playbackRate == -0.0 since 0.0 == -0.0 right)
>
>   2. If playbackRate == -0.0, set it to 0.0

I think this does not matter because -(-0.0) becomes 0.0.

>   3. Update the spec
> 
> I can do (3) if you agree.
> 
> For (2), if you do:
> 
>   anim.playbackRate = -0;
>   anim.reverse();
>   console.log(anim.playbackRate); // Does this print -0 or 0?
> 
> I was thinking it should return 0. What do you think?

Agree. It should return 0.

just note about setting playbackRate.

anim.playbackRate = -0;
console.log(anim.playbackRate);
does output 0. I do not know what magic happens there. But interestingly in C++ code mPlaybackRate is -0.0 there.

Comment 8

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #5)

> > > 7. reverse() when playbackRate == 0
> > 
> > In this case, attachment 8630331 [details] [diff] [review] returns -0.
> > Should we set 0 instead of -0 in this case? Chromium does return 0 in this
> > case.
> 
> Oh, that's interesting! I hadn't though of that!
> 
> I think playbackRate should probably return 0.
> 
> We should probably:
> 
>   1. Skip the step to change the playbackRate if playbackRate == 0.0 (I
> think this covers
>      the case when playbackRate == -0.0 since 0.0 == -0.0 right)
>   2. If playbackRate == -0.0, set it to 0.0

I was wrong. We can't distinguish 0.0 and -0.0 in C++ code (at least gcc-4.8.4 on Linux).

As you mentioned:
if playbackRate == 0.0 // 0.0 and -0.0 pass this check
and
if playbackRate == -0.0 // 0.0 and -0.0 pass this check.

So what we can do there is to set 0.0 if playbackRate == 0.0 (and -0.0) anyway.

Comment 9

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #8)
> I was wrong. We can't distinguish 0.0 and -0.0 in C++ code (at least
> gcc-4.8.4 on Linux).
> 
> As you mentioned:
> if playbackRate == 0.0 // 0.0 and -0.0 pass this check
> and
> if playbackRate == -0.0 // 0.0 and -0.0 pass this check.
> 
> So what we can do there is to set 0.0 if playbackRate == 0.0 (and -0.0)
> anyway.

In C++ code,

  SilentlySetPlaybackRate(mPlaybackRate != 0.0 ? -mPlaybackRate : 0.0);

works as expected.

Comment 10

[Brian Birtles (:birtles)]

On further thought, maybe it's not important to normalize mPlaybackRate. Maybe we can just do something like:

void
Animation::Reverse(ErrorResult& aRv)
{
  if (!mTimeline || mTimeline->GetCurrentTime().IsNull()) {
    aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR);
    return;
  }

  if (mPlaybackRate == 0.0) {
    return;
  }

  SilentlySetPlaybackRate(-mPlaybackRate);
  Play(aRv, LimitBehavior::AutoRewind);
}

?

Comment 11

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #10)
> On further thought, maybe it's not important to normalize mPlaybackRate.
> Maybe we can just do something like:
> 
> void
> Animation::Reverse(ErrorResult& aRv)
> {
>   if (!mTimeline || mTimeline->GetCurrentTime().IsNull()) {
>     aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR);
>     return;
>   }
> 
>   if (mPlaybackRate == 0.0) {
>     return;
>   }
> 
>   SilentlySetPlaybackRate(-mPlaybackRate);
>   Play(aRv, LimitBehavior::AutoRewind);
> }

Yes!

Comment 12

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #7)

> just note about setting playbackRate.
> 
> anim.playbackRate = -0;
> console.log(anim.playbackRate);
> does output 0. 

I was confused. This is Chrome's behavior. Firefox return -0 instead.
So I think we should handle this as another playbackRate issue.

Comment 13

[Hiroyuki Ikezoe (:hiro)]

Attachment: Implament Animation.reverse() v2

https://treeherder.mozilla.org/#/jobs?repo=try&revision=904962c384ac

Changes:
Early return if mPlaybackRate == 0.0 in Animation::Reverse()
Adds all tests in comment 2
Adds two infinite animation tests (currentTime < 0 cases)

Comment 14

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #13)
> Created attachment 8631842 [details] [diff] [review]
> Implament Animation.reverse() v2

Gosh!

Comment 15

[Brian Birtles (:birtles)]

Comment on attachment 8631842 [details] [diff] [review]
Implament Animation.reverse() v2

>+async_test(function(t) {
>+  var div = addDiv(t, { style: 'animation: anim 100s infinite' });
>+  var animation = div.getAnimations()[0];
>+
>+  animation.ready.then(t.step_func(function() {
>+    return waitForAnimationFrames(10);
>+  })).then(t.step_func(function() {
>+    animation.pause();
>+    return animation.ready;

Why do we need to wait for 10 frames here? Why don't we just seek the currentTime to 50000 like you do in later tests?

It seems like that would be more reliable and would be faster too.

(This is for the 'reverse() starts to play when pausing animation' test, by the way).

>+async_test(function(t) {
>+  var div = addDiv(t, { style: 'animation: anim 100s' });
>+  var animation = div.getAnimations()[0];
>+
>+  animation.playbackRate = 0;
>+  animation.reverse();
>+
>+  assert_equals(animation.playbackRate, 0,
>+    'reverse() should preserve playbackRate if the playbackRate == 0');
>+  t.done();
>+}, 'reverse() when playbackRate == 0');

It might be worth seeking the animation to 50000 in and checking that reverse does not affect the currentTime when playbackRate == 0? What do you think?


The rest looks great. Adding smaug for the WebIDL changes.

Comment 16

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

Comment on attachment 8631842 [details] [diff] [review]
Implament Animation.reverse() v2

The spec doesn't say the method should throw.
Why do we want to throw?

Comment 17

[Brian Birtles (:birtles)]

(In reply to Olli Pettay [:smaug] from comment #16)
> Comment on attachment 8631842 [details] [diff] [review]
> Implament Animation.reverse() v2
> 
> The spec doesn't say the method should throw.
> Why do we want to throw?

The spec calls the following procedure which throws:

http://w3c.github.io/web-animations/#reversing-an-animation-section

Comment 18

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #17)
> (In reply to Olli Pettay [:smaug] from comment #16)
> > Comment on attachment 8631842 [details] [diff] [review]
> > Implament Animation.reverse() v2
> > 
> > The spec doesn't say the method should throw.
> > Why do we want to throw?
> 
> The spec calls the following procedure which throws:
> 
> http://w3c.github.io/web-animations/#reversing-an-animation-section

Yes, playing an animation throws InvalidStateError.
http://w3c.github.io/web-animations/#play-an-animation

Comment 19

[Brian Birtles (:birtles)]

I'm guessing Olli was confused because sometimes the spec spells out the exceptions thrown and other times it doesn't. I filed an issue on the spec to fix this.[1]

[1] https://github.com/w3c/web-animations/issues/101

Comment 20

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

oh, my mistake.

Comment 21

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

Comment on attachment 8631842 [details] [diff] [review]
Implament Animation.reverse() v2

Thanks.

Comment 22

[Hiroyuki Ikezoe (:hiro)]

Attachment: Implement Animation.reverse() v3

Thank you all a lot!

(In reply to Brian Birtles (:birtles) from comment #15)
> >+async_test(function(t) {
> >+  var div = addDiv(t, { style: 'animation: anim 100s infinite' });
> >+  var animation = div.getAnimations()[0];
> >+
> >+  animation.ready.then(t.step_func(function() {
> >+    return waitForAnimationFrames(10);
> >+  })).then(t.step_func(function() {
> >+    animation.pause();
> >+    return animation.ready;
> 
> Why do we need to wait for 10 frames here? Why don't we just seek the
> currentTime to 50000 like you do in later tests?

That's because the test is a remnant when I did not know setting currentTime is immediately effective.
Fixed.

> >+async_test(function(t) {
> >+  var div = addDiv(t, { style: 'animation: anim 100s' });
> >+  var animation = div.getAnimations()[0];
> >+
> >+  animation.playbackRate = 0;
> >+  animation.reverse();
> >+
> >+  assert_equals(animation.playbackRate, 0,
> >+    'reverse() should preserve playbackRate if the playbackRate == 0');
> >+  t.done();
> >+}, 'reverse() when playbackRate == 0');
> 
> It might be worth seeking the animation to 50000 in and checking that
> reverse does not affect the currentTime when playbackRate == 0? What do you
> think?

I took it in this patch.

Comment 23

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #12)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #7)
> 
> > just note about setting playbackRate.
> > 
> > anim.playbackRate = -0;
> > console.log(anim.playbackRate);
> > does output 0. 
> 
> I was confused. This is Chrome's behavior. Firefox return -0 instead.
> So I think we should handle this as another playbackRate issue.

Filed bug 1182393.

Comment 24

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/13b425ef17b5

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/13b425ef17b5