Bug 1478643 - Apply change hints for transform animation in the case where the current transform style is 'none' and no change hint produced in AddLayerChangesForAnimation. r?birtles draft
authorHiroyuki Ikezoe <hikezoe@mozilla.com>
Mon, 30 Jul 2018 11:13:54 +0900
changeset 823943 b3f7c89f623da139b45cd5b7a80cf19ccc4e08b0
parent 823942 1ef094e620ba5bb54c00d9b6e214921aa09eac93
child 823946 02e0f0a002b1f7c0ffc24b946cbfd87b22650276
push id117826
push userhikezoe@mozilla.com
push dateMon, 30 Jul 2018 02:16:01 +0000
reviewersbirtles
bugs1478643
milestone63.0a1
Bug 1478643 - Apply change hints for transform animation in the case where the current transform style is 'none' and no change hint produced in AddLayerChangesForAnimation. r?birtles The applying change hints are the same as what we apply for transform style changed from something to 'none'. All test cases pass with this fix fail without the fix. MozReview-Commit-ID: 7HStU26lRPq
dom/animation/test/mozilla/test_style_after_finished_on_compositor.html
layout/base/RestyleManager.cpp
layout/base/RestyleManager.h
--- a/dom/animation/test/mozilla/test_style_after_finished_on_compositor.html
+++ b/dom/animation/test/mozilla/test_style_after_finished_on_compositor.html
@@ -71,10 +71,68 @@ promise_test(async t => {
   await waitForNextFrame();
 
   const opacity = SpecialPowers.DOMWindowUtils.getOMTAStyle(div, 'opacity');
 
   assert_equals(opacity, '', 'No opacity animation runs on the compositor');
 }, 'Opacity animation is removed from compositor even when it only visits ' +
    'exactly the point where the opacity: 1 value was set');
 
+promise_test(async t => {
+  const div = addDiv(t, { 'class': 'compositor' });
+  const anim = div.animate([ { offset: 0, transform: 'none' },
+                             { offset: 1, transform: 'translateX(100px)' } ],
+                           { delay: 10,
+                             duration: 100 });
+
+  await anim.finished;
+
+  await waitForNextFrame();
+
+  const transform = SpecialPowers.DOMWindowUtils.getOMTAStyle(div, 'transform');
+
+  assert_equals(transform, '', 'No transform animation runs on the compositor');
+}, 'Transform animation with positive delay is removed from compositor when ' +
+   'finished');
+
+promise_test(async t => {
+  const div = addDiv(t, { 'class': 'compositor' });
+  const anim = div.animate([ { offset: 0,   transform: 'none' },
+                             { offset: 0.9, transform: 'none' },
+                             { offset: 1,   transform: 'translateX(100px)' } ],
+                           { duration: 100 });
+
+  await anim.finished;
+
+  await waitForNextFrame();
+
+  const transform = SpecialPowers.DOMWindowUtils.getOMTAStyle(div, 'transform');
+
+  assert_equals(transform, '', 'No transform animation runs on the compositor');
+}, 'Transform animation initially transform: none is removed from compositor ' +
+   'when finished');
+
+
+promise_test(async t => {
+  const div = addDiv(t, { 'class': 'compositor' });
+  const anim = div.animate([ { offset: 0,   transform: 'translateX(100px)' },
+                             { offset: 0.5, transform: 'none' },
+                             { offset: 0.9, transform: 'none' },
+                             { offset: 1,   transform: 'translateX(100px)' } ],
+                           { delay: 10, duration: 100 });
+
+  await waitForAnimationFrames(2);
+
+  // Setting the current time at the offset generating transform: none.
+  anim.currentTime = 60;
+
+  await anim.finished;
+
+  await waitForNextFrame();
+
+  const transform = SpecialPowers.DOMWindowUtils.getOMTAStyle(div, 'transform');
+
+  assert_equals(transform, '', 'No transform animation runs on the compositor');
+}, 'Transform animation is removed from compositor even when it only visits ' +
+   'exactly the point where the transform: none value was set');
+
 </script>
 </body>
--- a/layout/base/RestyleManager.cpp
+++ b/layout/base/RestyleManager.cpp
@@ -1743,41 +1743,61 @@ RestyleManager::IncrementAnimationGenera
   if (!mInStyleRefresh) {
     ++mAnimationGeneration;
   }
 }
 
 /* static */ void
 RestyleManager::AddLayerChangesForAnimation(nsIFrame* aFrame,
                                             nsIContent* aContent,
+                                            nsChangeHint aHintForThisFrame,
                                             nsStyleChangeList&
                                               aChangeListToProcess)
 {
   if (!aFrame || !aContent) {
     return;
   }
 
   uint64_t frameGeneration =
     RestyleManager::GetAnimationGenerationForFrame(aFrame);
 
   nsChangeHint hint = nsChangeHint(0);
   for (const LayerAnimationInfo::Record& layerInfo :
          LayerAnimationInfo::sRecords) {
     layers::Layer* layer =
       FrameLayerBuilder::GetDedicatedLayer(aFrame, layerInfo.mLayerType);
     if (layer && frameGeneration != layer->GetAnimationGeneration()) {
-      // If we have a transform layer but don't have any transform style, we
+      // If we have a transform layer bug don't have any transform style, we
       // probably just removed the transform but haven't destroyed the layer
-      // yet. In this case we will add the appropriate change hint
-      // (nsChangeHint_UpdateContainingBlock) when we compare styles so we can
-      // skip adding any change hint here. (If we *were* to add
-      // nsChangeHint_UpdateTransformLayer, ApplyRenderingChangeToTree would
-      // complain that we're updating a transform layer without a transform).
+      // yet. In this case we will typically add the appropriate change hint
+      // (nsChangeHint_UpdateContainingBlock) when we compare styles so in
+      // theory we could skip adding any change hint here.
+      //
+      // However, sometimes when we compare styles we'll get no change. For
+      // example, if the transform style was 'none' when we sent the transform
+      // animation to the compositor and the current transform style is now
+      // 'none' we'll think nothing changed but actually we still need to
+      // trigger an update to clear whatever style the transform animation set
+      // on the compositor. To handle this case we simply set all the change
+      // hints relevant to removing transform style (since we don't know exactly
+      // what changes happened while the animation was running on the
+      // compositor).
+      //
+      // Note that we *don't* add nsChangeHint_UpdateTransformLayer since if we
+      // did, ApplyRenderingChangeToTree would complain that we're updating a
+      // transform layer without a transform.
       if (layerInfo.mLayerType == DisplayItemType::TYPE_TRANSFORM &&
           !aFrame->StyleDisplay()->HasTransformStyle()) {
+        // Add all the hints for a removing a transform if they are not already
+        // set for this frame.
+        if (!(NS_IsHintSubset(
+                nsChangeHint_ComprehensiveAddOrRemoveTransform,
+                aHintForThisFrame))) {
+          hint |= nsChangeHint_ComprehensiveAddOrRemoveTransform;
+        }
         continue;
       }
       hint |= layerInfo.mChangeHint;
     }
 
     // We consider it's the first paint for the frame if we have an animation
     // for the property but have no layer.
     // Note that in case of animations which has properties preventing running
@@ -2744,17 +2764,17 @@ RestyleManager::ProcessPostTraversal(
     // the Web Animations API won't affect an element's style but still
     // requires to update the animation on the layer.
     //
     // We can sometimes reach this when the animated style is being removed.
     // Since AddLayerChangesForAnimation checks if |styleFrame| has a transform
     // style or not, we need to call it *after* setting |newStyle| to
     // |styleFrame| to ensure the animated transform has been removed first.
     AddLayerChangesForAnimation(
-      styleFrame, aElement, aRestyleState.ChangeList());
+      styleFrame, aElement, changeHint, aRestyleState.ChangeList());
 
     childrenFlags |= SendA11yNotifications(mPresContext,
                                            aElement,
                                            oldOrDisplayContentsStyle,
                                            upToDateContext,
                                            aFlags);
   }
 
--- a/layout/base/RestyleManager.h
+++ b/layout/base/RestyleManager.h
@@ -392,18 +392,30 @@ public:
   // Update the animation generation count to mark that animation state
   // has changed.
   //
   // This is normally performed automatically by ProcessPendingRestyles
   // but it is also called when we have out-of-band changes to animations
   // such as changes made through the Web Animations API.
   void IncrementAnimationGeneration();
 
+  // Apply change hints for animations on the compositor.
+  //
+  // There are some cases where we forcibly apply change hints for animations
+  // even if there is no change hint produced in order to synchronize with
+  // animations running on the compositor.
+  //
+  // For example:
+  //
+  // a) Pausing animations via the Web Animations API
+  // b) When the style before sending the animation to the compositor exactly
+  // the same as the current style
   static void AddLayerChangesForAnimation(nsIFrame* aFrame,
                                           nsIContent* aContent,
+                                          nsChangeHint aHintForThisFrame,
                                           nsStyleChangeList&
                                             aChangeListToProcess);
 
   /**
    * Whether to clear all the style data (including the element itself), or just
    * the descendants' data.
    */
   enum class IncludeRoot {