Closed Bug 1178664 Opened 5 years ago Closed 4 years ago

Implement finish and cancel animation events

Categories

(Core :: DOM: Animation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: birtles, Assigned: hiro)

References

()

Details

(Keywords: dev-doc-needed)

Attachments

(4 files, 6 obsolete files)

4.75 KB, patch
smaug
: review+
Details | Diff | Splinter Review
6.30 KB, patch
hiro
: review+
Details | Diff | Splinter Review
12.35 KB, patch
hiro
: review+
Details | Diff | Splinter Review
6.68 KB, patch
hiro
: review+
Details | Diff | Splinter Review
This is yet to be merged in the main spec but is currently being specced:

  https://github.com/w3c/web-animations/tree/add-events
Taking.
I am going to implement onfinish event first.
Assignee: nobody → hiikezoe
This is now in the spec.
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
(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!
(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.
Depends on: 1178665
This patch relies on the patch for bug 1178665. I am still thinking MaybeNotifyFinished() is appropriate there.
Part 1 and 2 can be reviewed now since both of them are independent from bug bug 1178665.
Attachment #8633404 - Flags: review?(bbirtles)
Attachment #8633405 - Flags: review?(bugs)
Attachment #8633405 - Flags: review?(bugs) → review+
(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.
Attachment #8633407 - Attachment is obsolete: true
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.)
Attachment #8633404 - Flags: review?(bbirtles) → review?(bugs)
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.
Attachment #8633404 - Flags: review?(bugs) → review-
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().
Attachment #8633404 - Attachment is obsolete: true
Attachment #8633826 - Flags: review?(bugs)
Comment on attachment 8633826 [details] [diff] [review]
Part 1: Make Animation interface EventTarget inheritance v2

Clearing review request since this patch has a problem.
Attachment #8633826 - Flags: review?(bugs)
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).
Attachment #8633826 - Flags: review?(bugs)
Attachment #8633826 - Flags: review?(bugs) → review+
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
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.
(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.
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?
Attachment #8633764 - Attachment is obsolete: true
Attachment #8637487 - Flags: review?(bugs)
Comment on attachment 8637057 [details] [diff] [review]
Part 4: Implement animation.oncancel event v1

And this too
Attachment #8637057 - Flags: review?(bugs)
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.
Attachment #8637057 - Flags: review?(bugs) → review-
Comment on attachment 8637057 [details] [diff] [review]
Part 4: Implement animation.oncancel event v1

Oh, my mistake. reading again.
Attachment #8637057 - Flags: review- → review?(bugs)
Attachment #8637487 - Flags: review?(bugs) → review+
(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
Attachment #8637057 - Flags: review?(bugs) → review+
Attachment #8637057 - Flags: review?(bbirtles)
Attachment #8637487 - Flags: review?(bbirtles)
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.
Attachment #8637487 - Flags: review?(bbirtles) → review+
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.
Attachment #8637057 - Flags: review?(bbirtles) → review+
(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.
(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.
rebased.
Attachment #8633826 - Attachment is obsolete: true
Attachment #8641503 - Flags: review+
Remove animation.playbackRate = ANIM_DURATION line, and test description.
Attachment #8637487 - Attachment is obsolete: true
Attachment #8641506 - Flags: review+
Remove a redundant header inclusion.
Attachment #8637057 - Attachment is obsolete: true
Attachment #8641507 - Flags: review+
You need to log in before you can comment on or make changes to this bug.