[Web Animations] Layout of children of element with forwards-filling opacity animation may be incorrect after removal

[Antoine Quint]

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.

[Antoine Quint]

<rdar://problem/45311541>

[Antoine Quint]

Created attachment 384332 [details]
Patch

[Antti Koivisto]

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]

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

Created attachment 384334 [details]
Patch

[Antti Koivisto]

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]

I suppose that should be parentLayer->renderer().needsLayout() then.

[Antoine Quint]

Created attachment 384335 [details]
Patch

[Antoine Quint]

Committed r252879: <https://trac.webkit.org/changeset/252879>

[Simon Fraser (smfr)]

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

Please revert. Give the bug to me.

[Antoine Quint]

(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)]

This is the same issue as bug 202505.

[Simon Fraser (smfr)]

(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)]

I improved on this change in r252935.