Bug 1370123 - Skip restyling elements in documents without a pres shell; r?heycam draft
authorBrian Birtles <birtles@gmail.com>
Wed, 21 Jun 2017 14:45:24 +0900
changeset 606642 752f38bf91c358e5ab70760dbd2ffd5c3192cbb2
parent 606641 f83a5f13ab8611d18611769c04eea71c59c3b63b
child 636820 4b52a54d392f10d8bb31a5d5d7ac60695da1cc3e
push id67758
push userbbirtles@mozilla.com
push dateTue, 11 Jul 2017 07:47:52 +0000
reviewersheycam
bugs1370123
milestone56.0a1
Bug 1370123 - Skip restyling elements in documents without a pres shell; r?heycam The previous patch takes the approach that we should simply not add elements in documents without a pres shell to EffectCompositor's set of elements to restyle. However, there exists a case where we might have an element in a displayed document, then we might tickle it so that it requests an animation restyle, and then move it to a document without a browsing context. In that case we should skip the element when we next do animation restyles. However, even if we successfully skip the element in the document without a pres shell, we need to make sure it eventually gets removed from the set of elements to restyle rather than simply remaining there forever. For that reason this patch makes us unconditionally clear the set of elements to restyle whenever we do a full restyle from the root. This patch also adds a test case to trigger the scenario outlined in the first paragraph above. I have confirmed that without the code changes in this patch, if we simply assert that target.mElement has an associated pres shell in getNeededRestyleTarget, then that assertion will fail when running this test case. MozReview-Commit-ID: ED2X5g39hYZ
dom/animation/EffectCompositor.cpp
dom/animation/test/mozilla/file_restyling_xhr_doc.html
--- a/dom/animation/EffectCompositor.cpp
+++ b/dom/animation/EffectCompositor.cpp
@@ -995,16 +995,27 @@ EffectCompositor::PreTraverseInSubtree(E
     // (skippable) restyle, so we can skip it if flushThrottledRestyles is not
     // true.
     if (!flushThrottledRestyles && !aIter.Data()) {
       return returnTarget;
     }
 
     const NonOwningAnimationTarget& target = aIter.Key();
 
+    // Skip elements in documents without a pres shell. Normally we filter out
+    // such elements in RequestRestyle but it can happen that, after adding
+    // them to mElementsToRestyle, they are transferred to a different document.
+    //
+    // We will drop them from mElementsToRestyle at the end of the next full
+    // document restyle (at the end of this function) but for consistency with
+    // how we treat such elements in RequestRestyle, we just ignore them here.
+    if (!nsComputedDOMStyle::GetPresShellForContent(target.mElement)) {
+      return returnTarget;
+    }
+
     // Ignore restyles that aren't in the flattened tree subtree rooted at
     // aRoot.
     if (aRoot &&
         !nsContentUtils::ContentIsFlattenedTreeDescendantOf(target.mElement,
                                                             aRoot)) {
       return returnTarget;
     }
 
@@ -1075,17 +1086,26 @@ EffectCompositor::PreTraverseInSubtree(E
       for (KeyframeEffectReadOnly* effect : *effects) {
         effect->GetAnimation()->WillComposeStyle();
       }
 
       // Remove the element from the list of elements to restyle since we are
       // about to restyle it.
       iter.Remove();
     }
+
+    // If this is a full document restyle, then unconditionally clear
+    // elementSet in case there are any elements that didn't match above
+    // because they were moved to a document without a pres shell after
+    // posting an animation restyle.
+    if (!aRoot && flushThrottledRestyles) {
+      elementSet.Clear();
+    }
   }
+
   return foundElementsNeedingRestyle;
 }
 
 bool
 EffectCompositor::PreTraverse(dom::Element* aElement,
                               CSSPseudoElementType aPseudoType)
 {
   MOZ_ASSERT(NS_IsMainThread());
--- a/dom/animation/test/mozilla/file_restyling_xhr_doc.html
+++ b/dom/animation/test/mozilla/file_restyling_xhr_doc.html
@@ -76,11 +76,47 @@ promise_test(t => {
     // Gecko currently skips applying animation styles to elements in documents
     // without browsing contexts.
     assert_not_equals(getComputedStyle(div).opacity, '0',
                       'Style should NOT be updated');
   });
 }, 'Forcing an animation targetting an element in a document without a'
     + ' browsing context to play does not cause style to update');
 
+// Following is an additional Gecko-specific test to confirm the behavior
+// when we have an element with an animation restyle and then move it to
+// a document without a pres shell.
+
+promise_test(t => {
+  let anim;
+  return getXHRDoc(t).then(xhrdoc => {
+    const div = addDiv(t);
+    anim = div.animate({ opacity: [ 0, 1 ] }, 1000);
+    assert_equals(getComputedStyle(div).opacity, '0',
+                  'Style should be updated');
+    // Trigger an animation restyle to be queued
+    anim.currentTime = 0.1;
+    // Adopt node into XHR doc
+    xhrdoc.body.appendChild(div);
+    // We should skip applying animation styles to elements in documents
+    // without a pres shell.
+    //
+    // The Gecko style backend, however, does not do this. Since we expect the
+    // Gecko style backend to be obsolete in the near future, we only perform
+    // this check when the Servo backend is in use.
+    let isServo = false;
+    try {
+      isServo = SpecialPowers.getBoolPref('layout.css.servo.enabled');
+    } catch(e) {
+      // getBoolPref throws if the pref does not exist, in which case isServo
+      // should remain false.
+    }
+    if (isServo) {
+      assert_equals(getComputedStyle(div).opacity, '1',
+                    'Style should NOT be updated');
+    }
+  });
+}, 'Moving an element with a pending animation restyle to a document without'
+   + ' a browsing context resets animation style');
+
 done();
 
 </script>