Bug 1459536 - Replace CSSAnimation interaction with animation-play-state with a simpler "once overridden always overridden" approach; r=boris

This corresponds to the approach outlined in https://github.com/w3c/csswg-drafts/issues/4822 Specifically: * Calling play() / pause() always triggers the "animation play state is being overridden by the API" behavior (unless the method throws an exception). * Setting the startTime or calling reverse() only triggers this override behavior if it results in a change between a playState that is paused and a playState that is not paused. Differential Revision: https://phabricator.services.mozilla.com/D65100 --HG-- extra : moz-landing-system : lando
2024-10-18 07:45:30 +00:00 · 2020-03-04 00:38:30 +00:00 · 2020-03-04 00:38:30 +00:00 · 21f07a041a
commit 21f07a041a
parent 0d27d97f36
4 changed files with 232 additions and 181 deletions
--- a/dom/animation/Animation.h
+++ b/dom/animation/Animation.h
@ -101,7 +101,7 @@ class Animation : public DOMEventTargetHelper,
  Nullable<TimeDuration> GetStartTime() const { return mStartTime; }
  Nullable<double> GetStartTimeAsDouble() const;
  void SetStartTime(const Nullable<TimeDuration>& aNewStartTime);
-  void SetStartTimeAsDouble(const Nullable<double>& aStartTime);
+  virtual void SetStartTimeAsDouble(const Nullable<double>& aStartTime);

  // This is deliberately _not_ called GetCurrentTime since that would clash
  // with a macro defined in winbase.h
@ -134,21 +134,16 @@ class Animation : public DOMEventTargetHelper,

  void Finish(ErrorResult& aRv);

-  virtual void Play(ErrorResult& aRv, LimitBehavior aLimitBehavior);
+  void Play(ErrorResult& aRv, LimitBehavior aLimitBehavior);
  virtual void PlayFromJS(ErrorResult& aRv) {
    Play(aRv, LimitBehavior::AutoRewind);
  }

-  virtual void Pause(ErrorResult& aRv);
-  /**
-   * PauseFromJS is currently only here for symmetry with PlayFromJS but
-   * in future we will likely have to flush style in
-   * CSSAnimation::PauseFromJS so we leave it for now.
-   */
-  void PauseFromJS(ErrorResult& aRv) { Pause(aRv); }
+  void Pause(ErrorResult& aRv);
+  virtual void PauseFromJS(ErrorResult& aRv) { Pause(aRv); }

  void UpdatePlaybackRate(double aPlaybackRate);
-  void Reverse(ErrorResult& aRv);
+  virtual void Reverse(ErrorResult& aRv);

  void Persist();
  void CommitStyles(ErrorResult& aRv);
--- a/layout/style/nsAnimationManager.cpp
+++ b/layout/style/nsAnimationManager.cpp
@ -54,19 +54,41 @@ void CSSAnimation::SetEffect(AnimationEffect* aEffect) {
  AddOverriddenProperties(CSSAnimationProperties::Effect);
 }

+void CSSAnimation::SetStartTimeAsDouble(const Nullable<double>& aStartTime) {
+  // Note that we always compare with the paused state since for the purposes
+  // of determining if play control is being overridden or not, we want to
+  // treat the finished state as running.
+  bool wasPaused = PlayState() == AnimationPlayState::Paused;
+
+  Animation::SetStartTimeAsDouble(aStartTime);
+
+  bool isPaused = PlayState() == AnimationPlayState::Paused;
+
+  if (wasPaused != isPaused) {
+    AddOverriddenProperties(CSSAnimationProperties::PlayState);
+  }
+}
+
 mozilla::dom::Promise* CSSAnimation::GetReady(ErrorResult& aRv) {
  FlushUnanimatedStyle();
  return Animation::GetReady(aRv);
 }

-void CSSAnimation::Play(ErrorResult& aRv, LimitBehavior aLimitBehavior) {
-  mPauseShouldStick = false;
-  Animation::Play(aRv, aLimitBehavior);
-}
+void CSSAnimation::Reverse(ErrorResult& aRv) {
+  // As with CSSAnimation::SetStartTimeAsDouble, we're really only interested in
+  // the paused state.
+  bool wasPaused = PlayState() == AnimationPlayState::Paused;

-void CSSAnimation::Pause(ErrorResult& aRv) {
-  mPauseShouldStick = true;
-  Animation::Pause(aRv);
+  Animation::Reverse(aRv);
+  if (aRv.Failed()) {
+    return;
+  }
+
+  bool isPaused = PlayState() == AnimationPlayState::Paused;
+
+  if (wasPaused != isPaused) {
+    AddOverriddenProperties(CSSAnimationProperties::PlayState);
+  }
 }

 AnimationPlayState CSSAnimation::PlayStateFromJS() const {
@ -88,25 +110,30 @@ void CSSAnimation::PlayFromJS(ErrorResult& aRv) {
  // PlayFromStyle()/PauseFromStyle() on this object.
  FlushUnanimatedStyle();
  Animation::PlayFromJS(aRv);
-}
-
-void CSSAnimation::PlayFromStyle() {
-  mIsStylePaused = false;
-  if (!mPauseShouldStick) {
-    ErrorResult rv;
-    Animation::Play(rv, Animation::LimitBehavior::Continue);
-    // play() should not throw when LimitBehavior is Continue
-    MOZ_ASSERT(!rv.Failed(), "Unexpected exception playing animation");
-  }
-}
-
-void CSSAnimation::PauseFromStyle() {
-  // Check if the pause state is being overridden
-  if (mIsStylePaused) {
+  if (aRv.Failed()) {
    return;
  }

-  mIsStylePaused = true;
+  AddOverriddenProperties(CSSAnimationProperties::PlayState);
+}
+
+void CSSAnimation::PauseFromJS(ErrorResult& aRv) {
+  Animation::PauseFromJS(aRv);
+  if (aRv.Failed()) {
+    return;
+  }
+
+  AddOverriddenProperties(CSSAnimationProperties::PlayState);
+}
+
+void CSSAnimation::PlayFromStyle() {
+  ErrorResult rv;
+  Animation::Play(rv, Animation::LimitBehavior::Continue);
+  // play() should not throw when LimitBehavior is Continue
+  MOZ_ASSERT(!rv.Failed(), "Unexpected exception playing animation");
+}
+
+void CSSAnimation::PauseFromStyle() {
  ErrorResult rv;
  Animation::Pause(rv);
  // pause() should only throw when *all* of the following conditions are true:
@ -470,18 +497,13 @@ static void UpdateOldAnimationPropertiesWithNew(

  // Handle changes in play state. If the animation is idle, however,
  // changes to animation-play-state should *not* restart it.
-  if (aOld.PlayState() != AnimationPlayState::Idle) {
-    // CSSAnimation takes care of override behavior so that,
-    // for example, if the author has called pause(), that will
-    // override the animation-play-state.
-    // (We should check aNew->IsStylePaused() but that requires
-    //  downcasting to CSSAnimation and we happen to know that
-    //  aNew will only ever be paused by calling PauseFromStyle
-    //  making IsPausedOrPausing synonymous in this case.)
-    if (!aOld.IsStylePaused() && aNewIsStylePaused) {
+  if (aOld.PlayState() != AnimationPlayState::Idle &&
+      ~aOverriddenProperties & CSSAnimationProperties::PlayState) {
+    bool wasPaused = aOld.PlayState() == AnimationPlayState::Paused;
+    if (!wasPaused && aNewIsStylePaused) {
      aOld.PauseFromStyle();
      animationChanged = true;
-    } else if (aOld.IsStylePaused() && !aNewIsStylePaused) {
+    } else if (wasPaused && !aNewIsStylePaused) {
      aOld.PlayFromStyle();
      animationChanged = true;
    }
--- a/layout/style/nsAnimationManager.h
+++ b/layout/style/nsAnimationManager.h
@ -58,8 +58,6 @@ class CSSAnimation final : public Animation {
  explicit CSSAnimation(nsIGlobalObject* aGlobal, nsAtom* aAnimationName)
      : dom::Animation(aGlobal),
        mAnimationName(aAnimationName),
-        mIsStylePaused(false),
-        mPauseShouldStick(false),
        mNeedsNewAnimationIndexWhenRun(false),
        mPreviousPhase(ComputedTiming::AnimationPhase::Idle),
        mPreviousIteration(0) {
@ -85,9 +83,9 @@ class CSSAnimation final : public Animation {

  // Animation interface overrides
  void SetEffect(AnimationEffect* aEffect) override;
+  void SetStartTimeAsDouble(const Nullable<double>& aStartTime) override;
  Promise* GetReady(ErrorResult& aRv) override;
-  void Play(ErrorResult& aRv, LimitBehavior aLimitBehavior) override;
-  void Pause(ErrorResult& aRv) override;
+  void Reverse(ErrorResult& aRv) override;

  // NOTE: tabbrowser.xml currently relies on the fact that reading the
  // currentTime of a CSSAnimation does *not* flush style (whereas reading the
@ -100,6 +98,7 @@ class CSSAnimation final : public Animation {
  AnimationPlayState PlayStateFromJS() const override;
  bool PendingFromJS() const override;
  void PlayFromJS(ErrorResult& aRv) override;
+  void PauseFromJS(ErrorResult& aRv) override;

  void PlayFromStyle();
  void PauseFromStyle();
@ -129,8 +128,6 @@ class CSSAnimation final : public Animation {
  void QueueEvents(
      const StickyTimeDuration& aActiveTime = StickyTimeDuration());

-  bool IsStylePaused() const { return mIsStylePaused; }
-
  bool HasLowerCompositeOrderThan(const CSSAnimation& aOther) const;

  void SetAnimationIndex(uint64_t aIndex) {
@ -208,58 +205,6 @@ class CSSAnimation final : public Animation {
  // For (b) and (c) the owning element will return !IsSet().
  OwningElementRef mOwningElement;

-  // When combining animation-play-state with play() / pause() the following
-  // behavior applies:
-  // 1. pause() is sticky and always overrides the underlying
-  //    animation-play-state
-  // 2. If animation-play-state is 'paused', play() will temporarily override
-  //    it until animation-play-state next becomes 'running'.
-  // 3. Calls to play() trigger finishing behavior but setting the
-  //    animation-play-state to 'running' does not.
-  //
-  // This leads to five distinct states:
-  //
-  // A. Running
-  // B. Running and temporarily overriding animation-play-state: paused
-  // C. Paused and sticky overriding animation-play-state: running
-  // D. Paused and sticky overriding animation-play-state: paused
-  // E. Paused by animation-play-state
-  //
-  // C and D may seem redundant but they differ in how to respond to the
-  // sequence: call play(), set animation-play-state: paused.
-  //
-  // C will transition to A then E leaving the animation paused.
-  // D will transition to B then B leaving the animation running.
-  //
-  // A state transition chart is as follows:
-  //
-  //             A | B | C | D | E
-  //   ---------------------------
-  //   play()    A | B | A | B | B
-  //   pause()   C | D | C | D | D
-  //   'running' A | A | C | C | A
-  //   'paused'  E | B | D | D | E
-  //
-  // The base class, Animation already provides a boolean value,
-  // mIsPaused which gives us two states. To this we add a further two booleans
-  // to represent the states as follows.
-  //
-  // A. Running
-  //    (!mIsPaused; !mIsStylePaused; !mPauseShouldStick)
-  // B. Running and temporarily overriding animation-play-state: paused
-  //    (!mIsPaused; mIsStylePaused; !mPauseShouldStick)
-  // C. Paused and sticky overriding animation-play-state: running
-  //    (mIsPaused; !mIsStylePaused; mPauseShouldStick)
-  // D. Paused and sticky overriding animation-play-state: paused
-  //    (mIsPaused; mIsStylePaused; mPauseShouldStick)
-  // E. Paused by animation-play-state
-  //    (mIsPaused; mIsStylePaused; !mPauseShouldStick)
-  //
-  // (That leaves 3 combinations of the boolean values that we never set because
-  // they don't represent valid states.)
-  bool mIsStylePaused;
-  bool mPauseShouldStick;
-
  // When true, indicates that when this animation next leaves the idle state,
  // its animation index should be updated.
  bool mNeedsNewAnimationIndexWhenRun;
--- a/testing/web-platform/tests/css/css-animations/CSSAnimation-pausing.tentative.html
+++ b/testing/web-platform/tests/css/css-animations/CSSAnimation-pausing.tentative.html
@ -16,129 +16,218 @@
 <script>
 'use strict';

-const getMarginLeft = cs => parseFloat(cs.marginLeft);
-
 promise_test(async t => {
  const div = addDiv(t);
-  const cs = getComputedStyle(div);
  div.style.animation = 'anim 1000s paused';
+
  const animation = div.getAnimations()[0];
-  assert_equals(getMarginLeft(cs), 0,
-                'Initial value of margin-left is zero');
  animation.play();

  await animation.ready;
  await waitForNextFrame();

-  assert_greater_than(getMarginLeft(cs), 0,
-                      'Playing value of margin-left is greater than zero');
+  assert_equals(
+    animation.playState,
+    'running',
+    'Play state is running after calling play()'
+  );
+
+  // Flip the animation-play-state back and forth to check it has no effect
+
+  div.style.animationPlayState = 'running';
+  getComputedStyle(div).animationPlayState;
+  div.style.animationPlayState = 'paused';
+  getComputedStyle(div).animationPlayState;
+
+  assert_equals(
+    animation.playState,
+    'running',
+    'Should still be running even after flipping the animation-play-state'
+  );
 }, 'play() overrides animation-play-state');

 promise_test(async t => {
  const div = addDiv(t);
-  const cs = getComputedStyle(div);
-  div.style.animation = 'anim 1000s paused';
-  const animation = div.getAnimations()[0];
-  assert_equals(getMarginLeft(cs), 0,
-                'Initial value of margin-left is zero');
+  div.style.animation = 'anim 100s infinite paused';

-  animation.pause();
+  const animation = div.getAnimations()[0];
+  animation.playbackRate = -1;
+  animation.currentTime = -1;
+
+  assert_throws_dom('InvalidStateError', () => {
+    animation.play();
+  }, 'Trying to play a reversed infinite animation should throw');
+
+  assert_equals(
+    animation.playState,
+    'paused',
+    'Animation should still be paused'
+  );
+
+  animation.playbackRate = 1;
  div.style.animationPlayState = 'running';

+  assert_equals(
+    animation.playState,
+    'running',
+    'Changing the animation-play-state should play the animation'
+  );
+}, 'play() does NOT override the animation-play-state if there was an error');
+
+promise_test(async t => {
+  const div = addDiv(t);
+  div.style.animation = 'anim 1000s paused';
+
+  const animation = div.getAnimations()[0];
+  animation.pause();
+
+  div.style.animationPlayState = 'running';
+  getComputedStyle(div).animationPlayState;
+
  await animation.ready;
  await waitForNextFrame();

-  assert_equals(cs.animationPlayState, 'running',
-                'animation-play-state is running');
-  assert_equals(getMarginLeft(cs), 0,
-                'Paused value of margin-left is zero');
+  assert_equals(animation.playState, 'paused', 'playState is paused ');
+
+  // Flip the animation-play-state back and forth to check it has no effect
+
+  div.style.animationPlayState = 'paused';
+  getComputedStyle(div).animationPlayState;
+  div.style.animationPlayState = 'running';
+  getComputedStyle(div).animationPlayState;
+
+  assert_equals(
+    animation.playState,
+    'paused',
+    'Should still be paused even after flipping the animation-play-state'
+  );
 }, 'pause() overrides animation-play-state');

 promise_test(async t => {
  const div = addDiv(t);
-  const cs = getComputedStyle(div);
-  div.style.animation = 'anim 1000s paused';
-  const animation = div.getAnimations()[0];
-  assert_equals(getMarginLeft(cs), 0,
-                'Initial value of margin-left is zero');
-  animation.play();
+  div.style.animation = 'anim 100s paused';

-  await animation.ready;
+  const animation = div.getAnimations()[0];
+  animation.reverse();
+
+  assert_equals(
+    animation.playState,
+    'running',
+    'Play state is running after calling reverse()'
+  );
+
+  // Flip the animation-play-state back and forth to check it has no effect

  div.style.animationPlayState = 'running';
-  cs.animationPlayState; // Trigger style resolution
-  await waitForNextFrame();
-
-  assert_equals(cs.animationPlayState, 'running',
-                'animation-play-state is running');
+  getComputedStyle(div).animationPlayState;
  div.style.animationPlayState = 'paused';
-  await animation.ready;
+  getComputedStyle(div).animationPlayState;

-  assert_equals(cs.animationPlayState, 'paused',
-                'animation-play-state is paused');
-  const previousAnimVal = getMarginLeft(cs);
-  await waitForNextFrame();
-
-  assert_equals(getMarginLeft(cs), previousAnimVal,
-                'Animated value of margin-left does not change when'
-                + ' paused by style');
-}, 'play() is overridden by later setting "animation-play-state: paused"');
+  assert_equals(
+    animation.playState,
+    'running',
+    'Should still be running even after flipping the animation-play-state'
+  );
+}, 'reverse() overrides animation-play-state when it starts playing the'
+   + ' animation');

 promise_test(async t => {
  const div = addDiv(t);
-  const cs = getComputedStyle(div);
-  div.style.animation = 'anim 1000s';
+  div.style.animation = 'anim 100s';
+
  const animation = div.getAnimations()[0];
-  assert_equals(getMarginLeft(cs), 0,
-                'Initial value of margin-left is zero');
+  animation.reverse();
+
+  assert_equals(
+    animation.playState,
+    'running',
+    'Play state is running after calling reverse()'
+  );

-  // Set the specified style first. If implementations fail to
-  // apply the style changes first, they will ignore the redundant
-  // call to play() and fail to correctly override the pause style.
  div.style.animationPlayState = 'paused';
-  animation.play();
-  const previousAnimVal = getMarginLeft(cs);
+  getComputedStyle(div).animationPlayState;

-  await animation.ready;
-  await waitForNextFrame();
-
-  assert_equals(cs.animationPlayState, 'paused',
-                'animation-play-state is paused');
-  assert_greater_than(getMarginLeft(cs), previousAnimVal,
-                      'Playing value of margin-left is increasing');
-}, 'play() flushes pending changes to animation-play-state first');
+  assert_equals(
+    animation.playState,
+    'paused',
+    'Should be paused after changing the animation-play-state'
+  );
+}, 'reverse() does NOT override animation-play-state if the animation is'
+   + ' already running');

 promise_test(async t => {
  const div = addDiv(t);
-  const cs = getComputedStyle(div);
-  div.style.animation = 'anim 1000s paused';
+  div.style.animation = 'anim 100s';
+
  const animation = div.getAnimations()[0];
-  assert_equals(getMarginLeft(cs), 0,
-                'Initial value of margin-left is zero');
+  animation.startTime = null;

-  // Unlike the previous test for play(), since calling pause() is sticky,
-  // we'll apply it even if the underlying style also says we're paused.
-  //
-  // We would like to test that implementations flush styles before running
-  // pause() but actually there's no style we can currently set that will
-  // change the behavior of pause(). That may change in the future
-  // (e.g. if we introduce animation-timeline or animation-playback-rate etc.).
-  //
-  // For now this just serves as a sanity check that we do the same thing
-  // even if we set style before calling the API.
+  assert_equals(
+    animation.playState,
+    'paused',
+    'Play state is paused after setting the start time to null'
+  );
+
+  // Flip the animation-play-state back and forth to check it has no effect
+
+  div.style.animationPlayState = 'paused';
+  getComputedStyle(div).animationPlayState;
  div.style.animationPlayState = 'running';
-  animation.pause();
-  const previousAnimVal = getMarginLeft(cs);
+  getComputedStyle(div).animationPlayState;

-  await animation.ready;
-  await waitForNextFrame();
+  assert_equals(
+    animation.playState,
+    'paused',
+    'Should still be paused even after flipping the animation-play-state'
+  );
+}, 'Setting the startTime to null overrides animation-play-state if the'
+   + ' animation is already running');

-  assert_equals(cs.animationPlayState, 'running',
-                'animation-play-state is running');
-  assert_equals(getMarginLeft(cs), previousAnimVal,
-                'Paused value of margin-left does not change');
-}, 'pause() applies pending changes to animation-play-state first');
-// (Note that we can't actually test for this; see comment above, in test-body.)
+promise_test(async t => {
+  const div = addDiv(t);
+  div.style.animation = 'anim 100s paused';
+
+  const animation = div.getAnimations()[0];
+  animation.startTime = document.timeline.currentTime;
+
+  assert_equals(
+    animation.playState,
+    'running',
+    'Play state is running after setting the start time to non-null'
+  );
+
+  // Flip the animation-play-state back and forth to check it has no effect
+
+  div.style.animationPlayState = 'running';
+  getComputedStyle(div).animationPlayState;
+  div.style.animationPlayState = 'paused';
+  getComputedStyle(div).animationPlayState;
+
+  assert_equals(
+    animation.playState,
+    'running',
+    'Should still be running even after flipping the animation-play-state'
+  );
+}, 'Setting the startTime to non-null overrides animation-play-state if the'
+   + ' animation is paused');
+
+promise_test(async t => {
+  const div = addDiv(t);
+  div.style.animation = 'anim 100s';
+
+  const animation = div.getAnimations()[0];
+  animation.startTime = document.timeline.currentTime;
+
+  div.style.animationPlayState = 'paused';
+  getComputedStyle(div).animationPlayState;
+
+  assert_equals(
+    animation.playState,
+    'paused',
+    'Should be paused after changing the animation-play-state'
+  );
+}, 'Setting the startTime to non-null does NOT override the'
+   + ' animation-play-state if the animation is already running');

 promise_test(async t => {
  const div = addDiv(t, { style: 'animation: anim 1000s' });