This includes the following parts: * Making AnimationPlayer.currentTime writeable and supporting seeking * Implementing pause control (play(), paused(), paused) and mapping to animation-play-state * Implementing speed control (playbackRate, reverse()) * finish() * cancel() * finished - the spec is likely to change here to use a Promise Each of these should probably be a separate bug although some of the later ones could be tackled together. This probably needs bug 1032573 so we have player objects to work with. Implementation of time-limiting behavior (see "3.5.2.3 Limiting the current time" in the spec) is also a prerequisite but it could be tackled as part of making currentTime writeable. The spec has algorithms for all of this but they quite likely contain bugs. The behavior of startTime may change slightly. So we need to fix the spec as we implement this.

To match spec terminology.

startTime and currentTime should be nullable.

Comment on attachment 8474356 [details] [diff] [review] Part 2: Update AnimationPlayer webidl Review of attachment 8474356 [details] [diff] [review]: ----------------------------------------------------------------- Do you need review from a DOM peer for the WebIDL changes? ::: dom/webidl/AnimationPlayer.webidl @@ +12,5 @@ > > [Pref="dom.animations-api.core.enabled"] > interface AnimationPlayer { > + // Bug 1049975: Per spec, this should be a writeable AnimationNode? member > + // attribute AnimationNode? source; I'm not sure that this second line is needed. @@ +30,5 @@ > + void finish (); > + void play (); > + void pause (); > + void reverse (); > + */ Did you deliberately change this to 4-space indent? Is that necessary?

Hi David, looks good. I was just thinking this overlaps with what I started doing in the first two patches for bug 927349. The main difference is that those patches simultaneously rename mPauseStart to mHoldTime as making mHoldTime nullable (i.e. changing name and meaning simultaneously). Would you mind adding a part 3 that makes mHoldTime nullable and applies the additional changes from attachment 8465162 [details] [diff] [review] and attachment 8465163 [details] [diff] [review]?

(In reply to Brian Birtles (:birtles) from comment #3) > Comment on attachment 8474356 [details] [diff] [review] > Part 2: Update AnimationPlayer webidl > > Review of attachment 8474356 [details] [diff] [review]: > ----------------------------------------------------------------- > > Do you need review from a DOM peer for the WebIDL changes? > Probably not necessary since it's not actually changing the WebIDL, but I can ask. > ::: dom/webidl/AnimationPlayer.webidl > @@ +12,5 @@ > > > > [Pref="dom.animations-api.core.enabled"] > > interface AnimationPlayer { > > + // Bug 1049975: Per spec, this should be a writeable AnimationNode? member > > + // attribute AnimationNode? source; > > I'm not sure that this second line is needed. > When we deviate from the spec, we usually add the actual WebIDL and comment it out. I can remove the comment (except for the bug number) so it's not redundant. > @@ +30,5 @@ > > + void finish (); > > + void play (); > > + void pause (); > > + void reverse (); > > + */ > > Did you deliberately change this to 4-space indent? Is that necessary? I did that since that's what the spec did. Seems like other WebIDL uses 2 spaces, I'll change it back.

Comment on attachment 8474356 [details] [diff] [review] Part 2: Update AnimationPlayer webidl Review of attachment 8474356 [details] [diff] [review]: ----------------------------------------------------------------- Boris can you review the WebIDL changes?

This still isn't quite right - when pausing and unpausing multiple times, it seems like our start time keeps drifting. Perhaps we should be rolling it back by the previously held time or something?

Comment on attachment 8474356 [details] [diff] [review] Part 2: Update AnimationPlayer webidl r=me

Comment on attachment 8475985 [details] [diff] [review] Part 3: Make startTime an offset Review of attachment 8475985 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for this David. It looks good but see my suggestions below about another way of approaching the handling of the *timeline* start time. Not granting review just yet since I'd like to hear what you think about that approach and look it over if you agree with those changes. ::: dom/animation/AnimationPlayer.h @@ +76,5 @@ > " mHoldTime"); > > Nullable<TimeDuration> result; // Initializes to null > if (!timelineTime.IsNull() && !mStartTime.IsNull()) { > + result.SetValue((IsPaused() ? mHoldTime : timelineTime) - (mTimeline->GetStartTime() + mStartTime.Value())); This line is > 80 chars. It's also not immediately obvious what it's doing. More importantly, though, I don't think we should add GetStartTime() to the timeline interface. See comments below. ::: dom/animation/AnimationTimeline.h @@ +24,5 @@ > explicit AnimationTimeline(nsIDocument* aDocument) > : mDocument(aDocument) > { > SetIsDOMBinding(); > + mStartTime = TimeStamp::Now(); This isn't right. The start time of the timeline is navigationStart. I think what we should do is, instead of converting the mStartTime in AnimationPlayer::GetCurrentTimeDuration to a TimeStamp so we can do arithmetic with mHoldTime and timelineTime is to convert mHoldTime and timelineTime to offsets. e.g. 1. Add AnimationTimeline::GetCurrentTimeDuration * Make AnimationTimeline::ToTimelineTime return a Nullable<TimeDuration> Replace: result.SetValue(timing->TimeStampToDOMHighRes(aTimeStamp)); with: result.SetValue(aTimeStamp - timing->GetNavigationStartTimeStamp()); (which is all that TimeStampToDOMHighRes is doing anyway but without the final 'ToMilliseconds' call) * Make AnimationTimeline::GetCurrentTime do the necessary conversion from nullable<TimeDuration> to Nullable<double> much like you already have for AnimationPlayer::GetStartTime() * Add AnimationTimeline::GetCurrentTimeDuration that just does: return ToTimelineTime(GetCurrentTimeStamp()) 2. In AnimationPlayer::GetCurrentTimeDuration let timelineTime be the result of calling GetCurrentTimeDuration instead. 3. Convert mHoldTime to a time duration by calling mTimeline->ToTimelineTime(mHoldTime). Call it holdTimeDuration or something for now. It doesn't matter too much since we'll remove it in part 4. Then, in AnimationPlayer::GetCurrentTimeDuration, we should be able to do something like: result.SetValue((IsPaused() ? holdTimeDuration : timelineTime.Value()) - mStartTime.Value()); @@ +45,5 @@ > virtual ~AnimationTimeline() { } > > nsCOMPtr<nsIDocument> mDocument; > > + mozilla::TimeStamp mStartTime; Both this and mStartTime should no longer be needed. ::: layout/base/nsDisplayList.cpp @@ +349,5 @@ > aLayer->AddAnimationForNextTransaction() : > aLayer->AddAnimation(); > > const AnimationTiming& timing = aPlayer->GetSource()->Timing(); > + animation->startTime() = aPlayer->Timeline()->GetStartTime() + aPlayer->mStartTime.Value() + timing.mDelay; For this we probably need to add: TimeStamp AnimationTimeline::ToTimeStamp(const TimeDuration& aTimelineTime) and use that here: animation->startTime() = aPlayer->Timeline()->ToTimeStamp(aPlayer->mStartTime.Value()); It would just do the inverse of ToTimelineTime. If we couldn't get a timing object we'd set the TimeStamp to null so I guess we'd have to check for that here or further down the line. I'm not sure in what circumstances that object is not available, however, so I'm not sure if a MOZ_ASSERT would be sufficient. ::: layout/style/nsAnimationManager.cpp @@ +317,5 @@ > // offsets (not timestamps) we should be able to update the > // start time to something more appropriate when now IsNull. > // Handle change in pause state by adjusting start time to > // unpause. > + oldPlayer->mStartTime.SetValue(now + oldPlayer->mStartTime.Value() - oldPlayer->mHoldTime); This looks like its > 80 chars. @@ +455,5 @@ > nsRefPtr<Animation> destAnim = > new Animation(mPresContext->Document(), timing); > dest->SetSource(destAnim); > > + dest->mStartTime.SetValue(now - aTimeline->GetStartTime()); dest->mStartTime = aTimeline->GetCurrentTimeDuration(); At some point it might even be easier to just make |now| store the result of GetCurrentTimeDuration(). Perhaps after part 4. ::: layout/style/nsTransitionManager.cpp @@ +527,5 @@ > segment.mToKey = 1; > segment.mTimingFunction.Init(tf); > > nsRefPtr<dom::AnimationPlayer> player = new dom::AnimationPlayer(timeline); > + player->mStartTime.SetValue(timeline->GetCurrentTimeStamp() - timeline->GetStartTime()); player->mStartTime = timeline->GetCurrentTimeDuration();

Comment on attachment 8476148 [details] [diff] [review] Part 4: Make mHoldTime a Nullable TimeDuraiton Review of attachment 8476148 [details] [diff] [review]: ----------------------------------------------------------------- r=birtles with comments addressed assuming you agree with the changes I proposed for part 3. ::: dom/animation/AnimationPlayer.cpp @@ +42,1 @@ > result.SetValue(currentTime.Value().ToMilliseconds()); This pattern of converting a Nullable<TimeDuration> to a Nullable<double> comes up in a few places, e.g. AnimationPlayer::GetStartTime from part 3 and the changes to AnimationTimeline::GetCurrentTime I proposed in comment 12. We should probably factor out a utility method for this somewhere (AnimationUtils.h?). That can happen as a follow-up though. Alternatively, we might even consider wrapping up Nullable<TimeDuration> at some point. @@ +91,5 @@ > + } else { > + const TimeStamp& timelineTime = mTimeline->GetCurrentTimeStamp(); > + if (!timelineTime.IsNull() && !mStartTime.IsNull()) { > + result.SetValue(timelineTime - mTimeline->GetStartTime() - mStartTime.Value()); > + } Based on the changes proposed to part 3 in comment 12, I think this would become: const Nullable<TimeDuration>& timelineTime = mTimeline->GetCurrentTimeDuration(); if (!timelineTime.IsNull() && !mStartTime.IsNull()) { result.SetValue(timelineTime.Value() - mStartTime.Value()); } ::: layout/style/nsAnimationManager.cpp @@ +313,2 @@ > } else if (oldPlayer->IsPaused() && !newPlayer->IsPaused()) { > + oldPlayer->mStartTime.SetValue(timeline->GetCurrentTimeStamp() - timeline->GetStartTime() - oldPlayer->mHoldTime.Value()); oldPlayer->mStartTime.SetValue(timeline->GetCurrentTimeDuration() - oldPlayer->mHoldTime.Value()); @@ +313,5 @@ > } else if (oldPlayer->IsPaused() && !newPlayer->IsPaused()) { > + oldPlayer->mStartTime.SetValue(timeline->GetCurrentTimeStamp() - timeline->GetStartTime() - oldPlayer->mHoldTime.Value()); > + float start = oldPlayer->mStartTime.Value().ToMilliseconds(); > + float current = oldPlayer->GetCurrentTime().Value(); > + float elapsed = (timeline->GetCurrentTimeStamp() - timeline->GetStartTime()).ToMilliseconds(); As discussed on IRC, these three lines need to be removed @@ +453,5 @@ > > dest->mStartTime.SetValue(now - aTimeline->GetStartTime()); > dest->mPlayState = src.GetPlayState(); > if (dest->IsPaused()) { > + dest->mHoldTime.SetValue(now - aTimeline->GetStartTime()); dest->mHoldTime = aTimeline->GetCurrentTimeDuration(); (Or, it might be easier by this stage to simply set |now| to aTimeline->GetCurrentTimeDuration() if we're not using GetCurrentTimeStamp() anymore.)

Comment on attachment 8476169 [details] [diff] [review] Part 5: Expose and use the timeline's time value Review of attachment 8476169 [details] [diff] [review]: ----------------------------------------------------------------- And now I come to part 5 and see you've already done what I was suggesting :) I wonder if we still need this? I'd slightly prefer we made this change earlier on (i.e. part 3 or before) since it makes it easier to understand the subsequent changes I think. Not granting review for this just yet since I don't think we need it (i.e. we can incorporate it into part 3 or before) but I'd like to hear what you think. ::: dom/animation/AnimationTimeline.h @@ +38,2 @@ > > + TimeDuration GetCurrentTimeDuration() { return GetCurrentTimeStamp() - mStartTime; } As mentioned in part 3, the start time we're using here is wrong. Also, the above doesn't check for a null value for GetCurrentTimeStamp(). It should return a Nullable<TimeDuration>. But, again, I'd prefer we just took the approach outlined in comment 12. ::: layout/style/nsAnimationManager.cpp @@ +451,3 @@ > dest->mPlayState = src.GetPlayState(); > if (dest->IsPaused()) { > + dest->mHoldTime.SetValue(aTimeline->GetCurrentTimeDuration()); Rather than removing the |now| line earlier in this method, we should just set it to the result of aTimeline->GetCurrentTimeDuration() and then use it here.

Comment on attachment 8475985 [details] [diff] [review] Part 3: Make startTime an offset Review of attachment 8475985 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/animation/AnimationTimeline.h @@ +24,5 @@ > explicit AnimationTimeline(nsIDocument* aDocument) > : mDocument(aDocument) > { > SetIsDOMBinding(); > + mStartTime = TimeStamp::Now(); Ah, good point about the navigationStart. I didn't know about that. The rest of these comments are basically part 5, so I can just fold that into this part and part 4. @@ +45,5 @@ > virtual ~AnimationTimeline() { } > > nsCOMPtr<nsIDocument> mDocument; > > + mozilla::TimeStamp mStartTime; You mean GetStartTime(), right? ::: layout/base/nsDisplayList.cpp @@ +349,5 @@ > aLayer->AddAnimationForNextTransaction() : > aLayer->AddAnimation(); > > const AnimationTiming& timing = aPlayer->GetSource()->Timing(); > + animation->startTime() = aPlayer->Timeline()->GetStartTime() + aPlayer->mStartTime.Value() + timing.mDelay; Looks like we won't have a timing if the first page we load is a wyciwyg or javascript URI...but we may as well be safe and check for null and bail out here.

Comment on attachment 8476148 [details] [diff] [review] Part 4: Make mHoldTime a Nullable TimeDuraiton Review of attachment 8476148 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/animation/AnimationPlayer.cpp @@ +42,1 @@ > result.SetValue(currentTime.Value().ToMilliseconds()); I think I'll make this in Part 3 so we can avoid the duplication there. @@ +91,5 @@ > + } else { > + const TimeStamp& timelineTime = mTimeline->GetCurrentTimeStamp(); > + if (!timelineTime.IsNull() && !mStartTime.IsNull()) { > + result.SetValue(timelineTime - mTimeline->GetStartTime() - mStartTime.Value()); > + } Yup ::: layout/style/nsAnimationManager.cpp @@ +313,2 @@ > } else if (oldPlayer->IsPaused() && !newPlayer->IsPaused()) { > + oldPlayer->mStartTime.SetValue(timeline->GetCurrentTimeStamp() - timeline->GetStartTime() - oldPlayer->mHoldTime.Value()); Yup

Comment on attachment 8477095 [details] [diff] [review] Part 3: Make mStartTime a Nullable TimeDuration The lines that are over 80 chars are fixed in part 4.

Brian, I'm putting these up in case you want to look at them. Not asking for review yet because I haven't had a chance to write tests.

Comment on attachment 8477095 [details] [diff] [review] Part 3: Make mStartTime a Nullable TimeDuration Review of attachment 8477095 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/animation/AnimationTimeline.cpp @@ +93,5 @@ > + > +TimeStamp > +AnimationTimeline::ToTimeStamp(const TimeDuration& aTimeDuration) const > +{ > + TimeStamp result = TimeStamp(); I think TimeStamp result; is fine and more consistent with what we do elsewhere. ::: dom/animation/AnimationTimeline.h @@ +39,5 @@ > + > + Nullable<TimeDuration> ToTimelineTime(const TimeStamp& aTimeStamp) const; > + TimeStamp ToTimeStamp(const TimeDuration& aTimelineTime) const; > + > + TimeStamp GetCurrentTimeStamp() const; I think we should make both .h file and .cpp file have the same order for declarations and definitions. Probably something like: // AnimationTimeline interface Nullable<double> GetCurrentTime Nullable<TimeDuration> GetCurrentTimeDuration() const; TimeStamp GetCurrentTimeStamp() const; Nullable<TimeDuration> ToTimelineTime(const TimeStamp& aTimeStamp) const; TimeStamp ToTimeStamp(const TimeDuration& aTimelineTime) const; ::: dom/animation/AnimationUtils.h @@ +11,5 @@ > + > +class AnimationUtils > +{ > +public: > + static Nullable<double> TimeDurationToDouble(Nullable<TimeDuration> aTime) Make aTime a const ref. @@ +21,5 @@ > + } > + > + return result; > + } > + Remove this extra line

(In reply to Brian Birtles (:birtles, travelling 20~30 Aug, may be slower to respond) from comment #22) > Comment on attachment 8477095 [details] [diff] [review] > Part 3: Make mStartTime a Nullable TimeDuration > > > I think we should make both .h file and .cpp file have the same order for > declarations and definitions. Probably something like: > > // AnimationTimeline interface > Nullable<double> GetCurrentTime > > Nullable<TimeDuration> GetCurrentTimeDuration() const; > TimeStamp GetCurrentTimeStamp() const; > > Nullable<TimeDuration> ToTimelineTime(const TimeStamp& aTimeStamp) const; > TimeStamp ToTimeStamp(const TimeDuration& aTimelineTime) const; > I actually make GetCurrentTimeStamp protected in the next part since nobody should be using that API.

Comment on attachment 8476148 [details] [diff] [review] Part 4: Make mHoldTime a Nullable TimeDuraiton Boris, can you rubberstamp the webidl changes?

Comment on attachment 8476148 [details] [diff] [review] Part 4: Make mHoldTime a Nullable TimeDuraiton Review of attachment 8476148 [details] [diff] [review]: ----------------------------------------------------------------- Nevermind, only the first patch needed review. The commit hook just couldn't parse my message. remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/f1d621aa125e remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/4e2595393b7c remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/f9448cb58a5a remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/9db3e43c19c1

https://hg.mozilla.org/mozilla-central/rev/f1d621aa125e https://hg.mozilla.org/mozilla-central/rev/4e2595393b7c https://hg.mozilla.org/mozilla-central/rev/f9448cb58a5a https://hg.mozilla.org/mozilla-central/rev/9db3e43c19c1

I think this should have been marked leave-open since there are still more patches to land here?

Yes, there is more to do here.

Comment on attachment 8489782 [details] [diff] [review] Part 8: Add AnimationPlayer.play()/pause() Review of attachment 8489782 [details] [diff] [review]: ----------------------------------------------------------------- Not giving r+ yet because I'm not sure if this approach works after all. ::: dom/animation/AnimationPlayer.cpp @@ +69,5 @@ > + UpdatePauseState(); > + } > + > + Nullable<TimeDuration> now = mTimeline->GetCurrentTimeDuration(); > + mStartTime.SetValue(now.Value() - mHoldTime.Value()); How do we know mHoldTime is not null here? Looking at the spec, it doesn't seem like it handles this case either. I think that's typically only going to happen when we call play() while the animation is already playing (and, once we implement the auto-rewind behavior, hasn't reached the end). In that case, play() should probably be a no-op to match the behavior in HTMLMediaElement.play(). If you agree, I'll update the spec. We still need to add a test for this and handle that case here. @@ +134,5 @@ > + if (!currentTime.IsNull()) { > + const AnimationTiming& timing = mSource->Timing(); > + if (timing.mIterationCount != NS_IEEEPositiveInfinity()) { > + TimeDuration end = timing.mDelay + > + timing.mIterationDuration.MultDouble(timing.mIterationCount); Instead of calculating the active duration by hand, this should use: mSource->GetComputedTiming().mActiveDuration Possibly also add a FIXME/comment saying that this is using the 'end time' so that when we implement 'end delay' or 'animation node start time' we'll remember to update this (preferably by calling an EndTime() method on GetComputedTiming()). @@ +136,5 @@ > + if (timing.mIterationCount != NS_IEEEPositiveInfinity()) { > + TimeDuration end = timing.mDelay + > + timing.mIterationDuration.MultDouble(timing.mIterationCount); > + if (currentTime.Value() >= end.ToMilliseconds()) { > + mHoldTime.SetValue(mStartTime.Value() + end); Why do we add to the start time here? Shouldn't it just be mHoldTime.SetValue(end) ? Also, this doesn't do correct handling of previous current times. The procedure in the spec regarding storing the previous current time is for the following case: player.source.timing.duration = 5000; player.currentTime = 4000; player.source.timing.duration = 3000; alert(player.currentTime); // Should get 4000 i.e. it's stative. If you sample at a point that later becomes past the end of the source content, the finishing behavior (where we don't progress past the end of the content) *shouldn't* make you jump back to the new end. We should add tests for this too. I think it would be fine to address this "finishing behavior" in a separate patch/bug in which case we should drop it from here. @@ +145,5 @@ > + } > +} > + > +void > +AnimationPlayer::UpdatePauseState() I'd rather calling this something like UpdatePlayStateStyle. UpdatePauseState() sounds very similar to UpdateFinishedState() but what they do is completely different. @@ +151,5 @@ > + nsAutoString state; > + nsTArray<nsRefPtr<AnimationPlayer>> players; > + Element* element = mSource->GetTarget(); > + element->GetAnimationPlayers(players); > + uint32_t i; This should be size_t @@ +175,5 @@ > + state.AppendLiteral(", paused"); > + } else { > + state.AppendLiteral(", running"); > + } > + } Can't we factor this into one loop? (e.g. append ", " if i != 0) The duplication of the transition check is sub-optimal since we'll likely add some condition there and forget to update both places. @@ +178,5 @@ > + } > + } > + > + if (state.IsEmpty()) { > + return; We should actually clear the property value in this case (if there is one). I think it might also be nice that, if all animations are running, we clear the property value. That obviously needs to get specced though. @@ +181,5 @@ > + if (state.IsEmpty()) { > + return; > + } > + > + if (element->IsHTML()) { Why do we only do this for HTML? We can have CSS animations on SVG too. @@ +185,5 @@ > + if (element->IsHTML()) { > + static_cast<nsGenericHTMLElement*>(element)->Style()-> > + SetPropertyValue(eCSSProperty_animation_play_state, state); > + } > +} This method (UpdatePauseState/UpdatePlayStateStyle) needs to get called when we do CheckAnimationRule. That way, if we remove the animations on an element its local animation-play-state gets reset. However, see my comments at the end. This whole approach may not work after all. ::: dom/animation/AnimationPlayer.h @@ +80,5 @@ > nsRefPtr<Animation> mSource; > + > +protected: > + void UpdateFinishedState(); > + We probably don't need this extra blank line here. ::: dom/animation/test/css-integration/test_current-time.html @@ +1,1 @@ > +<!DOCTYPE HTML> Sorry, but all the files here are supposed to be in a format suitable for submitting to web-platform-tests. That means using testharness.js Docs here: http://testthewebforward.org/docs/testharness-tutorial.html Samples here: https://github.com/w3c/web-platform-tests/ It also means not using Gecko-specific APIs like DOMWindowUtils. ::: layout/style/nsAnimationManager.cpp @@ +316,2 @@ > } > + MOZ_ASSERT(!rv.Failed()); This seems good, but the behavior changes here if now.IsNull() is true when we unpause. Previously we'd set the start time to null, but now we'll throw an exception. I'm not sure what we should do in that case. It won't happen currently but it could happen once we support different kinds of timelines and will happen when we make timelines optional. I think the algorithms in Web Animations need an audit for the case where the timeline is null. Probably pause()/play() should not throw since setting currentTime doesn't throw in this case. I think this is ok for now. I'll try to fix the spec. @@ +455,1 @@ > dest->mHoldTime.SetValue(TimeDuration(0)); I think this last line should no longer be necessary right? Also, I believe this will mean that whenever we update animation play state, we'll also end up setting the same play state on the element. I don't think that's what we want. In fact, I don't know if this approach works at all. For example, suppose an author specifies: animation: a 5s, b 5s Then they use the API to pause 'b' on a particular element, e.g. div.getAnimationPlayers()[1].pause(); At that point we'll write local style equivalent to: <div style="animation-play-state: running, paused"></div> Now, suppose the author wants to change the delay of the first animation: animation: a 5s 1s, b 5s I *think* at this point we'll be fine. But then, say, the author adds in another animation: animation: a 5s 1s, c 5s, b 5s I *think* at this point when we resolve styles we'll end up associating the pause state with 'c' instead of 'b'. From an authoring point of view that's surprising. What do you think? I wonder if mapping back to element style can work at all. For changes to the play state made by the API I think we want to set local style, but for changes to the play state made by CSS I think we want to skip that step. I'm not quite sure how we get there but I think we want something like

Comment on attachment 8489837 [details] [diff] [review] Part 9: Add AnimationPlayer.playState Review of attachment 8489837 [details] [diff] [review]: ----------------------------------------------------------------- It all looks good but I'd like to have a look at the tests before this gets landed so deferring r+ for now. ::: dom/animation/AnimationPlayer.cpp @@ +69,5 @@ > + const AnimationTiming& timing = mSource->Timing(); > + if (timing.mIterationCount != NS_IEEEPositiveInfinity()) { > + TimeDuration end = timing.mDelay + > + timing.mIterationDuration.MultDouble(timing.mIterationCount); > + if (currentTime.Value() >= end.ToMilliseconds()) { As with part 8, we should use mSource->GetComputedTiming().mActiveDuration In fact, I think we should add a private/protected AnimationPlayer::GetSourceContentEnd method that returns a TimeDuration since we're using this in at least two different places. Having a separate method would better describe what that calculation is and mean that we can make adjustments in just one place when the support end delay / null source content / non-zero start time etc. @@ +74,5 @@ > + return AnimationPlayState::Finished; > + } > + } > + > + return AnimationPlayState::Playing; AnimationPlayState::Running ::: dom/animation/test/css-integration/test_current-time.html @@ -126,1 @@ > new_div(""); Sorry, as with part 8, we should make this testharness.js tests. ::: dom/webidl/AnimationPlayer.webidl @@ +9,5 @@ > * Copyright © 2014 W3C® (MIT, ERCIM, Keio), All Rights Reserved. W3C > * liability, trademark and document use rules apply. > */ > > +enum AnimationPlayState { "idle", "pending", "playing", "paused", "finished" }; Sorry, the spec has been updated here. "playing" is now "running" to match CSS: http://w3c.github.io/web-animations/#enumdef-animationplaystate

(In reply to Brian Birtles (:birtles) from comment #31) > Comment on attachment 8489782 [details] [diff] [review] > Part 8: Add AnimationPlayer.play()/pause() > > > How do we know mHoldTime is not null here? Looking at the spec, it doesn't > seem like it handles this case either. > > I think that's typically only going to happen when we call play() while the > animation is already playing (and, once we implement the auto-rewind > behavior, hasn't reached the end). In that case, play() should probably be a > no-op to match the behavior in HTMLMediaElement.play(). > > If you agree, I'll update the spec. We still need to add a test for this > and handle that case here. > Yes, that makes sense. > @@ +134,5 @@ > > + if (!currentTime.IsNull()) { > > + const AnimationTiming& timing = mSource->Timing(); > > + if (timing.mIterationCount != NS_IEEEPositiveInfinity()) { > > + TimeDuration end = timing.mDelay + > > + timing.mIterationDuration.MultDouble(timing.mIterationCount); > > Instead of calculating the active duration by hand, this should use: > > mSource->GetComputedTiming().mActiveDuration Ah, I didn't know about this. > > Possibly also add a FIXME/comment saying that this is using the 'end time' > so that when we implement 'end delay' or 'animation node start time' we'll > remember to update this (preferably by calling an EndTime() method on > GetComputedTiming()). > > @@ +136,5 @@ > > + if (timing.mIterationCount != NS_IEEEPositiveInfinity()) { > > + TimeDuration end = timing.mDelay + > > + timing.mIterationDuration.MultDouble(timing.mIterationCount); > > + if (currentTime.Value() >= end.ToMilliseconds()) { > > + mHoldTime.SetValue(mStartTime.Value() + end); > > Why do we add to the start time here? Shouldn't it just be > mHoldTime.SetValue(end) ? I don't think so... end time = start time + start delay + active duration + end delay, so we need both the start time of the player and the start delay of the animation > > Also, this doesn't do correct handling of previous current times. > > The procedure in the spec regarding storing the previous current time is for > the following case: > > player.source.timing.duration = 5000; > player.currentTime = 4000; > player.source.timing.duration = 3000; > alert(player.currentTime); // Should get 4000 > > i.e. it's stative. If you sample at a point that later becomes past the end > of the source content, the finishing behavior (where we don't progress past > the end of the content) *shouldn't* make you jump back to the new end. > > We should add tests for this too. > > I think it would be fine to address this "finishing behavior" in a separate > patch/bug in which case we should drop it from here. > You mean drop UpdateFinishedState? I can certainly do that for now. > > @@ +178,5 @@ > > + } > > + } > > + > > + if (state.IsEmpty()) { > > + return; > > We should actually clear the property value in this case (if there is one). > > I think it might also be nice that, if all animations are running, we clear > the property value. That obviously needs to get specced though. > > @@ +181,5 @@ > > + if (state.IsEmpty()) { > > + return; > > + } > > + > > + if (element->IsHTML()) { > > Why do we only do this for HTML? We can have CSS animations on SVG too. > Yeah, I forgot to add that. > > ::: dom/animation/test/css-integration/test_current-time.html > @@ +1,1 @@ > > +<!DOCTYPE HTML> > > Sorry, but all the files here are supposed to be in a format suitable for > submitting to web-platform-tests. That means using testharness.js > > Docs here: http://testthewebforward.org/docs/testharness-tutorial.html > Samples here: https://github.com/w3c/web-platform-tests/ > > It also means not using Gecko-specific APIs like DOMWindowUtils. > Yeah, I think we need to keep using DOMWindowUtils to advance the timer until we implement setting currentTime...I can move the test to layout/style for now. > ::: layout/style/nsAnimationManager.cpp > @@ +316,2 @@ > > } > > + MOZ_ASSERT(!rv.Failed()); > > This seems good, but the behavior changes here if now.IsNull() is true when > we unpause. Previously we'd set the start time to null, but now we'll throw > an exception. > > I'm not sure what we should do in that case. It won't happen currently but > it could happen once we support different kinds of timelines and will happen > when we make timelines optional. > > I think the algorithms in Web Animations need an audit for the case where > the timeline is null. Probably pause()/play() should not throw since setting > currentTime doesn't throw in this case. > > I think this is ok for now. I'll try to fix the spec. > > @@ +455,1 @@ > > dest->mHoldTime.SetValue(TimeDuration(0)); > > I think this last line should no longer be necessary right? Right, the current time is 0 because startTime = now. > > Also, I believe this will mean that whenever we update animation play state, > we'll also end up setting the same play state on the element. I don't think > that's what we want. In fact, I don't know if this approach works at all. > > For example, suppose an author specifies: > > animation: a 5s, b 5s > > Then they use the API to pause 'b' on a particular element, e.g. > > div.getAnimationPlayers()[1].pause(); > > At that point we'll write local style equivalent to: > > <div style="animation-play-state: running, paused"></div> > > Now, suppose the author wants to change the delay of the first animation: > > animation: a 5s 1s, b 5s > > I *think* at this point we'll be fine. But then, say, the author adds in > another animation: > > animation: a 5s 1s, c 5s, b 5s > > I *think* at this point when we resolve styles we'll end up associating the > pause state with 'c' instead of 'b'. From an authoring point of view that's > surprising. > > What do you think? I wonder if mapping back to element style can work at all. > Well, when we detect that you change the animation property, we will call BuildAnimations, so we could actually update the animation-play-state there based on the play state of the players, right? But yeah, I'm not really a fan of the API calls changing CSS properties. > For changes to the play state made by the API I think we want to set local > style, but for changes to the play state made by CSS I think we want to skip > that step. > > I'm not quite sure how we get there but I think we want something like I'm not sure where you were going with this.

(In reply to David Zbarsky (:dzbarsky) from comment #33) > (In reply to Brian Birtles (:birtles) from comment #31) > > Comment on attachment 8489782 [details] [diff] [review] > > Part 8: Add AnimationPlayer.play()/pause() > > > > > > How do we know mHoldTime is not null here? Looking at the spec, it doesn't > > seem like it handles this case either. > > > > I think that's typically only going to happen when we call play() while the > > animation is already playing (and, once we implement the auto-rewind > > behavior, hasn't reached the end). In that case, play() should probably be a > > no-op to match the behavior in HTMLMediaElement.play(). > > > > If you agree, I'll update the spec. We still need to add a test for this > > and handle that case here. > > > > Yes, that makes sense. Ok, I updated the spec. > > Possibly also add a FIXME/comment saying that this is using the 'end time' > > so that when we implement 'end delay' or 'animation node start time' we'll > > remember to update this (preferably by calling an EndTime() method on > > GetComputedTiming()). > > > > @@ +136,5 @@ > > > + if (timing.mIterationCount != NS_IEEEPositiveInfinity()) { > > > + TimeDuration end = timing.mDelay + > > > + timing.mIterationDuration.MultDouble(timing.mIterationCount); > > > + if (currentTime.Value() >= end.ToMilliseconds()) { > > > + mHoldTime.SetValue(mStartTime.Value() + end); > > > > Why do we add to the start time here? Shouldn't it just be > > mHoldTime.SetValue(end) ? > > > I don't think so... end time = start time + start delay + active duration + > end delay, so we need both the start time of the player and the start delay > of the animation But the start time in that equation is the *animation node* start time which is currently always zero. Maybe I should rename that to avoid confusion. > > Also, this doesn't do correct handling of previous current times. > > > > The procedure in the spec regarding storing the previous current time is for > > the following case: > > > > player.source.timing.duration = 5000; > > player.currentTime = 4000; > > player.source.timing.duration = 3000; > > alert(player.currentTime); // Should get 4000 > > > > i.e. it's stative. If you sample at a point that later becomes past the end > > of the source content, the finishing behavior (where we don't progress past > > the end of the content) *shouldn't* make you jump back to the new end. > > > > We should add tests for this too. > > > > I think it would be fine to address this "finishing behavior" in a separate > > patch/bug in which case we should drop it from here. > > > > You mean drop UpdateFinishedState? I can certainly do that for now. Yeah, it's pretty tricky to get right and needs lots of tests. So we should probably move that to its own patch/bug. > > ::: dom/animation/test/css-integration/test_current-time.html > > @@ +1,1 @@ > > > +<!DOCTYPE HTML> > > > > Sorry, but all the files here are supposed to be in a format suitable for > > submitting to web-platform-tests. That means using testharness.js > > > > Docs here: http://testthewebforward.org/docs/testharness-tutorial.html > > Samples here: https://github.com/w3c/web-platform-tests/ > > > > It also means not using Gecko-specific APIs like DOMWindowUtils. > > > > Yeah, I think we need to keep using DOMWindowUtils to advance the timer > until we implement setting currentTime...I can move the test to layout/style > for now. That would be fine if you plan to then move it back (and rework it) in a later patch. Alternatively you could just use requestAnimationFrame to wait for time to elapse. > > ... > > What do you think? I wonder if mapping back to element style can work at all. > > > > Well, when we detect that you change the animation property, we will call > BuildAnimations, so we could actually update the animation-play-state there > based on the play state of the players, right? > > But yeah, I'm not really a fan of the API calls changing CSS properties. I'm somewhat dubious that it works. I think we'd probably end up internally storing some flag to say whether the play state had been overridden or not so we know how to update the element style. Then we'd also need to track if the author overwrote the element style so we know if we can clobber it or not. I think it's not going to work. I think it's probably better to just have two pause sources that combine as a bit-field. e.g. MOZ_BEGIN_ENUM_CLASS(AnimationPlayerPauseSources, uint8_t) NONE = 0 API = 1 << 0, CSS = 1 << 1 MOZ_END_ENUM_CLASS(AnimationPlayerPauseSources) MOZ_MAKE_ENUM_CLASS_BITWISE_OPERATORS(AnimationPlayerPauseSources) AnimationPlayerPauseSources mPauseSources = AnimationPlayerPauseSources::NONE; animation-play-state -> paused: Pause(AnimationPlayerPauseSources::CSS) Pause(AnimationPlayerPauseSources aPauseSources = AnimationPlayerPauseSources::API): mPauseSources |= aPauseSources animation-play-state -> running: Play(AnimationPlayerPauseSources::CSS) Play(AnimationPlayerPauseSources aPauseSources = AnimationPlayerPauseSources::API): mPauseSources &= ~aPauseSource Then wherever the spec talks about the pause flag as a boolean value, we just use (mPauseSources == AnimationPlayerPauseSources::None). However, I'm not sure about how that layering works from a spec point of view. We might need to add some spec hooks in there since this is CSS Animations-specific functionality. What do you think? > > I'm not quite sure how we get there but I think we want something like > > I'm not sure where you were going with this. Yeah, I meant to drop that part :)

David, I think we should split up this bug. Part of it has already landed in mozilla 34 so it's getting hard to track like this. It was always meant to be a meta bug. Exposing playbackState already has its own bug too (bug 1037321). I'm keen to land something soon for play()/pause() since it would make bug 927349 easier (and if I land that first, it's going to make your job harder). How about: * Make a new meta bug for playback control * Rename this "Refactor AnimationPlayer startTime and currentTime" * Bug for making currentTime writeable * Bug for play()/pause() * Bug 1037321 for playState * Bug for setting startTime * Bug for playbackRate / reverse() * Bug finish() incl. finished promise * Bug for cancel() I'm working on all the pending stuff in bug 927349 but I might split some of that off and make a separate bug for the ready promise

Also note that Google are planning on shipping this part of the interface so it would be good to find any problems soon: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/gfifgxHdPXU

(In reply to Brian Birtles (:birtles) from comment #35) > David, I think we should split up this bug. Part of it has already landed in > mozilla 34 so it's getting hard to track like this. It was always meant to > be a meta bug. Exposing playbackState already has its own bug too (bug > 1037321). > > I'm keen to land something soon for play()/pause() since it would make bug > 927349 easier (and if I land that first, it's going to make your job harder). > > How about: > * Make a new meta bug for playback control > * Rename this "Refactor AnimationPlayer startTime and currentTime" > * Bug for making currentTime writeable > * Bug for play()/pause() > * Bug 1037321 for playState > * Bug for setting startTime > * Bug for playbackRate / reverse() > * Bug finish() incl. finished promise > * Bug for cancel() > > I'm working on all the pending stuff in bug 927349 but I might split some of > that off and make a separate bug for the ready promise Ok, I made the meta bug. I have updated the play/pause patch, I just need to figure out why it's failing the test I wrote and then I can land it. If you're really blocked on it, we can probably land it as-is, because the test only fails when calling player.pause()/play() - when changing the animation play state style, the behavior is correct. So we can just land this patch and not expose the methods in WebIDL until I sort out the failure.

(In reply to David Zbarsky (:dzbarsky) from comment #37) > Ok, I made the meta bug. I have updated the play/pause patch, I just need > to figure out why it's failing the test I wrote and then I can land it. If > you're really blocked on it, we can probably land it as-is, because the test > only fails when calling player.pause()/play() - when changing the animation > play state style, the behavior is correct. So we can just land this patch > and not expose the methods in WebIDL until I sort out the failure. Thanks! I think it's better to fix the bug and land it altogether. It's a public holiday here tomorrow and I won't finish bug 927349 today, so it's not going to block me for at least a couple of days. For the time being, I'll rebase my work on top of part 8 assuming it's going to land first.

Any chance the patches that haven't been checked-in here could be rebased? (especially the one making the current-time settable). I'm hoping to make a devtools animation editor panel my hack target for next week, and having this would help a lot.

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #39) > Any chance the patches that haven't been checked-in here could be rebased? > (especially the one making the current-time settable). > I'm hoping to make a devtools animation editor panel my hack target for next > week, and having this would help a lot. I rewrote the currentTime patch. It's not right in a number of ways (e.g. animation events won't be dispatched more than once) but hopefully it's enough for prototyping. It will probably bitrot fairly quickly since I'm currently touching this part of the code so if you need an updated patch just let me know.