corysanin/tubestation
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Bug 1816915 - Tweak how we handle animation restyles on bind/unbind. r=hiro

Browse Source 
Remove code on bind/unbind that requested a restyle on an unstyled
element, and that canceled that on an unbound element.

Instead, deal with detached nodes in EffectCompositor.

Tweak test restyles since we now expect one restyle as a result of the
initial composing of the animation that before happened eagerly.

Drive-by remove an unused test function in wpt (animationStartsRightNow
is not defined there anyways).

In practice, this makes it consistent with how we handle bind on
elements in shadow trees.

Differential Revision: https://phabricator.services.mozilla.com/D169932
This commit is contained in:
Emilio Cobos Álvarez
2023-02-21 08:51:00 +00:00
parent 69e41de443
commit fe8e5d2f7f
5 changed files with 22 additions and 53 deletions
Download Patch File Download Diff File Expand all files Collapse all files

5
dom/animation/EffectCompositor.cpp
View File

@@ -895,6 +895,11 @@ bool EffectCompositor::PreTraverseInSubtree(ServoTraversalFlags aFlags,
continue;
}
if (target.mElement->GetComposedDoc() != mPresContext->Document()) {
iter.Remove();
continue;
}
// We need to post restyle hints even if the target is not in EffectSet to
// ensure the final restyling for removed animations.
// We can't call PostRestyleEvent directly here since we are still in the

24
dom/animation/test/mozilla/file_restyles.html
View File

@@ -1212,9 +1212,11 @@ waitForAllPaints(() => {
document.body.appendChild(div);
await waitForNextFrame();
markers = await observeStyling(5);
is(markers.length, 5,
'Animation on re-attached to the document begins to update style');
'Animation on re-attached to the document begins to update style, got ' + markers.length);
await ensureElementRemoval(div);
});
@@ -1337,9 +1339,11 @@ waitForAllPaints(() => {
const markers = await observeStyling(5);
is(markers.length, 0,
// We possibly expect one restyle from the initial animation compose, in
// order to update animations, but nothing else.
ok(markers.length <= 1,
'Animation running on the main-thread while computed timing is not ' +
'changed should never cause restyles');
'changed should not cause extra restyles, got ' + markers.length);
await ensureElementRemoval(div);
});
@@ -2001,15 +2005,19 @@ waitForAllPaints(() => {
easing: 'step-end' });
const FLUSH_LAYOUT = SpecialPowers.DOMWindowUtils.FLUSH_LAYOUT;
const isWebRender =
SpecialPowers.DOMWindowUtils.layerManagerType.startsWith('WebRender');
ok(SpecialPowers.DOMWindowUtils.needsFlush(FLUSH_LAYOUT),
'Flush layout is needed for the appended div');
'Flush is needed for the appended div');
await waitForAnimationReadyToRestyle(animation);
ok(!SpecialPowers.wrap(animation).isRunningOnCompositor);
ok(!SpecialPowers.wrap(animation).isRunningOnCompositor, "Shouldn't be running in the compositor");
// We expect one restyle from the initial animation compose.
await waitForNextFrame();
ok(!SpecialPowers.wrap(animation).isRunningOnCompositor, "Still shouldn't be running in the compositor");
ok(!SpecialPowers.DOMWindowUtils.needsFlush(FLUSH_LAYOUT),
'No further flush layout needed');
'No further layout flush needed');
await ensureElementRemoval(div);
});

32
dom/base/Element.cpp
View File

@@ -1862,30 +1862,6 @@ nsresult Element::BindToTree(BindContext& aContext, nsINode& aParent) {
/* aForceInDataDoc = */ false);
}
// FIXME(emilio): Why is this needed? The element shouldn't even be styled in
// the first place, we should style it properly eventually.
//
// This check is rather broken:
//
// * Should use GetComposedDoc() to account for shadow DOM.
// * The GetEffectSet check is broken and only really works if pseudoType ==
// PseudoStyleType::NotPseudo.
//
// Removing this chunk of code causes failures in test_restyles.html. Some of
// them look benign, but another one at least looks like a real bug.
if (aParent.IsInUncomposedDoc() && MayHaveAnimations()) {
PseudoStyleType pseudoType = GetPseudoElementType();
if ((pseudoType == PseudoStyleType::NotPseudo ||
AnimationUtils::IsSupportedPseudoForAnimations(pseudoType)) &&
EffectSet::Get(this, pseudoType)) {
if (nsPresContext* presContext = aContext.OwnerDoc().GetPresContext()) {
presContext->EffectCompositor()->RequestRestyle(
this, pseudoType, EffectCompositor::RestyleType::Standard,
EffectCompositor::CascadeLevel::Animations);
}
}
}
// XXXbz script execution during binding can trigger some of these
// postcondition asserts.... But we do want that, since things will
// generally be quite broken when that happens.
@@ -1973,14 +1949,6 @@ void Element::UnbindFromTree(bool aNullParent) {
// FIXME(emilio): Why not clearing the effect set as well?
if (auto* data = GetAnimationData()) {
data->ClearAllAnimationCollections();
if (document) {
if (nsPresContext* presContext = document->GetPresContext()) {
// We have to clear all pending restyle requests for the animations on
// this element to avoid unnecessary restyles if/when we re-attach this
// element.
presContext->EffectCompositor()->ClearRestyleRequestsFor(this);
}
}
}
if (aNullParent) {

2
layout/base/PresShell.cpp
View File

@@ -3047,7 +3047,7 @@ void PresShell::RestyleForAnimation(Element* aElement, RestyleHint aHint) {
// Now that we no longer have separate non-animation and animation
// restyles, this method having a distinct identity is less important,
// but it still seems useful to offer as a "more public" API and as a
// chokepoint for these restyles to go through.
// checkpoint for these restyles to go through.
mPresContext->RestyleManager()->PostRestyleEvent(aElement, aHint,
nsChangeHint(0));
}

12
testing/web-platform/tests/css/css-animations/support/testcommon.js
View File

@@ -247,15 +247,3 @@ function flushComputedStyle(elem) {
var cs = getComputedStyle(elem);
cs.marginLeft;
}
// Waits for a given animation being ready to restyle.
async function waitForAnimationReadyToRestyle(aAnimation) {
await aAnimation.ready;
// If |aAnimation| begins at the current timeline time, we will not process
// restyling in the initial frame because of aligning with the refresh driver,
// the animation frame in which the ready promise is resolved happens to
// coincide perfectly with the start time of the animation. In this case no
// restyling is needed in the frame so we have to wait one more frame.
if (animationStartsRightNow(aAnimation)) {
await waitForNextFrame();
}
}