Bug 204602

Summary: [Web Animations] Layout of children of element with forwards-filling opacity animation may be incorrect after removal
Product: WebKit Reporter: Antoine Quint <graouts>
Component: AnimationsAssignee: Antoine Quint <graouts>
Status: RESOLVED FIXED    
Severity: Normal CC: dino, esprehn+autocc, ews-watchlist, fred.wang, glenn, koivisto, kondapallykalyan, pdr, simon.fraser, thorton, webkit-bug-importer, zalan
Priority: P2 Keywords: InRadar
Version: Safari Technology Preview   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=202505
Attachments:
Description Flags
Patch
none
Patch
none
Patch simon.fraser: review-

Description Antoine Quint 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.
Comment 1 Antoine Quint 2019-11-26 00:33:11 PST
<rdar://problem/45311541>
Comment 2 Antoine Quint 2019-11-26 00:43:23 PST
Created attachment 384332 [details]
Patch
Comment 3 Antti Koivisto 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...
Comment 4 Antoine Quint 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.
Comment 5 Antoine Quint 2019-11-26 00:56:13 PST
Created attachment 384334 [details]
Patch
Comment 6 Antti Koivisto 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.
Comment 7 Antti Koivisto 2019-11-26 01:04:34 PST
I suppose that should be parentLayer->renderer().needsLayout() then.
Comment 8 Antoine Quint 2019-11-26 01:05:26 PST
Created attachment 384335 [details]
Patch
Comment 9 Antoine Quint 2019-11-26 02:12:05 PST
Committed r252879: <https://trac.webkit.org/changeset/252879>
Comment 10 Simon Fraser (smfr) 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.
Comment 11 Simon Fraser (smfr) 2019-11-26 10:13:00 PST
Please revert. Give the bug to me.
Comment 12 Antoine Quint 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).
Comment 13 Simon Fraser (smfr) 2019-11-28 07:53:22 PST
This is the same issue as bug 202505.
Comment 14 Simon Fraser (smfr) 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.
Comment 15 Simon Fraser (smfr) 2019-11-28 22:51:14 PST
I improved on this change in r252935.