Closed Bug 1302648 Opened 8 years ago Closed 7 years ago

Implement animationcancel event.

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: mantaroh, Assigned: mantaroh)

References

()

Details

(Keywords: dev-doc-complete)

Attachments

(9 files)

59 bytes, text/x-review-board-request
masayuki
: review+
Details
59 bytes, text/x-review-board-request
baku
: review+
Details
59 bytes, text/x-review-board-request
birtles
: review+
Details
59 bytes, text/x-review-board-request
birtles
: review+
Details
59 bytes, text/x-review-board-request
birtles
: review+
Details
59 bytes, text/x-review-board-request
birtles
: review+
Details
59 bytes, text/x-review-board-request
birtles
: review+
Details
59 bytes, text/x-review-board-request
birtles
: review+
Details
59 bytes, text/x-review-board-request
birtles
: review+
Details
The animationcancel is defined by CSS Animations level 2 Editor's Draft[1].

[1] https://drafts.csswg.org/css-animations-2/#eventdef-animationevent-animationcancel

Currently, the user agent which implement this does not exist. But I think that we may need to implement this event in the future.
I've written the page for onanimationcancel:

https://developer.mozilla.org/en-US/docs/Web/API/GlobalEventHandlers/onanimationcancel

The page for animationcancel itself needs to be written still, and then the Using CSS transitions article updated.
I think that we will need to consider same things of transitioncancel.
i.e.
 - Synchronous queueing. (call the Animation.cancel() / set Animation.timeline to null)
 - Request one more tick when cancel from style.
 - Animation have null target effect.

Perhaps, implementation same to transitioncancel. However, CSS-Animation can cancel due to clear the animation-name, set display style to 'none'.
Assignee: nobody → mantaroh
Status: NEW → ASSIGNED
Currently, The timing of release the OwningElement is different from Transition.
However, I think that this order is not important. So I change the order of release timing of OwningElement. [1] 

[1] https://dxr.mozilla.org/mozilla-central/rev/8ff550409e1d1f8b54f6f7f115545dbef857be0b/layout/style/nsAnimationManager.h#113,128

i.e. change as following code:

Before:
-----------------------------
void CancelFromStyle() override
{
  mOwningElement = OwningElementRef();

  // some codes..

  Animation::CancelFromStyle();
}
-----------------------------

After:
-----------------------------
void CancelFromStyle() override
{
  Animation::CancelFromStyle();

  // some codes..

  mOwningElement = OwningElementRef();
}
-----------------------------


https://treeherder.mozilla.org/#/jobs?repo=try&revision=d5dd470be2623de704aaf80fbfd24b3a6250c48d
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #3)
> Currently, The timing of release the OwningElement is different from
> Transition.
> However, I think that this order is not important. So I change the order of
> release timing of OwningElement. [1] 
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/rev/
> 8ff550409e1d1f8b54f6f7f115545dbef857be0b/layout/style/nsAnimationManager.
> h#113,128
> 

As result of try, Perhaps we can change the order of releasing OwningElement in nsAnimationManager::CancelFromStyle().
I think that we will need to this change since we need to owning element when dispatching the animation event.
A cancel test failed...
https://treeherder.mozilla.org/#/jobs?repo=try&revision=43dd357862e57e551ef8e650726130b86fe6a3b7

This changeset won't fire the animationcancel event when calling the Animation.cancel().
In Animation::Cancel(), we remove animation from timeline after calling Animation::UpdateTiming, So next tick doesn't occur. In Animation::UpdateTiming(), we request adding refresh observer if the related timeline doesn't have a registered refresh observer.[1]

[1] https://dxr.mozilla.org/mozilla-central/rev/1d025ac534a6333a8170a59a95a8a3673d4028ee/dom/animation/DocumentTimeline.cpp#137-146

So I think that we will need to call Animation::UpdateTiming after removing the animation from the timeline.

Perhaps, transitioncancel has the same problem.

WIP: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ef1209f6fbcab1ae680591a49649b397b082a68b
Comment on attachment 8832791 [details]
Bug 1302648 part 1 - Add animationcancel event handler.

https://reviewboard.mozilla.org/r/109022/#review110202
Attachment #8832791 - Flags: review?(masayuki) → review+
Comment on attachment 8832792 [details]
Bug 1302648 part 2 - Add onanimationcancel EventHandler to WebIDL.

https://reviewboard.mozilla.org/r/109024/#review110224
Attachment #8832792 - Flags: review?(amarchesini) → review+
Comment on attachment 8832793 [details]
Bug 1302648 part 3 - Change order of releasing owning element when cancel animation.

https://reviewboard.mozilla.org/r/109060/#review110954
Attachment #8832793 - Flags: review?(bbirtles) → review+
Comment on attachment 8832793 [details]
Bug 1302648 part 3 - Change order of releasing owning element when cancel animation.

https://reviewboard.mozilla.org/r/109060/#review110956

::: layout/style/nsAnimationManager.h:129
(Diff revision 1)
> +    // To release owning element here is important to dispatch the animation
> +    // cancel event since we will dispatch animation cancel event synchronously.
> +    // Currently, we queue this event asynchronously according to ticking.
> +    // (For detail, see CSSAnimation::Tick())
> +    // If we queue the animation cancel synchronously,
> +    // we should need to owning element.
> +    // i.e. we need owning element when calling CancelFromStyle().

// We need to do this *after* calling CancelFromStyle() since
    // CancelFromStyle might synchronously trigger a cancel event for which
    // we need an owning element to target the event at.

(I added this issue as part of reviewing the previous patch but MozReview reported "HTTP 0 error comment not saved" or something like that and saved the r+ without the issue!)
Comment on attachment 8832794 [details]
Bug 1302648 part 4 - Call UpdateTiming() after removing the animation from the timeline.

https://reviewboard.mozilla.org/r/109062/#review110970

::: dom/animation/Animation.cpp
(Diff revision 1)
> -  UpdateTiming(SeekFlag::NoSeek, SyncNotifyFlag::Async);
> -

(I'm a little concerned that this might introduce bugs in future because we end up queuing events before updating all timing information such as finished state. I guess if all our tests pass it's ok but it feels a little dangerous.)

::: dom/animation/Animation.cpp:795
(Diff revision 1)
>    if (mTimeline) {
>      mTimeline->RemoveAnimation(this);
>    }
>    MaybeQueueCancelEvent(activeTime);
> +
> +  UpdateTiming(SeekFlag::NoSeek, SyncNotifyFlag::Async);

This needs a comment saying something like,

  // When an animation is cancelled it no longer needs further ticks from the
  // timeline. However, if we queued a cancel event and this was the last
  // animation attached to the timeline, the timeline will stop observing the
  // refresh driver and there may be no subsequent refresh driver tick for
  // dispatching the queued event.
  //
  // By calling UpdateTiming *after* removing ourselves from our timeline, we
  // ensure the timeline will register with the refresh driver for at least one
  // more tick.
Attachment #8832794 - Flags: review?(bbirtles) → review+
Comment on attachment 8832795 [details]
Bug 1302648 part 5 - Queue animationcancel when animation status is idle.

https://reviewboard.mozilla.org/r/109064/#review110960

Looking good but I want to have another look after the templated helper method has been added.

::: layout/style/nsAnimationManager.cpp:207
(Diff revision 1)
> +    intervalStartTime  = zeroDuration;
> +    intervalEndTime    = zeroDuration;
> +    iterationStartTime = zeroDuration;

StickyTimeDuration initializes to zero so we can drop these three lines (and probably do likewise in nsTransitionManager.cpp).

::: layout/style/nsAnimationManager.cpp:210
(Diff revision 1)
> +  if (!mEffect) {
> +    currentPhase       = GetAnimationPhaseWithoutEffect();
> +    intervalStartTime  = zeroDuration;
> +    intervalEndTime    = zeroDuration;
> +    iterationStartTime = zeroDuration;
> +    currentIteration   = 0;

We could also just initialize currentIteration to 0 above and drop this line here.

::: layout/style/nsAnimationManager.cpp:214
(Diff revision 1)
> +    iterationStartTime = zeroDuration;
> +    currentIteration   = 0;
> +  } else {
> +    ComputedTiming computedTiming = mEffect->GetComputedTiming();
> +    currentPhase = computedTiming.mPhase;
> +    currentIteration  = computedTiming.mCurrentIteration;

Nit: extra space here

::: layout/style/nsAnimationManager.cpp:241
(Diff revision 1)
>    TimeStamp startTimeStamp     = ElapsedTimeToTimeStamp(intervalStartTime);
>    TimeStamp endTimeStamp       = ElapsedTimeToTimeStamp(intervalEndTime);
>    TimeStamp iterationTimeStamp = ElapsedTimeToTimeStamp(iterationStartTime);
>  
>    AutoTArray<AnimationEventParams, 2> events;
> +  // Handle cancel event first

Nit: A blank line before this comment would make this easier to follow.

::: layout/style/nsAnimationManager.cpp:311
(Diff revision 1)
> +ComputedTiming::AnimationPhase
> +CSSAnimation::GetAnimationPhaseWithoutEffect() const
> +{
> +  MOZ_ASSERT(!mEffect, "Should only be called when we do NOT have an effect");
> +
> +  Nullable<TimeDuration> currentTime = GetCurrentTime();
> +  if (currentTime.IsNull()) {
> +    return ComputedTiming::AnimationPhase::Idle;
> +  }
> +
> +  // If we don't have a target effect, the duration will be zero so the phase is
> +  // 'before' if the current time is less than zero.
> +  return currentTime.Value() < TimeDuration()
> +         ? ComputedTiming::AnimationPhase::Before
> +         : ComputedTiming::AnimationPhase::After;
> +}
> +

This is almost identical to CSSTransition::GetTransitionPhaseWithoutEffect.

I'm pretty sure we can factor out a template method AnimationCommon.h that takes the PhaseType as a template parameter (TransitionPhase or ComputedTiming::AnimationPhase) and calls GetCurrentTime() and GetEffect() (instead of mEffect) on the passed-in Animation subclass.
Attachment #8832795 - Flags: review?(bbirtles)
Comment on attachment 8832796 [details]
Bug 1302648 part 6 - Add animationcancel test to legacy tests.

https://reviewboard.mozilla.org/r/109066/#review110974

r=me with comments addressed.

::: layout/style/test/test_animations_event_handler_attribute.html:92
(Diff revision 1)
>  advance_clock(200);
>  checkReceivedEvents("animationend", targets);
>  
>  targets.forEach(div => { div.remove(); });
>  
> +// 1b. Test CSS Animation cancel event handler.

Nit: Drop the '.' at the end of this sentence for consistency with the other headings

::: layout/style/test/test_animations_event_handler_attribute.html:105
(Diff revision 1)
> +advance_clock(200);
> +
> +targets.forEach(div => {
> +  div.style.display = "none"
> +});
> +getComputedStyle(targets[0]).display; // flush

Nit: Move this into the loop above for consistency with the previous loop.

i.e.

targets.forEach(div => {
  div.style.display = "none"
  getComputedStyle(div).display; // flush
});

::: layout/style/test/test_animations_event_handler_attribute.html:109
(Diff revision 1)
> +
> +advance_clock(200);
> +targets.forEach( div => { is(div.receivedEventType, undefined); });

What is this testing? Is it testing we don't get any animationend events, for example? Because we didn't register for any other event types so I don't think it tests for that.

Maybe we should just drop this part.

::: layout/style/test/test_animations_event_order.html:691
(Diff revision 1)
> +
> +checkEventOrder([ divs[1], 'animationstart' ],
> +                [ divs[0], 'animationstart' ],
> +                [ divs[1], 'animationcancel' ],
> +                [ divs[0], 'animationcancel' ],
> +                'Simultaneous animationstart/cancel on siblings');

Maybe just 'Simultaneous animationcancel on siblings' (the animationstart events aren't exactly simultaneous).
Attachment #8832796 - Flags: review?(bbirtles) → review+
Comment on attachment 8832797 [details]
Bug 1302648 part 7 - More simple animation event dispatch tests.

https://reviewboard.mozilla.org/r/109068/#review110976
Attachment #8832797 - Flags: review?(bbirtles) → review+
Comment on attachment 8832798 [details]
Bug 1302648 part 8 - Add animationcancel tests.

https://reviewboard.mozilla.org/r/109070/#review110978

r=me with comments addressed

::: dom/animation/test/css-animations/file_animation-cancel.html:141
(Diff revision 1)
> +
> +  var animation = div.getAnimations()[0];
> +
> +  return animation.ready.then(function() {
> +    assert_equals(animation.playState, 'running');
> +    // Set a no-keyframes

Let's drop this comment. I'm not sure what a 'no-keyframes' is. 'none' is a keyword, but this comment makes it sound like it is the name of some special keyframes.

::: dom/animation/test/css-animations/file_animation-cancel.html:149
(Diff revision 1)
> +    return waitForFrame();
> +  }).then(function() {
> +    assert_equals(animation.playState, 'idle');
> +    assert_equals(getComputedStyle(div).marginLeft, '0px');
> +  });
> +}, 'To set a no keyframe to animation-name cancels animation on that property');

'Setting animation-name to \'none\' cancels the animation'

::: dom/animation/test/css-animations/file_animation-cancel.html:157
(Diff revision 1)
> +    // Set display property to none.
> +    div.style.display = 'none';

This comment isn't needed.

::: dom/animation/test/css-animations/file_animation-cancel.html:178
(Diff revision 1)
> +
> +  var animation = childDiv.getAnimations()[0];
> +
> +  return animation.ready.then(function() {
> +    assert_equals(animation.playState, 'running');
> +    // Set ancestor's display property to none.

Again, this comment is not needed.

::: dom/animation/test/css-animations/file_animation-cancel.html:185
(Diff revision 1)
> +    return waitForFrame();
> +  }).then(function() {
> +    assert_equals(animation.playState, 'idle');
> +    assert_equals(getComputedStyle(childDiv).marginLeft, '0px');
> +  });
> +}, 'To set display:none on an ancestor of element cancels its animations');

'Setting display:none on an ancestor element cancels animations on descendants'

::: dom/animation/test/css-animations/file_event-dispatch.html:121
(Diff revision 1)
> +
> +promise_test(function(t) {
> +  const [animation, watcher, div] = setupAnimation(t, 'anim 100s');
> +
> +  return watcher.wait_for('animationstart').then(function(evt) {
> +    animation.currentTime = 0.0;

Would a non-zero value be more interesting to test? e.g. 100.0 ?

::: dom/animation/test/css-animations/file_event-dispatch.html:122
(Diff revision 1)
> +    // Make idle
> +    animation.timeline = null;
> +    return watcher.wait_for('animationcancel');

(This behavior is probably going to change soon but it's ok for now.)

::: dom/animation/test/css-animations/file_event-dispatch.html:126
(Diff revision 1)
> +    animation.currentTime = 0.0;
> +    // Make idle
> +    animation.timeline = null;
> +    return watcher.wait_for('animationcancel');
> +  }).then(function(evt) {
> +    assert_approx_equals(evt.elapsedTime, 0.0, 0.0001);

I think we can use assert_times_equal here right?

::: dom/animation/test/css-animations/file_event-dispatch.html:135
(Diff revision 1)
> +promise_test(function(t) {
> +  // we should NOT pause animation since calling cancel synchronously.
> +  const [animation, watcher, div] = setupAnimation(t, 'anim 100s');
> +
> +  return watcher.wait_for('animationstart').then(function(evt) {
> +    animation.currentTime = 0.0;

Use a more interesting value?

::: dom/animation/test/css-animations/file_event-dispatch.html:136
(Diff revision 1)
> +  // we should NOT pause animation since calling cancel synchronously.
> +  const [animation, watcher, div] = setupAnimation(t, 'anim 100s');
> +
> +  return watcher.wait_for('animationstart').then(function(evt) {
> +    animation.currentTime = 0.0;
> +    // Make idle

I don't think we need this comment.

::: dom/animation/test/css-animations/file_event-dispatch.html:140
(Diff revision 1)
> +    animation.currentTime = 0.0;
> +    // Make idle
> +    animation.cancel();
> +    return watcher.wait_for('animationcancel');
> +  }).then(function(evt) {
> +    assert_approx_equals(evt.elapsedTime, 0.0, 0.0001);

assert_times_equal

::: dom/animation/test/css-animations/file_event-dispatch.html:350
(Diff revision 1)
> +  }).then(function(evt) {
> +    animation.timeline = document.timeline;
> +    animation.play();
> +    return watcher.wait_for('animationstart');
> +  });
> +}, 'Set timeline and play transition after clear the timeline.');

s/after clear/after clearing/
Attachment #8832798 - Flags: review?(bbirtles) → review+
Comment on attachment 8832799 [details]
Bug 1302648 part 9 - Add test of setting target effect of CSS Animations.

https://reviewboard.mozilla.org/r/109072/#review110980

As a general comment, I notice we have a lot of tests with descriptions like 'Tests for ...'. I probably wrote quite a few of them. However, if we write test descriptions as some sort of assertion/expectation (e.g. 'Setting display:none causes an animationcancel event to be dispatched') then it makes the test more focused and easier to read/maintain/review.

This is very good but I think we want a test where we change the effect to an effect that targets a *different* element so we can test that the event is dispatched at the original element.

It would also be good to test setting an effect on a finished animation such that the animation becomes running again to check that we fire an animationstart event (or do we have that test somewhere already?)

I'd like to review this again with those tests added.

::: dom/animation/test/css-animations/file_setting-effect.html:28
(Diff revision 1)
> +    assert_equals(animation.playState, 'finished');
> +    assert_equals(window.getComputedStyle(div).marginLeft, '0px');
> +    return watcher.wait_for('animationend');

This is testing three different things:

* When the effect is set to null, the animation is finished (tested through the playState)
* When the effect is set to null, we update the target property
* When the effect is set to null we dispatch an animationend event

These should really be three separate tests. I'm ok with leaving this as-is for now, but in future I think you'll find that writing more fine-grained tests means we're less likely to miss important cases.

::: dom/animation/test/css-animations/file_setting-effect.html:29
(Diff revision 1)
> +  var animation = div.getAnimations()[0];
> +  return animation.ready.then(function() {
> +    animation.currentTime = 50 * MS_PER_SEC;
> +    animation.effect = null;
> +    assert_equals(animation.playState, 'finished');
> +    assert_equals(window.getComputedStyle(div).marginLeft, '0px');

nit: s/window.getComputedStyle/getComputedStyle/ would be a little shorter here and elsewhere in this file

::: dom/animation/test/css-animations/file_setting-effect.html:32
(Diff revision 1)
> +    animation.effect = null;
> +    assert_equals(animation.playState, 'finished');
> +    assert_equals(window.getComputedStyle(div).marginLeft, '0px');
> +    return watcher.wait_for('animationend');
> +  });
> +}, 'Test for removing a animation effect');

s/removing a animation/removing an animation/

::: dom/animation/test/css-animations/file_setting-effect.html:32
(Diff revision 1)
> +    animation.effect = null;
> +    assert_equals(animation.playState, 'finished');
> +    assert_equals(window.getComputedStyle(div).marginLeft, '0px');
> +    return watcher.wait_for('animationend');
> +  });
> +}, 'Test for removing a animation effect');

'Setting a null effect on a running animation fires an animationend event'

::: dom/animation/test/css-animations/file_setting-effect.html:38
(Diff revision 1)
> +  var watcher = new EventWatcher(t, div, [ 'animationend',
> +                                           'animationcancel' ]);
> +

This doesn't seem to be used.

::: dom/animation/test/css-animations/file_setting-effect.html:47
(Diff revision 1)
> +  return animation.ready.then(function() {
> +    animation.currentTime = 50 * MS_PER_SEC;
> +    animation.effect = new KeyframeEffect(div,
> +                                          { left: [ '0px' , '100px'] },
> +                                          100 * MS_PER_SEC);
> +    assert_equals(animation.playState, 'running');

(Not sure we need this line? If we want to test playState it should probably be a separate test.)

::: dom/animation/test/css-animations/file_setting-effect.html:51
(Diff revision 1)
> +                                          100 * MS_PER_SEC);
> +    assert_equals(animation.playState, 'running');
> +    assert_equals(window.getComputedStyle(div).marginLeft, '0px');
> +    assert_equals(window.getComputedStyle(div).left, '50px');
> +  });
> +}, 'Test for replacing the animation effect by a new keyframe effect');

'Replacing an animation's effect with an effect that targets a different property should update both properties'

::: dom/animation/test/css-animations/file_setting-effect.html:57
(Diff revision 1)
> +  var watcher = new EventWatcher(t, div, [ 'animationend',
> +                                           'animationcancel' ]);
> +

Not used?

::: dom/animation/test/css-animations/file_setting-effect.html:68
(Diff revision 1)
> +    animation.effect = new KeyframeEffect(div,
> +                                          { left: [ '0px' , '100px'] },
> +                                          20 * MS_PER_SEC);
> +    assert_equals(animation.playState, 'finished');
> +  });
> +}, 'Test for setting a new keyframe effect with a shorter duration');

'Replacing an animation's effect with a shorter one that should have already finished, the animation finishes immediately' ?

::: dom/animation/test/css-animations/file_setting-effect.html:74
(Diff revision 1)
> +  var watcher = new EventWatcher(t, div, [ 'animationend',
> +                                           'animationcancel' ]);
> +

Not used?

::: dom/animation/test/css-animations/file_setting-effect.html:88
(Diff revision 1)
> +  assert_equals(animation.playState, 'pending');
> +
> +  return animation.ready.then(function() {
> +    assert_equals(animation.playState, 'running');
> +  });
> +}, 'Test for setting a new keyframe effect to a pending animation');

A play-pending animation's effect whose effect is replaced still exits the pending state
Attachment #8832799 - Flags: review?(bbirtles)
Comment on attachment 8832795 [details]
Bug 1302648 part 5 - Queue animationcancel when animation status is idle.

https://reviewboard.mozilla.org/r/109064/#review110960

Thanks.

I changed GetAnimationPhaseWithoutEffect function.

> This is almost identical to CSSTransition::GetTransitionPhaseWithoutEffect.
> 
> I'm pretty sure we can factor out a template method AnimationCommon.h that takes the PhaseType as a template parameter (TransitionPhase or ComputedTiming::AnimationPhase) and calls GetCurrentTime() and GetEffect() (instead of mEffect) on the passed-in Animation subclass.

I think that CSSTransition class is subclass of Animation, not CommonAnimationManager.
So we can not implement this function into AnimationCommon.h.

I moved this function to Animation.h, and used template.
Comment on attachment 8832799 [details]
Bug 1302648 part 9 - Add test of setting target effect of CSS Animations.

https://reviewboard.mozilla.org/r/109072/#review110980

Sorry, previous patch doesn't contain necessary space.

>>It would also be good to test setting an effect on a finished animation such that the animation becomes running again to check
> that we fire an animationstart event (or do we have that test somewhere already?)

We have a test which checks the animationstart event after canceling animation. However, We haven't the test which checks the animationstart after ending animation.
So I added this test.
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #35)
> I think that CSSTransition class is subclass of Animation, not
> CommonAnimationManager.
> So we can not implement this function into AnimationCommon.h.

AnimationCommon.h is for more than just CommonAnimationManager. It contains all the common machinery for CSS animations and CSS transitions (e.g. DelayedEventDispatcher, OwningElementRef).
Sorry, I looked into the issue about whether we should fire animationcancel after animationend and discovered that level 1 of CSS Animations seems to suggest that we should NOT. I think that's also probably more intuitive and matches what we do for transitioncancel. I will update the spec for CSS Animations Level 2 to make this more clear. As discussed, let's update these patches to NOT fire animationcancel if we finished normally.
Comment on attachment 8832795 [details]
Bug 1302648 part 5 - Queue animationcancel when animation status is idle.

https://reviewboard.mozilla.org/r/109064/#review111866

This looks great. Cancelling review for now, however, since, as discussed, we need to address the two issues below.

::: dom/animation/Animation.h:412
(Diff revision 2)
> +  // Return the TransitionPhase or AnimationPhase to use when the animation
> +  // doesn't have a target effect.
> +  template <typename PhaseType>
> +  PhaseType GetAnimationPhaseWithoutEffect() const
> +  {
> +    MOZ_ASSERT(!mEffect, "Should only be called when we do NOT have an effect");
> +
> +    Nullable<TimeDuration> currentTime = GetCurrentTime();
> +    if (currentTime.IsNull()) {
> +      return PhaseType::Idle;
> +    }
> +
> +    // If we don't have a target effect, the duration will be zero so the phase is
> +    // 'before' if the current time is less than zero.
> +    return currentTime.Value() < TimeDuration()
> +      ? PhaseType::Before
> +      : PhaseType::After;
> +  }
> +

As discussed, let's move this to AnimationCommon.h and not make it a member function, but just a regular templated helper function that takes a const Animation& parameter (and uses GetEffect() instead of mEffect()).

::: layout/style/nsAnimationManager.cpp:240
(Diff revision 2)
> +  if (mPreviousPhase != AnimationPhase::Idle &&
> +      currentPhase == AnimationPhase::Idle) {

As discussed we should change this to NOT fire animationcancel if we finished normally.
Attachment #8832795 - Flags: review?(bbirtles)
Comment on attachment 8832799 [details]
Bug 1302648 part 9 - Add test of setting target effect of CSS Animations.

https://reviewboard.mozilla.org/r/109072/#review111870

I'm not sure that the added tests are quite right.

::: dom/animation/test/css-animations/file_setting-effect.html:84
(Diff revisions 1 - 3)
> +promise_test(function(t) {
> +  var div1 = addDiv(t);
> +  var div2 = addDiv(t);
> +  div1.style.animation = 'anim 100s';
> +  div2.style.animation = 'anim 50s';
> +  var watcher1 = new EventWatcher(t, div1, [ 'animationstart',
> +                                             'animationend' ]);
> +  var watcher2 = new EventWatcher(t, div2, [ 'animationstart',
> +                                             'animationend' ]);
> +
> +  var animation1 = div1.getAnimations()[0];
> +  var animation2 = div2.getAnimations()[0];
> +  animation1.effect = animation2.effect;
> +
> +  watcher1.wait_for('animationstart').then(function() {
> +    animation1.currentTime = 50 * MS_PER_SEC;
> +    animation2.currentTime = 50 * MS_PER_SEC;
> +    return watcher.wait_for('animationend');
> +  }).then(function() {
> +    animation1.currentTime = 100 * MS_PER_SEC;
> +    animation2.currentTime = 100 * MS_PER_SEC;
> +
> +    // Then wait a couple of frames and check that no event was dispatched.
> +    return waitForAnimationFrames(2);
> +  });
> +}, 'The event is dispatched at the original element even if set another ' +
> +   'animation effect');

I think this test was added due to this review comment:

"I think we want a test where we change the effect to an effect that targets a *different* element so we can test that the event is dispatched at the original element."

What I had in mind, was a test where we simply change the target element. Something like:

  promise_test(function(t) {
    var div1 = addDiv(t);
    var div2 = addDiv(t);

    var watcher1 = new EventWatcher(t, div1, [ 'animationstart' ]);
    // Watch |div2| as well to ensure it does *not* get events.
    var watcher2 = new EventWatcher(t, div2, [ 'animationstart' ]);

    div1.style.animation = 'anim 100s';

    // Update target from div1 -> div2
    var animation = div1.getAnimations()[0];
    animation.effect = new KeyframeEffect(div2,
                                          { left: [ '0px' , '100px'] },
                                          100 * MS_PER_SEC);

    watcher1.wait_for('animationstart').then(function() {
      // Wait a couple of frames and check that no other events were dispatched.
      return waitForAnimationFrames(2);
    });
  }, 'The event is dispatched at the original element even if set an effect ' +
     'with a different target element');

That seems simpler?

::: dom/animation/test/css-animations/file_setting-effect.html:101
(Diff revisions 1 - 3)
> +  animation1.effect = animation2.effect;
> +
> +  watcher1.wait_for('animationstart').then(function() {
> +    animation1.currentTime = 50 * MS_PER_SEC;
> +    animation2.currentTime = 50 * MS_PER_SEC;
> +    return watcher.wait_for('animationend');

How does this work? I don't think |watcher| is defined?

::: dom/animation/test/css-animations/file_setting-effect.html:112
(Diff revisions 1 - 3)
> +promise_test(function(t) {
> +  var div = addDiv(t);
> +  var watcher = new EventWatcher(t, div, [ 'animationstart',
> +                                           'animationend',
> +                                           'animationcancel' ]);
> +  div.style.animation = 'anim 100s';
> +  var animation = div.getAnimations()[0];
> +
> +  return watcher.wait_for('animationstart').then(function(evt) {
> +    animation.currentTime = 100 * MS_PER_SEC;
> +    return watcher.wait_for('animationend');
> +  }).then(function(evt) {
> +    animation.play();
> +    return watcher.wait_for('animationstart');
> +  });
> +}, 'The animationstart is dispatched after restarting an finished animation');

I think this test was added because of this review comment:

  "It would also be good to test setting an effect on a finished animation such that the animation becomes running again to check that we fire an animationstart event (or do we have that test somewhere already?)"

However, this test doesn't set an effect? I was thinking of something like:
  promise_test(function(t) {
    var div = addDiv(t);
    var watcher = new EventWatcher(t, div, [ 'animationstart',
                                            'animationend',
                                            'animationcancel' ]);
    div.style.animation = 'anim 100s';
    var animation = div.getAnimations()[0];
    animation.finish();

    return watcher.wait_for([ 'animationstart', 'animationend' ])
    .then(function(evt) {
      // Set a longer effect
      animation.effect = new KeyframeEffect(div,
                                            { left: [ '0px' , '100px'] },
                                            200 * MS_PER_SEC);
      return watcher.wait_for('animationstart');
    });
  }, 'After replacing a finished animation\'s effect with a longer one ' +
    'it fires an animationstart event');
Attachment #8832799 - Flags: review?(bbirtles)
We will need to send an intent to implement and ship before landing this. Something like the following:

Intent to implement and ship: animationcancel event

> Summary:
Adds a new event that fires when an animation is terminated without
finishing normally (e.g. element is made display:none, animation-name
property is updated etc.). This parallels the recently added
transitioncancel event and allows authors to know that they should not
keep waiting for an animationend event.

> Bug:
https://bugzilla.mozilla.org/show_bug.cgi?id=1302648

> Link to standard:
https://drafts.csswg.org/css-animations/#eventdef-animationevent-animationcancel

> Platform coverage:
All platforms

> Estimated or target release:
Firefox 54

> Preference behind which this will be implemented:
No pref

> DevTools bug:
No special DevTools handling required

> Do other browser engines implement this?
No.
Blink has a bug on file for implementing transitioncancel[1] which
is marked "Hotlist-Interop" and we expect they will implement
animationcancel at roughly same time.
[1] https://bugs.chromium.org/p/chromium/issues/detail?id=642487

> Tests:
The work-in-progress patches include tests in web-platform-tests
(testharness.js) format. However, we are still waiting for the CSSWG to
merge their test repository into web-platform-tests before submitting
them there.
Comment on attachment 8832799 [details]
Bug 1302648 part 9 - Add test of setting target effect of CSS Animations.

https://reviewboard.mozilla.org/r/109072/#review111870

Sorry, I misread your comment.
And I forgot to return the Promise object in previous patch.
Comment on attachment 8832795 [details]
Bug 1302648 part 5 - Queue animationcancel when animation status is idle.

https://reviewboard.mozilla.org/r/109064/#review112208

r=me with comment addressed

::: layout/style/AnimationCommon.h:257
(Diff revision 3)
>  }
>  
> +// Return the TransitionPhase or AnimationPhase to use when the animation
> +// doesn't have a target effect.
> +template <typename PhaseType>
> +PhaseType GetAnimationPhaseWithoutEffect(const dom::Animation* aAnimation)

Let's make this a reference instead of a pointer so we don't need to handle the case where it might be null (or assert that it's not).

::: layout/style/nsAnimationManager.h:164
(Diff revision 3)
>    }
>    // True for animations that are generated from CSS markup and continue to
>    // reflect changes to that markup.
>    bool IsTiedToMarkup() const { return mOwningElement.IsSet(); }
>  
> +  void MaybeQueueCancelEvent(StickyTimeDuration aActiveTime) override {

(We probably should make this a constant reference. That might save an extra copy. However we'd need to do that here, in Animation.h and in nsTransitionManager.h. Mind doing that in a separate patch? Feel free to land without that however.)
Attachment #8832795 - Flags: review?(bbirtles) → review+
Comment on attachment 8832799 [details]
Bug 1302648 part 9 - Add test of setting target effect of CSS Animations.

https://reviewboard.mozilla.org/r/109072/#review112224

r=me with that one nit fixed (which was my mistake to begin with)

::: dom/animation/test/css-animations/file_setting-effect.html:105
(Diff revision 4)
> +}, 'The event is dispatched at the original element even if set an effect ' +
> +   'with a different target element');

s/even if set/even after setting/
Attachment #8832799 - Flags: review?(bbirtles) → review+
Don't forget to send the "intent to implement and ship" before landing this!
See Also: → 1338069
Comment on attachment 8832795 [details]
Bug 1302648 part 5 - Queue animationcancel when animation status is idle.

https://reviewboard.mozilla.org/r/109064/#review112208

Thanks Brian.
Pushed by mantaroh@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/7c0b9537f2db
part 1 - Add animationcancel event handler. r=masayuki
https://hg.mozilla.org/integration/autoland/rev/4d04fda475d2
part 2 - Add onanimationcancel EventHandler to WebIDL. r=baku
https://hg.mozilla.org/integration/autoland/rev/f47d195f4b81
part 3 - Change order of releasing owning element when cancel animation. r=birtles
https://hg.mozilla.org/integration/autoland/rev/c997f8314875
part 4 - Call UpdateTiming() after removing the animation from the timeline. r=birtles
https://hg.mozilla.org/integration/autoland/rev/e641ee639469
part 5 - Queue animationcancel when animation status is idle. r=birtles
https://hg.mozilla.org/integration/autoland/rev/2a84d79d87b4
part 6 - Add animationcancel test to legacy tests. r=birtles
https://hg.mozilla.org/integration/autoland/rev/47cc753e2e6a
part 7 - More simple animation event dispatch tests. r=birtles
https://hg.mozilla.org/integration/autoland/rev/a3e7552cee76
part 8 - Add animationcancel tests. r=birtles
https://hg.mozilla.org/integration/autoland/rev/714d689cabbf
part 9 - Add test of setting target effect of CSS Animations. r=birtles
You need to log in before you can comment on or make changes to this bug.