Closed Bug 1287983 Opened 8 years ago Closed 8 years ago

Implement transitionstart/transitionrun event

Categories

(Core :: CSS Parsing and Computation, defect)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox50 --- affected
firefox52 --- fixed

People

(Reporter: birtles, Assigned: mantaroh)

References

(
URL
)

Details

(Keywords: dev-doc-complete)

Attachments

(8 files, 2 obsolete files)

InvesigateAddingTransitionStartEvent
8.01 KB, patch
Details | Diff | Splinter Review
Bug 1287983 part 1 - Add transitionstart/transitionrun event handlers.
58 bytes, text/x-review-board-request
masayuki
: review+
smaug
: review+
Details
Bug 1287983 part 2 - Add transitionstart/transitionrun event handling.
58 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1287983 part 3 - Add transitionstart/transitionrun test.
58 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1287983 part 4 - Add transition event handler test for checking elapsed time.
58 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1287983 part 5 - Clarify the function name of creating transition.
58 bytes, text/x-review-board-request
hiro
: review+
Details
Bug 1287983 part 6 - Integrate ElapsedTimeToTimeStamp function to the Animation class.
58 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1287983 part 7 - Add negative delay tests for CSS-Transitions event.
58 bytes, text/x-review-board-request
birtles
: review+
Details 
It's specced in the very early CSS Transitions 2 ED[1], IE10 apparently implements it[2], and there has been some interest in implementing in Blink recently it seems[3].

The behavior seems fairly uncontroversial and there are many valid use cases for this (e.g. determining if a transition was actually triggered or not so you know if you should wait on transitionend or not) so we should probably implement it (after sending an appropriate intent to implement).

[1] https://drafts.csswg.org/css-transitions-2/#eventdef-transitionevent-transitionstart
[2] https://msdn.microsoft.com/library/dn632683(v=vs.85).aspx
[3] https://bugs.chromium.org/p/chromium/issues/detail?id=439056
I'm going to work this.
This patch is the code of investigation.
Assignee: nobody → mantaroh
Comment on attachment 8782317 [details]
Bug 1287983 part 1 - Add transitionstart/transitionrun event handlers.

https://reviewboard.mozilla.org/r/72488/#review70126

::: dom/events/EventNameList.h:970
(Diff revision 1)
>  EVENT(transitionend,
>        eTransitionEnd,
>        EventNameType_All,
>        eTransitionEventClass)
> +EVENT(transitionstart,
> +      eTransitionStart,
> +      EventNameType_All,
> +      eTransitionEventClass)

nit: looks nicer if you move transitionstart before transitionend.

::: widget/EventMessageList.h:334
(Diff revision 1)
>  NS_EVENT_MESSAGE(eTransitionEnd)
> +NS_EVENT_MESSAGE(eTransitionStart)
>  NS_EVENT_MESSAGE(eAnimationStart)
>  NS_EVENT_MESSAGE(eAnimationEnd)

nit: I like following order:

eTransitionStart
eTransitionEnd
eAnimationStart
eAnimationEnd
Attachment #8782317 - Flags: review?(masayuki) → review+
Comment on attachment 8782317 [details]
Bug 1287983 part 1 - Add transitionstart/transitionrun event handlers.

https://reviewboard.mozilla.org/r/72488/#review70126

Thanks Masayuki,

I addressed the event definition order.
Intent to implement: https://groups.google.com/forum/#!topic/mozilla.dev.platform/PR_AP6i-xIo

After looking into this, however, it seems like the spec behavior does not agree with the Edge behavior: https://github.com/w3c/csswg-drafts/issues/416

The spec says we fire the event when the transition is created, but Edge appears to fire it at the end of the delay (presumably to be consistent with CSS Animations): https://jsfiddle.net/jph4eo2o/
Comment on attachment 8782316 [details]
Bug 1287983 part 1 - Add transitionstart test.

https://reviewboard.mozilla.org/r/72486/#review70948

This is mostly fine but we'll have to wait until the spec issue is clarified. Clearing review request for now. Please re-request once the spec has been clarified and the following changes made.

Also, we don't have any tests here for the elapsedTime member transitionstart events so we need to add that.

::: layout/style/test/test_animations_event_order.html:1
(Diff revision 1)
> -<!doctype html>
> +o<!doctype html>

We probably don't need this change ;)

::: layout/style/test/test_animations_event_order.html:57
(Diff revision 1)
>    // Argument format:
> -  // Arguments     = ExpectedEvent*, desc
> +  // Arguments     = ExpectedEvent*, (evalFunction), desc
>    // ExpectedEvent =
>    //       [ target|animationName|transitionProperty, (pseudo,) message ]
> -  var expectedEvents  = args.slice(0, -1);
> +  // If you don't specify the evalFunction, evalFunction will be is() function.
> +  var expectedEvents  = args.slice(0, -2);
> +  var evalFunc        = args[args.length - 2];
>    var desc            = args[args.length - 1];
> +  if (Array.isArray(evalFunc)) {
> +    // Legacy parameter type.
> +    expectedEvents  = args.slice(0, -1);
> +    evalFunc = is;
> +  }

(For a completely new feature like this--as opposed to a change to an existing feature--it's probably fine to add the tests afterwards, especially when it's this difficult to add the tests and mark them as failing.)

::: layout/style/test/test_animations_event_order.html:471
(Diff revision 1)
>  advance_clock(0);
> +
> +checkEventOrder([ divs[1], 'transitionstart' ],
> +                [ divs[0], 'transitionstart' ],
> +                todo_is,
> +                'Sorting of transitionstart events by time');
> +
>  advance_clock(10000);
>  
>  checkEventOrder([ divs[1], 'transitionend' ],
>                  [ divs[0], 'transitionend' ],
>                  'Sorting of transitionend events by time');

This test is about testing transitions are sorted by time. We should make it do something like the following instead:

  advance_clock(0);
  advance_clock(10000);

  checkEventOrder([ divs[0], 'transitionstart' ],
                  [ divs[1], 'transitionend' ],
                  [ divs[1], 'transitionend' ],
                  [ divs[0], 'transitionend' ],
                  'Sorting of transition events by time');

I guess that will make it impossible to use todo_is so I think we should just forget about evalFunc and todo_is and make this test patch the last in the series.

::: layout/style/test/test_animations_event_order.html:505
(Diff revision 1)
> +
> +checkEventOrder([ divs[1], 'transitionstart' ],
> +                [ divs[0], 'transitionstart' ],
> +                todo_is,
> +                'Sorting of transitionstart events by time (including delay)');
> +
>  advance_clock(10000);
>  
>  checkEventOrder([ divs[1], 'transitionend' ],
>                  [ divs[0], 'transitionend' ],
>                  'Sorting of transitionend events by time (including delay)');

Why do the transitionstart events arrive in this order? I thought we were dispatching them when they are created and when that time is the same it is document position. So shouldn't div[0] come first?

Also, as with the previous test, I think we should test the start and end events together.

::: layout/style/test/test_animations_event_order.html:534
(Diff revision 1)
> +
> +checkEventOrder([ 'margin-left', 'transitionstart' ],
> +                [ 'opacity', 'transitionstart' ],
> +                todo_is,
> +                'Sorting of transitionstart events by transition-property');
> +
>  advance_clock(10000);
>  
>  checkEventOrder([ 'margin-left', 'transitionend' ],
>                  [ 'opacity', 'transitionend' ],
>                  'Sorting of transitionend events by transition-property');

I think we can test these together as well (to make sure we sort first by time, and then by transition property).

::: layout/style/test/test_animations_event_order.html:567
(Diff revision 1)
> +
> +checkEventOrder([ divs[0], 'transitionstart' ],
> +                [ divs[1], 'transitionstart' ],
> +                todo_is,
> +                'Transition events are sorted by document position first, ' +
> +                'before transition-property');
> +
>  advance_clock(10000);
>  
>  checkEventOrder([ divs[0], 'transitionend' ],
>                  [ divs[1], 'transitionend' ],
>                  'Transition events are sorted by document position first, ' +
>                  'before transition-property');

Once again, let's test these together.

::: layout/style/test/test_animations_event_order.html:598
(Diff revision 1)
> +
> +checkEventOrder([ 'opacity', 'transitionstart' ],
> +                [ 'margin-left', 'transitionstart' ],
> +                todo_is,
> +                'Transition events are sorted by time first, before ' +
> +                'transition-property');
> +
>  advance_clock(10000);
>  
>  checkEventOrder([ 'opacity', 'transitionend' ],
>                  [ 'margin-left', 'transitionend' ],
>                  'Transition events are sorted by time first, before ' +
>                  'transition-property');

Likewise here.
Attachment #8782316 - Flags: review?(bbirtles)
Comment on attachment 8782318 [details]
Bug 1287983 part 2 - Add transitionstart/transitionrun event handling.

https://reviewboard.mozilla.org/r/72490/#review70954

This is mostly good except we need to set elapsedTime correctly. Also, I should probably look again once the spec issue has been resolved.

::: layout/style/nsTransitionManager.h:246
(Diff revision 2)
>    //    constructor.
>    //
>    // For (b) and (c) the owning element will return !IsSet().
>    OwningElementRef mOwningElement;
>  
> -  bool mWasFinishedOnLastTick;
> +  bool mWasFinishedOnLastTick, mWasStartedOnLastTick;

Let's put these on separate lines. Better still, how about replacing these two variables with a single PlayState member for mPlayStateLastTick?

::: layout/style/nsTransitionManager.cpp:187
(Diff revision 2)
> +  mWasStartedOnLastTick = playState == AnimationPlayState::Running;
>    bool newlyFinished = !mWasFinishedOnLastTick &&
>                         playState == AnimationPlayState::Finished;
>    mWasFinishedOnLastTick = playState == AnimationPlayState::Finished;
>  
> -  if (!newlyFinished || !mEffect || !mOwningElement.IsSet()) {
> +  // Bug 1287983 : We need to add transitionstart event here.

I don't understand why we need this comment?

::: layout/style/nsTransitionManager.cpp:188
(Diff revision 2)
> +  if (newlyFinished && mEffect && mOwningElement.IsSet()) {
> +    message = eTransitionEnd;
> +  } else if (newlyStarted && mEffect && mOwningElement.IsSet()) {
> +    message = eTransitionStart;
> +  } else {
>      return;
>    }

I think you could probably simplify this as:

  if (playState == mPlayStateOnLastTick) {
    return;
  }
  mPlayStateOnLastTick = playState;

  if (!mEffect || !mOwningElement.IsSet()) {
    return;
  }

  switch (playState) {
    case AnimationPlayState::Running:
      message = eTransitionStart;
      break;
    case AnimationPlayState::Finished:
      message = eTransitionEnd;
      break;
    default:
      return;
  }

But perhaps we should wait to see how the spec turns out first.

::: layout/style/nsTransitionManager.cpp:211
(Diff revision 2)
>    nsTransitionManager* manager = presContext->TransitionManager();
>    manager->QueueEvent(TransitionEventInfo(owningElement, owningPseudoType,
>                                            TransitionProperty(),
>                                            mEffect->GetComputedTiming()
>                                              .mDuration,
>                                            AnimationTimeToTimeStamp(EffectEnd()),

This will need to change I think.
Attachment #8782318 - Flags: review?(bbirtles)
Oh, also I realized that check for a transition to the 'running' state doesn't work. If we pause the transition and then resume it, we shouldn't dispatch a 'transitionstart' event. This probably needs to be based on phases rather than play states--although that too will depend on what we decide to do about delays.
According the specification changes of transitionstart event, we also need to implement transitionrun event.
Summary: Implement transitionstart event → Implement transitionstart/transitionrun event
'RESOLVED: move the four transition/animation new events to the level 1 specs'
https://logs.csswg.org/irc.w3.org/css/2016-09-19/#e722575
Hi Brian,

I'm wondering if we are on the same page about transitionrun.
Transition spec said that 'The transitionrun event occurs when a transition is created'.
Is it mean that we should notify this event only once when create transition? [1]

[1] https://dxr.mozilla.org/mozilla-central/source/layout/style/nsTransitionManager.cpp?q=%22new CSSTransition%22&redirect_type=single#795
Flags: needinfo?(bbirtles)
I discussed this case with Brian.
We will need to fire the transitionrun when a transition is created.

For example: If we change transition-property value, the previous transition will cancel, and a new transition will create.  And the previous transition will fire the transitioncancel, and a new transition will fire the transitionrun event.
Flags: needinfo?(bbirtles)
Attachment #8782316 - Attachment is obsolete: true
Comment on attachment 8782317 [details]
Bug 1287983 part 1 - Add transitionstart/transitionrun event handlers.

Hi Masayuki,

I added the transitionrun event handler.
This additional event handler is for notifying the creation of transition.
We have discussed on Github about this event handler.(For detail, see comment 10).

Would you please reconfirm this patch again?
Attachment #8782317 - Flags: review+ → review?(masayuki)
Comment on attachment 8782317 [details]
Bug 1287983 part 1 - Add transitionstart/transitionrun event handlers.

https://reviewboard.mozilla.org/r/72488/#review79660

Looks good at least the code level, not checking the spec, though.
Attachment #8782317 - Flags: review?(masayuki) → review+
It might be worth renaming ConsiderStarrrtingTransition to ConsiderRunningTransition or ConsiderCreatingTransition to avoid confusion?  RestyleManager::TryStartingTransition too?
Comment on attachment 8782318 [details]
Bug 1287983 part 2 - Add transitionstart/transitionrun event handling.

https://reviewboard.mozilla.org/r/72490/#review80194

This is getting close, but needs some more work I think.

::: layout/style/nsTransitionManager.h:267
(Diff revision 3)
>    // using the Web Animations API.
>    nsCSSPropertyID mTransitionProperty;
>    StyleAnimationValue mTransitionToValue;
> +
> +  ComputedTiming::AnimationPhase mPhaseOnLastTick;
> +  bool mNeedFireTransitionrun;

See my comments below, but I think we probably don't need this.

::: layout/style/nsTransitionManager.h:295
(Diff revision 3)
>    TransitionEventInfo(dom::Element* aElement,
>                        CSSPseudoElementType aPseudoType,
>                        nsCSSPropertyID aProperty,
>                        StickyTimeDuration aDuration,
>                        const TimeStamp& aTimeStamp,
> -                      dom::Animation* aAnimation)
> +                      dom::Animation* aAnimation,
> +                      EventMessage aMessage)

Let's put |aMessage| after |aPseudoType| to match the order in AnimationEventInfo (and because it is more important than the other fields)

(You'll need to change the order of the initializer lists after doing this.)

::: layout/style/nsTransitionManager.cpp:197
(Diff revision 3)
> -  AnimationPlayState playState = PlayState();
> -  bool newlyFinished = !mWasFinishedOnLastTick &&
> -                       playState == AnimationPlayState::Finished;
> +  if (!mEffect || !mOwningElement.IsSet()) {
> +    return;
> +  }
> -  mWasFinishedOnLastTick = playState == AnimationPlayState::Finished;
>  
> -  if (!newlyFinished || !mEffect || !mOwningElement.IsSet()) {
> +  ComputedTiming timing = mEffect->GetComputedTiming();

Nit: I think |computedTiming| would be more clear.

::: layout/style/nsTransitionManager.cpp:240
(Diff revision 3)
> +    if (needFireTransitionstart) {
> +      events.AppendElement(
> +        TransitionEventSet(eTransitionStart,
> +                           intervalStartTime,
> +                           AnimationTimeToTimeStamp(intervalStartTime)));
> +    }

I don't think transitionstart always corresponds to interval start, right? For example, if we're playing backwards the elapsedTime should be interval end, right?

::: layout/style/nsTransitionManager.cpp:255
(Diff revision 3)
> +                        timing.mActiveDuration),
> +               StickyTimeDuration(0));
> +    events.AppendElement(
> +      TransitionEventSet(eTransitionEnd,
> +                         intervalEndTime,
> +                         AnimationTimeToTimeStamp(EffectEnd())));

Is there a reason we need to use EffectEnd() as the timestamp but intervalEndTime as the time? If not, I think we could drop the whole TransitionEventSet type and just use a pair like in nsAnimationManager.

::: layout/style/nsTransitionManager.cpp:263
(Diff revision 3)
>    nsPresContext* presContext = mOwningElement.GetRenderedPresContext();
>    if (!presContext) {
>      return;
>    }

Move this early return to the top of the function.

::: layout/style/nsTransitionManager.cpp:263
(Diff revision 3)
>    nsPresContext* presContext = mOwningElement.GetRenderedPresContext();
>    if (!presContext) {
>      return;
>    }
>  
>    nsTransitionManager* manager = presContext->TransitionManager();
> +  for (const TransitionEventSet event : events) {

Drop the blank line between the check for a pres context and getting the manager, and add it before the loop. Conceptually we are only getting the pres context so we can get the manager from it, so this belongs together as a single block.

::: layout/style/nsTransitionManager.cpp:269
(Diff revision 3)
>    if (!presContext) {
>      return;
>    }
>  
>    nsTransitionManager* manager = presContext->TransitionManager();
> +  for (const TransitionEventSet event : events) {

I think you're missing a '&' here?

::: layout/style/nsTransitionManager.cpp:277
(Diff revision 3)
> -                                          mEffect->GetComputedTiming()
> -                                            .mDuration,
> -                                          AnimationTimeToTimeStamp(EffectEnd()),
> -                                          this));
> +                                            event.time,
> +                                            event.timeStamp,
> +                                            this,
> +                                            event.message));
> +  }
>  }

I have a lot of other detailed feedback on this method but perhaps it is easier if I explain using pseudo code. I think we could make this more simple and clear using something like the following:

  // Somewhere at the top of the file...
  using mozilla::ComputedTiming::AnimationPhase;
  // (Not sure about the syntax here, might need a type alias
  // e.g. using Phase = mozilla::ComputedTiming::AnimationPhase;)

  // Creating a single zero duration object makes the code a little
  // simpler.
  const StickyTimeDuration zeroDuration;

  // The following are not so expensive to calculate so we may as well
  // just do them up front.
  StickyTimeDuration intervalStartTime =
    std::max(
      std::min(StickyTimeDuration(-mEffect->SpecifiedTiming().mDelay),
              timing.mActiveDuration),
      zeroDuration);

  StickyTimeDuration intervalEndTime =
    std::max(
      std::min(timing.mEndTime - mEffect->SpecifiedTiming().mDelay,
              timing.mActiveDuration),
      zeroDuration);

  AutoTArray<TransitionEventSet, 2> events;

  // Rather than set a member variable to indicate we need a
  // transitionrun event, can we just detect a transition from the null
  // phase?
  if (mPhaseOnLastTick == AnimationPhase::Null) {
    events.AppendElement({ eTransitionRun, intervalStartTime });
  }

  // I think we don't want to dispatch the other events, however, until
  // we've finished pending.
  if (mPendingState != PendingState::NotPending) {
    return;
  }

  mPhaseOnLastTick = phase;

  // I think a switch block in this case is a little easier to follow
  // than the complex conditions in the patch.
  switch (phase) {
    case AnimationPhase::Before:
      // The following could be a switch as well but it might be a
      // little shorter and easier to read as if-else.
      if (mPhaseOnLastTick == AnimationPhase::Active) {
        // This is pseudo-syntax here... I don't remember the exact
        // circumstances where the following syntax works but I'm
        // guessing it doesn't work here.
        events.AppendElement({ eTransitionEnd, intervalEndTime });
      } else if (mPhaseOnLastTick == AnimationPhase::After) {
        events.AppendElement({ eTransitionStart, intervalEndTime });
        events.AppendElement({ eTransitionEnd, intervalStartTime });
      }
      break;

    case ComputedTiming::AnimationPhase::Active:
      if (mPhaseOnLastTick == AnimationPhase::Before) {
        events.AppendElement({ eTransitionStart, intervalStartTime });
      } else if (mPhaseOnLastTick == AnimationPhase::After) {
        events.AppendElement({ eTransitionStart, intervalEndTime });
      }
      break;

    case ComputedTiming::AnimationPhase::After:
      if (mPhaseOnLastTick == AnimationPhase::Before) {
        events.AppendElement({ eTransitionStart, intervalStartTime });
        events.AppendElement({ eTransitionEnd, intervalEndTime });
      } else if (mPhaseOnLastTick == AnimationPhase::Active) {
        events.AppendElement({ eTransitionEnd, intervalEndTime });
      }
      break;
  }

  ....

  for (const EventPair& pair : events) {
    manager->QueueEvent(
               TransitionEventInfo(owningElement,
                                   owningPseudoType,
                                   pair.first(),
                                   TransitionPropety(),
                                   pair.second(),
                                   ElapsedTimeToTimeStamp(pair.second()),
                                   this));
  }

::: layout/style/nsTransitionManager.cpp:879
(Diff revision 3)
>    animation->SetTimelineNoUpdate(timeline);
>    animation->SetCreationSequence(
>      mPresContext->RestyleManager()->AsGecko()->GetAnimationGeneration());
>    animation->SetEffectFromStyle(pt);
>    animation->PlayFromStyle();
> +  animation->RequestFireTransitionrunEvent();

I think we don't need this.
Attachment #8782318 - Flags: review?(bbirtles)
Comment on attachment 8794713 [details]
Bug 1287983 part 3 - Add transitionstart/transitionrun test.

https://reviewboard.mozilla.org/r/81044/#review80222

Looks good. We need tests for elapsedTime and negative playback rate though. Please add those tests before landing.

::: layout/style/test/test_animations_event_order.html:62
(Diff revision 1)
>  function checkEventOrder(...args) {
>    // Argument format:
>    // Arguments     = ExpectedEvent*, desc
>    // ExpectedEvent =
>    //       [ target|animationName|transitionProperty, (pseudo,) message ]
> +  // If you don't specify the evalFunction, evalFunction will be is() function.

Is this comment still needed?
Attachment #8794713 - Flags: review?(bbirtles) → review+
Comment on attachment 8782318 [details]
Bug 1287983 part 2 - Add transitionstart/transitionrun event handling.

https://reviewboard.mozilla.org/r/72490/#review80194

Thanks Brian,
I updated the event handling implementation.
Could you please review it?
Hi hiro,

(In reply to Hiroyuki Ikezoe (:hiro) from comment #24)
> It might be worth renaming ConsiderStarrrtingTransition to
> ConsiderRunningTransition or ConsiderCreatingTransition to avoid confusion? 
> RestyleManager::TryStartingTransition too?
Of course.
I prefer the **CreatingTransition, because this function will create the CSSTransition object. This patch is modification of this function name.
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #33)
> Hi hiro,
> 
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #24)
> > It might be worth renaming ConsiderStarrrtingTransition to
> > ConsiderRunningTransition or ConsiderCreatingTransition to avoid confusion? 
> > RestyleManager::TryStartingTransition too?
> Of course.
> I prefer the **CreatingTransition, because this function will create the
> CSSTransition object. This patch is modification of this function name.

Yes, but 'creating' may been seen that it does not start/run the transition.  

I found some comment using 'initiate'.  http://hg.mozilla.org/mozilla-central/file/7c576fe3279d/layout/base/RestyleManager.cpp#l1256 and dom/animation/test/css-transitions/file_animation-currenttime.html.
What about it?  Though I am fine with 'create' if you prefer it to 'initiate'.  As you know, an important thing is that we should not use 'start' here because it will be really confusing.
Comment on attachment 8782318 [details]
Bug 1287983 part 2 - Add transitionstart/transitionrun event handling.

https://reviewboard.mozilla.org/r/72490/#review81604

This is nearly there! I'd like to know about why we have the extra condition in ElapsedTimeToTimeStamp, however. Also, I think we want to dispatch transitionrun while pending.

Can we split the part where we move ElapsedTimeToTimeStamp to Animation into a separate patch?

::: dom/animation/Animation.h:387
(Diff revision 4)
> +  TimeStamp ElapsedTimeToTimeStamp(const StickyTimeDuration&
> +                                   aElapsedTime) const
> +  {
> +    if (GetCurrentTime().Value() < mEffect->SpecifiedTiming().mDelay) {
> +      return AnimationTimeToTimeStamp(aElapsedTime);
> +    }
> +    return AnimationTimeToTimeStamp(aElapsedTime +
> +                                    mEffect->SpecifiedTiming().mDelay);
> +  }

I don't think this is so simple or performance critical that it needs to be in the header file. Let's put it in the .cpp file.

Also, let's not wrap the line between the parameter type and name.

And this should probably go just after AnimationTimeToTimeStamp.

Finally, why do we behave differently when the current time is less than the delay? The result should be independent of the current time.

::: layout/style/nsTransitionManager.h:260
(Diff revision 4)
> +
> +  ComputedTiming::AnimationPhase mPhaseOnLastTick;

(I think this could just go up where mWasFinishedOnLastTick was.)

::: layout/style/nsTransitionManager.cpp:51
(Diff revision 4)
> +namespace {
> +
> +typedef Pair<EventMessage, StickyTimeDuration> EventPair;
> +
> +} // anonymous namespace

Maybe drop the blank lines here. (It makes sense in nsAnimationManager.cpp because we have a big comment above the typedef but here it just makes it harder to read.)

::: layout/style/nsTransitionManager.cpp:183
(Diff revision 4)
> -  AnimationPlayState playState = PlayState();
> -  bool newlyFinished = !mWasFinishedOnLastTick &&
> -                       playState == AnimationPlayState::Finished;
> +  if (!mEffect ||
> +      !mOwningElement.IsSet() ||
> +      mPendingState != PendingState::NotPending) {
> -  mWasFinishedOnLastTick = playState == AnimationPlayState::Finished;
> -
> -  if (!newlyFinished || !mEffect || !mOwningElement.IsSet()) {
>      return;
>    }

I think we want to dispatch the transitionrun event while we're still pending, right? i.e. I think the author expects to get the event on the frame immediately after triggering a transition, not the following frame.

::: layout/style/nsTransitionManager.cpp:215
(Diff revision 4)
> +  ComputedTiming::AnimationPhase phase = computedTiming.mPhase;
> +  switch (phase) {

Do we need this local variable? It doesn't seem like it?

::: layout/style/nsTransitionManager.cpp:241
(Diff revision 4)
> +      if (mPhaseOnLastTick == AnimationPhase::Null ||
> +          mPhaseOnLastTick == AnimationPhase::Before) {
> +        events.AppendElement(EventPair(eTransitionStart, intervalStartTime));
> +        events.AppendElement(EventPair(eTransitionEnd, intervalEndTime));
> +      } else if (mPhaseOnLastTick == AnimationPhase::Active) {
> +        events.AppendElement(EventPair( eTransitionEnd, intervalEndTime));

Nit: stray space here

::: layout/style/nsTransitionManager.cpp:244
(Diff revision 4)
> +    case AnimationPhase::Null:
> +    default:
> +      break;

I don't think we need this. Is there some warning we're getting if we don't specify this?
Attachment #8782318 - Flags: review?(bbirtles)
Comment on attachment 8797036 [details]
Bug 1287983 part 4 - Add transition event handler test for checking elapsed time.

https://reviewboard.mozilla.org/r/82682/#review81606

This looks good, but feels a little bit random. I think there are probably 9 cases we could test:

1) idle → active with playback rate > 0: animation start with interval start
2) idle → active with playback rate == 0: animation start with interval start
3) idle → active with playback rate < 0: animation end with interval end
4) before → active: animationstart with interval start
5) after → active: animationstart with interval end
6) before → after: animationstart with interval start, animationend with interval end
7) after → before: animationstart with interval end, animationend with interval start
8) active → before: animationend with interval start
9) active → after: animationend with interval end

Would it be possible to write 9 short tests that just check each of these?

Perhaps we could add a test or two that also checks the calculation of interval start / interval end with various combinations of negative delays.

::: dom/animation/test/css-transitions/file_csstransition-events.html:12
(Diff revision 1)
> +function TransitionEventHandler(target) {
> +  this.target = target;
> +  this.init();
> +}
> +TransitionEventHandler.prototype = {
> +  receivedRunTime: undefined,
> +  receivedStartTime: undefined,
> +  receivedEndTime: undefined,
> +
> +  init: function() {
> +    this.target.ontransitionrun = (function(self) {
> +      return function(evt){ self.receivedRunTime = evt.elapsedTime; };
> +    })(this);
> +    this.target.ontransitionstart = (function(self) {
> +      return function(evt){ self.receivedStartTime = evt.elapsedTime; };
> +    })(this);
> +    this.target.ontransitionend = (function(self) {
> +      return function(evt){ self.receivedEndTime = evt.elapsedTime; };
> +    })(this);
> +  },
> +
> +  clearReceivedEvent: function() {
> +    this.receivedRunTime = undefined;
> +    this.receivedStartTime = undefined;
> +    this.receivedEndTime = undefined;
> +  }
> +};

I'm no Javascript whizz but couldn't you write this more simply as something like:

  function TransitionEventHandler(target) {
    this.target = target;

    // Register event handlers
    target.ontransitionrun = function() {
      this.receivedRunTime = evt.elapsedTime;
    }.bind(this);
    target.ontransitionstart = function() {
      this.receivedStartTime = evt.elapsedTime;
    }.bind(this);
    target.ontransitionend = function() {
      this.receivedEndTime = evt.elapsedTime;
    }.bind(this);
  }

  TransitionEventHandler.prototype.clear = function() {
    this.receivedRunTime = undefined;
    this.receivedStartTime = undefined;
    this.receivedEndTime = undefined;
  };

::: dom/animation/test/css-transitions/file_csstransition-events.html:46
(Diff revision 1)
> +  var div = addDiv(t, { style: 'margin-left: 0px' });
> +  var watcher = new EventWatcher(t, div, CSS_TRANSITION_EVENTS);
> +  var handler = new TransitionEventHandler(div);
> +  flushComputedStyle(div);
> +
> +  div.style.transition = 'margin-left 1s 10s';

Let's make these times longer just to be safe (otherwise we might get the transitionrun/start/end events all happening in the one frame when under load). e.g. 100s and 100s

::: dom/animation/test/css-transitions/file_csstransition-events.html:52
(Diff revision 1)
> +  div.style.marginLeft = '1000px';
> +  flushComputedStyle(div);
> +  var animation = div.getAnimations()[0];
> +
> +  return watcher.wait_for('transitionrun').then(function() {
> +    assert_equals(handler.receivedRunTime, 0.0);

Perhaps add a message here so that it's easier to see what when wrong from the logs if this fails.

e.g. 'transitionrun elapsedTime should be zero'

::: dom/animation/test/css-transitions/file_csstransition-events.html:56
(Diff revision 1)
> +  return watcher.wait_for('transitionrun').then(function() {
> +    assert_equals(handler.receivedRunTime, 0.0);
> +    handler.clearReceivedEvent();
> +
> +    // Seek to the after phase.
> +    animation.currentTime = animation.effect.getComputedTiming().endTime;

(Nit: Just call animation.finish()?)

::: dom/animation/test/css-transitions/file_csstransition-events.html:64
(Diff revision 1)
> +    assert_equals(handler.receivedStartTime, 0.0);
> +    handler.clearReceivedEvent();
> +    animation.pause();
> +
> +    // Seek to the active phase.
> +    animation.currentTime = animation.effect.getComputedTiming().delay + 500;

No need for the + 500 I think?

::: dom/animation/test/css-transitions/file_csstransition-events.html:71
(Diff revision 1)
> +
> +

Nit: one blank line here is probably enough

::: dom/animation/test/css-transitions/file_csstransition-events.html:79
(Diff revision 1)
> +  var div = addDiv(t, { style: 'margin-left: 0px' });
> +  var watcher = new EventWatcher(t, div, CSS_TRANSITION_EVENTS);
> +  var handler = new TransitionEventHandler(div);
> +  flushComputedStyle(div);
> +
> +  div.style.transition = 'margin-left 1s 1s';

As before, let's make these 100s 100s

::: dom/animation/test/css-transitions/file_csstransition-events.html:91
(Diff revision 1)
> +
> +    // Seek to the after phase.
> +    animation.currentTime = animation.effect.getComputedTiming().endTime;
> +    return watcher.wait_for(['transitionstart', 'transitionend']);
> +  }).then(function() {
> +    assert_equals(handler.receivedStartTime, 0.0);

We should test receivedEndTime here I think (since this is a test for transitionstart/end)

::: dom/animation/test/css-transitions/file_csstransition-events.html:102
(Diff revision 1)
> +    return watcher.wait_for(['transitionstart', 'transitionend']);
> +  }).then(function() {
> +    assert_equals(handler.receivedStartTime, 1.0);
> +    assert_equals(handler.receivedEndTime, 0.0);
> +  });
> +}, 'Receive transitionstart/end when seeking to after phase from before phase');

This doesn't really match what the test does. It seeks before -> after, then back to before. See comment at start.

::: dom/animation/test/css-transitions/file_csstransition-events.html:110
(Diff revision 1)
> +  var div = addDiv(t, { style: 'margin-left: 0px' });
> +  var watcher = new EventWatcher(t, div, CSS_TRANSITION_EVENTS);
> +  var handler = new TransitionEventHandler(div);
> +  flushComputedStyle(div);
> +
> +  div.style.transition = 'margin-left 1s 1s';

100s 100s

::: dom/animation/test/css-transitions/file_csstransition-events.html:119
(Diff revision 1)
> +   animation.currentTime =
> +     animation.effect.getComputedTiming().delay + 500;

I think we don't need the + 500.

::: dom/animation/test/css-transitions/file_csstransition-events.html:138
(Diff revision 1)
> +}, 'Receive transitionend when seeking to before phase from active phase');
> +
> +done();
> +</script>
> +</body>
> +<

Stray < here
Attachment #8797036 - Flags: review?(bbirtles)
Comment on attachment 8782318 [details]
Bug 1287983 part 2 - Add transitionstart/transitionrun event handling.

https://reviewboard.mozilla.org/r/72490/#review81604

Thanks, Brian,

I thought that this code need for event ordering.
For example if we have two transition as follow:
 div0.style.transition = "margin-left 100s 10s";
 div1.style.transition = "margin-left 100s  5s";
 
Each interval start and interval end will be as follow:
 div0: interval start = 0 / interval end = 100
 div1: interval start = 0 / interval end = 100

We expect that the order of transitionrun will be div0->div1. But the result is div1->div0 because we use the formula which including the start delay. [1]
(However when to consider the transitionend, I think that we need the start delay to this formula. )
[1] https://dxr.mozilla.org/mozilla-central/rev/42c95d88aaaa7c2eca1d278399421d437441ac4d/layout/style/nsAnimationManager.cpp#288

So I think that we will need to separate elapsedTime and TimeStamp of ordering.
As you mentioned, the result should be independent of the current time. So I'll simplify this calculation.
Comment on attachment 8782318 [details]
Bug 1287983 part 2 - Add transitionstart/transitionrun event handling.

https://reviewboard.mozilla.org/r/72490/#review81604

> I don't think we need this. Is there some warning we're getting if we don't specify this?

This line need in order to prevent error of 'Werror=switch' flag.
https://treeherder.mozilla.org/logviewer.html#?job_id=28383552&repo=try#L11295
Comment on attachment 8797037 [details]
Bug 1287983 part 5 - Clarify the function name of creating transition.

https://reviewboard.mozilla.org/r/82684/#review81288

There are couple of phrase 'start transition',  it would be nice to change them especially in cpp files, it apparently should be 'initiate' now, right?

http://searchfox.org/mozilla-central/search?q=start+transition&case=true&regexp=false&path=

::: layout/base/RestyleManager.h:220
(Diff revision 1)
>     *
>     * For the pseudo-elements, aContent must be the anonymous content
>     * that we're creating for that pseudo-element, not the real element.
>     */
>    static bool
> -  TryStartingTransition(nsPresContext* aPresContext, nsIContent* aContent,
> +  TryCreatingTransition(nsPresContext* aPresContext, nsIContent* aContent,

We should also change the comment of this function. There is "Try starting a transaction..".
Attachment #8797037 - Flags: review?(hiikezoe) → review+
Comment on attachment 8782318 [details]
Bug 1287983 part 2 - Add transitionstart/transitionrun event handling.

https://reviewboard.mozilla.org/r/72490/#review82402

I thought we were going to split the code where we move ElapsedTimeToTimeStamp into a separate patch (comment 35)?

::: layout/style/nsTransitionManager.cpp:207
(Diff revision 5)
> +  AutoTArray<EventPair, 3> events;
> +  if (mPhaseOnLastTick == AnimationPhase::Null) {
> +    events.AppendElement(EventPair(eTransitionRun, intervalStartTime));
> +  }
> +
> +  switch (computedTiming.mPhase) {
> +    case AnimationPhase::Before:
> +      if (mPhaseOnLastTick == AnimationPhase::Active) {
> +        events.AppendElement(EventPair(eTransitionEnd, intervalStartTime));
> +      } else if(mPhaseOnLastTick == AnimationPhase::After) {
> +        events.AppendElement(EventPair(eTransitionStart, intervalEndTime));
> +        events.AppendElement(EventPair(eTransitionEnd, intervalStartTime));
> +      }
> +      break;

As per the outline in comment 25, I think we want to dispatch transitionrun while we're pending (so the author gets the notification that a transition was created on the very next tick) but we don't want to dispatch transitionstart etc. until after we have finished pending (to be consistent with what we do with animation events).

Maybe this whole switch needs to go inside an if() block that checks the pending state?

::: layout/style/nsTransitionManager.cpp:242
(Diff revision 5)
> +    case AnimationPhase::Null:
> +      break;

Nit: Blank like before this branch
Attachment #8782318 - Flags: review?(bbirtles)
Comment on attachment 8782318 [details]
Bug 1287983 part 2 - Add transitionstart/transitionrun event handling.

https://reviewboard.mozilla.org/r/72490/#review82404

Oops, that was supposed to be "r=me with the following comments addressed".
Attachment #8782318 - Flags: review+
Comment on attachment 8797036 [details]
Bug 1287983 part 4 - Add transition event handler test for checking elapsed time.

Oh sorry,
This patch is wrong. I've mistyped transition property name when replacing the property name after local test.
Attachment #8797036 - Flags: review?(bbirtles)
Comment on attachment 8798707 [details]
Bug 1287983 part 6 - Integrate ElapsedTimeToTimeStamp function to the Animation class.

https://reviewboard.mozilla.org/r/84132/#review82714
Attachment #8798707 - Flags: review?(bbirtles) → review+
Comment on attachment 8797036 [details]
Bug 1287983 part 4 - Add transition event handler test for checking elapsed time.

https://reviewboard.mozilla.org/r/82682/#review82716

This is looking really really good. However, I'd like to see it once more with the simplifications mentioned.

::: dom/animation/test/css-transitions/file_csstransition-events.html:26
(Diff revision 3)
> + *
> + * So we need to this object when checking multiple events parameter.
> + */
> +function TransitionEventHandler(target) {
> +  this.target = target;
> +  this.target.ontransitionrun = function(evt){

Nit: Space between "function(evt)" and "{"

::: dom/animation/test/css-transitions/file_csstransition-events.html:36
(Diff revision 3)
> +  }.bind(this);
> +  this.target.ontransitionend = function(evt) {
> +    this.receivedEndTime = evt.elapsedTime;
> +  }.bind(this);
> +}
> +TransitionEventHandler.prototype.clear = function() {

Nit: add a blank line before this.

::: dom/animation/test/css-transitions/file_csstransition-events.html:51
(Diff revision 3)
> +  return watcher.wait_for('transitionrun').then(function(evt) {
> +    assert_equals(evt.elapsedTime, 0.0,
> +                  'transitionrun elapsedTime should be zero');
> +    return animation.ready;
> +  }).then(function() {
> +    // Seek to the active phase.
> +    animation.currentTime = 100 * MS_PER_SEC;
> +    return watcher.wait_for('transitionstart');

I think if we wait for 'transitionrun' and animation.ready before seeking, we will end up doing a transition from *before* to active, not *idle* to active. I think we should just seek to 100 immediately after getting |animation|.

::: dom/animation/test/css-transitions/file_csstransition-events.html:63
(Diff revision 3)
> +}, 'Receive animationstart when seeking to active from idle with ' +
> +   'playbackRate > 0');

Sorry, this was my mistake. I wrote 'animationstart' and 'animationend' in comment 36. In all these comments we should replace 'animationstart' with 'transitionstart', likewise 'animationend'.

::: dom/animation/test/css-transitions/file_csstransition-events.html:76
(Diff revision 3)
> +  return watcher.wait_for('transitionrun').then(function(evt) {
> +    assert_equals(evt.elapsedTime, 0.0,
> +                  'transitionrun elapsedTime should be zero');
> +    return animation.ready;
> +  }).then(function() {
> +    // Seek to the active phase.
> +    animation.currentTime = 100 * MS_PER_SEC;

Likewise here, we want to test the transition from idle so we should not wait on 'transitionrun' and animation.ready here.

::: dom/animation/test/css-transitions/file_csstransition-events.html:98
(Diff revision 3)
> +  var animation = div.getAnimations()[0];
> +  animation.pause();
> +
> +  return watcher.wait_for('transitionrun')
> +    .then(animation.ready)
> +    .then(function() {
> +      animation.playbackRate = -1;
> +      animation.currentTime = 150 * MS_PER_SEC;

Likewise here, we want to test the transition from idle so we should not wait on 'transitionrun' and animation.ready here.

::: dom/animation/test/css-transitions/file_csstransition-events.html:145
(Diff revision 3)
> +  var animation = div.getAnimations()[0];
> +
> +  return watcher.wait_for('transitionrun').then(function(evt) {
> +    assert_equals(evt.elapsedTime, 0.0,
> +                  'transitionrun elapsedTime should be zero');
> +    return animation.ready;
> +  }).then(function() {
> +    // Seek to the after phase.
> +    animation.finish();
> +    return watcher.wait_for(['transitionstart', 'transitionend']);
> +  }).then(function() {
> +    // Seek to the active phase.
> +    animation.currentTime = 100 * MS_PER_SEC;
> +    return watcher.wait_for('transitionstart');

This test is for checking the active -> after transition. I think we can make this test simpler by just doing something like:

  var animation = div.getAnimations()[0];
  // Seek to after phase
  anim.finish();

  return Promise.all([watcher.wait_for('transitionrun'), animation.ready])
  .then(function() {
    // Seek to active phase
    animation.currentTime = 100 * MS_PER_SEC;
    return watcher.wait_for('transitionstart');
  }).then(function(evt) {
    assert_equals(evt.elapsedTime, 100.0);
  });

The same goes for the rest of the tests. We should try to make each one as simple and focused as possible. Just try to test one transition.

(Otherwise we don't really need the next test--we have already covered the 'before' -> 'after' transition here.)
Attachment #8797036 - Flags: review?(bbirtles)
These events have now been added to CSS Transitions level 1: https://drafts.csswg.org/css-transitions-1/#event-transitionevent
Also, CSS Transitions level 2 has been updated with a generic description of how these events should be queued: https://drafts.csswg.org/css-transitions-2/#event-dispatch
Comment on attachment 8782318 [details]
Bug 1287983 part 2 - Add transitionstart/transitionrun event handling.

https://reviewboard.mozilla.org/r/72490/#review83960

Clearing r+ for now since, as discussed in person, we need to take extra care of the pending state (particularly when e10s is enabled).
Attachment #8782318 - Flags: review+
Attachment #8782318 - Flags: review?(bbirtles)
Attachment #8797036 - Flags: review?(bbirtles)
(In reply to Brian Birtles (:birtles) from comment #55)
> These events have now been added to CSS Transitions level 1:
> https://drafts.csswg.org/css-transitions-1/#event-transitionevent
> Also, CSS Transitions level 2 has been updated with a generic description of
> how these events should be queued:
> https://drafts.csswg.org/css-transitions-2/#event-dispatch
Thanks.
I fixed event handling based on CSS-Transition L2 spec.
So could you please review it?
Comment on attachment 8782318 [details]
Bug 1287983 part 2 - Add transitionstart/transitionrun event handling.

https://reviewboard.mozilla.org/r/72490/#review84932

r=me with the following comments addressed

::: layout/style/nsTransitionManager.h:234
(Diff revision 8)
>  
> +  // Converts an TransitionEvent's elapsedTime value to an equivalent TimeStamp
> +  // that can be used to sort events by when they occurred.
> +  TimeStamp ElapsedTimeToTimeStamp(const StickyTimeDuration& aElapsedTime) const;
> +
> -  // The (pseudo-)element whose computed transition-property refers to this
> +   // The (pseudo-)element whose computed transition-property refers to this

Nit: extra space/tab added here?

::: layout/style/nsTransitionManager.h:253
(Diff revision 8)
> -  bool mWasFinishedOnLastTick;
> +  // This parameter will use for generate transition events.
> +  // For detail, see CSS-Transitions Level 2 Spec.

// The 'transition phase' used to determine which transition events need
// to be queued on this tick.
// See: https://drafts.csswg.org/css-transitions-2/#transition-phase

::: layout/style/nsTransitionManager.h:256
(Diff revision 8)
> +    Idle            = -2,
> +    Pending         = -1,
> +    EffectIdle      = ComputedTiming::AnimationPhase::Null,
> +    EffectBefore    = ComputedTiming::AnimationPhase::Before,
> +    EffectActive    = ComputedTiming::AnimationPhase::Active,
> +    EffectAfter     = ComputedTiming::AnimationPhase::After

I think we shouldn't have two "idle" phases. Also, can we rename the 'Effect*' ones to just Before etc.

e.g.

enum class TransitionPhase {
  Idle = ComputedTiming::AnimationPhase::Idle,
  Before = ComputedTiming::AnimationPhase::Before,
  Active = ComputedTiming::AnimationPhase::Active,
  After = ComputedTiming::AnimationPhase::After,
  Pending
};

(Making pending = -1 is ok too, whichever you prefer. Adding it at the end might make it more obvious that it is additional?)

Does that work?

I think without this we'll fail to dispatch a second transitionrun event if a CSSTransition object is cancel()'ed then play()'ed again, right? Since we'll never return to the Idle state, only the EffectIdle state.

::: layout/style/nsTransitionManager.cpp:52
(Diff revision 8)
> +struct TransitionEvent {
> +  EventMessage mMessage;
> +  StickyTimeDuration mElapsedTime;
> +  TimeStamp mTimeStamp;
> +
> +  TransitionEvent(EventMessage aMessage,
> +                  StickyTimeDuration aElapsedTime,
> +                  TimeStamp aTimeStamp)
> +    : mMessage(aMessage)
> +    , mElapsedTime(aElapsedTime)
> +    , mTimeStamp(aTimeStamp)
> +  { }
> +};

If we drop the constructor, I think we can use initializer list syntax to create these objects.

e.g. instead of

  events.AppendElement(TransitionEvent(eTransitionRun,
                                       intervalStartTime,
                                       zeroTimeStamp));

I think we could write:

  events.AppendElement({ eTransitionRun,
                         intervalStartTime,
                         zeroTimeStamp });

Also, the name 'TransitionEvent' is a bit confusing. It sounds like this is the actual event but it's really just a temporary collection of event parameters we use to create the event. Could we call it 'TransitionEventParams' ?

::: layout/style/nsTransitionManager.cpp:217
(Diff revision 8)
> +  // zero time stamp for transitionrun.
> +  // transitionrun won't need to consider delay when ordering event.
> +  TimeStamp zeroTimeStamp  = AnimationTimeToTimeStamp(zeroDuration);
> +  TimeStamp startTimeStamp = ElapsedTimeToTimeStamp(intervalStartTime);
> +  TimeStamp endTimeStamp   = ElapsedTimeToTimeStamp(intervalEndTime);

So, if I understand correctly. This calculation is not quite right. It will give us the correct sequence of events within a single element, but not necessarily between elements? And not necessarily with regards to animation events? But we are going to fix that in another bug? Is that right?

If that's right, I think that's ok since I think the issue only occurs when the animation runs backwards (which is very uncommon). But we should file a bug.

As for the comment here, perhaps something like:

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

::: layout/style/nsTransitionManager.cpp:223
(Diff revision 8)
> +  // transitionrun won't need to consider delay when ordering event.
> +  TimeStamp zeroTimeStamp  = AnimationTimeToTimeStamp(zeroDuration);
> +  TimeStamp startTimeStamp = ElapsedTimeToTimeStamp(intervalStartTime);
> +  TimeStamp endTimeStamp   = ElapsedTimeToTimeStamp(intervalEndTime);
> +
> +  TransitionPhase currentPhase = TransitionPhase::Idle;

Can we just leave this uninitialized? I think we'll overwrite this value immediately below so it seems less confusing to just leave it uninitialized here.
Attachment #8782318 - Flags: review?(bbirtles) → review+
Comment on attachment 8797036 [details]
Bug 1287983 part 4 - Add transition event handler test for checking elapsed time.

https://reviewboard.mozilla.org/r/82682/#review84944

This is looking good but I'd like to have another look with the following simplifications made.

Also, we should probably add a test for multiple transitionrun events (see comment 69).

e.g. Create a transition, wait for transitionrun, cancel() it. Then call play() again and check for a new transitionrun event.

::: dom/animation/test/css-transitions/file_csstransition-events.html:3
(Diff revision 5)
> +<!doctype html>
> +<meta charset=utf-8>
> +<title>Tests for elapsed time of CSS-Transition events</title>

I think this test covers more than just elapsed time?

'Tests for CSS transition events' ?

::: dom/animation/test/css-transitions/file_csstransition-events.html:14
(Diff revision 5)
> + * This function will help for receiving multiple events.
> + * EventWatcher of testharness.js can wait to multiple events,
> + * but it can't notify each event parameter.
> + * e.g: watcher.wait_for([ 'transitionrun', 'transitionstart' ])
> + *             .then(function(evt) { console.log(evt.type); });
> + *      The above example will print 'transitionstart' only.
> + *
> + * So we need to this object when checking multiple events parameter.

"Helper class to record the elapsedTime member of each event. The EventWatcher class in testharness.js allows us to wait on multiple events in a certain order but only records the event parameters of the most recent event."

::: dom/animation/test/css-transitions/file_csstransition-events.html:42
(Diff revision 5)
> +/**
> + * Create specified transitions.
> + */
> +function getAnimation(t, prepareFunc, watchEvents) {

How about we call this createTransition since that's what it does?

If we do that I don't think we need the comment (particularly since are no 'specified' transitions)

Then again, a function that is called 'createTransition' would be expected to return just a transition but this returns a transition *and* a div, watcher, and handler. So perhaps we should call it 'setupTransition'?

::: dom/animation/test/css-transitions/file_csstransition-events.html:45
(Diff revision 5)
> +};
> +
> +/**
> + * Create specified transitions.
> + */
> +function getAnimation(t, prepareFunc, watchEvents) {

Can we drop |watchEvents| and just make CSS_TRANSITION_EVENTS a local variable to this function? It seems like every call site ends up passing CSS_TRANSITION_EVENTS.

(Also, I think we try to avoid using 'const' since I believe IE10 doesn't support it, and we try not to require ES6 support in these tests.)

::: dom/animation/test/css-transitions/file_csstransition-events.html:46
(Diff revision 5)
> +
> +/**
> + * Create specified transitions.
> + */
> +function getAnimation(t, prepareFunc, watchEvents) {
> +  var div, watcher, handler, animation;

Would

 s/animation/transition/

be more clear?

::: dom/animation/test/css-transitions/file_csstransition-events.html:56
(Diff revision 5)
> +
> +  div.style.marginLeft = '100px';
> +  flushComputedStyle(div);
> +
> +  animation = div.getAnimations()[0];
> +  prepareFunc(animation);

What is the advantage of having prepareFunc? Wouldn't these tests be easier to read if we just returned the animation and did the prepareFunc steps inline?

e.g. instead of

  promise_test(function(t) {
    var prepareFunc =  function(anim) {
      anim.playbackRate = 1;
    };
    var [ div, animation, watcher, handler ] =
      getAnimation(t, prepareFunc, CSS_TRANSITION_EVENTS);
    return watcher.wait_for('transitionrun').then(function(evt) {
      assert_equals(evt.elapsedTime, 0.0);
    });
  }, 'Idle -> Pending or Before : Receive transitionrun');

we could write:

  promise_test(function(t) {
    var [ div, animation, watcher, handler ] =
      getAnimation(t, CSS_TRANSITION_EVENTS);
    animation.playbackRate = 1;
    return watcher.wait_for('transitionrun').then(function(evt) {
      assert_equals(evt.elapsedTime, 0.0);
    });
  }, 'Idle -> Pending or Before : Receive transitionrun');

::: dom/animation/test/css-transitions/file_csstransition-events.html:58
(Diff revision 5)
> +  flushComputedStyle(div);
> +
> +  animation = div.getAnimations()[0];
> +  prepareFunc(animation);
> +
> +  return [div, animation, watcher, handler];

(Nit: I don't think we actually use the div so maybe we don't need to return it?)

::: dom/animation/test/css-transitions/file_csstransition-events.html:61
(Diff revision 5)
> +// We integrated tests which the start phase is idle or pending.
> +// Because we can't seek to pending phase via API or Transitions.
> +

We can't force the implementation to still be in the pending state but we can force it to leave the pending state. So I think we could write something like:

  // On the next frame (i.e. when events are queued), whether or not the
  // transition is still pending depends on the implementation.
  promise_test(function(t) {
    var [ transition, watcher, handler ] = setupTransition(t);
    return watcher.wait_for('transitionrun').then(function(evt) {
      assert_equals(evt.elapsedTime, 0.0);
    });
  }, 'Idle -> Pending or Before');

  promise_test(function(t) {
    var [ transition, watcher, handler ] = setupTransition(t);
    // Force the transition to leave the pending state
    transition.startTime = document.timeline.currentTime;
    return watcher.wait_for('transitionrun').then(function(evt) {
      assert_equals(evt.elapsedTime, 0.0);
    });
  }, 'Idle -> Before');

::: dom/animation/test/css-transitions/file_csstransition-events.html:64
(Diff revision 5)
> +promise_test(function(t) {
> +  var prepareFunc =  function(anim) {
> +    anim.playbackRate = 1;
> +  };
> +  var [ div, animation, watcher, handler ] =
> +    getAnimation(t, prepareFunc, CSS_TRANSITION_EVENTS);
> +  return watcher.wait_for('transitionrun').then(function(evt) {
> +    assert_equals(evt.elapsedTime, 0.0);
> +  });
> +}, 'Idle -> Pending or Before : Receive transitionrun');

With the various simplifications suggested I think this would just become:

  promise_test(function(t) {
    var [ transition, watcher, handler ] = setupTransition(t);
    return watcher.wait_for('transitionrun').then(function(evt) {
      assert_equals(evt.elapsedTime, 0.0);
    });
  }, 'Idle -> Pending or Before');

::: dom/animation/test/css-transitions/file_csstransition-events.html:66
(Diff revision 5)
> +// We integrated tests which the start phase is idle or pending.
> +// Because we can't seek to pending phase via API or Transitions.
> +
> +promise_test(function(t) {
> +  var prepareFunc =  function(anim) {
> +    anim.playbackRate = 1;

No need to set playbackRate to 1 right?

::: dom/animation/test/css-transitions/file_csstransition-events.html:73
(Diff revision 5)
> +  var [ div, animation, watcher, handler ] =
> +    getAnimation(t, prepareFunc, CSS_TRANSITION_EVENTS);
> +  return watcher.wait_for('transitionrun').then(function(evt) {
> +    assert_equals(evt.elapsedTime, 0.0);
> +  });
> +}, 'Idle -> Pending or Before : Receive transitionrun');

I wonder if we need to put the expected test result in the description here? Would 'Idle -> Pending or Before' be enough? Likewise for the other tests in this file.

::: dom/animation/test/css-transitions/file_csstransition-events.html:78
(Diff revision 5)
> +}, 'Idle -> Pending or Before : Receive transitionrun');
> +
> +promise_test(function(t) {
> +  var prepareFunc = function(anim) {
> +    // Seek to Active phase.
> +    anim.playbackRate = 1;

Setting playbackRate is not necessary?
Attachment #8797036 - Flags: review?(bbirtles)
Also, I think we need to sound out another intent to implement/ship before we land this. We should probably list transitioncancel in the same message and try to land it in the same release cycle. (It's ok if animationcancel is in a separate release cycle.)
Comment on attachment 8782318 [details]
Bug 1287983 part 2 - Add transitionstart/transitionrun event handling.

https://reviewboard.mozilla.org/r/72490/#review84932

> If we drop the constructor, I think we can use initializer list syntax to create these objects.
> 
> e.g. instead of
> 
>   events.AppendElement(TransitionEvent(eTransitionRun,
>                                        intervalStartTime,
>                                        zeroTimeStamp));
> 
> I think we could write:
> 
>   events.AppendElement({ eTransitionRun,
>                          intervalStartTime,
>                          zeroTimeStamp });
> 
> Also, the name 'TransitionEvent' is a bit confusing. It sounds like this is the actual event but it's really just a temporary collection of event parameters we use to create the event. Could we call it 'TransitionEventParams' ?

The compiler can't deduce the 'TransitionEventParam'. We need to specify the 'TarnsitionEventParam' when calling the AppendElement.

> So, if I understand correctly. This calculation is not quite right. It will give us the correct sequence of events within a single element, but not necessarily between elements? And not necessarily with regards to animation events? But we are going to fix that in another bug? Is that right?
> 
> If that's right, I think that's ok since I think the issue only occurs when the animation runs backwards (which is very uncommon). But we should file a bug.
> 
> As for the comment here, perhaps something like:
> 
>   // 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.

Yes,
As discussed with you, this will give us the incorrect result when we use multiple elements.
I'll file the another bug for this.
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #78)
> The compiler can't deduce the 'TransitionEventParam'. We need to specify the
> 'TarnsitionEventParam' when calling the AppendElement.

Specifically, the issue is it can't disambiguate between the following two methods:

  elem_type* AppendElement(Item&& aItem);
  elem_type* AppendElement(const mozilla::fallible_t&);
Comment on attachment 8782318 [details]
Bug 1287983 part 2 - Add transitionstart/transitionrun event handling.

https://reviewboard.mozilla.org/r/72490/#review85340

::: layout/style/nsTransitionManager.cpp:52
(Diff revision 9)
> +struct TransitionEventParam {
> +  EventMessage mMessage;
> +  StickyTimeDuration mElapsedTime;
> +  TimeStamp mTimeStamp;
> +};

Nit: Can we call this TransitionEventParams (with an 's' since the are multiple parameters)
Comment on attachment 8797036 [details]
Bug 1287983 part 4 - Add transition event handler test for checking elapsed time.

https://reviewboard.mozilla.org/r/82682/#review85344

Looks good. It would be good to have some tests of values of elapsedTime that include negative delays and negative end delays and generally test some of the edge cases of the calculation of "iteration start" and "iteration end" (e.g. in particular, when there is a negative delay greater than the active duration). Perhaps that can be a separate patch though?

::: dom/animation/test/css-transitions/file_csstransition-events.html:10
(Diff revision 6)
> +var CSS_TRANSITION_EVENTS = [ 'transitionrun',
> +                              'transitionstart',
> +                              'transitionend' ];

Nit: Move this closer to setupTransition or (even inside setupTransition).

::: dom/animation/test/css-transitions/file_csstransition-events.html:56
(Diff revision 6)
> +}
> +
> +// On the next frame (i.e. when events are queued), whether or not the
> +// transition is still pending depends on the implementation.
> +promise_test(function(t) {
> +  var [transition, watcher, handler ] = setupTransition(t);

Nit: Here and through out this file we have

  [transition, watcher, handler ]

But I think we want:

  [transition, watcher, handler]

::: dom/animation/test/css-transitions/file_csstransition-events.html:64
(Diff revision 6)
> +  });
> +}, 'Idle -> Pending or Before');
> +
> +promise_test(function(t) {
> +  var [transition, watcher, handler ] = setupTransition(t);
> +  // Force the transition to leave the idle

Nit: Missing 'state' at end of the comment.

i.e. ... to leave the idle state

::: dom/animation/test/css-transitions/file_csstransition-events.html:71
(Diff revision 6)
> +promise_test(function(t) {
> +  var [transition, watcher, handler ] = setupTransition(t);
> +  // Seek to Active phase.
> +  transition.currentTime = 100 * MS_PER_SEC;
> +  transition.pause();
> +  return watcher.wait_for([ 'transitionrun',
> +                            'transitionstart' ]).then(function(evt) {
> +    assert_equals(handler.transitionrun, 0.0);
> +    assert_equals(handler.transitionstart, 0.0);
> +  });
> +}, 'Idle or Pending -> Active');

So this tests *either*:

Idle -> Pending -> Active

*or*:

Idle -> Active

which, is up to the implementation. We *could* test specifically Idle -> Active by doing something like:

  promise_test(function(t) {
    var [transition, watcher, handler ] = setupTransition(t);
    // Make transition idle
    transition.cancel();
    return watcher.wait_for([ 'transitionrun' ]).then(function() {
      handler.clear();

      // Then seek to active phase bypassing pending state
      transition.startTime = document.timeline.currentTime - 100 * MS_PER_SEC;
      return watcher.wait_for([ 'transitionrun',
                                'transitionstart' ]);
    }).then(function(evt) {
      assert_equals(handler.transitionrun, 0.0);
      assert_equals(handler.transitionstart, 0.0);
    });
  }, 'Idle -> Active');

But maybe it's not worth it.

::: dom/animation/test/css-transitions/file_csstransition-events.html:137
(Diff revision 6)
> +  });
> +}, 'Active -> Before');
> +
> +promise_test(function(t) {
> +  var [transition, watcher, handler ] = setupTransition(t);
> +  //Seek to Active phase.

Nit: Space after //
Attachment #8797036 - Flags: review?(bbirtles) → review+
Don't forget to send an updated intent to implement/ship before landing this.

Something like the following:


Intent to implement and ship: transitionrun, transitionstart, and transitioncancel events

This is a follow up to my previous intent to implement[1]. It was
discovered that the implementation of transitionstart in IE10~11/Edge
differs from the spec so the spec was subsequently updated to rename
transitionstart to transitionrun and add IE/Edge's transitionstart
event.

This updated "intent to implement" is also expanded to cover the
transitioncancel event and includes an intent to ship as well.

Summary: CSS Transitions Level 1 now includes additional transitionrun,
transitionstart, and transitioncancel events to allow an author to
detect when a transition has been created, when it begins animating, and
if it was terminated without running to completion. Authors often wait
for transitions to complete before updating UI state and these events
allow authors to know which transitions they need to wait for and
whether they should continue waiting (transitions may be prematurely
stopped for a number of reasons such as DOM nodes being detached from
the document tree--something that is particularly common with frameworks
such as Backbone and React).

Bugs:
https://bugzilla.mozilla.org/show_bug.cgi?id=1287983 (transitionstart/transitionrun)
https://bugzilla.mozilla.org/show_bug.cgi?id=1264125 (transitioncancel)

Link to standard:
https://drafts.csswg.org/css-transitions/#event-transitionevent

Platform coverage: all platforms.

Estimated or target release: 52

Preference behind which this will be implemented: Not needed.

DevTools bug: Not needed?

Do other browser engines implement this?
* IE10~11/Edge: Shipped transitionstart
* Blink: Considering? Bugs on file for implementing transitionstart[2]
  and transitioncancel[3] have been given the "Hotlist-Squash-A-Bug" and
  "Hotlist-Interop" flags respectively.
* The CSS WG resolved at TPAC this year to move these events from CSS
  Transitions Level 2 to Level 1 to indicate that they are shippable.[4]

Tests: We have been writing tests for these using testharness.js (Web
platform tests) format. However, we don't intend to upstream these to
web-platform-tests immediately since (a) they rely on the Web Animations
API and (b) merging the CSS WG test repository with web-platform-tests
is a work in progress.

[1] https://groups.google.com/forum/#!topic/mozilla.dev.platform/PR_AP6i-xIo
[2] https://bugs.chromium.org/p/chromium/issues/detail?id=439056
[3] https://bugs.chromium.org/p/chromium/issues/detail?id=642487
[4] https://logs.csswg.org/irc.w3.org/css/2016-09-19/#e722575
Comment on attachment 8797036 [details]
Bug 1287983 part 4 - Add transition event handler test for checking elapsed time.

https://reviewboard.mozilla.org/r/82682/#review85344

Thanks, Brian.

I think that we should fire the 'transitioncancel' in this case. The specification of CSS-Transitions explained as following:
' Otherwise, if the combined duration is less than or equal to 0s, or if the current value of the property in the running transition cannot be interpolated with the value of the property in the after-change style, then implementations must cancel the running transition. ' [1]

[1] https://drafts.csswg.org/css-transitions-1/#ref-for-transition-combined-duration-2

So I think that this test will need to fix in bug 1264125.

How do you think this?
Flags: needinfo?(bbirtles)
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #88)
> Comment on attachment 8797036 [details]
> Bug 1287983 part 4 - Add transition event handler test for checking elapsed
> time.
> 
> https://reviewboard.mozilla.org/r/82682/#review85344
> 
> Thanks, Brian.
> 
> I think that we should fire the 'transitioncancel' in this case. The
> specification of CSS-Transitions explained as following:
> ' Otherwise, if the combined duration is less than or equal to 0s, or if the
> current value of the property in the running transition cannot be
> interpolated with the value of the property in the after-change style, then
> implementations must cancel the running transition. ' [1]
> 
> [1]
> https://drafts.csswg.org/css-transitions-1/#ref-for-transition-combined-
> duration-2
> 
> So I think that this test will need to fix in bug 1264125.
> 
> How do you think this?

Oh good point. I guess the situation I was wondering about only occurs when we substitute in a different effect using the API--not for transitions generated using markup. In that case, it's probably not worth testing, or at least not in this case. We still should, however, test elapsedTime with a negative start delay and a negative end delay. But, again, that can be a separate patch.
Flags: needinfo?(bbirtles)
(In reply to Brian Birtles (:birtles, away most of Oct 19~20) from comment #89)
> Oh good point. I guess the situation I was wondering about only occurs when
> we substitute in a different effect using the API--not for transitions
> generated using markup. In that case, it's probably not worth testing, or at
> least not in this case. We still should, however, test elapsedTime with a
> negative start delay and a negative end delay. But, again, that can be a
> separate patch.
Thanks, Brian,

I th we can't set the negative end delay to the CSS-Transition via Web Animations API. Because we used AnimationEffectTimingReadOnly as timing parameter of CSS-Transition.[1]

[1] https://dxr.mozilla.org/mozilla-central/rev/01ab78dd98805e150b0311cce2351d5b408f3001/layout/style/nsTransitionManager.h#44

In this patch, I try to set the new KeyframeEffect object to the CSS-Transition.
Could you please review this patch?
Comment on attachment 8802399 [details]
Bug 1287983 part 7 - Add negative delay tests for CSS-Transitions event.

https://reviewboard.mozilla.org/r/86794/#review86424

Looks good! r=me with comments addressed

::: dom/animation/test/css-transitions/file_csstransition-events.html:179
(Diff revision 1)
> +promise_test(function(t) {
> +  var div, watcher, handler, transition;
> +  div = addDiv(t, { style: 'transition: margin-left 100s -50s' });
> +  watcher = new EventWatcher(t, div, [ 'transitionrun',
> +                                       'transitionstart',
> +                                       'transitionend' ]);
> +  handler = new TransitionEventHandler(div);
> +  flushComputedStyle(div);
> +
> +  div.style.marginLeft = '100px';
> +  flushComputedStyle(div);
> +
> +  var animation = div.getAnimations()[0];
> +
> +  return watcher.wait_for([ 'transitionrun',
> +                            'transitionstart' ]).then(function() {
> +    assert_equals(handler.transitionrun, 50.0);
> +    assert_equals(handler.transitionstart, 50.0);
> +    animation.currentTime = 50 * MS_PER_SEC;
> +    return watcher.wait_for('transitionend');
> +  }).then(function(evt) {
> +    assert_equals(evt.elapsedTime, 100.0);
> +  });
> +}, 'Calculating the interval start and end time with negative start delay.');

This is good. However, it seems like the first half of the test is ~99% identical to setupTransition? Could we just add an optional parameter to setupTransition to override the transition style?

Then do in setupTransition, e.g.

  transitionStyle = transitionStyle ||
                    'transition: margin-left 100s 100s';
  div = addDiv(t, style: transitionStyle);

Would that work?

::: dom/animation/test/css-transitions/file_csstransition-events.html:197
(Diff revision 1)
> +
> +  return watcher.wait_for([ 'transitionrun',
> +                            'transitionstart' ]).then(function() {
> +    assert_equals(handler.transitionrun, 50.0);
> +    assert_equals(handler.transitionstart, 50.0);
> +    animation.currentTime = 50 * MS_PER_SEC;

Could we just call animation.finish() here?

::: dom/animation/test/css-transitions/file_csstransition-events.html:208
(Diff revision 1)
> +    transition.pause();
> +

Is this necessary?

::: dom/animation/test/css-transitions/file_csstransition-events.html:210
(Diff revision 1)
> +    // We can't set the end delay via generated effect timing.
> +    // Because CSS-Transition use the AnimationEffectTimingReadOnly.
> +    transition.effect = new KeyframeEffect(handler.target,
> +                                           { marginleft: [ '0px', '100px' ]},
> +                                           { duration: 100 * MS_PER_SEC,
> +                                             endDelay: -50 * MS_PER_SEC });

(Once we do bug 1273784, we can make this just `transition.effect = new KeyframeEffect(transition.effect)`)

::: dom/animation/test/css-transitions/file_csstransition-events.html:219
(Diff revision 1)
> +                                           { duration: 100 * MS_PER_SEC,
> +                                             endDelay: -50 * MS_PER_SEC });
> +    // Seek to Before and play.
> +    transition.cancel();
> +    transition.play();
> +    handler.clear();

Is this call to clear() needed?

::: dom/animation/test/css-transitions/file_csstransition-events.html:225
(Diff revision 1)
> +    return watcher.wait_for('transitionstart');
> +  }).then(function() {
> +    assert_equals(handler.transitionstart, 0.0);
> +
> +    // Seek to After phase.
> +    transition.currentTime = 50 * MS_PER_SEC;

transition.finish() ?
Attachment #8802399 - Flags: review?(bbirtles) → review+
Attachment #8802399 - Attachment is obsolete: true
Comment on attachment 8802399 [details]
Bug 1287983 part 7 - Add negative delay tests for CSS-Transitions event.

https://reviewboard.mozilla.org/r/86794/#review86424

Thank you for your review.

I addressed this patch.

> Is this necessary?

Sorry, This was just purpose to test.
Comment on attachment 8803209 [details]
Bug 1287983 part 7 - Add negative delay tests for CSS-Transitions event.

https://reviewboard.mozilla.org/r/87486/#review86466
Attachment #8803209 - Flags: review?(bbirtles) → review+
Attachment #8782317 - Flags: review?(bugs)
Hi Olli,
I changed the EventHandler.webidl, but I've forget the DOM Peer review.
Could you please confirm this part of patch?
The spec doesn't have ontransitionrun nor ontransitionstart event handlers, and I couldn't see them been implemented in any other browser.
Why are we adding them? Can't really r+ without at least some spec bug.


Bug 911987 was different since there we tried to be compatible with others (but did spec bugs get filed even for that issue).
Comment on attachment 8782317 [details]
Bug 1287983 part 1 - Add transitionstart/transitionrun event handlers.

https://reviewboard.mozilla.org/r/72488/#review87292

We need some justification in specs for this change. Can't really r+ before it.
I do assume the handlers will be added in some spec, but would be good to know on which event target objects and so.
Attachment #8782317 - Flags: review?(bugs) → review-
Comment on attachment 8782317 [details]
Bug 1287983 part 1 - Add transitionstart/transitionrun event handlers.

https://reviewboard.mozilla.org/r/72488/#review87294

(do we have a bug open to add ontransitioncancel ?)
Attachment #8782317 - Flags: review- → review+
Yes, confusing. 

(In reply to Olli Pettay [:smaug] from comment #112)
> (do we have a bug open to add ontransitioncancel ?)

It's bug 1264125.
URL: https://bugzilla.mozilla.org/show_bug...
Pushed by mantaroh@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/eb5e66f42433
part 1 - Add transitionstart/transitionrun event handlers. r=masayuki,smaug
https://hg.mozilla.org/integration/autoland/rev/203cc2a68c0c
part 2 - Add transitionstart/transitionrun event handling. r=birtles
https://hg.mozilla.org/integration/autoland/rev/c1df30f6a940
part 3 - Add transitionstart/transitionrun test. r=birtles
https://hg.mozilla.org/integration/autoland/rev/2f525f7afa56
part 4 - Add transition event handler test for checking elapsed time. r=birtles
https://hg.mozilla.org/integration/autoland/rev/41870a71d1f4
part 5 - Clarify the function name of creating transition. r=hiro
https://hg.mozilla.org/integration/autoland/rev/be44e592e9e7
part 6 - Integrate ElapsedTimeToTimeStamp function to the Animation class. r=birtles
https://hg.mozilla.org/integration/autoland/rev/deb8d7d93608
part 7 - Add negative delay tests for CSS-Transitions event. r=birtles
Hi Chris, thanks very much for doing this!

A couple of minor notes.

There are actually three new events: transitionstart, transitionrun, AND transitioncancel. transitioncancel was implemented in bug 1264125 but since it didn't land in time for Firefox 52, we disabled transitionstart and transitionrun in Firefox 52 (see bug 1324985) so that these new events all arrive in the same release (Firefox 53).

Also, IE10/Edge only implement transitionstart, and not transitionrun.

I've updated the pages for transitionstart and transitionrun accordingly.

Would you mind updating the release notes to drop the mention from Firefox 52 and add it to Firefox 53 along with mention of transitioncancel (bug 1264125)? Thanks!
Flags: needinfo?(cmills)
(In reply to Brian Birtles (:birtles, away most of Jan 21 - Feb 1) from comment #118)
> Hi Chris, thanks very much for doing this!
> 
> A couple of minor notes.
> 
> There are actually three new events: transitionstart, transitionrun, AND
> transitioncancel. transitioncancel was implemented in bug 1264125 but since
> it didn't land in time for Firefox 52, we disabled transitionstart and
> transitionrun in Firefox 52 (see bug 1324985) so that these new events all
> arrive in the same release (Firefox 53).
> 
> Also, IE10/Edge only implement transitionstart, and not transitionrun.
> 
> I've updated the pages for transitionstart and transitionrun accordingly.
> 
> Would you mind updating the release notes to drop the mention from Firefox
> 52 and add it to Firefox 53 along with mention of transitioncancel (bug
> 1264125)? Thanks!

All updates made. Thanks for the review Brian.
Flags: needinfo?(cmills)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: