Closed Bug 1150808 Opened 4 years ago Closed 4 years ago

Implement Animation.reverse()

Categories

(Core :: DOM: Animation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox40 --- affected
firefox42 --- fixed

People

(Reporter: birtles, Assigned: hiro)

References

()

Details

(Keywords: dev-doc-needed)

Attachments

(1 file, 2 obsolete files)

No description provided.
Component: DOM → DOM: Animation
Blocks: 1178660
Assignee: nobody → hiikezoe
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)
Attachment #8630331 - Flags: review?(bbirtles) → review+
(In reply to Hiroyuki Ikezoe (:hiro) from comment #1)
> Created attachment 8630331 [details] [diff] [review]
> Implament Animation.reverse()

Nit: Implement
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.
Flags: needinfo?(bbirtles)
(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?
Flags: needinfo?(bbirtles)
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
(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.
(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.
(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.
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);
}

?
(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!
(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.
Attached patch Implament Animation.reverse() v2 (obsolete) — Splinter Review
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)
Attachment #8630331 - Attachment is obsolete: true
Attachment #8631842 - Flags: review?(bbirtles)
(In reply to Hiroyuki Ikezoe (:hiro) from comment #13)
> Created attachment 8631842 [details] [diff] [review]
> Implament Animation.reverse() v2

Gosh!
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.
Attachment #8631842 - Flags: review?(bugs)
Attachment #8631842 - Flags: review?(bbirtles)
Attachment #8631842 - Flags: review+
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?
(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
(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
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
oh, my mistake.
Comment on attachment 8631842 [details] [diff] [review]
Implament Animation.reverse() v2

Thanks.
Attachment #8631842 - Flags: review?(bugs) → review+
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.
Attachment #8631842 - Attachment is obsolete: true
Attachment #8631987 - Flags: review+
(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.
https://hg.mozilla.org/mozilla-central/rev/13b425ef17b5
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.