Off main thread animations don't fit in the CSS cascade the same way the normal animation codepath does. I have parts of a plan for fixing this (tied in to a plan to make the spec more precise about a bunch of issues), but I wanted to get a bug on file about the issue.

Some thoughts on how to fix this are in bug 1125455.

(In reply to David Baron [:dbaron] (UTC-8) from bug 1125455 comment #2) > 3. bug 847287 for CSS Transitions requires exactly the boolean from (1) to > decide whether to send animations to the compositor. I actually handled this part in bug 1125455. > 2. bug 847287 for CSS Animations requires a boolean for each animation of a > property on an element to decide whether to send animations to the > compositor -- whether that animation won in the cascade. The style set > could notify the animation manager when it's about to do cascading for an > element, and it could set all of the booleans for that element to false, and > then set them to true again This is actually harder than I was thinking due to the various interesting cases in nsRuleNode::WalkRuleTree (which is actually where the computations that matter live). It's extra fun because we might switch between two rule nodes that both have cached data, but for which the resulting booleans should be different, without actually rerunning WalkRuleTree, because we have cached data.

I think it might be easier to just do all the calculations in CheckAnimationRule.

I have most of the code written for this (within https://hg.mozilla.org/users/dbaron_mozilla.com/patches/ , as usual), although I still need to solve a bug with dynamic updating when an overriding animation stops filling, and clean up a temporary fix for the mess of HasAnimationsOfProperty callers that are all subtly different.

A try push of just this patch series: https://treeherder.mozilla.org/#/jobs?repo=try&revision=38966bec57ee And another try push with this patch series plus enabling OMT Animations: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6622f9ac9faf

All of the todos will be fixed by later patches in this bug (as will some already-existing todos in the same file).

Patch 3 adds sanity-checks to check these flags against other equivalent data. This flag is used in patch 5 and patch 6.

These flags were added in patch 2.

This is used in patch 6.

This is used in patch 6. Bug 1148853 covers using this more in existing code.

This is the main patch for the bug; it makes us use the mechanism added in bug 1125455 to avoid sending animations that aren't currently applying to the compositor. Patch 7 is needed to make this code rerun in all the cases where we need to rerun it, though.

This is an additional part of the main work in this bug; it keeps mWinsInCascade updated in cases where we need to update it.

This avoids some extra work that was added in bug 1125455 now that we have a mechanism for detecting when animations start and stop filling (introduced in patch 7). This is also needed to prevent infinite recursion in patch 9.

I don't have a test case that requires this, but it seems like a good idea. (It was an incorrect theory for fixing a test failure that I was debugging, but still seems worth doing.)

This saves some extra work that we don't need to do. I want to do this here since patch 9 introduces a new call to EnsureStyleRuleFor.

This saves some extra work that we don't need to do. Mechanically, the patch moves a chunk of code that is around the last part of the function and converts it to an early return that's slightly earlier than that last part, thus also including the skipping of the throttling checks in what we skip for the early return. I want to do this here since patch 9 introduces a new call to EnsureStyleRuleFor.

This patch (after stepping through the call graph) affects the following places: * CommonAnimationManager::GetAnimationsForCompositor, which is used only by nsDisplayListBuilder::AddAnimationsAndTransitionsToLayer, which already checks the individual animations (so really no change) * AnimationPlayerCollection::CanThrottleAnimation * ActiveLayerTracker::IsStyleAnimated * nsLayoutUtils::HasAnimationsForCompositor * nsLayoutUtils::HasAnimations (which is used only to check whether we can make the 0-opacity optimization) I believe it makes sense to change all of these locations (although in the long term we want to throttle (or similar) more animations). Without this patch, I believe we're forcing the creation of an opacity layer because we think we have animations to send to it.

This independently would have fixed some of the problems fixed in this bug. It would not have fixed them fully, though, since we always fill animations on layers forwards in the expectation of another update (to avoid jumping and then jumping back), so we need to send updates after animations complete in order to allow the animations that they override to start applying on the compositor.

Also note that following this patch series, test_animations_omta.html no longer has any todo (KNOWN-FAIL) tests in it (which it does right now).

Comment on attachment 8585111 [details] [diff] [review] patch 1 - Add additional tests >+// Bug 847287 - Test that changes of when an animation is dynamically >+// overridden work correctly. >+addAsyncAnimTest(function *() { >+ advance_clock(0); This isn't needed. runAsyncAnimTest does this automatically: https://hg.mozilla.org/mozilla-central/file/9c786bf6a7ff/layout/style/test/animation_utils.js#l363 >+ // One animation overriding another, and then not. >+ new_div("animation: anim2 1s linear, anim3 500ms linear reverse"); >+ yield waitForPaintsFlushed(); >+ omta_todo_is("opacity", 1, RunningOn.Compositor, >+ "anim3 overriding anim2 at start (0s)"); >+ advance_clock(400); >+ omta_todo_is("opacity", 0.2, RunningOn.Compositor, >+ "anim3 overriding anim2 at 400ms"); >+ advance_clock(200); >+ // Wait for paints because we're resending animations to the >+ // compositor via an UpdateOpacityLayer hint, which does the resending >+ // via painting. >+ yield waitForPaintsFlushed(); Here, and a number of other places in this file, I think this should still pass with just 'yield waitForPaints()'? In bug 975261 there were some bugs that were masked by the tests triggering a flush so I'd prefer to use waitForPaints where possible. >+ // One animation overriding another, and then not, but without a >+ // restyle when the overriding one ends. >+ new_div("animation: anim2 1s steps(8, end)"); >+ yield waitForPaintsFlushed(); >+ omta_is("opacity", 0, RunningOn.Compositor, >+ "anim2 at start (0s)"); >+ advance_clock(300); >+ omta_is("opacity", 0.25, RunningOn.Compositor, >+ "anim2 at 300ms"); >+ gDiv.style.animation = "anim2 1s steps(8, end), anim3 500ms steps(4, end)"; >+ yield waitForPaintsFlushed(); (Here, obviously, it makes sense to flush, however.) >+ omta_todo_is("opacity", 0, RunningOn.Compositor, >+ "anim3 overriding anim2 at 300ms"); >+ advance_clock(475); >+ omta_is("opacity", 0.75, RunningOn.Compositor, >+ "anim3 the same as anim2 at 775ms"); >+ advance_clock(50); >+ // Wait for paints because we're resending animations to the >+ // compositor via an UpdateOpacityLayer hint, which does the resending >+ // via painting. >+ yield waitForPaintsFlushed(); >+ omta_is("opacity", 0.75, RunningOn.Compositor, >+ "anim2 at 825ms"); >+ advance_clock(75); >+ // Wait for paints because we're resending animations to the >+ // compositor via an UpdateOpacityLayer hint, which does the resending >+ // via painting. >+ yield waitForPaintsFlushed(); >+ omta_is("opacity", 0.875, RunningOn.Compositor, >+ "anim2 at 900ms"); >+ done_div(); At which point do we actually expect the layer transaction to occur? If the two animations produce identical output at 775ms will we fail to notice the change? I wonder if we need the final waitForPaintsFlushed.

Comment on attachment 8585113 [details] [diff] [review] patch 3 - Add assertions about consistency of the flags for animating on the compositor >+#ifdef DEBUG >+/* static */ void >+CommonAnimationManager::Initialize() >+{ ... >+ // Set that every property with the flag for animating on the >+ // compositor has an entry in sLayerAnimationInfo. "Check that..."

Comment on attachment 8585114 [details] [diff] [review] patch 4 - Add a method to nsRuleNode that reports the properties overriding a CSS animation >+/* static */ void >+nsRuleNode::ComputePropertiesOverridingAnimation( >+ const nsTArray<nsCSSProperty>& aProperties, >+ nsStyleContext* aStyleContext, >+ nsCSSPropertySet& aPropertiesOverridde ... >+ // Transitions are the only non-!important level overriding >+ // animations in the cascade ordering. They also don't actually >+ // override animations. We should probably clarify these two seemingly contradictory sentences. I believe this is referring to the fact that although transitions are higher up in the cascade, they're not added if the property is undergoing a CSS Animation at the time.

(In reply to Brian Birtles (:birtles) from comment #20) > Comment on attachment 8585111 [details] [diff] [review] > patch 1 - Add additional tests > > >+// Bug 847287 - Test that changes of when an animation is dynamically > >+// overridden work correctly. > >+addAsyncAnimTest(function *() { > >+ advance_clock(0); > > This isn't needed. runAsyncAnimTest does this automatically: Indeed. I missed this when applying bug 1125455 comment 14 to this patch; fixed now. > Here, and a number of other places in this file, I think this should still > pass with just 'yield waitForPaints()'? Yes, it does pass. I've made that change; I still need to double-check the intermediate results (the patches removing the various todos) are all the same. > At which point do we actually expect the layer transaction to occur? If the > two animations produce identical output at 775ms will we fail to notice the > change? I wonder if we need the final waitForPaintsFlushed. I was indeed able to remove the final waitForPaints() in the second and third test.

(In reply to Brian Birtles (:birtles) from comment #22) > We should probably clarify these two seemingly contradictory sentences. I > believe this is referring to the fact that although transitions are higher > up in the cascade, they're not added if the property is undergoing a CSS > Animation at the time. I updated the comment to read: // Transitions are the only non-!important level overriding // animations in the cascade ordering. They also don't actually // override animations, since transitions are suppressed when both // are present. And since we might not have called // UpdateCascadeResults (which updates when they are suppressed // due to the presence of animations for the same element and // property) for transitions yet (which will make their // MapRuleInfoInto skip the properties that are currently // animating), we should skip them explicitly.

Comment on attachment 8585115 [details] [diff] [review] patch 5 - Add method to update animations on layer >+void >+AnimationPlayerCollection::PostResendToLayer() >+{ Is this method name correct? It seems like two verbs in a row, or does "resend" have another meaning here? PostRestyleToLayer? >+ nsCSSPropertySet propsHandled; >+ const auto& info = css::CommonAnimationManager::sLayerAnimationInfo; >+ for (size_t playerIdx = mPlayers.Length(); playerIdx-- != 0; ) { >+ const auto& properties = mPlayers[playerIdx]->GetSource()->Properties(); >+ for (size_t propIdx = properties.Length(); propIdx-- != 0; ) { >+ nsCSSProperty prop = properties[propIdx].mProperty; >+ if (nsCSSProps::PropHasFlags(prop, >+ CSS_PROPERTY_CAN_ANIMATE_ON_COMPOSITOR) && >+ !propsHandled.HasProperty(prop)) { >+ propsHandled.AddProperty(prop); >+ for (size_t i = 0; i < ArrayLength(info); ++i) { >+ if (prop == info[i].mProperty) { >+ nsChangeHint changeHint = info[i].mChangeHint; >+ dom::Element* element = GetElementToRestyle(); >+ if (element) { >+ mManager->mPresContext->RestyleManager()-> >+ PostRestyleEvent(element, nsRestyleHint(0), changeHint); >+ } >+ break; I think this is fine, but I would find it a little easier to follow if we factored it out a bit so we don't have three levels of nested loops with a 'break' in one. Perhaps we could factor out a method like: static nsChangeHint* GetLayerChangeHintForProperty(nsCSSProperty aProperty)? We could also possibly negate the 'if (nsCSSProps::PropHasFlags...' condition and make it continue but I think something like GetLayerChangeHintForProperty would probably be enough.

(In reply to David Baron [:dbaron] ⏰UTC-7 from comment #24) > (In reply to Brian Birtles (:birtles) from comment #22) > > We should probably clarify these two seemingly contradictory sentences. I > > believe this is referring to the fact that although transitions are higher > > up in the cascade, they're not added if the property is undergoing a CSS > > Animation at the time. > > I updated the comment to read: > > // Transitions are the only non-!important level overriding > // animations in the cascade ordering. They also don't actually > // override animations, since transitions are suppressed when both > // are present. And since we might not have called > // UpdateCascadeResults (which updates when they are suppressed > // due to the presence of animations for the same element and > // property) for transitions yet (which will make their > // MapRuleInfoInto skip the properties that are currently > // animating), we should skip them explicitly. That's great. Thank you!

(In reply to David Baron [:dbaron] ⏰UTC-7 from comment #23) > (In reply to Brian Birtles (:birtles) from comment #20) > > Here, and a number of other places in this file, I think this should still > > pass with just 'yield waitForPaints()'? > > Yes, it does pass. I've made that change; I still need to double-check the > intermediate results (the patches removing the various todos) are all the > same. The intermediate results are still valid at patches 5, 6, 7, 8, 9, 11, and 12.

(In reply to Brian Birtles (:birtles) from comment #25) > Is this method name correct? It seems like two verbs in a row, or does > "resend" have another meaning here? PostRestyleToLayer? Maybe PostReaddToLayer, given that the method we're trying to trigger is nsDisplayListBuilder::AddAnimationsAndTransitionsToLayer ? Though I'm not crazy about Add there, either. > 'break' in one. Perhaps we could factor out a method like: > > static nsChangeHint* GetLayerChangeHintForProperty(nsCSSProperty > aProperty)? Yeah, I'll move the inner loop into such a method.

Comment on attachment 8585116 [details] [diff] [review] patch 6 - Set mWinsInCascade for CSS Animations >+ /* >+ * Set mWinsInCascade based both on what is overridden at levels >+ * higher than animations and based on one animation overriding >+ * another. >+ * >+ * We iterate from the last animation to the first, just like we do >+ * when calling ComposeStyle from >+ * AnimationPlayerCollection::EnsureStyleRuleFor. Later animations >+ * override earlier ones, so we add properties to the set of >+ * overridden properties as we encounter them, if the animation is >+ * currently filling. >+ */ Can we make this last sentence say "... if the animation is currently in effect"? Filling sounds like it refers only to when applying a backwards/forwards fill but I think we also add properties when the animation is actually animating? 'In effect' is the term used by Web Animations for this.[1] [1] http://w3c.github.io/web-animations/#in-effect >+ for (size_t propIdx = 0, propEnd = anim->Properties().Length(); >+ propIdx != propEnd; ++propIdx) { >+ AnimationProperty& prop = anim->Properties()[propIdx]; >+ // We only bother setting mWinsInCascade for properties that we >+ // can animate on the compositor. >+ if (nsCSSProps::PropHasFlags(prop.mProperty, >+ CSS_PROPERTY_CAN_ANIMATE_ON_COMPOSITOR)) { >+ bool newWinsInCascade = >+ !propertiesOverridden.HasProperty(prop.mProperty); >+ if (newWinsInCascade != prop.mWinsInCascade) { >+ changed = true; >+ } >+ prop.mWinsInCascade = newWinsInCascade; If I've understood this correctly, we could end up setting mWinsInCascade true for the same property in several different animations since we don't actually check if the animation is in effect until after we update mWinsInCascade. I guess that's fine since we're not going to try and send any animations that aren't in effect to the compositor but I just wanted to check I that I've understood this correctly. >+ ComputedTiming computedTiming = anim->GetComputedTiming(); >+ if (computedTiming.mTimeFraction != >+ ComputedTiming::kNullTimeFraction) { We can replace this with just: if (anim->IsInEffect()) { >+ // This animation is filling right now, so it overrides >+ // earlier animations. And make this, "This animation is in effect right now, ..." (although maybe we don't even need the comment now). >+ if (changed) { >+ nsPresContext* presContext = aElementAnimations->mManager->PresContext(); >+ presContext->RestyleManager()->IncrementAnimationGeneration(); >+ aElementAnimations->UpdateAnimationGeneration(presContext); >+ aElementAnimations->PostResendToLayer(); >+ >+ // Invalidate our style rule. >+ aElementAnimations->mNeedsRefreshes = true; >+ aElementAnimations->mStyleRuleRefreshTime = TimeStamp(); This is quite similar to calling aElementAnimations->NotifyPlayerUpdated but with a few differences. I think this is fine as-is, but in future we might be able to find a way to combine these somehow.

(In reply to David Baron [:dbaron] ⏰UTC-7 from comment #28) > (In reply to Brian Birtles (:birtles) from comment #25) > > Is this method name correct? It seems like two verbs in a row, or does > > "resend" have another meaning here? PostRestyleToLayer? > > Maybe PostReaddToLayer, given that the method we're trying to trigger is > nsDisplayListBuilder::AddAnimationsAndTransitionsToLayer ? Though I'm not > crazy about Add there, either. In that case, PostResendToLayer is fine with me.

Comment on attachment 8585117 [details] [diff] [review] patch 7 - Dynamically update cascade results when animations start or stop filling >+void >+nsAnimationManager::MaybeUpdateCascadeResults(AnimationPlayerCollection* aCollection) >+{ >+ for (size_t playerIdx = aCollection->mPlayers.Length(); playerIdx-- != 0; ) { >+ CSSAnimationPlayer* player = >+ aCollection->mPlayers[playerIdx]->AsCSSAnimationPlayer(); >+ const Animation* anim = player->GetSource(); >+ >+ bool filling = anim && >+ anim->GetComputedTiming().mTimeFraction != >+ ComputedTiming::kNullTimeFraction; >+ if (filling != player->mFillingForCascadeResults) { This could just be: if (player->HasInEffectSource()) { Then we can drop |anim| altogether I think. Also, as a general comment, as with the previous patch, we should refer to "in effect" rather than "filling". This applies to mFillingForCascadeResults and a number of other references. > for (size_t playerIdx = aElementAnimations->mPlayers.Length(); > playerIdx-- != 0; ) { >- AnimationPlayer* player = aElementAnimations->mPlayers[playerIdx]; >+ CSSAnimationPlayer* player = >+ aElementAnimations->mPlayers[playerIdx]->AsCSSAnimationPlayer(); > Animation* anim = player->GetSource(); >+ >+ player->mFillingForCascadeResults = >+ anim && >+ anim->GetComputedTiming().mTimeFraction != >+ ComputedTiming::kNullTimeFraction; >+ Again, this can just be player->HasInEffectSource();

Comment on attachment 8585123 [details] [diff] [review] patch 13 - Apply animations/transitions on the layer in the order that reflects how they override, rather than the opposite ># HG changeset patch ># User L. David Baron <dbaron@dbaron.org> ># Date 1427600062 25200 ># Sat Mar 28 20:34:22 2015 -0700 ># Node ID a58e1cf7eaf4618103cc7c825d22606891e82e41 ># Parent 12150489434db19fb7f25be3940ba01e13a57902 >Bug 847287 patch 13 - Apply animations/transitions on the layer in the order that reflects how they override, rather than the opposite. > >This independently would have fixed some of the problems fixed in this >bug. It would not have fixed them fully, though, since we always fill >animations on layers forwards in the expectation of another update (to >avoid jumping and then jumping back), so we need to send updates after >animations complete in order to allow the animations that they override >to start applying on the compositor. Shouldn't this be happening already? We already send a layer transaction when an animation completes at which point we should send the previously overridden animations minus any finished animations. (Sorry, I went away to lunch in the middle of reviewing this patch queue and I seem to have forgotten some important context!) >diff --git a/gfx/layers/composite/AsyncCompositionManager.cpp b/gfx/layers/composite/AsyncCompositionManager.cpp ... >- for (uint32_t i = animations.Length(); i-- !=0; ) { >+ // Process in reverse, since later animations override earlier ones. >+ for (size_t i = 0, iEnd = animations.Length(); i < iEnd; ++i) { Nit: Going 0 to end feels like going forwards to me, not reverse. This patch certainly doesn't do any harm, so r=birtles, but now I'm curious about which circumstances produce a change in the animation cascade and don't also trigger a layer transaction (which probably has less to do with this patch and more to do with part 7 and mFillingForCascadeResults).

(In reply to Brian Birtles (:birtles) from comment #32) > >This independently would have fixed some of the problems fixed in this > >bug. It would not have fixed them fully, though, since we always fill > >animations on layers forwards in the expectation of another update (to > >avoid jumping and then jumping back), so we need to send updates after > >animations complete in order to allow the animations that they override > >to start applying on the compositor. > > Shouldn't this be happening already? We already send a layer transaction > when an animation completes at which point we should send the previously > overridden animations minus any finished animations. Hmm. I think that's probably right, although I haven't tested. That said, I think it was my comment that was off. (That's sometimes what happens when I write the comments a week or more after writing the patch.) What this patch certainly wouldn't have fixed is the problem of animations that were overridden by !important rules; without the earlier patches we'd have sent such animations to the compositor, and they'd have ended up overriding. > Nit: Going 0 to end feels like going forwards to me, not reverse. Indeed.

(In reply to Brian Birtles (:birtles) from comment #25) > Comment on attachment 8585115 [details] [diff] [review] > patch 5 - Add method to update animations on layer > > 'break' in one. Perhaps we could factor out a method like: > > static nsChangeHint* GetLayerChangeHintForProperty(nsCSSProperty > aProperty)? I'm actually inclined to make the method return LayerAnimationRecord*, since that's more likely to be reusable. (In reply to Brian Birtles (:birtles) from comment #30) > (In reply to David Baron [:dbaron] ⏰UTC-7 from comment #28) > > Maybe PostReaddToLayer, given that the method we're trying to trigger is > > nsDisplayListBuilder::AddAnimationsAndTransitionsToLayer ? Though I'm not > > crazy about Add there, either. > > In that case, PostResendToLayer is fine with me. Maybe PostUpdateLayer, or PostUpdateLayerAnimations?

(In reply to Brian Birtles (:birtles) from comment #29) > If I've understood this correctly, we could end up setting mWinsInCascade > true for the same property in several different animations since we don't > actually check if the animation is in effect until after we update > mWinsInCascade. I guess that's fine since we're not going to try and send > any animations that aren't in effect to the compositor but I just wanted to > check I that I've understood this correctly. Yes, we set mWinsInCascade based on whether it would win if it were in effect, essentially. And, indeed, it shouldn't matter. > This is quite similar to calling aElementAnimations->NotifyPlayerUpdated but > with a few differences. I think this is fine as-is, but in future we might > be able to find a way to combine these somehow. Yes, there's some extra work that NotifyPlayerUpdated does that I didn't think we should be doing here.

https://hg.mozilla.org/integration/mozilla-inbound/rev/402d65395967 https://hg.mozilla.org/integration/mozilla-inbound/rev/347bb007150d https://hg.mozilla.org/integration/mozilla-inbound/rev/47c483fe563b https://hg.mozilla.org/integration/mozilla-inbound/rev/3a4b44544ad0 https://hg.mozilla.org/integration/mozilla-inbound/rev/5b1671857589 https://hg.mozilla.org/integration/mozilla-inbound/rev/de066f41c377 https://hg.mozilla.org/integration/mozilla-inbound/rev/2238d707a844 https://hg.mozilla.org/integration/mozilla-inbound/rev/a8dae2838831 https://hg.mozilla.org/integration/mozilla-inbound/rev/75a15c283fab https://hg.mozilla.org/integration/mozilla-inbound/rev/b9f6b4b17ff1 https://hg.mozilla.org/integration/mozilla-inbound/rev/aeed14f29025 https://hg.mozilla.org/integration/mozilla-inbound/rev/54cf27ab0936 https://hg.mozilla.org/integration/mozilla-inbound/rev/58277c3590bc

https://hg.mozilla.org/mozilla-central/rev/402d65395967 https://hg.mozilla.org/mozilla-central/rev/347bb007150d https://hg.mozilla.org/mozilla-central/rev/47c483fe563b https://hg.mozilla.org/mozilla-central/rev/3a4b44544ad0 https://hg.mozilla.org/mozilla-central/rev/5b1671857589 https://hg.mozilla.org/mozilla-central/rev/de066f41c377 https://hg.mozilla.org/mozilla-central/rev/2238d707a844 https://hg.mozilla.org/mozilla-central/rev/a8dae2838831 https://hg.mozilla.org/mozilla-central/rev/75a15c283fab https://hg.mozilla.org/mozilla-central/rev/b9f6b4b17ff1 https://hg.mozilla.org/mozilla-central/rev/aeed14f29025 https://hg.mozilla.org/mozilla-central/rev/54cf27ab0936 https://hg.mozilla.org/mozilla-central/rev/58277c3590bc