Bug 1518816 - Clarify when and why KeyframeEffect::HasEffectiveAnimationOfPropertySet might return false even when there are effective animations in a property set; r=boris

It took me a long time to understand why
KeyframeEffect::HasEffectiveAnimationOfPropertySet behaved so differently to
KeyframeEffect::HasAnimationOfPropertySet. This patch attempts to clarify that
while making KeyframeEffect::HasEffectiveAnimationOnPropertySet a little more
generally useful. This will allow us to tidy up the various animation checks in
nsLayoutUtils later in this patch series.

Ultimately, however, we should make this check part of the regular compositor
animation vetting machinery in bug 1534884. That should remove a number of
inconsistencies such that we don't need the extended comments added in this
patch.

Differential Revision: https://phabricator.services.mozilla.com/D23281

--HG--
extra : moz-landing-system : lando

dom/animation/KeyframeEffect.cpp

@@ -258,23 +258,36 @@ const AnimationProperty* KeyframeEffect::GetEffectiveAnimationOfProperty(
 }
 
 bool KeyframeEffect::HasEffectiveAnimationOfPropertySet(
-    const nsCSSPropertyIDSet& aPropertySet, const EffectSet& aEffect) const {
-  bool ret = false;
+    const nsCSSPropertyIDSet& aPropertySet, const EffectSet& aEffectSet) const {
+  // The various transform properties ('transform', 'scale' etc.) get combined
+  // on the compositor.
+  //
+  // As a result, if we have an animation of 'scale' and 'translate', but the
+  // 'translate' property is covered by an !important rule, we will not be
+  // able to combine the result on the compositor since we won't have the
+  // !important rule to incorporate. In that case we should run all the
+  // transform-related animations on the main thread (where we have the
+  // !important rule).
+  //
+  // Bug 1534884: Move this check to ShouldBlockAsyncTransformAnimations (or
+  // similar) and add a performance warning for this case.
+
+  bool result = false;
+
   for (const AnimationProperty& property : mProperties) {
     if (!aPropertySet.HasProperty(property.mProperty)) {
       continue;
     }
 
-    // Only consider the property if it is not overridden by !important rules in
-    // the transitions level. If one of the properties is overridden by
-    // !important rules, we return false. This is especially for transform-like
-    // properties because all of them should be running on the same thread.
-    if (!IsEffectiveProperty(aEffect, property.mProperty)) {
+    if (IsEffectiveProperty(aEffectSet, property.mProperty)) {
+      result = true;
+    } else if (nsCSSPropertyIDSet::TransformLikeProperties().HasProperty(
+                   property.mProperty)) {
       return false;
     }
-    ret = true;
   }
-  return ret;
+
+  return result;
 }
 
 nsCSSPropertyIDSet KeyframeEffect::GetPropertiesForCompositor(

dom/animation/KeyframeEffect.h

@@ -192,8 +192,9 @@ class KeyframeEffect : public AnimationEffect {
   // this function is typically called for all KeyframeEffects on an element
   // so that we can avoid multiple calls of EffectSet::GetEffect().
   //
-  // NOTE: We don't currently check for !important rules for properties that
-  // can't run on the compositor.
+  // NOTE: Unlike HasEffectiveAnimationOfPropertySet below, this function does
+  // not check for the effect of !important rules on animations of related
+  // transform properties.
   bool HasEffectiveAnimationOfProperty(nsCSSPropertyID aProperty,
                                        const EffectSet& aEffect) const {
     return GetEffectiveAnimationOfProperty(aProperty, aEffect) != nullptr;
@@ -201,12 +202,24 @@ class KeyframeEffect : public AnimationEffect {
   const AnimationProperty* GetEffectiveAnimationOfProperty(
       nsCSSPropertyID aProperty, const EffectSet& aEffect) const;
 
-  // This is a similar version as the above function, but for a
-  // nsCSSPropertyIDSet, and this returns true if this keyframe effect has
-  // properties in |aPropertySet| and if the properties are not overridden by
-  // !important rule or transition level.
+  // Similar to HasEffectiveAnimationOfProperty, above, but for
+  // an nsCSSPropertyIDSet. Returns true if this keyframe effect has at least
+  // one property in |aPropertySet| that is not overridden by an !important
+  // rule.
+  //
+  // Unlike HasEffectiveAnimationOfProperty, however, when |aPropertySet|
+  // includes transform-like properties (transform, rotate etc.) this function
+  // returns true if and only if all the transform-like properties that are
+  // present are effective.
+  //
+  // That is because the transform-like properties, unlike other properties, are
+  // combined on the compositor and if !important rules affect any of the
+  // individual properties we will not be able to correctly compose the result
+  // on the compositor so we should run the animations on the main thread
+  // instead.
   bool HasEffectiveAnimationOfPropertySet(
-      const nsCSSPropertyIDSet& aPropertySet, const EffectSet& aEffect) const;
+      const nsCSSPropertyIDSet& aPropertySet,
+      const EffectSet& aEffectSet) const;
 
   // Returns all the effective animated CSS properties that can be animated on
   // the compositor and are not overridden by a higher cascade level.

layout/base/nsLayoutUtils.cpp

@@ -247,9 +247,11 @@ static bool MayHaveAnimationOfPropertySet(
     return aTarget->MayHaveOpacityAnimation();
   }
 
-  MOZ_ASSERT(aPropertySet.Equals(nsCSSPropertyIDSet::TransformLikeProperties()),
-             "Should equal to transform-like properties at this branch");
-  return aTarget->MayHaveTransformAnimation();
+  if (aPropertySet.Equals(nsCSSPropertyIDSet::TransformLikeProperties())) {
+    return aTarget->MayHaveTransformAnimation();
+  }
+
+  return true;
 }
 
 template <typename EffectSetOrFrame>

layout/base/nsLayoutUtils.h

@@ -2307,16 +2307,26 @@ class nsLayoutUtils {
                                     nsCSSPropertyID aProperty);
 
   /**
-   * Returns true if |aFrame| has animations of properties in |aPropertySet|,
-   * and all of these properties are not overridden by !important rules.
+   * Returns true if |aFrame| has an animation where at least one of the
+   * properties in |aPropertySet| is not overridden by !important rules.
+   *
+   * If |aPropertySet| includes transform-like properties (transform, rotate,
+   * etc.) however, this will return false if any of the transform-like
+   * properties is overriden by an !important rule since these properties should
+   * be combined on the compositor.
    */
   static bool HasEffectiveAnimation(const nsIFrame* aFrame,
                                     const nsCSSPropertyIDSet& aPropertySet);
 
   /**
-   * Returns all effective animated CSS properties on |aFrame|. That means
-   * properties that can be animated on the compositor and are not overridden by
-   * a higher cascade level.
+   * Returns all effective animated CSS properties on |aFrame|. Properties that
+   * can be animated on the compositor but which are overridden by !important
+   * rules are not returned.
+   *
+   * Unlike HasEffectiveAnimation, however, this does not check the set of
+   * transform-like properties to ensure that if any such properties are
+   * overridden by !important rules, the other transform-like properties are
+   * not run on the compositor (see bug 1534884).
    */
   static nsCSSPropertyIDSet GetAnimationPropertiesForCompositor(
       const nsIFrame* aFrame);

layout/painting/ActiveLayerTracker.cpp

@@ -449,9 +449,8 @@ bool ActiveLayerTracker::IsBackgroundPositionAnimated(
     }
   }
   return nsLayoutUtils::HasEffectiveAnimation(
-             aFrame, eCSSProperty_background_position_x) ||
-         nsLayoutUtils::HasEffectiveAnimation(
-             aFrame, eCSSProperty_background_position_y);
+      aFrame, nsCSSPropertyIDSet({eCSSProperty_background_position_x,
+                                  eCSSProperty_background_position_y}));
 }
 
 /* static */