Bug 1443427 - Don't flush throttled animations in Animation::FlushStyle(). r?birtles draft
authorHiroyuki Ikezoe <hikezoe@mozilla.com>
Wed, 11 Apr 2018 18:01:14 +0900
changeset 780348 23ce1e08f7bef622389d56dd94fa25d676c9bcdb
parent 780347 d1db4d5030c827ada3c01ec9d2ab5d665de2cd59
push id105974
push userhikezoe@mozilla.com
push dateWed, 11 Apr 2018 09:06:59 +0000
reviewersbirtles
bugs1443427
milestone61.0a1
Bug 1443427 - Don't flush throttled animations in Animation::FlushStyle(). r?birtles Animation::FlushStyle() gets called only for CSS animations/transitions' playState changes in JS or ready Promise for CSS animations. In either case throttled animation state, which is, to be precise, transformed position or opacity value on the compositor, doesn't affect those results. The first test case for CSS animations and the first test case for CSS transitions in this patch fail without this fix. MozReview-Commit-ID: EVym4qputL4
dom/animation/Animation.cpp
dom/animation/Animation.h
dom/animation/test/mozilla/file_restyles.html
layout/style/nsAnimationManager.cpp
layout/style/nsTransitionManager.cpp
--- a/dom/animation/Animation.cpp
+++ b/dom/animation/Animation.cpp
@@ -1413,21 +1413,22 @@ Animation::UpdateEffect()
     KeyframeEffectReadOnly* keyframeEffect = mEffect->AsKeyframeEffect();
     if (keyframeEffect) {
       keyframeEffect->NotifyAnimationTimingUpdated();
     }
   }
 }
 
 void
-Animation::FlushStyle() const
+Animation::FlushUnanimatedStyle() const
 {
   nsIDocument* doc = GetRenderedDocument();
   if (doc) {
-    doc->FlushPendingNotifications(FlushType::Style);
+    doc->FlushPendingNotifications(
+      ChangesToFlush(FlushType::Style, false /* flush animations */));
   }
 }
 
 void
 Animation::PostUpdate()
 {
   if (!mEffect) {
     return;
--- a/dom/animation/Animation.h
+++ b/dom/animation/Animation.h
@@ -427,17 +427,21 @@ protected:
     Async
   };
 
   virtual void UpdateTiming(SeekFlag aSeekFlag,
                             SyncNotifyFlag aSyncNotifyFlag);
   void UpdateFinishedState(SeekFlag aSeekFlag,
                            SyncNotifyFlag aSyncNotifyFlag);
   void UpdateEffect();
-  void FlushStyle() const;
+  /**
+   * Flush all pending styles other than throttled animation styles (e.g.
+   * animations running on the compositor).
+   */
+  void FlushUnanimatedStyle() const;
   void PostUpdate();
   void ResetFinishedPromise();
   void MaybeResolveFinishedPromise();
   void DoFinishNotification(SyncNotifyFlag aSyncNotifyFlag);
   friend class AsyncFinishNotification;
   void DoFinishNotificationImmediately(MicroTaskRunnable* aAsync = nullptr);
   void DispatchPlaybackEvent(const nsAString& aName);
 
--- a/dom/animation/test/mozilla/file_restyles.html
+++ b/dom/animation/test/mozilla/file_restyles.html
@@ -1802,12 +1802,113 @@ waitForAllPaints(() => {
       is(markers.length, 2, // One is done through UpdateOnlyAnimationStyles(),
                             // the other is for discarding the animation.
          'Element.getAnimations() should flush throttled animation style that ' +
          'the throttled animation is discarded');
     }
 
     await ensureElementRemoval(div);
   });
+
+  add_task(
+    async function no_restyling_for_throttled_animation_on_querying_play_state() {
+      var div = addDiv(null, { style: 'animation: opacity 100s' });
+      var animation = div.getAnimations()[0];
+      var sibling = addDiv(null);
+
+      await animation.ready;
+      ok(SpecialPowers.wrap(animation).isRunningOnCompositor);
+
+      var markers = observeAnimSyncStyling(() => {
+        sibling.style.opacity = '0.5';
+        is(animation.playState, 'running',
+           'Animation.playState should be running');
+      });
+      is(markers.length, 0,
+         'Animation.playState should not flush throttled animation in the ' +
+         'case where there are only style changes that don\'t affect the ' +
+         'throttled animation');
+
+      await ensureElementRemoval(div);
+      await ensureElementRemoval(sibling);
+    }
+  );
+
+  add_task(
+    async function restyling_for_throttled_animation_on_querying_play_state() {
+      var div = addDiv(null, { style: 'animation: opacity 100s' });
+      var animation = div.getAnimations()[0];
+
+      await animation.ready;
+      ok(SpecialPowers.wrap(animation).isRunningOnCompositor);
+
+      var markers = observeAnimSyncStyling(() => {
+        div.style.animationPlayState = 'paused';
+        is(animation.playState, 'paused',
+           'Animation.playState should be reflected by pending style');
+      });
+
+      is(markers.length, 1,
+         'Animation.playState should flush throttled animation style that ' +
+         'affects the throttled animation');
+
+      await ensureElementRemoval(div);
+    }
+  );
+
+  add_task(
+    async function no_restyling_for_throttled_transition_on_querying_play_state() {
+      var div = addDiv(null, { style: 'transition: opacity 100s; opacity: 0' });
+      var sibling = addDiv(null);
+
+      getComputedStyle(div).opacity;
+      div.style.opacity = 1;
+
+      var transition = div.getAnimations()[0];
+
+      await transition.ready;
+      ok(SpecialPowers.wrap(transition).isRunningOnCompositor);
+
+      var markers = observeAnimSyncStyling(() => {
+        sibling.style.opacity = '0.5';
+        is(transition.playState, 'running',
+           'Animation.playState should be running');
+      });
+
+      is(markers.length, 0,
+         'Animation.playState should not flush throttled transition in the ' +
+         'case where there are only style changes that don\'t affect the ' +
+         'throttled animation');
+
+      await ensureElementRemoval(div);
+      await ensureElementRemoval(sibling);
+    }
+  );
+
+  add_task(
+    async function restyling_for_throttled_transition_on_querying_play_state() {
+      var div = addDiv(null, { style: 'transition: opacity 100s; opacity: 0' });
+      getComputedStyle(div).opacity;
+      div.style.opacity = '1';
+
+      var transition = div.getAnimations()[0];
+
+      await transition.ready;
+      ok(SpecialPowers.wrap(transition).isRunningOnCompositor);
+
+      var markers = observeAnimSyncStyling(() => {
+        div.style.transitionProperty = 'none';
+        is(transition.playState, 'idle',
+           'Animation.playState should be reflected by pending style change ' +
+           'which cancel the transition');
+      });
+
+      is(markers.length, 1,
+         'Animation.playState should flush throttled transition style that ' +
+         'affects the throttled animation');
+
+      await ensureElementRemoval(div);
+    }
+  );
 });
 
 </script>
 </body>
--- a/layout/style/nsAnimationManager.cpp
+++ b/layout/style/nsAnimationManager.cpp
@@ -48,17 +48,17 @@ JSObject*
 CSSAnimation::WrapObject(JSContext* aCx, JS::Handle<JSObject*> aGivenProto)
 {
   return dom::CSSAnimationBinding::Wrap(aCx, this, aGivenProto);
 }
 
 mozilla::dom::Promise*
 CSSAnimation::GetReady(ErrorResult& aRv)
 {
-  FlushStyle();
+  FlushUnanimatedStyle();
   return Animation::GetReady(aRv);
 }
 
 void
 CSSAnimation::Play(ErrorResult &aRv, LimitBehavior aLimitBehavior)
 {
   mPauseShouldStick = false;
   Animation::Play(aRv, aLimitBehavior);
@@ -71,35 +71,35 @@ CSSAnimation::Pause(ErrorResult& aRv)
   Animation::Pause(aRv);
 }
 
 AnimationPlayState
 CSSAnimation::PlayStateFromJS() const
 {
   // Flush style to ensure that any properties controlling animation state
   // (e.g. animation-play-state) are fully updated.
-  FlushStyle();
+  FlushUnanimatedStyle();
   return Animation::PlayStateFromJS();
 }
 
 bool
 CSSAnimation::PendingFromJS() const
 {
   // Flush style since, for example, if the animation-play-state was just
   // changed its possible we should now be pending.
-  FlushStyle();
+  FlushUnanimatedStyle();
   return Animation::PendingFromJS();
 }
 
 void
 CSSAnimation::PlayFromJS(ErrorResult& aRv)
 {
   // Note that flushing style below might trigger calls to
   // PlayFromStyle()/PauseFromStyle() on this object.
-  FlushStyle();
+  FlushUnanimatedStyle();
   Animation::PlayFromJS(aRv);
 }
 
 void
 CSSAnimation::PlayFromStyle()
 {
   mIsStylePaused = false;
   if (!mPauseShouldStick) {
--- a/layout/style/nsTransitionManager.cpp
+++ b/layout/style/nsTransitionManager.cpp
@@ -141,37 +141,37 @@ CSSTransition::GetTransitionProperty(nsS
              "Transition Property should be initialized");
   aRetVal =
     NS_ConvertUTF8toUTF16(nsCSSProps::GetStringValue(mTransitionProperty));
 }
 
 AnimationPlayState
 CSSTransition::PlayStateFromJS() const
 {
-  FlushStyle();
+  FlushUnanimatedStyle();
   return Animation::PlayStateFromJS();
 }
 
 bool
 CSSTransition::PendingFromJS() const
 {
   // Transitions don't become pending again after they start running but, if
   // while the transition is still pending, style is updated in such a way
   // that the transition will be canceled, we need to report false here.
   // Hence we need to flush, but only when we're pending.
   if (Pending()) {
-    FlushStyle();
+    FlushUnanimatedStyle();
   }
   return Animation::PendingFromJS();
 }
 
 void
 CSSTransition::PlayFromJS(ErrorResult& aRv)
 {
-  FlushStyle();
+  FlushUnanimatedStyle();
   Animation::PlayFromJS(aRv);
 }
 
 void
 CSSTransition::UpdateTiming(SeekFlag aSeekFlag, SyncNotifyFlag aSyncNotifyFlag)
 {
   if (mNeedsNewAnimationIndexWhenRun &&
       PlayState() != AnimationPlayState::Idle) {