From bf583442cce09ac4c39fa5551e966dd17b12a9a5 Mon Sep 17 00:00:00 2001 From: Boris Chiou Date: Tue, 21 May 2024 16:55:00 +0000 Subject: [PATCH] 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 | 5 +- dom/animation/Animation.h | 5 +- dom/animation/EffectCompositor.cpp | 51 +++++++----- dom/animation/EffectSet.h | 6 +- dom/animation/KeyframeEffect.cpp | 9 ++- dom/animation/KeyframeEffect.h | 4 +- layout/style/AnimatedPropertyIDSet.h | 79 ++++++++++++++++++- layout/style/nsCSSPropertyIDSet.h | 11 +-- .../at-property-animation.html.ini | 3 - .../at-property-animation.html | 39 +++++++++ 10 files changed, 165 insertions(+), 47 deletions(-) diff --git a/dom/animation/Animation.cpp b/dom/animation/Animation.cpp index fbfd689c9a6d..384fd901ca2f 100644 --- a/dom/animation/Animation.cpp +++ b/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; } diff --git a/dom/animation/Animation.h b/dom/animation/Animation.h index e80370370e88..ece140d0dcf4 100644 --- a/dom/animation/Animation.h +++ b/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& CachedChildIndexRef() { return mCachedChildIndex; } diff --git a/dom/animation/EffectCompositor.cpp b/dom/animation/EffectCompositor.cpp index 7421a83ddba1..966fabbf0bb8 100644 --- a/dom/animation/EffectCompositor.cpp +++ b/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) diff --git a/dom/animation/EffectSet.h b/dom/animation/EffectSet.h index c74367679404..8511b84f0de6 100644 --- a/dom/animation/EffectSet.h +++ b/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 diff --git a/dom/animation/KeyframeEffect.cpp b/dom/animation/KeyframeEffect.cpp index f85415b834ca..53b143cf83ac 100644 --- a/dom/animation/KeyframeEffect.cpp +++ b/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; diff --git a/dom/animation/KeyframeEffect.h b/dom/animation/KeyframeEffect.h index 7042ee962565..5ee4e81aa47a 100644 --- a/dom/animation/KeyframeEffect.h +++ b/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; diff --git a/layout/style/AnimatedPropertyIDSet.h b/layout/style/AnimatedPropertyIDSet.h index 667f081421c1..771a38ca410b 100644 --- a/layout/style/AnimatedPropertyIDSet.h +++ b/layout/style/AnimatedPropertyIDSet.h @@ -17,6 +17,20 @@ namespace mozilla { class AnimatedPropertyIDSet { public: + using CustomNameSet = nsTHashSet>; + + 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>; + // 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 diff --git a/layout/style/nsCSSPropertyIDSet.h b/layout/style/nsCSSPropertyIDSet.h index 18488815eea9..44236767b1d8 100644 --- a/layout/style/nsCSSPropertyIDSet.h +++ b/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; diff --git a/testing/web-platform/meta/css/css-properties-values-api/at-property-animation.html.ini b/testing/web-platform/meta/css/css-properties-values-api/at-property-animation.html.ini index ead60fa815dc..287ff93e7fde 100644 --- a/testing/web-platform/meta/css/css-properties-values-api/at-property-animation.html.ini +++ b/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 diff --git a/testing/web-platform/tests/css/css-properties-values-api/at-property-animation.html b/testing/web-platform/tests/css/css-properties-values-api/at-property-animation.html index 233b63239fc3..f7a8e1697840 100644 --- a/testing/web-platform/tests/css/css-properties-values-api/at-property-animation.html +++ b/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: ""; + 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: '""', inherits: false,