Changeset 252879 in webkit

Timestamp:

Nov 26, 2019 2:11:55 AM (4 years ago)

Author:

graouts@webkit.org

Message:

[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
<rdar://problem/45311541>

Reviewed by Antti Koivisto.

Source/WebCore:

Test: webanimations/child-layer-position-after-removal-of-animation-triggering-stacking-context-with-fill-forwards.html

In the case where we animate a property that affects whether an element establishes a stacking context, for instance opacity, the animation code,
specifically KeyframeEffect::apply(), forces a stacking context by setting setUsedZIndex(0) on the animated RenderStyle in case the element otherwise
has an "auto" z-index. This is required by the Web Animations specification (​https://w3c.github.io/web-animations/#side-effects-section).

This means that a fill-forwards animation will still force the element to establish a stacking context after the animation is no longer "active". When
we remove such an animation, it may go from having a z-index to not having one, unless it had an explicit z-index provided through style. When this change
happens, RenderStyle::diff() will merely return "RepaintLayer" and thus will not foce a layout. However, updating the positions of child layers may be
necessary as the animation being removed may mean that there may not be a RenderLayer associated with that element's renderer anymore, and if that RenderLayer
had a position, then the position of child layers will no longer be correct.

Now, in the case where we destroy a layer in RenderLayerModelObject::styleDidChange(), we check whether the layer had a position before it is removed, and
update the position of child layers if it did.

• rendering/RenderLayerModelObject.cpp:

(WebCore::RenderLayerModelObject::styleDidChange):

LayoutTests:

Add a new ref test that checks that removing a forwards-filling animation that triggers a stacking context (for instance, animating opacity)
after it has completed from an element affecting layout yields the correct layout.

• webanimations/child-layer-position-after-removal-of-animation-triggering-stacking-context-with-fill-forwards-expected.html: Added.
• webanimations/child-layer-position-after-removal-of-animation-triggering-stacking-context-with-fill-forwards.html: Added.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/webanimations/child-layer-position-after-removal-of-animation-triggering-stacking-context-with-fill-forwards-expected.html (added)
• LayoutTests/webanimations/child-layer-position-after-removal-of-animation-triggering-stacking-context-with-fill-forwards.html (added)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/rendering/RenderLayerModelObject.cpp (modified) (1 diff)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2019-11-26  Antoine Quint  <graouts@apple.com>
+
+        [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
+        <rdar://problem/45311541>
+
+        Reviewed by Antti Koivisto.
+
+        Add a new ref test that checks that removing a forwards-filling animation that triggers a stacking context (for instance, animating opacity)
+        after it has completed from an element affecting layout yields the correct layout.
+
+        * webanimations/child-layer-position-after-removal-of-animation-triggering-stacking-context-with-fill-forwards-expected.html: Added.
+        * webanimations/child-layer-position-after-removal-of-animation-triggering-stacking-context-with-fill-forwards.html: Added.
+
 2019-11-25  Fujii Hironori  <Hironori.Fujii@sony.com>
 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2019-11-26  Antoine Quint  <graouts@apple.com>
+
+        [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
+        <rdar://problem/45311541>
+
+        Reviewed by Antti Koivisto.
+
+        Test: webanimations/child-layer-position-after-removal-of-animation-triggering-stacking-context-with-fill-forwards.html
+
+        In the case where we animate a property that affects whether an element establishes a stacking context, for instance `opacity`, the animation code,
+        specifically KeyframeEffect::apply(), forces a stacking context by setting setUsedZIndex(0) on the animated RenderStyle in case the element otherwise
+        has an "auto" z-index. This is required by the Web Animations specification (https://w3c.github.io/web-animations/#side-effects-section).
+
+        This means that a fill-forwards animation will still force the element to establish a stacking context after the animation is no longer "active". When
+        we remove such an animation, it may go from having a z-index to not having one, unless it had an explicit z-index provided through style. When this change
+        happens, RenderStyle::diff() will merely return "RepaintLayer" and thus will not foce a layout. However, updating the positions of child layers may be
+        necessary as the animation being removed may mean that there may not be a RenderLayer associated with that element's renderer anymore, and if that RenderLayer
+        had a position, then the position of child layers will no longer be correct.
+
+        Now, in the case where we destroy a layer in RenderLayerModelObject::styleDidChange(), we check whether the layer had a position before it is removed, and
+        update the position of child layers if it did.
+
+        * rendering/RenderLayerModelObject.cpp:
+        (WebCore::RenderLayerModelObject::styleDidChange):
+
 2019-11-26  youenn fablet  <youenn@apple.com>
 

trunk/Source/WebCore/rendering/RenderLayerModelObject.cpp

@@ -185 +185 @@
         if (layer()->isSelfPaintingLayer() && layer()->repaintStatus() == NeedsFullRepaint && hasRepaintLayoutRects())
             repaintUsingContainer(containerForRepaint(), repaintLayoutRects().m_repaintRect);
+        // If the layer we're about to destroy had a position, then the positions of the current children will no longer be correct.
+        auto* parentLayer = layer()->parent();
+        bool layerHadLocation = !layer()->location().isZero();
         layer()->removeOnlyThisLayer(); // calls destroyLayer() which clears m_layer
+        if (layerHadLocation && parentLayer && !parentLayer->renderer().needsLayout())
+            parentLayer->updateLayerPositionsAfterStyleChange();
         if (s_wasFloating && isFloating())
             setChildNeedsLayout();