RESOLVED FIXED 204602
[Web Animations] Layout of children of element with forwards-filling opacity animation may be incorrect after removal
https://bugs.webkit.org/show_bug.cgi?id=204602
Summary [Web Animations] Layout of children of element with forwards-filling opacity ...
Antoine Quint
Reported 2019-11-26 00:32:46 PST
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.
Attachments
Patch (7.11 KB, patch)
2019-11-26 00:43 PST, Antoine Quint
no flags
Patch (7.19 KB, patch)
2019-11-26 00:56 PST, Antoine Quint
no flags
Patch (7.18 KB, patch)
2019-11-26 01:05 PST, Antoine Quint
simon.fraser: review-
Antoine Quint
Comment 1 2019-11-26 00:33:11 PST
Antoine Quint
Comment 2 2019-11-26 00:43:23 PST
Antti Koivisto
Comment 3 2019-11-26 00:46:33 PST
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...
Antoine Quint
Comment 4 2019-11-26 00:48:28 PST
(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.
Antoine Quint
Comment 5 2019-11-26 00:56:13 PST
Antti Koivisto
Comment 6 2019-11-26 01:02:13 PST
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.
Antti Koivisto
Comment 7 2019-11-26 01:04:34 PST
I suppose that should be parentLayer->renderer().needsLayout() then.
Antoine Quint
Comment 8 2019-11-26 01:05:26 PST
Antoine Quint
Comment 9 2019-11-26 02:12:05 PST
Simon Fraser (smfr)
Comment 10 2019-11-26 10:11:20 PST
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.
Simon Fraser (smfr)
Comment 11 2019-11-26 10:13:00 PST
Please revert. Give the bug to me.
Antoine Quint
Comment 12 2019-11-27 04:04:29 PST
(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).
Simon Fraser (smfr)
Comment 13 2019-11-28 07:53:22 PST
This is the same issue as bug 202505.
Simon Fraser (smfr)
Comment 14 2019-11-28 07:54:12 PST
(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.
Simon Fraser (smfr)
Comment 15 2019-11-28 22:51:14 PST
I improved on this change in r252935.
Note You need to log in before you can comment on or make changes to this bug.