Bug 1888317 - Use AnimatedPropertyIDSet for EffectSet::mPropertiesForAnimationsLevel. r=layout-reviewers,zrhoffman,emilio

We keep a set of properties which are running on the
`CascadeLevel::Animations`, to make sure we compose the correct properties
for the rule on Animations/Transitions cascade level.

In other words, without this patch, we may accidentally compose an
animation rule for `CascadeLevel::Animations` (from the KeyframeEffect of
the existing transition), even if we don't have any running animations,
if this transition property is a custom property.

The resolution is to use `AnimatedPropertyIDSet`, for
`mPropertiesForAnimationsLevel`, so we can skip the custom properties which
shouldn't belong to Animations cascade level when composing them.

Note that we don't change the logic, and we are still using
`nsCSSPropertyIDSet` for `EffectSet::mPropertiesWithImportantRules`
because compositor animations don't allow custom properties now.
(This could be a future work I guess, if we are using custom property
values for opacity, translate, rotate, scale, etc.)

Also, I noticed the subtest, "No transition when removing @property rule",
is similar to the testcase in this bug, and so it got passed as well.

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

dom/animation/Animation.cpp

@@ -1266,8 +1266,9 @@ void Animation::WillComposeStyle() {
   }
 }
 
-void Animation::ComposeStyle(StyleAnimationValueMap& aComposeResult,
-                             const nsCSSPropertyIDSet& aPropertiesToSkip) {
+void Animation::ComposeStyle(
+    StyleAnimationValueMap& aComposeResult,
+    const InvertibleAnimatedPropertyIDSet& aPropertiesToSkip) {
   if (!mEffect) {
     return;
   }

dom/animation/Animation.h

@@ -9,6 +9,7 @@
 
 #include "X11UndefineNone.h"
 #include "nsCycleCollectionParticipant.h"
+#include "mozilla/AnimatedPropertyIDSet.h"
 #include "mozilla/AnimationPerformanceWarning.h"
 #include "mozilla/Attributes.h"
 #include "mozilla/BasePrincipal.h"
@@ -330,7 +331,7 @@ class Animation : public DOMEventTargetHelper,
    * updated in |aComposeResult|.
    */
   void ComposeStyle(StyleAnimationValueMap& aComposeResult,
-                    const nsCSSPropertyIDSet& aPropertiesToSkip);
+                    const InvertibleAnimatedPropertyIDSet& aPropertiesToSkip);
 
   void NotifyEffectTimingUpdated();
   void NotifyEffectPropertiesUpdated();
@@ -344,7 +345,7 @@ class Animation : public DOMEventTargetHelper,
    * is canceled, it will be released by its owning element and may not still
    * exist when we would normally go to queue events on the next tick.
    */
-  virtual void MaybeQueueCancelEvent(const StickyTimeDuration& aActiveTime){};
+  virtual void MaybeQueueCancelEvent(const StickyTimeDuration& aActiveTime) {};
 
   Maybe<uint32_t>& CachedChildIndexRef() { return mCachedChildIndex; }
 

dom/animation/EffectCompositor.cpp

@@ -372,7 +372,7 @@ static void ComposeSortedEffects(
     StyleAnimationValueMap* aAnimationValues) {
   const bool isTransition =
       aCascadeLevel == EffectCompositor::CascadeLevel::Transitions;
-  nsCSSPropertyIDSet propertiesToSkip;
+  InvertibleAnimatedPropertyIDSet propertiesToSkip;
   // Transitions should be overridden by running animations of the same
   // property per https://drafts.csswg.org/css-transitions/#application:
   //
@@ -386,9 +386,11 @@ static void ComposeSortedEffects(
   //
   // MOZ_ASSERT_IF(aEffectSet, !aEffectSet->CascadeNeedsUpdate());
   if (aEffectSet) {
-    propertiesToSkip =
-        isTransition ? aEffectSet->PropertiesForAnimationsLevel()
-                     : aEffectSet->PropertiesForAnimationsLevel().Inverse();
+    // Note that we do invert the set on CascadeLevel::Animations because we
+    // don't want to skip those properties when composing the animation rule on
+    // CascadeLevel::Animations.
+    propertiesToSkip.Setup(&aEffectSet->PropertiesForAnimationsLevel(),
+                           !isTransition);
   }
 
   for (KeyframeEffect* effect : aSortedEffects) {
@@ -608,9 +610,11 @@ nsCSSPropertyIDSet EffectCompositor::GetOverriddenProperties(
     nsCSSPropertyIDSet propertiesToTrackAsSet;
     for (KeyframeEffect* effect : aEffectSet) {
       for (const AnimationProperty& property : effect->Properties()) {
+        // Custom properties don't run on the compositor.
         if (property.mProperty.IsCustom()) {
           continue;
         }
+
         if (nsCSSProps::PropHasFlags(property.mProperty.mID,
                                      CSSPropFlags::CanAnimateOnCompositor) &&
             !propertiesToTrackAsSet.HasProperty(property.mProperty.mID)) {
@@ -663,8 +667,6 @@ void EffectCompositor::UpdateCascadeResults(EffectSet& aEffectSet,
 
   nsCSSPropertyIDSet& propertiesWithImportantRules =
       aEffectSet.PropertiesWithImportantRules();
-  nsCSSPropertyIDSet& propertiesForAnimationsLevel =
-      aEffectSet.PropertiesForAnimationsLevel();
 
   static constexpr nsCSSPropertyIDSet compositorAnimatables =
       nsCSSPropertyIDSet::CompositorAnimatables();
@@ -673,13 +675,10 @@ void EffectCompositor::UpdateCascadeResults(EffectSet& aEffectSet,
   nsCSSPropertyIDSet prevCompositorPropertiesWithImportantRules =
       propertiesWithImportantRules.Intersect(compositorAnimatables);
 
-  nsCSSPropertyIDSet prevPropertiesForAnimationsLevel =
-      propertiesForAnimationsLevel;
-
   propertiesWithImportantRules.Empty();
-  propertiesForAnimationsLevel.Empty();
 
-  nsCSSPropertyIDSet propertiesForTransitionsLevel;
+  AnimatedPropertyIDSet propertiesForAnimationsLevel;
+  AnimatedPropertyIDSet propertiesForTransitionsLevel;
 
   for (const KeyframeEffect* effect : sortedEffectList) {
     MOZ_ASSERT(effect->GetAnimation(),
@@ -687,19 +686,21 @@ void EffectCompositor::UpdateCascadeResults(EffectSet& aEffectSet,
     CascadeLevel cascadeLevel = effect->GetAnimation()->CascadeLevel();
 
     for (const AnimationProperty& prop : effect->Properties()) {
-      if (prop.mProperty.IsCustom()) {
-        continue;
-      }
-      if (overriddenProperties.HasProperty(prop.mProperty.mID)) {
+      // Note that nsCSSPropertyIDSet::HasProperty() returns false for custom
+      // properties. We don't support custom properties for compositor
+      // animations, so we are still using nsCSSPropertyIDSet to handle these
+      // properties.
+      // TODO: Bug 1869475. Support custom properties for compositor animations.
+      if (overriddenProperties.HasProperty(prop.mProperty)) {
         propertiesWithImportantRules.AddProperty(prop.mProperty.mID);
       }
 
       switch (cascadeLevel) {
         case EffectCompositor::CascadeLevel::Animations:
-          propertiesForAnimationsLevel.AddProperty(prop.mProperty.mID);
+          propertiesForAnimationsLevel.AddProperty(prop.mProperty);
           break;
         case EffectCompositor::CascadeLevel::Transitions:
-          propertiesForTransitionsLevel.AddProperty(prop.mProperty.mID);
+          propertiesForTransitionsLevel.AddProperty(prop.mProperty);
           break;
       }
     }
@@ -707,6 +708,13 @@ void EffectCompositor::UpdateCascadeResults(EffectSet& aEffectSet,
 
   aEffectSet.MarkCascadeUpdated();
 
+  // Update EffectSet::mPropertiesForAnimationsLevel to the new set, after
+  // exiting this scope.
+  auto scopeExit = MakeScopeExit([&] {
+    aEffectSet.PropertiesForAnimationsLevel() =
+        std::move(propertiesForAnimationsLevel);
+  });
+
   nsPresContext* presContext = nsContentUtils::GetContextForContent(aElement);
   if (!presContext) {
     return;
@@ -726,10 +734,13 @@ void EffectCompositor::UpdateCascadeResults(EffectSet& aEffectSet,
   // If we have transition properties and if the same propery for animations
   // level is newly added or removed, we need to update the transition level
   // rule since the it will be added/removed from the rule tree.
-  nsCSSPropertyIDSet changedPropertiesForAnimationLevel =
+  const AnimatedPropertyIDSet& prevPropertiesForAnimationsLevel =
+      aEffectSet.PropertiesForAnimationsLevel();
+  const AnimatedPropertyIDSet& changedPropertiesForAnimationLevel =
       prevPropertiesForAnimationsLevel.Xor(propertiesForAnimationsLevel);
-  nsCSSPropertyIDSet commonProperties = propertiesForTransitionsLevel.Intersect(
-      changedPropertiesForAnimationLevel);
+  const AnimatedPropertyIDSet& commonProperties =
+      propertiesForTransitionsLevel.Intersect(
+          changedPropertiesForAnimationLevel);
   if (!commonProperties.IsEmpty()) {
     EffectCompositor::RestyleType restyleType =
         changedPropertiesForAnimationLevel.Intersects(compositorAnimatables)

dom/animation/EffectSet.h

@@ -188,10 +188,10 @@ class EffectSet {
   nsCSSPropertyIDSet& PropertiesWithImportantRules() {
     return mPropertiesWithImportantRules;
   }
-  nsCSSPropertyIDSet PropertiesForAnimationsLevel() const {
+  const AnimatedPropertyIDSet& PropertiesForAnimationsLevel() const {
     return mPropertiesForAnimationsLevel;
   }
-  nsCSSPropertyIDSet& PropertiesForAnimationsLevel() {
+  AnimatedPropertyIDSet& PropertiesForAnimationsLevel() {
     return mPropertiesForAnimationsLevel;
   }
 
@@ -221,7 +221,7 @@ class EffectSet {
   // Specifies the properties for which the result will be added to the
   // animations level of the cascade and hence should be skipped when we are
   // composing the animation style for the transitions level of the cascede.
-  nsCSSPropertyIDSet mPropertiesForAnimationsLevel;
+  AnimatedPropertyIDSet mPropertiesForAnimationsLevel;
 
 #ifdef DEBUG
   // Track how many iterators are referencing this effect set when we are

dom/animation/KeyframeEffect.cpp

@@ -608,8 +608,9 @@ void KeyframeEffect::ComposeStyleRule(StyleAnimationValueMap& aAnimationValues,
                          &aComputedTiming, mEffectOptions.mIterationComposite);
 }
 
-void KeyframeEffect::ComposeStyle(StyleAnimationValueMap& aComposeResult,
-                                  const nsCSSPropertyIDSet& aPropertiesToSkip) {
+void KeyframeEffect::ComposeStyle(
+    StyleAnimationValueMap& aComposeResult,
+    const InvertibleAnimatedPropertyIDSet& aPropertiesToSkip) {
   ComputedTiming computedTiming = GetComputedTiming();
 
   // If the progress is null, we don't have fill data for the current
@@ -1652,8 +1653,8 @@ bool KeyframeEffect::ShouldBlockAsyncTransformAnimations(
   // transform-related animations on the main thread (where we have the
   // !important rule).
   nsCSSPropertyIDSet blockedProperties =
-      effectSet->PropertiesWithImportantRules().Intersect(
-          effectSet->PropertiesForAnimationsLevel());
+      effectSet->PropertiesForAnimationsLevel().Intersect(
+          effectSet->PropertiesWithImportantRules());
   if (blockedProperties.Intersects(aPropertySet)) {
     aPerformanceWarning =
         AnimationPerformanceWarning::Type::TransformIsBlockedByImportantRules;

dom/animation/KeyframeEffect.h

@@ -76,7 +76,7 @@ struct AnimationProperty {
 
   // The copy constructor/assignment doesn't copy mIsRunningOnCompositor and
   // mPerformanceWarning.
-  AnimationProperty() : mProperty(eCSSProperty_UNKNOWN){};
+  AnimationProperty() : mProperty(eCSSProperty_UNKNOWN) {};
   AnimationProperty(const AnimationProperty& aOther)
       : mProperty(aOther.mProperty), mSegments(aOther.mSegments.Clone()) {}
   AnimationProperty& operator=(const AnimationProperty& aOther) {
@@ -275,7 +275,7 @@ class KeyframeEffect : public AnimationEffect {
   // AnimationEffect for the current time except any properties contained
   // in |aPropertiesToSkip|.
   void ComposeStyle(StyleAnimationValueMap& aComposeResult,
-                    const nsCSSPropertyIDSet& aPropertiesToSkip);
+                    const InvertibleAnimatedPropertyIDSet& aPropertiesToSkip);
 
   // Returns true if at least one property is being animated on compositor.
   bool IsRunningOnCompositor() const;

layout/style/AnimatedPropertyIDSet.h

@@ -17,6 +17,20 @@ namespace mozilla {
 
 class AnimatedPropertyIDSet {
  public:
+  using CustomNameSet = nsTHashSet<RefPtr<nsAtom>>;
+
+  AnimatedPropertyIDSet() = default;
+  AnimatedPropertyIDSet(AnimatedPropertyIDSet&& aOther) = default;
+  AnimatedPropertyIDSet(nsCSSPropertyIDSet&& aIDs, CustomNameSet&& aNames)
+      : mIDs(std::move(aIDs)), mCustomNames(std::move(aNames)) {}
+
+  AnimatedPropertyIDSet& operator=(AnimatedPropertyIDSet&& aRhs) {
+    MOZ_ASSERT(&aRhs != this);
+    this->~AnimatedPropertyIDSet();
+    new (this) AnimatedPropertyIDSet(std::move(aRhs));
+    return *this;
+  }
+
   void AddProperty(const AnimatedPropertyID& aProperty) {
     if (aProperty.IsCustom()) {
       mCustomNames.Insert(aProperty.mCustomName);
@@ -57,6 +71,8 @@ class AnimatedPropertyIDSet {
     return true;
   }
 
+  bool IsEmpty() const { return mIDs.IsEmpty() && mCustomNames.IsEmpty(); }
+
   void AddProperties(const AnimatedPropertyIDSet& aOther) {
     mIDs |= aOther.mIDs;
     for (const auto& name : aOther.mCustomNames) {
@@ -64,7 +80,46 @@ class AnimatedPropertyIDSet {
     }
   }
 
-  using CustomNameSet = nsTHashSet<RefPtr<nsAtom>>;
+  // Returns a new nsCSSPropertyIDSet with all properties that are both in this
+  // set and |aOther|.
+  nsCSSPropertyIDSet Intersect(const nsCSSPropertyIDSet& aOther) const {
+    return mIDs.Intersect(aOther);
+  }
+
+  // Returns a new AnimatedPropertyIDSet with all properties that are both in
+  // this set and |aOther|.
+  AnimatedPropertyIDSet Intersect(const AnimatedPropertyIDSet& aOther) const {
+    nsCSSPropertyIDSet ids = mIDs.Intersect(aOther.mIDs);
+    CustomNameSet names;
+    for (const auto& name : mCustomNames) {
+      if (!aOther.mCustomNames.Contains(name)) {
+        continue;
+      }
+      names.Insert(name);
+    }
+    return AnimatedPropertyIDSet(std::move(ids), std::move(names));
+  }
+
+  // Returns a new AnimatedPropertyIDSet with all properties that are in either
+  // this set or |aOther| but not both.
+  AnimatedPropertyIDSet Xor(const AnimatedPropertyIDSet& aOther) const {
+    nsCSSPropertyIDSet ids = mIDs.Xor(aOther.mIDs);
+    CustomNameSet names;
+    for (const auto& name : mCustomNames) {
+      if (aOther.mCustomNames.Contains(name)) {
+        continue;
+      }
+      names.Insert(name);
+    }
+
+    for (const auto& name : aOther.mCustomNames) {
+      if (mCustomNames.Contains(name)) {
+        continue;
+      }
+      names.Insert(name);
+    }
+    return AnimatedPropertyIDSet(std::move(ids), std::move(names));
+  }
 
   // Iterator for use in range-based for loops
   class Iterator {
@@ -140,10 +195,32 @@ class AnimatedPropertyIDSet {
   Iterator end() const { return Iterator::EndIterator(*this); }
 
  private:
+  AnimatedPropertyIDSet(const AnimatedPropertyIDSet&) = delete;
+  AnimatedPropertyIDSet& operator=(const AnimatedPropertyIDSet&) = delete;
+
   nsCSSPropertyIDSet mIDs;
   CustomNameSet mCustomNames;
 };
 
+// A wrapper to support the inversion of AnimatedPropertyIDSet.
+//
+// We are using this struct (for convenience) to check if we should skip the
+// AnimatedPropertyIDs when composing an animation rule, on either
+// CascadeLevel::Animations or CascadeLevel::Tranistions.
+struct InvertibleAnimatedPropertyIDSet {
+  const AnimatedPropertyIDSet* mSet = nullptr;
+  bool mIsInverted = false;
+
+  void Setup(const AnimatedPropertyIDSet* aSet, bool aIsInverted) {
+    mSet = aSet;
+    mIsInverted = aIsInverted;
+  }
+
+  bool HasProperty(const AnimatedPropertyID& aProperty) const {
+    return mSet && mIsInverted != mSet->HasProperty(aProperty);
+  }
+};
+
 }  // namespace mozilla
 
 #endif  // mozilla_AnimatedPropertyIDSet_h

layout/style/nsCSSPropertyIDSet.h

@@ -147,15 +147,6 @@ class nsCSSPropertyIDSet {
     return this->Intersect(aOther).Equals(*this);
   }
 
-  // Return a new nsCSSPropertyIDSet which is the inverse of this set.
-  nsCSSPropertyIDSet Inverse() const {
-    nsCSSPropertyIDSet result;
-    for (size_t i = 0; i < mozilla::ArrayLength(mProperties); ++i) {
-      result.mProperties[i] = ~mProperties[i];
-    }
-    return result;
-  }
-
   // Returns a new nsCSSPropertyIDSet with all properties that are both in
   // this set and |aOther|.
   nsCSSPropertyIDSet Intersect(const nsCSSPropertyIDSet& aOther) const {
@@ -166,7 +157,7 @@ class nsCSSPropertyIDSet {
     return result;
   }
 
-  // Return a new nsCSSPropertyIDSet with all properties that are in either
+  // Returns a new nsCSSPropertyIDSet with all properties that are in either
   // this set or |aOther| but not both.
   nsCSSPropertyIDSet Xor(const nsCSSPropertyIDSet& aOther) const {
     nsCSSPropertyIDSet result;

testing/web-platform/meta/css/css-properties-values-api/at-property-animation.html.ini

@@ -1,7 +1,4 @@
 [at-property-animation.html]
-  [No transition when removing @property rule]
-    expected: FAIL
-
   [JS-originated animation setting "currentColor" for a custom property on a keyframe is responsive to changing "color" on the parent.]
     expected: FAIL
 

testing/web-platform/tests/css/css-properties-values-api/at-property-animation.html

@@ -295,6 +295,45 @@ test(() => {
   });
 }, 'No transition when removing @property rule');
 
+test(() => {
+  let name = generate_name();
+  with_style_node(`div { transition: ${name} steps(2, start) 100s; }`, () => {
+    let style1 = document.createElement('style');
+    style1.textContent = `
+      @property ${name} {
+        syntax: "<angle>";
+        inherits: false;
+        initial-value: -90deg;
+      }
+    `;
+
+    let style2 = document.createElement('style');
+    style2.textContent = `div { ${name}: 90deg; }`;
+
+    try {
+      // Register the property:
+      document.body.append(style1);
+      // No transition in the beginning.
+      assert_equals(getComputedStyle(div).getPropertyValue(name), '-90deg');
+
+      // Change the computed value:
+      document.body.append(style2);
+      // This should cause an interpolation between -90deg and 90deg (for more
+      // specifically, it is 50% from -90deg to 90deg, with steps(2, start)).
+      assert_equals(getComputedStyle(div).getPropertyValue(name), '0deg');
+
+      // In the middle of the transition above, remove style2, which creates
+      // a transition which reverses the existing one, from 0deg to -90deg.
+      // Also, it is 50% from 0deg to -90deg, with steps(2, start).
+      style2.remove();
+      assert_equals(getComputedStyle(div).getPropertyValue(name), '-45deg');
+    } finally {
+      style1.remove();
+      style2.remove();
+    }
+  });
+}, 'Ongoing transition reverts to its initial state');
+
 test_with_at_property({
   syntax: '"<length>"',
   inherits: false,