Closed Bug 1202333 Opened 9 years ago Closed 7 years ago

AnimationEvent elapsedTime should reflect playbackRate

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:
mozilla53
Tracking Flags:
Tracking Status
firefox43 --- affected
firefox53 --- fixed

People

(Reporter: birtles, Assigned: mantaroh)

References

(
URL
)

Details

Attachments

(4 files, 3 obsolete files)

Bug 1202333 part 1 - Remove excessive animationiteration event.
58 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1202333 part 2 - Update the CSSTransition::QueueEvents to specification.
58 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1202333 part 3 - Add CSS-Animations event tests.
58 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1202333 part 4 - Add test of event order for CSS-Animation.
58 bytes, text/x-review-board-request
birtles
: review+
Details 
CSS Animations describes elapsedTime in a manner that seems to assume a forwards playback direction. However, the Web Animations API means we can seek and reverse animations. This means, amongst other things, it is possible to dispatch an animationstart when hitting the right-hand-side of the active interval. In this case, what should the behavior be?

There are many cases to consider including:

a) Playback rate negative, going RHS to active
   => animationstart with elapsedTime = animation-duration?

b) Playback rate negative, going active to LHS
   => animationend with elapsedTime = 0 (or that odd negative delay thing)

c) Playback rate negative, going RHS to LHS
   => animationstart with elapsedTime = animation-duration AND
      animationend with elapsedTime = 0 (or negative delay thing)

Alternatively you could argue that elapsedTime really means the amount of time the animation has been running and we should change the elapsedTime for animationiteration events instead. On further thought, that might actually make more sense.

Then there is seeking:

d) Playback rate positive, seeking from RHS to active
   => animationstart with elapsedTime = animation-duration since we entered
       from the RHS?
      Or should it be zero and we should pretend that we started from the LHS?
      Or should we use the time that we seeked to?

e) Playback rate positive, seeking from RHS to LHS
   => animationstart,animationend with elapsedTime = animation-duration,0
      Or should it be elapsedTime = 0,animation-duration?
      Or should it be elapsedTime = 0,0?
      Or nothing at all?

f) Playback rate position, seeking from active to LHS
   => animationend with elapsedTime = 0? duration?

There are plenty of other permutations but if we can answer the above we can probably extrapolate the answers to those.

The real question is, what is elapsedTime used for?

Of the above, (e) is probably the weirdest so if we can decide what it should do the rest might follow. I'm currently thinking it should be elapsedTime = animation-duration, 0.

This needs to be discussed on the mailing list but I'm just filing for now to track this.


In implementing this the tricky part is being careful of the work in bug 1183461. Basically, that bug takes the elapsedTime and converts it to a timestamp for sorting events. In particular, we need to be sure that if we ever generate animationstart followed by animationend they still end up being dispatched in that order.
I tried the test case for comment #0. [1]

[1] http://output.jsbin.com/duqunev/

As Brian said on comment #0, the elapsed time of iteration is not correct currently.
I think that we should consider the elapsed time when we implement the transitioncancel event(bug 1264125) and transitionstart event(bug 1287983).
This should be mostly specced now:

  https://drafts.csswg.org/css-animations-2/#event-dispatch
  https://drafts.csswg.org/css-transitions-2/#transition-event-types

(The transitions spec still has a few gaps, but the idea is to just do the same thing as we do for CSS Animations.)
Hi Brian,

I have a question about transitionend.
In the spec, transitionend will fire at the completion of the transition.
However if we have negative playbackRate in transition, we will fire this event after the start delay.
This behavior does not same to CSS-Animations. I think that this behavior will probably cause confusion to some developers.

Example: http://output.jsbin.com/girafor

I think that it is reasonable to fire this event when become the out of active phase range.
How do you feel about this event behavior with negative playback rate?
Flags: needinfo?(bbirtles)
The desired behavior for transition events is just an extrapolation of the behavior defined for animations[1] which I've added some description of in bug 1287983 comment 25. I'll update CSS transitions spec in the next week or two with these details.

[1] https://drafts.csswg.org/css-animations-2/#event-dispatch
Flags: needinfo?(bbirtles)
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #5)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=012789031f12b14414e53cf0791eee0afebc31d9
Oops, the previous patch is wrong.
I fixed patch to prevent build failure and some test.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=48d04c39839c508dd17ab583b01dee73ae42b8e8
Comment on attachment 8802804 [details]
Bug 1202333 part 1 - Remove excessive animationiteration event.

https://reviewboard.mozilla.org/r/87110/#review89178

r=me with the following changes

::: dom/events/test/test_legacy_event.html:76
(Diff revision 1)
>  // events to fire for the animation beginning & ending.
>  function triggerShortAnimation(node) {
>    node.style.animation = "anim1 1ms linear";
>  }
>  
> -// This function triggers a long animation with two iterations, which is
> +// This function triggers a short animation with many iterations.

"This function triggers a very short (1ms long) animation with many iterations, which will cause a start event followed by an iteration event on each subsequent tick, to fire.

NOTE: We need the many iterations since if an animation frame coincides with the animation starting or ending we dispatch only the start or end event and not the iteration event."

(Although you might need to s/1ms/10ms/ above depending on the values you choose based on the next comment.)

::: dom/events/test/test_legacy_event.html:78
(Diff revision 1)
> -// sure our event does fire, we use a long duration and a nearly-as-long
> -// negative delay. This ensures we hit the end of the first iteration right
> -// away, and that we don't risk hitting the end of the second iteration at the
> -// same time.
>  function triggerAnimationIteration(node) {
> -  node.style.animation = "anim1 300s -299.999s linear 2";
> +  node.style.animation = "anim1 1ms linear 1000";

I think this is too short, right? If we get one frame after the animation starts and then one frame after it finishes we'll never get the event since we don't dispatch iteration events at the same time as start or finish events.

1000 x 1ms = 1s so we'll timeout if frames are longer than 500ms which is quite common.

Perhaps we can use an animation duration of 10ms and 20,000 iterations? That would mean frames would need to be at least 100s apart to timeout which is our usual figure?
Attachment #8802804 - Flags: review?(bbirtles) → review+
Comment on attachment 8802805 [details]
Bug 1202333 part 2 - Update the CSSTransition::QueueEvents to specification.

https://reviewboard.mozilla.org/r/87112/#review90264

This looks good but I'd like to have one more quick check with the changes to the previous phase / iteration members.

::: layout/style/nsAnimationManager.h:266
(Diff revision 1)
>    enum {
>      PREVIOUS_PHASE_BEFORE = uint64_t(-1),
>      PREVIOUS_PHASE_AFTER = uint64_t(-2)
>    };
>    // One of the PREVIOUS_PHASE_* constants, or an integer for the iteration
>    // whose start we last notified on.
>    uint64_t mPreviousPhaseOrIteration;
> +
> +  ComputedTiming::AnimationPhase mPreviousPhase;
> +
> +  uint64_t mPreviousIteration;

We don't need mPreviousPhaseOrIteration AND mPreviousPhase and mPreviousIteration, right?

We should either drop mPreviousPhaseOrIteration or, probably preferably, just extend the PREVIOUS_PHASE_ enum to include the extra values we need.

::: layout/style/nsAnimationManager.cpp:40
(Diff revision 1)
>  
> +typedef mozilla::ComputedTiming::AnimationPhase AnimationPhase;
> +
>  namespace {
>  
> -// Pair of an event message and elapsed time used when determining the set of
> +// This structure used when determining the set of events to queue.

I think we could probably drop this comment.

::: layout/style/nsAnimationManager.cpp:219
(Diff revision 1)
> -  bool isSameIteration =
> -         computedTiming.mCurrentIteration == mPreviousPhaseOrIteration;
> +  uint64_t iterationBoundary = (mPreviousIteration > currentIteration)?
> +                                 (currentIteration + 1) : currentIteration;

Drop the unnecessary braces here and put each clause on a new line as in:

  uint64_t iterationBoundary = mPreviousIteration > currentIteration
                               ? currentIteration + 1
                               : currentIteration;

::: layout/style/nsAnimationManager.cpp:231
(Diff revision 1)
> -    mPreviousPhaseOrIteration = PREVIOUS_PHASE_BEFORE;
> -  } else if (computedTiming.mPhase == ComputedTiming::AnimationPhase::Active) {
> -    mPreviousPhaseOrIteration = computedTiming.mCurrentIteration;
> -  } else if (computedTiming.mPhase == ComputedTiming::AnimationPhase::After) {
> +  if (currentPhase == mPreviousPhase &&
> +      currentIteration == mPreviousIteration) {
> +    return;
> +  }

Let's move this early return up with the other early return so we avoid the calculation on startTimeStamp etc. in the case when nothing has changed.

::: layout/style/nsAnimationManager.cpp:258
(Diff revision 1)
> +        events.AppendElement(AnimationEventParams{ eAnimationIteration,
> +                                                   iterationStartTime,
> +                                                   iterationTimeStamp });

Add a comment before this along the lines of:

// The current iteration must have changed or else we would have returned early above.
MOZ_ASSERT(currentIteration != mPreviousIteration);
Attachment #8802805 - Flags: review?(bbirtles)
Comment on attachment 8802806 [details]
Bug 1202333 part 3 - Add CSS-Animations event tests.

https://reviewboard.mozilla.org/r/87114/#review87508

Looking good but we need some tests for a negative playback rate I think? Also with regards to ordering events between different elements? Might be a different file / test though?

::: dom/animation/test/css-animations/file_cssanimation-events.html:3
(Diff revision 1)
> +<!doctype html>
> +<meta charset=utf-8>
> +<title>Tests for CSS-Animations events</title>

Tests for CSS animation event dispatch

::: dom/animation/test/css-animations/file_cssanimation-events.html:16
(Diff revision 1)
> +</style>
> +<body>
> +<script>
> +'use strict';
> +
> +function AnimationEventHandler(target) {

Just copy the comment from TransitionEventHandler to explain why we need this helper.

::: dom/animation/test/css-animations/file_cssanimation-events.html:34
(Diff revision 1)
> +function setupAnimation(t, styleSyntax) {
> +  if (!styleSyntax) {
> +    styleSyntax = 'anim 10s';
> +  }
> +  var div = addDiv(t, { style: "animation: " + styleSyntax });

Nit: s/styleSyntax/animationStyle/ ?

::: dom/animation/test/css-animations/file_cssanimation-events.html:35
(Diff revision 1)
> +  if (!styleSyntax) {
> +    styleSyntax = 'anim 10s';
> +  }

I think this could be just written as

  animationStyle = animationStyle || 'anim 10s';

*but* I think it would actually be better if we just always pass in 'anim 10s' since it makes the tests easier to read.

(We should probably do the same to setupTransition too I guess.)

::: dom/animation/test/css-animations/file_cssanimation-events.html:44
(Diff revision 1)
> +  var watcher = new EventWatcher(t, div, [ 'animationstart',
> +                                           'animationiteration',
> +                                           'animationend' ]);
> +  var handler = new AnimationEventHandler(div);
> +
> +  flushComputedStyle(div);

This is not needed since getAnimations() is defined to flush style.

::: dom/animation/test/css-animations/file_cssanimation-events.html:51
(Diff revision 1)
> +  var [animation, watcher, handler] = setupAnimation(t);
> +
> +  return watcher.wait_for('animationstart').then(function(evt) {
> +    assert_equals(evt.elapsedTime, 0.0);

(This might be a more interesting test if we add, say, a 1ms delay? That would check that the elapsedTime does not include the delay?)

::: dom/animation/test/css-animations/file_cssanimation-events.html:63
(Diff revision 1)
> +  return Promise.all([ watcher.wait_for([ 'animationstart', 'animationend' ]),
> +                       animation.finished ]).then(function(evt) {

Do we need to wait for animation.finished? Isn't waiting for the events enough?

::: dom/animation/test/css-animations/file_cssanimation-events.html:88
(Diff revision 1)
> +promise_test(function(t) {
> +  var [animation, watcher, handler] = setupAnimation(t, 'anim 10s 10s paused');
> +
> +  return animation.ready.then(function() {
> +    // Seek to After phase.
> +    animation.currentTime = 20 * MS_PER_SEC;

animation.finish() ?

::: dom/animation/test/css-animations/file_cssanimation-events.html:111
(Diff revision 1)
> +    assert_equals(evt.elapsedTime, 0.0);
> +  });
> +}, 'Active -> Before');
> +
> +promise_test(function(t) {
> +  var [animation, watcher, handler] = setupAnimation(t, 'anim 10s 10s paused');

I don't think we need the delay here?

::: dom/animation/test/css-animations/file_cssanimation-events.html:113
(Diff revision 1)
> +  // Seek to Active phase.
> +  animation.currentTime = 10 * MS_PER_SEC;

I think we could just wait on ready / animationstart once we drop the delay

::: dom/animation/test/css-animations/file_cssanimation-events.html:117
(Diff revision 1)
> +
> +  // Seek to Active phase.
> +  animation.currentTime = 10 * MS_PER_SEC;
> +  return watcher.wait_for('animationstart').then(function(evt) {
> +    // Seek to After phase.
> +    animation.currentTime = 20 * MS_PER_SEC;

animation.finish()

::: dom/animation/test/css-animations/file_cssanimation-events.html:169
(Diff revision 1)
> +    // Seek to iteration 0 phase. (confirm it doesn't receive iteration.)
> +    animation.currentTime = 10 * MS_PER_SEC;
> +    return watcher.wait_for('animationstart');
> +  }).then(function(evt) {
> +    // Seek to iteration 1 phase.
> +    animation.currentTime = 20 * MS_PER_SEC;

This might be more interesting if we seek to 30 * MS_PER_SEC to confirm that we only get the *second* iteration (i.e. elapsedTime == 20).

::: dom/animation/test/css-animations/file_cssanimation-events.html:175
(Diff revision 1)
> +    handler.clear();
> +    return watcher.wait_for('animationiteration');
> +  }).then(function(evt) {
> +    assert_equals(evt.elapsedTime, 10);
> +
> +    // Seek to After phase. (confirm it doesn't receive iteration.)

// Seek to after phase (no animationinteration event should be dispatched)

::: dom/animation/test/css-animations/file_cssanimation-events.html:191
(Diff revision 1)
> +
> +  // Seek to After phase.
> +  animation.finish();
> +  return watcher.wait_for([ 'animationstart',
> +                            'animationend' ]).then(function() {
> +    // Seek to iteration 2 phase. (confirm it doesn't receive iteration)

// Seek to iteration 2 (no animationinteration event should be dispatched)

::: dom/animation/test/css-animations/file_cssanimation-events.html:202
(Diff revision 1)
> +    animation.currentTime = 20 * MS_PER_SEC;
> +    return watcher.wait_for('animationiteration');
> +  }).then(function(evt) {
> +    assert_equals(evt.elapsedTime, 20.0);
> +
> +    // Seek to Before phase.(confirm it doesn't receive iteration)

// Seek to before phase (no animationinteration event should be dispatched)

::: dom/animation/test/mochitest.ini:19
(Diff revision 1)
>    css-animations/file_animation-ready.html
>    css-animations/file_animation-reverse.html
>    css-animations/file_animation-starttime.html
>    css-animations/file_animations-dynamic-changes.html
>    css-animations/file_cssanimation-animationname.html
> +  css-animations/file_cssanimation-events.html

We use 'cssanimation-animationname' in the line above because we are testing the 'animationName' property of the 'CSSAnimation' interface. This should just be file_events or file_event-dispatch or something like that.

::: dom/animation/test/testcommon.js:32
(Diff revision 1)
> +function assert_second_times_equal(actual, expected, description) {
> +  assert_approx_equals(actual, expected, TIME_PRECISION * MS_PER_SEC, description);
> +}

I think we can drop this. It doesn't seem to be used.
Attachment #8802806 - Flags: review?(bbirtles)
Comment on attachment 8802805 [details]
Bug 1202333 part 2 - Update the CSSTransition::QueueEvents to specification.

https://reviewboard.mozilla.org/r/87112/#review91856

This looks good but I'll have another quick look tomorrow afternoon. In the mean time here are some initial comments.

::: layout/style/nsAnimationManager.h:261
(Diff revision 2)
>    enum {
>      PREVIOUS_PHASE_BEFORE = uint64_t(-1),
>      PREVIOUS_PHASE_AFTER = uint64_t(-2)
>    };
> -  // One of the PREVIOUS_PHASE_* constants, or an integer for the iteration
> -  // whose start we last notified on.
> -  uint64_t mPreviousPhaseOrIteration;
> +  ComputedTiming::AnimationPhase mPreviousPhase;
> +
> +  uint64_t mPreviousIteration;

As discussed, I think here we either want to:

a) Drop the enum, or
b) Keep the existing code and add PREVIOUS_PHASE_NULL to the enum

After some discussion (b) seems preferable since:

* Combining these into one member means you never have odd states like "null phase with iteration of 3".
* It's a little less memory to store a single 64-bit int than a 64-bit integer plus an enum value.

::: layout/style/nsAnimationManager.cpp:222
(Diff revision 2)
> -     computedTiming.mPhase == ComputedTiming::AnimationPhase::Before);
> -
> -  MOZ_ASSERT(!skippedActivePhase || (!isActive && !wasActive),
> +  uint64_t iterationBoundary = (mPreviousIteration > currentIteration)
> +                     ? currentIteration + 1
> +                     : currentIteration;

Can we line up the ? and : so they are under the ( from the first line?

That's how I interpret [1] anyway.

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Operators

::: layout/style/nsAnimationManager.cpp:256
(Diff revision 2)
> +        // The currentiteration must have change or element we would have
> +        // returned early above.

Nit: s/currentiteration/currentIteration/
Nit: s/have change/have changed/
Comment on attachment 8802804 [details]
Bug 1202333 part 1 - Remove excessive animationiteration event.

https://reviewboard.mozilla.org/r/87110/#review92240

::: dom/events/test/test_legacy_event.html:76
(Diff revision 3)
>  // events to fire for the animation beginning & ending.
>  function triggerShortAnimation(node) {
>    node.style.animation = "anim1 1ms linear";
>  }
>  
> -// This function triggers a long animation with two iterations, which is
> +// This function triggers a very short (1ms long) animation with many

This should be 10ms

::: dom/events/test/test_legacy_event.html:80
(Diff revision 3)
>  
> -// This function triggers a long animation with two iterations, which is
> -// *nearly* at the end of its first iteration.  It will hit the end of that
> -// iteration (firing an event) almost immediately, 1ms in the future.
> +// This function triggers a very short (1ms long) animation with many
> +// iterations, which will cause a start event followed by an iteration event
> +// on each subsequent tick, to fire.
>  //
> -// NOTE: It's important that this animation have a *long* duration.  If it were
> +// NOTE: We need the many iterations sice if an animation frame coincides

s/sice/since/

::: dom/events/test/test_legacy_event.html:82
(Diff revision 3)
> -// *nearly* at the end of its first iteration.  It will hit the end of that
> -// iteration (firing an event) almost immediately, 1ms in the future.
> +// iterations, which will cause a start event followed by an iteration event
> +// on each subsequent tick, to fire.
>  //
> -// NOTE: It's important that this animation have a *long* duration.  If it were
> -// short (e.g. 1ms duration), then we might jump past all its iterations in
> -// a single refresh-driver tick. And if that were to happens, we'd *never* fire
> +// NOTE: We need the many iterations sice if an animation frame coincides
> +// with the animation starting or ending we dispatch only the start or end
> +// event and not be iteration event.

s/be/the/
This patch is rough implementation of using PREVIOUS_PHASE_NULL enum.
Comment on attachment 8802805 [details]
Bug 1202333 part 2 - Update the CSSTransition::QueueEvents to specification.

https://reviewboard.mozilla.org/r/87112/#review93298

I think we need to update the previous phase and iteration when the animation is cancelled.

::: layout/style/nsAnimationManager.h:261
(Diff revision 4)
> -  enum {
> -    PREVIOUS_PHASE_BEFORE = uint64_t(-1),
> -    PREVIOUS_PHASE_AFTER = uint64_t(-2)
> +  ComputedTiming::AnimationPhase mPreviousPhase;
> +
> +  uint64_t mPreviousIteration;

Please put these two lines together and add a comment:

  // Phase and current iteration from the previous time we queued events.
  // This is used to determine what new events to dispatch.
  ComputedTiming::AnimationPhase mPreviousPhase;
  uint64_t mPreviousIteration;

::: layout/style/nsAnimationManager.cpp:201
(Diff revision 4)
>    if (computedTiming.mPhase == ComputedTiming::AnimationPhase::Null) {
>      return; // do nothing
>    }

I think this means that we won't update mPreviousPhase when the animation is cancelled and hence we won't dispatch an animationstart if we then restart the animation (except when we have a delay since then we'll visit the before phase so we'll be ok).

Can we add a test for this and fix it?

(I know we'll have to fix this in bug 1302648 anyway, but I think we could easily fix this here for now?)

::: layout/style/nsAnimationManager.cpp:222
(Diff revision 4)
> -     computedTiming.mPhase == ComputedTiming::AnimationPhase::Before);
> -
> -  MOZ_ASSERT(!skippedActivePhase || (!isActive && !wasActive),
> +  uint64_t iterationBoundary = (mPreviousIteration > currentIteration)
> +                               ? currentIteration + 1
> +                               : currentIteration;

Any reason you didn't drop the braces as indicated in comment 11?

(If there is, please say so!)
Attachment #8802805 - Flags: review?(bbirtles)
Comment on attachment 8802806 [details]
Bug 1202333 part 3 - Add CSS-Animations event tests.

https://reviewboard.mozilla.org/r/87114/#review93304

What did you decide to do about:

(In reply to Brian Birtles (:birtles) from comment #12)
> Looking good but we need some tests for a negative playback rate I think?
> Also with regards to ordering events between different elements? Might be a
> different file / test though?

?

::: dom/animation/test/css-animations/file_events_dispatch.html:41
(Diff revision 4)
> +  this.animationiteration = undefined;
> +  this.animationend = undefined;
> +}
> +
> +function setupAnimation(t, animationStyle) {
> +  animationStyle = animationStyle || 'anim 10s';

As mentioned in comment 12, can we just always pass in 'anim 10s'?

(Or, if you disagree, please say so! It's fine to disagree, but where there is review feedback, please either follow it or add a comment saying why you decided not to.)

::: dom/animation/test/css-animations/file_events_dispatch.html:53
(Diff revision 4)
> +
> +  return [animation, watcher, handler];
> +}
> +
> +promise_test(function(t) {
> +  var [animation, watcher, handler] = setupAnimation(t, 'anim 10s 1ms');

Add a comment saying:

// Add 1ms delay to ensure that the delay is not included in the elapesdTime.

::: dom/animation/test/css-animations/file_events_dispatch.html:164
(Diff revision 4)
> +    // Seek to iteration 0 phase. (confirm it doesn't receive iteration.)
> +    animation.currentTime = 10 * MS_PER_SEC;
> +    return watcher.wait_for('animationstart');
> +  }).then(function(evt) {
> +    // Seek to iteration 2 phase.

Nit: iterations are not phases so I would just make these two comments:

    // Seek to iteration 0 (no animationiteration event should be dispatched)

    // Seek to iteration 2

::: dom/animation/test/css-animations/file_events_dispatch.html:175
(Diff revision 4)
> +    handler.clear();
> +    return watcher.wait_for('animationiteration');
> +  }).then(function(evt) {
> +    assert_equals(evt.elapsedTime, 20);
> +
> +    // Seek to After phase. (no animationiteration event should be dispatched)

(Nit: We can drop the '.' here as per comment 12)

::: dom/animation/test/mochitest.ini:77
(Diff revision 4)
>  [css-animations/test_animation-starttime.html]
>  [css-animations/test_cssanimation-animationname.html]
>  [css-animations/test_document-get-animations.html]
>  [css-animations/test_effect-target.html]
>  [css-animations/test_element-get-animations.html]
> +[css-animations/test_events_dispatch.html]

Please follow the file naming convention here:

  test_event-dispatch.html
  file_event-dispatch.html

(See comment 12 where the suggestion was either file_events or file_event-dispatch)

I should have another look at this patch after adding the test mentioned in comment 26 for cancelled and restarted animations.
Attachment #8802806 - Flags: review?(bbirtles)
Status: NEW → ASSIGNED
Comment on attachment 8802806 [details]
Bug 1202333 part 3 - Add CSS-Animations event tests.

https://reviewboard.mozilla.org/r/87114/#review93304

Thanks, Brian,

I thought that we don't need negative playback rate tests, since this change won't effect playback rate.
However, We will need to create sanity check for negative playback rate.

So I'll add the these tests in this patch.

> As mentioned in comment 12, can we just always pass in 'anim 10s'?
> 
> (Or, if you disagree, please say so! It's fine to disagree, but where there is review feedback, please either follow it or add a comment saying why you decided not to.)

Oh, I specified 'anim 10' to each tests, but  I forgot this statement to remove it from this function.
Comment on attachment 8802805 [details]
Bug 1202333 part 2 - Update the CSSTransition::QueueEvents to specification.

https://reviewboard.mozilla.org/r/87112/#review93298

> Any reason you didn't drop the braces as indicated in comment 11?
> 
> (If there is, please say so!)

Sorry, I misread your comment.
I've fixed it.
Comment on attachment 8817928 [details]
Bug 1202333 part 4 - Add test of event order for CSS-Animation.

https://reviewboard.mozilla.org/r/98096/#review98332

As discussed, cancelling this review for now.
Attachment #8817928 - Flags: review?(bbirtles)
Comment on attachment 8802805 [details]
Bug 1202333 part 2 - Update the CSSTransition::QueueEvents to specification.

https://reviewboard.mozilla.org/r/87112/#review98334

The changes here look good to me and I see the requested test has been added to part 3. r=me with the following change made.

::: layout/style/nsAnimationManager.cpp:219
(Diff revision 5)
> -  // or after the animation's active phase).
> -
> -  bool wasActive = mPreviousPhaseOrIteration != PREVIOUS_PHASE_BEFORE &&
> -                   mPreviousPhaseOrIteration != PREVIOUS_PHASE_AFTER;
> -  bool isActive =
> -    computedTiming.mPhase == ComputedTiming::AnimationPhase::Active;
> +  StickyTimeDuration intervalEndTime =
> +    std::max(std::min((EffectEnd() - mEffect->SpecifiedTiming().mDelay),
> +                      computedTiming.mActiveDuration),
> +             zeroDuration);
> +
> +  uint64_t iterationBoundary = (mPreviousIteration > currentIteration)

We need to drop the ()s here.
Attachment #8802805 - Flags: review?(bbirtles) → review+
Comment on attachment 8802806 [details]
Bug 1202333 part 3 - Add CSS-Animations event tests.

https://reviewboard.mozilla.org/r/87114/#review98336

As with the corresponding test for transitions, can we rename this test file from {file,test}_events-dispatch.html to {file,test}_event-dispatch.html to match the spec heading?

In fact, I think I made that comment in comment 27 and comment 12?

Where is the negative playback rate you said you added in comment 28?

::: dom/animation/test/css-animations/file_events-dispatch.html:53
(Diff revision 5)
> +  return [animation, watcher, handler, div];
> +}
> +
> +promise_test(function(t) {
> +  // Add 1ms delay to ensure that the delay is not included in the elapsedTime.
> +  var [animation, watcher, handler, div] = setupAnimation(t, 'anim 10s 1ms');

Here and elsewhere in this file, I think if we're not using |handle| or |div| we don't need to list them here.

i.e. we can just do:

  var [animation, watcher] = setupAnimation(...);

(And so long as we're using ES6 destructuring assignment, we may as well use 'const' here instead of 'var' I guess, since it has better support[1] than destructuring assignment[2].)

[1] https://kangax.github.io/compat-table/es6/#test-const_basic_support
[2] https://kangax.github.io/compat-table/es6/#test-destructuring,_assignment

::: dom/animation/test/css-animations/file_events-dispatch.html:53
(Diff revision 5)
> +  return [animation, watcher, handler, div];
> +}
> +
> +promise_test(function(t) {
> +  // Add 1ms delay to ensure that the delay is not included in the elapsedTime.
> +  var [animation, watcher, handler, div] = setupAnimation(t, 'anim 10s 1ms');

Use 100s here

::: dom/animation/test/css-animations/file_events-dispatch.html:61
(Diff revision 5)
> +    assert_equals(evt.elapsedTime, 0.0);
> +  });
> +}, 'Idle -> Active');
> +
> +promise_test(function(t) {
> +  var [animation, watcher, handler, div] = setupAnimation(t, 'anim 10s');

Use 100s here

::: dom/animation/test/css-animations/file_events-dispatch.html:70
(Diff revision 5)
> +  return watcher.wait_for([ 'animationstart',
> +                            'animationend' ]).then(function() {
> +    assert_equals(handler.animationstart, 0.0);
> +    assert_equals(handler.animationend, 10);
> +  });
> +

Let's drop this blank line.

::: dom/animation/test/css-animations/file_events-dispatch.html:74
(Diff revision 5)
> +  var [animation, watcher, handler, div] =
> +    setupAnimation(t, 'anim 10s 10s paused');

If we drop |handler| and |div| here this should fit on one line.

Likewise elsewhere in this file.

::: dom/animation/test/css-animations/file_events-dispatch.html:75
(Diff revision 5)
> +
> +}, 'Idle -> After');
> +
> +promise_test(function(t) {
> +  var [animation, watcher, handler, div] =
> +    setupAnimation(t, 'anim 10s 10s paused');

Use 100s instead of 10s for both value here?

::: dom/animation/test/css-animations/file_events-dispatch.html:87
(Diff revision 5)
> +  var [animation, watcher, handler, div] =
> +    setupAnimation(t, 'anim 10s 10s paused');

As before and elsewhere in this file, let's use:

  const [animation, watcher, handler] =
    setupAnimation(t, 'anim 100s 100s paused');

i.e.
* use const (not so important)
* drop unnecessary |div|
* use 100s in 10s

Please apply the same to the rest of this file.

::: dom/animation/test/css-animations/file_events-dispatch.html:122
(Diff revision 5)
> +    setupAnimation(t, 'anim 10s paused');
> +
> +  return watcher.wait_for('animationstart').then(function(evt) {
> +    // Seek to After phase.
> +    animation.finish();
> +

Drop this blank line? (To be consistent with other tests)

Likewise below. It doesn't really matter but it's preferable to be consistent if possible.

::: dom/animation/test/css-animations/file_events-dispatch.html:190
(Diff revision 5)
> +  var [animation, watcher, handler, div]
> +    = setupAnimation(t, 'anim 10s 10s 3');

This fits on one line? (Especially when we drop the unused |div| from the list here?

Likewise elsewhere in this file.

::: dom/animation/test/css-animations/file_events-dispatch.html:226
(Diff revision 5)
> +
> +    // FIXME: bug 1302648: Add test for animationcancel event here.
> +
> +    // Restart this animation.
> +    div.style.display = '';
> +    getComputedStyle(div).display;

Is this line needed? Won't we recalculate style on the next tick anyway?
Attachment #8802806 - Flags: review?(bbirtles)
Attachment #8817928 - Attachment is obsolete: true
Comment on attachment 8802805 [details]
Bug 1202333 part 2 - Update the CSSTransition::QueueEvents to specification.

https://reviewboard.mozilla.org/r/87112/#review98334

> We need to drop the ()s here.

Sorry, I really misunderstood your comment.
Comment on attachment 8802806 [details]
Bug 1202333 part 3 - Add CSS-Animations event tests.

https://reviewboard.mozilla.org/r/87114/#review98336

Thanks, Brian,

>Where is the negative playback rate you said you added in comment 28?

Sorry, I mismerged to previous patch.
In this patch, contain negative playback tests.

Could you confirm this test again?
(In reply to Brian Birtles (:birtles, away 13-16 Dec) from comment #36)
> Comment on attachment 8817928 [details]
> Bug 1202333 part 4 - Add test of event order for CSS-Animation.
> 
> https://reviewboard.mozilla.org/r/98096/#review98332
> 
> As discussed, cancelling this review for now.

I brushed up this patch.
Could you confirm?
Comment on attachment 8802806 [details]
Bug 1202333 part 3 - Add CSS-Animations event tests.

https://reviewboard.mozilla.org/r/87114/#review99044

This looks beautiful. Nice work.

When you run this through try, please retrigger a few times on Android to make sure we don't hit timeouts there.

::: dom/animation/test/css-animations/file_event-dispatch.html:245
(Diff revision 6)
> +    // Seek to before
> +    animation.currentTime = 100 * MS_PER_SEC - 1;
> +    return watcher.wait_for('animationend');
> +  }).then(function(evt) {
> +    assert_equals(evt.elapsedTime, 0);
> +    assert_equals(animation.playState, 'running'); // delay

s/delay/delay phase/
would be more clear
Attachment #8802806 - Flags: review?(bbirtles) → review+
Comment on attachment 8818481 [details]
Bug 1202333 part 4 - Add test of event order for CSS-Animation.

https://reviewboard.mozilla.org/r/98540/#review99046

As discussed and described below, it seems like it would be better to store the events in an array and then assert their order based on that?

::: dom/animation/test/css-animations/file_event-order.html:20
(Diff revision 1)
> +
> +/**
> + * 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 elapsedTime
> + * of event and receive time of the most recent event.

s/receive/received/

::: dom/animation/test/css-animations/file_event-order.html:22
(Diff revision 1)
> +function AnimationEventHandler(target) {
> +  this.target = target;
> +  this.start = this.end = this.iteration = {};
> +  ['start', 'iteration', 'end'].forEach(name => {
> +    this.target['onanimation' + name] = function(evt) {
> +      this[name].elapsedTime = evt.elapsedTime;
> +      this[name].receiveTime = performance.now();
> +    }.bind(this);
> +  });
> +}
> +AnimationEventHandler.prototype.clear = function() {
> +  this.start     = { elapsedTime: undefined, receiveTime: undefined};
> +  this.iteration = { elapsedTime: undefined, receiveTime: undefined};
> +  this.end       = { elapsedTime: undefined, receiveTime: undefined};
> +}

Is it possible to rework this to just store the received events in an array? The receiveTime thing is ok (although it should probably be called receivedTime) but it seems a bit indirect.

We are testing the order of events, so I think it would make more sense to just stick them all in some array and then check they have the expected order?

I guess what I'm thinking of is something like check_events.[1] However, while check_events uses a global array for received events, I think we could probably use a test-local array that we pass to setupAnimation.

The main work would be writing a comparison function like check_events. e.g. something like:

  let events = [];
  const [...] = setupAnimation(t, '...', events);

  assert_events([ 'animationstart', div1, 0 ],
                [ 'animationend', div1, 0 ], events);

  // When we want to ignore previous events in the array
  // (Or should we make assert_events clear |events| ?)
  events = [];

[1] http://searchfox.org/mozilla-central/rev/36bfd0a8da5490274ca49c100957244253845e52/layout/style/test/animation_utils.js
Attachment #8818481 - Flags: review?(bbirtles)
Attachment #8810739 - Attachment is obsolete: true
Attachment #8818481 - Attachment is obsolete: true
Comment on attachment 8818481 [details]
Bug 1202333 part 4 - Add test of event order for CSS-Animation.

https://reviewboard.mozilla.org/r/98540/#review99046

> Is it possible to rework this to just store the received events in an array? The receiveTime thing is ok (although it should probably be called receivedTime) but it seems a bit indirect.
> 
> We are testing the order of events, so I think it would make more sense to just stick them all in some array and then check they have the expected order?
> 
> I guess what I'm thinking of is something like check_events.[1] However, while check_events uses a global array for received events, I think we could probably use a test-local array that we pass to setupAnimation.
> 
> The main work would be writing a comparison function like check_events. e.g. something like:
> 
>   let events = [];
>   const [...] = setupAnimation(t, '...', events);
> 
>   assert_events([ 'animationstart', div1, 0 ],
>                 [ 'animationend', div1, 0 ], events);
> 
>   // When we want to ignore previous events in the array
>   // (Or should we make assert_events clear |events| ?)
>   events = [];
> 
> [1] http://searchfox.org/mozilla-central/rev/36bfd0a8da5490274ca49c100957244253845e52/layout/style/test/animation_utils.js

I've changed to store the event to function's variable, and this patch check event order and event property on check_events funciton.

This patch used setting length to zero for clearing the events array, since tests specify argument of received events array when calling setupAnimation function.
(The object will different, if we use assingment operator for clearing.)
Comment on attachment 8819129 [details]
Bug 1202333 part 4 - Add test of event order for CSS-Animation.

https://reviewboard.mozilla.org/r/98964/#review99642

Looking good but we need to make sure this is not going to fail intermittently when automation servers are under load.

::: dom/animation/test/css-animations/file_event-order.html:17
(Diff revision 1)
> + * This function enable to check the received events array.
> + * Check the event's type / target element / elapsed time.
> + *  expectedEvents: [event type, target element, elapsed time]...

/**
 * Asserts that the set of actual and received events match.
 * @param actualEvents   An array of the received AnimationEvent objects.
 * @param expectedEvents A series of array objects representing the expected
 * events, each having the form:
 *   [ event type, target element, elapsed time]
 */

::: dom/animation/test/css-animations/file_event-order.html:21
(Diff revision 1)
> +/*
> + * This function enable to check the received events array.
> + * Check the event's type / target element / elapsed time.
> + *  expectedEvents: [event type, target element, elapsed time]...
> + */
> +function check_events(actualEvents, ...expectedEvents) {

Let's call this either checkEvents or assert_events. We generally use camelCase for functions (there are some exceptions in layout/style/test but they're wrong). However, if we're adding new assertion methods / test methods we often use assert_* just to match the style used in testharness.js.

I think I prefer checkEvents.

::: dom/animation/test/css-animations/file_event-order.html:22
(Diff revision 1)
> + * This function enable to check the received events array.
> + * Check the event's type / target element / elapsed time.
> + *  expectedEvents: [event type, target element, elapsed time]...
> + */
> +function check_events(actualEvents, ...expectedEvents) {
> +  assert_equals(actualEvents.length, expectedEvents.length);

Can we add a comment here, 'Number of expected and received events should match' ?

Or, ideally, we could produce something that helps us debug this test if it fails on tryserver. For example, could we list just the event names?

e.g.

  `Number of actual events (${actualEvents.length}: \
${actualEvents.map(event => event.type).join(', ')}) should match expected \
events (${expectedEvents.length}: ${expectedEvents.map(event => event[0])})`;

::: dom/animation/test/css-animations/file_event-order.html:23
(Diff revision 1)
> +  actualEvents.forEach((actualEvent, i) => {
> +    assert_equals(expectedEvents[i][0], actualEvent.type);
> +    assert_equals(expectedEvents[i][1], actualEvent.target);
> +    assert_equals(expectedEvents[i][2], actualEvent.elapsedTime);
> +  });

Are we sure that assert_equal aborts this function? If not we could end up trying to read indexes of expectedEvents that don't exist (and generating spurious errors).

It would be good to just check that this produces the expected output when the test fails (including when expectedEvents is shorter than actualEvents).

Also, we should add messages to each of the assert_equals calls here, e.g.

  assert_equals(expectedEvents[i][0], actualEvent.type,
                'Event type should match');

::: dom/animation/test/css-animations/file_event-order.html:31
(Diff revision 1)
> +  var div = addDiv(t, { style: "animation: " + animationStyle });
> +  var watcher = new EventWatcher(t, div, [ 'animationstart',
> +                                           'animationiteration',
> +                                           'animationend' ]);
> +
> +  ['start', 'iteration', 'end'].forEach(name => {
> +    div['onanimation' + name] = function(evt) {
> +    receiveEvents.push({ type:        evt.type,
> +                         target:      evt.target,
> +                         elapsedTime: evt.elapsedTime });
> +    }.bind(this);
> +  });
> +
> +  var animation = div.getAnimations()[0];

Nit: We're using let/const in the rest of the file, perhaps we should do s/var/const/ here to be consistent.

::: dom/animation/test/css-animations/file_event-order.html:51
(Diff revision 1)
> +  const [animation1, watcher1, div1] =
> +    setupAnimation(t, 'anim 5s 2 paused', events);
> +  const [animation2, watcher2, div2] =
> +    setupAnimation(t, 'anim 5s 2 paused', events);

Let's make the duration at least 100s long.

::: dom/animation/test/css-animations/file_event-order.html:66
(Diff revision 1)
> +    return  Promise.all([ watcher1.wait_for('animationiteration'),
> +                          watcher2.wait_for('animationiteration') ]);

Nit: Extra space between 'return' and 'Promise' here.

::: dom/animation/test/css-animations/file_event-order.html:74
(Diff revision 1)
> +    check_events(events, ['animationiteration', div1, 5],
> +                         ['animationiteration', div2, 5]);
> +
> +    events.length = 0;  // Clear received event array
> +
> +    // Make Idle

Finish doesn't make the animation idle, just 'finished'.
(Personally I would just drop the comment. It's obvious what this is doing.)

::: dom/animation/test/css-animations/file_event-order.html:88
(Diff revision 1)
> +  const [animation1, watcher1, div1] =
> +    setupAnimation(t, 'anim 5s 6s', events);
> +  const [animation2, watcher2, div2] =
> +    setupAnimation(t, 'anim 5s 2', events);
> +
> +  return watcher2.wait_for('animationstart').then(function(evt) {
> +    // Seek to start of animation1
> +    animation1.currentTime = 6 * MS_PER_SEC;
> +    animation2.currentTime = 6 * MS_PER_SEC;
> +
> +    events.length = 0;  // Clear received event array
> +
> +    return Promise.all([ watcher1.wait_for('animationstart'),
> +                         watcher2.wait_for('animationiteration') ]);

This seems fragile--if automation is under heavy load then we might have more than 5s between frames and hence we could end queuing up animationiteration and animationend events before running the second part of the test (and then seeking back to 6s will generate more events). Let's make the animations at least 100s long and have a 110s delay?

::: dom/animation/test/css-animations/file_event-order.html:106
(Diff revision 1)
> +                         watcher2.wait_for('animationiteration') ]);
> +  }).then(function() {
> +    check_events(events, ['animationiteration', div2, 5],
> +                         ['animationstart',     div1, 0]);
> +  });
> +}, 'Fire the start and iteration event');

'Test start and iteration events are ordered by time'

::: dom/animation/test/css-animations/file_event-order.html:110
(Diff revision 1)
> +  const [animation1, watcher1, div1] =
> +    setupAnimation(t, 'anim 5s 4s', events);
> +  const [animation2, watcher2, div2] =
> +    setupAnimation(t, 'anim 5s 2', events);

Duration should be at least 100s.

::: dom/animation/test/css-animations/file_event-order.html:126
(Diff revision 1)
> +    // Seek to end of animation1
> +    animation1.currentTime = 9 * MS_PER_SEC;
> +    animation2.currentTime = 9 * MS_PER_SEC;
> +
> +    return Promise.all([ watcher1.wait_for('animationend'),
> +                         watcher2.wait_for('animationiteration') ]);

This seems fragile too--isn't there a chance we'll get the animationend for div2 as well within the same frame? (since it occurs only 1s later)

We need to make all this work properly even when there are large delays between frames.

::: dom/animation/test/css-animations/file_event-order.html:136
(Diff revision 1)
> +                         watcher2.wait_for('animationiteration') ]);
> +  }).then(function() {
> +    check_events(events, ['animationiteration', div2, 5],
> +                         ['animationend',       div1, 5]);
> +  });
> +}, 'Fire the iteration and end event');

'Test end and iteration events are ordered by time'

(This seems a bit redundant with the previous test? Do we need it?)

::: dom/animation/test/css-animations/file_event-order.html:140
(Diff revision 1)
> +  const [animation1, watcher1, div1] =
> +    setupAnimation(t, 'anim 5s 4s', events);
> +  const [animation2, watcher2, div2] =
> +    setupAnimation(t, 'anim 5s 2', events);

Duration should be at least 100s.

::: dom/animation/test/css-animations/file_event-order.html:158
(Diff revision 1)
> +    check_events(events, ['animationstart', div2, 0],
> +                         ['animationstart', div1, 0],
> +                         ['animationend',   div1, 5],
> +                         ['animationend',   div2, 10]);
> +  });
> +}, 'Fire the start and end event');

'Test start and end events are sorted correctly when fired simultaneously'
Attachment #8819129 - Flags: review?(bbirtles)
Comment on attachment 8819129 [details]
Bug 1202333 part 4 - Add test of event order for CSS-Animation.

https://reviewboard.mozilla.org/r/98964/#review99642

> This seems fragile--if automation is under heavy load then we might have more than 5s between frames and hence we could end queuing up animationiteration and animationend events before running the second part of the test (and then seeking back to 6s will generate more events). Let's make the animations at least 100s long and have a 110s delay?

I used 200ms instead of previous time.
Comment on attachment 8819129 [details]
Bug 1202333 part 4 - Add test of event order for CSS-Animation.

https://reviewboard.mozilla.org/r/98964/#review99722

r=me with the following changes

::: dom/animation/test/css-animations/file_event-order.html:26
(Diff revision 2)
> + *          [ event type, target element, elapsed time ]
> + */
> +function checkEvents(actualEvents, ...expectedEvents) {
> +  assert_equals(actualEvents.length, expectedEvents.length,
> +                `Number of actual events (${actualEvents.length}: \
> +${actualEvents.map(event => event.type).join(', ')}) should match expected\

Shouldn't there be a space before '\' ?

Did you check that the error message produced looks correct when the test fails?

::: dom/animation/test/css-animations/file_event-order.html:141
(Diff revision 2)
> +                         watcher2.wait_for('animationiteration') ]);
> +  }).then(function() {
> +    checkEvents(events, ['animationiteration', div2, 200],
> +                        ['animationend',       div1, 200]);
> +  });
> +}, 'Test iteration and end events are ordered by time.');

This actually tests the order of start times too in the first part of the test.

We should either update the description to say that this also tests the ordering of start times, or update the test so that the two animations start at the same time so we are really focusing on just the last part, e.g. the following two animations

  'anim 150s'
  'anim 100s 2'

Then seek to 150s.
Attachment #8819129 - Flags: review?(bbirtles) → review+
Comment on attachment 8819129 [details]
Bug 1202333 part 4 - Add test of event order for CSS-Animation.

https://reviewboard.mozilla.org/r/98964/#review99722

> This actually tests the order of start times too in the first part of the test.
> 
> We should either update the description to say that this also tests the ordering of start times, or update the test so that the two animations start at the same time so we are really focusing on just the last part, e.g. the following two animations
> 
>   'anim 150s'
>   'anim 100s 2'
> 
> Then seek to 150s.

Ah, As you mentioned, this test also verified the start event. So I've focused on last part.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c3964af26a3a&group_state=expanded

In Fennec, fail the reftest related this changes. [1]

[1] layout/reftests/transform-3d/animate-preserve3d-parent.html | load failed: timed out waiting for reftest-wait to be removed
https://treeherder.mozilla.org/logviewer.html#?job_id=33013542&repo=try#L1965

This tests set the animation with negative delay. (animation: spin 100s -99.9s linear 2). [2]
And it used animationiteration event for finishing the tests.

[2] https://dxr.mozilla.org/mozilla-central/rev/d4b3146a5567a7ddbcdfa5244945db55616cb8d1/layout/reftests/transform-3d/animate-preserve3d-parent.html#21

However, we will remove the excessive animationiteration event. So this test may not receive the iteration event if tick duration is longer than animation duration.
Comment on attachment 8802804 [details]
Bug 1202333 part 1 - Remove excessive animationiteration event.

Hi Brian,

I added a slight change of first tests patch.
This is caused by the failure of reftests. See comment #63, for detail.

Could you confirm this patch again?
Attachment #8802804 - Flags: review+ → review?(bbirtles)
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #67)
> Comment on attachment 8802804 [details]
> Bug 1202333 part 1 - Remove excessive animationiteration event.
> 
> Hi Brian,
> 
> I added a slight change of first tests patch.
> This is caused by the failure of reftests. See comment #63, for detail.
> 
> Could you confirm this patch again?

Sorry, I've forgot changing the another tests.
https://reviewboard.mozilla.org/r/87098/diff/9-11/
Comment on attachment 8802804 [details]
Bug 1202333 part 1 - Remove excessive animationiteration event.

https://reviewboard.mozilla.org/r/87110/#review100064

As discussed, I don't think this is the correct fix.

::: layout/reftests/transform-3d/animate-backface-hidden.html:20
(Diff revision 10)
>  #test {
>    background: blue;
>    height: 200px; width: 200px;
>    backface-visibility: hidden;
>    /* use a -99.9s delay to start at 99.9% and then move to 0% */
> -  animation: flip 100s -99.9s linear 2;
> +  animation: flip 100s -99.9s linear 3;

I think this might only work because we actually wait 100s for the next iteration--obviously that's too long for a single test!

Instead, can we fix this by initially pausing the the animation:

  animation: flip 100s -99.9s linear 2 paused;

then waiting for it to start:

  document.getElementById("test").addEventListener("animationstart", StartListener, false);

then unpausing it and waiting for the next iteration:

  function StartListener(event) {
    var test = document.getElementById("test");
    test.style.animationPlayState = 'running';
    test.addEventListener("animationiteration", IterationListener, false);
  }

(And by that point we should just add a global variable for 'test' instead of calling getElementById() twice. In fact 'test' is already a global variable since elements with IDs get exported like that but we probably shouldn't rely on that behavior.)
Attachment #8802804 - Flags: review?(bbirtles) → review+
Comment on attachment 8802804 [details]
Bug 1202333 part 1 - Remove excessive animationiteration event.

https://reviewboard.mozilla.org/r/87110/#review100064

Thanks, Brian,

I've addressed.
Pushed by mantaroh@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/2a4134929fd2
part 1 - Remove excessive animationiteration event. r=birtles
https://hg.mozilla.org/integration/autoland/rev/c2811a3d8da3
part 2 - Update the CSSTransition::QueueEvents to specification. r=birtles
https://hg.mozilla.org/integration/autoland/rev/4f3f38c65d7a
part 3 - Add CSS-Animations event tests. r=birtles
https://hg.mozilla.org/integration/autoland/rev/20774bffb62a
part 4 - Add test of event order for CSS-Animation. r=birtles
FWIW, the tests seem to fail quite badly when we update our Promise handling to follow the spec
(== use microtask scheduling).
https://treeherder.mozilla.org/logviewer.html#?job_id=33320442&repo=try#L9473

I'll investigate a bit why the tests fail.
And the fix is simple:

diff --git a/dom/animation/test/css-animations/file_event-dispatch.html b/dom/animation/test/css-animations/file_event-dispatch.html
--- a/dom/animation/test/css-animations/file_event-dispatch.html
+++ b/dom/animation/test/css-animations/file_event-dispatch.html
@@ -34,20 +34,20 @@ function AnimationEventHandler(target) {
 AnimationEventHandler.prototype.clear = function() {
   this.animationstart = undefined;
   this.animationiteration = undefined;
   this.animationend = undefined;
 }
 
 function setupAnimation(t, animationStyle) {
   var div = addDiv(t, { style: "animation: " + animationStyle });
+  var handler = new AnimationEventHandler(div);
   var watcher = new EventWatcher(t, div, [ 'animationstart',
                                            'animationiteration',
                                            'animationend' ]);
-  var handler = new AnimationEventHandler(div);
   var animation = div.getAnimations()[0];
 
   return [animation, watcher, handler, div];
 }

I'll incorporate the fix in other test fixes I need to do.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: