David Zbarsky (:dzbarsky) Reporter
	Description • 10 years ago

The spec seems to have a bug here - since the type of currentTime is double (rather than unrestricted double), infinity and null are already invalid values, and there is no need to specify that in the prose.

In our implementation, these values will be intercepted by the generated bindings and will throw, without calling AnimationPlayer::SetCurrentTime.

	Brian Birtles (:birtles, Feb 26~ parental leave)
	Comment 1 • 10 years ago

Comment on attachment 8494151 [details] [diff] [review]
Patch

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

As discussed on IRC:

1. This needs to adjust the dispatch of CSS animation events

Specifically, the model discussed was that if we go from being "inside" an animation to "outside" we dispatch animationend, likewise we dispatch animationstart for the opposite case, and if we skip from one side of the animation to the other we dispatch start followed by end.

Probably the same is true for transition events.

In terms of nsAnimationManager::GetEventsForCurrentTime "inside" refers to the active phase.

2. Now that we have current time, we can convert these tests to testharness tests I think?

3. We should test OMTA (probably as a mochitest).

Clearing review flag for now since I'd like to check the event changes at least.

::: dom/animation/AnimationPlayer.cpp
@@ +36,5 @@
>  
>  void
> +AnimationPlayer::SilentlySetCurrentTime(Nullable<double> aTime, ErrorResult& rv)
> +{
> +  TimeDuration offset = TimeDuration::FromMilliseconds(aTime.Value());

This needs to check if aTime is null first?

@@ +37,5 @@
>  void
> +AnimationPlayer::SilentlySetCurrentTime(Nullable<double> aTime, ErrorResult& rv)
> +{
> +  TimeDuration offset = TimeDuration::FromMilliseconds(aTime.Value());
> +  if (!mHoldTime.IsNull() || !mTimeline) {

This condition should also check if the current time of mTimeline is null.

  http://w3c.github.io/web-animations/#set-the-current-time

@@ +51,5 @@
> +  SilentlySetCurrentTime(aTime, rv);
> +  if (!rv.Failed()) {
> +    nsLayoutUtils::PostRestyleEvent(mSource->GetTarget(),
> +                                    eRestyle_Self,
> +                                    nsChangeHint_AllReflowHints);

I'm not sure exactly how this should work but I think we need a general "updated animation time, refresh stuff" method that we call here and also when we change the start time etc.

I'm not sure, however, if it should trigger events to be queued or not. For example, if you do:

  // player.currentTime = 100 and the animation is 1000ms long
  player.currentTime = 500;
  player.currentTime = 2000;
  player.currentTime = 500;

Do you get start/end events for when you finish the animation and re-enter it? Or is the whole lot coalesced?

I *think* I prefer the latter--coalescing--but I'm not sure.

In that case what we basically need to happen is:

1. Make sure AnimationPlayerCollection::mNeedsRefreshes gets set to true as needed
   (Maybe just always set it to true) for the animation player collection this animation player is part of.

   We can get to the collection by looking up mSource->GetTarget()

   e.g. for CSS animations we'd do something like static_cast<AnimationPlayerCollection*>(element->GetProperty(nsGkAtoms::animationsProperty)).

   If mSource or mSource->GetTarget() is null, we could just skip the update for now since we don't currently expect that to happen.

2. Call collection->EnsureStyleRuleFor(<latest refresh driver time>, EnsureStyleRule_IsNotThrottled) to update styles

3. Call collection->PostRestyleForAnimation(<pres context>)

4. Call nsAnimationManager::CheckNeedsRefresh to make sure it restarts observing the refresh driver as necessary.

5. Call presContext->Document()->SetNeedStyleFlush() ? (I think we'll need this to make sure events get queued eventually?)

And for transitions do something equivalent.

AnimationPlayerCollection objects have a pointer to their manager so this could all be bundled up in a method on the AnimationPlayerCollection.


If we don't coalesce events then it might be simpler. Maybe just a SetNeedStyleFlush?

	Brian Birtles (:birtles, Feb 26~ parental leave)
	Comment 2 • 10 years ago

(In reply to David Zbarsky (:dzbarsky) from comment #0)
> The spec seems to have a bug here - since the type of currentTime is double
> (rather than unrestricted double), infinity and null are already invalid
> values, and there is no need to specify that in the prose.

Should be fixed now: http://w3c.github.io/web-animations/#silently-set-the-current-time

	Brian Birtles (:birtles, Feb 26~ parental leave)
	Comment 3 • 10 years ago

(In reply to Brian Birtles (:birtles) from comment #1)
> I'm not sure, however, if it should trigger events to be queued or not. For
> example, if you do:
> 
>   // player.currentTime = 100 and the animation is 1000ms long
>   player.currentTime = 500;
>   player.currentTime = 2000;
>   player.currentTime = 500;
> 
> Do you get start/end events for when you finish the animation and re-enter
> it? Or is the whole lot coalesced?

Actually, I remembered the more compelling example:

   // player.currentTime = 1000 and the animation is 2000ms long

   // Case A:
   player.currentTime = 3000;
   player.source.timing.duration = 5000;

   // Case B:
   player.source.timing.duration = 5000;
   player.currentTime = 3000;

If we queue events immediately when adjusting the current time, in Case A we'll dispatch an additional start and end event.

I think it's generally preferable if authors don't have to worry about doing all these things in the right order, hence why I think it's better to queue events later.

	Brian Birtles (:birtles, Feb 26~ parental leave)
	Comment 4 • 10 years ago

This is a slightly simplified patch without tests just here for the purposes of bootstrapping devtools work until we fix this bug properly.

	Jonathan Watt [:jwatt] Assignee
	Comment 7 • 9 years ago

I should note that part 4 in bug 1073379 got the events firing as we decided they should when startTime/currentTime are used to skip backwards through an animation.

	Jonathan Watt [:jwatt] Assignee
	Comment 8 • 9 years ago

As for CSS transitions tests for currentTime changes, I'll wait until the CSS transitions test for startTime changes in bug 1073379 have been reviewed in case any substantial changes are required.

	Daniel Holbert [:dholbert]
	Comment 9 • 9 years ago

I won't be able to get to this until next week -- I'm behind on reviews, and my tomorrow is mostly eaten up by meetings (several interviews, an interview-debrief, and a 1:1), and I'm on PTO on Friday.

So -- it might be best to just have birtles review this, since he knows this code much better than I do, and since it's not getting reviewed until next week regardless.

	Brian Birtles (:birtles, Feb 26~ parental leave)
	Comment 10 • 9 years ago

Comment on attachment 8568882 [details] [diff] [review]
part 1 - allow setting of AnimationPlayer.currentTime

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

Looks good but I'm curious to know why we're cancelling a pending play when setting currentTime. It may be the right thing to do but it's not what the spec currently says.

::: dom/animation/AnimationPlayer.cpp
@@ +100,5 @@
> +AnimationPlayer::SilentlySetCurrentTime(const Nullable<TimeDuration>& aSeekTime,
> +                                        ErrorResult& aRv)
> +{
> +  if (aSeekTime.IsNull()) {
> +    aRv.Throw(NS_ERROR_DOM_NOT_SUPPORTED_ERR);

The spec is probably wrong here. I think this should be a TypeError as per:

https://www.w3.org/Bugs/Public/show_bug.cgi?id=27283#c0

If you agree, I'll update the spec.

@@ +139,5 @@
> +  if (mReady) {
> +    // We may have already resolved mReady, but in that case calling
> +    // MaybeResolve is a no-op, so that's okay.
> +    mReady->MaybeResolve(this);
> +  }

Why do we cancel a pending play here? I don't think that's the correct behavior. You should be able to set the current time to, e.g. 2000ms, while a pending play is in progress and it should continue to be pending until it starts playing from 2000ms.

::: dom/webidl/AnimationPlayer.webidl
@@ +21,5 @@
>    readonly attribute AnimationTimeline timeline;
>    [BinaryName="startTimeAsDouble"]
>    attribute double? startTime;
> +  [SetterThrows, BinaryName="currentTimeAsDouble"]
> +  attribute double? currentTime;

Obviously this needs review from a DOM peer.

	Brian Birtles (:birtles, Feb 26~ parental leave)
	Comment 11 • 9 years ago

Comment on attachment 8569233 [details] [diff] [review]
part 2 - Tests for the effect of setting CSS animation's AnimationPlayer.currentTime

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

Looks good but I want to check about the intermittent failure you reported and also refactoring the redundant changes tests.

::: dom/animation/test/css-animations/test_animation-player-currenttime.html
@@ +21,5 @@
> +    <script src="/resources/testharnessreport.js"></script>
> +  </head>
> +  <body>
> +    <div id="log"></div>
> +    <script type="text/javascript;version=1.8">

What is this needed for? let? If it's something that not likely to be implemented in the next version of IE, WebKit and Blink then we probably shouldn't use it.

@@ +45,5 @@
> +// Expected computed 'margin-left' values at points during the active interval:
> +// When we assert_between_inclusive using these values we could in theory cause
> +// intermittent failure 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

"... then we have issues ... "

@@ +46,5 @@
> +// When we assert_between_inclusive using these values we could in theory cause
> +// intermittent failure 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.

Can you rephrase this so it's less Mozilla-specific (e.g. replace references to the refresh driver with some generic talk about ticks)? These tests are intended to ultimately end up in web-platform-tests and be cross-browser.

@@ +54,5 @@
> +const FIFTY_PCT_POSITION = 150;
> +const NINETY_PCT_POSITION = 190;
> +const END_POSITION = 200;
> +
> +function addDiv(id)

Can we just use the version of addDiv in /dom/animation/test/testcommon.js? It does automatic cleanup so we can drop all the calls to removeChild below.
Obviously, feel free to wrap or extend it to add the extra class / ID setting behaviour.

@@ +67,5 @@
> +}
> +
> +/**
> + * CSS animation events fire asynchronously after we set 'startTime'. This
> + * helper class allows us to handle such events using Promises.

This looks really similar to the code in test_animation-player-starttime.html. Can we move the version in that file to testcommon.js and re-use it here?

@@ +285,5 @@
> +  var div = player.source.target;
> +  var marginLeft = parseFloat(getComputedStyle(div).marginLeft);
> +  assert_equals(marginLeft, UNANIMATED_POSITION,
> +    'the computed value of margin-left should be unaffected ' +
> +    'by the animation at the end of the active duration');

It might be worth mentioning that this is because the animation-fill-mode is none.

@@ +323,5 @@
> +  player.currentTime = 0;
> +  assert_approx_equals(player.currentTime, 0, 0.0001, // rounding error
> +    'Check setting of currentTime actually works');
> +
> +  checkStateOnSettingCurrentTimeToZero(player);

Since this method is only called once, I'd find this easier to follow if we just included those checks inline.

@@ +326,5 @@
> +
> +  checkStateOnSettingCurrentTimeToZero(player);
> +
> +  div.parentNode.removeChild(div);
> +}, 'Examine newly created Animation');

This does more than just examine a newly created Animation. It also sets its currentTime. I'm not quite sure what this test is actually supposed to be covering.

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

What do we actually expect to be different between setting the time to 50% and 90%? It seems like it should be the same code path. In fact, since we aren't testing for the absence of an event here, I think we could probably drop both these steps and just test setting the the end of the delay.

(If we are testing the absence of an event by, e.g., waiting for a couple of requestAnimationFrame ticks, then it might make sense to test a time in the middle of the animation interval.)

@@ +363,5 @@
> +    assert_true(false, reason);
> +  })).then(function() {
> +    t.done();
> +  });
> +}, 'Skipping forward through animation');

'Test skipping forward through animation'?

@@ +375,5 @@
> +
> +  var player = div.getAnimationPlayers()[0];
> +
> +  // Just so that player is running instead of paused when we set currentTime:
> +  player.startTime = player.timeline.currentTime;

Why not just wait on player.ready instead? I guess it doesn't really matter.

@@ +383,5 @@
> +  // Skipping over the active interval will dispatch an 'animationstart' then
> +  // an 'animationend' event. We need to wait for these events before we start
> +  // testing going backwards since EventWatcher will fail the test if it gets
> +  // an event that we haven't told it about.
> +  eventWatcher.waitForEvents(['animationstart', 'animationend']).then(t.step_func(function() {

This line (and others) is over 80 chars.

Is it worth checking that document.timeline.currentTime - previousTimelineTime is less than ANIMATION_DUR_MS to make sure we actually seeked the animation rather than just waiting for the events to fire due to regular playback? In our test harness hopefully we'd timeout before that happened but the same may not be true for other test harnesses.

@@ +423,5 @@
> +  // This must come after we've set up the Promise chain, since requesting
> +  // computed style will force events to be dispatched.
> +  // XXX For some reason this fails occasionally (either the player.playState
> +  // check or the marginLeft check).
> +  //checkStateAtActiveIntervalEndTime(player);

Did you work out what is going on here?

@@ +430,5 @@
> +
> +// Here we have multiple tests to check that redundant currentTime changes do
> +// NOT dispatch events. It's impossible to distinguish between events not being
> +// dispatched and events just taking an incredibly long time to dispatch
> +// without some sort of risk of creating intermittent failure. We have a short

Wouldn't it be enough to wait for a couple of requestAnimationFrame ticks and if no event was dispatched in that time, then assume that the event is not dispatched?

@@ +450,5 @@
> +// timeout time multiplied by number of tests.
> +async_test(function(t) {
> +  var divs = new Array(6);
> +  var eventWatchers = new Array(6);
> +  var players = new Array(6);

Is it possible to split these out into separate tests? web-platform-tests tends to favour very simple tests and it would be a lot easier to debug that way (and identify what the problem is when you have a specific test that is failing, rather than one called 'Redundant changes')? We could factor out a method that does all the common setup.

@@ +546,5 @@
> +      'AnimationPlayer.currentTime not null on ready Promise resolve');
> +
> +    var prePauseCurrentTime = player.currentTime;
> +
> +    player.pause();

We should add a comment here pointing to bug 1109390 saying that we should also wait on the ready promise and check the currentTime after that. (For what it's worth, I strongly disagree with the behaviour in the spec that says currentTime should be null while waiting to pause. I'm not sure we should implement that.)

	Jonathan Watt [:jwatt] Assignee
	Comment 12 • 9 years ago

(In reply to Brian Birtles (:birtles) from comment #10)
> Comment on attachment 8568882 [details] [diff] [review]
> part 1 - allow setting of AnimationPlayer.currentTime
>
> ::: dom/animation/AnimationPlayer.cpp
> @@ +100,5 @@
> > +AnimationPlayer::SilentlySetCurrentTime(const Nullable<TimeDuration>& aSeekTime,
> > +                                        ErrorResult& aRv)
> > +{
> > +  if (aSeekTime.IsNull()) {
> > +    aRv.Throw(NS_ERROR_DOM_NOT_SUPPORTED_ERR);
> 
> The spec is probably wrong here. I think this should be a TypeError as per:
> 
> https://www.w3.org/Bugs/Public/show_bug.cgi?id=27283#c0
> 
> If you agree, I'll update the spec.

I agree. I'll update the patch.

> @@ +139,5 @@
> > +  if (mReady) {
> > +    // We may have already resolved mReady, but in that case calling
> > +    // MaybeResolve is a no-op, so that's okay.
> > +    mReady->MaybeResolve(this);
> > +  }
> 
> Why do we cancel a pending play here? I don't think that's the correct
> behavior. You should be able to set the current time to, e.g. 2000ms, while
> a pending play is in progress and it should continue to be pending until it
> starts playing from 2000ms.

Because that's what AnimationPlayer::SetStartTime is doing currently. If the two should be different then we should at least document why SetStartTime is different.

	Jonathan Watt [:jwatt] Assignee
	Comment 13 • 9 years ago

Tagging smaug for minor DOM changes.

	Jonathan Watt [:jwatt] Assignee
	Comment 14 • 9 years ago

Comment on attachment 8575243 [details] [diff] [review]
part 1 - allow setting of AnimationPlayer.currentTime

I can remove the canceling of any pending play if that's what the correct thing to do is.

	Brian Birtles (:birtles, Feb 26~ parental leave)
	Comment 15 • 9 years ago

Comment on attachment 8575243 [details] [diff] [review]
part 1 - allow setting of AnimationPlayer.currentTime

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

r=me with those comments addressed

::: dom/animation/AnimationPlayer.cpp
@@ +105,5 @@
> +    return;
> +  }
> +
> +  MOZ_ASSERT(mTimeline && !mTimeline->GetCurrentTime().IsNull(),
> +             "We don't support inactive/missing timelines yet");

We don't need this assertion. We test for these two conditions below.

@@ +139,5 @@
> +  if (mReady) {
> +    // We may have already resolved mReady, but in that case calling
> +    // MaybeResolve is a no-op, so that's okay.
> +    mReady->MaybeResolve(this);
> +  }

In response to comment 12 and comment 14, we shouldn't cancel a pending play here. The reason is that a pending play resolves the start time using the hold time once the animation is ready to run. It's fine to adjust the hold time while we're still waiting for the animation to start.

::: dom/animation/AnimationPlayer.h
@@ +76,5 @@
>    Nullable<TimeDuration> GetCurrentTime() const;
> +  void SilentlySetCurrentTime(const Nullable<TimeDuration>& aNewCurrentTime,
> +                              ErrorResult& aRv);
> +  void SetCurrentTime(const Nullable<TimeDuration>& aNewCurrentTime,
> +                      ErrorResult& aRv);

As far as I can tell, the only time SetCurrentTime throws is when it is passed a null seek time?

Firstly, the spec is probably wrong here. Sure, it doesn't make sense to set the current time to null in most cases, but if it is *already* null, then it probably shouldn't throw an exception when you simply set it to its current value.

If it's non-null and you set it to null we could either an exception (an alternative would be to effectively call Cancel() but that's probably a bit surprising).

What do you think?

In any case, I think handling of null values should be done entirely in SetCurrentTimeAsDouble. Then we can change these two methods to take a const TimeDuration& and drop the ErrorResult& param.

	Jonathan Watt [:jwatt] Assignee
	Comment 16 • 9 years ago

(In reply to Brian Birtles (:birtles) from comment #15)
> As far as I can tell, the only time SetCurrentTime throws is when it is
> passed a null seek time?

Yes.

> Firstly, the spec is probably wrong here. Sure, it doesn't make sense to set
> the current time to null in most cases, but if it is *already* null, then it
> probably shouldn't throw an exception when you simply set it to its current
> value.

I agree with that. It should just be a no-op.

> If it's non-null and you set it to null we could either an exception (an
> alternative would be to effectively call Cancel() but that's probably a bit
> surprising).
> 
> What do you think?

In this case I still think it would be better to throw an exception. Calling Cancel() does seem to unexpected to me.

> In any case, I think handling of null values should be done entirely in
> SetCurrentTimeAsDouble. Then we can change these two methods to take a const
> TimeDuration& and drop the ErrorResult& param.

Sure, I'll do that.

	Brian Birtles (:birtles, Feb 26~ parental leave)
	Comment 18 • 9 years ago

Comment on attachment 8575721 [details] [diff] [review]
part 1 - allow setting of AnimationPlayer.currentTime

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

r=me with comment addressed

::: dom/animation/AnimationPlayer.cpp
@@ +128,5 @@
> +  if (mReady) {
> +    // We may have already resolved mReady, but in that case calling
> +    // MaybeResolve is a no-op, so that's okay.
> +    mReady->MaybeResolve(this);
> +  }

As discussed on IRC, we should drop this bit too.

	Brian Birtles (:birtles, Feb 26~ parental leave)
	Comment 19 • 9 years ago

(In reply to Jonathan Watt [:jwatt] from comment #12)
> (In reply to Brian Birtles (:birtles) from comment #10)
> > Comment on attachment 8568882 [details] [diff] [review]
...
> > > +AnimationPlayer::SilentlySetCurrentTime(const Nullable<TimeDuration>& aSeekTime,
> > > +                                        ErrorResult& aRv)
> > > +{
> > > +  if (aSeekTime.IsNull()) {
> > > +    aRv.Throw(NS_ERROR_DOM_NOT_SUPPORTED_ERR);
> > 
> > The spec is probably wrong here. I think this should be a TypeError as per:
> > 
> > https://www.w3.org/Bugs/Public/show_bug.cgi?id=27283#c0
> > 
> > If you agree, I'll update the spec.
> 
> I agree. I'll update the patch.

For the above change, and...

(In reply to Jonathan Watt [:jwatt] from comment #16)
> (In reply to Brian Birtles (:birtles) from comment #15)
> > Firstly, the spec is probably wrong here. Sure, it doesn't make sense to set
> > the current time to null in most cases, but if it is *already* null, then it
> > probably shouldn't throw an exception when you simply set it to its current
> > value.
> 
> I agree with that. It should just be a no-op.

... this change too, I've updated the spec accordingly:

https://github.com/w3c/web-animations/compare/6b85b490a41b...b37d5cd42ae4

	Jonathan Watt [:jwatt] Assignee
	Comment 20 • 9 years ago

Cool, sounds good.

(In reply to Brian Birtles (:birtles) from comment #11)
> This looks really similar to the code in
> test_animation-player-starttime.html. Can we move the version in that file
> to testcommon.js and re-use it here?

Actually, I'm in the process of moving it to be part of the W3C Testharness API. I'll pull down the changes as soon as I can and get rid of this code then.

> @@ +423,5 @@
> > +  // This must come after we've set up the Promise chain, since requesting
> > +  // computed style will force events to be dispatched.
> > +  // XXX For some reason this fails occasionally (either the player.playState
> > +  // check or the marginLeft check).
> > +  //checkStateAtActiveIntervalEndTime(player);
> 
> Did you work out what is going on here?

No. This is an existing problem from the -starttime.html test though that's just been copied over here, and there's bug 1142065 for figuring out what's going on.

I believe I've addressed the other comments in bug 1141710, and the |hg cp| of the test to this bugs test transports these changes over.

	Jonathan Watt [:jwatt] Assignee
	Comment 21 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/2aff6dae3c68
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e4d37bd4e41

	Jonathan Watt [:jwatt] Assignee
	Comment 22 • 9 years ago

Oh, and I'll need another test file to test transitions.

	Carsten Book [:Tomcat]
	Comment 23 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/2aff6dae3c68
https://hg.mozilla.org/mozilla-central/rev/7e4d37bd4e41

	Brian Birtles (:birtles, Feb 26~ parental leave)
	Comment 25 • 9 years ago

Comment on attachment 8586488 [details] [diff] [review]
part 3 - Tests for the effect of setting CSS transition's AnimationPlayer.currentTime

>+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';

I don't think we use ANIM_PROPERTY_VAL anywhere.

>+/**
>+ * These helpers get the value that the currentTime needs to be set to, to put
>+ * an animation that uses the above ANIM_DELAY_MS and ANIM_DUR_MS values into
>+ * the middle of various phases or points through the active duration.
>+ */
>+function currentTimeForBeforePhase(timeline) {
>+  return ANIM_DELAY_MS / 2;
>+}
...

Why are all these functions taking a timeline parameter? It looks like they could just be variable assignments.

Also, I think it would be more clear to just put these calculations in the tests where they are needed rather than having to skip back and forth between definitions at the top and test, but that's just my preference.

>+// Expected computed 'margin-left' values at points during the active interval:
>+// When we assert_between_inclusive using these values we could in theory cause
>+// intermittent failure due to very long delays between paints, but since the
>+// active duration is 1000s long, a delay would need to be around 100s to cause
>+// that. If that's happening then there are likely other issues that should be
>+// fixed, so a failure to make us look into that seems like a good thing.

We could also stick a step() timing function on the transition to force it to increment in discrete amounts?

>+/**
>+ * CSS animation events fire asynchronously after we set 'startTime'. This

Is this supposed to refer to currentTime as well or instead of startTime?

I presume the rest of this class is the same as in other files and just reproduced here until it lands in web-platform-tests? And, if so, doesn't it still make sense to split it into a separate file for now?

>+// The terms used for the naming of the following helper functions refer to
>+// terms used in the Web Animations specification for specific phases of an
>+// animation. The terms can be found here:
>+//
>+//   http://w3c.github.io/web-animations/#animation-node-phases-and-states

Should be http://w3c.github.io/web-animations/#animation-effect-phases-and-states

>+// Note the distinction between "player start time" and "animation start time".
>+// The former is the start of the start delay. The latter is the start of the
>+// active interval. (If there is no delay, they are the same.)

I think this should be "animation start time" and "effect start time"?　But is this just boilerplate copied from the start time tests that doesn't need to be here?

>+// Called when currentTime is set to zero (the beginning of the start delay).
>+function checkStateOnSettingCurrentTimeToZero(animation)
>+{
>+  // We don't test animation.currentTime since our caller just set it.
>+
>+  assert_equals(animation.playState, 'running',
>+    'Animation.playState should be "running" at the start of ' +
>+    'the start delay');
>+
>+  assert_equals(animation.source.target.style.animationPlayState, 'running',
>+    'Animation.source.target.style.animationPlayState should be ' +
>+    '"running" at the start of the start delay');
>+
>+  var div = animation.source.target;
>+  var marginLeft = parseFloat(getComputedStyle(div).marginLeft);
>+  assert_equals(marginLeft, UNANIMATED_POSITION,
>+                'the computed value of margin-left should be unaffected ' +
>+                'at the beginning of the start delay');
>+}

Do we really need this? It seems like we should focus on testing just the currentTime (although checking the playState might make sense sometimes).

Likewise for checkStateOnReadyPromiseResolved, checkStateAtActiveIntervalStartTime, and checkStateAtFiftyPctOfActiveInterval?  Perhaps we can just check the property of marginLeft where we need to? It's a bit hard to read the tests at the moment since it's not clear what they're testing without referring to these functions. It's up to you, but I'd personally prefer moving the necessary checks inline.

>+test(function(t)
>+{
>+  var div = addDiv(t, {'class': 'animated-div'});
>+  flushComputedStyle(div);
>+  div.style.marginLeft = '200px'; // initiate transition
>+
>+  var animation = div.getAnimations()[0];
>+  assert_equals(animation.currentTime, 0, 'currentTime is unresolved');

The comment doesn't match the test here (presumably because we copied it from startTime tests?).

>+async_test(function(t) {
>+  var div = addDiv(t, {'class': 'animated-div'});
>+  var eventWatcher = new EventWatcher(div, 'transitionend');
>+
>+  flushComputedStyle(div);
>+  div.style.marginLeft = '200px'; // initiate transition
>+
>+  var animation = div.getAnimations()[0];
>+
>+  // 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 Animation object no longer affect the element. For
>+  // this reason we only skip forwards as far as the 90% through point.
>+
>+  animation.ready.then(t.step_func(function() {
>+    animation.currentTime =
>+      currentTimeForFiftyPercentThroughActiveInterval(animation.timeline);

According to the comment we skip 90% of the way through but the code seems to skip 50% of the way through.

>+    eventWatcher.waitForEvent('transitionend').then(function() {
>+      eventWatcher.stopWatching();
>+      t.done();
>+      alert(1)

Is this last line supposed to be here?

>+async_test(function(t) {
>+  var div = addDiv(t, {'class': 'animated-div'});
>+  flushComputedStyle(div);
>+  div.style.marginLeft = '200px'; // initiate transition
>+
>+  var animation = div.getAnimations()[0];
>+  var pauseTime;
>+
>+  animation.ready.then(t.step_func(function() {
>+    assert_not_equals(animation.currentTime, null,
>+      'Animation.currentTime not null on ready Promise resolve');
>+    animation.pause();
>+    return animation.ready;
>+  })).then(t.step_func(function() {
>+    pauseTime = animation.currentTime;
>+    return waitForFrame();
>+  })).then(t.step_func(function() {
>+    assert_equals(animation.currentTime, pauseTime,
>+      'Animation.currentTime is unchanged after pausing');
>+  })).catch(t.step_func(function(reason) {
>+    assert_unreached(reason);
>+  })).then(function() {
>+    t.done();
>+  });
>+}, 'Animation.currentTime after pausing');

I wonder if we need this test? I don't think there's any transition-specific behaviour involved?

The rest looks good. Thanks Jonathan!

	Jonathan Watt [:jwatt] Assignee
	Comment 26 • 9 years ago

(In reply to Brian Birtles (:birtles) from comment #25)
> Comment on attachment 8586488 [details] [diff] [review]
> 
> Also, I think it would be more clear to just put these calculations in the
> tests where they are needed rather than having to skip back and forth
> between definitions at the top and test, but that's just my preference.

You'd then need to think "what do these calculations mean", rather than it just saying it on the tin.

> >+/**
> >+ * CSS animation events fire asynchronously after we set 'startTime'. This
> 
> Is this supposed to refer to currentTime as well or instead of startTime?
> 
> I presume the rest of this class is the same as in other files and just
> reproduced here until it lands in web-platform-tests? And, if so, doesn't it
> still make sense to split it into a separate file for now?

I'm literally hoping to land that stuff upstream and downstream today, subject to the critic reviews being all that I'm getting.

> I think this should be "animation start time" and "effect start time"?　But
> is this just boilerplate copied from the start time tests that doesn't need
> to be here?

Yes, and I think this can be removed from all the other test files now (since the function names changed, and there is no longer the potential for confusion).

> Do we really need this? It seems like we should focus on testing just the
> currentTime (although checking the playState might make sense sometimes).

I think it's probably good to test that changes to currentTime result in the expected changes to playState. Maybe it's overkill, but if we decide to remove it let's be consistent and remove it from all the test files that do this.

> Is this last line supposed to be here?

Nope, thanks.

> I wonder if we need this test? I don't think there's any transition-specific
> behaviour involved?

If we decide to remove it, let's do that as part of our test cleanup, and do it across the other similar test files.

	Ryan VanderMeulen [:RyanVM]
	Comment 27 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/48f0c039ba38

	Jean-Yves Perrier [:teoli]
	Comment 28 • 9 years ago

Updated
https://developer.mozilla.org/en-US/docs/Web/API/AnimationPlayer/currentTime
and
https://developer.mozilla.org/en-US/Firefox/Releases/39#Interfaces.2FAPIs.2FDOM