Bug 1040543 introduces the Animation interface and the AnimationPlayer.source member. This member, per spec, should be a writeable AnimationNode member but currently is a readonly Animation member. We should make this writeable. That, however, depends on: a) Fixing up the spec so the behavior when replacing the source content is more robustly defined. b) Making animations better handle jumping backwards in time. For example, currently we have an mLastNotification member which is used for dispatching CSS Animation events. This member assumes times is monotonically increasing. What events get dispatched when time goes backwards needs to be spec'ed and then we need to make this class operate correctly in this case. There are possibly other situations where we assume time goes forwards. For the other difference, having an Animation member instead of an AnimationNode member, assuming AnimationNode becomes [NoInterfaceObject] then until we implement AnimationGroup (the only other subinterface of AnimationNode at this stage) I don't know that this difference will be observable. If it is somehow observable then we should file a separate bug for that. Otherwise we can fix this when we implement AnimationGroup.

Review commit: https://reviewboard.mozilla.org/r/63662/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/63662/

Review commit: https://reviewboard.mozilla.org/r/63664/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/63664/

Review commit: https://reviewboard.mozilla.org/r/63666/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/63666/

In order to merge these two SetEffect()s, we have to convert AnimationEffectReadOnly into KeyframeEffectReadOnly, so implement AnimationEffectReadOnly::AsKeyframeEffectReadOnly(). Review commit: https://reviewboard.mozilla.org/r/63668/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/63668/

Both Cancel() and SetEffect() need the procedure of "Reset an animation's pending tasks", so factor it out and then we can reuse it. Review commit: https://reviewboard.mozilla.org/r/64556/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/64556/

Review commit: https://reviewboard.mozilla.org/r/64558/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/64558/

Review commit: https://reviewboard.mozilla.org/r/64560/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/64560/

Great! Boris, would you mind adding some reftests to check stacking context and mochitests to check isRunningOnCompositor flag when effects are changed? Just like tests I added recently; https://hg.mozilla.org/mozilla-central/rev/ba6a3c404ebb https://hg.mozilla.org/mozilla-central/rev/fe5ff9a26314

(In reply to Hiroyuki Ikezoe (:hiro) from comment #8) > Great! > > Boris, would you mind adding some reftests to check stacking context and > mochitests to check isRunningOnCompositor flag when effects are changed? > Just like tests I added recently; > https://hg.mozilla.org/mozilla-central/rev/ba6a3c404ebb > https://hg.mozilla.org/mozilla-central/rev/fe5ff9a26314 Sure. Thanks for your examples. I'm still working on Mutation observer when effects are changed, and will add more tests to check stacking context and isRunningOnCompositor flag.

We have to call nsNodeUtils::AnimationRemoved(this) before calling mEffect->SetAnimation(nullptr), so the mutation observer works correctly for the original effect. Review commit: https://reviewboard.mozilla.org/r/64850/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/64850/

Review commit: https://reviewboard.mozilla.org/r/64852/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/64852/

Review commit: https://reviewboard.mozilla.org/r/64870/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/64870/

Review commit: https://reviewboard.mozilla.org/r/64872/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/64872/

Comment on attachment 8771925 [details] Bug 1049975 - Part 12: Test for running on the compositor when effects are changed. https://reviewboard.mozilla.org/r/64870/#review63088 ::: dom/animation/test/chrome/test_running_on_compositor.html:519 (Diff revision 1) > + var div1 = addDiv(t); > + var div2 = addDiv(t); I don't like this kind of numbering variable names. What will happen for effect change on the same element? If it works (I think it should work), we should use one element here.

https://reviewboard.mozilla.org/r/64870/#review63094 ::: dom/animation/test/chrome/test_running_on_compositor.html:537 (Diff revision 1) > + 'Opacity animation on a 100% opacity keyframe keeps ' + > + 'running on the compositor after changing the target ' + > + 'effect'); > + }); > +}, '100% opacity animations which keeps running on the compositor after ' + > + 'changing the effects'); Oops, these descriptions are slightly wrong. The opacity animation do not actually run before changing effect, right?

https://reviewboard.mozilla.org/r/64870/#review63094 > Oops, these descriptions are slightly wrong. The opacity animation do not actually run before changing effect, right? Yes. I should update this. Sorry about that. I will upload a new patch to change effect on the same element and update the description soon. Thanks.

Comment on attachment 8771926 [details] Bug 1049975 - Part 13: Add reftests for stacking context when effects are changed. https://reviewboard.mozilla.org/r/64872/#review63098 I think these reftest should work on the same element too, so we don't need stacking-context-animation-changing-effect-ref.html.

Comment on attachment 8771925 [details] Bug 1049975 - Part 12: Test for running on the compositor when effects are changed. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64870/diff/1-2/

Comment on attachment 8771926 [details] Bug 1049975 - Part 13: Add reftests for stacking context when effects are changed. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64872/diff/1-2/

Comment on attachment 8771333 [details] Bug 1049975 - Part 2: Make effect writable in Animation.webidl. https://reviewboard.mozilla.org/r/63666/#review63414

Comment on attachment 8771331 [details] Bug 1049975 - Part 1: Support null effect. https://reviewboard.mozilla.org/r/63662/#review63496

Comment on attachment 8771332 [details] Bug 1049975 - Part 3: Handle removed/replaced effect for CSS Transition. https://reviewboard.mozilla.org/r/63664/#review63498 (This should probably be merged into part 1, since if we land part 1 without this these tests will pass unexpectedly.)

https://reviewboard.mozilla.org/r/63664/#review63498 OK. I will merge this patch into part 1 later. Thanks.

Comment on attachment 8771334 [details] Bug 1049975 - Part 4: Merge two Animation::SetEffect()s. https://reviewboard.mozilla.org/r/63668/#review63494 This seems odd to me. We have all the methods to safely downcast, but we hardcode them to just cast directly. I think we should either: a) Make Animation store a pointer to an AnimationEffectReadOnly object and move any methods used by Animation to interact with mEffect to AnimationEffectReadOnly -- either by moving the implementation there (e.g. for timing-related methods) or by adding a pure virtual method there and overriding it in KeyframeEffectReadOnly (e.g. for keyframe-related methods). b) Make AsKeyframeEffectReadOnly a virtual method and override it in KeyframeEffectReadOnly. (a) is better since it matches the spec better and separates out the code for timing vs keyframes. However, we might discover it adds too many virtual methods and leads to other inefficiencies. It might be ok to do (b) here and try (a) in another bug (which we can drop / backout if it is inefficient). More importantly, before landing this patch, we need to fix nsTransitionManager to work when an effect that is not a transition effect is used. Just search the code base for "1049975" and you'll see what I mean. ::: dom/animation/Animation.cpp:133 (Diff revision 2) > { > RefPtr<Animation> kungFuDeathGrip(this); > > - if (mEffect == aEffect) { > + KeyframeEffectReadOnly* newEffect = aEffect > + ? aEffect->AsKeyframeEffectReadOnly() > + : nullptr; It might be worth adding an assertion here that if aEffect is non-null, newEffect is also non-null e.g. MOZ_ASSERT(!aEffect == !newEffect, "Unexpected effect type"); ::: dom/animation/KeyframeEffect.h:488 (Diff revision 2) > > protected: > ~KeyframeEffect() override; > }; > > + Unnecessary extra line here.

Comment on attachment 8771335 [details] Bug 1049975 - Part 6: Factor out the procedure of resetting an animation's pending tasks. https://reviewboard.mozilla.org/r/64556/#review63502 I&#39;m a bit unsure how this should work. I think our current implementation differs from the spec but I&#39;m not sure which is right. In our implementation, we only reject the ready promise if we&#39;re pending. However, the spec doesn&#39;t say that. The spec appears to cancel the promise every time. I guess if we do something like: // Assume animation is not currently pending var ready1 = animation.ready; animation.cancel(); var ready2 = animation.ready; console.log(ready1 === ready2); I guess we&#39;ll get true in Firefox but the spec says it should be false. Can you check what Chrome does? I think what Firefox does is probably better, so if that&#39;s the case we should update the spec. Cancelling review until we work out what Chrome does and (most likely) fix the spec. ::: dom/animation/Animation.h:373 (Diff revision 2) > * mPendingState as necessary. The caller is responsible for resolving or > * aborting the mReady promise as necessary. > */ > void CancelPendingTasks(); > > + void ResetAnimationPendingTask(); Let's just call this ResetPendingTasks and add a comment to indicate how it differs from CancelPendingTasks(). e.g. /** * Performs the same steps as CancelPendingTasks and also rejects and recreates the ready promise. */ ::: dom/animation/Animation.cpp:1162 (Diff revision 2) > + if (mPendingState != PendingState::NotPending) { > + CancelPendingTasks(); > + if (mReady) { > + mReady->MaybeReject(NS_ERROR_DOM_ABORT_ERR); > + } > + } Generally our preferred coding style is: if (mPendingState == PendingState::NotPending) { return; } CancelPendingTasks(); if (mReady) { mReady->MaybeReject(NS_ERROR_DOM_ABORT_ERR); } i.e. return early and reduce identation.

Comment on attachment 8771336 [details] Bug 1049975 - Part 9: Implement writable Animation effect. https://reviewboard.mozilla.org/r/64558/#review63508 ::: dom/animation/Animation.cpp:143 (Diff revision 2) > > if (mEffect) { > + mIsRelevant = false; > mEffect->SetAnimation(nullptr); > + > + // Request a restyle for the target of old the effect. Missing 'the' before 'old' ::: dom/animation/Animation.cpp:143 (Diff revision 2) > + // Request a restyle for the target of old the effect. > + // Note: mEffect->SetAnimation() will set the animation to nullptr first, > + // so it cannot request a restyle for its target element, and > + // therefore we have do it explicitly. > + PostUpdate(); Is there any reason we don't just fix this in SetAnimation? In either case, this will be called when creating animations nsTransitionManager/nsAnimationManager. Is that ok? Do we need a SetEffectNoUpdate or something? ::: dom/animation/Animation.cpp:150 (Diff revision 2) > mEffect = newEffect; > + > if (mEffect) { > + // Reset previous animation of the new effect. > + Animation* prevAnim = mEffect->GetAnimation(); > + if (prevAnim) { > + prevAnim->SetEffect(nullptr); > + } > + > mEffect->SetAnimation(this); This puts us in a slightly odd situation where we can temporarily have: oldAnim -> newEffect newAnim -> newEffect newEffect -> oldAnim I'm a bit concerned that as we call things like PostUpdate we'll end up with obscure bugs. Would it be possible to do something like: 1. auto oldEffect = mEffect (i.e. hold an owning reference to it) 2. mEffect = nullptr; 3. oldEffect->SetAnimation(nullptr); 4. prevAnim->SetEffect(nullptr); 5. mEffect = newEffect; 6. mEffect->SetAnimation(this); (Not sure if we'll need to add an owning ref to newEffect in case prevAnim holds the last reference to it) Something like that might be more robust, particularly if this code becomes more complicated. What do you think? ::: dom/animation/Animation.cpp:167 (Diff revision 2) > + if (mPendingState == PendingState::PlayPending) { > + tracker->RemovePlayPending(*this); > + tracker->AddPlayPending(*this); > + } else { > + tracker->RemovePausePending(*this); > + tracker->AddPausePending(*this); > + } Does this actually work? i.e. is there a test case we can write that fails if we don't do this? Perhaps we need to check if mPendingReadyTime is set, and, if it is, clear it and re-add ourselves to the tracker? ::: dom/animation/Animation.cpp:179 (Diff revision 2) > + // The spec doesn't ask for unresolving the hold time and start time, but if > + // we don't do this, PlayState() may return "paused" (if these is a pending > + // task) or return "finished" (if there is a running/finished task), > + // Unresolving the hold time and start time makes PlayState() return "idle". > + mHoldTime.SetNull(); > + mStartTime.SetNull(); That sounds like the correct behavior to me. That playState should *not* become idle. For example, it should be possible to do: // Swap effects var effectA = animA.effect; animA.effect = animB.effect; animB.effect = effectA; And, assuming both animations are playing, they should maintain their current time and continue playing. If animB becomes idle when its effect is stolen, it will lose its current time. In fact, we should probably add that as a test case. ::: dom/animation/Animation.cpp:187 (Diff revision 2) > + // If mEffect is not null, UpdateTiming will request a restyle of the current > + // target element eventually, so we don't need to PostUpdate() after setting > + // a new valid effect. > UpdateTiming(SeekFlag::NoSeek, SyncNotifyFlag::Async); But it won't request a layer update right? So if we substitute in an effect with exactly the same target effect and, say, a different iteration count, I guess we might fail to update the animations on layers? We should probably write a test case for this (and check it fails) before fixing it.

(In reply to Brian Birtles (:birtles) from comment #28) > a) Make Animation store a pointer to an AnimationEffectReadOnly object and > move any methods used by Animation to interact with mEffect to > AnimationEffectReadOnly -- either by moving the implementation there (e.g. > for timing-related methods) or by adding a pure virtual method there and > overriding it in KeyframeEffectReadOnly (e.g. for keyframe-related methods). > > b) Make AsKeyframeEffectReadOnly a virtual method and override it in > KeyframeEffectReadOnly. > > (a) is better since it matches the spec better and separates out the code > for timing vs keyframes. However, we might discover it adds too many virtual > methods and leads to other inefficiencies. It might be ok to do (b) here and > try (a) in another bug (which we can drop / backout if it is inefficient). OK. Let's try (a) first because I think matching the spec and making code clearer are more important. I'd like to add some pure virtual functions in AnimationEffectReadOnly and override them. > More importantly, before landing this patch, we need to fix > nsTransitionManager to work when an effect that is not a transition effect > is used. Just search the code base for "1049975" and you'll see what I mean. I didn't notice this. I will check it. Thanks.

(In reply to Boris Chiou [:boris] from comment #31) > (In reply to Brian Birtles (:birtles) from comment #28) > > a) Make Animation store a pointer to an AnimationEffectReadOnly object and > > move any methods used by Animation to interact with mEffect to > > AnimationEffectReadOnly -- either by moving the implementation there (e.g. > > for timing-related methods) or by adding a pure virtual method there and > > overriding it in KeyframeEffectReadOnly (e.g. for keyframe-related methods). > > > > (a) is better since it matches the spec better and separates out the code > > for timing vs keyframes. However, we might discover it adds too many virtual > > methods and leads to other inefficiencies. It might be ok to do (b) here and > > try (a) in another bug (which we can drop / backout if it is inefficient). > I will implement (a) in two parts (i.e. two patches): 1. Just add those virtual functions to make sure that we pass the test cases and compilation. 2. Try to move some implementation into AnimationEffectReadOnly. I think mAnimation and mTiming should be moved into AnimationEffectReadOnly because they don't belong to KeyframeEffectReadOnly, right? Therefore, AnimationEffectReadOnly is still an abstract class, and leave the implementation of other functions, e.g. ComposeStyle, SetKeyframes, AnimationProperty related functions, ..., and other complicated functions, in KeyframeEffectReadOnly. This might make KeyframeEffectReadOnly simpler.

Comment on attachment 8771337 [details] Bug 1049975 - Part 10: Test for writable effect. https://reviewboard.mozilla.org/r/64560/#review63888 I've only reviewed about 2/3rds of this since I think my feedback for the remaining 1/3 will be the same. Namely: * These tests will need to be updated to reflect the changes mentioned in comment 30 * The tests in set-the-target-effect-of-an-animation.html should be more closely aligned with the algorithm they are testing * Tests using getComputedStyle should go in dom/animation/tests/style Apart from that it looks good. ::: testing/web-platform/tests/web-animations/timing-model/animations/set-the-target-effect-of-an-animation.html:17 (Diff revision 2) > + > +// --------------------------------------------------------------------- > +// > +// Tests from no effect to a valid effect > +// > +// --------------------------------------------------------------------- Nit: blank line after this ::: testing/web-platform/tests/web-animations/timing-model/animations/set-the-target-effect-of-an-animation.html:18 (Diff revision 2) > +test(function(t) { > + var anim = new Animation(null, document.timeline); > + var target = createDiv(t); > + var effect = new KeyframeEffectReadOnly(target, > + { marginLeft: [ '0px', '100px' ] }, > + 100 * MS_PER_SEC); > + anim.effect = effect; > + anim.currentTime = 50 * MS_PER_SEC; > + assert_equals(getComputedStyle(target).marginLeft, '50px'); > +}, 'After setting target effect on an animation with null effect, the ' + > + 'animation works normally'); Although this seems like a useful test, I don't think it matches anything in the spec. We should try to test just the behavior described in: https://w3c.github.io/web-animations/#setting-the-target-effect This kind of test probably belongs in dom/animation/test/style In fact, as some general feedback, I think it would be ideal if we structured these tests around the steps in that procedure[1]. If that's easy to do then that's great, otherwise don't feel like you need to completely rewrite these. [1] i.e. https://w3c.github.io/web-animations/#setting-the-target-effect ::: testing/web-platform/tests/web-animations/timing-model/animations/set-the-target-effect-of-an-animation.html:19 (Diff revision 2) > +// > +// Tests from no effect to a valid effect > +// > +// --------------------------------------------------------------------- > +test(function(t) { > + var anim = new Animation(null, document.timeline); We should just be able to write: var anim = new Animation(); I think the default values will be null and document.timeline (unless we haven't implemented that yet?). ::: testing/web-platform/tests/web-animations/timing-model/animations/set-the-target-effect-of-an-animation.html:30 (Diff revision 2) > +test(function(t) { > + var anim = new Animation(null, document.timeline); > + var effect = new KeyframeEffectReadOnly(createDiv(t), > + { marginLeft: [ "0px", "100px" ] }, > + 100 * MS_PER_SEC); > + anim.finish(); > + assert_equals(anim.currentTime, 0); > + anim.effect = effect; > + assert_equals(anim.playState, 'running'); > +}, 'After setting target effect on a finished animation with null effect, the ' + > + 'animation.playState becomes running'); Do we actually need to call finish? The order of the lines here makes it hard to follow what is being tested. Perhaps something like: var anim = ....; assert_equals(anim.playState, 'finished'); anim.effect = new KeyframeEffectReadOnly(...); assert_equals(anim.playState('running')); (If you're curious, this video gives some excellent advice for making tests easy to read: https://amara.org/en/videos/sHcrjrn64Qvp/url/2001841/) ::: testing/web-platform/tests/web-animations/timing-model/animations/set-the-target-effect-of-an-animation.html:31 (Diff revision 2) > + assert_equals(getComputedStyle(target).marginLeft, '50px'); > +}, 'After setting target effect on an animation with null effect, the ' + > + 'animation works normally'); > + > +test(function(t) { > + var anim = new Animation(null, document.timeline); Likewise for the default here. ::: testing/web-platform/tests/web-animations/timing-model/animations/set-the-target-effect-of-an-animation.html:42 (Diff revision 2) > +promise_test(function(t) { > + var anim = new Animation(null, document.timeline); > + var effect = new KeyframeEffectReadOnly(createDiv(t), > + { marginLeft: [ '0px', '100px' ] }, > + 100 * MS_PER_SEC); > + anim.play(); > + assert_equals(anim.playState, 'pending'); > + anim.effect = effect; > + assert_equals(anim.playState, 'pending'); > + return anim.ready.then(function() { > + assert_equals(anim.playState, 'running'); > + }); > +}, 'After setting target effect on an pending animation with null effect, we ' + > + 'reschedule the pending task and run as soon as the animation is ready'); Does this actually fail if we don't reschedule the task? It might be more useful to do something like: 1. Play 2. Wait an animation frame (At this point the animation *may* still be pending. In Firefox is sometimes is still pending, in Chrome is appears to be always still pending) 3. Set the effect 4. Check that it is still pending and, if it was previously pending, that the ready promise is re-used (i.e. same object). Would that work? ::: testing/web-platform/tests/web-animations/timing-model/animations/set-the-target-effect-of-an-animation.html:57 (Diff revision 2) > +test(function(t) { > + var target = createDiv(t); > + var effect = new KeyframeEffectReadOnly(target, > + { marginLeft: [ '0px', '100px' ] }, > + 100 * MS_PER_SEC); > + var prevAnim = new Animation(effect, document.timeline); > + var anim = new Animation(null, document.timeline); > + prevAnim.currentTime = 50 * MS_PER_SEC; > + anim.effect = effect; > + assert_equals(prevAnim.effect, null); > + assert_equals(prevAnim.playState, 'idle', > + 'playState of the previous animation becomes idle'); > +}, 'After setting target effect with a previous animation, ' + > + 'effect of the previous animation should be clear correctly'); As mentioned in comment 30, I don't think this is the correct behavior. I think we should be doing what the spec does here--i.e. prevAnim should end up being finished and maintain its currentTime. ::: testing/web-platform/tests/web-animations/timing-model/animations/set-the-target-effect-of-an-animation.html:73 (Diff revision 2) > + var target = createDiv(t); > + var effect = new KeyframeEffectReadOnly(target, > + { marginLeft: [ '0px', '100px' ] }, > + 100 * MS_PER_SEC); > + var prevAnim = new Animation(effect, document.timeline); > + var anim = new Animation(null, document.timeline); > + prevAnim.currentTime = 50 * MS_PER_SEC; > + assert_equals(effect.getComputedTiming().progress, 0.5); > + anim.currentTime = 20 * MS_PER_SEC; > + anim.effect = effect; > + assert_equals(effect.getComputedTiming().progress, 0.2); Perhaps something like the following would be more clear? var target = createDiv(t); var animA = target.animate({ ... }, 100 * MS_PER_SEC); var animB = new Animation(); var effect = animA.effect; animA.currentTime = 50 * MS_PER_SEC; animB.currentTime = 20 * MS_PER_SEC; assert_equals(effect.getComputedTiming().progress, 0.5, 'Originally timing comes from first animation'); animB.effect = animA.effect; assert_equals(effect.getComputedTiming().progress, 0.2, 'After setting the effect on a different animation' + ' it uses the new animation\'s timing'); ::: testing/web-platform/tests/web-animations/timing-model/animations/set-the-target-effect-of-an-animation.html:88 (Diff revision 2) > + var target = createDiv(t); > + var effect = new KeyframeEffectReadOnly(target, > + { marginLeft: [ '0px', '100px' ] }, > + 100 * MS_PER_SEC); > + var prevAnim = new Animation(effect, document.timeline); > + var anim = new Animation(null, document.timeline); > + prevAnim.currentTime = 50 * MS_PER_SEC; > + assert_equals(getComputedStyle(target).marginLeft, '50px', > + 'original computed style of the target element'); > + anim.currentTime = 20 * MS_PER_SEC; > + anim.effect = effect; > + assert_equals(getComputedStyle(target).marginLeft, '20px', > + 'new computed style of the target element'); Again, this isn't really testing the algorithm in the spec it's supposed to be testing. If this is an important test then we should put it in dom/animation/tests/style ::: testing/web-platform/tests/web-animations/timing-model/animations/set-the-target-effect-of-an-animation.html:110 (Diff revision 2) > + var effect = new KeyframeEffectReadOnly(createDiv(t), > + { marginLeft: [ "0px", "100px" ] }, > + 100 * MS_PER_SEC); > + var anim = new Animation(effect, document.timeline); > + anim.play(); > + assert_equals(anim.playState, 'pending'); > + anim.effect = null; > + assert_equals(anim.playState, 'idle'); As per comment 30, I don't think this is the correct behavior. ::: testing/web-platform/tests/web-animations/timing-model/animations/set-the-target-effect-of-an-animation.html:111 (Diff revision 2) > +// Tests from effect to no effect > +// > +// --------------------------------------------------------------------- > +test(function(t) { > + var effect = new KeyframeEffectReadOnly(createDiv(t), > + { marginLeft: [ "0px", "100px" ] }, Nit: Sometimes we use " and sometimes we use '. We should probably stick to ' where possible (the tests coming from Google all use ').

Comment on attachment 8771923 [details] Bug 1049975 - Part 11: Fix mutation observer when setting effects. https://reviewboard.mozilla.org/r/64850/#review63894 Clearing review request for now since it seems like we should do the change handling described below. For what it's worth, I think it would be ok to combine part 9 together with this part (i.e. tests and code change together). ::: dom/animation/Animation.cpp:140 (Diff revision 1) > + if (IsRelevant()) { > + nsNodeUtils::AnimationRemoved(this); > + } I wonder what we should do for the case where we have: var target = ...; var effectA = new KeyframeEffectReadOnly(target, ...); var effectB = new KeyframeEffectReadOnly(target, ...); var anim = new Animation(effectA); requestAnimationFrame(function() { anim.effect = effectB; }); Technically we should just get an animation changed notification since it's the same animation we're applying. We could probably accomplish that by calling nsNodeUtils::AnimationChanged(this) later on when we set newEffect (if newEffect is not null) -- I think that would mean that we end up evaluating true on this line: http://searchfox.org/mozilla-central/rev/336105a0de49f41a2cc203db7b7ee7266d0d558b/dom/base/nsDOMMutationObserver.cpp#1148

Comment on attachment 8771924 [details] Bug 1049975 - Part 5: Move timing related code into AnimationEffectReadOnly. https://reviewboard.mozilla.org/r/64852/#review63896 ::: dom/animation/test/chrome/test_animation_observers.html:1797 (Diff revision 1) > +addAsyncAnimTest("set_animation_effect", > + { observe: div, subtree: true }, function*() { This test is doing a little too much. Can we split it up into three test cases so make it easier to follow? And can we add a test for replacing the effect of an animation with another effect that targets the same element? (Should generate a change record, I think.) ::: dom/animation/test/chrome/test_animation_observers.html:1812 (Diff revision 1) > + assert_records([{ added: [], changed: [], removed: [anim1] }], > + "records after animation is removed"); > + > + anim1.effect = new KeyframeEffect(div, { opacity: [ 0, 1 ] }, > + 50 * MS_PER_SEC); > + anim1.play(); I think once we make the changes in comment 30 this last line won't be needed.

(In reply to Brian Birtles (:birtles) from comment #29) > I'm a bit unsure how this should work. I think our current > implementation differs from the spec but I'm not sure which is right. > > In our implementation, we only reject the ready promise if we're > pending. However, the spec doesn't say that. The spec appears to cancel > the promise every time. > > I guess if we do something like: > > // Assume animation is not currently pending > var ready1 = animation.ready; > animation.cancel(); > var ready2 = animation.ready; > > console.log(ready1 === ready2); > > I guess we'll get true in Firefox but the spec says it should be false. > > Can you check what Chrome does? > > I think what Firefox does is probably better, so if that's the case we > should update the spec. > > Cancelling review until we work out what Chrome does and (most likely) fix > the spec. I just checked what Chrome Canary does by your example, and Yes, Chrome returns _false_, and Firefox returns _true_. So should we update the spec so that Chrome can fix the behavior?

(In reply to Brian Birtles (:birtles) from comment #28) > More importantly, before landing this patch, we need to fix > nsTransitionManager to work when an effect that is not a transition effect > is used. Just search the code base for "1049975" and you'll see what I mean. I have a question about this. If the effect of a CSS Transition is set to null, what is the expected transition property got by getTransitionProperty? I think we should return 'none', right?

(In reply to Boris Chiou [:boris] from comment #37) > (In reply to Brian Birtles (:birtles) from comment #29) > > I guess if we do something like: > > > > // Assume animation is not currently pending > > var ready1 = animation.ready; > > animation.cancel(); > > var ready2 = animation.ready; > > > > console.log(ready1 === ready2); > > > > I guess we'll get true in Firefox but the spec says it should be false. > > > > Can you check what Chrome does? > > > > I think what Firefox does is probably better, so if that's the case we > > should update the spec. > > > > Cancelling review until we work out what Chrome does and (most likely) fix > > the spec. > > I just checked what Chrome Canary does by your example, and Yes, Chrome > returns _false_, and Firefox returns _true_. > > So should we update the spec so that Chrome can fix the behavior? Yes, I think so. I've sent the Chrome folks a message but have yet to hear back from them. We'll also need to add a test for this. (In reply to Boris Chiou [:boris] from comment #38) > (In reply to Brian Birtles (:birtles) from comment #28) > > More importantly, before landing this patch, we need to fix > > nsTransitionManager to work when an effect that is not a transition effect > > is used. Just search the code base for "1049975" and you'll see what I mean. > > I have a question about this. If the effect of a CSS Transition is set to > null, what is the expected transition property got by getTransitionProperty? > I think we should return 'none', right? The suggested behavior in [1] is that we actually store the original transition property on the CSSTransition so that even if its effect is removed or replaced we continue to return the same value. I *think* we need to do that so that we can continue to detect existing transitions even when the transition effect has been replaced (i.e. if we change the effect of a running transition it should continue to behave the same). Now I start to wonder how many other places in the code expect that transitions only animate a single property. [1] http://searchfox.org/mozilla-central/rev/cb74fc1327028e13bb1f66817da07ca09e4edcec/layout/style/nsTransitionManager.cpp#136

(In reply to Brian Birtles (:birtles) from comment #39) > > I have a question about this. If the effect of a CSS Transition is set to > > null, what is the expected transition property got by getTransitionProperty? > > I think we should return 'none', right? > > The suggested behavior in [1] is that we actually store the original > transition property on the CSSTransition so that even if its effect is > removed or replaced we continue to return the same value. I *think* we need > to do that so that we can continue to detect existing transitions even when > the transition effect has been replaced (i.e. if we change the effect of a > running transition it should continue to behave the same). > > Now I start to wonder how many other places in the code expect that > transitions only animate a single property. > > [1] > http://searchfox.org/mozilla-central/rev/ > cb74fc1327028e13bb1f66817da07ca09e4edcec/layout/style/nsTransitionManager. > cpp#136 About storing the values of the previous effect, I checked ConsiderStartingTransition() which uses |oldPT| (an ElementPropertyTransition pointer) as the effect of the current transition. Now it could be null or not an ElementPropertyTransition, so should we also store the data members of ElementPropertyTransition? Or just us "oldPT && oldPT->xxxxxx" to check the conditions [2] in ConsiderStartingTransition()? However, if we skip them by checking |oldPT|, it might have some weird behaviors because the current transition is still running. Or just partially skip them (e.g. only skip ReplacedTransition part [3]). We still can store all the values, but I'm not sure whether it is a good idea. Do you have any suggestion about this? Thanks. [2] http://searchfox.org/mozilla-central/rev/ff0e3782b1c44ffd1a68ef8e9e4f8be3866741e2/layout/style/nsTransitionManager.cpp#640-655,720,724 [3] http://searchfox.org/mozilla-central/rev/ff0e3782b1c44ffd1a68ef8e9e4f8be3866741e2/layout/style/nsTransitionManager.cpp#821-842

(In reply to Boris Chiou [:boris] from comment #40) > About storing the values of the previous effect, I checked > ConsiderStartingTransition() which uses |oldPT| (an > ElementPropertyTransition pointer) as the effect of the current transition. > Now it could be null or not an ElementPropertyTransition, so should we also > store the data members of ElementPropertyTransition? Or just us "oldPT && > oldPT->xxxxxx" to check the conditions [2] in ConsiderStartingTransition()? > However, if we skip them by checking |oldPT|, it might have some weird > behaviors because the current transition is still running. Or just partially > skip them (e.g. only skip ReplacedTransition part [3]). We still can store > all the values, but I'm not sure whether it is a good idea. Do you have any > suggestion about this? Thanks. And looks like we may skip some of them which are calculated by current time.

https://reviewboard.mozilla.org/r/64558/#review63508 > Is there any reason we don't just fix this in SetAnimation? > > In either case, this will be called when creating animations nsTransitionManager/nsAnimationManager. Is that ok? Do we need a SetEffectNoUpdate or something? 1. SetAnimation() calls NotifyAnimationTimingUpdate() which uses RequestRestyle() to update the style of the current element. If mAnimation is nullptr, we cannot get a correct cascade level. However, I think we can fix this. Just use |EffectCompositor::CascadeLevel::Animations| if mAnimation is null. I will try to remove PostUpdate(). 2. I think we will not call this when creating CSS Animations/Transitions because mEffect is initialized to null, and nsTransitionManager/nsAnimationManager set a valid effect to it. So it is the case: null effect -> a valid effect. Therefore, they skip this PostUpdate().

https://reviewboard.mozilla.org/r/64558/#review63508 > This puts us in a slightly odd situation where we can temporarily have: > > oldAnim -> newEffect > newAnim -> newEffect > newEffect -> oldAnim > > I'm a bit concerned that as we call things like PostUpdate we'll end up with obscure bugs. > > Would it be possible to do something like: > > 1. auto oldEffect = mEffect (i.e. hold an owning reference to it) > 2. mEffect = nullptr; > 3. oldEffect->SetAnimation(nullptr); > > 4. prevAnim->SetEffect(nullptr); > 5. mEffect = newEffect; > 6. mEffect->SetAnimation(this); > > (Not sure if we'll need to add an owning ref to newEffect in case prevAnim holds the last reference to it) > > Something like that might be more robust, particularly if this code becomes more complicated. > > What do you think? Yes, it is a weird situation. OK. Let's break all the connections between oldAnim and newEffect first, and then create the links between newAnim and newEffect. BTW, I will add an owning ref to newEffect because I'm not sure if the caller ensures the lifetime of |aEffect| is longer than the function call. > That sounds like the correct behavior to me. That playState should *not* become idle. > > For example, it should be possible to do: > > // Swap effects > var effectA = animA.effect; > animA.effect = animB.effect; > animB.effect = effectA; > > And, assuming both animations are playing, they should maintain their current time and continue playing. If animB becomes idle when its effect is stolen, it will lose its current time. > > In fact, we should probably add that as a test case. Yes, you are right. I will a test case for this.

(In reply to Brian Birtles (:birtles) from comment #39) > (In reply to Boris Chiou [:boris] from comment #37) > > (In reply to Brian Birtles (:birtles) from comment #29) > > > I guess if we do something like: > > > > > > // Assume animation is not currently pending > > > var ready1 = animation.ready; > > > animation.cancel(); > > > var ready2 = animation.ready; > > > > > > console.log(ready1 === ready2); > > > > > > I guess we'll get true in Firefox but the spec says it should be false. > > > > > > Can you check what Chrome does? > > > > > > I think what Firefox does is probably better, so if that's the case we > > > should update the spec. > > > > > > Cancelling review until we work out what Chrome does and (most likely) fix > > > the spec. > > > > I just checked what Chrome Canary does by your example, and Yes, Chrome > > returns _false_, and Firefox returns _true_. > > > > So should we update the spec so that Chrome can fix the behavior? > > Yes, I think so. I've sent the Chrome folks a message but have yet to hear > back from them. Chrome guys agree with this change so I've updated the spec.[1] [1] https://github.com/w3c/web-animations/commit/62c9694450f9fb3200b3188d1e03fd93a4944d99 > We'll also need to add a test for this. We still need to add this test, however. (In reply to Boris Chiou [:boris] from comment #40) > About storing the values of the previous effect, I checked > ConsiderStartingTransition() which uses |oldPT| (an > ElementPropertyTransition pointer) as the effect of the current transition. > Now it could be null or not an ElementPropertyTransition, so should we also > store the data members of ElementPropertyTransition? Or just us "oldPT && > oldPT->xxxxxx" to check the conditions [2] in ConsiderStartingTransition()? > However, if we skip them by checking |oldPT|, it might have some weird > behaviors because the current transition is still running. Or just partially > skip them (e.g. only skip ReplacedTransition part [3]). We still can store > all the values, but I'm not sure whether it is a good idea. Do you have any > suggestion about this? Thanks. > > [2] > http://searchfox.org/mozilla-central/rev/ > ff0e3782b1c44ffd1a68ef8e9e4f8be3866741e2/layout/style/nsTransitionManager. > cpp#640-655,720,724 > [3] > http://searchfox.org/mozilla-central/rev/ > ff0e3782b1c44ffd1a68ef8e9e4f8be3866741e2/layout/style/nsTransitionManager. > cpp#821-842 These are good questions. This behavior is not yet specified so we need to work out what is sensible. The main use case is polyfilling transitions to do something different. For example, suppose I want to create a new type of color transition that instead of animating directly from red -> green, animates red -> orange -> green. In future I guess I will write something like: // transitionstart is defined in CSS transitions level 2 elem.addEventListener('transitionstart', () => { // The getAnimations filter parameters are yet to be specced const colorTransitions = elem.getAnimations({ transitionProperty: 'width' }); if (colorTransitions.length != 1) { return; } const colorTranition = colorTransitions[0]; // Make effect mutable colorTransition.effect = new KeyframeEffect(colorTransition.effect); effect.setKeyframes({ color: [ 'red', 'orange', 'green' ] }); }); // Trigger transition elem.style.transition = 'color 2s ease-in'; getComputedStyle(elem).color; elem.style.color = 'green'; Now, the important thing at this point is that if we do: elem.style.transitionProperty = 'none'; The modified transition should be cancelled. So far, I think this is easy. The question is, what should happen if we do: elem.style.color = 'red'; Normally we would detect that we are reversing an animation and calculate how far we are into the original transition and use that to shorten the new transition. I *think* we can still support that. The information we need from oldPT is basically: a. Its endpoint value (so we can skip creating redundant transitions) b. Its effective start value (mStartForReversingTest) c. Its CurrentValuePortion -- this is purely based on timing so we don't need to store anything. *However* currently we store the timing function as on the first keyframe. It seems like it would be better to store the timing function as an effect-level timing function since it is less likely to be overwritten then. d. Its reverse portion As for the replaced transition behavior -- I think we can skip that if the effect is not a ElementPropertyTransition. It's just a performance optimization that we can say simply doesn't apply once you replace the effect. So, to make this work we'd basically need to: * Move mStartForReversingTest, mReversePortion from ElementPropertyTransition to CSSTransition * Add mTransitionProperty to CSSTransition * Probably add mTransitionToValue to CSSTransition (We might be able to be clever about this and, for example, only assign it when the effect is replaced with something that that is not an ElementPropertyTransition. Or we could even use mTransitionProperty to try to look up the end-point in the new set of keyframes--I'm not sure if that would always work, however.) * Set the timing function on transitions as an effect-level easing (rather than keyframe-level easing) That's quite a big change, its unspecified, and it needs a fair bit of testing so I think we should do that as a separate bug. In this bug we simply need to make sure we don't crash when the effect attached to a CSSTransition is not an ElementPropertyTransition. What do you think?

(In reply to Boris Chiou [:boris] from comment #42) > https://reviewboard.mozilla.org/r/64558/#review63508 > > > Is there any reason we don't just fix this in SetAnimation? > > > > In either case, this will be called when creating animations nsTransitionManager/nsAnimationManager. Is that ok? Do we need a SetEffectNoUpdate or something? > > 1. SetAnimation() calls NotifyAnimationTimingUpdate() which uses > RequestRestyle() to update the style of the current element. If mAnimation > is nullptr, we cannot get a correct cascade level. However, I think we can > fix this. Just use |EffectCompositor::CascadeLevel::Animations| if > mAnimation is null. I will try to remove PostUpdate(). Oh, I see. I'd rather not fill in the cascade level automatically like that. I guess we could have a situation where an effect is taken from the transition and applied to an animation? e.g. var transition = elem.getAnimations({ transitionProperty: 'width' })[0]; var anim = new Animation(); anim.effect = transition.effect; anim.play(); In that case I guess we really need to make sure that both the transitions-level rule and the animations-level rule get updated? So maybe we need to do something like: 1. If mAnimation == aAnimation return 2. Call RequestRestyle(EffectCompositor::RestyleType::Layer) using the existing mAnimation. 3. mAnimation = aAnimation. 4. Call RequestRestyle(EffectCompositor::RestyleType::Layer) using the new mAnimation. 5. Call MarkCascadeNeedsUpdate() since we have a new animation which might have a different priority or even target a different level of the cascade Would that work? > 2. I think we will not call this when creating CSS Animations/Transitions > because mEffect is initialized to null, and > nsTransitionManager/nsAnimationManager set a valid effect to it. So it is > the case: null effect -> a valid effect. Therefore, they skip this > PostUpdate(). On the second call to SetAnimation() we will end up requesting a restyle, won't we? Maybe we need to add a flag to EffectCompositor to say "processing restyles" so and skip calling PostRestyle when that is true? Then set the flag when we are processing restyles inside nsTransitionManager / nsAnimationManager. Would that work? It's a bit of a scary change but if it works we could remove a few of those "NoUpdate" methods. Perhaps we can do something simpler in this bug?

(In reply to Brian Birtles (:birtles) from comment #44) > * Move mStartForReversingTest, mReversePortion from > ElementPropertyTransition to > CSSTransition > * Add mTransitionProperty to CSSTransition > * Probably add mTransitionToValue to CSSTransition > (We might be able to be clever about this and, for example, only assign it > when the effect is replaced with something that that is not an > ElementPropertyTransition. Or we could even use mTransitionProperty to try > to > look up the end-point in the new set of keyframes--I'm not sure if that > would > always work, however.) > * Set the timing function on transitions as an effect-level easing (rather > than > keyframe-level easing) > > That's quite a big change, its unspecified, and it needs a fair bit of > testing > so I think we should do that as a separate bug. > > In this bug we simply need to make sure we don't crash when the effect > attached > to a CSSTransition is not an ElementPropertyTransition. > > What do you think? OK. It makes sense. Currently, I will skip the oldPT in this bug and only store mTransitionProperty and mTransitionToValue, so TransitionProperty() and TransitionToValue() still work. Let's fix this in another bug.

(In reply to Brian Birtles (:birtles) from comment #45) > So maybe we need to do something like: > > 1. If mAnimation == aAnimation return > 2. Call RequestRestyle(EffectCompositor::RestyleType::Layer) using the > existing > mAnimation. > 3. mAnimation = aAnimation. > 4. Call RequestRestyle(EffectCompositor::RestyleType::Layer) using the new > mAnimation. > 5. Call MarkCascadeNeedsUpdate() since we have a new animation which might > have > a different priority or even target a different level of the cascade > > Would that work? Yes. That work, and looks better. This make sure we update a layer after calling SetAnimation(). > On the second call to SetAnimation() we will end up requesting a restyle, > won't we? Oh, if the playState is idle, the second call to SetAnimation() doesn't request a restyle because NotifyAnimationTimingUpdate() checks InEffect first during creating CSS Animation/Transition. However, your suggestion above would request a restyle and force a layer update after the second call to SetAnimation(). Is that what we want? > Maybe we need to add a flag to EffectCompositor to say "processing restyles" > so > and skip calling PostRestyle when that is true? Then set the flag when we are > processing restyles inside nsTransitionManager / nsAnimationManager. Would > that > work? It's a bit of a scary change but if it works we could remove a few of > those "NoUpdate" methods. Perhaps we can do something simpler in this bug? OK. I could try to add this flag, i.e. PostUpdate(bool aProcessRestyle).

(In reply to Boris Chiou [:boris] from comment #47) > > Maybe we need to add a flag to EffectCompositor to say "processing restyles" > > so > > and skip calling PostRestyle when that is true? Then set the flag when we are > > processing restyles inside nsTransitionManager / nsAnimationManager. Would > > that > > work? It's a bit of a scary change but if it works we could remove a few of > > those "NoUpdate" methods. Perhaps we can do something simpler in this bug? > > OK. I could try to add this flag, i.e. PostUpdate(bool aProcessRestyle). Oh. Sorry, this flag shouldn't be added to the functions in Animation. I should add a flag in EffectCompositor, and after setting the flag, we skip all the restyle requests in EffectCompositor. Maybe we could also add a special class, e.g. EffectCompositor::AutoSkipRestyle, and it only lives in the local variable scope. Therefore, we don't have to reset it manually.

https://reviewboard.mozilla.org/r/64558/#review63508 > Does this actually work? i.e. is there a test case we can write that fails if we don't do this? > > Perhaps we need to check if mPendingReadyTime is set, and, if it is, clear it and re-add ourselves to the tracker? Actually, I tried to not reschedule it before. If the test case is: var anim = new Animation(effectA, document); anim.play(); // now, it has a pending play task. anim.effect = effectB; // assign another effect Because this animation is already in the list of pendingAnimationTracker, after chaning its effect, it is still in the list, so it runs after gettting ready. BTW, in this example, when we call SetEffect(), mPendingReadyTime is alway null. Therefore, as your suggestion, I just have to check if mPendingReadyTime is null, instead of only checking mPendingState, because we only need to rechedule this animation when mPendingReadTime is not null, right? I might remove this part because it looks like we don't really need to reschedule it.

https://reviewboard.mozilla.org/r/64560/#review63888 > Although this seems like a useful test, I don't think it matches anything in the spec. We should try to test just the behavior described in: > > https://w3c.github.io/web-animations/#setting-the-target-effect > > This kind of test probably belongs in dom/animation/test/style > > In fact, as some general feedback, I think it would be ideal if we structured these tests around the steps in that procedure[1]. If that's easy to do then that's great, otherwise don't feel like you need to completely rewrite these. > > [1] i.e. https://w3c.github.io/web-animations/#setting-the-target-effect Thanks. I will follow the items in the spec, and put the test cases, e.g. swap effects, into dom/animation/tests/style > Does this actually fail if we don't reschedule the task? > > It might be more useful to do something like: > > 1. Play > 2. Wait an animation frame > (At this point the animation *may* still be pending. In Firefox is sometimes is still pending, in Chrome is appears to be always still pending) > 3. Set the effect > 4. Check that it is still pending and, if it was previously pending, that the ready promise is re-used (i.e. same object). > > Would that work? I tried this idea, and my steps are: 1. Comment out the reschedule part (i.e. we don't reschedule pending task now) 2. The test case is like this: promise_test(function(t) { var anim = createDiv(t).animate({ marginLeft: [ '0px', '100px' ] }, 100 * MS_PER_SEC); assert_equals(anim.playState, 'pending'); return waitForAnimationFrames(1).then(function() { assert_equals(anim.playState, 'pending'); var ready1 = anim.ready; anim.effect = new KeyframeEffectReadOnly(createDiv(t), { marginLeft: [ '0px', '100px' ] }, 100 * MS_PER_SEC); assert_equals(anim.playState, 'pending'); var ready2 = anim.ready; assert_equals(ready1, ready2); }); }, '...'); The result is: it still works. Note: I have to create an animation with an effect, instead of null effect. Otherwise, after waitForAnimationFrame(1), the playState becomes 'finished'. Therefore, it looks like if we don't reschedule pending tasks, the result is the same. > As mentioned in comment 30, I don't think this is the correct behavior. I think we should be doing what the spec does here--i.e. prevAnim should end up being finished and maintain its currentTime. Thanks for your review. Actually, I found this test case may be wrong because I didn't wait for prevAnim's ready promise. if prevAnim isn't ready, and then we set its effect to another animation, i.e. anim, its playState becomes _paused_. However, if we set the effect after prevAnim is ready, its playState becomes _finished_.

(In reply to Brian Birtles (:birtles) from comment #39) > I *think* we need > to do that so that we can continue to detect existing transitions even when > the transition effect has been replaced (i.e. if we change the effect of a > running transition it should continue to behave the same). I am writing the test cases for CSS Transition, so I'd like to double check its behavior after setting a new effect: 1. When the transition is running a) Set null effect: The transition property should always return the original property, and as comment 39 said, it should continue to behave the same (still running), so I have to write a special version of SetEffect() for CSSTransition because our current Animation::SetEffect(), revised according to comment 30, will jump to the end value directly after setting null effect. Or do I misunderstand your meaning? The expected behavior is that transition doesn't have to continue to run, instead. (i.e. jump to the end value) b) Set a different effect which is not an ElementPropertyTransition: It's a invalid effect for this effect, so the behavior is like null effect. c) Set a different ElementPropertyTransition effect: According to comment 44, it looks like we apply the new effect to this transition, just like what we do for Animation/CSS Animation. 2. When the transition is not running a) Set null effect: We still return the original property by Animation.TransitionProperty. However, this transition will finish immediately after it starts. b) Set a different effect which is not an ElementPropertyTransition: The behavior is like null effect. c) Set a different ElementPropertyTransition effect: Apply the new effect. Do these make sense?

(In reply to Boris Chiou [:boris] from comment #51) > (In reply to Brian Birtles (:birtles) from comment #39) > > I *think* we need > > to do that so that we can continue to detect existing transitions even when > > the transition effect has been replaced (i.e. if we change the effect of a > > running transition it should continue to behave the same). > > I am writing the test cases for CSS Transition, so I'd like to double check > its behavior after setting a new effect: > > 1. When the transition is running > a) Set null effect: > The transition property should always return the original property, and as > comment 39 said, it should continue to behave the same (still running), so I > have to write a special version of SetEffect() for CSSTransition because our > current Animation::SetEffect(), revised according to comment 30, will jump > to the end value directly after setting null effect. Or do I misunderstand > your meaning? The expected behavior is that transition doesn't have to > continue to run, instead. (i.e. jump to the end value) And If we want to keep the transition running as the original one, we have to do something special for CSSTransition::ComposeStyle(), which uses mEffect->ComposeStyle() to calculate the interpolation.

(In reply to Boris Chiou [:boris] from comment #50) > > Does this actually fail if we don't reschedule the task? > > > > It might be more useful to do something like: > > > > 1. Play > > 2. Wait an animation frame > > (At this point the animation *may* still be pending. In Firefox is sometimes is still pending, in Chrome is appears to be always still pending) > > 3. Set the effect > > 4. Check that it is still pending and, if it was previously pending, that the ready promise is re-used (i.e. same object). > > > > Would that work? > > I tried this idea, and my steps are: > 1. Comment out the reschedule part (i.e. we don't reschedule pending task > now) > 2. The test case is like this: > > promise_test(function(t) { > var anim = createDiv(t).animate({ marginLeft: [ '0px', '100px' ] }, > 100 * MS_PER_SEC); > assert_equals(anim.playState, 'pending'); > > return waitForAnimationFrames(1).then(function() { > assert_equals(anim.playState, 'pending'); > var ready1 = anim.ready; > anim.effect = new KeyframeEffectReadOnly(createDiv(t), > { marginLeft: [ '0px', '100px' > ] }, > 100 * MS_PER_SEC); > assert_equals(anim.playState, 'pending'); > var ready2 = anim.ready; > assert_equals(ready1, ready2); > }); > }, '...'); > > The result is: it still works. I guess that makes sense. I suppose the difference in behavior is that if we *don't* reschedule the pending task, then on the *next* animation frame the animation will probably not be still pending, but if we *do* reschedule the pending task, on the next frame the animation probably will be still pending. That's really hard to test and ultimately implementation-specific so perhaps we only need to test that the ready promise is not replaced.

(In reply to Boris Chiou [:boris] from comment #51) > (In reply to Brian Birtles (:birtles) from comment #39) > > I *think* we need > > to do that so that we can continue to detect existing transitions even when > > the transition effect has been replaced (i.e. if we change the effect of a > > running transition it should continue to behave the same). > > I am writing the test cases for CSS Transition, so I'd like to double check > its behavior after setting a new effect: > > 1. When the transition is running > a) Set null effect: > The transition property should always return the original property, and as > comment 39 said, it should continue to behave the same (still running), so I > have to write a special version of SetEffect() for CSSTransition because our > current Animation::SetEffect(), revised according to comment 30, will jump > to the end value directly after setting null effect. Or do I misunderstand > your meaning? The expected behavior is that transition doesn't have to > continue to run, instead. (i.e. jump to the end value) Oh, that's a really good question. I suppose the animation transition should become finished. > b) Set a different effect which is not an ElementPropertyTransition: > It's a invalid effect for this effect, so the behavior is like null effect. No, it should be possible to set a different kind of effect. Comment 44 uses a KeyframeEffect (not an ElementPropertyTransition). > c) Set a different ElementPropertyTransition effect: > According to comment 44, it looks like we apply the new effect to this > transition, just like what we do for Animation/CSS Animation. It should behave the same as (b). > 2. When the transition is not running The state of the transition shouldn't make any difference. (In reply to Boris Chiou [:boris] from comment #52) > And If we want to keep the transition running as the original one, we have > to do something special for CSSTransition::ComposeStyle(), which uses > mEffect->ComposeStyle() to calculate the interpolation. I'm not sure we need to keep it running if the new effect is shorter / null.

(In reply to Brian Birtles (:birtles) from comment #53) > I guess that makes sense. I suppose the difference in behavior is that if we > *don't* reschedule the pending task, then on the *next* animation frame the > animation will probably not be still pending, but if we *do* reschedule the > pending task, on the next frame the animation probably will be still pending. > > That's really hard to test and ultimately implementation-specific so perhaps > we only need to test that the ready promise is not replaced. OK. I will keep the reschedule code.

(In reply to Brian Birtles (:birtles) from comment #54) > > 1. When the transition is running > ... OK. Thanks. And I think the transitionProperty always returns the original transition property. > > 2. When the transition is not running > > The state of the transition shouldn't make any difference. Does this mean that SetEffect() doesn't work if the transition is not running?

(In reply to Boris Chiou [:boris] from comment #56) > (In reply to Brian Birtles (:birtles) from comment #54) > > > 1. When the transition is running > > ... > > OK. Thanks. And I think the transitionProperty always returns the original > transition property. > > > > 2. When the transition is not running > > > > The state of the transition shouldn't make any difference. > > Does this mean that SetEffect() doesn't work if the transition is not > running? Oh. I guess it should be like the normal SetEffect(). Just like our first call to SetEffect in nsTransitionManager.

Try to merge these methods: 1. SetTimeline with SetTimelineNoUpdate 2. Play with playNoUpdate 3. Pause with PauseNoUpdate 4. Cancel with CancelNoUpdate Review commit: https://reviewboard.mozilla.org/r/68844/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/68844/

Comment on attachment 8771332 [details] Bug 1049975 - Part 3: Handle removed/replaced effect for CSS Transition. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63664/diff/1-2/

Comment on attachment 8771924 [details] Bug 1049975 - Part 5: Move timing related code into AnimationEffectReadOnly. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64852/diff/1-2/

Hi, Brian, The review statuses in mozreview are not correct, and It seems I cannot reset them. Brian, could you please take a look at part 3 ~ part 7? I think you can override their original statuses. Thanks.

Comment on attachment 8771332 [details] Bug 1049975 - Part 3: Handle removed/replaced effect for CSS Transition. https://reviewboard.mozilla.org/r/63664/#review66240 ::: layout/style/nsTransitionManager.h:255 (Diff revision 2) > // When true, indicates that when this transition next leaves the idle state, > // its animation index should be updated. > bool mNeedsNewAnimationIndexWhenRun; > + > + nsCSSProperty mTransitionProperty; > + StyleAnimationValue mTransitionToValue; I was thinking we should use Maybe<StyleAnimationValue> and only store the to-value if the effect is replaced (since sometimes StyleAnimationValue objects can be heavy weight) but maybe it's not worth the extra complexity? We should at least add a comment here explaining these members: // Store the transition property and to-value here since we need that // information in order to determine if there is an existing transition // for a given style change. We can't store that information on the // ElementPropertyTransition (effect) however since it can be replaced // using the Web Animations API. ::: layout/style/nsTransitionManager.cpp:724 (Diff revision 2) > > // If the new transition reverses an existing one, we'll need to > // handle the timing differently. > if (haveCurrentTransition && > aElementTransitions->mAnimations[currentIndex]->HasCurrentEffect() && > - oldPT->mStartForReversingTest == endValue) { > + oldPT && oldPT->mStartForReversingTest == endValue) { Nit: All the other conditions in this if block have their own line so it might be easier to read if we add a line break after 'oldPtr &&' i.e. oldPT && oldPT->mStartForReversingTest (Also, just to check, is the plan still to move mStartForReversingTest to CSSTransition as per comment 44 -- but to do that in another bug?)

https://reviewboard.mozilla.org/r/63664/#review66244 I added a comment to the previous review to say "Nice work" but MozReview dropped it. Trying again.

https://reviewboard.mozilla.org/r/63664/#review66240 > I was thinking we should use Maybe<StyleAnimationValue> and only store the to-value if the effect is replaced (since sometimes StyleAnimationValue objects can be heavy weight) but maybe it's not worth the extra complexity? > > We should at least add a comment here explaining these members: > > // Store the transition property and to-value here since we need that > // information in order to determine if there is an existing transition > // for a given style change. We can't store that information on the > // ElementPropertyTransition (effect) however since it can be replaced > // using the Web Animations API. A little bit. Storing to-value during the initializaion makes the code much simpler, so I don't have to do many checks in CSSTransition::SetEffect(). > Nit: All the other conditions in this if block have their own line so it might be easier to read if we add a line break after 'oldPtr &&' > > i.e. > > oldPT && > oldPT->mStartForReversingTest > > (Also, just to check, is the plan still to move mStartForReversingTest to CSSTransition as per comment 44 -- but to do that in another bug?) My plan is to do all the things mentioned in comment 44 in another bug, so I didn't move mStartForReversingTest, mReversePortion in this bug because it looks like we need mStartForReversingTest, mReversePortion, and the timing function together to calculate the time difference for reversing the exist transition [1]. I can file a bug now and add the bug number in nsTransitionManager. [1] http://searchfox.org/mozilla-central/rev/740bb4ed16d070cf5d466231b30e80b9204aec54/layout/style/nsTransitionManager.cpp#718-725

(In reply to Boris Chiou [:boris] from comment #75) > https://reviewboard.mozilla.org/r/63664/#review66240 > > > I was thinking we should use Maybe<StyleAnimationValue> and only store the to-value if the effect is replaced (since sometimes StyleAnimationValue objects can be heavy weight) but maybe it's not worth the extra complexity? ... > A little bit. Storing to-value during the initializaion makes the code much > simpler, so I don't have to do many checks in CSSTransition::SetEffect(). Ok, that sounds fine as-is then.

Comment on attachment 8771334 [details] Bug 1049975 - Part 4: Merge two Animation::SetEffect()s. https://reviewboard.mozilla.org/r/63668/#review66246 Thanks for this. This is looking good but going back to comment 28, I think in this bug I either want to: i. Reduce the amount of virtual method calls for we introduce by using something like AsKeyframeEffect() and then calling non-virtual methods on KeyframeEffectReadOnly for methods that only apply to keyframe effects (the other methods should be mostly timing-related methods that are implemented in AnimationEffectReadOnly as of part 5 and so are also non-virtual). ii. Or, just give up and leave everything on KeyframeEffectReadOnly in this bug and fix things in another bug (option (b) from comment 28). I think this patch is nearly there so we should probably try and get (i) to work. ::: dom/animation/Animation.cpp:520 (Diff revision 3) > mPendingReadyTime.SetNull(); > } > > if (IsPossiblyOrphanedPendingAnimation()) { > MOZ_ASSERT(mTimeline && !mTimeline->GetCurrentTime().IsNull(), > - "Orphaned pending animtaions should have an active timeline"); > + "Orphaned pending animations should have an active timeline"); Thank you! ::: dom/animation/AnimationEffectReadOnly.h:65 (Diff revision 3) > + virtual Maybe<NonOwningAnimationTarget> GetTarget() const = 0; > + virtual nsIDocument* GetRenderedDocument() const = 0; > + virtual nsPresContext* GetPresContext() const = 0; > + > + virtual void SetKeyframes(nsTArray<Keyframe>&& aKeyframes, > + nsStyleContext* aStyleContext) = 0; Some of these don't belong in AnimationEffectReadOnly. For example, SetKeyframes clearly belongs on KeyframeEffectReadOnly only. Also, we want to avoiding adding too many virtual function calls. I know that in the next patch in this series we remove a number of virtual function calls, but these ones remain. I think we should add AsKeyframeEffectReadOnly() like we have AsTransition(). (Should we call it just AsKeyframeEffect?) Then, we can leave some of these methods on KeyframeEffectReadOnly. For example, suppose we leave the following four methods on KeyframeEffectReadOnly: * GetTarget * GetRenderedDocument * GetPresContext * SetKeyframes Then, in Animation::PostUpdate() we could write something like: if (!mEffect) { return; } KeyframeEffectReadOnly* keyframeEffect = mEffect->AsKeyframeEffect(); if (!keyframeEffect) { return; } Maybe<NonOwningAnimationTarget> target = keyframeEffect->GetTarget(); if (!target) { return; } nsPresContext* presContext = keyframeEffect->GetPresContext(); if (!presContext) { return; } ... And drop Animation::GetPresContext(). That would mean we'd only have one virtual method call. I haven't worked through all the methods yet but it seems like everything related to having a target element or keyframes should only be on KeyframeEffectReadOnly. This comes back to comment 28: > (a) is better since it matches the spec better and separates out the code > for timing vs keyframes. However, we might discover it adds too many > virtual methods and leads to other inefficiencies. It might be ok to do > (b) here and try (a) in another bug (which we can drop / backout if it is > inefficient). i.e. having seen how this looks I think we need to do a combination of (a) and (b). ::: dom/animation/AnimationEffectReadOnly.h:72 (Diff revision 3) > + virtual const AnimationProperty* GetAnimationOfProperty( > + nsCSSProperty aProperty) const = 0; > + virtual const InfallibleTArray<AnimationProperty>& Properties() const = 0; > + virtual InfallibleTArray<AnimationProperty>& Properties() = 0; > + > + // Updates |aStyleRule| with the animation values produced by this > + // AnimationEffect for the current time except any properties already > + // contained in |aSetProperties|. > + // Any updated properties are added to |aSetProperties|. > + virtual void ComposeStyle(RefPtr<AnimValuesStyleRule>& aStyleRule, > + nsCSSPropertySet& aSetProperties) = 0; > + > + // Returns true if at least one property is being animated on compositor. > + virtual bool IsRunningOnCompositor() const = 0; > + virtual void SetIsRunningOnCompositor(nsCSSProperty aProperty, > + bool aIsRunning) = 0; I think all of these should probably be on KeyframeEffectReadOnly--however we might discover that for some of them it's more convenient to simply keep the virtual method (since we only ever call the method in isolation and so we don't save any virtual method calls by getting the KeyframeEffectReadOnly first).

https://reviewboard.mozilla.org/r/63668/#review66246 > Some of these don't belong in AnimationEffectReadOnly. For example, SetKeyframes clearly belongs on KeyframeEffectReadOnly only. > > Also, we want to avoiding adding too many virtual function calls. I know that in the next patch in this series we remove a number of virtual function calls, but these ones remain. > > I think we should add AsKeyframeEffectReadOnly() like we have AsTransition(). (Should we call it just AsKeyframeEffect?) > > Then, we can leave some of these methods on KeyframeEffectReadOnly. > > For example, suppose we leave the following four methods on KeyframeEffectReadOnly: > > * GetTarget > * GetRenderedDocument > * GetPresContext > * SetKeyframes > > Then, in Animation::PostUpdate() we could write something like: > > if (!mEffect) { > return; > } > > KeyframeEffectReadOnly* keyframeEffect = mEffect->AsKeyframeEffect(); > if (!keyframeEffect) { > return; > } > > Maybe<NonOwningAnimationTarget> target = keyframeEffect->GetTarget(); > if (!target) { > return; > } > > nsPresContext* presContext = keyframeEffect->GetPresContext(); > if (!presContext) { > return; > } > > ... > > And drop Animation::GetPresContext(). That would mean we'd only have one virtual method call. > > I haven't worked through all the methods yet but it seems like everything related to having a target element or keyframes should only be on KeyframeEffectReadOnly. > > This comes back to comment 28: > > > (a) is better since it matches the spec better and separates out the code > > for timing vs keyframes. However, we might discover it adds too many > > virtual methods and leads to other inefficiencies. It might be ok to do > > (b) here and try (a) in another bug (which we can drop / backout if it is > > inefficient). > > i.e. having seen how this looks I think we need to do a combination of (a) and (b). Thanks for your suggestion. I agree. Combining (a) and (b) is much better. For those methods not belonging to AnimationEffect, we use AsKeyframeEffect(), and others, e.g. NotifyAnimationTimingUpdate(), we still use virtual method. > I think all of these should probably be on KeyframeEffectReadOnly--however we might discover that for some of them it's more convenient to simply keep the virtual method (since we only ever call the method in isolation and so we don't save any virtual method calls by getting the KeyframeEffectReadOnly first). OK. I will move them back to KeyframeEffectReadOnly(). Thanks.

Part 7 makes some tests failed in dom/animation/tests/css-animation/test_animation-ready.html, so I remove the review request and will update it soon.

Comment on attachment 8771924 [details] Bug 1049975 - Part 5: Move timing related code into AnimationEffectReadOnly. https://reviewboard.mozilla.org/r/64852/#review66248 This looks mostly fine but I&#39;m clearing the review request for now since I think it will change based on what happens in part 4? ::: dom/animation/AnimationEffectReadOnly.h:85 (Diff revision 2) > + void GetComputedTimingAsDict( > + ComputedTimingProperties& aRetVal) const; Nit: Does this fit on one line now? ::: dom/animation/AnimationEffectReadOnly.cpp:16 (Diff revision 2) > +NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(AnimationEffectReadOnly) > +if (tmp->mTiming) { > + tmp->mTiming->Unlink(); > +} > +NS_IMPL_CYCLE_COLLECTION_UNLINK(mDocument, mTiming, mAnimation) > +NS_IMPL_CYCLE_COLLECTION_UNLINK_PRESERVED_WRAPPER > +NS_IMPL_CYCLE_COLLECTION_UNLINK_END The lines between ...UNLINK_BEGIN and ...UNLINK_END should be indented ::: dom/animation/AnimationEffectReadOnly.cpp:24 (Diff revision 2) > +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(AnimationEffectReadOnly) > +NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mDocument, mTiming, mAnimation) > +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS > +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END Likewise between ...TRAVERSE_BEGIN and ...TRAVERSE_END ::: dom/animation/AnimationEffectReadOnly.cpp:34 (Diff revision 2) > NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(AnimationEffectReadOnly) > - NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY > +NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY > - NS_INTERFACE_MAP_ENTRY(nsISupports) > +NS_INTERFACE_MAP_ENTRY(nsISupports) > NS_INTERFACE_MAP_END We should leave this indentation as-is. ::: dom/animation/KeyframeEffect.h:300 (Diff revision 2) > KeyframeEffectReadOnly(nsIDocument* aDocument, > const Maybe<OwningAnimationTarget>& aTarget, > AnimationEffectTimingReadOnly* aTiming, > const KeyframeEffectParams& aOptions); > > - virtual ~KeyframeEffectReadOnly(); > + ~KeyframeEffectReadOnly() override = default; I wonder, is this actually needed? I think we can drop it, right?

Comment on attachment 8771335 [details] Bug 1049975 - Part 6: Factor out the procedure of resetting an animation's pending tasks. https://reviewboard.mozilla.org/r/64556/#review66294 ::: dom/animation/Animation.h:374 (Diff revision 3) > + * Performs the same steps as CancelPendingTasks and also rejects and > + * recreates the ready promise. ... recreates the ready promise if the animation was pending. ::: dom/animation/Animation.h:377 (Diff revision 3) > > + /** > + * Performs the same steps as CancelPendingTasks and also rejects and > + * recreates the ready promise. > + */ > + void ResetPendingTask(); Let's call this ResetPendingTasks to match the spec and the CancelPendingTasks above. ::: dom/animation/Animation.cpp:1154 (Diff revision 3) > +void > +Animation::ResetPendingTask() Perhaps add above this: // https://w3c.github.io/web-animations/#reset-an-animations-pending-tasks

https://reviewboard.mozilla.org/r/64852/#review66248 > The lines between ...UNLINK_BEGIN and ...UNLINK_END should be indented Oh. yes. I copied them but forget to double check the indention. Thanks.

https://reviewboard.mozilla.org/r/64852/#review66248 > I wonder, is this actually needed? I think we can drop it, right? We still need this, or the compilar would say: error: static_assert failed "Reference-counted class KeyframeEffectReadOnly should not have a public destructor. Make this class's destructor non-public"

Comment on attachment 8771334 [details] Bug 1049975 - Part 4: Merge two Animation::SetEffect()s. https://reviewboard.mozilla.org/r/63668/#review67024 ::: dom/animation/Animation.cpp:1123 (Diff revision 4) > nsPresContext* presContext = GetPresContext(); > if (!presContext) { > return; > } > > if (!mEffect) { > return; > } > > - Maybe<NonOwningAnimationTarget> target = mEffect->GetTarget(); > + KeyframeEffectReadOnly* keyframeEffect = mEffect->AsKeyframeEffect(); > + if (!keyframeEffect) { > + return; > + } > + > + Maybe<NonOwningAnimationTarget> target = keyframeEffect->GetTarget(); > if (!target) { > return; > } As per comment 77 I think we can drop Animation::GetPresContext() and rewrite this more efficiently as: if (!mEffect) { return; } KeyframeEffectReadOnly* keyframeEffect = mEffect->AsKeyframeEffect(); if (!keyframeEffect) { return; } Maybe<NonOwningAnimationTarget> target = keyframeEffect->GetTarget(); if (!target) { return; } nsPresContext* presContext = keyframeEffect->GetPresContext(); if (!presContext) { return; } ::: dom/animation/Animation.cpp:1246 (Diff revision 4) > nsPresContext* > Animation::GetPresContext() const > { > - if (!mEffect) { > + if (!mEffect || !mEffect->AsKeyframeEffect()) { > return nullptr; > } > > - return mEffect->GetPresContext(); > + return mEffect->AsKeyframeEffect()->GetPresContext(); > } > I think we can drop this method if we make the changes above. ::: dom/animation/AnimationEffectReadOnly.h:56 (Diff revision 4) > + // Shortcut for that gets the computed timing using the current local time as > + // calculated from the timeline time. Nit: While we're moving this can we fix: s/Shortcut for that/Shortcut that/

Comment on attachment 8777258 [details] Bug 1049975 - Part 7: Add SetEffectNoUpdate. https://reviewboard.mozilla.org/r/68844/#review67036 This is a pretty risky change but it looks like it might work. I'd like to have another look after making the following changes. ::: dom/animation/Animation.cpp:98 (Diff revision 2) > { > nsCOMPtr<nsIGlobalObject> global = do_QueryInterface(aGlobal.GetAsSupports()); > RefPtr<Animation> animation = new Animation(global); > > AnimationTimeline* timeline; > + nsIDocument* doc = AnimationUtils::GetCurrentRealmDocument(aGlobal.Context()); Rather than getting the current realm document (which may or may not be correct--and I suspect it's not), we should just grab the pres context off aEffect. That's what PostUpdate does and we want to be sure we're using the same pres context as it. We can probably use Maybe<> and emplace to handle the case when aEffect is null. ::: dom/animation/EffectCompositor.h:218 (Diff revision 2) > static void SetPerformanceWarning( > const nsIFrame* aFrame, > nsCSSProperty aProperty, > const AnimationPerformanceWarning& aWarning); > > + // A wrapper for automatically seting/reseting mSkipProcessRestyle, s/seting/setting/ s/reseting/resetting/ Perhaps something like: // RAII wrapper to automatically setting and restoring the // mIgnoreRequestRestyle flag. We use this when we are already processing // restyles (such as when rebuidling CSS animations) or in other situations // where we know a restyle is not required. We should also add a comment saying that this should only be used when |aPresContext| is guaranteed to outlive the scope of this object. ::: dom/animation/EffectCompositor.h:221 (Diff revision 2) > const AnimationPerformanceWarning& aWarning); > > + // A wrapper for automatically seting/reseting mSkipProcessRestyle, > + // especially for CSS Animation and CSS Transition, which don't want to > + // request a restyle during the creation. > + class AutoSkipProcessRestyle { AutoIgnoreRequestRestyle? Also, we should declare this MOZ_RAII and use the appropriate guard object macros. (See mfbt/GuardObjects.h and examples such as AutoMutationBatchForAnimation). ::: dom/animation/EffectCompositor.h:227 (Diff revision 2) > + public: > + explicit AutoSkipProcessRestyle(nsPresContext* aPresContext) > + : mPresContext(aPresContext) > + { > + // If mPresContext is null, which means we also cannot get an > + // EffectCompotisor, so this AutoSkipProcessRestyle doesn't need to do EffectCompositor ::: dom/animation/EffectCompositor.h:332 (Diff revision 2) > + > + // If set this flag, RequestRestyle returns early. > + bool mSkipProcessRestyle = false; I think we should call this mIgnoreRequestRestyle ("process restyle" sounds like it has something to do with ProcessRestyles in RestyleManager). We should also include something like the comment from above... "We use this when we are already processing restyles (such as when rebuidling CSS animations) or in other situations where we know a restyle is not required." Although we probably don't need the same comment in two places. This might be the better spot. ::: dom/animation/EffectCompositor.h:335 (Diff revision 2) > OwningNonNull<AnimationStyleRuleProcessor>> > mRuleProcessors; > + > + // If set this flag, RequestRestyle returns early. > + bool mSkipProcessRestyle = false; > + Nit: Drop the extra blank line. ::: layout/style/nsAnimationManager.cpp:96 (Diff revision 2) > void > CSSAnimation::PlayFromStyle() > { > mIsStylePaused = false; > if (!mPauseShouldStick) { > + EffectCompositor::AutoSkipProcessRestyle skipRestyle(GetPresContext()); Won't this already be set from whatever calls us? ::: layout/style/nsAnimationManager.cpp:114 (Diff revision 2) > return; > } > > mIsStylePaused = true; > + > + EffectCompositor::AutoSkipProcessRestyle skipRestyle(GetPresContext()); Won't this already be set from whatever calls us? ::: layout/style/nsAnimationManager.cpp:657 (Diff revision 2) > new CSSAnimation(aPresContext->Document()->GetScopeObject(), > aSrc.GetName()); > animation->SetOwningElement( > OwningElementRef(*mTarget, mStyleContext->GetPseudoType())); > > - animation->SetTimelineNoUpdate(mTimeline); > + EffectCompositor::AutoSkipProcessRestyle skipRestyle(aPresContext); Shouldn't we set this further up the stack such as in nsAnimationManager::UpdateAnimations? Perhaps alongside where we add the nsAutoAnimationMutationBatch there? That way we'd catch all the unnecessary restyles possibly dispatched from places like SetKeyframes (above). ::: layout/style/nsTransitionManager.cpp:793 (Diff revision 2) > new CSSTransition(mPresContext->Document()->GetScopeObject()); > animation->SetOwningElement( > OwningElementRef(*aElement, aNewStyleContext->GetPseudoType())); > - animation->SetTimelineNoUpdate(timeline); > + > + { > + EffectCompositor::AutoSkipProcessRestyle skipRestyle(mPresContext); Again, couldn't we do this further up the stack such as in nsTransitionManager::StyleContextChanged? (And, once again, alongside the instantation of nsAutoAnimationMutationBatch might work?)

Comment on attachment 8777258 [details] Bug 1049975 - Part 7: Add SetEffectNoUpdate. https://reviewboard.mozilla.org/r/68844/#review67054 ::: dom/animation/EffectCompositor.h:240 (Diff revision 2) > + ~AutoSkipProcessRestyle() > + { > + if (!mPresContext) { > + return; > + } > + mPresContext->EffectCompositor()->mSkipProcessRestyle = false; Oh, one more comment: We should make this class able to be nested -- i.e. it should restore the value of mIgnoreRequestRestyle not just blindly set it to false.

Comment on attachment 8771336 [details] Bug 1049975 - Part 9: Implement writable Animation effect. https://reviewboard.mozilla.org/r/64558/#review67034 I think this is good but I want to have one more look with the following changes made (I had a feeling that we will be triggering three calls to RequestRestyle when stealing another animation's effect--and that we should only be doing two but I can't work out now why that would be a problem). ::: dom/animation/Animation.cpp:146 (Diff revision 5) > + // because ResetPendingTasks needs to get the rendered document by its > + // current effect. > + ResetPendingTasks(); > - } > + } > - mEffect = aEffect; > - if (mEffect) { > + > + mIsRelevant = false; Why do we need to do this? ::: dom/animation/Animation.cpp:149 (Diff revision 5) > - } > + } > - mEffect = aEffect; > - if (mEffect) { > + > + mIsRelevant = false; > + > + // Break links with the old effect and then drop it. > + auto oldEffect = mEffect; Perhaps it's better to replace 'auto' with 'RefPtr<AnimationEffectReadOnly>' to make it clear that we're keeping this effect alive. ::: dom/animation/Animation.cpp:155 (Diff revision 5) > + mEffect = nullptr; > + oldEffect->SetAnimation(nullptr); > + } > + > + if (aEffect) { > + // Reset previous animation of the new effect. // Break link from the new effect to its previous animation, if any. ::: dom/animation/Animation.cpp:172 (Diff revision 5) > + PendingAnimationTracker* tracker = > + doc->GetOrCreatePendingAnimationTracker(); > + if (tracker) { > + if (mPendingState == PendingState::PlayPending) { > + tracker->RemovePlayPending(*this); > + tracker->AddPlayPending(*this); > + } else { > + tracker->RemovePausePending(*this); > + tracker->AddPausePending(*this); > + } > + } I thought we decided this wasn't necessary unless mPendingReadyTime was set? Is that right? If mPendingReadyTime is set then I guess we won't be in the tracker so we can skip the RemoveXXXPending call? ::: dom/animation/KeyframeEffect.cpp:1222 (Diff revision 5) > > mCumulativeChangeHint |= changeHint; > } > } > } > +void Nit: Blank line before this. ::: dom/animation/KeyframeEffect.cpp:1223 (Diff revision 5) > mCumulativeChangeHint |= changeHint; > } > } > } > +void > +KeyframeEffectReadOnly::SetAnimation(Animation* aAnimation) I notice we don't call AnimationEffectReadOnly::SetAnimation at all from within this override. Would it make sense to: * Make AnimationEffectReadOnly::SetAnimation pure virtual for now? * Make NotifyAnimationTimingUpdated specific to KeyframeEffectReadOnly (it seems to be only doing style-related things)?

Comment on attachment 8771337 [details] Bug 1049975 - Part 10: Test for writable effect. https://reviewboard.mozilla.org/r/64560/#review67076 r=birtles with the following changes made ::: dom/animation/test/style/file_animation-setting-effect.html:50 (Diff revision 5) > +}, 'After setting target effect with a previous animation, the computed ' + > + 'style of the target element should be correct'); After setting the target effect from an existing animation, the computed style of the target effect should reflect the time of the updated animation. ::: dom/animation/test/style/file_animation-setting-effect.html:74 (Diff revision 5) > + 'new margin-left of the target element'); > + assert_equals(getComputedStyle(target).marginTop, '-10px', > + 'new margin-top of the target element'); > +}, 'After setting target effect with an animation to another animation which ' + > + 'also has an target effect and both animation effects target to the same ' + > + 'element, the computed style of this element should be correct'); s/should be correct/should reflect the time and effect of the animation that was set/ ::: dom/animation/test/style/file_animation-setting-effect.html:98 (Diff revision 5) > + 'different elements, the computed styles of the two elements should ' + > + 'be correct'); ...should reflect the time and effect of the animation that was set ? ::: dom/animation/test/style/file_animation-setting-effect.html:118 (Diff revision 5) > + animA.effect = animB.effect; > + animB.effect = effectA; > + assert_equals(getComputedStyle(target).marginLeft, '60px'); > + assert_equals(getComputedStyle(target).marginTop, '10px'); > +}, 'After swapping effects of two playing animations, both animations are ' + > + 'still running'); still running with the same current time ::: testing/web-platform/tests/web-animations/interfaces/Animation/effect.html:15 (Diff revision 5) > + var anim = new Animation(new KeyframeEffectReadOnly(createDiv(t), null), > + document.timeline); Perhaps we should either initially test that effect is not null, or just call new Animation() and test that anim.effect *is* null. Otherwise there doesn't seem to be any reason to pass 'new KeyframeEffectReadOnly(...)' initially. ('new Animation()' should now cause us to use a null effect and document.timeline as the timeline, I believe.) ::: testing/web-platform/tests/web-animations/timing-model/animations/set-the-target-effect-of-an-animation.html:19 (Diff revision 5) > + var anim = createDiv(t).animate({ marginLeft: [ '0px', '100px' ] }, > + 100 * MS_PER_SEC); > + assert_equals(anim.playState, 'pending'); > + > + var retPromise = anim.ready.then(function() { > + assert_unreached('ready promise is fulfilled\n'); Do we need the \n ? ::: testing/web-platform/tests/web-animations/timing-model/animations/set-the-target-effect-of-an-animation.html:32 (Diff revision 5) > +promise_test(function(t) { > + var anim = createDiv(t).animate({ marginLeft: [ '0px', '100px' ] }, > + 100 * MS_PER_SEC); > + return anim.ready.then(function() { > + var ready1 = anim.ready; > + anim.effect = null; > + var ready2 = anim.ready; > + assert_equals(ready1, ready2, 'Get the same promise after reset'); > + }); > +}, 'If new effect is null and old effect is not null, don\'t reject ' + > + 'animation\'s current ready promise if we are not pending'); I'm not actually sure that you can reject a promise after it has been resolved. Last time I checked our code I think that was true. Can you check the code / spec and confirm? If so, I'm not sure this test makes sense and we can probably drop it. ::: testing/web-platform/tests/web-animations/timing-model/animations/set-the-target-effect-of-an-animation.html:65 (Diff revision 5) > + anim.effect = new KeyframeEffectReadOnly(createDiv(t), > + { marginLeft: [ '0px', '100px' ] }, > + 100 * MS_PER_SEC); Should we assert that we are still pending after this? ::: testing/web-platform/tests/web-animations/timing-model/animations/set-the-target-effect-of-an-animation.html:75 (Diff revision 5) > + var anim = createDiv(t).animate(null); > + assert_equals(anim.playState, 'pending'); > + > + return waitForAnimationFrames(1).then(function() { > + assert_equals(anim.playState, 'pending'); > + var ready1 = anim.ready; > + anim.effect = new KeyframeEffectReadOnly(createDiv(t), > + { marginLeft: [ '0px', '100px' ] }, > + 100 * MS_PER_SEC); > + assert_equals(anim.playState, 'pending'); > + var ready2 = anim.ready; > + assert_equals(ready1, ready2); Does this test ever fail? If not, maybe we can drop it. The spec doesn't actually require that the animation is still pending after the next animation frame so I'm not sure we can use this as an interop test. ::: testing/web-platform/tests/web-animations/timing-model/animations/set-the-target-effect-of-an-animation.html:101 (Diff revision 5) > +}, 'If new effect is not null and if new effect is the target effect ' + > + 'of another animation, previous animation, set the target effect as null ' + > + 'to previous animation.'); 'When setting the effect of an animation to the effect of an existing animation, the existing animation's target effect should be set to null.' ::: testing/web-platform/tests/web-animations/timing-model/animations/set-the-target-effect-of-an-animation.html:118 (Diff revision 5) > +}, 'After setting target effect with a previous animation, this animation ' + > + 'effect is aligned to the new animation because we let target effect of ' + > + 'this animation be the new effect.'); 'After setting the target effect of animation to the target effect of an existing animation, the target effect's timing is updated to reflect the current time of the new animation.'

Comment on attachment 8771923 [details] Bug 1049975 - Part 11: Fix mutation observer when setting effects. https://reviewboard.mozilla.org/r/64850/#review67068 It seems like we should be able to do this a lot more simply. I'm not sure yet if this works, but can we just use nsAutoAnimationMutationBatch and then call UpdateRelevance() once after disassociating from the old effect, and then again after associating with the new effect? That would seem to be a lot simpler if it works. I think we could remove all the changes to nsNodeUtils and confine updating mIsRelevant to UpdateRelevance() that way. ::: dom/animation/Animation.cpp:215 (Diff revision 4) > + } else if (wasRelevant) { > + nsNodeUtils::AnimationRemoved(this, oldTarget); > + } > } > > + // UpdateTimine() will call UpdateRelevance() which updates mIsRelevant and UpdateTiming

Comment on attachment 8771923 [details] Bug 1049975 - Part 11: Fix mutation observer when setting effects. https://reviewboard.mozilla.org/r/64850/#review67514 Clearing review request because this can be done a lot more simply. For example, this patch works for me: https://pastebin.mozilla.org/8890722 With this you won't need any of the changes to dom/base/* or even AnimationTarget although you'll need to update the set_effect_with_previous_animation test in test_animation_observers.html since the deletion and addition will get batched giving you: assert_records([{ added: [], changed: [], removed: [anim1] }, // div { added: [anim1], changed: [], removed: [anim2] }], // child "records after animation effects are changed");

Comment on attachment 8771334 [details] Bug 1049975 - Part 4: Merge two Animation::SetEffect()s. https://reviewboard.mozilla.org/r/63668/#review67024 > I think we can drop this method if we make the changes above. Oops. Sorry, I missed that comment. Thanks.

Comment on attachment 8777258 [details] Bug 1049975 - Part 7: Add SetEffectNoUpdate. https://reviewboard.mozilla.org/r/68844/#review67036 > AutoIgnoreRequestRestyle? > > Also, we should declare this MOZ_RAII and use the appropriate guard object macros. (See mfbt/GuardObjects.h and examples such as AutoMutationBatchForAnimation). OK. I didn't notice this before. Thanks for your example. > Shouldn't we set this further up the stack such as in nsAnimationManager::UpdateAnimations? Perhaps alongside where we add the nsAutoAnimationMutationBatch there? > > That way we'd catch all the unnecessary restyles possibly dispatched from places like SetKeyframes (above). OK. Putting it in UpdateAnimations() is much better. Actually, I thought PlayFromStyle and PauseFromStyle might be called by other classes. However, after I checked it again, they are called only in the scope of UpdateAnimations(), so I will remove AutoIgnoreRequestRestyle from PlayFromStyle and PauseFromStyle and use AutoIgnoreRequestRestyle alongside where we add the nsAutoAnimationMutationBatch, as your comments suggested. Thanks. > Again, couldn't we do this further up the stack such as in nsTransitionManager::StyleContextChanged? (And, once again, alongside the instantation of nsAutoAnimationMutationBatch might work?) Sure. Thank you.

Comment on attachment 8777258 [details] Bug 1049975 - Part 7: Add SetEffectNoUpdate. https://reviewboard.mozilla.org/r/68844/#review67036 > Sure. Thank you. I found PruneCompletedTransitions(), which used by nsCSSFrameConstructor, also calls CancelFromStyle() [1], so I'd like to declare an AutoIgnoreRequestRestyle object in PruneCompletedTransitions(), too. [1] http://searchfox.org/mozilla-central/rev/9ec085584d7491ddbaf6574d3732c08511709172/layout/base/nsCSSFrameConstructor.cpp#1888

Comment on attachment 8777258 [details] Bug 1049975 - Part 7: Add SetEffectNoUpdate. https://reviewboard.mozilla.org/r/68844/#review67054 > Oh, one more comment: > > We should make this class able to be nested -- i.e. it should restore the value of mIgnoreRequestRestyle not just blindly set it to false. OK. I will add one more data member to store the cached value.

BTW, I just notice that the second argument of the Animation constructor is |KeyframeEffectReadOnly*|, not |AnimationEffectReadOnly*|, so I think it's better to add one extra patch to revise it.

Comment on attachment 8771336 [details] Bug 1049975 - Part 9: Implement writable Animation effect. https://reviewboard.mozilla.org/r/64558/#review67034 > Why do we need to do this? I added this is for the updating of mutation observer in UpdateRelevance(). However, after simplying the mutation observer, we can remove this. > I thought we decided this wasn't necessary unless mPendingReadyTime was set? Is that right? If mPendingReadyTime is set then I guess we won't be in the tracker so we can skip the RemoveXXXPending call? OK. Yes, we can remove the RemovexxxPending if mPendingReadyTime is not null. > I notice we don't call AnimationEffectReadOnly::SetAnimation at all from within this override. Would it make sense to: > * Make AnimationEffectReadOnly::SetAnimation pure virtual for now? > * Make NotifyAnimationTimingUpdated specific to KeyframeEffectReadOnly (it seems to be only doing style-related things)? Sure. I can make AnimationEffectReadOnly::SetAnimation pure virtual and remove AnimationEffectReadOnly::NotifyAnimationTimingUpdated.

(In reply to Brian Birtles (:birtles) from comment #107) > Comment on attachment 8771923 [details] > Bug 1049975 - Part 10: Fix mutation observer when setting effects. > > https://reviewboard.mozilla.org/r/64850/#review67514 Thanks, Brian. When I wrote this part, I didn't notice that nsAutoAnimationMutationBatch can also check whether the target is the same or not if we call multiple AnimationRemoved, AnimationAdded, and AnimationChanged function calls, and it combines them if necessary. After recalling it and comment 35, your idea is much simpler and clearer. Sorry for letting you spend time on explaining this. The code in the pastebin works correctly, so I will use them directly. Thanks.

(In reply to Brian Birtles (:birtles) from comment #104) > I think this is good but I want to have one more look with the following > changes made (I had a feeling that we will be triggering three calls to > RequestRestyle when stealing another animation's effect--and that we should > only be doing two but I can't work out now why that would be a problem). Yes, we may trigger three calls to RequestRestyle when stealing animation's effect. if (mEffect) { ... mEffect = nullptr oldEffect->SetAnimation(nullptr); // RequestRestyle for oldEffect ... } if (aEffect) { ... if (prevAnim) { prevAnim->SetEffect(nullptr); // RequestRestyle for newEffect } // (because we call SetAnimation(nullptr) in SetEffect(nullptr)) mEffect = newEffect; mEffect->SetAnimation(this); // RequestRestyle for newEffect } ... UpdateTiming() // This may trigger a RequestRestyle from NotifyAnimationTimingUpdated, // but not force to layer update. The three calls to SetAnimation trigger three RequestRestyles, and one of the RequestRestyle to the new effect may be redundant. If old effect and new effect target to the same target element, one RequestRestyle may be enough. Maybe we can do something in EffectCompositor::RequestRestyle to filter out the redundant calls, but I'm not sure how many things we have to do. Or just store the target elements, and trigger RequestRestyles at the end of SetEffect(). However, this may make the code more complicate.

Comment on attachment 8771337 [details] Bug 1049975 - Part 10: Test for writable effect. https://reviewboard.mozilla.org/r/64560/#review67076 > Perhaps we should either initially test that effect is not null, or just call new Animation() and test that anim.effect *is* null. Otherwise there doesn't seem to be any reason to pass 'new KeyframeEffectReadOnly(...)' initially. > > ('new Animation()' should now cause us to use a null effect and document.timeline as the timeline, I believe.) OK. I will change this to: just call new Animation() initially and test that anim.effect *is* null. And then assign a new effect, and the test anim.effect is this new effect. Thanks. > Do we need the \n ? I will remove '\n'. > I'm not actually sure that you can reject a promise after it has been resolved. Last time I checked our code I think that was true. Can you check the code / spec and confirm? > > If so, I'm not sure this test makes sense and we can probably drop it. The spec say: Attempting to resolve or reject a resolved promise has no effect [1]. Therefore, I'd like to drop this test. [1] http://www.ecma-international.org/ecma-262/6.0/#sec-promise-objects > Does this test ever fail? If not, maybe we can drop it. The spec doesn't actually require that the animation is still pending after the next animation frame so I'm not sure we can use this as an interop test. It never fails. OK, let drop this test.

Comment on attachment 8777258 [details] Bug 1049975 - Part 7: Add SetEffectNoUpdate. https://reviewboard.mozilla.org/r/68844/#review67036 > OK. Putting it in UpdateAnimations() is much better. Actually, I thought PlayFromStyle and PauseFromStyle might be called by other classes. However, after I checked it again, they are called only in the scope of UpdateAnimations(), so I will remove AutoIgnoreRequestRestyle from PlayFromStyle and PauseFromStyle and use AutoIgnoreRequestRestyle alongside where we add the nsAutoAnimationMutationBatch, as your comments suggested. Thanks. Sorry, This casued some test failed in layout/style/test/test_animations.html and test_animations.omta.html. Reasons: 1. We still need to request a restyle for old effect in UpdateOldAnimationPropertiesWithNew() [1]. (i.e. oldEffect->SetKeyframes(...) still need to request a restyle) 2. At the end of UpdateAnimations(), we call mPresContext->EffectCompositor()->MaybeUpdateAnimationRule(), which updates the cascade and need to request a restyle. [2] [1] http://searchfox.org/mozilla-central/rev/b35975ec17c8227e827800fa2d9642655cb103a8/layout/style/nsAnimationManager.cpp#338-344 [2] http://searchfox.org/mozilla-central/rev/b35975ec17c8227e827800fa2d9642655cb103a8/layout/style/nsAnimationManager.cpp#443-447 In nsTransitionManager.cpp, we also call mPresContext->EffectCompositor()->MaybeUpdateAnimationRule() at the end of StyleContextChanged(). BTW, There is another minor problem. PlayNoUpdate(), PauseNoUpdate(), CancelNoUpdate(), and SetTimelineNoUpdate() will call UpdateTimine() which eventually requests a restyle for Throttle/Standard restyle type. Therefore, I think we shouldn't ignore all the types in RequestRestyle(), and only |layer| type is enough. I will update these and let you review again. I'm not sure which way is better, using a flag in RequestRestyle or keeping xxxNoUpdate. You can check my new patch again. Thanks.

Comment on attachment 8781954 [details] Bug 1049975 - Part 8: Use AnimationEffectReadOnly as the argument type of Animation constructor. https://reviewboard.mozilla.org/r/72250/#review70746

Comment on attachment 8781954 [details] Bug 1049975 - Part 8: Use AnimationEffectReadOnly as the argument type of Animation constructor. https://reviewboard.mozilla.org/r/72250/#review70930

Comment on attachment 8771923 [details] Bug 1049975 - Part 11: Fix mutation observer when setting effects. https://reviewboard.mozilla.org/r/64850/#review70934

I'm having a bit of trouble working out what to do with part 7. We started off adding this class to simplify the code but it's starting to feel more complex. I guess we need to understand what restyling needs to happen when updating animations. Thanks for looking into it in comment 130, but I still don't understand exactly what needs to happen. (In reply to Boris Chiou [:boris] from comment #130) > 1. We still need to request a restyle for old effect in > UpdateOldAnimationPropertiesWithNew() [1]. (i.e. > oldEffect->SetKeyframes(...) still need to request a restyle) Why is this? Why do we need to request a restyle in this case, but not when we have a new animation? What actually needs to happen? > 2. At the end of UpdateAnimations(), we call > mPresContext->EffectCompositor()->MaybeUpdateAnimationRule(), which updates > the cascade and need to request a restyle. [2] So we only need to request a restyle if the cascade has changed? > BTW, There is another minor problem. PlayNoUpdate(), PauseNoUpdate(), > CancelNoUpdate(), and SetTimelineNoUpdate() will call UpdateTimine() which > eventually requests a restyle for Throttle/Standard restyle type. Therefore, > I think we shouldn't ignore all the types in RequestRestyle(), and only > |layer| type is enough. This part feels flaky to me. Having a mode that only applies to certain restyle types seems odd. > I will update these and let you review again. I'm not sure which way is > better, using a flag in RequestRestyle or keeping xxxNoUpdate. You can check > my new patch again. Thanks. I'm starting to lean towards xxxNoUpdate. But, if you have time, would you mind investigating this a little more deeply to understand the above questions so we can decide what is the best way of handling this? Thanks Boris! We're nearly there!

Comment on attachment 8771336 [details] Bug 1049975 - Part 9: Implement writable Animation effect. https://reviewboard.mozilla.org/r/64558/#review70940 r=me with the following comments addressed. ::: dom/animation/Animation.cpp:141 (Diff revision 7) > + // If the new effect is null, we have to reset the pending task first > + // because ResetPendingTasks needs to get the rendered document by its > + // current effect. This might be a little more clear as: "If the new effect is null, call ResetPendingTasks before clearing mEffect since ResetPendingTasks needs it to get the appropriate PendingAnimationTracker." ::: dom/animation/Animation.cpp:165 (Diff revision 7) > + > + // Create links with the new effect. > + mEffect = newEffect; > mEffect->SetAnimation(this); > + > + // Reschedule pending pause or pending play tasks. Perhaps we can add an extra explanatory sentence along the lines of, "If we have a pending animation, it will either be registered in the pending animation tracker and have a null pending ready time, or, after it has been painted, it will be removed from the tracker and assigned a pending ready time. After updating the effect we'll typically need to repaint so if we've already been assigned a pending ready time, we should clear it and put the animation back in the tracker." ::: dom/animation/Animation.cpp:173 (Diff revision 7) > + if (tracker) { > + if (mPendingState == PendingState::PlayPending) { > + tracker->AddPlayPending(*this); > + } else { > + tracker->AddPausePending(*this); > + } > + } GetOrCreatePendingAnimationTracker should never return nullptr so we don't need to null-check |tracker| here. ::: dom/animation/Animation.cpp:1124 (Diff revision 7) > + if (!mEffect->AsKeyframeEffect()) { > + return; > + } > + mEffect->AsKeyframeEffect()->NotifyAnimationTimingUpdated(); AsKeyframeEffect() is a virtual method. Could we avoid calling it twice in this very-often called method like so: KeyframeEffectReadOnly* keyframeEffect = mEffect->AsKeyframeEffect(); if (keyframeEffect) { keyframeEffect->NotifyAnimationTimingUpdated(); } ? ::: dom/animation/AnimationEffectReadOnly.cpp:96 (Diff revision 7) > // NotifyEffectTimingUpdated will eventually cause > - // NotifyAnimationTimingUpdated to be called on this object which will > - // update our registration with the target element. > + // KeyframeEffectReadOnly::NotifyAnimationTimingUpdated to be called on > + // this object which will update our registration with the target element. I wonder if this comment makes much now that AnimationEffectReadOnly doesn't have anything to do with registering with target elements? Perhaps we could just say, "For keyframe effects, NotifyEffectTimingUpdated above will eventually cause KeyframeEffectReadOnly::NotifyAnimationTimingUpdated to be called so it can update its registration with the target element as necessary."

I think this is a complicated question, so let's use some examples to explain what happened if a) we put AutoIgnoreRequestRestyle along with nsAutoAnimationMutationBatch in nsAnimationManager.cpp b) Early return RequestStyle for all Restyle types when AutoIgnoreRequestRestyle is living. (In reply to Brian Birtles (:birtles) from comment #141) > (In reply to Boris Chiou [:boris] from comment #130) > > 1. We still need to request a restyle for old effect in > > UpdateOldAnimationPropertiesWithNew() [1]. (i.e. > > oldEffect->SetKeyframes(...) still need to request a restyle) > > Why is this? Why do we need to request a restyle in this case, but not when > we have a new animation? What actually needs to happen? Let's check the example (simplified from test_animations.html): (1) @keyframes anim1 { from { margin-left: 0px } to { margin-left: 100px } } @keyframes anim2 { from { margin-right: 0px } to { margin-right: 100px } } test(function() { var div = addDiv(t); div.style.animation = "anim1 linear 10s, anim2 linear 10s"; var anims = div.getAnimations(); for (let a of anims) { a.currentTime += 1000; } assert_equals(getComputedStyle(div).marginRight, "10px"); assert_equals(getComputedStyle(div).marginLeft, "10px"); div.style.animation = "anim1 linear 10s"; // remove anim2 from end assert_equals(getComputedStyle(div).marginRight, "0px"); // test failed, got unexpected "10px" assert_equals(getComputedStyle(div).marginLeft, "10px"); }, ...); This test trys to remove an animation from div. In the current implementation on m-c, we cancel the removed animation in UpdateAnimations() by CancelFromStyle() [1]. CancelFromStyle() calls UpdateTiming(), which calls NotifyAnimationTimingUpdated() eventually, and then NotifyAnimationTimingUpdated() will call ReqeustRestyle(RestyleType::Standard) if throttle is disabled [2]. RequestRestyle(...) will put an entry into mElementsToRestyle[aCascadeLevel] with 'true' value for the current element [3]. OK, I think this is important. After putting an entry for the target element, MaybeUpdateAnimationRule() [4] decides whether we have to call ComposeAnimationRule() or not by checking the existence and the value of this entry in mElementsToRestyle [5]. When we declare AutoIgnoreRequestRestyle, we will not put an entry into mElementsToRestyle in CancelFromStyle(), so ComposeAnimationRule() won't be called, so the test will be failed. [1] http://searchfox.org/mozilla-central/rev/bb22cc4067c3832b943507497389b0b13d6f3a2b/layout/style/nsAnimationManager.cpp#438-441 [2] http://searchfox.org/mozilla-central/rev/bb22cc4067c3832b943507497389b0b13d6f3a2b/dom/animation/KeyframeEffect.cpp#189-197 [3] http://searchfox.org/mozilla-central/rev/bb22cc4067c3832b943507497389b0b13d6f3a2b/dom/animation/EffectCompositor.cpp#176-182 [4] http://searchfox.org/mozilla-central/rev/bb22cc4067c3832b943507497389b0b13d6f3a2b/layout/style/nsAnimationManager.cpp#443-447 [5] http://searchfox.org/mozilla-central/rev/bb22cc4067c3832b943507497389b0b13d6f3a2b/dom/animation/EffectCompositor.cpp#272-277 The 2nd example: (2) test(function(t) { var div = addDiv(t); div.style.animation = "anim2 linear 10s, anim1 linear 10s"; var anims = div.getAnimations(); for (let a of anims) { a.currentTime += 1000; } assert_equals(getComputedStyle(div).marginRight, "10px"); assert_equals(getComputedStyle(div).marginLeft, "10px"); div.style.animation = "anim1 linear 10s, anim2 linear 5s"; // swap the order and reduce the duration of anim2 assert_equals(getComputedStyle(div).marginRight, "20px"); // test failed, got unexpected "10px" assert_equals(getComputedStyle(div).marginLeft, "10px"); }, ...); This happens when we call oldEffect->SetSpecifiedTiming(aNewTiming) in UpdateOldAnimationPropertiesWithNew() [6]. This function will update the timing of the old animations. In the current implementation, we will call RequestRestyle(), which also puts an entry for the target element with 'true' value. Therefore, ComposeAnimationRule() will be called [4]. If we ignore RequestRestyle() here, ComposeAnimationRule() will be skipped, and then the test will be failed. [6] http://searchfox.org/mozilla-central/rev/bb22cc4067c3832b943507497389b0b13d6f3a2b/layout/style/nsAnimationManager.cpp#342 OK, let's see the next example: (3) test(function(t) { var style = addStyle(t, { "@keyframes dyn1": "from { margin-left: 0 } to { margin-left: 100px }", // cssRules[0] "@keyframes dyn2": "from { margin-left: 100px } to { margin-left: 200px }" }); // cssRules[1] var div = addDiv(t); div.style.animation = "dyn1 1s linear"; var anims = div.getAnimations(); for (let a of anims) { a.currentTime += 250; } assert_equals(getComputedStyle(div).marginLeft, "25px"); style.sheet.cssRules[1].name = "dyn1"; assert_equals(getComputedStyle(div).marginLeft, "125px"); // test failed, got unexpected "25px" }, ...); The root cause here is the same as that of the above example. This test case is failed if oldEffect->SetKeyframes(...) doesn't put an entry into mElementsToRestyle while calling RequestRestyle(), so ComposeAnimationRule() won't be called. [7] http://searchfox.org/mozilla-central/rev/bb22cc4067c3832b943507497389b0b13d6f3a2b/layout/style/nsAnimationManager.cpp#343 > > > 2. At the end of UpdateAnimations(), we call > > mPresContext->EffectCompositor()->MaybeUpdateAnimationRule(), which updates > > the cascade and need to request a restyle. [2] > > So we only need to request a restyle if the cascade has changed? If the cascade is changed, we have to call RequestRestyle(), which puts an entry into mElementsToRestyle, and then ComposeAnimationRule() is called successfully, so everything is ok. This is also the reason that adding a new animation can pass the test. So we shouldn't skip the RequestRestyle() unconditionally while calling MaybeUpdateAnimationRule(). By the way, when setting keyframes for the new animation [8], we haven't set the animation to the effect, so effect->SetKeyframes() won't call RequestRestyle(). [8] http://searchfox.org/mozilla-central/source/layout/style/nsAnimationManager.cpp#643 In conclusion, I think ComposeAnimationRule() is necessary for the above cases. We don't really need to call PostRestyleForAnimation() from RequestRestyle() in example (1), (2), and (3). If we skip RequestRestyle() unconditionally, we will skip ComposeAnimationRule() in the above cases because we will early return in MaybeUpdateAnimationRule(), so these tests will be failed. If we use SetEffectNoUpdate(), we won't meet these problems. Therefore, I will update a new patch to use SetEffectNoUpdate(). Thanks.

Comment on attachment 8777258 [details] Bug 1049975 - Part 7: Add SetEffectNoUpdate. https://reviewboard.mozilla.org/r/68844/#review71852 This looks fine. I think I was more interested in understanding the interaction with the RestyleTracker. Calling ComposeAnimation from UpdateAnimations is fine but I was hoping we could find a more robust/simple was to avoid adding entries to the RestyleTracker and triggering any further restyle processing. We can try that again later in another bug, however.

(In reply to Brian Birtles (:birtles) from comment #151) > Comment on attachment 8777258 [details] > Bug 1049975 - Part 7: Add SetEffectNoUpdate. > > https://reviewboard.mozilla.org/r/68844/#review71852 > > This looks fine. I think I was more interested in understanding the > interaction with the RestyleTracker. Calling ComposeAnimation from > UpdateAnimations is fine but I was hoping we could find a more robust/simple > was to avoid adding entries to the RestyleTracker and triggering any further > restyle processing. We can try that again later in another bug, however. Thanks, Brian. All I know now is we don't really need to call PostRestyleForAnimation, which will add a pending restyle event to the tracker, in those functions, such as SetKeyframes, SetSpecifiedTiming. I can try to understand the necessary interaction with RestyleTracker in UpdateAnimations in another bug later. Thanks.

Pushed by bchiou@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/e38d20155dbf Part 1: Support null effect. r=birtles https://hg.mozilla.org/integration/autoland/rev/d3f16aced88c Part 2: Make effect writable in Animation.webidl. r=smaug https://hg.mozilla.org/integration/autoland/rev/b66d5ee81615 Part 3: Handle removed/replaced effect for CSS Transition. r=birtles https://hg.mozilla.org/integration/autoland/rev/54e45a249e9b Part 4: Merge two Animation::SetEffect()s. r=birtles https://hg.mozilla.org/integration/autoland/rev/7d3e1f0771be Part 5: Move timing related code into AnimationEffectReadOnly. r=birtles https://hg.mozilla.org/integration/autoland/rev/88086ae8ce7d Part 6: Factor out the procedure of resetting an animation's pending tasks. r=birtles https://hg.mozilla.org/integration/autoland/rev/b5f0b070339b Part 7: Add SetEffectNoUpdate. r=birtles https://hg.mozilla.org/integration/autoland/rev/8d8b2a4f2245 Part 8: Use AnimationEffectReadOnly as the argument type of Animation constructor. r=birtles,smaug https://hg.mozilla.org/integration/autoland/rev/af6ce54ce3bf Part 9: Implement writable Animation effect. r=birtles https://hg.mozilla.org/integration/autoland/rev/1a3f2e5f456b Part 10: Test for writable effect. r=birtles https://hg.mozilla.org/integration/autoland/rev/cd68ccd6bf2d Part 11: Fix mutation observer when setting effects. r=birtles https://hg.mozilla.org/integration/autoland/rev/4123cef199ca Part 12: Test for running on the compositor when effects are changed. r=hiro https://hg.mozilla.org/integration/autoland/rev/93f1020581a7 Part 13: Add reftests for stacking context when effects are changed. r=hiro

Yay! Nice work!

https://hg.mozilla.org/mozilla-central/rev/e38d20155dbf https://hg.mozilla.org/mozilla-central/rev/d3f16aced88c https://hg.mozilla.org/mozilla-central/rev/b66d5ee81615 https://hg.mozilla.org/mozilla-central/rev/54e45a249e9b https://hg.mozilla.org/mozilla-central/rev/7d3e1f0771be https://hg.mozilla.org/mozilla-central/rev/88086ae8ce7d https://hg.mozilla.org/mozilla-central/rev/b5f0b070339b https://hg.mozilla.org/mozilla-central/rev/8d8b2a4f2245 https://hg.mozilla.org/mozilla-central/rev/af6ce54ce3bf https://hg.mozilla.org/mozilla-central/rev/1a3f2e5f456b https://hg.mozilla.org/mozilla-central/rev/cd68ccd6bf2d https://hg.mozilla.org/mozilla-central/rev/4123cef199ca https://hg.mozilla.org/mozilla-central/rev/93f1020581a7

This has been documented, but not closed, by Sheppy: https://developer.mozilla.org/en-US/docs/Web/API/Animation https://developer.mozilla.org/en-US/docs/Web/API/Animation/effect and https://developer.mozilla.org/en-US/Firefox/Releases/51

(In reply to Jean-Yves Perrier [:teoli] from comment #169) > This has been documented, but not closed, by Sheppy: > https://developer.mozilla.org/en-US/docs/Web/API/Animation > https://developer.mozilla.org/en-US/docs/Web/API/Animation/effect > and > https://developer.mozilla.org/en-US/Firefox/Releases/51 This is not shipping. It is preffed off.[1] [1] http://searchfox.org/mozilla-central/rev/efcb1644e49c36445e75d89b434fdf4c84832c84/dom/webidl/Animation.webidl#20

Thanks Brian! A quick question: Does the condition (Func="nsDocument::IsWebAnimationsEnabled") means these parts of the WAAPI are not shipping (and the funcname is slightly misleading) or does it means that the whole WAAPI is not yet shipping? In other words should we double check the others constructors/properties/methods that use this prefix too?

It means the former. It used to mean the whole API but then we shipped a subset of the API under a different pref (dom.animations-api.element-animate.enabled). It's a bit confusing because if that pref is true, the whole API is exposed, regardless of the setting of the dom.animations-api.element-animate.enabled pref. I think at one point I went through most of the other API docs and made it say "unsupported" for anything covered by this pref. This was after discussion with Google where we agreed that anything not shipping to the release channel we would mark as unsupported (it was getting too confusing to say, "sort of supported in version 35, a bit more supported in 37, and fully supported in 41 but behind pref X").