Bug 1278136 - Part 5: Create a stacking context for opacity/transform animations even if it's in delay phase and even if the property is overridden by !important rules. r=birtles

This patch introduces a new functions named HasEffectiveAnimationOfProperty.
This function checks that a given CSS property is overridden by !important
rules.
On the other hand, now KeyframeEffetReadOnly::HasAnimationOfProperty() does
just check that the effect has a given CSS property.  This is used to create
a stacking context because we should create a stacking context for opacity or
transform animations even if the property is overridden by !important rules.

MozReview-Commit-ID: AG1Y0IgoB3U

dom/animation/EffectCompositor.cpp

@@ -148,7 +148,7 @@ FindAnimationsForCompositor(const nsIFrame* aFrame,
       return false;
     }
 
-    if (!effect->HasAnimationOfProperty(aProperty)) {
+    if (!effect->HasEffectiveAnimationOfProperty(aProperty)) {
       continue;
     }
 

dom/animation/KeyframeEffectReadOnly.cpp

@@ -192,7 +192,8 @@ KeyframeEffectReadOnly::SetKeyframes(nsTArray<Keyframe>&& aKeyframes,
 }
 
 const AnimationProperty*
-KeyframeEffectReadOnly::GetAnimationOfProperty(nsCSSPropertyID aProperty) const
+KeyframeEffectReadOnly::GetEffectiveAnimationOfProperty(
+  nsCSSPropertyID aProperty) const
 {
   if (!IsInEffect()) {
     return nullptr;
@@ -219,6 +220,17 @@ KeyframeEffectReadOnly::GetAnimationOfProperty(nsCSSPropertyID aProperty) const
   return nullptr;
 }
 
+bool
+KeyframeEffectReadOnly::HasAnimationOfProperty(nsCSSPropertyID aProperty) const
+{
+  for (const AnimationProperty& property : mProperties) {
+    if (property.mProperty == aProperty) {
+      return true;
+    }
+  }
+  return false;
+}
+
 #ifdef DEBUG
 bool
 SpecifiedKeyframeArraysAreEqual(const nsTArray<Keyframe>& aA,
@@ -926,10 +938,12 @@ KeyframeEffectReadOnly::CanThrottle() const
   // already running on compositor.
   for (const LayerAnimationInfo::Record& record :
         LayerAnimationInfo::sRecords) {
-    // Skip properties that are overridden in the cascade.
+    // Skip properties that are overridden by !important rules.
-    // (GetAnimationOfProperty, as called by HasAnimationOfProperty,
+    // (GetEffectiveAnimationOfProperty, as called by
-    // only returns an animation if it currently wins in the cascade.)
+    // HasEffectiveAnimationOfProperty, only returns a property which is
-    if (!HasAnimationOfProperty(record.mProperty)) {
+    // neither overridden by !important rules nor overridden by other
+    // animation.)
+    if (!HasEffectiveAnimationOfProperty(record.mProperty)) {
       continue;
     }
 

dom/animation/KeyframeEffectReadOnly.h

@@ -235,12 +235,25 @@ public:
                     ErrorResult& aRv);
   void SetKeyframes(nsTArray<Keyframe>&& aKeyframes,
                     nsStyleContext* aStyleContext);
-  const AnimationProperty*
+
-  GetAnimationOfProperty(nsCSSPropertyID aProperty) const;
+  // Returns true if the effect includes |aProperty| regardless of whether the
-  bool HasAnimationOfProperty(nsCSSPropertyID aProperty) const
+  // property is overridden by !important rule.
+  bool HasAnimationOfProperty(nsCSSPropertyID aProperty) const;
+
+  // GetEffectiveAnimationOfProperty returns AnimationProperty corresponding
+  // to a given CSS property if the effect includes the property and the
+  // property is not overridden by !important rules.
+  // Also EffectiveAnimationOfProperty returns true under the same condition.
+  //
+  // NOTE: We don't currently check for !important rules for properties that
+  // can't run on the compositor.
+  bool HasEffectiveAnimationOfProperty(nsCSSPropertyID aProperty) const
   {
-    return GetAnimationOfProperty(aProperty) != nullptr;
+    return GetEffectiveAnimationOfProperty(aProperty) != nullptr;
   }
+  const AnimationProperty* GetEffectiveAnimationOfProperty(
+    nsCSSPropertyID aProperty) const;
+
   const InfallibleTArray<AnimationProperty>& Properties() const
   {
     return mProperties;

layout/base/RestyleManager.cpp

@@ -1569,8 +1569,7 @@ ElementRestyler::AddLayerChangesForAnimation()
     // setKeyframes or changing target element from other target which prevents
     // running on the compositor, etc.
     if (!layer &&
-        nsLayoutUtils::HasRelevantAnimationOfProperty(mFrame,
+        nsLayoutUtils::HasEffectiveAnimation(mFrame, layerInfo.mProperty)) {
-                                                      layerInfo.mProperty)) {
       hint |= layerInfo.mChangeHint;
     }
   }

layout/base/nsDisplayList.cpp

@@ -487,21 +487,21 @@ AddAnimationsForProperty(nsIFrame* aFrame, nsCSSPropertyID aProperty,
     MOZ_ASSERT(keyframeEffect,
                "A playing animation should have a keyframe effect");
     const AnimationProperty* property =
-      keyframeEffect->GetAnimationOfProperty(aProperty);
+      keyframeEffect->GetEffectiveAnimationOfProperty(aProperty);
     if (!property) {
       continue;
     }
 
     // Note that if the property is overridden by !important rules,
-    // GetAnimationOfProperty returns null instead.
+    // GetEffectiveAnimationOfProperty returns null instead.
     // This is what we want, since if we have animations overridden by
     // !important rules, we don't want to send them to the compositor.
     MOZ_ASSERT(anim->CascadeLevel() !=
                  EffectCompositor::CascadeLevel::Animations ||
                !effects->PropertiesWithImportantRules()
                   .HasProperty(aProperty),
-               "GetAnimationOfProperty already tested the property is not "
+               "GetEffectiveAnimationOfProperty already tested the property "
-               "overridden by !important rules");
+               "is not overridden by !important rules");
 
     // Don't add animations that are pending if their timeline does not
     // track wallclock time. This is because any pending animations on layers

layout/base/nsLayoutUtils.cpp

@@ -483,7 +483,7 @@ nsLayoutUtils::HasActiveAnimationOfProperty(const nsIFrame* aFrame,
     [&aProperty](KeyframeEffectReadOnly& aEffect)
     {
       return aEffect.IsCurrent() && aEffect.IsInEffect() &&
-        aEffect.HasAnimationOfProperty(aProperty);
+        aEffect.HasEffectiveAnimationOfProperty(aProperty);
     }
   );
 }
@@ -502,8 +502,8 @@ nsLayoutUtils::HasCurrentTransitions(const nsIFrame* aFrame)
 }
 
 bool
-nsLayoutUtils::HasRelevantAnimationOfProperty(const nsIFrame* aFrame,
+nsLayoutUtils::HasAnimationOfProperty(const nsIFrame* aFrame,
-                                              nsCSSPropertyID aProperty)
+                                      nsCSSPropertyID aProperty)
 {
   return HasMatchingAnimations(aFrame,
     [&aProperty](KeyframeEffectReadOnly& aEffect)
@@ -514,6 +514,19 @@ nsLayoutUtils::HasRelevantAnimationOfProperty(const nsIFrame* aFrame,
   );
 }
 
+bool
+nsLayoutUtils::HasEffectiveAnimation(const nsIFrame* aFrame,
+                                     nsCSSPropertyID aProperty)
+{
+  return HasMatchingAnimations(aFrame,
+    [&aProperty](KeyframeEffectReadOnly& aEffect)
+    {
+      return (aEffect.IsInEffect() || aEffect.IsCurrent()) &&
+             aEffect.HasEffectiveAnimationOfProperty(aProperty);
+    }
+  );
+}
+
 static float
 GetSuitableScale(float aMaxScale, float aMinScale,
                  nscoord aVisibleDimension, nscoord aDisplayDimension)

layout/base/nsLayoutUtils.h

@@ -2244,12 +2244,18 @@ public:
   static bool HasCurrentTransitions(const nsIFrame* aFrame);
 
   /**
-   * Returns true if the frame has current or in-effect (i.e. in before phase,
+   * Returns true if |aFrame| has an animation of |aProperty| regardless of
-   * running or filling) animations or transitions for the
+   * whether the property is overridden by !important rule.
-   * property.
    */
-  static bool HasRelevantAnimationOfProperty(const nsIFrame* aFrame,
+  static bool HasAnimationOfProperty(const nsIFrame* aFrame,
-                                             nsCSSPropertyID aProperty);
+                                     nsCSSPropertyID aProperty);
+
+  /**
+   * Returns true if |aFrame| has an animation of |aProperty| which is
+   * not overridden by !important rules.
+   */
+  static bool HasEffectiveAnimation(const nsIFrame* aFrame,
+                                    nsCSSPropertyID aProperty);
 
   /**
    * Checks if off-main-thread animations are enabled.

layout/generic/nsFrame.cpp

@@ -554,8 +554,7 @@ nsFrame::Init(nsIContent*       aContent,
   }
   const nsStyleDisplay *disp = StyleDisplay();
   if (disp->HasTransform(this) ||
-      nsLayoutUtils::HasRelevantAnimationOfProperty(this,
+      nsLayoutUtils::HasAnimationOfProperty(this, eCSSProperty_transform)) {
-                                                    eCSSProperty_transform)) {
     // The frame gets reconstructed if we toggle the -moz-transform
     // property, so we can set this bit here and then ignore it.
     mState |= NS_FRAME_MAY_BE_TRANSFORMED;
@@ -1140,8 +1139,8 @@ nsIFrame::IsTransformed() const
           (StyleDisplay()->HasTransform(this) ||
            IsSVGTransformed() ||
            (mContent &&
-            nsLayoutUtils::HasRelevantAnimationOfProperty(
+            nsLayoutUtils::HasAnimationOfProperty(this,
-              this, eCSSProperty_transform) &&
+                                                  eCSSProperty_transform) &&
             IsFrameOfType(eSupportsCSSTransforms) &&
             mContent->GetPrimaryFrame() == this)));
 }
@@ -1153,8 +1152,7 @@ nsIFrame::HasOpacityInternal(float aThreshold) const
   return StyleEffects()->mOpacity < aThreshold ||
          (StyleDisplay()->mWillChangeBitField & NS_STYLE_WILL_CHANGE_OPACITY) ||
          (mContent &&
-           nsLayoutUtils::HasRelevantAnimationOfProperty(
+           nsLayoutUtils::HasAnimationOfProperty(this, eCSSProperty_opacity) &&
-             this, eCSSProperty_opacity) &&
            mContent->GetPrimaryFrame() == this);
 }
 
@@ -2115,8 +2113,7 @@ nsIFrame::BuildDisplayListForStackingContext(nsDisplayListBuilder* aBuilder,
   bool opacityItemForEventsAndPluginsOnly = false;
   if (effects->mOpacity == 0.0 && aBuilder->IsForPainting() &&
       !(disp->mWillChangeBitField & NS_STYLE_WILL_CHANGE_OPACITY) &&
-      !nsLayoutUtils::HasActiveAnimationOfProperty(this,
+      !nsLayoutUtils::HasAnimationOfProperty(this, eCSSProperty_opacity)) {
-                                                   eCSSProperty_opacity)) {
     if (needEventRegions ||
         aBuilder->WillComputePluginGeometry()) {
       opacityItemForEventsAndPluginsOnly = true;

layout/reftests/css-animations/reftest.list

@@ -14,8 +14,8 @@ test-pref(layers.offmainthreadcomposition.async-animations,false) == stacking-co
 test-pref(layers.offmainthreadcomposition.async-animations,false) == stacking-context-transform-none-animation.html stacking-context-animation-ref.html
 == no-stacking-context-opacity-removing-animation-in-delay.html no-stacking-context-animation-ref.html
 == no-stacking-context-transform-removing-animation-in-delay.html no-stacking-context-animation-ref.html
-fails == stacking-context-lose-opacity-1.html stacking-context-animation-ref.html
+== stacking-context-lose-opacity-1.html stacking-context-animation-ref.html
-fails == stacking-context-lose-transform-none.html stacking-context-animation-ref.html
+== stacking-context-lose-transform-none.html stacking-context-animation-ref.html
 == stacking-context-opacity-win-in-delay.html stacking-context-animation-ref.html
 == stacking-context-opacity-win-in-delay-on-main-thread.html stacking-context-animation-ref.html
 == stacking-context-opacity-wins-over-transition.html stacking-context-animation-ref.html
@@ -33,10 +33,10 @@ fails == stacking-context-lose-transform-none.html stacking-context-animation-re
 == stacking-context-transform-none-animation-with-preserve-3d.html stacking-context-animation-ref.html
 == stacking-context-transform-none-with-fill-backwards.html stacking-context-animation-ref.html
 == stacking-context-transform-none-with-fill-forwards.html stacking-context-animation-ref.html
-fails == stacking-context-opacity-1-in-delay.html stacking-context-animation-ref.html # bug 1278136 and bug 1279403
+== stacking-context-opacity-1-in-delay.html stacking-context-animation-ref.html
-fails == stacking-context-opacity-removing-important-in-delay.html stacking-context-animation-ref.html
+== stacking-context-opacity-removing-important-in-delay.html stacking-context-animation-ref.html
-fails == stacking-context-transform-none-in-delay.html stacking-context-animation-ref.html # bug 1278136 and bug 1279403
+== stacking-context-transform-none-in-delay.html stacking-context-animation-ref.html
-fails == stacking-context-transform-removing-important-in-delay.html stacking-context-animation-ref.html
+== stacking-context-transform-removing-important-in-delay.html stacking-context-animation-ref.html
 == background-position-in-delay.html background-position-ref.html
 == background-position-after-finish.html background-position-ref.html
 fails == background-position-running.html background-position-ref.html # This test fails the reftest-opaque-layer check since animating background-position currently creates an active layer, and reftest-opaque-layer only handles items assigned to PaintedLayers.

layout/reftests/css-transitions/reftest.list

@@ -2,7 +2,7 @@
 == transitions-inline-already-wrapped-2.html transitions-inline-ref.html
 == transitions-inline-rewrap-1.html transitions-inline-ref.html
 == transitions-inline-rewrap-2.html transitions-inline-ref.html
-fails == stacking-context-opacity-lose-to-animation.html stacking-context-transition-ref.html
+== stacking-context-opacity-lose-to-animation.html stacking-context-transition-ref.html
-fails == stacking-context-transform-lose-to-animation.html stacking-context-transition-ref.html
+== stacking-context-transform-lose-to-animation.html stacking-context-transition-ref.html
 == stacking-context-opacity-wins-over-important-style.html stacking-context-transition-ref.html
 == stacking-context-transform-wins-over-important-style.html stacking-context-transition-ref.html