Consider this markup: <div class="container animates" onAnimationEnd="event.target.classList.remove('animates')"> <div class="text">Hello.</div> </div> With these style rules: .container { margin-left: 100px; margin-top: 100px; } .animates { animation: fade 1s forwards; } .text { position: relative; } @keyframes fade { from { opacity: 0; } to { opacity: 1; } } As the animation runs, container has a layer because it has opacity < 1 and when itβs laid out originally it has a non-0,0 position. Meanwhile .text always has a layer because it has position: relative and it has a 0,0 position. As the animation ends, container still has a layer because its animation has fill: forwards and so we set setUsedZIndex(0) in KeyframeEffect::apply() because the spec tells us to (https://w3c.github.io/web-animations/#side-effects-section) since the animation targets opacity, a property which impacts whether an element establishes a stacking context. As the animation is removed, then the old style for container has usedZIndex() == 0 while the new style has hasAutoUsedZIndex() == true, which is expected as the animation is gone. However, when we get in that state RenderStyle::diff() will only return RepaintLayer and not Layout.
<rdar://problem/45311541>
Created attachment 384332 [details] Patch
Comment on attachment 384332 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=384332&action=review > Source/WebCore/rendering/RenderLayerModelObject.cpp:188 > + // If the layer we're about to destroy had a position, then the layout of children will no longer be correct. > + bool layerHadLocation = !layer()->location().isZero(); I wonder if there is some existing way to just update layer positions instead of triggering a layout in render tree...
(In reply to Antti Koivisto from comment #3) > Comment on attachment 384332 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=384332&action=review > > > Source/WebCore/rendering/RenderLayerModelObject.cpp:188 > > + // If the layer we're about to destroy had a position, then the layout of children will no longer be correct. > > + bool layerHadLocation = !layer()->location().isZero(); > > I wonder if there is some existing way to just update layer positions > instead of triggering a layout in render tree... There is RenderLayer::updateLayerPositionsAfterStyleChange() which I could call on all the children I suppose. I'll give it a try.
Created attachment 384334 [details] Patch
Comment on attachment 384334 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=384334&action=review > Source/WebCore/rendering/RenderLayerModelObject.cpp:192 > + if (layerHadLocation && parentLayer) > + parentLayer->updateLayerPositionsAfterStyleChange(); Maybe you should also add !needsLayout() check here (like the other call site) as this is likely not needed then.
I suppose that should be parentLayer->renderer().needsLayout() then.
Created attachment 384335 [details] Patch
Committed r252879: <https://trac.webkit.org/changeset/252879>
Comment on attachment 384335 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=384335&action=review > Source/WebCore/rendering/RenderLayerModelObject.cpp:192 > + parentLayer->updateLayerPositionsAfterStyleChange(); Nooo, you can't call this here. This needs to happen after the entire style update. This is why we need some new RenderStyle::Diff values.
Please revert. Give the bug to me.
(In reply to Simon Fraser (smfr) from comment #10) > Comment on attachment 384335 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=384335&action=review > > > Source/WebCore/rendering/RenderLayerModelObject.cpp:192 > > + parentLayer->updateLayerPositionsAfterStyleChange(); > > Nooo, you can't call this here. This needs to happen after the entire style > update. This is why we need some new RenderStyle::Diff values. Why is not good to call this here? That method is also called in RenderLayerModelObject::styleDidChange() in the case where we destroy a layer, although it's called on a different layer (the destroyed one).
This is the same issue as bug 202505.
(In reply to Antoine Quint from comment #12) > (In reply to Simon Fraser (smfr) from comment #10) > > Comment on attachment 384335 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=384335&action=review > > > > > Source/WebCore/rendering/RenderLayerModelObject.cpp:192 > > > + parentLayer->updateLayerPositionsAfterStyleChange(); > > > > Nooo, you can't call this here. This needs to happen after the entire style > > update. This is why we need some new RenderStyle::Diff values. > > Why is not good to call this here? That method is also called in > RenderLayerModelObject::styleDidChange() in the case where we destroy a > layer, although it's called on a different layer (the destroyed one). updateLayerPositionsAfterStyleChange walks all the descendant layers, so if you cal it from here, you risk walking some subtrees multiple times. updateLayerPositionsAfterStyleChange() is intended to called once on the root.
I improved on this change in r252935.