Fire transitioncancel event when a transition is canceled

RESOLVED FIXED in Firefox 53

Status

()

defect
RESOLVED FIXED
3 years ago
4 months ago

People

(Reporter: mozilla, Assigned: mantaroh)

Tracking

({dev-doc-complete, testcase})

Trunk
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 affected, firefox53 fixed)

Details

()

Attachments

(11 attachments, 1 obsolete attachment)

58 bytes, text/x-review-board-request
masayuki
: review+
Details
58 bytes, text/x-review-board-request
birtles
: review+
Details
58 bytes, text/x-review-board-request
birtles
: review+
Details
58 bytes, text/x-review-board-request
birtles
: review+
Details
58 bytes, text/x-review-board-request
birtles
: review+
Details
58 bytes, text/x-review-board-request
baku
: review+
Details
58 bytes, text/x-review-board-request
birtles
: review+
Details
58 bytes, text/x-review-board-request
birtles
: review+
Details
58 bytes, text/x-review-board-request
birtles
: review+
Details
1.64 KB, patch
mantaroh
: review+
Details | Diff | Splinter Review
1.65 KB, patch
mantaroh
: review+
Details | Diff | Splinter Review
See https://drafts.csswg.org/css-transitions-2/#eventdef-transitionevent-transitioncancel

Testcase: https://github.com/w3c/csswg-test/blob/master/css-transitions-2/transitioncancel-001.html

Without transitioncancel, scripts which are waiting for the end (whether normal or canceled early) of a CSS transition have to resort to using setTimeout (or similar) to ensure that their callbacks still get called even if the transition gets canceled (e.g. due to a parent getting styled as display:none during the transition). This extra complexity is annoying to authors, and frameworks often include such setTimeout-based workarounds (see comments 9 & 10 on https://bugs.chromium.org/p/chromium/issues/detail?id=437860 ).
Posted patch (WIP)Rough_Implementation (obsolete) — Splinter Review
(This patch is rough investigation code. So I need to rewrite this.)

I'm investigating this event handler.

Perhaps, We will need to concern about the following point:
 - An animation will remove RefreshObserver after canceling animation.
    We will call Animation::CancelFromStyle via nsTransitionManager.[1]
    In Animation::CancelFromStyle will remove RefreshObserver.
    Even if add transitioncancel to the Event Queue, we can't dispatch this event after animation cancel.
    So we will need to add refresh driver observer once in this case.

[1] https://dxr.mozilla.org/mozilla-central/rev/66a77b9bfe5dcacd50eccf85de7c0e7e15ce0ffd/layout/style/nsTransitionManager.cpp#969
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #1)
> Created attachment 8795608 [details] [diff] [review]
> (WIP)Rough_Implementation
> 
> (This patch is rough investigation code. So I need to rewrite this.)
> 
> I'm investigating this event handler.
> 
> Perhaps, We will need to concern about the following point:
>  - An animation will remove RefreshObserver after canceling animation.
>     We will call Animation::CancelFromStyle via nsTransitionManager.[1]
>     In Animation::CancelFromStyle will remove RefreshObserver.
>     Even if add transitioncancel to the Event Queue, we can't dispatch this
> event after animation cancel.
>     So we will need to add refresh driver observer once in this case.
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/rev/
> 66a77b9bfe5dcacd50eccf85de7c0e7e15ce0ffd/layout/style/nsTransitionManager.
> cpp#969
I created this patch, but this code related with bug 1287983. So I request review after land bug 1287983.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b7cc75239ba7a928fb34448df4f038847abdc830
Assignee: nobody → mantaroh
Attachment #8795608 - Attachment is obsolete: true
I think we also need to fix bug 1302644 at least for transition before landing these patches.  I mean that we need to know when cancel event is fired and when it's not.
Comment on attachment 8803218 [details]
Bug 1264125 part 1 - Add transitioncancel event handler.

https://reviewboard.mozilla.org/r/87524/#review86536

Otherwise, looks okay to me.

::: dom/base/nsGkAtomList.h:940
(Diff revision 1)
>  GK_ATOM(ontransitionend, "ontransitionend")
>  GK_ATOM(ontransitionrun, "ontransitionrun")
>  GK_ATOM(ontransitionstart, "ontransitionstart")
> +GK_ATOM(ontransitioncancel, "ontransitioncancel")

Please move "ontransitioncancel" to before  "ontransitionend" for keeping AtoZ order of the definition list.
Attachment #8803218 - Flags: review?(masayuki) → review+
Comment on attachment 8803218 [details]
Bug 1264125 part 1 - Add transitioncancel event handler.

https://reviewboard.mozilla.org/r/87524/#review86536

> Please move "ontransitioncancel" to before  "ontransitionend" for keeping AtoZ order of the definition list.

I fixed this patch, and separated previous patch for DOM Peer review.
Comment on attachment 8805017 [details]
Bug 1264125 part 2 - Add ontransitioncancel EventHandler to WebIDL.

https://reviewboard.mozilla.org/r/88842/#review88530
Attachment #8805017 - Flags: review?(amarchesini) → review+
Comment on attachment 8803219 [details]
Bug 1264125 part 3 - Add member of active time to ComputedTiming.

https://reviewboard.mozilla.org/r/87526/#review91426

r=me with the following changes

::: dom/animation/Animation.cpp:24
(Diff revision 2)
> +typedef mozilla::ComputedTiming::AnimationPhase AnimationPhase;
> +

Is this necessary? It doesn't seem to be used in this patch.

::: dom/animation/ComputedTiming.h:75
(Diff revision 2)
> +  // Active time is based on local time and 'start delay' to calculate
> +  // the overall progress. And it will use for calculating elapsed time of
> +  // event of cancelling CSS-Animation and CSS-Transition.
> +  StickyTimeDuration mActiveTime;

Let's move this up to just after mActiveDuration (to match the order in the spec). Then perhaps the comment should just be:

  // The time within the active interval.
Attachment #8803219 - Flags: review?(bbirtles) → review+
Comment on attachment 8803220 [details]
Bug 1264125 part 4 - Queue transitioncancel when animation status is idle.

https://reviewboard.mozilla.org/r/87528/#review91430

As per my comment below, I don't think this will work for transitions where we clear the timeline. As a result, I think we might need to try a different approach so I'm cancelling this review request for now.

::: layout/style/nsTransitionManager.cpp:405
(Diff revision 2)
> +void
> +CSSTransition::RequestTickOnce()
> +{
> +  if (mTimeline) {
> +    mTimeline->NotifyAnimationUpdated(*this);
> +  }
> +}
> +

This doesn't seem right. If we clear a transition's timeline and cancel it, we should still get the event, right?

I think there are two things we could possibly do.

a. Change the spec to make transitioncancel events be queued immediately.

b. Find some way to dispatch cancel events for transitions without a timeline on the next tick (if necessary).

(a) seems easier but I think it has a problem: if we queue transitioncancel events immediately but *not* transitionrun events, then we could have multiple transitioncancel events within a frame and no corresponding transitionrun events. That seems wrong.

We *could* make transitionrun events be dispatched immediately but that seems sub-optimal since that will likely produce a lot of unnecessary events.

So I think that leaves us with (b). I think we need to store some state on the transition manager for transitions that have been cancelled. We could probably store either:

* The event information, i.e. basically TransitionEventInfo AND some sort of weak pointer to the CSSTransition so we can test if it is still idle when we go to dispatch events, OR
* A strong reference to the CSSTransition and then we can generate all the event information on the next tick, if needed

I suspect we'll need to do the latter even though that will mean we need to hook that up to cycle collection. With the former, we probably need to hook up to cycle collection anyway since I think we need a reference to the owning element.

It's not a great arrangement, but that's the best I can think of at the moment. We'd end up creating/dropping the event in TransitionManager::SortEvents I guess (perhaps we'd rename it to PrepareEvents?)
Attachment #8803220 - Flags: review?(bbirtles)
Comment on attachment 8803221 [details]
Bug 1264125 part 7 - Update legacy transition event tests.

https://reviewboard.mozilla.org/r/87530/#review91142

r=me with the following changes

Nit: s/envet/event/ in commit message

::: layout/style/test/test_animations_event_handler_attribute.html:91
(Diff revision 2)
>  advance_clock(200);
>  checkReceivedEvents("animationend", targets);
>  
>  targets.forEach(div => { div.remove(); });
>  
> -// 2. Test CSS Transition event handlers.
> +// 2a. Test CSS Transition event handlers. (without transitioncancel)

Nit: either drop the '.' here or put it after the ')'

::: layout/style/test/test_animations_event_handler_attribute.html:93
(Diff revision 2)
>  var targets = createAndRegisterTargets([ 'ontransitionrun',
>                                           'ontransitionstart',
>                                           'ontransitionend' ]);

We should probably add ontransitioncancel to this list so we can detect an unexpected cancel event?

::: layout/style/test/test_animations_event_handler_attribute.html:116
(Diff revision 2)
> +var targets = createAndRegisterTargets([ 'ontransitionrun',
> +                                         'ontransitionstart',
> +                                         'ontransitionend',
> +                                         'ontransitioncancel' ]);
> +targets.forEach(div => {
> +  div.style.transition = 'margin-left 100ms 200ms';
> +  getComputedStyle(div).marginLeft; // flush
> +  div.style.marginLeft = "200px";
> +  getComputedStyle(div).marginLeft; // flush
> +});
> +
> +advance_clock(0);
> +checkReceivedEvents("transitionrun", targets);
> +
> +advance_clock(200);
> +checkReceivedEvents("transitionstart", targets);

Do you think we should make this simpler by *not* registering for transitionrun/transitionstart and just testing for transitioncancel?

(We should still register for transitionend to ensure we don't get one of those in error.)

::: layout/style/test/test_animations_event_order.html:648
(Diff revision 2)
> +
> +advance_clock(0);
> +advance_clock(5 * 1000);
> +divs.forEach(div => {
> +  div.style.display = "none";
> +  getComputedStyle(div).display; // flush

Drop 'flush' here (we don't write that anywhere else in this file).

In fact, we should just mirror the style earlier in this test:

  divs.forEach(div => div.style.display = 'none');
  getComputedStyle(divs[0]).display;

::: layout/style/test/test_animations_event_order.html:650
(Diff revision 2)
> +advance_clock(5 * 1000);
> +divs.forEach(div => {
> +  div.style.display = "none";
> +  getComputedStyle(div).display; // flush
> +});
> +advance_clock(10*1000);

Nit: space before and after '*'

::: layout/style/test/test_animations_event_order.html:658
(Diff revision 2)
> +                [ divs[1], 'transitionrun' ],
> +                [ divs[1], 'transitionstart' ],
> +                [ divs[0], 'transitionstart' ],
> +                [ divs[0], 'transitioncancel' ],
> +                [ divs[1], 'transitioncancel' ],
> +                'Simultaneous transitionrun/start/cancel');

Nit: Simultaneous transitionrun/start/cancel on siblings
Attachment #8803221 - Flags: review?(bbirtles) → review+
Comment on attachment 8803222 [details]
Bug 1264125 part 8 - Add transitioncancel tests.

https://reviewboard.mozilla.org/r/87532/#review91458

This is good but misses some important tests.

For a start this needs tests for when we set the animation's timeline to null.

This is tricky because currently setting an animation's timeline to null cancels it. I'm not sure if that will continue to be the case. We might make changes to the "set the timeline of an animation" procedure based on what we're learning from scroll-based timelines. In that case, any tests we add will probably need to change.

For now I think we could add the following tests:

a) Set the timeline to null -> cancel event on next tick
b) Cancel the transition (using display:none) -> cancel event on next tick
c) Set the timeline to null, then call cancel() on the transition -> still just a single cancel event on next tick
d) Cancel the transition (using display:none) then restart it using the API -> no event
e) Cancel the transition (using display:none), restart it using the API, then call cancel() on it -> single cancel event on next tick
f) Set the timeline to null, then set it back again and call play() -> no event

Something like that?

We should also add a test for each way of cancelling a transition:

1) With display:none on the element [covered already?]
2) With display:none on an ancestor
3) By changing the transition property to something else (i.e. replacing it with a new transition)
4) By setting transition-duration to 0 (with transition-delay = 0)
5) By setting transition-delay to minus transition-duration (i.e. make the combined duration zero using the delay)
6) By setting transition-property property to something that doesn't include the property being transitioned
7) By setting the transition property to its current computed style value?
8) By setting the transition property to something that can't be interpolated [Not sure if such cases will exist after Daisuke's patches land]

Also, I think we need a test that cancelling a finished transition (using one of the methods above, e.g. 6), does NOT fire a cancel event.

::: dom/animation/test/css-transitions/file_csstransition-events.html:106
(Diff revision 2)
>  promise_test(function(t) {
>    var [transition, watcher, handler] = setupTransition(t);
>  
>    return Promise.all([ watcher.wait_for('transitionrun'),
>                         transition.ready ]).then(function() {
> +    // Seek to Idle

Nit: 'Seeking' is about changing the playback position. Perhaps, 'Cancel transition' (or 'Make idle').

::: dom/animation/test/css-transitions/file_csstransition-events.html:107
(Diff revision 2)
>    var [transition, watcher, handler] = setupTransition(t);
>  
>    return Promise.all([ watcher.wait_for('transitionrun'),
>                         transition.ready ]).then(function() {
> +    // Seek to Idle
> +    handler.target.style.display = 'none';

Should we just return the target element as the last element in the array from setupTransition? Using 'handler.target' seems a bit odd?

::: dom/animation/test/css-transitions/file_csstransition-events.html:109
(Diff revision 2)
>    return Promise.all([ watcher.wait_for('transitionrun'),
>                         transition.ready ]).then(function() {
> +    // Seek to Idle
> +    handler.target.style.display = 'none';
> +    flushComputedStyle(handler.target);
> +

Nit: I think we don't need this blank line

::: dom/animation/test/css-transitions/file_csstransition-events.html:147
(Diff revision 2)
> +  var [transition, watcher, handler] =
> +    setupTransition(t, 'transition: margin-left 100s');
> +
> +  // Seek to Active start position.
> +  transition.pause();
> +  transition.currentTime = 0;

Do we need to set the currentTime here?

::: dom/animation/test/css-transitions/file_csstransition-events.html:151
(Diff revision 2)
> +  transition.pause();
> +  transition.currentTime = 0;
> +
> +  return watcher.wait_for([ 'transitionrun',
> +                            'transitionstart' ]).then(function(evt) {
> +    // Seek to Idle

'Cancel transition' / 'Make idle'

::: dom/animation/test/css-transitions/file_csstransition-events.html:161
(Diff revision 2)
> +    assert_equals(evt.elapsedTime, 0.0);
> +  });
> +}, 'Active -> Idle, no delay');
> +
> +promise_test(function(t) {
> +  var [transition, watcher, handler] = setupTransition(t);

I think I made the same comment regarding the tests for CSS animations, but these tests would be a lot easier to read if we always pass the transition property to setupTransition.

i.e. we should drop the part that does:

  transitionStyle = transitionStyle || 'transition: margin-left 100s 100s';

::: dom/animation/test/css-transitions/file_csstransition-events.html:163
(Diff revision 2)
> +}, 'Active -> Idle, no delay');
> +
> +promise_test(function(t) {
> +  var [transition, watcher, handler] = setupTransition(t);
> +  // Seek to Active phase.
> +  transition.pause();  // This need for cancel elapsed time.

'Pause so the currentTime is fixed and we can accurately compare the event time in transitioncancel events.'

::: dom/animation/test/css-transitions/file_csstransition-events.html:167
(Diff revision 2)
> +  // Seek to Active phase.
> +  transition.pause();  // This need for cancel elapsed time.
> +  transition.currentTime = 100 * MS_PER_SEC;
> +  return watcher.wait_for([ 'transitionrun',
> +                            'transitionstart' ]).then(function(evt) {
> +    // Seek to Idle

'Cancel transition' / 'Make idle'

::: dom/animation/test/css-transitions/file_csstransition-events.html:182
(Diff revision 2)
> +  var [transition, watcher, handler] =
> +    setupTransition(t, 'transition: margin-left 100s -50s');
> +
> +  // Seek to Active start position
> +  transition.pause();
> +  transition.currentTime = 0;

Is setting currentTime required?

::: dom/animation/test/css-transitions/file_csstransition-events.html:186
(Diff revision 2)
> +  transition.pause();
> +  transition.currentTime = 0;
> +
> +  return watcher.wait_for([ 'transitionrun',
> +                            'transitionstart' ]).then(function(evt) {
> +    // Seek to Idle

'Cancel transition' / 'Make idle'
Attachment #8803222 - Flags: review?(bbirtles)
Regarding part 4, one idea for how we could implement it is:

* Extend PendingAnimationTracker to include an additional set of "cancel pending transitions".
  We might want to use a different type to AnimationSet that stores CSSTransitions instead to save some complexity, e.g. CSSTransitionSet.
* When a CSSTransition is cancelled we put it in the PendingAnimationTracker's set of cancel pending transitions.
* If we are played again we need to remove ourselves from the PendingAnimationTracker. That should be cheap, though, since it's just a hashtable lookup.
* On a tick, we queue events as normal by ticking animations attached to timelines. We *don't* include cancel events in this (i.e. QueueEvents goes unchanged).
* After that, and before calling nsTransitionManager::SortEvents, we call something else on nsTransitionManager (QueueCancelEvents) that:
  * Looks up the corresponding
(In reply to Brian Birtles (:birtles) from comment #23)
...
>   * Looks up the corresponding

Oops, that got submitted while I was still typing. I must have touched the trackpad.

Continuing...
    * Looks up the corresponding PendingAnimationTracker
    * Iterates over all the transitions there. If they are still idle, queues a cancel event for them.
    * Clears the table.
* Then after that we sort the events as usual.

Where it gets tricky is when we lose touch with the PendingAnimationTracker or get associated with a different document, but maybe the above works? I can't think of a test case yet that doesn't but I'm sure there are some.
Thanks, Brian.

(In reply to Brian Birtles (:birtles) from comment #23)
> Regarding part 4, one idea for how we could implement it is:
> 
> * Extend PendingAnimationTracker to include an additional set of "cancel
> pending transitions".
>   We might want to use a different type to AnimationSet that stores
> CSSTransitions instead to save some complexity, e.g. CSSTransitionSet.
> * When a CSSTransition is cancelled we put it in the
> PendingAnimationTracker's set of cancel pending transitions.
> * If we are played again we need to remove ourselves from the
> PendingAnimationTracker. That should be cheap, though, since it's just a
> hashtable lookup.
> * On a tick, we queue events as normal by ticking animations attached to
> timelines. We *don't* include cancel events in this (i.e. QueueEvents goes
> unchanged).
> * After that, and before calling nsTransitionManager::SortEvents, we call
> something else on nsTransitionManager (QueueCancelEvents) that:
I tried your suggestion implementation, then I think that we can't calculate event's elapsed time.
The effect of transition was deleted when calling the SortEvent, So we can't get the effect of transition for calculating elapsed time.
As result of it, we couldn't queue event of cancel.

Perhaps we will need to drop the cancel event in SortEvents.
How do you feel this implementation?
Flags: needinfo?(bbirtles)
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #25)
> Thanks, Brian.
> 
> (In reply to Brian Birtles (:birtles) from comment #23)
> > Regarding part 4, one idea for how we could implement it is:
> > 
> > * Extend PendingAnimationTracker to include an additional set of "cancel
> > pending transitions".
> >   We might want to use a different type to AnimationSet that stores
> > CSSTransitions instead to save some complexity, e.g. CSSTransitionSet.
> > * When a CSSTransition is cancelled we put it in the
> > PendingAnimationTracker's set of cancel pending transitions.
> > * If we are played again we need to remove ourselves from the
> > PendingAnimationTracker. That should be cheap, though, since it's just a
> > hashtable lookup.
> > * On a tick, we queue events as normal by ticking animations attached to
> > timelines. We *don't* include cancel events in this (i.e. QueueEvents goes
> > unchanged).
> > * After that, and before calling nsTransitionManager::SortEvents, we call
> > something else on nsTransitionManager (QueueCancelEvents) that:
> I tried your suggestion implementation, then I think that we can't calculate
> event's elapsed time.

Did you see comment 20? We probably need to store the active time along with a strong reference to the CSS transition and hook it up to the cycle collector.
Flags: needinfo?(bbirtles)
(In reply to Brian Birtles (:birtles) from comment #22)
> We should also add a test for each way of cancelling a transition:
> 
> 1) With display:none on the element [covered already?]
> 2) With display:none on an ancestor
> 3) By changing the transition property to something else (i.e. replacing it
> with a new transition)
> 4) By setting transition-duration to 0 (with transition-delay = 0)
> 5) By setting transition-delay to minus transition-duration (i.e. make the
> combined duration zero using the delay)
> 6) By setting transition-property property to something that doesn't include
> the property being transitioned
> 7) By setting the transition property to its current computed style value?
> 8) By setting the transition property to something that can't be
> interpolated [Not sure if such cases will exist after Daisuke's patches land]

While I was reading the spec, I noticed that we need one more case that the element is removed from the document.

From the spec:
 If an element is no longer in the document, implementations must cancel any running transitions on it
https://drafts.csswg.org/css-transitions/#starting
Comment on attachment 8803220 [details]
Bug 1264125 part 4 - Queue transitioncancel when animation status is idle.

https://reviewboard.mozilla.org/r/87528/#review91430

> This doesn't seem right. If we clear a transition's timeline and cancel it, we should still get the event, right?
> 
> I think there are two things we could possibly do.
> 
> a. Change the spec to make transitioncancel events be queued immediately.
> 
> b. Find some way to dispatch cancel events for transitions without a timeline on the next tick (if necessary).
> 
> (a) seems easier but I think it has a problem: if we queue transitioncancel events immediately but *not* transitionrun events, then we could have multiple transitioncancel events within a frame and no corresponding transitionrun events. That seems wrong.
> 
> We *could* make transitionrun events be dispatched immediately but that seems sub-optimal since that will likely produce a lot of unnecessary events.
> 
> So I think that leaves us with (b). I think we need to store some state on the transition manager for transitions that have been cancelled. We could probably store either:
> 
> * The event information, i.e. basically TransitionEventInfo AND some sort of weak pointer to the CSSTransition so we can test if it is still idle when we go to dispatch events, OR
> * A strong reference to the CSSTransition and then we can generate all the event information on the next tick, if needed
> 
> I suspect we'll need to do the latter even though that will mean we need to hook that up to cycle collection. With the former, we probably need to hook up to cycle collection anyway since I think we need a reference to the owning element.
> 
> It's not a great arrangement, but that's the best I can think of at the moment. We'd end up creating/dropping the event in TransitionManager::SortEvents I guess (perhaps we'd rename it to PrepareEvents?)

As discussed with Brian, we should queueing the cancel event immediately in this case. (This is (a) of Brian's suggestion.)
So this patch won't include the modification of PendingAnimationTracker or so..)

But we have the important timing issue in this fix.
It is that we can't sort the event after canceling the transition due to Owning element reference cycle.
We will destroy owning element reference when canceling the transition according to the spec.[1]

[1] https://drafts.csswg.org/css-transitions-2/#owning-element-section
A few notes from my discussion with Mantaroh:

* Firstly, my own reference, we decided to queue cancel events synchronously
  because otherwise we lose the reference to owning element (and the
  absence/presence of that is what tells us if we should dispatch events at
  all).

  Furthermore, if an animation is cancelled by clearing its timeline it
  won't be ticked so QueueEvents will never be called (and its not clear who
  could call it).

* For the case of a timeline that becomes inactive, we should probably queue
  a cancel event. We discussed that that might happen in an asynchronous way but
  actually I'm not sure anymore. This doesn't currently happen yet. I suspect
  that once it becomes possible, we'll actually have logic that runs when we
  detect an inactive timeline (we might even make timelines call their children
  when they become inactive)--and at that point we can queue cancel in the
  regular synchronous manner.

* If you have the following code:

    transition.effect = nullptr;

  Should we dispatch cancel? After some thought, I think we shouldn't.
  We still have an owning element and, after all, the transition wasn't
  cancelled just neutered. Also, for the (more likely) use case where you
  want to substitute in a different effect you probably don't want or expect
  cancel events:

    transition.effect = new KeyframeEffect(transition.effect);

I'll follow up with the other things we discussed as review comments.
Comment on attachment 8803220 [details]
Bug 1264125 part 4 - Queue transitioncancel when animation status is idle.

https://reviewboard.mozilla.org/r/87528/#review95876

Nit: in the commit message s/Queueing/Queue/

::: layout/style/nsTransitionManager.cpp:261
(Diff revision 3)
>      case TransitionPhase::Pending:
>      case TransitionPhase::Before:
> -      if (currentPhase == TransitionPhase::Active) {
> +      if (currentPhase == TransitionPhase::Idle) {
> +        events.AppendElement(TransitionEventParams{ eTransitionCancel,
> +                                                    activeTime,
> +                                                    activeTimeStamp });
> +      } else if (currentPhase == TransitionPhase::Active) {

As discussed, rather than repeating this bit of code three times, I think it might be simpler to just stick a separate if() before or after the switch statement (perhaps after--to match the spec?) that covers the cancel case.
Attachment #8803220 - Flags: review?(bbirtles)
Comment on attachment 8814762 [details]
Bug 1264125 part 5 - Call the queueing events when canceling transition via Style or Script.

https://reviewboard.mozilla.org/r/95852/#review96352

::: dom/animation/Animation.h:329
(Diff revision 1)
> +  /**
> +   * Queue CSS-Transitions or CSS-Animations event.
> +   * This function will call from CSSAnimationManager or CSSTransitionManager.
> +   */
> +  virtual void QueueEvents() {};

At first I thought we should name this MaybeQueueCancelEvent() but I guess it's just going to call QueueEvents anyway?

Maybe we can rename it to QueueCSSEvents (so we don't confuse it with dispatching regular animation events?) and drop the comment (since it is no longer only called from nsAnimationManager/nsTransitionManager).

Ideally we should do the renaming as a separate patch since a few places will need to be updated.

::: dom/animation/Animation.h:335
(Diff revision 1)
> +  virtual void SetCancelActiveTime(StickyTimeDuration aActiveTime) {
> +    mCancelActiveTime = aActiveTime;
> +  };

As discussed, I think we should see if we can re-use mPreviousCurrentTime for this.

::: dom/animation/Animation.h:458
(Diff revision 1)
> +
> +  // We will need to store active time since elapsedTime of CSS-Transitions or
> +  // CSS-Animations is based on active time.
> +  StickyTimeDuration mCancelActiveTime;

As discussed, with some luck we can probably drop this.

::: dom/animation/Animation.cpp:247
(Diff revision 1)
>    mTimeline = aTimeline;
>    if (!mStartTime.IsNull()) {
>      mHoldTime.SetNull();
>    }
>  
> +  QueueEvents();

Should we only do this if mTimeline is null?

::: dom/animation/Animation.cpp:831
(Diff revision 1)
>      if (thisTransition || otherTransition) {
> +      // FIXME: We will need to copy the owning element of transition
> +      // when queueing transitioncancel event, since we can't get
> +      // owning element after canceling the transition.
> +      // We can't sort the events by using owning element,
> +      // if we don't have these reference.
>        return thisTransition;
>      }

I'm not sure we need this comment. Perhaps just something like:

  // Cancelled transitions no longer have an owning element. To be
  // strictly correct we should store a strong reference to the owning element
  // element so that if we arrive here while sorting cancel events, we can
  // sort them in the correct order.
  //
  // However, given that cancel events are almost always queued synchronously
  // in some deterministic manner, we can be fairly sure that at cancel events
  // will be dispatched in a deterministic order (which is our only hard
  // requirement until specs say otherwise). Furthermore, we only reach here
  // when we have events with equal timestamps so this is an edge case we can
  // probably ignore for now.
Attachment #8814762 - Flags: review?(bbirtles)
Comment on attachment 8803222 [details]
Bug 1264125 part 8 - Add transitioncancel tests.

https://reviewboard.mozilla.org/r/87532/#review96380

I'm still reviewing this but here are my review comments so far.

Also, as discussed, I think we need to add tests for:
* Setting transition.effect to null (should generate transitionend NOT transitioncancel)
* Setting transition.effect to a new effect with a different target (should not generate any event initially but subsequent events should be dispatched to the *owning* element)

::: dom/animation/test/css-transitions/file_animation-cancel.html:9
(Diff revision 3)
>  <body>
>  <script>
>  'use strict';
>  
> -promise_test(function(t) {
> -  var div = addDiv(t, { style: 'margin-left: 0px' });
> +function setupTransition(t, transitionStyle) {
> +  var div, watcher, transition;

I'm not sure what our usual practice for JS is but for other languages we normally do one per declaration per line (and putting on them one line doesn't seem to help in this case).

::: dom/animation/test/css-transitions/file_animation-cancel.html:18
(Diff revision 3)
>    flushComputedStyle(div);
>  
> -  var animation = div.getAnimations()[0];
> +  transition = div.getAnimations()[0];

We don't need flushComputedStyle here, getAnimations() flushes style.

::: dom/animation/test/css-transitions/file_animation-cancel.html:39
(Diff revision 3)
>  promise_test(function(t) {
> -  var div = addDiv(t, { style: 'margin-left: 0px' });
> -  flushComputedStyle(div);
> +  var [transition, watcher, div] =
> +    setupTransition(t, 'margin-left 100s');
>  
> -  div.style.transition = 'margin-left 100s';
> -  div.style.marginLeft = '1000px';
> -  flushComputedStyle(div);
> -
> -  div.addEventListener('transitionend', function() {
> -    assert_unreached('Got unexpected end event on cancelled transition');
> +  return watcher.wait_for([ 'transitionrun',
> +                            'transitionstart' ]).then(function() {
> +    transition.finish();
> +    return watcher.wait_for('transitionend');
> +  }).then(function() {
> +    transition.cancel();
> -  });
> -
> -  var animation = div.getAnimations()[0];
> -  return animation.ready.then(function() {
> -    // Seek to just before the end then cancel
> -    animation.currentTime = 99.9 * 1000;
> -    animation.cancel();
>  
> -    // Then wait a couple of frames and check that no event was dispatched
> +    return watcher.wait_for('transitioncancel');
> -    return waitForAnimationFrames(2);
>    });
> -}, 'Cancelled CSS transitions do not dispatch events');
> +}, 'Cancelled CSS transitions after finishing the transition');

I think the setupTransition helper makes most of these tests simpler and easier to read but sometimes it makes it harder too, it seems.

For example, in this test it's not clear what we're actually testing. If we're testing that cancel events are still dispatched for a finished transition then it seems like we could write it more simply as:

promise_test(function(t) {
  var div = addDiv(t, { style: 'transition: margin-left 100s' });
  getComputedStyle(div).marginLeft;
  div.style.marginLeft = '1000px';
  var transition = div.getAnimations()[0];

  transition.finish();

  return transition.ready.then(function() {
    transition.cancel();
    var watcher = new EventWatcher(t, div, [ 'transitioncancel' ]);
    return watcher.wait_for('transitioncancel');
  });
}, 'transitioncancel events are dispatched when a finished transition is'
   + ' cancelled');

Perhaps we don't need to use the helper for every test?

::: dom/animation/test/mochitest.ini:31
(Diff revision 3)
> -  css-transitions/file_csstransition-events.html
> +  css-transitions/file_events-dispatch.html
>    css-transitions/file_csstransition-transitionproperty.html
>    css-transitions/file_document-get-animations.html
>    css-transitions/file_effect-target.html
>    css-transitions/file_element-get-animations.html

This doesn't appear to be in alphabetical order.

(Also, a very minor nit, but could we name this file_event-dispatch.html -- i.e. no 's'. The reason is just to match the heading in spec: 'Event dispatch')

::: dom/animation/test/mochitest.ini:85
(Diff revision 3)
> -[css-transitions/test_csstransition-events.html]
> +[css-transitions/test_events-dispatch.html]
>  [css-transitions/test_csstransition-transitionproperty.html]
>  [css-transitions/test_document-get-animations.html]
>  [css-transitions/test_effect-target.html]
>  [css-transitions/test_element-get-animations.html]
>  [css-transitions/test_keyframeeffect-getkeyframes.html]

Likewise this is not in alphabetical order.
Comment on attachment 8803222 [details]
Bug 1264125 part 8 - Add transitioncancel tests.

https://reviewboard.mozilla.org/r/87532/#review96648

In general this looks good although I haven't looked at all the tests in detail yet. I've left a number of comments here and in comment 40 but my high-level comment is that I think we can drop event watching from file_animation-cancel.html and test for cancelling behavior in other ways (either by checking that the transition's play state is idle or that oncancel is called--probably the former). That would make the tests simpler and more focused since we wouldn't be checking for various events that are basically unrelated to what we're testing.

If you agree, I'll have another look after we make that change.

::: dom/animation/test/css-transitions/file_animation-cancel.html:25
(Diff revision 3)
> +promise_test(function(t) {
> +  var [transition, watcher, div] =
> +    setupTransition(t, 'margin-left 100s');
> +
> +  return watcher.wait_for([ 'transitionrun',
> +                            'transitionstart' ]).then(function() {
>      assert_not_equals(getComputedStyle(div).marginLeft, '1000px',
>                        'transform style is animated before cancelling');
> -    animation.cancel();
> +    transition.cancel();
>      assert_equals(getComputedStyle(div).marginLeft, div.style.marginLeft,
>                    'transform style is no longer animated after cancelling');
>    });
>  }, 'Animated style is cleared after cancelling a running CSS transition');

I think testing for events here complicates this test and distracts from what it is testing.

I was thinking we could add a simple one-off waitForEvent() helper here (and add a version of setupTransition that doesn't create an EventWatcher) but after going through the other tests I think we probably don't need to test for events at all in this file. See comments below.

::: dom/animation/test/css-transitions/file_animation-cancel.html:54
(Diff revision 3)
>  promise_test(function(t) {
> -  var div = addDiv(t, { style: 'margin-left: 0px' });
> -  flushComputedStyle(div);
> +  var [transition, watcher, div] =
> +    setupTransition(t, 'margin-left 100s');
>  
> -  div.style.transition = 'margin-left 100s';
> -  div.style.marginLeft = '1000px';
> -  flushComputedStyle(div);
> +  return watcher.wait_for([ 'transitionrun',
> +                            'transitionstart' ]).then(function() {
> +    transition.cancel();
> -
> -  var animation = div.getAnimations()[0];
> -  return animation.ready.then(function() {
> -    animation.cancel();
>      assert_equals(getComputedStyle(div).marginLeft, '1000px',
>                    'margin-left style is not animated after cancelling');
> -    animation.play();
> +    transition.play();
>      assert_equals(getComputedStyle(div).marginLeft, '0px',
>                    'margin-left style is animated after re-starting transition');
> -    return animation.ready;
> +    return Promise.all([transition.ready,
> +                        watcher.wait_for(['transitioncancel',
> +                                          'transitionrun',
> +                                          'transitionstart'])]);
>    }).then(function() {
> -    assert_equals(animation.playState, 'running',
> +    assert_equals(transition.playState, 'running',
>                    'Transition succeeds in running after being re-started');
>    });
> -}, 'After cancelling a transition, it can still be re-used');
> +}, 'After canceling a transition, it can still be re-used');

Again, I feel like most of this would be a lot simpler without all the event watching (which seems orthogonal to what is actually being tested here).

::: dom/animation/test/css-transitions/file_animation-cancel.html:172
(Diff revision 3)
> +promise_test(function(t) {
> +  var [transition, watcher, div] =
> +    setupTransition(t, 'margin-left 100s');
> +
> +  return watcher.wait_for([ 'transitionrun',
> +                            'transitionstart' ]).then(function() {
> +    div.style.transitionDuration = '0s';
> +    div.style.transitionDelay = '0s';
> +    flushComputedStyle(div);
> +
> +    return waitForFrame();
> +  });
> +}, 'Setting zero duration');

What is this testing? That setting a zero duration cancels? If so, it should probably:

a) Say that in the test description
b) Check for a cancel event

Although, if we want to check that a transition is cancelled there are a few different things we can check:

i) That a transitioncancel event was filed
ii) That the CSSTransition object becomes idle
iii) That the CSSTransition object calls oncancel / has its finish promise rejected / etc.

Only (i) can be done without the Web Animations API but it seems like we're ok with using the Web Animations API in this file so perhaps we should just check the transition becomes idle?

Then we can test for events in the separate event-dispatch test instead.

::: dom/animation/test/css-transitions/file_events-dispatch.html:417
(Diff revision 3)
> +  var [transition, watcher, handler, div] =
> +    setupTransition(t, 'margin-left 100s');

I don't think we use handler or div here so perhaps we can drop them from the list?

(And on that note, it looks like we use |div| more than |handler| so we could put it earlier in the list and not list |handler| in those tests that don't use it?)
Attachment #8803222 - Flags: review?(bbirtles)
Thanks Brian,

(In reply to Brian Birtles (:birtles) from comment #37)
> * If you have the following code:
> 
>     transition.effect = nullptr;
Currently, The transition is ignore the condition of Non-Effect. [1]
So, We can't queue the end event, even if Animation will idle phase due to clear the effect.

[1] https://dxr.mozilla.org/mozilla-central/rev/741a720c98cdb92c229376be0badbf036f653bff/layout/style/nsTransitionManager.cpp#185

We will need to special handling in this case.
Comment on attachment 8814762 [details]
Bug 1264125 part 5 - Call the queueing events when canceling transition via Style or Script.

https://reviewboard.mozilla.org/r/95852/#review96352

> At first I thought we should name this MaybeQueueCancelEvent() but I guess it's just going to call QueueEvents anyway?
> 
> Maybe we can rename it to QueueCSSEvents (so we don't confuse it with dispatching regular animation events?) and drop the comment (since it is no longer only called from nsAnimationManager/nsTransitionManager).
> 
> Ideally we should do the renaming as a separate patch since a few places will need to be updated.

I added the QueueCSSEvents instead calling QueueEvents directly.
Comment on attachment 8803222 [details]
Bug 1264125 part 8 - Add transitioncancel tests.

https://reviewboard.mozilla.org/r/87532/#review96380

Thanks,

I added setting target effect tests to file_setting_effect.html.
Comment on attachment 8803222 [details]
Bug 1264125 part 8 - Add transitioncancel tests.

https://reviewboard.mozilla.org/r/87532/#review96648

I droped event watching on this tests.
However We need to watching transitioncancel event little. e.g. detect canceling transition.

> What is this testing? That setting a zero duration cancels? If so, it should probably:
> 
> a) Say that in the test description
> b) Check for a cancel event
> 
> Although, if we want to check that a transition is cancelled there are a few different things we can check:
> 
> i) That a transitioncancel event was filed
> ii) That the CSSTransition object becomes idle
> iii) That the CSSTransition object calls oncancel / has its finish promise rejected / etc.
> 
> Only (i) can be done without the Web Animations API but it seems like we're ok with using the Web Animations API in this file so perhaps we should just check the transition becomes idle?
> 
> Then we can test for events in the separate event-dispatch test instead.

Umm, This is my misreading the specification. I think that we won't need this tests, so I dropped this one.

> I don't think we use handler or div here so perhaps we can drop them from the list?
> 
> (And on that note, it looks like we use |div| more than |handler| so we could put it earlier in the list and not list |handler| in those tests that don't use it?)

This handler used by only few tests, so I remove this handler parameter on this function for readability of tests
Comment on attachment 8803222 [details]
Bug 1264125 part 8 - Add transitioncancel tests.

https://reviewboard.mozilla.org/r/87532/#review98318

As discussed, cancelling this review for now since I think we should:

* Do renaming s/animation/transition/ in a separate patch
* Rather than mixing testing of events into the existing animation-cancel tests (which are already complicated because they test *both* playState and computedStyle) -- we should just test transitioncancel events in the event-dispatch test. If those tests already overlap with the animation-cancel ones then we should probably just drop the ones in animation-cancel.

I left a few other comments below, but they're probably irrelevant in light of the above comments.

::: dom/animation/test/css-transitions/file_animation-cancel.html:28
(Diff revision 4)
>  }, 'Animated style is cleared after cancelling a running CSS transition');
>  
>  promise_test(function(t) {
>    var div = addDiv(t, { style: 'margin-left: 0px' });
> +  var watcher = new EventWatcher(t, div, 'transitioncancel');
> +  div.ontransitioncancel = function(evt) { console.log('cancel'); };

I don't think we want this line?

::: dom/animation/test/css-transitions/file_animation-cancel.html:37
(Diff revision 4)
> -    assert_unreached('Got unexpected end event on cancelled transition');
> -  });
>  
> -  var animation = div.getAnimations()[0];
> -  return animation.ready.then(function() {
> -    // Seek to just before the end then cancel
> +  // Make Idle.
> +  transition.finish();
> +  return transition.finished.then(waitForFrame).then(function() {

I don't think it makes sense to wait on finished here. We called finish() so it is already finished. The point of waiting on |ready| (in comment 40) was to make sure we pass a tick while being finished. We should probably just use 'return waitForFrame()' here I guess.

::: dom/animation/test/css-transitions/file_animation-cancel.html:38
(Diff revision 4)
> -    animation.currentTime = 99.9 * 1000;
> -    animation.cancel();
> -
> +    var retPromise = watcher.wait_for('transitioncancel');
> +    transition.cancel();
> +    return retPromise;

I'm curious, did the following (from comment 40) not work here?

    transition.cancel();
    var watcher = new EventWatcher(t, div, [ 'transitioncancel' ]);
    return watcher.wait_for('transitioncancel');

I would have thought that cancel would queue the event, but not dispatch it so that we could register the event listener afterwards and still catch it?
Attachment #8803222 - Flags: review?(bbirtles)
Comment on attachment 8803220 [details]
Bug 1264125 part 4 - Queue transitioncancel when animation status is idle.

https://reviewboard.mozilla.org/r/87528/#review98322

r=me with the following changes made

::: layout/style/nsTransitionManager.cpp:209
(Diff revision 4)
> +  // FIXME: bug 1264125: We will need to get active time when cancelling
> +  //                     the transition.
> +  StickyTimeDuration activeTime(0);
> +
>    // TimeStamps to use for ordering the events when they are dispatched. We
>    // use a TimeStamp so we can compare events produced by different elements,
>    // perhaps even with different timelines.
>    // The zero timestamp is for transitionrun events where we ignore the delay
>    // for the purpose of ordering events.
> -  TimeStamp zeroTimeStamp  = AnimationTimeToTimeStamp(zeroDuration);
> -  TimeStamp startTimeStamp = ElapsedTimeToTimeStamp(intervalStartTime);
> -  TimeStamp endTimeStamp   = ElapsedTimeToTimeStamp(intervalEndTime);
> +  TimeStamp zeroTimeStamp   = AnimationTimeToTimeStamp(zeroDuration);
> +  TimeStamp startTimeStamp  = ElapsedTimeToTimeStamp(intervalStartTime);
> +  TimeStamp endTimeStamp    = ElapsedTimeToTimeStamp(intervalEndTime);
> +  TimeStamp activeTimeStamp = ElapsedTimeToTimeStamp(activeTime);

I think we should move this inside the branch below.

::: layout/style/nsTransitionManager.cpp:234
(Diff revision 4)
> +  if ((mPreviousTransitionPhase != TransitionPhase::Idle) &&
> +      (currentPhase == TransitionPhase::Idle)) {
> +        events.AppendElement(TransitionEventParams{ eTransitionCancel,
> +                                                    activeTime,
> +                                                    activeTimeStamp });
> +  }

I think it would be better to but a blank line between this and the switch below (otherwise it's hard to see that they are two different control structures). And perhaps add a comment, e.g.

  AutoTArray<TransitionEventParams, 3> events;

  // Handle cancel events first
  if (mPreviousTransitionPhase != TransitionPhase::Idle &&
      currentPhase == TransitionPhase::Idle) {
    TimeStamp activeTimeStamp = ElapsedTimeToTimeStamp(activeTime);
    events.AppendElement(TransitionEventParams{ eTransitionCancel,
                                                activeTime,
                                                activeTimeStamp });
  }

  // All other events
  switch (mPreviousTransitionPhase) {

::: layout/style/nsTransitionManager.cpp:234
(Diff revision 4)
> +  if ((mPreviousTransitionPhase != TransitionPhase::Idle) &&
> +      (currentPhase == TransitionPhase::Idle)) {

The () around each of the conditions is not necessary here.

::: layout/style/nsTransitionManager.cpp:236
(Diff revision 4)
> +        events.AppendElement(TransitionEventParams{ eTransitionCancel,
> +                                                    activeTime,
> +                                                    activeTimeStamp });

The indentation here appears wrong.
Attachment #8803220 - Flags: review?(bbirtles) → review+
Comment on attachment 8814762 [details]
Bug 1264125 part 5 - Call the queueing events when canceling transition via Style or Script.

https://reviewboard.mozilla.org/r/95852/#review98324

r=me with the following changes made

::: dom/animation/Animation.h:329
(Diff revision 2)
>    void ComposeStyle(RefPtr<AnimValuesStyleRule>& aStyleRule,
>                      const nsCSSPropertyIDSet& aPropertiesToSkip);
>  
>    void NotifyEffectTimingUpdated();
>  
> +  virtual void QueueCSSEventsNow(StickyTimeDuration activeTime) {};

Let's call this MaybeQueueCancelEvent() and add a comment:

"Used by subclasses to synchronously queue a cancel event in situations where the Animation may have been cancelled.

We need to do this synchronously because after a CSS animation/transition is canceled, it will be released by its owning element and may not still exist when we would normally go to queue events on the next tick."

Also, s/activeTime/aActiveTime/

::: dom/animation/Animation.cpp:233
(Diff revision 2)
> +  ComputedTiming timing;
> +  if (mEffect) {
> +    timing = mEffect->GetComputedTiming();
> +  }

Since we only actually *need* the active time, would it be simpler to just store that?

e.g.

  StickyTimeDuration activeTime = mEffect
                                  ? mEffect->GetComputedTiming().mActiveTime
                                  : StickyTimeDuration();

::: dom/animation/Animation.cpp:782
(Diff revision 2)
> +  ComputedTiming timing;
> +  if (mEffect) {
> +    timing = mEffect->GetComputedTiming();
> +  }

Likewise here, it might be clearer if we just store the activeTime.

::: dom/animation/Animation.cpp:836
(Diff revision 2)
> +      // Cancelled transitions no long have an owning element. To be strictly
> +      // correct we should store a strong reference to the owning element
> +      // so that if we arrive here while sorting cancel events, we can sort
> +      // them in the correct order.
> +      //
> +      // However, given that cancel events are almost always queued
> +      // synchronously in some deterministic manner, we can be fairly sure
> +      // that at cancel event will be dispatched in a deterministic order
> +      // (which is our only hard requirement until spec say otherwise).
> +      // Furthermore, we only reach here when we have events with equal
> +      // timestamp so this is an edge case we can probably ignore for now.

A few typos here (some mine, some copy-and-paste errors):

s/no long/no longer/
s/that at cancel event/that cancel events/
s/until spec/until specs/
s/equal timestamp/equal timestamps/

::: layout/style/nsTransitionManager.h:165
(Diff revision 2)
> +    // The transition will queue the transition cancel when restyling.
> +    // However we removed the unregist the refresh observer after calling
> +    // Animation::CancelFromStyle().
> +    // So we will need to add the refresh observer for transitioncancel.
> +    // This refresh observer will remove when calling
> +    // DocumentTimeline::WillRefresh().

"The above call to Animation::CancelFromStyle may cause a transitioncancel event to be queued. However, it will also remove the transition from its timeline. If this transition was the last animation attached to the timeline, the timeline will stop observing the refresh driver and there may be no subsequent tick for dispatching animation events.

To ensure the cancel event is dispatched we tell the timeline it needs to observe the refresh driver for at least one more tick."

::: layout/style/nsTransitionManager.h:227
(Diff revision 2)
>        const DocumentTimeline& aTimeline,
>        const TimeStamp& aBaseTime,
>        const TimeDuration& aStartTime,
>        double aPlaybackRate);
>  
> +  void QueueCSSEventsNow(StickyTimeDuration activeTime) override;

s/activeTime/aActiveTime/

::: layout/style/nsTransitionManager.h:236
(Diff revision 2)
>    // Animation overrides
>    void UpdateTiming(SeekFlag aSeekFlag,
>                      SyncNotifyFlag aSyncNotifyFlag) override;
> +  void QueueEvents(StickyTimeDuration activeTime);
>  
> -  void QueueEvents();
> -

(Since this is not actually an override, we probably shouldn't move it up under the section that says "Animation overrides", i.e. we should leave the blank line there.)

::: layout/style/nsTransitionManager.h:239
(Diff revision 2)
>    }
>  
>    // Animation overrides
>    void UpdateTiming(SeekFlag aSeekFlag,
>                      SyncNotifyFlag aSyncNotifyFlag) override;
> +  void QueueEvents(StickyTimeDuration activeTime);

As discussed, let's add a default value for this argument. I think '0' will work but I think the following might be slightly easier for the compiler to optimize since it is a constexpr constructor:

  StickyTimeDuration aActiveTime = StickyTimeDuration()

Note that we need to do s/activeTime/aActiveTime/

::: layout/style/nsTransitionManager.cpp:183
(Diff revision 2)
>  
>    Animation::UpdateTiming(aSeekFlag, aSyncNotifyFlag);
>  }
>  
>  void
> -CSSTransition::QueueEvents()
> +CSSTransition::QueueEvents(StickyTimeDuration activeTime)

s/activeTime/aActiveTime/

::: layout/style/nsTransitionManager.cpp:208
(Diff revision 2)
> +  if (!activeTime) {
> +    // If don't have parameter, i.e. asynchronus queueing event.
> +    // We should calculate active time from previous current time.
> +    ComputedTiming oldTiming =
> +      mEffect->GetComputedTimingAt(mPreviousCurrentTime,
> +                                   mEffect->SpecifiedTiming(),
> +                                   mPlaybackRate);
> +    activeTime = oldTiming.mActiveTime;
> +  }

As discussed, I can't think of a case where we need this code. That is, I think we only go idle when we cancel the animation or remove its timeline--in which case I think we already pass an active time when we call this?

Also, testing for "!activeTime" isn't quite right since it will evaluate true when we pass a zero active time (even though I think we're trying to test whether or not an active time was passed or not).

So I think we can just drop this code.

(We still need to drop the local declaration of activeTime, however and make the code below refer to |aActiveTime| instead of |activeTime|.)

::: layout/style/nsTransitionManager.cpp:332
(Diff revision 2)
>  
>  void
>  CSSTransition::Tick()
>  {
>    Animation::Tick();
> -  QueueEvents();
> +  QueueEvents(nullptr);

We shouldn't need this change anymore (and if we do, we should pass '0' instead of nullptr).

::: layout/style/nsTransitionManager.cpp:395
(Diff revision 2)
>  void
> +CSSTransition::QueueCSSEventsNow(StickyTimeDuration activeTime) {
> +  QueueEvents(activeTime);
> +}

Perhaps just move this to the header file? (It's a virtual function so it's not likely to be inlined--at least not the way we're using it--but it just makes it easier to maintain to have these one-line functions be defined in one place.)
Attachment #8814762 - Flags: review?(bbirtles) → review+
Comment on attachment 8816949 [details]
Bug 1264125 part 6 - Queue CSS related event when setting null target effect.

https://reviewboard.mozilla.org/r/97430/#review98330

I'd like to take another look at this with the following changes made.

::: dom/animation/Animation.cpp:145
(Diff revision 1)
>      return;
>    }
>  
>    AutoMutationBatchForAnimation mb(*this);
>    bool wasRelevant = mIsRelevant;
> +  ComputedTiming timing;

See below, but I think we won't need this.

::: dom/animation/Animation.cpp:161
(Diff revision 1)
> +    // Remain current computed timing for events
> +    if (!aEffect) {
> +      timing = mEffect->GetComputedTiming();
> +    }
> +
>      // Break links with the old effect and then drop it.
>      RefPtr<AnimationEffectReadOnly> oldEffect = mEffect;
>      mEffect = nullptr;
>      oldEffect->SetAnimation(nullptr);
>  
>      // The following will not do any notification because mEffect is null.
>      UpdateRelevance();
> +
> +    // Queue the css related events.
> +    if (!aEffect) {
> +      QueueCSSEventsNow(timing.mActiveTime);
> +    }

As discussed, I don't *think* we need to queue this synchronously--it also seems like we don't use the active time we pass to QueueCSSEventsNow. If there is a test that fails without this, we should add a comment to describe *why* we need to do this synchronously.

::: dom/animation/Animation.cpp:744
(Diff revision 1)
> +  if (!mEffect) {
> +    return AnimationTimeToTimeStamp(aElapsedTime);
> +  }

I think the following might be easier to follow:

  TimeDuration delay = mEffect
                       ? mEffect->SpecifiedTiming().mDelay
                       : TimeDuration();
  return AnimationTimeToTimeStamp(aElapsedTime + delay);

::: layout/style/nsTransitionManager.cpp:200
(Diff revision 1)
> +  if (!mEffect &&
> +      (mPreviousTransitionPhase != TransitionPhase::After)) {
> +    // If no target effect after transition run, we should treat as after.
> +    // So we will need to queue transitionend event.
> +    // The elapsed time is zero due to don't have target effect.
> +    manager->QueueEvent(TransitionEventInfo(owningElement, owningPseudoType,
> +                                            eTransitionEnd,
> +                                            TransitionProperty(),
> +                                            StickyTimeDuration(0),
> +                                            ElapsedTimeToTimeStamp(0),
> +                                            this));
> +    mPreviousTransitionPhase = TransitionPhase::After;
> +    return;
> +  }
> +
> +  if (!mEffect) {
> +    return;
> +  }

* We don't need the extra ()s around the != condition
* I think this would be easier to follow if we did the (!mEffect) branch and
  had just one early return.
* I'm not sure if testing for != TransitionPhase::After is correct. If we clear
  the effect and then cancel() it, it seems like we should not dispatch
  a transitioncancel event. Perhaps that's why we were doing the above in
  a synchronous manner?

  I think we need to add a test for:

  a) Call cancel() then call anim.effect = null;
  b) Call anim.effect = null then call cancel()

  In both cases we should only get a cancel event. I *think* what we want to
  test is simply if mPreviousTransitionPhase is before / active / pending, e.g.

  if (!mEffect) {
    if (mPreviousTransitionPhase == TransitionPhase::Pending ||
        mPreviousTransitionPhase == TransitionPhase::Before ||
        mPreviousTransitionPhase == TransitionPhase::Active) {
      // If we no longer have a target effect, we should behave as if we
      // are in the after phase. In this case we simply set the elapsed time
      // to zero.
      manager->QueueEvent(TransitionEventInfo(owningElement, owningPseudoType,
                                              eTransitionEnd,
                                              TransitionProperty(),
                                              StickyTimeDuration(0),
                                              ElapsedTimeToTimeStamp(0),
                                              this));
      mPreviousTransitionPhase = TransitionPhase::After;
    }
    return;
  }

However, having said all that, I think if we go, e.g. from pending -> after due
to losing our target effect we should follow the usual behavior for that
transition defined in the spec, i.e. dispatch both transitionstart and
transitionend.

So I think we should handle this by simply setting |intervalStartTime|,
|intervalEndTime|, and |currentPhase| appropriately when mEffect is null and
then following the usual logic. Would that work?

::: layout/style/nsTransitionManager.cpp:220
(Diff revision 1)
> +  if (!mEffect) {
> +    return;
> +  }
>  
>    ComputedTiming computedTiming = mEffect->GetComputedTiming();
> +  TimingParams timing = mEffect->SpecifiedTiming();

It seems like we don't use this line.
Attachment #8816949 - Flags: review?(bbirtles)
Comment on attachment 8814762 [details]
Bug 1264125 part 5 - Call the queueing events when canceling transition via Style or Script.

https://reviewboard.mozilla.org/r/95852/#review98324

> As discussed, I can't think of a case where we need this code. That is, I think we only go idle when we cancel the animation or remove its timeline--in which case I think we already pass an active time when we call this?
> 
> Also, testing for "!activeTime" isn't quite right since it will evaluate true when we pass a zero active time (even though I think we're trying to test whether or not an active time was passed or not).
> 
> So I think we can just drop this code.
> 
> (We still need to drop the local declaration of activeTime, however and make the code below refer to |aActiveTime| instead of |activeTime|.)

As you mention, this code is not necessary.
So I remove this modification.
Comment on attachment 8816949 [details]
Bug 1264125 part 6 - Queue CSS related event when setting null target effect.

https://reviewboard.mozilla.org/r/97430/#review98330

> As discussed, I don't *think* we need to queue this synchronously--it also seems like we don't use the active time we pass to QueueCSSEventsNow. If there is a test that fails without this, we should add a comment to describe *why* we need to do this synchronously.

Sorry.
I misunderstood that we remove the refresh observer when removing the target effect.
As you said, this code is not necessary.

> * We don't need the extra ()s around the != condition
> * I think this would be easier to follow if we did the (!mEffect) branch and
>   had just one early return.
> * I'm not sure if testing for != TransitionPhase::After is correct. If we clear
>   the effect and then cancel() it, it seems like we should not dispatch
>   a transitioncancel event. Perhaps that's why we were doing the above in
>   a synchronous manner?
> 
>   I think we need to add a test for:
> 
>   a) Call cancel() then call anim.effect = null;
>   b) Call anim.effect = null then call cancel()
> 
>   In both cases we should only get a cancel event. I *think* what we want to
>   test is simply if mPreviousTransitionPhase is before / active / pending, e.g.
> 
>   if (!mEffect) {
>     if (mPreviousTransitionPhase == TransitionPhase::Pending ||
>         mPreviousTransitionPhase == TransitionPhase::Before ||
>         mPreviousTransitionPhase == TransitionPhase::Active) {
>       // If we no longer have a target effect, we should behave as if we
>       // are in the after phase. In this case we simply set the elapsed time
>       // to zero.
>       manager->QueueEvent(TransitionEventInfo(owningElement, owningPseudoType,
>                                               eTransitionEnd,
>                                               TransitionProperty(),
>                                               StickyTimeDuration(0),
>                                               ElapsedTimeToTimeStamp(0),
>                                               this));
>       mPreviousTransitionPhase = TransitionPhase::After;
>     }
>     return;
>   }
> 
> However, having said all that, I think if we go, e.g. from pending -> after due
> to losing our target effect we should follow the usual behavior for that
> transition defined in the spec, i.e. dispatch both transitionstart and
> transitionend.
> 
> So I think we should handle this by simply setting |intervalStartTime|,
> |intervalEndTime|, and |currentPhase| appropriately when mEffect is null and
> then following the usual logic. Would that work?

Hmm, As you mentioned, this code won't fire the transition start/end following example:

target.style.transition = "margin-left 100s";
target.style.marginLeft = "200px";
var anim = target.getAnimations()[0];
anim.effect = null;

I decided that go through same pass even if we don't have target effect.

>  I think we need to add a test for:
>
>  a) Call cancel() then call anim.effect = null;
>  b) Call anim.effect = null then call cancel()

I added above situation tests to the part 8 patch. (file_event-dispatch.html)
Comment on attachment 8803222 [details]
Bug 1264125 part 8 - Add transitioncancel tests.

https://reviewboard.mozilla.org/r/87532/#review98318

> I don't think it makes sense to wait on finished here. We called finish() so it is already finished. The point of waiting on |ready| (in comment 40) was to make sure we pass a tick while being finished. We should probably just use 'return waitForFrame()' here I guess.

This test duplicated with file_event-dispatch.html, so I removed this test from animation-cancel.
Comment on attachment 8816949 [details]
Bug 1264125 part 6 - Queue CSS related event when setting null target effect.

https://reviewboard.mozilla.org/r/97430/#review99028

::: layout/style/nsTransitionManager.cpp:199
(Diff revision 2)
> +  const StickyTimeDuration zeroDuration = StickyTimeDuration(0);
> +  StickyTimeDuration intervalStartTime;
> +  StickyTimeDuration intervalEndTime;
> +  TransitionPhase currentPhase;
> +
> +  if (!mEffect) {
> +    // If we no longer have a target effect, we should behave as if we
> +    // are in the after phase. In this case we simply set the elapsed time
> +    // to zero.
> +    currentPhase      = TransitionPhase::After;
> +    intervalStartTime = StickyTimeDuration(0);
> +    intervalEndTime   = StickyTimeDuration(0);
> +  } else {
> -  ComputedTiming computedTiming = mEffect->GetComputedTiming();
> +    ComputedTiming computedTiming = mEffect->GetComputedTiming();
> -  const StickyTimeDuration zeroDuration;
> +    intervalStartTime =
> -  StickyTimeDuration intervalStartTime =
> -    std::max(std::min(StickyTimeDuration(-mEffect->SpecifiedTiming().mDelay),
> +      std::max(std::min(StickyTimeDuration(-mEffect->SpecifiedTiming().mDelay),
> -                      computedTiming.mActiveDuration), zeroDuration);
> +                        computedTiming.mActiveDuration), zeroDuration);
> -  StickyTimeDuration intervalEndTime =
> +    intervalEndTime =
> -    std::max(std::min((EffectEnd() - mEffect->SpecifiedTiming().mDelay),
> +      std::max(std::min((EffectEnd() - mEffect->SpecifiedTiming().mDelay),
> -                      computedTiming.mActiveDuration), zeroDuration);
> +                        computedTiming.mActiveDuration), zeroDuration);
> +    currentPhase = static_cast<TransitionPhase>(computedTiming.mPhase);
> +  }

This is a really minor nit so feel free to ignore it, however, in terms of making this code easier to read, I would suggest declaring the variables in the same order as you assign them (where possible).

Also, conceptually I think the 'phase' comes first and this also matches the order in the comment for the 'mEffect' branch.

I would add a line between where we set up the const |zeroDuration| value and the three variables we're about to assign since logically they have a different role and scope.

(Furthermore, you *could* put a blank line between where we get the computed timing and where we use it to set the three variables in the second branch. Up to you.)

So, for example, something like the following might be a little easier to read:

  const StickyTimeDuration zeroDuration = StickyTimeDuration();

  TransitionPhase currentPhase;
  StickyTimeDuration intervalStartTime;
  StickyTimeDuration intervalEndTime;

  if (!mEffect) {
    // If we no longer have a target effect, we should behave as if we
    // are in the after phase. In this case we simply set the elapsed time
    // to zero.
    currentPhase      = TransitionPhase::After;
    intervalStartTime = zeroDuration;
    intervalEndTime   = zeroDuration;
  } else {
    ComputedTiming computedTiming = mEffect->GetComputedTiming();

    currentPhase = static_cast<TransitionPhase>(computedTiming.mPhase);
    intervalStartTime =
      std::max(std::min(StickyTimeDuration(-mEffect->SpecifiedTiming().mDelay),
                        computedTiming.mActiveDuration), zeroDuration);
    intervalEndTime =
      std::max(std::min((EffectEnd() - mEffect->SpecifiedTiming().mDelay),
                        computedTiming.mActiveDuration), zeroDuration);
  }

::: layout/style/nsTransitionManager.cpp:199
(Diff revision 2)
>    nsPresContext* presContext = mOwningElement.GetRenderedPresContext();
>    if (!presContext) {
>      return;
>    }
>  
> +  const StickyTimeDuration zeroDuration = StickyTimeDuration(0);

Let's use StickyTimeDuration() instead of StickyTimeDuration(0) since it's constexpr (and since the name of the variable, zeroDuration, makes it obvious what the value is).

::: layout/style/nsTransitionManager.cpp:209
(Diff revision 2)
> +    intervalStartTime = StickyTimeDuration(0);
> +    intervalEndTime   = StickyTimeDuration(0);

Just set these to |zeroDuration|.

::: layout/style/nsTransitionManager.cpp:226
(Diff revision 2)
>  
>    // TimeStamps to use for ordering the events when they are dispatched. We
>    // use a TimeStamp so we can compare events produced by different elements,
>    // perhaps even with different timelines.
>    // The zero timestamp is for transitionrun events where we ignore the delay
> -  // for the purpose of ordering events.
> +  // for the purpose of ordering events.  TimeStamp startTimeStamp;

I don't think we want this change, right?
Attachment #8816949 - Flags: review?(bbirtles) → review+
Comment on attachment 8818191 [details]
Bug 1264125 part 9 - Rename variable name of CSS-Transition tests.

https://reviewboard.mozilla.org/r/98316/#review99030
Attachment #8818191 - Flags: review?(bbirtles) → review+
Comment on attachment 8816949 [details]
Bug 1264125 part 6 - Queue CSS related event when setting null target effect.

https://reviewboard.mozilla.org/r/97430/#review99038

I'm really sorry but I guess I should check this patch once more with the change discussed below.

::: layout/style/nsTransitionManager.cpp:208
(Diff revision 2)
> +
> +  if (!mEffect) {
> +    // If we no longer have a target effect, we should behave as if we
> +    // are in the after phase. In this case we simply set the elapsed time
> +    // to zero.
> +    currentPhase      = TransitionPhase::After;

As discussed, I think this shouldn't actually always go to 'after'. In some cases it can be 'before' or 'idle' as well.

So I think we need to add something like:

  Nullable<TimeDuration> currentTime = GetCurrentTime();
  currentPhase = currentTime.IsNull()
                 ? TransitionPhase::Idle;
                 : (currentTime.Value() < 0
                    ?  TransitionPhase::Before
                    : TransitionPhase::After);

Although that's really messy so I guess we should factor out a method like, GetPhaseForTransitionWithoutEffect, e.g.

  MOZ_ASSERT(!mEffect, "Should not have no effect");

  Nullable<TimeDuration> currentTime = GetCurrentTime();
  if (currentTime.IsNull()) {
    return TransitionPhase::Idle;
  }

  return currentTime.Value() < 0
         ?  TransitionPhase::Before
         : TransitionPhase::After;

Also, we need to update the comment.
Attachment #8816949 - Flags: review+
Comment on attachment 8803222 [details]
Bug 1264125 part 8 - Add transitioncancel tests.

https://reviewboard.mozilla.org/r/87532/#review99032

Looking really good, but I'd like to check the final 'Cancel the transition after clear the target effect' test since I think it is missing something.

::: dom/animation/test/css-transitions/file_animation-cancel.html
(Diff revision 5)
> -  flushComputedStyle(div);
>  
>    var animation = div.getAnimations()[0];
> -  return animation.ready.then(function() {
> +  return animation.ready.then(waitForFrame).then(function() {
>      assert_equals(animation.playState, 'running');
>      div.style.display = 'none';
> +    flushComputedStyle(div);

As discussed, it doesn't seem like we should need this change.

::: dom/animation/test/css-transitions/file_animation-cancel.html:128
(Diff revision 5)
>  
>    var animation = childDiv.getAnimations()[0];
>    return animation.ready.then(function() {
>      assert_equals(animation.playState, 'running');
>      parentDiv.style.display = 'none';
> +    flushComputedStyle(childDiv);

I wonder if we need this change either?

::: dom/animation/test/css-transitions/file_animation-cancel.html:144
(Diff revision 5)
> +
> +  div.style.transition = 'margin-left 100s';
> +  div.style.marginLeft = '1000px';
> +
> +  var animation = div.getAnimations()[0];
> +  return animation.ready.then(waitForFrame).then(function() {

Here and below, do we need to call waitForFrame? Isn't waiting on 'ready' enough?

As discussed, this is probably left over from when we were testing events.

::: dom/animation/test/css-transitions/file_animation-cancel.html:146
(Diff revision 5)
> +    div.style.transitionProperty = 'marg'; // set the unavailable property value
> +    flushComputedStyle(div);
> +    return waitForFrame();
> +  }).then(function() {
> +    assert_equals(animation.playState, 'idle');
> +    assert_equals(getComputedStyle(div).marginLeft, '1000px');
> +  });
> +}, 'Setting unavailable property value to transitionProperty');

s/unavailable/unrecognized/ ?
(2 occurrences)

Actually, strictly speaking, I think all this test is really checking is that if we remove 'margin-left' from 'transition-property' the transition is cancelled.

If we already have a test for that, we could just drop this one. If we don't, perhaps we should change the description to just, 'Removing the property from transition-property'.

::: dom/animation/test/css-transitions/file_animation-cancel.html:184
(Diff revision 5)
> +  div.style.marginLeft = '1000px';
> +
> +  var animation = div.getAnimations()[0];
> +  return animation.ready.then(waitForFrame).then(function() {
> +    assert_equals(animation.playState, 'running');
> +    div.style.transition = 'background-image 10s';

As discussed, this is not really testing that we cancel when we can't interpolate. For that, we should set 'marginLeft' to 'auto' (or find some other property which has this behavior that some values are interpolable and some are not).

Also, I think once we do that getComputedStyle(div).marginLeft will be come 'auto' (since, as dbaron pointed out to me recently, 'auto' is already a computed value).

::: dom/animation/test/css-transitions/file_animation-cancel.html:191
(Diff revision 5)
> +    return waitForFrame();
> +  }).then(function() {
> +    assert_equals(animation.playState, 'idle');
> +    assert_equals(getComputedStyle(div).marginLeft, '1000px');
> +  });
> +}, 'An after-change style value won\'t be interpolated');

s/won't/can't/

::: dom/animation/test/css-transitions/file_animation-cancel.html:203
(Diff revision 5)
> +  div.style.marginLeft = '1000px';
> +
> +  var animation = div.getAnimations()[0];
> +  return animation.ready.then(function() {
> +    assert_equals(animation.playState, 'running');
> +    // reversing the transition.

I would probably just drop this comment (or if you want to keep it, make 'reversing' the imperative: 'Reverse')

::: dom/animation/test/css-transitions/file_animation-cancel.html:208
(Diff revision 5)
> +    // reversing the transition.
> +    div.style.marginLeft = '0px';
> +    flushComputedStyle(div);
> +    return waitForFrame();
> +  }).then(function() {
> +    // Old transition will be idle.

Again, I'd probably just drop this comment.

::: dom/animation/test/css-transitions/file_animation-cancel.html:211
(Diff revision 5)
> +    return waitForFrame();
> +  }).then(function() {
> +    // Old transition will be idle.
> +    assert_equals(animation.playState, 'idle');
> +  });
> +}, 'Reversing adjusted start value is same as after-change style value');

Perhaps just, 'Reversing a running transition cancels the original transition' ?

::: dom/animation/test/css-transitions/file_animation-cancel.html:213
(Diff revision 5)
> +promise_test(function(t) {
> +  var div = addDiv(t, { style: 'margin-left: 0px' });
> +  flushComputedStyle(div);
> +
> +  div.style.transition = 'margin-left 100s';
> +  div.style.marginLeft = '1000px';
> +
> +  var animation = div.getAnimations()[0];
> +  return animation.ready.then(waitForFrame).then(function() {
> +    assert_equals(animation.playState, 'running');
> +    // Setting interlatable transition property
> +    div.style.transitionProperty = 'margin-top';
> +    flushComputedStyle(div);
> +    return waitForFrame();
> +  }).then(function(evt) {
> +    assert_equals(animation.playState, 'idle');
> +    assert_equals(getComputedStyle(div).marginLeft, '1000px');
> +  });
> +}, 'Setting interpolatable transition');

I don't understand what this test is supposed to cover, but I don't think we need it. It seems to be just another case of changing the transition-property so that it no longer includes the property we're transitioning.

::: dom/animation/test/css-transitions/file_event-dispatch.html:126
(Diff revision 5)
> +    transition.timeline = undefined;
> +    return watcher.wait_for('transitioncancel');
> +  }).then(function(evt) {
> +    assert_equals(evt.elapsedTime, 0.0);
> +  });
> +}, 'Before -> Idle (Animation.timeline = undefined)');

Let's use s/undefined/null/ here since I think that's what the WebIDL bindings end up doing for us anyway.

::: dom/animation/test/css-transitions/file_event-dispatch.html:134
(Diff revision 5)
> +  var [transition, watcher, div] =
> +    setupTransition(t, 'margin-left 100s 100s');

(Nit: I don't think we need |div| here)

::: dom/animation/test/css-transitions/file_event-dispatch.html:148
(Diff revision 5)
> -  var [transition, watcher, handler] = setupTransition(t);
> +  var [transition, watcher, div] =
> +    setupTransition(t, 'margin-left 100s 100s');

(Likewise I don't think we need |div| here or a few other places in this file.)

::: dom/animation/test/css-transitions/file_event-dispatch.html:188
(Diff revision 5)
> +    transition.timeline = undefined;
> +    return watcher.wait_for('transitioncancel');
> +  }).then(function(evt) {
> +    assert_equals(evt.elapsedTime, 0.0);
> +  });
> +}, 'Active -> Idle, no delay (Animation.timeline = undefined)');

Here too, s/undefined/null/

Likewise for a number of other places in this file.

::: dom/animation/test/css-transitions/file_event-dispatch.html:297
(Diff revision 5)
> -  var [transition, watcher, handler] = setupTransition(t);
> +  var [transition, watcher, div] =
> +    setupTransition(t, 'margin-left 100s 100s');

No need for |div| here?

::: dom/animation/test/css-transitions/file_event-dispatch.html:332
(Diff revision 5)
> -  var [transition, watcher, handler] =
> -    setupTransition(t, 'transition: margin-left 100s -50s');
> +  var [transition, watcher, div] =
> +    setupTransition(t, 'margin-left 100s -50s');

No need for |div| here?

Likewise elsewhere in this file.

::: dom/animation/test/css-transitions/file_event-dispatch.html:417
(Diff revision 5)
> +    // Make idle
> +    div.style.display = 'none';
> +    flushComputedStyle(div);
> +    transition.play();
> +    transition.cancel();
> +    return watcher.wait_for('transitioncancel');

Do we need to wait a few frames to check we don't get a subsequent run and cancel?

::: dom/animation/test/css-transitions/file_event-dispatch.html:419
(Diff revision 5)
> +    flushComputedStyle(div);
> +    transition.play();
> +    transition.cancel();
> +    return watcher.wait_for('transitioncancel');
> +  });
> +}, 'Call Animation.cancel after restarting transtion immediately');

s/transtion/transition/

::: dom/animation/test/css-transitions/file_event-dispatch.html:445
(Diff revision 5)
> +    return watcher.wait_for('transitioncancel');
> +  }).then(function() {
> +    // Make After phase
> +    transition.effect = null;
> +
> +    return watcher.wait_for([ 'transitionrun',
> +                              'transitionstart',
> +                              'transitionend' ]);

As discussed and described in comment 72, I think setting the effect to null should not cause the transition to appear to run again like this. We need to fix part 6 and then adjust this test to check that no other events are dispatched.

::: dom/animation/test/css-transitions/file_event-dispatch.html:456
(Diff revision 5)
> +promise_test(function(t) {
> +  var [transition, watcher, div] =
> +    setupTransition(t, 'margin-left 100s');
> +
> +  return watcher.wait_for([ 'transitionrun',
> +                            'transitionstart' ]).then(function(evt) {
> +    transition.cancel();
> +
> +    return watcher.wait_for('transitioncancel');
> +  });
> +}, 'Cancel the transition after clear the target effect');

We don't seem to actually clear the target effect here.
Attachment #8803222 - Flags: review?(bbirtles)
Comment on attachment 8816949 [details]
Bug 1264125 part 6 - Queue CSS related event when setting null target effect.

https://reviewboard.mozilla.org/r/97430/#review99028

> This is a really minor nit so feel free to ignore it, however, in terms of making this code easier to read, I would suggest declaring the variables in the same order as you assign them (where possible).
> 
> Also, conceptually I think the 'phase' comes first and this also matches the order in the comment for the 'mEffect' branch.
> 
> I would add a line between where we set up the const |zeroDuration| value and the three variables we're about to assign since logically they have a different role and scope.
> 
> (Furthermore, you *could* put a blank line between where we get the computed timing and where we use it to set the three variables in the second branch. Up to you.)
> 
> So, for example, something like the following might be a little easier to read:
> 
>   const StickyTimeDuration zeroDuration = StickyTimeDuration();
> 
>   TransitionPhase currentPhase;
>   StickyTimeDuration intervalStartTime;
>   StickyTimeDuration intervalEndTime;
> 
>   if (!mEffect) {
>     // If we no longer have a target effect, we should behave as if we
>     // are in the after phase. In this case we simply set the elapsed time
>     // to zero.
>     currentPhase      = TransitionPhase::After;
>     intervalStartTime = zeroDuration;
>     intervalEndTime   = zeroDuration;
>   } else {
>     ComputedTiming computedTiming = mEffect->GetComputedTiming();
> 
>     currentPhase = static_cast<TransitionPhase>(computedTiming.mPhase);
>     intervalStartTime =
>       std::max(std::min(StickyTimeDuration(-mEffect->SpecifiedTiming().mDelay),
>                         computedTiming.mActiveDuration), zeroDuration);
>     intervalEndTime =
>       std::max(std::min((EffectEnd() - mEffect->SpecifiedTiming().mDelay),
>                         computedTiming.mActiveDuration), zeroDuration);
>   }

Thanks.

I've brushed up around this code.
Comment on attachment 8803222 [details]
Bug 1264125 part 8 - Add transitioncancel tests.

https://reviewboard.mozilla.org/r/87532/#review99032

> (Likewise I don't think we need |div| here or a few other places in this file.)

We will need to div element for creating TransitionEventHandler object.

> No need for |div| here?

As same reason of previous my comment, we will need to create TransitionEventHandler.

> No need for |div| here?
> 
> Likewise elsewhere in this file.

As same reason of previous my comment, we will need to create TransitionEventHandler.
Comment on attachment 8816949 [details]
Bug 1264125 part 6 - Queue CSS related event when setting null target effect.

https://reviewboard.mozilla.org/r/97430/#review99698

Looks good with the following changes.

::: layout/style/nsTransitionManager.cpp:206
(Diff revisions 2 - 3)
>      // If we no longer have a target effect, we should behave as if we
>      // are in the after phase. In this case we simply set the elapsed time
>      // to zero.

Sorry, this was the comment I was referring when I said, "we need to update the comment."

Let's just drop it, actually.

::: layout/style/nsTransitionManager.cpp:338
(Diff revisions 2 - 3)
>  }
>  
> +CSSTransition::TransitionPhase
> +CSSTransition::GetTransitionPhaseWithoutEffect() const
> +{
> +  MOZ_ASSERT(!mEffect, "Should not have effect.");

"Should not have an effect"

or, better still,

"Should only be called when when we do not have an effect"

(Sorry, my original comment was wrong)

::: layout/style/nsTransitionManager.cpp:345
(Diff revisions 2 - 3)
> +  // If we don't have target effect, duration will be zero.
> +  // So phase is before if current time less than zero.

// 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.

::: layout/style/nsTransitionManager.cpp:347
(Diff revisions 2 - 3)
> +    return TransitionPhase::Idle;
> +  }
> +
> +  // If we don't have target effect, duration will be zero.
> +  // So phase is before if current time less than zero.
> +  return currentTime.Value() < TimeDuration(0)

Let's use just 'TimeDuration()' here.

::: layout/style/nsTransitionManager.h:289
(Diff revision 3)
>    // ElementPropertyTransition (effect) however since it can be replaced
>    // using the Web Animations API.
>    nsCSSPropertyID mTransitionProperty;
>    StyleAnimationValue mTransitionToValue;
> +
> +  // Return current TransitionPhase when transition doesn't have target effect.

// Return the TransitionPhase to use when the transition doesn't have a target
  // effect.

::: layout/style/nsTransitionManager.h:290
(Diff revision 3)
>    // using the Web Animations API.
>    nsCSSPropertyID mTransitionProperty;
>    StyleAnimationValue mTransitionToValue;
> +
> +  // Return current TransitionPhase when transition doesn't have target effect.
> +  TransitionPhase GetTransitionPhaseWithoutEffect() const;

When adding methods to a class, please consider the order in which they appear. Otherwise, over time, the class becomes very hard to read.

Generally we try to keep data members at the bottom.

This method should go after QueueEvents() since it logically is related to it (and it doesn't belong with 'UpdateTiming' since it's not an 'Animation override').
Attachment #8816949 - Flags: review?(bbirtles) → review+
(In reply to Brian Birtles (:birtles) from comment #85)
> // 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.

(There seems to be a bug in MozReview that it trims spaces at the start of the first line.)
Comment on attachment 8803222 [details]
Bug 1264125 part 8 - Add transitioncancel tests.

https://reviewboard.mozilla.org/r/87532/#review99700

Nearly there! I just want to quickly check the changes to the non-interpolable value test before we land this.

::: dom/animation/test/css-transitions/file_event-dispatch.html:192
(Diff revisions 5 - 6)
> -    transition.timeline = undefined;
> +    transition.timeline = null;
>      return watcher.wait_for('transitioncancel');
>    }).then(function(evt) {
>      assert_equals(evt.elapsedTime, 0.0);
>    });
> -}, 'Active -> Idle, no delay (Animation.timeline = undefined)');
> +}, 'Active -> Idle, no delay (Animation.timeline = )');

I think this might be missing a 'null' ?

::: dom/animation/test/css-transitions/file_event-dispatch.html:263
(Diff revisions 5 - 6)
> -    transition.timeline = undefined;
> +    transition.timeline = null;
>      return watcher.wait_for('transitioncancel');
>    }).then(function(evt) {
>      assert_equals(evt.elapsedTime, 0.0);
>    });
> -}, 'Active -> Idle, with negative delay (Animation.timeline = undefined)');
> +}, 'Active -> Idle, with negative delay (Animation.timeline = )');

I think this might be missing a 'null' ?

::: dom/animation/test/css-transitions/file_event-dispatch.html:469
(Diff revisions 5 - 6)
> +  }).then(function(evt) {
>      transition.cancel();
> -
>      return watcher.wait_for('transitioncancel');
>    });
>  }, 'Cancel the transition after clear the target effect');

s/clear/clearing/

::: dom/animation/test/css-transitions/file_animation-cancel.html:144
(Diff revision 6)
> +  div.style.marginLeft = '1000px';
> +
> +  var animation = div.getAnimations()[0];
> +  return animation.ready.then(function() {
> +    assert_equals(animation.playState, 'running');
> +    // set the unrecognized property value

Set an unrecognized property value

::: dom/animation/test/css-transitions/file_animation-cancel.html:152
(Diff revision 6)
> +    return waitForFrame();
> +  }).then(function() {
> +    assert_equals(animation.playState, 'idle');
> +    assert_equals(getComputedStyle(div).marginLeft, '1000px');
> +  });
> +}, 'Removing the property from transition-property');

'Removing a property from transition-property cancels transitions on that property'

::: dom/animation/test/css-transitions/file_animation-cancel.html:173
(Diff revision 6)
> +promise_test(function(t) {
> +  var div = addDiv(t, { style: 'margin-left: 0px' });
> +  flushComputedStyle(div);
> +
> +  div.style.transition = 'margin-left 100s';
> +  div.style.marginLeft = '1000px';
> +
> +  var animation = div.getAnimations()[0];
> +  return animation.ready.then(function() {
> +    assert_equals(animation.playState, 'running');
> +    div.style.marginLeft = 'auto';
> +    flushComputedStyle(div);
> +    return waitForFrame();
> +  }).then(function() {
> +    // We will need to confirm computed style value, however
> +    // gecko can't get correct value when setting 'auto'.(bug 381328)
> +    assert_equals(animation.playState, 'idle');
> +  });
> +}, 'An after-change style value can\'t be interpolated');

This is a slightly odd test. We have at least three cases we could test:

a) Updating the transition property to a different value
   => cancels current transition, triggers new transition
b) Updating the transition property to a different, non-interpolable value
   => cancels current transition
c) Updating the transition property to the transition start point
   => cancels current transitions, creates a new transition with timing adjusted
      to make it act as a reverse transition

We have covered (c) later in this file. This test covers (b) but we don't have (a) as far as I can tell. I'm also not sure if we really need to cover *both* (a) and (b) or whether (a) would be enough.

In either case, we probably don't need to worry about checking getComputedStyle. Checking that the original transition has been cancelled would be enough.

If we decide to test (b) as well, then the only difference is that at the end div.getAnimations().length should be zero because we should not have generated a new transition for the non-interpolable value.

Since you've written this test for (b), how about we:

* Add a similar test that updates marginLeft to something interpolable (but not equal to the start point) like '2000px'
* Drop the comment here about getComputedStyle and just make it test that div.getAnimations().length is zero.
Attachment #8803222 - Flags: review?(bbirtles)
Comment on attachment 8816949 [details]
Bug 1264125 part 6 - Queue CSS related event when setting null target effect.

https://reviewboard.mozilla.org/r/97430/#review99698

> When adding methods to a class, please consider the order in which they appear. Otherwise, over time, the class becomes very hard to read.
> 
> Generally we try to keep data members at the bottom.
> 
> This method should go after QueueEvents() since it logically is related to it (and it doesn't belong with 'UpdateTiming' since it's not an 'Animation override').

Thanks, Brian,

I've moved declaration of this function. However this function type is TransisitionPhase, it is defined bottom of class structures.
So we need to specify the forward declaration of this.
Comment on attachment 8803222 [details]
Bug 1264125 part 8 - Add transitioncancel tests.

https://reviewboard.mozilla.org/r/87532/#review100056

r=me with the following changes.

::: dom/animation/test/css-transitions/file_animation-cancel.html:184
(Diff revisions 6 - 7)
>    div.style.marginLeft = '1000px';
>  
>    var animation = div.getAnimations()[0];
>    return animation.ready.then(function() {
>      assert_equals(animation.playState, 'running');
> +    div.style.transitionProperty = 'margin-top';

This test is supposed to test when the transition is cancelled by changing the *value* of the property being transitioned, not the transition property itself.

e.g. this should do div.style.marginLeft = '500px';

::: dom/animation/test/css-transitions/file_animation-cancel.html:191
(Diff revisions 6 - 7)
> +    return waitForFrame();
> +  }).then(function() {
> +    assert_equals(getComputedStyle(div).marginLeft, '1000px');
> +    assert_equals(animation.playState, 'idle');
> +  });
> +}, 'An after-change style value can be interpolated');

'Changing style to another interpolable value cancels the original transition'

::: dom/animation/test/css-transitions/file_animation-cancel.html:207
(Diff revisions 6 - 7)
> +    assert_equals(animation.playState, 'running');
>      div.style.marginLeft = 'auto';
>      flushComputedStyle(div);
>      return waitForFrame();
>    }).then(function() {
> -    // We will need to confirm computed style value, however
> +    assert_equals(div.getAnimations().length, 0);

Let's add a comment to this assertion, 'There should be no transitions'.
Attachment #8803222 - Flags: review?(bbirtles) → review+
Comment on attachment 8803222 [details]
Bug 1264125 part 8 - Add transitioncancel tests.

https://reviewboard.mozilla.org/r/87532/#review100056

Thanks, Brian,

I've fixed it.
Pushed by mantaroh@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/58ee8425d30e
part 1 - Add transitioncancel event handler. r=masayuki
https://hg.mozilla.org/integration/autoland/rev/25b861c8365a
part 2 - Add ontransitioncancel EventHandler to WebIDL. r=baku
https://hg.mozilla.org/integration/autoland/rev/c06e038a1baf
part 3 - Add member of active time to ComputedTiming. r=birtles
https://hg.mozilla.org/integration/autoland/rev/6a037add5f91
part 4 - Queue transitioncancel when animation status is idle. r=birtles
https://hg.mozilla.org/integration/autoland/rev/a1caae52e600
part 5 - Call the queueing events when canceling transition via Style or Script. r=birtles
https://hg.mozilla.org/integration/autoland/rev/b811ecefc8d3
part 6 - Queue CSS related event when setting null target effect. r=birtles
https://hg.mozilla.org/integration/autoland/rev/4bc7d59ee7ae
part 7 - Update legacy transition event tests. r=birtles
https://hg.mozilla.org/integration/autoland/rev/65dfda4abd3e
part 8 - Add transitioncancel tests. r=birtles
https://hg.mozilla.org/integration/autoland/rev/ab93ba4409d8
part 9 - Rename variable name of CSS-Transition tests. r=birtles
Keywords: dev-doc-needed
I have documented the event handler:
https://developer.mozilla.org/en-US/docs/Web/API/GlobalEventHandlers/ontransitioncancel

And made sure the corresponding event page is there:
https://developer.mozilla.org/en-US/docs/Web/Events/transitioncancel

I've also added a note to the Fx53 release notes:
https://developer.mozilla.org/en-US/Firefox/Releases/53#DOM_HTML_DOM

Let me know if that's OK. Thanks!
In this bug, I've added the unnecessary whitespace and din't align the variable definition. So I tidy up this code.

This fix is modifying white space only, so I use the r=whitespace-only in this patch.
Attachment #8861761 - Flags: review+
Pushed by mantaroh@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dd5867630919
part 10 - Remove unnecessary whitespace and align the colum of variable definition on ComputedTiming.h. r=whitespace-only DONTBUILD
Depends on: 1541767
You need to log in before you can comment on or make changes to this bug.