Bug 1405548 - Post restyles when creating or removing new CSS animations when using the Servo backend; r?hiro
As explained in the extended comment in this patch, for Servo we want to post
restyles when creating new animations so that we run a second animation
restyle and incorporate the result of new animations into style immediately.
(Gecko does everything in the one restyle, and although this causes other bugs
related to triggering transitions, at least it means it does not require
restyles to be posted here).
It turns out that we normally end up posting a restyle anyway in
CSSAnimation::SetAnimationIndex.
Bug 1332958 was supposed to drop that but it
never landed.
However, CSSAnimation::SetAnimationIndex only posts a restyle when there is
a change to the animation index. It turns out that, by chance, there normally
*is* a change to a CSSAnimation's animation index when it is created. Initially
it takes its animation index from Animation::sNextAnimationIndex which is
incremented each time it is assigned to an animation.
If the first Animation we create for a given content process is a CSSAnimation
then sNextAnimationIndex will be zero and so we will initially assign an
animation index of zero. If that CSS animation is also the first in the list
of animations in animation-name, when we call SetAnimationIndex we will pass
zero as the index to use, and when we go to update the animation index we will
detect that there is no change, and will NOT post an animation restyle.
As a result the target element's style will NOT reflect the animated style.
To fix this we need to ensure that *new* CSS animations trigger a restyle.
For *changes* to animations, the corresponding calls to SetKeyframes and
SetSpecifiedTiming post restyles so the behavior should be correct in those
cases.
For *removed* animations I observed that in at least some cases we successfully
post a restyle. However, this appeared to be as much by chance as anything so
this patch also posts a restyle for removed animations. (Note that the
EffectCompositor will ignore redundant restyle requests so this is ok.)
This patch deliberately does not expose Animation::PostUpdate and call that
because the code introduced here is intended to be temporary. Long-term we
should remove the Gecko style backend and allow the calls to PlayFromStyle,
PauseFromStyle, CancelFromStyle etc. to post restyles just like calls to Play,
Pause, and Cancel do. At that point this code can also be removed.
MozReview-Commit-ID: 4c3vJdLBqeY
--- a/layout/style/nsAnimationManager.cpp
+++ b/layout/style/nsAnimationManager.cpp
@@ -423,16 +423,55 @@ public:
aKeyframes);
}
void SetKeyframes(KeyframeEffectReadOnly& aEffect,
nsTArray<Keyframe>&& aKeyframes)
{
aEffect.SetKeyframes(Move(aKeyframes), mStyleContext);
}
+ // Currently all the animation building code in this file is based on
+ // assumption that creating and removing animations should *not* trigger
+ // additional restyles since those changes will be handled within the same
+ // restyle.
+ //
+ // While that is true for the Gecko style backend, it is not true for the
+ // Servo style backend where we want restyles to be triggered so that we
+ // perform a second animation restyle where we will incorporate the changes
+ // arising from creating and removing animations.
+ //
+ // Fortunately, our attempts to avoid posting extra restyles as part of the
+ // processing here are imperfect and most of the time we happen to post
+ // them anyway. Occasionally, however, we don't. For example, we don't post
+ // a restyle when we create a new animation whose an animation index matches
+ // the default value it was given already (which is typically only true when
+ // the CSSAnimation we create is the first Animation created in a particular
+ // content process).
+ //
+ // As a result, when we are using the Servo backend, whenever we have an added
+ // or removed animation we need to explicitly trigger a restyle.
+ //
+ // This code should eventually disappear along with the Gecko style backend
+ // and we should simply call Play() / Pause() / Cancel() etc. which will
+ // post the required restyles.
+ void NotifyNewOrRemovedAnimation(const Animation& aAnimation)
+ {
+ AnimationEffectReadOnly* effect = aAnimation.GetEffect();
+ if (!effect) {
+ return;
+ }
+
+ KeyframeEffectReadOnly* keyframeEffect = effect->AsKeyframeEffect();
+ if (!keyframeEffect) {
+ return;
+ }
+
+ keyframeEffect->RequestRestyle(EffectCompositor::RestyleType::Standard);
+ }
+
private:
const ServoStyleContext* mStyleContext;
};
class MOZ_STACK_CLASS GeckoCSSAnimationBuilder final {
public:
GeckoCSSAnimationBuilder(GeckoStyleContext* aStyleContext,
const NonOwningAnimationTarget& aTarget)
@@ -447,16 +486,18 @@ public:
const StyleAnimation& aSrc,
nsTArray<Keyframe>& aKeyframs);
void SetKeyframes(KeyframeEffectReadOnly& aEffect,
nsTArray<Keyframe>&& aKeyframes)
{
aEffect.SetKeyframes(Move(aKeyframes), mStyleContext);
}
+ void NotifyNewOrRemovedAnimation(const Animation&) {}
+
private:
nsTArray<Keyframe> BuildAnimationFrames(nsPresContext* aPresContext,
const StyleAnimation& aSrc,
const nsCSSKeyframesRule* aRule);
Maybe<ComputedTimingFunction> GetKeyframeTimingFunction(
nsPresContext* aPresContext,
nsCSSKeyframeRule* aKeyframeRule,
const Maybe<ComputedTimingFunction>& aInheritedTimingFunction);
@@ -600,16 +641,18 @@ BuildAnimation(nsPresContext* aPresConte
animation->SetEffectNoUpdate(effect);
if (isStylePaused) {
animation->PauseFromStyle();
} else {
animation->PlayFromStyle();
}
+ aBuilder.NotifyNewOrRemovedAnimation(*animation);
+
return animation.forget();
}
bool
GeckoCSSAnimationBuilder::BuildKeyframes(nsPresContext* aPresContext,
const StyleAnimation& aSrc,
nsTArray<Keyframe>& aKeyframes)
{
@@ -1094,11 +1137,12 @@ nsAnimationManager::DoUpdateAnimations(
if (createdCollection) {
AddElementCollection(collection);
}
}
collection->mAnimations.SwapElements(newAnimations);
// Cancel removed animations
for (size_t newAnimIdx = newAnimations.Length(); newAnimIdx-- != 0; ) {
+ aBuilder.NotifyNewOrRemovedAnimation(*newAnimations[newAnimIdx]);
newAnimations[newAnimIdx]->CancelFromStyle();
}
}