Bug 1253476 - Don't composite removed animations; r=boris

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

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

dom/animation/Animation.cpp

@@ -846,7 +846,8 @@ bool Animation::ShouldBeSynchronizedWithMainThread(
 
 void Animation::UpdateRelevance() {
   bool wasRelevant = mIsRelevant;
-  mIsRelevant = HasCurrentEffect() || IsInEffect();
+  mIsRelevant = mReplaceState != AnimationReplaceState::Removed &&
+                (HasCurrentEffect() || IsInEffect());
 
   // Notify animation observers.
   if (wasRelevant && !mIsRelevant) {
@@ -961,6 +962,9 @@ void Animation::Remove() {
 
   mReplaceState = AnimationReplaceState::Removed;
 
+  UpdateEffect(PostRestyleMode::IfNeeded);
+  PostUpdate();
+
   QueuePlaybackEvent(NS_LITERAL_STRING("remove"),
                      GetTimelineCurrentTimeAsTimeStamp());
 }

dom/animation/KeyframeEffect.cpp

@@ -317,7 +317,7 @@ nsCSSPropertyIDSet KeyframeEffect::GetPropertiesForCompositor(
 
   nsCSSPropertyIDSet properties;
 
-  if (!IsInEffect() && !IsCurrent()) {
+  if (!mAnimation || !mAnimation->IsRelevant()) {
     return properties;
   }
 
@@ -796,7 +796,9 @@ void KeyframeEffect::UpdateTargetRegistration() {
   // something calls Animation::UpdateRelevance. Whenever our timing changes,
   // we should be notifying our Animation before calling this, so
   // Animation::IsRelevant() should be up-to-date by the time we get here.
-  MOZ_ASSERT(isRelevant == IsCurrent() || IsInEffect(),
+  MOZ_ASSERT(isRelevant ==
+                 ((IsCurrent() || IsInEffect()) && mAnimation &&
+                  mAnimation->ReplaceState() != AnimationReplaceState::Removed),
              "Out of date Animation::IsRelevant value");
 
   if (isRelevant && !mInEffectSet) {
@@ -1736,6 +1738,11 @@ bool KeyframeEffect::ContainsAnimatedScale(const nsIFrame* aFrame) const {
     return false;
   }
 
+  if (!mAnimation ||
+      mAnimation->ReplaceState() == AnimationReplaceState::Removed) {
+    return false;
+  }
+
   for (const AnimationProperty& prop : mProperties) {
     if (prop.mProperty != eCSSProperty_transform &&
         prop.mProperty != eCSSProperty_scale &&

dom/animation/test/chrome.ini

@@ -1,5 +1,6 @@
 [DEFAULT]
 prefs =
+  dom.animations-api.autoremove.enabled=true
   dom.animations-api.compositing.enabled=true
   gfx.omta.background-color=true
   layout.css.individual-transform.enabled=true

dom/animation/test/chrome/test_animation_observers_async.html

@@ -578,5 +578,60 @@ promise_test(t => {
   });
 }, "tree_ordering: subtree");
 
+// Test that animations removed by auto-removal trigger an event
+promise_test(async t => {
+  setupAsynchronousObserver(t, { observe: div, subtree: false });
+
+  // Start two animations such that one will be auto-removed
+  const animA = div.animate(
+    { opacity: 1 },
+    { duration: 100 * MS_PER_SEC, fill: 'forwards' }
+  );
+  const animB = div.animate(
+    { opacity: 1 },
+    { duration: 100 * MS_PER_SEC, fill: 'forwards' }
+  );
+
+  // Wait for the MutationRecords corresponding to each addition.
+  await waitForNextFrame();
+
+  assert_records(
+    [
+      { added: [animA], changed: [], removed: [] },
+      { added: [animB], changed: [], removed: [] },
+    ],
+    'records after animation start'
+  );
+
+  // Finish the animations -- this should cause animA to be replaced, and
+  // automatically removed.
+  animA.finish();
+  animB.finish();
+
+  // Wait for the MutationRecords corresponding to the timing changes and the
+  // subsequent removal to be delivered.
+  await waitForNextFrame();
+
+  assert_records(
+    [
+      { added: [], changed: [animA], removed: [] },
+      { added: [], changed: [animB], removed: [] },
+      { added: [], changed: [], removed: [animA] },
+    ],
+    'records after finishing'
+  );
+
+  // Tidy up
+  animA.cancel();
+  animB.cancel();
+
+  await waitForNextFrame();
+
+  assert_records(
+    [{ added: [], changed: [], removed: [animB] }],
+    'records after tidying up end'
+  );
+}, 'Animations automatically removed are reported');
+
 runTest();
 </script>

dom/animation/test/mozilla/file_disable_animations_api_autoremove.html

@@ -32,6 +32,38 @@ promise_test(async t => {
   await waitForNextFrame();
 }, 'Remove events should not be fired if the pref is not set');
 
+promise_test(async t => {
+  const div = addDiv(t);
+  div.style.opacity = '0.1';
+
+  const animA = div.animate(
+    { opacity: 0.2 },
+    { duration: 1, fill: 'forwards' }
+  );
+  const animB = div.animate(
+    { opacity: 0.3, composite: 'add' },
+    { duration: 1, fill: 'forwards' }
+  );
+
+  await animA.finished;
+
+  assert_approx_equals(
+    parseFloat(getComputedStyle(div).opacity),
+    0.5,
+    0.0001,
+    'Covered animation should still contribute to effect stack when adding'
+  );
+
+  animB.cancel();
+
+  assert_approx_equals(
+    parseFloat(getComputedStyle(div).opacity),
+    0.2,
+    0.0001,
+    'Covered animation should still contribute to animated style when replacing'
+  );
+}, 'Covered animations should still affect style if the pref is not set');
+
 done();
 </script>
 </body>

layout/base/nsLayoutUtils.cpp

@@ -225,6 +225,10 @@ static ContentMap& GetContentMap() {
 template <typename TestType>
 static bool HasMatchingAnimations(EffectSet& aEffects, TestType&& aTest) {
   for (KeyframeEffect* effect : aEffects) {
+    if (!effect->GetAnimation() || !effect->GetAnimation()->IsRelevant()) {
+      continue;
+    }
+
     if (aTest(*effect, aEffects)) {
       return true;
     }
@@ -263,8 +267,7 @@ bool nsLayoutUtils::HasAnimationOfPropertySet(
   return HasMatchingAnimations(
       aFrame, aPropertySet,
       [&aPropertySet](KeyframeEffect& aEffect, const EffectSet&) {
-        return (aEffect.IsInEffect() || aEffect.IsCurrent()) &&
-               aEffect.HasAnimationOfPropertySet(aPropertySet);
+        return aEffect.HasAnimationOfPropertySet(aPropertySet);
       });
 }
 
@@ -294,8 +297,7 @@ bool nsLayoutUtils::HasAnimationOfPropertySet(
   return HasMatchingAnimations(
       *aEffectSet,
       [&aPropertySet](KeyframeEffect& aEffect, const EffectSet& aEffectSet) {
-        return (aEffect.IsInEffect() || aEffect.IsCurrent()) &&
-               aEffect.HasAnimationOfPropertySet(aPropertySet);
+        return aEffect.HasAnimationOfPropertySet(aPropertySet);
       });
 }
 
@@ -305,8 +307,7 @@ bool nsLayoutUtils::HasEffectiveAnimation(
   return HasMatchingAnimations(
       aFrame, aPropertySet,
       [&aPropertySet](KeyframeEffect& aEffect, const EffectSet& aEffectSet) {
-        return (aEffect.IsInEffect() || aEffect.IsCurrent()) &&
-               aEffect.HasEffectiveAnimationOfPropertySet(aPropertySet,
+        return aEffect.HasEffectiveAnimationOfPropertySet(aPropertySet,
                                                           aEffectSet);
       });
 }

testing/web-platform/tests/web-animations/animation-model/keyframe-effects/effect-value-replaced-animations.html

@@ -0,0 +1,78 @@
+<!doctype html>
+<meta charset=utf-8>
+<title>The effect value of a keyframe effect: Overlapping keyframes</title>
+<link rel="help" href="https://drafts.csswg.org/web-animations/#the-effect-value-of-a-keyframe-animation-effect">
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<script src="../../testcommon.js"></script>
+<body>
+<div id="log"></div>
+<script>
+'use strict';
+
+function assert_opacity_value(opacity, expected, description) {
+  return assert_approx_equals(
+    parseFloat(opacity),
+    expected,
+    0.0001,
+    description
+  );
+}
+
+promise_test(async t => {
+  const div = createDiv(t);
+  div.style.opacity = '0.1';
+
+  const animA = div.animate(
+    { opacity: 0.2 },
+    { duration: 1, fill: 'forwards' }
+  );
+  const animB = div.animate(
+    { opacity: 0.3 },
+    { duration: 1, fill: 'forwards' }
+  );
+  await animA.finished;
+
+  // Sanity check
+  assert_equals(animA.replaceState, 'removed');
+  assert_equals(animB.replaceState, 'active');
+
+  // animA is now removed so if we cancel animB, we should go back to the
+  // underlying value
+  animB.cancel();
+  assert_opacity_value(
+    getComputedStyle(div).opacity,
+    0.1,
+    'Opacity should be the un-animated value'
+  );
+}, 'Removed animations do not contribute to animated style');
+
+promise_test(async t => {
+  const div = createDiv(t);
+  div.style.opacity = '0.1';
+
+  const animA = div.animate(
+    { opacity: 0.2 },
+    { duration: 1, fill: 'forwards' }
+  );
+  const animB = div.animate(
+    { opacity: 0.3, composite: 'add' },
+    { duration: 1, fill: 'forwards' }
+  );
+  await animA.finished;
+
+  // Sanity check
+  assert_equals(animA.replaceState, 'removed');
+  assert_equals(animB.replaceState, 'active');
+
+  // animA has been removed so the final result should be 0.1 + 0.3 = 0.4.
+  // (If animA were not removed it would be 0.2 + 0.3 = 0.5.)
+  assert_opacity_value(
+    getComputedStyle(div).opacity,
+    0.4,
+    'Opacity value should not include the removed animation'
+  );
+}, 'Removed animations do not contribute to the effect stack');
+
+</script>
+</body>

testing/web-platform/tests/web-animations/interfaces/Animatable/getAnimations.html

@@ -209,6 +209,16 @@ test(t => {
 }, 'Returns animations based on dynamic changes to individual'
    + ' animations\' current time');
 
+promise_test(async t => {
+  const div = createDiv(t);
+
+  const animA = div.animate({ opacity: 1 }, { duration: 1, fill: 'forwards' });
+  const animB = div.animate({ opacity: 1 }, { duration: 1, fill: 'forwards' });
+  await animA.finished;
+
+  assert_array_equals(div.getAnimations(), [animB]);
+}, 'Does not return an animation that has been removed');
+
 promise_test(async t => {
   const div = createDiv(t);
   const watcher = EventWatcher(t, div, 'transitionrun');