Bug 1534884 - Add new animation warning for animations overridden by important rules. r=birtles

We move the check of important rule and animation level into
KeyframeEffect::ShouldBlockAsyncTransformAnimations(), and add a new warning
for it.

Note:
1. ShouldBlockAsyncTransformAnimations() only cares about transforms. And
   for other compositor animation properties, we count on
   HasEffectiveAnimationOfPropertySet() (in IsMatchForCompositor()).
2. If we check the important rules in both
   EffectCompositor::HasAnimationsForCompositor() and
   ActiveLayerTracker::IsTransformMaybeAnimated(), we may get the incorrect
   animation warnings (i.e. TransformFrameInactive). In most cases, we
   check these two functions together, so perhaps move the check of important
   rules outside HasEffectiveAnimationOfPropertySet() is fine.
   Besides, ActiveLayerTracker just tracks if there is a style change on this
   property (or display item) on the active layers, so should be OK to not
   check important rules in it.

So IsMatchForCompositor() should check all transform-like properties,
instead of each one, to get the correct result. (That's why we have to
refactor KeyframeEffect::GetPropertiesForCompositor() as well.)

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

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

dom/animation/Animation.cpp

@@ -964,7 +964,7 @@ bool Animation::ShouldBeSynchronizedWithMainThread(
   }
 
   return keyframeEffect->ShouldBlockAsyncTransformAnimations(
-      aFrame, aPerformanceWarning);
+      aFrame, aPropertySet, aPerformanceWarning);
 }
 
 void Animation::UpdateRelevance() {

dom/animation/AnimationPerformanceWarning.cpp

@@ -59,6 +59,9 @@ bool AnimationPerformanceWarning::ToLocalizedString(
     case Type::TransformFrameInactive:
       key = "CompositorAnimationWarningTransformFrameInactive";
       break;
+    case Type::TransformIsBlockedByImportantRules:
+      key = "CompositorAnimationWarningTransformIsBlockedByImportantRules";
+      break;
     case Type::OpacityFrameInactive:
       key = "CompositorAnimationWarningOpacityFrameInactive";
       break;

dom/animation/AnimationPerformanceWarning.h

@@ -27,6 +27,7 @@ struct AnimationPerformanceWarning {
     TransformWithGeometricProperties,
     TransformWithSyncGeometricAnimations,
     TransformFrameInactive,
+    TransformIsBlockedByImportantRules,
     OpacityFrameInactive,
     HasRenderingObserver,
   };

dom/animation/EffectCompositor.cpp

@@ -138,14 +138,6 @@ bool FindAnimationsForCompositor(
     }
   }
 
-  // If the property will be added to the animations level of the cascade but
-  // there is an !important rule for that property in the cascade then the
-  // animation will not be applied since the !important rule overrides it.
-  if (effects->PropertiesWithImportantRules().Intersects(aPropertySet) &&
-      effects->PropertiesForAnimationsLevel().Intersects(aPropertySet)) {
-    return false;
-  }
-
   AnimationPerformanceWarning::Type warning =
       AnimationPerformanceWarning::Type::None;
   if (!EffectCompositor::AllowCompositorAnimationsOnFrame(aFrame, warning)) {

dom/animation/KeyframeEffect.cpp

@@ -279,35 +279,13 @@ const AnimationProperty* KeyframeEffect::GetEffectiveAnimationOfProperty(
 
 bool KeyframeEffect::HasEffectiveAnimationOfPropertySet(
     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;
-    }
-
-    if (IsEffectiveProperty(aEffectSet, property.mProperty)) {
-      result = true;
-    } else if (nsCSSPropertyIDSet::TransformLikeProperties().HasProperty(
-                   property.mProperty)) {
-      return false;
+    if (aPropertySet.HasProperty(property.mProperty) &&
+        IsEffectiveProperty(aEffectSet, property.mProperty)) {
+      return true;
     }
   }
-
-  return result;
+  return false;
 }
 
 nsCSSPropertyIDSet KeyframeEffect::GetPropertiesForCompositor(
@@ -323,22 +301,44 @@ nsCSSPropertyIDSet KeyframeEffect::GetPropertiesForCompositor(
 
   static constexpr nsCSSPropertyIDSet compositorAnimatables =
       nsCSSPropertyIDSet::CompositorAnimatables();
+  static constexpr nsCSSPropertyIDSet transformLikeProperties =
+      nsCSSPropertyIDSet::TransformLikeProperties();
+
+  nsCSSPropertyIDSet transformSet;
+  AnimationPerformanceWarning::Type dummyWarning;
+
   for (const AnimationProperty& property : mProperties) {
     if (!compositorAnimatables.HasProperty(property.mProperty)) {
       continue;
     }
 
-    AnimationPerformanceWarning::Type warning;
+    // Transform-like properties are combined together on the compositor so we
+    // need to evaluate them as a group. We build up a separate set here then
+    // evaluate it as a separate step below.
+    if (transformLikeProperties.HasProperty(property.mProperty)) {
+      transformSet.AddProperty(property.mProperty);
+      continue;
+    }
+
     KeyframeEffect::MatchForCompositor matchResult = IsMatchForCompositor(
-        nsCSSPropertyIDSet{property.mProperty}, aFrame, aEffects, warning);
+        nsCSSPropertyIDSet{property.mProperty}, aFrame, aEffects, dummyWarning);
     if (matchResult ==
             KeyframeEffect::MatchForCompositor::NoAndBlockThisProperty ||
         matchResult == KeyframeEffect::MatchForCompositor::No) {
       continue;
     }
-
     properties.AddProperty(property.mProperty);
   }
+
+  if (!transformSet.IsEmpty()) {
+    KeyframeEffect::MatchForCompositor matchResult =
+        IsMatchForCompositor(transformSet, aFrame, aEffects, dummyWarning);
+    if (matchResult == KeyframeEffect::MatchForCompositor::Yes ||
+        matchResult == KeyframeEffect::MatchForCompositor::IfNeeded) {
+      properties |= transformSet;
+    }
+  }
+
   return properties;
 }
 
@@ -1495,10 +1495,28 @@ bool KeyframeEffect::CanAnimateTransformOnCompositor(
 }
 
 bool KeyframeEffect::ShouldBlockAsyncTransformAnimations(
-    const nsIFrame* aFrame,
+    const nsIFrame* aFrame, const nsCSSPropertyIDSet& aPropertySet,
     AnimationPerformanceWarning::Type& aPerformanceWarning /* out */) const {
   EffectSet* effectSet =
       EffectSet::GetEffectSet(mTarget->mElement, mTarget->mPseudoType);
+  // 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).
+  nsCSSPropertyIDSet blockedProperties =
+      effectSet->PropertiesWithImportantRules().Intersect(
+          effectSet->PropertiesForAnimationsLevel());
+  if (blockedProperties.Intersects(aPropertySet)) {
+    aPerformanceWarning =
+        AnimationPerformanceWarning::Type::TransformIsBlockedByImportantRules;
+    return true;
+  }
+
   for (const AnimationProperty& property : mProperties) {
     // If there is a property for animations level that is overridden by
     // !important rules, it should not block other animations from running

dom/animation/KeyframeEffect.h

@@ -202,9 +202,10 @@ 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: Unlike HasEffectiveAnimationOfPropertySet below, this function does
-  // not check for the effect of !important rules on animations of related
-  // transform properties.
+  // Note that does not consider the interaction between related transform
+  // properties where an !important rule on another transform property may
+  // cause all transform properties to be run on the main thread. That check is
+  // performed by GetPropertiesForCompositor.
   bool HasEffectiveAnimationOfProperty(nsCSSPropertyID aProperty,
                                        const EffectSet& aEffect) const {
     return GetEffectiveAnimationOfProperty(aProperty, aEffect) != nullptr;
@@ -217,16 +218,10 @@ class KeyframeEffect : public AnimationEffect {
   // 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.
+  // Note that does not consider the interaction between related transform
+  // properties where an !important rule on another transform property may
+  // cause all transform properties to be run on the main thread. That check is
+  // performed by GetPropertiesForCompositor.
   bool HasEffectiveAnimationOfPropertySet(
       const nsCSSPropertyIDSet& aPropertySet,
       const EffectSet& aEffectSet) const;
@@ -282,7 +277,7 @@ class KeyframeEffect : public AnimationEffect {
   // When returning true, |aPerformanceWarning| stores the reason why
   // we shouldn't run the transform animations.
   bool ShouldBlockAsyncTransformAnimations(
-      const nsIFrame* aFrame,
+      const nsIFrame* aFrame, const nsCSSPropertyIDSet& aPropertySet,
       AnimationPerformanceWarning::Type& aPerformanceWarning /* out */) const;
   bool HasGeometricProperties() const;
   bool AffectsGeometry() const override {

dom/animation/test/chrome/test_animation_performance_warning.html

@@ -1602,6 +1602,48 @@ function testTransformSVG() {
   });
 }
 
+function testImportantRuleOverride() {
+  promise_test(async t => {
+    const elem = addDiv(t, { class: 'compositable' });
+    const anim = elem.animate({ translate: [ '0px', '100px' ],
+                                rotate: ['0deg', '90deg'] },
+                                100 * MS_PER_SEC);
+
+    await waitForAnimationReadyToRestyle(anim);
+    await waitForPaints();
+
+    assert_animation_property_state_equals(
+      anim.effect.getProperties(),
+      [ { property: 'translate', runningOnCompositor: true },
+        { property: 'rotate', runningOnCompositor: true } ]
+    );
+
+    elem.style.setProperty('rotate', '45deg', 'important');
+    getComputedStyle(elem).rotate;
+
+    await waitForFrame();
+
+    assert_animation_property_state_equals(
+      anim.effect.getProperties(),
+      [
+        {
+          property: 'translate',
+          runningOnCompositor: false,
+          warning:
+            'CompositorAnimationWarningTransformIsBlockedByImportantRules'
+        },
+        {
+          property: 'rotate',
+          runningOnCompositor: false,
+          warning:
+            'CompositorAnimationWarningTransformIsBlockedByImportantRules'
+        },
+      ]
+    );
+  }, 'The animations of transform-like properties are not running on the ' +
+     'compositor because any of the properties has important rules');
+}
+
 function start() {
   var bundleService = SpecialPowers.Cc['@mozilla.org/intl/stringbundle;1']
     .getService(SpecialPowers.Ci.nsIStringBundleService);
@@ -1620,6 +1662,7 @@ function start() {
   testSynchronizedAnimations();
   testTooLargeFrame();
   testTransformSVG();
+  testImportantRuleOverride();
 
   promise_test(async t => {
     var div = addDiv(t, { class: 'compositable',

dom/animation/test/chrome/test_running_on_compositor.html

@@ -1130,8 +1130,8 @@ promise_test(async t => {
   await waitForPaints();
 
   assert_animation_is_not_running_on_compositor(animation,
-     'Animation overridden by an !important rule reports that it is ' +
-     'NOT running on the compositor');
+    'Animation overridden by an !important rule reports that it is ' +
+    'NOT running on the compositor');
 
   const properties = animation.effect.getProperties();
   properties.forEach(property => {
@@ -1142,5 +1142,63 @@ promise_test(async t => {
    'compositor because one of the transform-like property is overridden ' +
    'by an !important rule');
 
+promise_test(async t => {
+  const div = addDiv(t);
+  const animation = div.animate({ rotate: ['0deg', '45deg'],
+                                  transform: ['translate(20px)',
+                                              'translate(30px)'] },
+                                100 * MS_PER_SEC);
+
+  div.style.setProperty('translate', '50px', 'important');
+  getComputedStyle(div).translate;
+
+  await waitForAnimationReadyToRestyle(animation);
+  await waitForPaints();
+
+  assert_animation_is_running_on_compositor(animation,
+    'Animation is still running on the compositor');
+
+  const properties = animation.effect.getProperties();
+  properties.forEach(property => {
+    assert_true(property.runningOnCompositor,
+                property.property + ' is running on the compositor');
+  });
+}, 'Multiple transform-like properties animation still runs on the ' +
+   'compositor because the overridden-by-!important property does not have ' +
+   'animation');
+
+promise_test(async t => {
+  // We should run the animations on the compositor for this case:
+  // 1. A transition of 'translate'
+  // 2. An !important rule on 'translate'
+  // 3. An animation of 'scale'
+  const div = addDiv(t, { style: 'translate: 100px !important;' });
+  const animation = div.animate({ rotate: ['0deg', '45deg'] },
+                                100 * MS_PER_SEC);
+  div.style.transition = 'translate 100s';
+  getComputedStyle(div).transition;
+
+  await waitForAnimationReadyToRestyle(animation);
+  await waitForPaints();
+
+  assert_animation_is_running_on_compositor(animation,
+    'rotate animation should be running on the compositor');
+
+  div.style.setProperty('translate', '200px', 'important');
+  getComputedStyle(div).translate;
+
+  const anims = div.getAnimations();
+  await waitForPaints();
+
+  assert_animation_is_running_on_compositor(anims[0],
+    `${anims[0].effect.getProperties()[0].property} animation should be ` +
+    `running on the compositor`);
+  assert_animation_is_running_on_compositor(anims[1],
+    `${anims[1].effect.getProperties()[0].property} animation should be ` +
+    `running on the compositor`);
+}, 'Transform-like animations and transitions still runs on the compositor ' +
+   'because the !important rule is overridden by a transition, and the ' +
+   'transition property does not have animations');
+
 </script>
 </body>

dom/locales/en-US/chrome/layout/layout_errors.properties

@@ -36,5 +36,6 @@ CompositorAnimationWarningTransformSVG=Animations of ‘transform’ on elements
 CompositorAnimationWarningTransformWithGeometricProperties=Animations of ‘transform’ cannot be run on the compositor when geometric properties are animated on the same element at the same time
 CompositorAnimationWarningTransformWithSyncGeometricAnimations=Animation of ‘transform’ cannot be run on the compositor because it should be synchronized with animations of geometric properties that started at the same time
 CompositorAnimationWarningTransformFrameInactive=Animation cannot be run on the compositor because the frame was not marked active for ‘transform’ animation
+CompositorAnimationWarningTransformIsBlockedByImportantRules=Transform animation cannot be run on the compositor because transform-related properties are overridden by !important rules
 CompositorAnimationWarningOpacityFrameInactive=Animation cannot be run on the compositor because the frame was not marked active for ‘opacity’ animation
 CompositorAnimationWarningHasRenderingObserver=Animation cannot be run on the compositor because the element has rendering observers (-moz-element or SVG clipping/masking)