Bug 1459536 - Ignore subsequent changes to @keyframes after calling setKeyframes; r=boris

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

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

dom/animation/KeyframeEffect.h

@@ -180,8 +180,8 @@ class KeyframeEffect : public AnimationEffect {
   void NotifyAnimationTimingUpdated(PostRestyleMode aPostRestyle);
   void RequestRestyle(EffectCompositor::RestyleType aRestyleType);
   void SetAnimation(Animation* aAnimation) override;
-  void SetKeyframes(JSContext* aContext, JS::Handle<JSObject*> aKeyframes,
-                    ErrorResult& aRv);
+  virtual void SetKeyframes(JSContext* aContext,
+                            JS::Handle<JSObject*> aKeyframes, ErrorResult& aRv);
   void SetKeyframes(nsTArray<Keyframe>&& aKeyframes,
                     const ComputedStyle* aStyle);
 

layout/style/nsAnimationManager.cpp

@@ -288,6 +288,23 @@ void CSSAnimation::UpdateTiming(SeekFlag aSeekFlag,
   Animation::UpdateTiming(aSeekFlag, aSyncNotifyFlag);
 }
 
+/////////////////////// CSSAnimationKeyframeEffect ////////////////////////
+
+void CSSAnimationKeyframeEffect::SetKeyframes(JSContext* aContext,
+                                              JS::Handle<JSObject*> aKeyframes,
+                                              ErrorResult& aRv) {
+  KeyframeEffect::SetKeyframes(aContext, aKeyframes, aRv);
+
+  if (aRv.Failed()) {
+    return;
+  }
+
+  if (mAnimation && mAnimation->AsCSSAnimation()) {
+    mAnimation->AsCSSAnimation()->AddOverriddenProperties(
+        CSSAnimationProperties::Keyframes);
+  }
+}
+
 ////////////////////////// nsAnimationManager ////////////////////////////
 
 // Find the matching animation by |aName| in the old list
@@ -378,6 +395,7 @@ class MOZ_STACK_CLASS ServoCSSAnimationBuilder final {
 static void UpdateOldAnimationPropertiesWithNew(
     CSSAnimation& aOld, TimingParams&& aNewTiming,
     nsTArray<Keyframe>&& aNewKeyframes, bool aNewIsStylePaused,
+    CSSAnimationProperties aOverriddenProperties,
     ServoCSSAnimationBuilder& aBuilder) {
   bool animationChanged = false;
 
@@ -389,7 +407,8 @@ static void UpdateOldAnimationPropertiesWithNew(
     oldEffect->SetSpecifiedTiming(std::move(aNewTiming));
 
     KeyframeEffect* oldKeyframeEffect = oldEffect->AsKeyframeEffect();
-    if (oldKeyframeEffect) {
+    if (~aOverriddenProperties & CSSAnimationProperties::Keyframes &&
+        oldKeyframeEffect) {
       aBuilder.SetKeyframes(*oldKeyframeEffect, std::move(aNewKeyframes));
     }
   }
@@ -463,14 +482,14 @@ static already_AddRefed<CSSAnimation> BuildAnimation(
     // them.  See
     // http://lists.w3.org/Archives/Public/www-style/2011Apr/0079.html
     // In order to honor what the spec said, we'd copy more data over.
-    UpdateOldAnimationPropertiesWithNew(*oldAnim, std::move(timing),
-                                        std::move(keyframes), isStylePaused,
-                                        aBuilder);
+    UpdateOldAnimationPropertiesWithNew(
+        *oldAnim, std::move(timing), std::move(keyframes), isStylePaused,
+        oldAnim->GetOverriddenProperties(), aBuilder);
     return oldAnim.forget();
   }
 
   KeyframeEffectParams effectOptions;
-  RefPtr<KeyframeEffect> effect = new KeyframeEffect(
+  RefPtr<KeyframeEffect> effect = new CSSAnimationKeyframeEffect(
       aPresContext->Document(),
       OwningAnimationTarget(aTarget.mElement, aTarget.mPseudoType),
       std::move(timing), effectOptions);

layout/style/nsAnimationManager.h

@@ -11,6 +11,7 @@
 #include "mozilla/EventForwards.h"
 #include "AnimationCommon.h"
 #include "mozilla/dom/Animation.h"
+#include "mozilla/dom/KeyframeEffect.h"
 #include "mozilla/dom/MutationObservers.h"
 #include "mozilla/Keyframe.h"
 #include "mozilla/MemoryReporting.h"
@@ -34,6 +35,22 @@ class Promise;
 enum class PseudoStyleType : uint8_t;
 struct NonOwningAnimationTarget;
 
+// Properties of CSS Animations that can be overridden by the Web Animations API
+// in a manner that means we should ignore subsequent changes to markup for that
+// property.
+enum class CSSAnimationProperties {
+  None = 0,
+  Keyframes = 1 << 0,
+  Duration = 1 << 1,
+  IterationCount = 1 << 2,
+  Direction = 1 << 3,
+  Delay = 1 << 4,
+  FillMode = 1 << 5,
+  Effect = Keyframes | Duration | IterationCount | Direction | Delay | FillMode,
+  PlayState = 1 << 6,
+};
+MOZ_MAKE_ENUM_CLASS_BITWISE_OPERATORS(CSSAnimationProperties)
+
 namespace dom {
 
 class CSSAnimation final : public Animation {
@@ -139,6 +156,13 @@ class CSSAnimation final : public Animation {
     QueueEvents(aActiveTime);
   }
 
+  CSSAnimationProperties GetOverriddenProperties() const {
+    return mOverriddenProperties;
+  }
+  void AddOverriddenProperties(CSSAnimationProperties aProperties) {
+    mOverriddenProperties |= aProperties;
+  }
+
  protected:
   virtual ~CSSAnimation() {
     MOZ_ASSERT(!mOwningElement.IsSet(),
@@ -243,10 +267,29 @@ class CSSAnimation final : public Animation {
   // This is used to determine what new events to dispatch.
   ComputedTiming::AnimationPhase mPreviousPhase;
   uint64_t mPreviousIteration;
+
+  // Properties that would normally be defined by the cascade but which have
+  // since been explicitly set via the Web Animations API.
+  CSSAnimationProperties mOverriddenProperties = CSSAnimationProperties::None;
 };
 
 } /* namespace dom */
 
+// A subclass of KeyframeEffect that reports when specific properties have been
+// overridden via the Web Animations API.
+class CSSAnimationKeyframeEffect : public dom::KeyframeEffect {
+ public:
+  CSSAnimationKeyframeEffect(dom::Document* aDocument,
+                             OwningAnimationTarget&& aTarget,
+                             TimingParams&& aTiming,
+                             const KeyframeEffectParams& aOptions)
+      : KeyframeEffect(aDocument, std::move(aTarget), std::move(aTiming),
+                       aOptions) {}
+
+  void SetKeyframes(JSContext* aContext, JS::Handle<JSObject*> aKeyframes,
+                    ErrorResult& aRv) override;
+};
+
 template <>
 struct AnimationTypeTraits<dom::CSSAnimation> {
   static nsAtom* ElementPropertyAtom() { return nsGkAtoms::animationsProperty; }

testing/web-platform/tests/css/css-animations/KeyframeEffect-getKeyframes.tentative.html

@@ -648,5 +648,53 @@ test(t => {
 }, 'KeyframeEffect.getKeyframes() returns expected values for ' +
    'animations with only custom property in a keyframe');
 
+test(t => {
+  const div = addDiv(t);
+
+  // Add custom @keyframes rule
+  const stylesheet = document.styleSheets[0];
+  const keyframes = '@keyframes anim-custom { to { left: 100px } }';
+  const ruleIndex = stylesheet.insertRule(keyframes, 0);
+  const keyframesRule = stylesheet.cssRules[ruleIndex];
+
+  t.add_cleanup(function() {
+    stylesheet.deleteRule(ruleIndex);
+  });
+
+  div.style.animation = 'anim-custom 100s';
+
+  // Sanity check the initial result
+  let frames = getKeyframes(div);
+  assert_frames_equal(
+    frames[frames.length - 1],
+    {
+      offset: 1,
+      computedOffset: 1,
+      easing: 'ease',
+      composite: 'auto',
+      left: '100px',
+    },
+    'Keyframes reflect the initial @keyframes rule'
+  );
+
+  // Update the @keyframes rule
+  keyframesRule.deleteRule(0);
+  keyframesRule.appendRule('to { left: 200px }');
+
+  // Check the result from getKeyframes() is updated
+  frames = getKeyframes(div);
+  assert_frames_equal(
+    frames[frames.length - 1],
+    {
+      offset: 1,
+      computedOffset: 1,
+      easing: 'ease',
+      composite: 'auto',
+      left: '200px',
+    },
+    'Keyframes reflects the updated @keyframes rule'
+  );
+}, 'KeyframeEffect.getKeyframes() reflects changes to @keyframes rules');
+
 </script>
 </body>

testing/web-platform/tests/css/css-animations/KeyframeEffect-setKeyframes.tentative.html

@@ -0,0 +1,122 @@
+<!doctype html>
+<meta charset=utf-8>
+<title>KeyframeEffect.setKeyframes() for CSS animations</title>
+<!-- TODO: Add a more specific link for this once it is specified. -->
+<link rel="help" href="https://drafts.csswg.org/css-animations-2/">
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<script src="support/testcommon.js"></script>
+<style>
+@keyframes anim-simple {
+  from { left: 0px }
+  to   { left: 100px }
+}
+</style>
+<body>
+<div id="log"></div>
+<script>
+"use strict";
+
+// Note that the sanity check that getKeyframes() normally DOES return the
+// updated keyframes is contained in KeyframeEffect-getKeyframes.html.
+test(t => {
+  const div = addDiv(t);
+
+  // Add custom @keyframes rule
+  const stylesheet = document.styleSheets[0];
+  const keyframes = '@keyframes anim-custom { to { left: 100px } }';
+  const ruleIndex = stylesheet.insertRule(keyframes, 0);
+  const keyframesRule = stylesheet.cssRules[ruleIndex];
+
+  t.add_cleanup(function() {
+    stylesheet.deleteRule(ruleIndex);
+  });
+
+  div.style.animation = 'anim-custom 100s';
+
+  // Update the keyframes via the API
+  const animation = div.getAnimations()[0];
+  animation.effect.setKeyframes({ left: '200px' });
+
+  // Then update them via style
+  keyframesRule.deleteRule(0);
+  keyframesRule.appendRule('to { left: 300px }');
+
+  // The result should be the keyframes as set by the API, not via style.
+  const frames = animation.effect.getKeyframes();
+  assert_frames_equal(
+    frames[frames.length - 1],
+    {
+      offset: null,
+      computedOffset: 1,
+      easing: 'linear',
+      composite: 'auto',
+      left: '200px',
+    },
+    'Keyframes reflect the value set via setKeyframes'
+  );
+}, 'KeyframeEffect.setKeyframes() causes subsequent changes to @keyframes'
+   + ' rules to be ignored');
+
+test(t => {
+  const div = addDiv(t);
+  div.style.animation = 'anim-simple 100s';
+
+  const animation = div.getAnimations()[0];
+  assert_equals(animation.effect.getKeyframes()[0].easing, 'ease');
+
+  animation.effect.setKeyframes({ left: ['200px', '300px'] });
+  assert_equals(animation.effect.getKeyframes()[0].easing, 'linear');
+
+  div.style.animationTimingFunction = 'ease-in';
+  getComputedStyle(div).animationTimingFunction;
+
+  assert_equals(
+    animation.effect.getKeyframes()[0].easing,
+    'linear',
+    'Easing should be the easing set by the API'
+  );
+}, 'KeyframeEffect.setKeyframes() causes subsequent changes to'
+   + ' animation-timing-function to be ignored');
+
+test(t => {
+  const div = addDiv(t);
+
+  const stylesheet = document.styleSheets[0];
+  const keyframes = '@keyframes anim-custom { to { left: 100px } }';
+  const ruleIndex = stylesheet.insertRule(keyframes, 0);
+  const keyframesRule = stylesheet.cssRules[ruleIndex];
+
+  t.add_cleanup(function() {
+    stylesheet.deleteRule(ruleIndex);
+  });
+
+  div.style.animation = 'anim-custom 100s';
+
+  // Try updating in a way that throws an error
+  const animation = div.getAnimations()[0];
+  assert_throws_js(TypeError, () => {
+    animation.effect.setKeyframes({ left: '200px', offset: 'yer' });
+  });
+
+  keyframesRule.deleteRule(0);
+  keyframesRule.appendRule('to { left: 300px }');
+
+  // The result should be the keyframes as set via style.
+  const frames = animation.effect.getKeyframes();
+  assert_frames_equal(
+    frames[frames.length - 1],
+    {
+      offset: 1,
+      computedOffset: 1,
+      easing: 'ease',
+      composite: 'auto',
+      left: '300px',
+    },
+    'Keyframes reflect the value set via style'
+  );
+}, 'KeyframeEffect.setKeyframes() should NOT cause subsequent changes to'
+   + ' @keyframes rules to be ignored if it threw');
+
+</script>
+</body>