Changeset 252935 in webkit

Timestamp:

Nov 28, 2019 10:27:49 PM (4 years ago)

Author:

Simon Fraser

Message:

Element jumps to wrong position after perspective change on ancestor
​https://bugs.webkit.org/show_bug.cgi?id=202505
<rdar://problem/55930710>

Reviewed by Antti Koivisto.
Source/WebCore:

This modifies the fix in r252879 to be better-performing and to avoid a new call site for updateLayerPositions*.

Style can change in a way that creates or destroys RenderLayers, but does not result in a layout; this can happen
with changes of properties like opacity or perspective. When this happens, something needs to trigger a call to
RenderLayer::updateLayerPositions() on the root of the changed subtree. This is best done after the style update,
to avoid multiple updateLayerPositions traversals.

Implement this by storing on RenderView the rootmost changed layer, and having FrameView::styleDidChange()
call updateLayerPositionsAfterStyleChange() if we're after a style change with no pending layout.

Test: compositing/geometry/layer-position-after-removing-perspective.html

• page/FrameView.cpp:

(WebCore::FrameView::styleDidChange):

• page/FrameView.h:
• platform/ScrollView.h:
• rendering/RenderElement.cpp:

(WebCore::RenderElement::didAttachChild):

• rendering/RenderLayer.cpp:

(WebCore::RenderLayer::insertOnlyThisLayer):
(WebCore::RenderLayer::removeOnlyThisLayer):
(WebCore::findCommonAncestor):
(WebCore::RenderLayer::commonAncestorWithLayer const):

• rendering/RenderLayer.h:
• rendering/RenderLayerModelObject.cpp:

(WebCore::RenderLayerModelObject::createLayer):
(WebCore::RenderLayerModelObject::styleDidChange):

• rendering/RenderView.cpp:

(WebCore::RenderView::layerChildrenChangedDuringStyleChange):
(WebCore::RenderView::takeStyleChangeLayerTreeMutationRoot):

• rendering/RenderView.h:

LayoutTests:

• compositing/geometry/layer-position-after-removing-perspective-expected.html: Added.
• compositing/geometry/layer-position-after-removing-perspective.html: Added.
• css3/blending/repaint/blend-mode-isolate-stacking-context-expected.txt: Rebaselined.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/compositing/geometry/layer-position-after-removing-perspective-expected.html (added)
• LayoutTests/compositing/geometry/layer-position-after-removing-perspective.html (added)
• LayoutTests/css3/blending/repaint/blend-mode-isolate-stacking-context-expected.txt (modified) (2 diffs)
• LayoutTests/platform/ios/css3/blending/repaint/blend-mode-isolate-stacking-context-expected.txt (modified) (2 diffs)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/page/FrameView.cpp (modified) (1 diff)
• Source/WebCore/page/FrameView.h (modified) (1 diff)
• Source/WebCore/platform/ScrollView.h (modified) (1 diff)
• Source/WebCore/rendering/RenderElement.cpp (modified) (1 diff)
• Source/WebCore/rendering/RenderLayer.cpp (modified) (3 diffs)
• Source/WebCore/rendering/RenderLayer.h (modified) (2 diffs)
• Source/WebCore/rendering/RenderLayerModelObject.cpp (modified) (3 diffs)
• Source/WebCore/rendering/RenderView.cpp (modified) (1 diff)
• Source/WebCore/rendering/RenderView.h (modified) (2 diffs)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2019-11-28  Simon Fraser  <simon.fraser@apple.com>
+
+        Element jumps to wrong position after perspective change on ancestor
+        https://bugs.webkit.org/show_bug.cgi?id=202505
+        <rdar://problem/55930710>
+
+        Reviewed by Antti Koivisto.
+
+        * compositing/geometry/layer-position-after-removing-perspective-expected.html: Added.
+        * compositing/geometry/layer-position-after-removing-perspective.html: Added.
+        * css3/blending/repaint/blend-mode-isolate-stacking-context-expected.txt: Rebaselined.
+
 2019-11-28  Jonathan Bedard  <jbedard@apple.com>
 

trunk/LayoutTests/css3/blending/repaint/blend-mode-isolate-stacking-context-expected.txt

@@ -12 +12 @@
 
 (repaint rects
-  (rect 48 54 60 60)
   (rect 48 54 60 60)
   (rect 48 54 60 60)
@@ -30 +29 @@
   (rect 28 290 60 60)
   (rect 48 644 60 60)
+  (rect 48 54 60 60)
   (rect 48 290 60 60)
   (rect 28 526 60 60)

trunk/LayoutTests/platform/ios/css3/blending/repaint/blend-mode-isolate-stacking-context-expected.txt

@@ -12 +12 @@
 
 (repaint rects
-  (rect 48 56 60 60)
   (rect 48 56 60 60)
   (rect 48 56 60 60)
@@ -30 +29 @@
   (rect 28 296 60 60)
   (rect 48 656 60 60)
+  (rect 48 56 60 60)
   (rect 48 296 60 60)
   (rect 28 536 60 60)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2019-11-28  Simon Fraser  <simon.fraser@apple.com>
+
+        Element jumps to wrong position after perspective change on ancestor
+        https://bugs.webkit.org/show_bug.cgi?id=202505
+        <rdar://problem/55930710>
+
+        Reviewed by Antti Koivisto.
+        
+        This modifies the fix in r252879 to be better-performing and to avoid a new call site for updateLayerPositions*.
+
+        Style can change in a way that creates or destroys RenderLayers, but does not result in a layout; this can happen
+        with changes of properties like opacity or perspective. When this happens, something needs to trigger a call to
+        RenderLayer::updateLayerPositions() on the root of the changed subtree. This is best done after the style update,
+        to avoid multiple updateLayerPositions traversals.
+
+        Implement this by storing on RenderView the rootmost changed layer, and having FrameView::styleDidChange()
+        call updateLayerPositionsAfterStyleChange() if we're after a style change with no pending layout.
+
+        Test: compositing/geometry/layer-position-after-removing-perspective.html
+
+        * page/FrameView.cpp:
+        (WebCore::FrameView::styleDidChange):
+        * page/FrameView.h:
+        * platform/ScrollView.h:
+        * rendering/RenderElement.cpp:
+        (WebCore::RenderElement::didAttachChild):
+        * rendering/RenderLayer.cpp:
+        (WebCore::RenderLayer::insertOnlyThisLayer):
+        (WebCore::RenderLayer::removeOnlyThisLayer):
+        (WebCore::findCommonAncestor):
+        (WebCore::RenderLayer::commonAncestorWithLayer const):
+        * rendering/RenderLayer.h:
+        * rendering/RenderLayerModelObject.cpp:
+        (WebCore::RenderLayerModelObject::createLayer):
+        (WebCore::RenderLayerModelObject::styleDidChange):
+        * rendering/RenderView.cpp:
+        (WebCore::RenderView::layerChildrenChangedDuringStyleChange):
+        (WebCore::RenderView::takeStyleChangeLayerTreeMutationRoot):
+        * rendering/RenderView.h:
+
 2019-11-28  Antti Koivisto  <antti@apple.com>
 

trunk/Source/WebCore/page/FrameView.cpp

@@ -800 +800 @@
 }
 
+void FrameView::styleDidChange()
+{
+    ScrollView::styleDidChange();
+    RenderView* renderView = this->renderView();
+    if (!renderView)
+        return;
+
+    RenderLayer* layerTreeMutationRoot = renderView->takeStyleChangeLayerTreeMutationRoot();
+    if (layerTreeMutationRoot && !needsLayout())
+        layerTreeMutationRoot->updateLayerPositionsAfterStyleChange();
+}
+
 bool FrameView::updateCompositingLayersAfterStyleChange()
 {

trunk/Source/WebCore/page/FrameView.h

@@ -139 +139 @@
 
     void willRecalcStyle();
+    void styleDidChange() override;
     bool updateCompositingLayersAfterStyleChange();
     void updateCompositingLayersAfterLayout();

trunk/Source/WebCore/platform/ScrollView.h

@@ -141 +141 @@
     virtual Ref<Scrollbar> createScrollbar(ScrollbarOrientation);
 
-    void styleDidChange();
+    virtual void styleDidChange();
 
     // If the prohibits scrolling flag is set, then all scrolling in the view (even programmatic scrolling) is turned off.

trunk/Source/WebCore/rendering/RenderElement.cpp

@@ -471 +471 @@
     // and stop creating layers at all for these cases - they're not used anyways.
     if (child.hasLayer() && !layerCreationAllowedForSubtree())
-        downcast<RenderLayerModelObject>(child).layer()->removeOnlyThisLayer();
+        downcast<RenderLayerModelObject>(child).layer()->removeOnlyThisLayer(RenderLayer::LayerChangeTiming::RenderTreeConstruction);
 }
 

trunk/Source/WebCore/rendering/RenderLayer.cpp

@@ -497 +497 @@
 }
 
-void RenderLayer::insertOnlyThisLayer()
+void RenderLayer::insertOnlyThisLayer(LayerChangeTiming timing)
 {
     if (!m_parent && renderer().parent()) {
@@ -512 +512 @@
         child.moveLayers(m_parent, this);
 
+    if (parent()) {
+        if (timing == LayerChangeTiming::StyleChange)
+            renderer().view().layerChildrenChangedDuringStyleChange(*parent());
+    }
+    
     // Clear out all the clip rects.
     clearClipRectsIncludingDescendants();
 }
 
-void RenderLayer::removeOnlyThisLayer()
+void RenderLayer::removeOnlyThisLayer(LayerChangeTiming timing)
 {
     if (!m_parent)
         return;
+
+    if (timing == LayerChangeTiming::StyleChange)
+        renderer().view().layerChildrenChangedDuringStyleChange(*parent());
 
     // Mark that we are about to lose our layer. This makes render tree
@@ -2249 +2257 @@
     }
     return false;
+}
+
+static RenderLayer* findCommonAncestor(const RenderLayer& firstLayer, const RenderLayer& secondLayer)
+{
+    if (&firstLayer == &secondLayer)
+        return const_cast<RenderLayer*>(&firstLayer);
+
+    HashSet<const RenderLayer*> ancestorChain;
+    for (auto* currLayer = &firstLayer; currLayer; currLayer = currLayer->parent())
+        ancestorChain.add(currLayer);
+
+    for (auto* currLayer = &secondLayer; currLayer; currLayer = currLayer->parent()) {
+        if (ancestorChain.contains(currLayer))
+            return const_cast<RenderLayer*>(currLayer);
+    }
+    return nullptr;
+}
+
+RenderLayer* RenderLayer::commonAncestorWithLayer(const RenderLayer& layer) const
+{
+    return findCommonAncestor(*this, layer);
 }
 

trunk/Source/WebCore/rendering/RenderLayer.h

@@ -164 +164 @@
     RenderLayer* lastChild() const { return m_last; }
     bool isDescendantOf(const RenderLayer&) const;
+    RenderLayer* commonAncestorWithLayer(const RenderLayer&) const;
 
     // This does an ancestor tree walk. Avoid it!
@@ -177 +178 @@
     void removeChild(RenderLayer&);
 
-    void insertOnlyThisLayer();
-    void removeOnlyThisLayer();
+    enum class LayerChangeTiming {
+        StyleChange,
+        RenderTreeConstruction,
+    };
+    void insertOnlyThisLayer(LayerChangeTiming);
+    void removeOnlyThisLayer(LayerChangeTiming);
 
     bool isNormalFlowOnly() const { return m_isNormalFlowOnly; }

trunk/Source/WebCore/rendering/RenderLayerModelObject.cpp

@@ -98 +98 @@
     m_layer = makeUnique<RenderLayer>(*this);
     setHasLayer(true);
-    m_layer->insertOnlyThisLayer();
+    m_layer->insertOnlyThisLayer(RenderLayer::LayerChangeTiming::StyleChange);
 }
 
@@ -170 +170 @@
                 setChildNeedsLayout();
             createLayer();
-            if (parent() && !needsLayout() && containingBlock()) {
+            if (parent() && !needsLayout() && containingBlock())
                 layer()->setRepaintStatus(NeedsFullRepaint);
-                layer()->updateLayerPositionsAfterStyleChange();
-            }
         }
     } else if (layer() && layer()->parent()) {
@@ -180 +178 @@
             layer()->willRemoveChildWithBlendMode();
 #endif
-        setHasTransformRelatedProperty(false); // All transform-related propeties force layers, so we know we don't have one or the object doesn't support them.
+        setHasTransformRelatedProperty(false); // All transform-related properties force layers, so we know we don't have one or the object doesn't support them.
         setHasReflection(false);
         // Repaint the about to be destroyed self-painting layer when style change also triggers repaint.
         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();
+
+        layer()->removeOnlyThisLayer(RenderLayer::LayerChangeTiming::StyleChange); // calls destroyLayer() which clears m_layer
         if (s_wasFloating && isFloating())
             setChildNeedsLayout();

trunk/Source/WebCore/rendering/RenderView.cpp

@@ -878 +878 @@
 }
 
+void RenderView::layerChildrenChangedDuringStyleChange(RenderLayer& layer)
+{
+    if (!m_styleChangeLayerMutationRoot) {
+        m_styleChangeLayerMutationRoot = makeWeakPtr(layer);
+        return;
+    }
+
+    RenderLayer* commonAncestor = m_styleChangeLayerMutationRoot->commonAncestorWithLayer(layer);
+    m_styleChangeLayerMutationRoot = makeWeakPtr(commonAncestor);
+}
+
+RenderLayer* RenderView::takeStyleChangeLayerTreeMutationRoot()
+{
+    auto* result = m_styleChangeLayerMutationRoot.get();
+    m_styleChangeLayerMutationRoot.clear();
+    return result;
+}
+
 #if ENABLE(CSS_SCROLL_SNAP)
 void RenderView::registerBoxWithScrollSnapPositions(const RenderBox& box)

trunk/Source/WebCore/rendering/RenderView.h

@@ -185 +185 @@
     void scheduleLazyRepaint(RenderBox&);
     void unscheduleLazyRepaint(RenderBox&);
+    
+    void layerChildrenChangedDuringStyleChange(RenderLayer&);
+    RenderLayer* takeStyleChangeLayerTreeMutationRoot();
 
     void protectRenderWidgetUntilLayoutIsDone(RenderWidget& widget) { m_protectedRenderWidgets.append(&widget); }
@@ -219 +222 @@
     mutable std::unique_ptr<Region> m_accumulatedRepaintRegion;
     SelectionRangeData m_selection;
+
+    WeakPtr<RenderLayer> m_styleChangeLayerMutationRoot;
 
     // FIXME: Only used by embedded WebViews inside AppKit NSViews.  Find a way to remove.