1178664 - Implement finish and cancel animation events

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect)

Description

[Brian Birtles (:birtles)]

This is yet to be merged in the main spec but is currently being specced:

  https://github.com/w3c/web-animations/tree/add-events

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Taking.
I am going to implement onfinish event first.

Comment 2

[Brian Birtles (:birtles)]

This is now in the spec.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Wow thanks! I've been reading the spec on my local!

Upcoming patch sets will consist of three parts:

1. Make Animation interface EventTarget inheritance
2. Implement AnimationPlaybackEvent
3. Implement onfinish event

Comment 4

[Brian Birtles (:birtles)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #3)
> Wow thanks! I've been reading the spec on my local!
> 
> Upcoming patch sets will consist of three parts:
> 
> 1. Make Animation interface EventTarget inheritance
> 2. Implement AnimationPlaybackEvent
> 3. Implement onfinish event

Sorry for the delay, I had to chase up Google to come to agreement about whether finish events should be queued synchronously or asynchronously. There's an explanation of the behavior we decided on in the non-normative section at the end of:

  http://w3c.github.io/web-animations/#update-an-animations-finished-state

We can still change it, of course, if you disagree!

Comment 5

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #4)
> Sorry for the delay, I had to chase up Google to come to agreement about
> whether finish events should be queued synchronously or asynchronously.
> There's an explanation of the behavior we decided on in the non-normative
> section at the end of:
> 
>   http://w3c.github.io/web-animations/#update-an-animations-finished-state
> 
> We can still change it, of course, if you disagree!

I've also been reading the related thread [1].

[1] https://lists.w3.org/Archives/Public/public-fx/2015AprJun/0102.html

I don't understand it well so I am going to implement the event and use it, then will consider what behavior is good for users.

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 1: Make Animation interface EventTarget inheritance

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 2: Add AnimationPlaybackEvent interface

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 3: Implement Animation.onfinish event

This patch relies on the patch for bug 1178665. I am still thinking MaybeNotifyFinished() is appropriate there.

Comment 9

[Hiroyuki Ikezoe (:hiro)]

Part 1 and 2 can be reviewed now since both of them are independent from bug bug 1178665.

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 3: Implement Animation.onfinish event v2

(In reply to Hiroyuki Ikezoe (:hiro) from comment #8)
> This patch relies on the patch for bug 1178665. I am still thinking
> MaybeNotifyFinished() is appropriate there.

I've decided to leave MaybeNotifyFinished as it is and introduce a new method which calling MaybeNotifyFinished and DispatchPlaybackEvent, named DoFinishNotification.

Comment 11

[Brian Birtles (:birtles)]

Comment on attachment 8633404 [details] [diff] [review]
Part 1: Make Animation interface EventTarget inheritance

Olli should review this.

One comment though, I think you can just replace the CLASS/TRAVERSE/UNLINK section in Animation.cpp with:

  NS_IMPL_CYCLE_COLLECTION_INHERITED(Animation, DOMEventTargetHelper, mGlobal, mTimeline, ...)

https://hg.mozilla.org/mozilla-central/file/e7e69cc8c07b/xpcom/glue/nsCycleCollectionParticipant.h#l832

(A while ago we made all these macros variadic so you can just list all the members at once.)

Comment 12

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

Comment on attachment 8633404 [details] [diff] [review]
Part 1: Make Animation interface EventTarget inheritance

>+NS_IMPL_CYCLE_COLLECTION_CLASS(Animation)
>+NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED(Animation,
>+                                                  DOMEventTargetHelper)
>+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mGlobal)
It is not clear to me why you still need mGlobal. DETH as already its owner object and such.

> class Animation
>-  : public nsISupports
>-  , public nsWrapperCache
>+  : public DOMEventTargetHelper
> {
> protected:
>   virtual ~Animation() {}
> 
> public:
>   explicit Animation(nsIGlobalObject* aGlobal)
>     : mGlobal(aGlobal)
>     , mPlaybackRate(1.0)
>@@ -61,18 +61,19 @@ public:
>     , mSequenceNum(kUnsequenced)
>     , mIsRunningOnCompositor(false)
>     , mIsPreviousStateFinished(false)
>     , mFinishedAtLastComposeStyle(false)
>     , mIsRelevant(false)
>   {
>   }
You should call BindToOwner somewhere here.

Comment 13

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 1: Make Animation interface EventTarget inheritance v2

Thank you for reviewing. I did not read in DOMEventTargetHelper.h carefully.

(In reply to Olli Pettay [:smaug] from comment #12)
> > class Animation
> >-  : public nsISupports
> >-  , public nsWrapperCache
> >+  : public DOMEventTargetHelper
> > {
> > protected:
> >   virtual ~Animation() {}
> > 
> > public:
> >   explicit Animation(nsIGlobalObject* aGlobal)
> >     : mGlobal(aGlobal)
> >     , mPlaybackRate(1.0)
> >@@ -61,18 +61,19 @@ public:
> >     , mSequenceNum(kUnsequenced)
> >     , mIsRunningOnCompositor(false)
> >     , mIsPreviousStateFinished(false)
> >     , mFinishedAtLastComposeStyle(false)
> >     , mIsRelevant(false)
> >   {
> >   }
> You should call BindToOwner somewhere here.

I changed to call DOMEventTargetHelper constructor there and replace mGlobal in Animation::Get(Ready|Finished) with the value obtained by GetOwnerGlobal().

Comment 14

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8633826 [details] [diff] [review]
Part 1: Make Animation interface EventTarget inheritance v2

Clearing review request since this patch has a problem.

Comment 15

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8633826 [details] [diff] [review]
Part 1: Make Animation interface EventTarget inheritance v2

The problem is not in this patch, it was in an incoming patch (implementing oncancel event).

Comment 16

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8633764 [details] [diff] [review]
Part 3: Implement Animation.onfinish event v2

Changes from v2:

* Renamed DoFinishNotification to DoFinishNotificationImeediately
* Uses AsyncEventDispatcher instead of calling DispatchDOMEvent to avoid crashes oncancel event
* Check that event timelineTime equals to the animation timelineTime on finished promise resolved

Comment 17

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 4: Implement animation.oncancel event v1

Note about the crashes on cancel event:

Animation::CancelFromStyle is called in nsAnimationManager, nsTransitionManager or AnimationCollection before the animation is destroyed. The target animation has been destroyed at the moment when the cancel event is delivered to the target if we use DispatchDOMEvent directly.

Comment 18

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #17)
> Created attachment 8637057 [details] [diff] [review]

This patch includes a redundant AsyncEventDispatcher.h line. Please ignore it.

Comment 19

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 3: Asyncronous finish notification v3

I did post a wrong patch.


Changes from v2:

* Renamed DoFinishNotification to DoFinishNotificationImeediately
* Uses AsyncEventDispatcher instead of calling DispatchDOMEvent to avoid crashes oncancel event
* Check that event timelineTime equals to the animation timelineTime on finished promise resolved

Olli, could you please check event handling in this patch as well as a patch against bug 1178665?

Comment 20

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8637057 [details] [diff] [review]
Part 4: Implement animation.oncancel event v1

And this too

Comment 21

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

Comment on attachment 8637057 [details] [diff] [review]
Part 4: Implement animation.oncancel event v1

The spec isn't quite clear here, but it says
"cancel
  Queued whenever an animation enters the idle play state from another state.
"
'Queued' hints that cancel should happen asynchronously.

So I think this is wrong and you should actually use
AsyncEventDispatcher to dispatch the event, but not 100% given that the spec is a bit vague.

Comment 22

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

Comment on attachment 8637057 [details] [diff] [review]
Part 4: Implement animation.oncancel event v1

Oh, my mistake. reading again.

Comment 23

[Brian Birtles (:birtles)]

(In reply to Olli Pettay [:smaug] from comment #21)
> So I think this is wrong and you should actually use
> AsyncEventDispatcher to dispatch the event, but not 100% given that the spec
> is a bit vague.

I think you already found it, but just in case, this is more precisely defined here:

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

Comment 24

[Brian Birtles (:birtles)]

Comment on attachment 8637487 [details] [diff] [review]
Part 3: Asyncronous finish notification v3

>+async_test(function(t) {
>+  var div = addDiv(t);
>+  div.style.animation = ANIM_PROP_VAL;
>+  var animation = div.getAnimations()[0];
>+
>+  var finishedTimelineTime;
>+  animation.finished.then(function() {
>+    finishedTimelineTime = animation.timeline.currentTime;
>+  });
>+
>+  animation.onfinish = t.step_func_done(function(event) {
>+    assert_equals(event.currentTime, 0,
>+      'event.currentTime should be zero');
>+    assert_equals(event.timelineTime, finishedTimelineTime,
>+      'event.timelineTime should equal to the animation timeline ' +
>+      'when finished promise is resolved');
>+  });
>+
>+  animation.playbackRate = -1;
>+  animation.currentTime = -ANIM_DURATION;
>+}, 'onfinish event is fired when the currentTime < 0 and ' +
>+   'the playbackRate < 0');

I'm curious why it's necessary to test currentTime to -ANIM_DURATION here? If playbackRate < 0 and currentTime == 0 then it should be finished. Is it to test that the currentTime reported in the event is zero?

>+async_test(function(t) {
>+  var div = addDiv(t, {'class': 'animated-div'});
>+  div.style.animation = ANIM_PROP_VAL;
>+  var animation = div.getAnimations()[0];
>+
>+  var finishedTimelineTime;
>+  animation.finished.then(function() {
>+    finishedTimelineTime = animation.timeline.currentTime;
>+  });
>+
>+  animation.onfinish = t.step_func_done(function(event) {
>+    assert_equals(event.currentTime, ANIM_DURATION,
>+      'event.currentTime should be the effect end');
>+    assert_equals(event.timelineTime, finishedTimelineTime,
>+      'event.timelineTime should equal to the animation timeline ' +
>+      'when finished promise is resolved');
>+  });
>+
>+  animation.finish();
>+}, 'onfinish event is fired when animation.finish()');

Nit: '... when finish() is called'

The rest looks really good. One question however,

Do we need a test for when you seek to ANIM_DURATION + 1000? In that case should event.currentTime be ANIM_DURATION or ANIM_DURATION + 1000. I think the spec says it should be ANIM_DURATION + 1000 but I wonder which is more helpful?

Actually, I can't really remember what the purpose of event.currentTime and event.timelineTime are. In any case, we can change the spec later for that part if we need to. I think this patch is good as it is.

Comment 25

[Brian Birtles (:birtles)]

Comment on attachment 8637057 [details] [diff] [review]
Part 4: Implement animation.oncancel event v1

>diff --git a/dom/animation/Animation.cpp b/dom/animation/Animation.cpp
>--- a/dom/animation/Animation.cpp
>+++ b/dom/animation/Animation.cpp
>@@ -12,16 +12,17 @@
> #include "mozilla/AsyncEventDispatcher.h" //For AsyncEventDispatcher
> #include "AnimationCommon.h" // For AnimationCollection,
>                              // CommonAnimationManager
> #include "nsIDocument.h" // For nsIDocument
> #include "nsIPresShell.h" // For nsIPresShell
> #include "nsLayoutUtils.h" // For PostRestyleEvent (remove after bug 1073336)
> #include "nsThreadUtils.h" // For nsRunnableMethod and nsRevocableEventPtr
> #include "PendingAnimationTracker.h" // For PendingAnimationTracker
>+#include "mozilla/AsyncEventDispatcher.h"

I guess this was added in part 3 so you don't need it any more.

Comment 26

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #25)
> Comment on attachment 8637057 [details] [diff] [review]
> Part 4: Implement animation.oncancel event v1
> 
> >diff --git a/dom/animation/Animation.cpp b/dom/animation/Animation.cpp
> >--- a/dom/animation/Animation.cpp
> >+++ b/dom/animation/Animation.cpp
> >@@ -12,16 +12,17 @@
> > #include "mozilla/AsyncEventDispatcher.h" //For AsyncEventDispatcher
> > #include "AnimationCommon.h" // For AnimationCollection,
> >                              // CommonAnimationManager
> > #include "nsIDocument.h" // For nsIDocument
> > #include "nsIPresShell.h" // For nsIPresShell
> > #include "nsLayoutUtils.h" // For PostRestyleEvent (remove after bug 1073336)
> > #include "nsThreadUtils.h" // For nsRunnableMethod and nsRevocableEventPtr
> > #include "PendingAnimationTracker.h" // For PendingAnimationTracker
> >+#include "mozilla/AsyncEventDispatcher.h"
> 
> I guess this was added in part 3 so you don't need it any more.

Right. I did forget to mentioning it again before asking the review.

Comment 27

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #24)
> Comment on attachment 8637487 [details] [diff] [review]
> Part 3: Asyncronous finish notification v3
> 
> >+async_test(function(t) {
> >+  var div = addDiv(t);
> >+  div.style.animation = ANIM_PROP_VAL;
> >+  var animation = div.getAnimations()[0];
> >+
> >+  var finishedTimelineTime;
> >+  animation.finished.then(function() {
> >+    finishedTimelineTime = animation.timeline.currentTime;
> >+  });
> >+
> >+  animation.onfinish = t.step_func_done(function(event) {
> >+    assert_equals(event.currentTime, 0,
> >+      'event.currentTime should be zero');
> >+    assert_equals(event.timelineTime, finishedTimelineTime,
> >+      'event.timelineTime should equal to the animation timeline ' +
> >+      'when finished promise is resolved');
> >+  });
> >+
> >+  animation.playbackRate = -1;
> >+  animation.currentTime = -ANIM_DURATION;
> >+}, 'onfinish event is fired when the currentTime < 0 and ' +
> >+   'the playbackRate < 0');
> 
> I'm curious why it's necessary to test currentTime to -ANIM_DURATION here?
> If playbackRate < 0 and currentTime == 0 then it should be finished. Is it
> to test that the currentTime reported in the event is zero?

Right. That line is unnecessary. This test should be contrastive the subsequent test.

> Do we need a test for when you seek to ANIM_DURATION + 1000? 

I think it depends on the behavior of currentTime. If animation.currentTime returns ANIM_DURATION + 1000, I don't think the test case is necessary. If animation.currentTime returns ANIM_DURATION, I think the test is needed here.

> In that case should event.currentTime be ANIM_DURATION or ANIM_DURATION + 1000. I think
> the spec says it should be ANIM_DURATION + 1000 but I wonder which is more
> helpful?

I don't know the reason why setting currentTime value does not cut off to the target effect end if the value is over the effect end, but I think animation.currentTime should be cut off first because it is easy to know the value is out of the bounds.

Comment 28

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 1: Make Animation interface EventTarget inheritance v3

rebased.

Comment 29

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 3: Implement Animation.onfinish event v3

Remove animation.playbackRate = ANIM_DURATION line, and test description.

Comment 30

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 4: Implement animation.oncancel event v2

Remove a redundant header inclusion.

Comment 31

[Hiroyuki Ikezoe (:hiro)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=01f94cbc94db

Thank you all!

Comment 32

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4064f631f28a
https://hg.mozilla.org/integration/mozilla-inbound/rev/98c90fc9a0bb
https://hg.mozilla.org/integration/mozilla-inbound/rev/9338f9c7fb95
https://hg.mozilla.org/integration/mozilla-inbound/rev/db31cfe55809

Comment 33

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

https://hg.mozilla.org/mozilla-central/rev/4064f631f28a
https://hg.mozilla.org/mozilla-central/rev/98c90fc9a0bb
https://hg.mozilla.org/mozilla-central/rev/9338f9c7fb95
https://hg.mozilla.org/mozilla-central/rev/db31cfe55809