1073379 - Make AnimationPlayer.startTime writeable

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: part 1 - Make startTime setable

I'm still working on the refactoring for correct firing of the events defined by the CSS animations and CSS transitions specs.

Comment 2

[Jonathan Watt [:jwatt]]

Note to self - make sure that we're also flushing styles as necessary. See the other code in CSSAnimationPlayer that does this.

Comment 3

[Brian Birtles (:birtles)]

Comment on attachment 8537868 [details] [diff] [review]
part 1 - Make startTime setable

># HG changeset patch
># Parent 139e84c2fe8196ead2a0aabde4182e658945e49c
># User Jonathan Watt <[e-mail]>
>
>diff --git a/dom/animation/AnimationPlayer.h b/dom/animation/AnimationPlayer.h
>--- a/dom/animation/AnimationPlayer.h
>+++ b/dom/animation/AnimationPlayer.h
>@@ -58,16 +58,43 @@ public:
> 
>   virtual CSSAnimationPlayer* AsCSSAnimationPlayer() { return nullptr; }
>   virtual CSSTransitionPlayer* AsCSSTransitionPlayer() { return nullptr; }
> 
>   // AnimationPlayer methods
>   Animation* GetSource() const { return mSource; }
>   AnimationTimeline* Timeline() const { return mTimeline; }
>   Nullable<TimeDuration> GetStartTime() const { return mStartTime; }
>+  void SetStartTime(TimeDuration aNewStartTime) {

This should go in the .cpp file

>+    Nullable<TimeDuration> timeline_time;

Why not timelineTime?

>+    if (mTimeline) {
>+      // The spec says to check if the timeline is active (has a resolved time)
>+      // before using it here, but we don't need to since it's harmless to set
>+      // the already null time to null.
>+      timeline_time = mTimeline->GetCurrentTime();
>+    }

Until we do bug 1096776 it's better to just assert mTimeline && !mTimeline->GetCurrentTime.IsNull(). We're not very consistent about this but I think we shouldn't pretend we support inactive/missing timelines when we don't.

>+    if (timeline_time.IsNull() && !aNewStartTime.IsNull()) {
>+      mHoldTime.SetNull();
>+    }

Doing that would mean we could probably drop this part.

>+    Nullable<TimeDuration> previousCurrentTime = GetCurrentTime();
>+    mStartTime = aNewStartTime;
>+    if (!aNewStartTime.IsNull()) {
>+      if (rate != 0) {

Where is rate defined? We don't support playbackRate yet so we can skip this check.

>+        mHoldTime.SetNull();
>+      } else {
>+        mHoldTime = previousCurrentTime;
>+      }
>+    }

I'm pretty sure the statement, "mHoldTime = previousCurrentTime;" should happen if aNewStartTime.IsNull() is true, i.e. the outer if-block.

>+    // FIXME: call CancelPendingPlay() once bug 927349 part 5 lands
>+    // if mReady, call mReady->MaybeResolve(this); too
>+
>+    // FIXME: Once bug 1074630 is fixed, run the procedure to update a playerâs

Is there some non-ASCII character here?

This should probably call UpdateSourceContent too.

We need to define SetStartTimeAsDouble as well and call PostUpdate there. See attachment 8536249 [details] [diff] [review] for an example.

>diff --git a/dom/webidl/AnimationPlayer.webidl b/dom/webidl/AnimationPlayer.webidl
>--- a/dom/webidl/AnimationPlayer.webidl
>+++ b/dom/webidl/AnimationPlayer.webidl
>@@ -15,17 +15,17 @@ enum AnimationPlayState { "idle", "pendi
> [Pref="dom.animations-api.core.enabled"]
> interface AnimationPlayer {
>   // Bug 1049975
>   //           attribute AnimationNode?     source;
>   [Pure]
>   readonly attribute Animation? source;
>   readonly attribute AnimationTimeline timeline;
>   [BinaryName="startTimeAsDouble"]
>-  readonly attribute double? startTime;
>+  attribute double? startTime;

This will need review from a DOM peer.

I think this needs a bunch of tests. I started on these a while ago. I'm upload what I wrote so far but bear in mind it needs to be rewritten based on bug 927349. And there are heaps of other tests still needed that aren't there yet (e.g. checking that redundant changes--set startTime to x then back again--don't trigger events).

Comment 4

[Brian Birtles (:birtles)]

Attachment: Tests for making startTime writeable (incomplete)

(In reply to Brian Birtles (:birtles) from comment #3)
> I think this needs a bunch of tests. I started on these a while ago. I'm
> upload what I wrote so far but bear in mind it needs to be rewritten based
> on bug 927349. And there are heaps of other tests still needed that aren't
> there yet (e.g. checking that redundant changes--set startTime to x then
> back again--don't trigger events).

Oops, forgot to do this.

Comment 5

[Brian Birtles (:birtles)]

(In reply to Brian Birtles (:birtles) from comment #4)
> Created attachment 8556732 [details] [diff] [review]
> Tests for making startTime writeable (incomplete)
> 
> (In reply to Brian Birtles (:birtles) from comment #3)
> > I think this needs a bunch of tests. I started on these a while ago. I'm
> > upload what I wrote so far but bear in mind it needs to be rewritten based
> > on bug 927349. And there are heaps of other tests still needed that aren't
> > there yet (e.g. checking that redundant changes--set startTime to x then
> > back again--don't trigger events).
> 
> Oops, forgot to do this.

Also, those tests add their own setTimeout which, apparently, is not a good idea. We should rely on the testharness timeout instead.

Comment 6

[Jonathan Watt [:jwatt]]

Attachment: part 1 - Add an AnimationUtils::DoubleToTimeDuration helper

Comment 7

[Jonathan Watt [:jwatt]]

Attachment: part 2 - Make AnimationPlayer.startTime writeable

Comment 8

[Jonathan Watt [:jwatt]]

Comment on attachment 8556761 [details] [diff] [review]
part 2 - Make AnimationPlayer.startTime writeable

For DOM changes.

Comment 9

[Brian Birtles (:birtles)]

Comment on attachment 8556761 [details] [diff] [review]
part 2 - Make AnimationPlayer.startTime writeable

>Bug 1073379, part 2 - Implement web-animations proceedure to 'update the player start time' and make AnimationPlayer.startTime writeable. r=birtles
...
>+void
>+AnimationPlayer::SetStartTime(Nullable<TimeDuration> aNewStartTime)
>+{

I think 'const Nullable<TimeDuration>& aNewStartTime' might be a little cheaper here.

>+  Nullable<TimeDuration> timelineTime;
>+#if 1
>+  MOZ_ASSERT(mTimeline && !mTimeline->GetCurrentTime().IsNull(),
>+             "We don't support inactive/missing timelines yet (bug 1096776)");
>+  timelineTime = mTimeline->GetCurrentTime();
>+#else
>+  if (mTimeline) {
>+    // The spec says to check if the timeline is active (has a resolved time)
>+    // before using it here, but we don't need to since it's harmless to set
>+    // the already null time to null.
>+    timelineTime = mTimeline->GetCurrentTime();
>+  }
>+  if (timelineTime.IsNull() && !aNewStartTime.IsNull()) {
>+    mHoldTime.SetNull();
>+  }
>+#endif

We don't actually use timelineTime later in this method so we could drop the first branch altogether. In fact, I'd be fine with just a comment such as:

// Bug 1096776: Once we support inactive/missing timelines we need
// to add the following step here:
// "If timeline time is unresolved and new start time is resolved,
//  make player’s hold time unresolved."

If you prefer to leave the disabled code though, that's fine.

>+  CancelPendingPlay();
>+  if (mReady) {
>+    mReady->MaybeResolve(this);
>+  }

I double-checked that if we've already resolved the ready promise that MaybeResolve is a no-op and that appears to be the case. Feel free to add a comment to that effect if you think it's necessary.

Also, when we come to write the tests for this, I'd particularly like to make sure this line gets tested: i.e. create a CSS animation, check it is pending, then manually force the start time to, say, the current timeline time, and check that the promise gets resolved.

>+void
>+AnimationPlayer::SetStartTimeAsDouble(Nullable<double> aStartTime)
>+{

This too could be 'const Nullable<double>& aStartTime' here for consistency (I suspect it wouldn't actually make any difference in terms of performance).

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8556761 [details] [diff] [review]
part 2 - Make AnimationPlayer.startTime writeable

the
#if 1
#else
#endif
part is a bit odd.

Couldn't you just drop the #if 1 part, and if you really need
to assert the expected current behavior, leave the MOZ_ASSERT

And yes, Nullable<double>& aStartTime.

And this needs tests, including testing setting null value.

Comment 11

[Jonathan Watt [:jwatt]]

Brian, one thing that came up when I was discussing the tests with jgraham is that he thinks they should go in the W3C Web Platform repo rather than in dom/animation/test/css-animations. Thoughts?

Comment 12

[Jonathan Watt [:jwatt]]

Attachment: WIP part 4 - startTime tests for CSS animations

I've been tying myself in knots trying to get this testing right. It turns out testing the timing, order and relationship between multiple async events (in the generic sense of the word "events") is really hard. I've also ended up rewriting the tests multiple times over based on subtle misunderstandings of the workings of Web Animations. Anyway, this is where I'm at. I've finally got the skipping forwards logic sorted out and passing. The skipping backwards and other TODO marked items in this diff are still to come. I also want to try and integrate some way of checking that "events" happen within, say, three paint ticks, but trying to integrate that into the control flow may just finally blow my mind.

Comment 13

[Brian Birtles (:birtles)]

(In reply to Jonathan Watt [:jwatt] from comment #11)
> Brian, one thing that came up when I was discussing the tests with jgraham
> is that he thinks they should go in the W3C Web Platform repo rather than in
> dom/animation/test/css-animations. Thoughts?

That's what we'll do ultimately. Currently, the main problem is there's no spec covering any of this (i.e. CSS and Web Animations integration) so it's not clear where it should go in the web platform repo.

Comment 14

[Jonathan Watt [:jwatt]]

Attachment: part 3 - flush style as necessary for startTime changes

Comment 15

[Jonathan Watt [:jwatt]]

Attachment: part 4 - dispatch CSS animations events as appropriate on backwards setting of startTime

Comment 16

[Jonathan Watt [:jwatt]]

Attachment: part 5 - add assert_between_* methods to imptests

Upstream patch, but included here for completeness.

Comment 17

[Jonathan Watt [:jwatt]]

Attachment: part 5 - CSS animation tests

I have one remaining "TODO reenable" comment in here with notes on the failure that I've not figured out the cause of yet.

Comment 18

[Jonathan Watt [:jwatt]]

Attachment: part 5 - CSS animation tests

Comment 19

[Jonathan Watt [:jwatt]]

Attachment: part 3 - call PostUpdate() as necessary for startTime changes to get computed style working

Comment 20

[Daniel Holbert [:dholbert]]

Comment on attachment 8568020 [details] [diff] [review]
part 3 - call PostUpdate() as necessary for startTime changes to get computed style working

r=me, though this needs a commit message.

Assuming your attachment-title is the draft commit message...
>  call PostUpdate() as necessary for startTime changes to get computed style working
...then I'd have two suggestions for tweaking:
- Maybe drop "as necessary" since we do it unconditionally, not "as necessary"
- s/to get computed style working/to prompt AnimationPlayerCollection to update its style rule/

(or something like that)

Comment 21

[Daniel Holbert [:dholbert]]

Comment on attachment 8567761 [details] [diff] [review]
part 4 - dispatch CSS animations events as appropriate on backwards setting of startTime

Review of attachment 8567761 [details] [diff] [review]:
-----------------------------------------------------------------

Stealing review, per discussion w/ jwatt in IRC. r=me w/ the following:

::: layout/style/nsAnimationManager.cpp
@@ +107,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;

Nit -- this line is 81 characters long.  Maybe wrap after the "="?

@@ +109,5 @@
> +  bool wasActive = mPreviousPhaseOrIteration != PREVIOUS_PHASE_BEFORE &&
> +                   mPreviousPhaseOrIteration != PREVIOUS_PHASE_AFTER;
> +  bool isActive = computedTiming.mPhase == ComputedTiming::AnimationPhase_Active;
> +  bool skippedActivePhase =
> +    (mPreviousPhaseOrIteration == PREVIOUS_PHASE_BEFORE &&

Maybe worth adding an assertion like this somewhere, to sanity-check that these flag assignments are internally consistent:

  MOZ_ASSERT(!skippedActivePhase || (!isActive && !wasActive),
             "skippedActivePhase only makes sense if we were & are inactive");

@@ +139,5 @@
> +  } else if (skippedActivePhase) {
> +    // First append an 'animationstart':
> +    StickyTimeDuration elapsedTime =
> +    std::min(StickyTimeDuration(mSource->InitialAdvance()),
> +             computedTiming.mActiveDuration);

3 things:
(1) Indentation is off here -- the last two lines (std::min, etc) need to be indented by 2 extra spaces).

(2) The old code mentions "start of 0th iteration" in a comment here -- keep that around in a comment somehow. (That makes it clearer why this differs from the NS_ANIMATION_START elapsedTime-value further down -- that further-down code needs to consider the iteration we're in now, whereas this code does not.)

(3) Is the min()-clamp with "computedTiming.mActiveDuration" there just to prevent the start time from being later than the end time, for cases where the animation has a negative active-duration? Might be nice to call that out in a comment as well.  Otherwise, it's a bit confusing why this std::min() call is up here, but *not* in the NS_ANIMATION_START chunk down below.

@@ +148,5 @@
> +    // Then have the shared code below append an 'animationend':
> +    message = NS_ANIMATION_END;
> +  } else {
> +    return;
> +  }

Add a comment to explain this early-return "else" case. Even just " // No events need to be sent." (or more of an explanation if there's an easy way to describe this case briefly)

@@ +159,5 @@
> +                                    computedTiming.mCurrentIteration;
> +    elapsedTime = StickyTimeDuration(std::max(iterationStart,
> +                                              mSource->InitialAdvance()));
> +  } else {
> +    elapsedTime = computedTiming.mActiveDuration;

Assert that message == NS_ANIMATION_END in this "else" clause. (both for documentation & for sanity-checking purposes.)

Comment 22

[Daniel Holbert [:dholbert]]

Comment on attachment 8568007 [details] [diff] [review]
part 5 - CSS animation tests

Review of attachment 8568007 [details] [diff] [review]:
-----------------------------------------------------------------

Partial review of the test patch -- I'm not all the way through it yet, but I figured I'd post some of my review in case you're still awake & waiting for review feedback:

::: dom/animation/test/css-animations/test_animation-player-starttime.html
@@ +6,5 @@
> +
> +div {
> +  margin-left: 10px;
> +  /* Make it easier to calculate expected values: */
> +  animation-timing-function: linear ! important;

Note that this test has the following explicit HTML:
  <div id="log"></div>

Your "div {" style rule here will apply to that log-<div> as well -- I doubt that's what you intended. Doesn't matter too much right now, but might matter down the line if someone adds more styling to this rule, to further tweak the tested divs. So, we should probably change the selector here to "div.test" or "div.animTarget", and adjust your addDiv() function to give your generated divs a matching class attribute.

@@ +45,5 @@
> +// When we assert_between_inclusive using these values we could in theory cause
> +// intermittent orange due to long refresh driver delays, but since the active
> +// duration is 1000s long, a delay would need to be around 100s to cause that.
> +// If that's happening than we have issues that we should solve anyway, so a
> +// failure to make us look into that seems like a good thing.

I think we need to make the randomorange threshold way higher here.

You're depending on our test machines *never* falling below 10fps (100ms between refresh driver ticks) here.  I'll bet there are plenty of mundane reasons why a test-runner might occasionally, briefly, have a random-delay that triggers a worse-than-10fps frame rate. Given that we have test runners running zillions of times a day, I'm not be comfortable with using this as the threshold (or with decreeing that any such instance is investigation-worthy).

I'd suggest that a random-delay on the order of 10-20 seconds (at a minimum) should be our threshold for test-failure here.  So, we need to scale up our animation timing values here to match that sort of a timeline. Though if there are any places where we simply let the animation play for a fraction of its duration (I don't think there are any such places?), we should make sure that this scaling-up doesn't scale-up the time that we're waiting in those spots.

@@ +63,5 @@
> +  return div;
> +}
> +
> +function waitForNextPaintTick()
> +{

This function is unused, so let's delete it. (unless you intended to use it, in which case let's use it :))

@@ +72,5 @@
> +  });
> +}
> +
> +/**
> + * CSS animantion events fire asynchronously after we set 'startTime'. This

Typo: s/animantion/animation/

@@ +100,5 @@
> + */
> +function EventWatcher(watchedNode, eventTypes)
> +{
> +  this.watchedNode = watchedNode;
> +  this.eventTypes = eventTypes;

Do we actually need the "this." versions of these variables?

It looks like these variables are only referenced inside the scope of function EventWatcher() { ... }, and never by their "this." names, which I think means all references are using the args instead of the "this." versions. (though I may be wrong; JS scoping is weird and I could easily be misunderstanding)

Anyway; if these are indeed unused in practice, we should probably drop them.

@@ +189,5 @@
> +// Called when startTime is set to the time the start delay would ideally
> +// start (not accounting for any delay to next paint tick).
> +function checkStateOnSettingStartTimeToAnimationCreationTime(player)
> +{
> +  // We don't test player.startTime since our caller just set it.

Seems like it'd still be worth testing startTime here -- even though we expect that the caller just set it -- because the setter & getter for startTime are non-trivial; it's not like they just set & read a JS variable. They exercise our C++ AnimationPlayer::SetStartTime/GetStartTime code, and it's conceivable that those functions could be broken in a way that you could detect here if you didn't omit this check.

(Also, less-importantly: the caller might've goofed, and it's nice to documenting/verify an expectation that the rest of this function depends on.)

So, I'd suggest actually including a check for startTime here (maybe with a comment saying it *should* trivially be satisfied since we expect the caller to have just set startTime). Alternately, I'd also be happy with a test somewhere else that "player.startTime" can be set & then read back faithfully.  (though it seems like it's worth testing this in a variety of scenarios, and testing it in these "checkStateOn" functions would be handy for that...)

@@ +203,5 @@
> +  var div = player.source.target;
> +  var marginLeft = parseFloat(getComputedStyle(div).marginLeft);
> +  assert_equals(marginLeft, 10,
> +                'the computed value of margin-left should be unaffected ' +
> +                'at the beginning of the start delay');

"10" is kinda magic here. You have "INITIAL_POSITION", "TEN_PCT_POSITION", etc. used during the animation -- maybe use "UNANIMATED_POSITION" for 10?

(Same goes for the "10" check in checkStateOnReadyPromiseResolved and checkStateAtActiveIntervalEndTime)

@@ +261,5 @@
> +}
> +
> +
> +function checkStateAtNinetyPctOfActiveInterval(player)
> +{

nit:  you've got 2 blank lines before this method, for no apparent reason. Probably collapse to 1 blank line.

@@ +323,5 @@
> +
> +  player.startTime = animationCreationTime;
> +  checkStateOnSettingStartTimeToAnimationCreationTime(player);
> +
> +  div.remove();

According to MDN...
 https://developer.mozilla.org/en-US/docs/Web/API/ChildNode/remove#Browser_compatibility
...remove() isn't supported everywhere -- it's supported not in IE, at least. Given that this is a testharness.js test & hence can in theory be run w/ other browsers, maybe better to avoid using remove(). (and use div.parentNode.removeChild(div) instead)

::: dom/animation/test/mochitest.ini
@@ +12,1 @@
>  [css-animations/test_animation-player-ready.html]

Looks like we should insert the new test 1 line further down (since "-starttime" sorts after "-ready", alphabetically).

Comment 23

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #22)
> I think we need to make the randomorange threshold way higher here.
> 
> You're depending on our test machines *never* falling below 10fps (100ms

Sorry, disregard this bit. Per IRC, I misread your mention of "100s" as "100ms".

Comment 24

[Daniel Holbert [:dholbert]]

Comment on attachment 8568007 [details] [diff] [review]
part 5 - CSS animation tests

Review of attachment 8568007 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/animation/test/css-animations/test_animation-player-starttime.html
@@ +339,5 @@
> +  var player = div.getAnimationPlayers()[0];
> +
> +  player.ready.then(function() {
> +    animationStartTime = player.startTime;
> +    checkStateOnReadyPromiseResolved(player);

Do we need a "t.step()" call here somewhere? I haven't reviewed/written many testharness.js tests before, but the documentation seems to say that async tests's assertions all have to happen inside of a call to "t.step()", or via the convenience wrapper "t.step_func()":
http://testthewebforward.org/docs/testharness-documentation.html#asynchronous-tests

It looks like your async tests only bother with step[_func] in an error-handling ".catch" block, whereas the testharness doc seems to suggest that we need it around all assertions (including those in checkStateOnReadyPromiseResolved) when we're running an async_test.

@@ +349,5 @@
> +
> +    player.startTime = document.timeline.currentTime - ANIM_DELAY_MS - ANIM_DUR_MS * 0.5; // 50% through active interval
> +    checkStateAtFiftyPctOfActiveInterval(player);
> +
> +    player.startTime = animationStartTime - ANIM_DELAY_MS - ANIM_DUR_MS * 0.9; // 90% through active interval

You're inconsistent here about use of "document.timeline.currentTime" vs. "animationStartTime". 

Maybe you should just do away with animationStartTime and use document.timeline.currentTime? Because:
 (1) it's not actually the animation start time -- it's approximately the current time. (and you're adjusting startTime so that animationStartTime ends up e.g. 90% of the way through the animation, IIUC)

 (2) it's not even *quite* the current time, I think -- e.g. here, this is after we've resolved a ".then()", which I expect means that document.timeline.currentTime may have advanced by an arbitrary amount.

If you're really wanting to set startTime such that the current time is e.g. 90% of the way through, I think you need to reliably use document.timeline.currentTime (or a per-then(){}-scope snapshot of it).

@@ +362,5 @@
> +    div.remove();
> +    t.done();
> +  }).catch(t.step_func(function(reason) {
> +    assert_true(false, reason);
> +  }));

Do we need "t.done()" in the failure case here, too?

I would expect that without an explicit done(), testharness might just sit and wait until some large threshold expires, and that's wasteful.

@@ +363,5 @@
> +    t.done();
> +  }).catch(t.step_func(function(reason) {
> +    assert_true(false, reason);
> +  }));
> +}, 'Skipping forward through animation', {timeout: 50 * 1000});

This hardcoded 50-second timeout seems a bit hacky/unnecessary.  Can't we just lean on the mochitest-harness 300s timeout here?

See https://groups.google.com/d/msg/mozilla.dev.platform/xEi0jXRkGP8/HsZL7_FpvfkJ for a note from bz advocating against some controversial 30-to-60-second setTimeout calls, to watch for missed event-firing. (You're not using setTimeout directly, but it's pretty analogous.)

@@ +398,5 @@
> +
> +    player.startTime = document.timeline.currentTime - ANIM_DELAY_MS - ANIM_DUR_MS * 0.5; // 50% through active interval
> +    checkStateAtFiftyPctOfActiveInterval(player);
> +
> +    player.startTime = animationStartTime - ANIM_DELAY_MS; // jump to start of active interval

As above, this seems inconsistent on using document.timeline.currentTime vs. animationStartTime here. (this in the "skipping backwards" async test).  I think you might just want to use document.timeline.currentTime.

@@ +408,5 @@
> +    // because we went from inside to outside the active interval.
> +    return eventWatcher.waitForEvent('animationend');
> +  }).then(function() {
> +    animationStartTime = player.startTime;
> +    checkStateOnReadyPromiseResolved(player);

Here, you're setting animationStartTime to something else entirely, and then not using that value AFAICT.  I think this "animationStartTime = player.startTime;" assignment can just be dropped. (?)

@@ +442,5 @@
> +//   * after -> active, then back
> +//
> +// We do all these tests in a single async_test since that allows us to share
> +// the timeout that we use to wait so that this test file isn't delayed by the
> +// timeout time multiplied by number of tests.

I'm unclear on how this test file would be delayed by that amount. That would only happen if we truly don't fire the events, right? (which would already be bad & should be addressed immediately)  And per above, I'm not sure we should be using an explicit timeout time here anyway

(Maybe you're aiming for this test to be runnable in browsers that don't fire these events & still complete [fail] in a bounded amount of time? If so, that makes some sense I suppose, though it's unfortunate that this makes us rely on adding extra timeouts inside of a test.)

@@ +464,5 @@
> +  div2.style.animation = animStr;
> +  div3.style.animation = animStr;
> +  div4.style.animation = animStr;
> +  div5.style.animation = animStr;
> +  div6.style.animation = animStr;

Seems like the ~24 lines of "div{1-6} = ...", "eventWatcher{1-6} = ...", "div{1-6}.style.animation = ...", "player{1-6} = ..." could be condensed a lot using some "new Array(6)" and a "for (let i = 0; i < 6; i++)" loop to populate the arrays.

e.g.
  var divs = new Array(6);
  var eventWatchers = new Array(6);
  var players = new Array(6);
  for (let i = 0; i < 6; i++) {
    divs[i] = addDiv();
    eventWatchers[i] = new EventWatcher(divs[i], CSS_ANIM_EVENTS);
    players[i] = divs[i].getAnimationPlayers()[0];
  }

This would make for shorter & more maintainable code (including cleanup code at the end of this function), I think.

@@ +486,5 @@
> +  player1.startTime = beforeTime;
> +
> +  /* TODO reenable after bug 1134163 is fixed!
> +     Right now the flushing of style in SetStartTime() (to make sure computed
> +     style is updated after setting startTime) means that we can end up

This comment is disabling most of this function -- but is the thing it's warning about actually still a problem?

In particular -- SetStartTime() *no longer flushes styles*, IIUC, per the updated version of part 3 that you posted today. So if this *is* still a problem, then this comment probably needs a correction/clarification, RE "the flushing of style in SetStartTime()".

@@ +538,5 @@
> +    div4.remove();
> +    div5.remove();
> +    div6.remove();
> +    t.done();
> +  }, 1000);

Can we kill this setTimeout(..., 1000) here and make this section event-driven? (letting the mochitest harness time out if some event is missing)

@@ +565,5 @@
> +  var div = addDiv();
> +  div.style.animation = 'anim 100s';
> +
> +  var player = div.getAnimationPlayers()[0];
> +  

^^ whitespace on empty line

Comment 25

[Jonathan Watt [:jwatt]]

(In reply to Daniel Holbert [:dholbert] from comment #22)
> @@ +189,5 @@
> > +// Called when startTime is set to the time the start delay would ideally
> > +// start (not accounting for any delay to next paint tick).
> > +function checkStateOnSettingStartTimeToAnimationCreationTime(player)
> > +{
> > +  // We don't test player.startTime since our caller just set it.
> 
> Seems like it'd still be worth testing startTime here -- even though we
> expect that the caller just set it -- because the setter & getter for
> startTime are non-trivial; it's not like they just set & read a JS variable.
> They exercise our C++ AnimationPlayer::SetStartTime/GetStartTime code, and
> it's conceivable that those functions could be broken in a way that you
> could detect here if you didn't omit this check.

Actually I don't think we can check this anyway. Getting the currentTime from the timeline doesn't get a cached time based on a tick, it reevaluates the current time to high precision. I think asserting against its value would introduce a race.

(In reply to Daniel Holbert [:dholbert] from comment #24)
> > +// We do all these tests in a single async_test since that allows us to share
> > +// the timeout that we use to wait so that this test file isn't delayed by the
> > +// timeout time multiplied by number of tests.
> 
> I'm unclear on how this test file would be delayed by that amount. That
> would only happen if we truly don't fire the events, right? (which would
> already be bad & should be addressed immediately)  And per above, I'm not
> sure we should be using an explicit timeout time here anyway

You've misunderstood this test. It is checking that events are NOT fired for redundant changes that don't require them. Hopefully the comment and the code makes more sense read in that light. :)

> Can we kill this setTimeout(..., 1000) here and make this section
> event-driven? (letting the mochitest harness time out if some event is
> missing)

No, per the above. Let me know if this still isn't clear after rereading the comment.

Comment 26

[Jonathan Watt [:jwatt]]

Attachment: part 5 - CSS animation tests

Great review BTW, thanks for going into to such detail. Except where otherwise noted, I've agreed with and addressed your comments (I hope).

Comment 27

[Daniel Holbert [:dholbert]]

(In reply to Jonathan Watt [:jwatt] from comment #25)
> > Seems like it'd still be worth testing startTime here
> 
> Actually I don't think we can check this anyway. Getting the currentTime
> from the timeline doesn't get a cached time based on a tick, it reevaluates
> the current time to high precision. I think asserting against its value
> would introduce a race.

Ah, ok. So we can't do:
 foo.startTime = document.timeline.currentTime;
 [... stuff happens ...]
 [assert that startTime is still equal to document.timeline.currentTime ]

That's ok.  Still, though, it seems worth making sure that foo.startTime round-trips correctly, somewhere. (if you don't already have that.)

e.g. just assert that it's equal to a saved copy of the value that we assigned to it. (Pretty trivial, but worth checking, since it's a round-trip through our setter/getter C++ implementations)

> (In reply to Daniel Holbert [:dholbert] from comment #24)
> You've misunderstood this test. It is checking that events are NOT fired for
> redundant changes that don't require them. Hopefully the comment and the
> code makes more sense read in that light. :)

Ah! Thanks, this makes more sense then. We should add a comment directly alongside the setTimeout, making this explicitly clear (maybe referencing a longer comment elsewhere in the test file) -- otherwise the setTimeout(...,1000) stands out & looks suspicious as a randomorange-source.

(In reply to Jonathan Watt [:jwatt] from comment #26)
> Great review BTW, thanks for going into to such detail. Except where
> otherwise noted, I've agreed with and addressed your comments (I hope).

Thanks! I'll look over the updated test-patch shortly.

Comment 28

[Jonathan Watt [:jwatt]]

Attachment: part 6 - CSS animation tests

(In reply to Daniel Holbert [:dholbert] from comment #27)
> it seems worth making sure that foo.startTime
> round-trips correctly, somewhere.

I've added a test 'Check setting of startTime actually works'.

> Ah! Thanks, this makes more sense then. We should add a comment directly
> alongside the setTimeout

done.

Comment 29

[Daniel Holbert [:dholbert]]

Comment on attachment 8568341 [details] [diff] [review]
part 6 - CSS animation tests

Thanks! r=me

Comment 30

[Daniel Holbert [:dholbert]]

Sorry, one final nit I just thought of, RE that last "intermittent orange" comment -- we should probably replace "intermittent orange" with "intermittent failure" or "sporadic failure".  "Intermittent orange" may be meaningless to other browser-vendors who want to use this test down the line.

(I believe "orange" is mentioned twice here; we should fix both of them.)

Comment 31

[Jonathan Watt [:jwatt]]

Comment on attachment 8567762 [details] [diff] [review]
part 5 - add assert_between_* methods to imptests

According to jgraham the upstream patch isn't merged to this file automatically, so I need a separate review here.

Comment 32

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 8567762 [details] [diff] [review]
part 5 - add assert_between_* methods to imptests

Review of attachment 8567762 [details] [diff] [review]:
-----------------------------------------------------------------

Please create a PR at <https://github.com/w3c/testharness.js/> for this change.

Comment 33

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 8567762 [details] [diff] [review]
part 5 - add assert_between_* methods to imptests

Review of attachment 8567762 [details] [diff] [review]:
-----------------------------------------------------------------

rs+ once it merges upstream

Comment 34

[Jonathan Watt [:jwatt]]

The pull request is at https://github.com/w3c/testharness.js/pull/107

Comment 35

[Jonathan Watt [:jwatt]]

Comment on attachment 8556760 [details] [diff] [review]
part 1 - Add an AnimationUtils::DoubleToTimeDuration helper

https://hg.mozilla.org/integration/mozilla-inbound/rev/3e29c42f8f64

Comment 36

[Jonathan Watt [:jwatt]]

Comment on attachment 8556761 [details] [diff] [review]
part 2 - Make AnimationPlayer.startTime writeable

https://hg.mozilla.org/integration/mozilla-inbound/rev/8dfa0925ee89

Comment 37

[Jonathan Watt [:jwatt]]

Comment on attachment 8568020 [details] [diff] [review]
part 3 - call PostUpdate() as necessary for startTime changes to get computed style working

https://hg.mozilla.org/integration/mozilla-inbound/rev/4b81f0b5ed8b

Comment 38

[Jonathan Watt [:jwatt]]

Comment on attachment 8567761 [details] [diff] [review]
part 4 - dispatch CSS animations events as appropriate on backwards setting of startTime

https://hg.mozilla.org/integration/mozilla-inbound/rev/65b7039b6eb9

Comment 39

[Jonathan Watt [:jwatt]]

Comment on attachment 8567762 [details] [diff] [review]
part 5 - add assert_between_* methods to imptests

https://hg.mozilla.org/integration/mozilla-inbound/rev/157b8ab0a3d2

Comment 40

[Jonathan Watt [:jwatt]]

Attachment: part 7 - CSS transitions tests

Comment 41

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3e29c42f8f64
https://hg.mozilla.org/mozilla-central/rev/8dfa0925ee89
https://hg.mozilla.org/mozilla-central/rev/4b81f0b5ed8b
https://hg.mozilla.org/mozilla-central/rev/65b7039b6eb9
https://hg.mozilla.org/mozilla-central/rev/157b8ab0a3d2

Comment 42

[Jonathan Watt [:jwatt]]

Still have tests to land.

Comment 43

[Jonathan Watt [:jwatt]]

Comment on attachment 8568341 [details] [diff] [review]
part 6 - CSS animation tests

https://hg.mozilla.org/integration/mozilla-inbound/rev/c76d018d9e1c

The test for |player.playState == 'finished'| in checkStateAtActiveIntervalEndTime() when it is called right at the end of the 'Skipping backwards through animation' tests - i.e. immediately (synchronously) after the startTime is set to the end time - apparently fails very intermittently. This seems like a very minor detail (content seems very unlikely to care about that), and I'm not even sure if it's valid so for now I've disabled that assert_equals() call.

Comment 44

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c76d018d9e1c
https://hg.mozilla.org/mozilla-central/rev/8d4b6bed484b

Comment 45

[Daniel Holbert [:dholbert]]

Comment on attachment 8568790 [details] [diff] [review]
part 7 - CSS transitions tests

(If you like, it might make sense to generate this file using "hg cp" from the existing animation test, since they're 90% the same. Then your commit will just show the relevant differences.  Doesn't matter too much, though.)

>+++ b/dom/animation/test/css-transitions/test_animation-player-starttime.html
>+.animated-div {
>+  margin-left: 100px;
>+  transition: margin-left 1000s linear 1000s;
>+}
[...]
>+const ANIM_DELAY_MS = 1000000; // 1000s
>+const ANIM_DUR_MS = 1000000; // 1000s

Perhaps it'd be better to use a different delay vs. duration here, to catch bugs where we accidentally use one in place of the other.  (so e.g. 500s delay, 1000s duration maybe?) (This affects the already-landed animation test, too -- sorry for not thinking of this earlier when reviewing that test.)

>+  /**
>+   * This is useful when two events are expected to fire one immediately after
>+   * the other. This happens when we skip over the entire active interval for
>+   * instance. In this case an 'animationstart' and an 'animationend' are fired

This comment (for EventWatcher) & other comments here mention 'animationstart' and 'animationend' (and "CSS Animation Events"), but these events aren't used in this test at all.

Does the comment need pruning? Or perhaps just some clarification to indicate that EventWatcher is indended to support all sorts of animation events, though in this test we only use it for "transitionend".

>+   * and due to the asynchronous nature of Promise callbacks this won't work:
>+   *
>+   *   eventWatcher.waitForEvent('animationstart').then(function() {
>+   *     return waitForEvent('transitionend');
>+   *   }).then(...);

Here in the example, you've done s/animationend/transitionend/ -- but does this setup in the example actually make sense? (Do we ever actually fire 'animationstart' followed by 'transitionend', for a transition? I'm guessing not, given that we don't test for 'animationstart' anywhere in this test.)

Also, note that the surrounding documentation (outside of this sample-code) refers to 'animationend' instead of 'transitionend', too, e.g.:
>+   * It doesn't work because the 'animationend' listener is added too late,
The sample-code and the documentation that talks about it needs to be made consistent, one way or another.

>+  // Unlike in the case of CSS animations, we cannot skip to the end and skip
>+  // backwards since when we reach the end the transition effect is removed and
>+  // changes to the AnimationPlayer object no longer affect the element. For
>+  // this reason we only skip forwards as far as the 90% through point.

I think it'd still be worth testing this scenario (skip to the end and then skip backwards), to verify that things are as you say they are here. (Of course, the expectation will be different from the animation test's corresponding section, as you say here, since the animation effect will have been removed in this case.)

Comment 46

[Jonathan Watt [:jwatt]]

(In reply to Daniel Holbert [:dholbert] from comment #45)
> Comment on attachment 8568790 [details] [diff] [review]
> part 7 - CSS transitions tests
> 
> (If you like, it might make sense to generate this file using "hg cp" from
> the existing animation test, since they're 90% the same. Then your commit
> will just show the relevant differences.  Doesn't matter too much, though.)

Yeah, good idea.

> This comment (for EventWatcher)...

I'm in the process of getting the EventWatcher helper added to the W3C framework. I'll take your comments into account there, and just leave the EventWatcher implementation and comments untouched in this file (it will all be removed from these tests once available from the W3C framework).

Regarding your other comments I'll role them into whatever changes Brian wants too.

Comment 47

[Jonathan Watt [:jwatt]]

Attachment: part 7 - CSS transitions tests

Comment 48

[Jonathan Watt [:jwatt]]

Brian, for reference the css-animations test that has been copied and edited for the css-transitions test:

https://mxr.mozilla.org/mozilla-central/source/dom/animation/test/css-animations/test_animation-player-starttime.html?force=1

Comment 49

[Jonathan Watt [:jwatt]]

Attachment: part 7 - CSS transitions tests

Brian: and your feedback on the css-animations test would also be very welcome.

Comment 50

[Jonathan Watt [:jwatt]]

Attachment: part 7 - CSS transitions tests

Comment 51

[Brian Birtles (:birtles)]

Comment on attachment 8575709 [details] [diff] [review]
part 7 - CSS transitions tests

Review of attachment 8575709 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with comments addressed and a test like the following added:

1. Transition is half-way through the active interval
2. transition-duration is reduced to a quarter of its previous value (meaning the transition should now be finished) but style is not flushed
3. startTime is increased so that the currentTime now falls inside the active interval again

In that case I'd expect setting startTime to first flush the pending style changes so that the transitionend was fired and the transition was cancelled. Then I suppose setting startTime would retrigger the animation. Does that make sense?

::: dom/animation/test/css-transitions/test_animation-player-starttime.html
@@ +37,3 @@
>  const ANIM_DELAY_MS = 1000000; // 1000s
>  const ANIM_DUR_MS = 1000000; // 1000s
>  const ANIM_PROPERTY_VAL = 'anim ' + ANIM_DUR_MS + 'ms ' + ANIM_DELAY_MS + 'ms';

While we're at it, we might as well rename these to TRANSITION_DELAY_MS etc.?

@@ +354,5 @@
> +
> +test(function() {
> +  var div = addDiv();
> +
> +  var eventWatcher = new EventWatcher(div, TRANSITIONEND);

I'm really not sure what using a const variable here buys us over just writing 'transitionend'. I'd prefer we just wrote the event name directly (less cross-referencing and magic to decode) but it's not a big deal.

@@ +366,5 @@
> +  // backwards since when we reach the end the transition effect is removed and
> +  // changes to the AnimationPlayer object no longer affect the element. For
> +  // this reason we only skip forwards as far as the 90% through point.
> +
> +  player.startTime = document.timeline.currentTime - ANIM_DELAY_MS - ANIM_DUR_MS * 0.9; // 90% through active interval

I'm pretty sure this line (and others like it later) is over 80 chars.

@@ +375,5 @@
> +
> +  player.startTime = document.timeline.currentTime - ANIM_DELAY_MS; // jump to start of active interval
> +  checkStateAtActiveIntervalStartTime(player);
> +
> +  eventWatcher.stopWatching();

It doesn't seem like we actually use the event watcher? Perhaps we could past the end of the transition, then use requestAnimationFrame and check the event was received?

@@ +394,5 @@
>      return player.ready;
>    })).catch(t.step_func(function(reason) {
>      assert_unreached(reason);
>    })).then(function() {
>      t.done();

We should probably test here that we actually complete the following step:

> Set player’s hold time to previous current time even if previous current time is unresolved.

by querying player.currentTime and checking that it matches the currentTime immediately before setting the startTime to null (i.e. after the player is ready since before then the startTime will be null).

Comment 52

[Jonathan Watt [:jwatt]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/26ce6ccb4162

Comment 53

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/26ce6ccb4162