Bug 1405548 - Post restyles when creating or removing new CSS animations when using the Servo backend; r?hiro draft
authorBrian Birtles <birtles@gmail.com>
Wed, 11 Oct 2017 16:36:36 +0900
changeset 678328 c036b6169d20888dac3ad9cac0328b4c0cd8c197
parent 678275 e897e367d3bd489422d86fbdfac54925c18329d2
child 735299 e7b10bfec4df58e1d506d95d5d07d41b4c80a351
push id83891
push userbmo:bbirtles@mozilla.com
push dateWed, 11 Oct 2017 07:38:45 +0000
reviewershiro
bugs1405548, 1332958
milestone58.0a1
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
layout/style/nsAnimationManager.cpp
--- 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();
   }
 }