Changeset 268898 in webkit

Timestamp:

Oct 22, 2020 4:15:41 PM (4 years ago)

Author:

Simon Fraser

Message:

Twitter Photo gallery incorrectly rendered after opening another modal
​https://bugs.webkit.org/show_bug.cgi?id=217737
<rdar://problem/70314822>

Reviewed by Zalan Bujtas.
Source/WebCore:

If a layer with a certain configuration of clipping layers had non-zero border-radius, then
changes to zero border-radius, we'd fail to undo the composited rounded-corner masking.
This was because RenderLayerBacking::updateGeometry() didn't call setMasksToBoundsRect()
in the non-rounded corner case.

The compositing code in general tries to separate "configuration" changes (i.e. setting
up the right layer hierarchy) from "geometry" changes (setting positions and sizes).
In this area, those responsibilities were muddied because the return value from
setMasksToBoundsRect() was used to know whether the platform-specific GraphicsLayer
supports rounded-corner clips. This forced RenderLayerBacking::updateChildClippingStrategy()
to do some geometry computation.

Clean that up by adding the static GraphicsLayer::supportsRoundedClip(), and use
that to know if the platform needs a m_childClippingMaskLayer (all non-CA platforms).
Now updateChildClippingStrategy() can be purely about hierarchy, and updateGeometry()
can focus on geometry. In updateGeometry(), we are sure to call setMasksToBoundsRect()
in the non-rounded corner case too.

Test: compositing/style-change/border-radius-change.html

• platform/graphics/GraphicsLayer.cpp:

(WebCore::GraphicsLayer::supportsRoundedClip):
(WebCore::GraphicsLayer::setMasksToBoundsRect):

• platform/graphics/GraphicsLayer.h:

(WebCore::GraphicsLayer::setMasksToBoundsRect): Deleted.

• platform/graphics/ca/GraphicsLayerCA.cpp:

(WebCore::GraphicsLayer::supportsRoundedClip):
(WebCore::GraphicsLayerCA::setMasksToBoundsRect):

• platform/graphics/ca/GraphicsLayerCA.h:
• rendering/RenderLayerBacking.cpp:

(WebCore::RenderLayerBacking::updateGeometry):
(WebCore::RenderLayerBacking::updateChildClippingStrategy):

LayoutTests:

• compositing/style-change/border-radius-change-expected.html: Added.
• compositing/style-change/border-radius-change.html: Added.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/compositing/style-change/border-radius-change-expected.html (added)
• LayoutTests/compositing/style-change/border-radius-change.html (added)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/platform/graphics/GraphicsLayer.cpp (modified) (2 diffs)
• Source/WebCore/platform/graphics/GraphicsLayer.h (modified) (2 diffs)
• Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp (modified) (2 diffs)
• Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h (modified) (1 diff)
• Source/WebCore/rendering/RenderLayerBacking.cpp (modified) (2 diffs)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2020-10-22  Simon Fraser  <simon.fraser@apple.com>
+
+        Twitter Photo gallery incorrectly rendered after opening another modal
+        https://bugs.webkit.org/show_bug.cgi?id=217737
+        <rdar://problem/70314822>
+
+        Reviewed by Zalan Bujtas.
+
+        * compositing/style-change/border-radius-change-expected.html: Added.
+        * compositing/style-change/border-radius-change.html: Added.
+
 2020-10-22  Lauro Moura  <lmoura@igalia.com>
 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2020-10-22  Simon Fraser  <simon.fraser@apple.com>
+
+        Twitter Photo gallery incorrectly rendered after opening another modal
+        https://bugs.webkit.org/show_bug.cgi?id=217737
+        <rdar://problem/70314822>
+
+        Reviewed by Zalan Bujtas.
+        
+        If a layer with a certain configuration of clipping layers had non-zero border-radius, then
+        changes to zero border-radius, we'd fail to undo the composited rounded-corner masking.
+        This was because RenderLayerBacking::updateGeometry() didn't call setMasksToBoundsRect()
+        in the non-rounded corner case.
+        
+        The compositing code in general tries to separate "configuration" changes (i.e. setting
+        up the right layer hierarchy) from "geometry" changes (setting positions and sizes).
+        In this area, those responsibilities were muddied because the return value from 
+        setMasksToBoundsRect() was used to know whether the platform-specific GraphicsLayer
+        supports rounded-corner clips. This forced RenderLayerBacking::updateChildClippingStrategy()
+        to do some geometry computation.
+        
+        Clean that up by adding the static GraphicsLayer::supportsRoundedClip(), and use
+        that to know if the platform needs a m_childClippingMaskLayer (all non-CA platforms).
+        Now updateChildClippingStrategy() can be purely about hierarchy, and updateGeometry()
+        can focus on geometry. In updateGeometry(), we are sure to call setMasksToBoundsRect()
+        in the non-rounded corner case too.
+
+        Test: compositing/style-change/border-radius-change.html
+
+        * platform/graphics/GraphicsLayer.cpp:
+        (WebCore::GraphicsLayer::supportsRoundedClip):
+        (WebCore::GraphicsLayer::setMasksToBoundsRect):
+        * platform/graphics/GraphicsLayer.h:
+        (WebCore::GraphicsLayer::setMasksToBoundsRect): Deleted.
+        * platform/graphics/ca/GraphicsLayerCA.cpp:
+        (WebCore::GraphicsLayer::supportsRoundedClip):
+        (WebCore::GraphicsLayerCA::setMasksToBoundsRect):
+        * platform/graphics/ca/GraphicsLayerCA.h:
+        * rendering/RenderLayerBacking.cpp:
+        (WebCore::RenderLayerBacking::updateGeometry):
+        (WebCore::RenderLayerBacking::updateChildClippingStrategy):
+
 2020-10-22  Chris Dumez  <cdumez@apple.com>
 

trunk/Source/WebCore/platform/graphics/GraphicsLayer.cpp

@@ -89 +89 @@
 }
 
+bool GraphicsLayer::supportsRoundedClip()
+{
+    return false;
+}
+
 bool GraphicsLayer::supportsBackgroundColorContent()
 {
@@ -381 +386 @@
     
     m_maskLayer = WTFMove(layer);
+}
+
+void GraphicsLayer::setMasksToBoundsRect(const FloatRoundedRect& roundedRect)
+{
+    m_masksToBoundsRect = roundedRect;
 }
 

trunk/Source/WebCore/platform/graphics/GraphicsLayer.h

@@ -466 +466 @@
 
     // Set a rounded rect that is used to clip this layer and its descendants (implies setting masksToBounds).
-    // Returns false if the platform can't support this rounded clip, and we should fall back to painting a mask.
+    // Consult supportsRoundedClip() to know whether non-zero radii are supported.
     FloatRoundedRect maskToBoundsRect() const { return m_masksToBoundsRect; };
-    virtual bool setMasksToBoundsRect(const FloatRoundedRect& roundedRect) { m_masksToBoundsRect = roundedRect; return false; }
+    virtual void setMasksToBoundsRect(const FloatRoundedRect&);
 
     Path shapeLayerPath() const;
@@ -608 +608 @@
     void addRepaintRect(const FloatRect&);
 
+    static bool supportsRoundedClip();
     static bool supportsBackgroundColorContent();
     static bool supportsLayerType(Type);

trunk/Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp

@@ -326 +326 @@
 }
 
+bool GraphicsLayer::supportsRoundedClip()
+{
+    return true;
+}
+
 bool GraphicsLayer::supportsSubpixelAntialiasedLayerText()
 {
@@ -972 +977 @@
 }
 
-bool GraphicsLayerCA::setMasksToBoundsRect(const FloatRoundedRect& roundedRect)
+void GraphicsLayerCA::setMasksToBoundsRect(const FloatRoundedRect& roundedRect)
 {
     if (roundedRect == m_masksToBoundsRect)
-        return true;
+        return;
 
     GraphicsLayer::setMasksToBoundsRect(roundedRect);
     noteLayerPropertyChanged(MasksToBoundsRectChanged);
-    return true;
 }
 

trunk/Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h

@@ -123 +123 @@
     WEBCORE_EXPORT void setContentsClippingRect(const FloatRoundedRect&) override;
     WEBCORE_EXPORT void setContentsRectClipsDescendants(bool) override;
-    WEBCORE_EXPORT bool setMasksToBoundsRect(const FloatRoundedRect&) override;
+    WEBCORE_EXPORT void setMasksToBoundsRect(const FloatRoundedRect&) override;
 
     WEBCORE_EXPORT void setShapeLayerPath(const Path&) override;

trunk/Source/WebCore/rendering/RenderLayerBacking.cpp

@@ -1323 +1323 @@
         clipLayer->setOffsetFromRenderer(toLayoutSize(clippingBox.location() - snappedClippingGraphicsLayer.m_snapDelta));
 
-        if ((renderer().style().clipPath() || renderer().style().hasBorderRadius()) && !m_childClippingMaskLayer) {
-            FloatRoundedRect contentsClippingRect = renderBox.roundedBorderBoxRect().pixelSnappedRoundedRectForPainting(deviceScaleFactor);
-            contentsClippingRect.move(LayoutSize(-clipLayer->offsetFromRenderer()));
-            clipLayer->setMasksToBoundsRect(contentsClippingRect);
-        }
+        auto computeMasksToBoundsRect = [&] {
+            if ((renderer().style().clipPath() || renderer().style().hasBorderRadius()) && !m_childClippingMaskLayer) {
+                FloatRoundedRect contentsClippingRect = renderBox.roundedBorderBoxRect().pixelSnappedRoundedRectForPainting(deviceScaleFactor);
+                contentsClippingRect.move(LayoutSize(-clipLayer->offsetFromRenderer()));
+                return contentsClippingRect;
+            }
+
+            return FloatRoundedRect { FloatRect { { }, snappedClippingGraphicsLayer.m_snappedRect.size() } };
+        };
+
+        clipLayer->setMasksToBoundsRect(computeMasksToBoundsRect());
 
         if (m_childClippingMaskLayer && !m_scrollContainerLayer) {
@@ -2211 +2217 @@
 void RenderLayerBacking::updateChildClippingStrategy(bool needsDescendantsClippingLayer)
 {
-    if (hasClippingLayer() && needsDescendantsClippingLayer) {
-        if (is<RenderBox>(renderer()) && (renderer().style().clipPath() || renderer().style().hasBorderRadius())) {
-            auto* clipLayer = clippingLayer();
-            FloatRoundedRect contentsClippingRect = downcast<RenderBox>(renderer()).roundedBorderBoxRect().pixelSnappedRoundedRectForPainting(deviceScaleFactor());
-            contentsClippingRect.move(LayoutSize(-clipLayer->offsetFromRenderer()));
-            // Note that we have to set this rounded rect again during the geometry update (clipLayer->offsetFromRenderer() may be stale here).
-            if (clipLayer->setMasksToBoundsRect(contentsClippingRect)) {
-                clipLayer->setMaskLayer(nullptr);
-                GraphicsLayer::clear(m_childClippingMaskLayer);
-                return;
-            }
-
-            if (!m_childClippingMaskLayer) {
-                m_childClippingMaskLayer = createGraphicsLayer("child clipping mask");
-                m_childClippingMaskLayer->setDrawsContent(true);
-                m_childClippingMaskLayer->setPaintingPhase({ GraphicsLayerPaintingPhase::ChildClippingMask });
-                clippingLayer()->setMaskLayer(m_childClippingMaskLayer.copyRef());
-            }
-        }
-    } else {
-        if (m_childClippingMaskLayer) {
-            if (hasClippingLayer())
-                clippingLayer()->setMaskLayer(nullptr);
-            GraphicsLayer::clear(m_childClippingMaskLayer);
-        } else 
-            if (hasClippingLayer())
-                clippingLayer()->setMasksToBoundsRect(FloatRoundedRect(FloatRect({ }, clippingLayer()->size())));
+    auto needsClipMaskLayer = [&] {
+        return needsDescendantsClippingLayer && !GraphicsLayer::supportsRoundedClip() && is<RenderBox>(renderer()) && (renderer().style().hasBorderRadius() || renderer().style().clipPath());
+    };
+
+    auto* clippingLayer = this->clippingLayer();
+    if (needsClipMaskLayer()) {
+        m_childClippingMaskLayer = createGraphicsLayer("child clipping mask");
+        m_childClippingMaskLayer->setDrawsContent(true);
+        m_childClippingMaskLayer->setPaintingPhase({ GraphicsLayerPaintingPhase::ChildClippingMask });
+        if (clippingLayer)
+            clippingLayer->setMaskLayer(m_childClippingMaskLayer.copyRef());
+    } else if (m_childClippingMaskLayer) {
+        if (clippingLayer)
+            clippingLayer->setMaskLayer(nullptr);
+        GraphicsLayer::clear(m_childClippingMaskLayer);
     }
 }