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
--- 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>