Skip to content
Permalink
Browse files
[LBSE] Pixel snapping logic is incorrect for SVG, when elements are c…
…omposited

https://bugs.webkit.org/show_bug.cgi?id=245416

Reviewed by Rob Buis.

In bug webkit.org/b/244966 ("[LBSE] Outermost <svg> elements are not device-pixel aligned")
the rendering was adapted in such a way that the outermost <svg> element is the only element
in the SVG subtree that is pixel snapped, delivering consistent results between e.g. HTML
<div> elements and SVG <svg> elements, embedded in a CSS formatting context. The <div> will
get pixel snapped upon painting -- the same was enforced for the outermost <svg>, however
without propagating/accumulating sub-pixel errors for the descendant layers, since no pixel
snapping is applied within the SVG subtree.

When elements are composited another logic is used (RenderLayerBacking), and the device-pixel
alignment is applied for all SVG layers -- effectively breaking content. By fixing that, the
pixel snapping logic is consistent for all painting modes we have, no matter if the outer
<svg> is composited, or any of its descendants.

Added two new reftests covering LBSE + compositing + sub-pixel locations in different display modes.
Doesn't affect any other existing tests.

* LayoutTests/svg/compositing/inline-svg-non-integer-position-display-block-composited-expected.html: Added.
* LayoutTests/svg/compositing/inline-svg-non-integer-position-display-block-composited.html: Added.
* LayoutTests/svg/compositing/inline-svg-non-integer-position-display-inline-composited-expected.html: Added.
* LayoutTests/svg/compositing/inline-svg-non-integer-position-display-inline-composited.html: Added.
* Source/WebCore/rendering/RenderLayer.cpp:
(WebCore::RenderLayer::convertToLayerCoords const):
* Source/WebCore/rendering/RenderLayerBacking.cpp:
(WebCore::snappedGraphicsLayer):
(WebCore::RenderLayerBacking::computeParentGraphicsLayerRect const):
(WebCore::RenderLayerBacking::updateGeometry):
(WebCore::RenderLayerBacking::adjustOverflowControlsPositionRelativeToAncestor):
(WebCore::RenderLayerBacking::updateMaskingLayerGeometry):
(WebCore::RenderLayerBacking::updateContentsRects):
(WebCore::RenderLayerBacking::updateClippingStackLayerGeometry):
(WebCore::RenderLayerBacking::setContentsNeedDisplayInRect):

Canonical link: https://commits.webkit.org/254863@main
  • Loading branch information
nikolaszimmermann committed Sep 26, 2022
1 parent e28b8cb commit d2c0299eb83a36da49226c4c3696e8ae7382c39b
Show file tree
Hide file tree
Showing 6 changed files with 243 additions and 12 deletions.
@@ -0,0 +1,51 @@
<!DOCTYPE html><!-- webkit-test-runner [ LayerBasedSVGEngineEnabled=true ] -->
<html>
<head>
<style>
.rect1 {
fill: blue;
}

.rect2 {
fill: green;
}

svg {
display: block;
overflow: hidden;
}

.show-overflow {
overflow: visible;
}

.scaled {
transform: scale(2);
transform-origin: 0 0;
}
</style>
</head>
<body>
<h1>Enforce non-integer SVG location</h1>
<svg width="50" height="50" class="show-overflow">
<rect x="0" y="0" width="50" height="50" class="rect1"/>
</svg>
<svg width="50" height="50" class="show-overflow">
<rect x="0" y="0" width="50" height="50" class="rect2"/>
</svg>
<h1>Effect of overflow: hidden</h1>
<svg width="50" height="50">
<rect x="0" y="0" width="50" height="50" class="rect1"/>
</svg>
<svg width="50" height="50">
<rect x="0" y="0" width="50" height="50" class="rect2"/>
</svg>
<h1>With transformation</h1>
<svg width="50" height="50" class="show-overflow">
<rect x="0" y="0" width="50" height="50" class="rect1"/>
</svg>
<svg width="50" height="50" class="show-overflow">
<rect x="0" y="0" width="50" height="50" class="rect2"/>
</svg>
</body>
</html>
@@ -0,0 +1,61 @@
<!DOCTYPE html><!-- webkit-test-runner [ LayerBasedSVGEngineEnabled=true ] -->
<!-- Based on svg/in-html/inline-svg-non-integer-position-display-block.html + compositing enabled either for <svg> or <rect> -->
<html>
<head>
<style>
.rect1 {
fill: blue;
}

.rect2 {
fill: green;
}

svg {
display: block;
overflow: hidden;
}

.move-svg {
top: 25px;
position: relative;
}

.show-overflow {
overflow: visible;
}

.composited {
transform: translateZ(0);
}

.scaled {
transform: scale(2);
transform-origin: 0 0;
}
</style>
</head>
<body>
<h1>Enforce non-integer SVG location</h1>
<svg width="50" height="50" class="show-overflow composited">
<rect x="0" y="0" width="50" height="50" class="rect1"/>
</svg>
<svg width="50" height="50" class="show-overflow">
<rect x="0" y="0" width="50" height="50" class="rect2 composited"/>
</svg>
<h1>Effect of overflow: hidden</h1>
<svg width="50" height="50" class="composited">
<rect x="0" y="0" width="50" height="50" class="rect1"/>
</svg>
<svg width="50" height="50">
<rect x="0" y="0" width="50" height="50" class="rect2 composited"/>
</svg>
<h1>With transformation</h1>
<svg width="25" height="25" class="show-overflow composited scaled">
<rect x="0" y="0" width="25" height="25" class="rect1"/>
</svg>
<svg width="25" height="25" class="show-overflow move-svg scaled">
<rect x="0" y="0" width="25" height="25" class="rect2 composited"/>
</svg>
</body>
</html>
@@ -0,0 +1,51 @@
<!DOCTYPE html><!-- webkit-test-runner [ LayerBasedSVGEngineEnabled=true ] -->
<html>
<head>
<style>
.rect1 {
fill: blue;
}

.rect2 {
fill: green;
}

svg {
display: inline-block;
overflow: hidden;
}

.show-overflow {
overflow: visible;
}

.scaled {
transform: scale(2);
transform-origin: 0 0;
}
</style>
</head>
<body>
<h1>Enforce non-integer SVG location</h1>
<svg width="50" height="50" class="show-overflow">
<rect x="0" y="0" width="50" height="50" class="rect1"/>
</svg>
<svg width="50" height="50" class="show-overflow">
<rect x="0" y="0" width="50" height="50" class="rect2"/>
</svg>
<h1>Effect of overflow: hidden</h1>
<svg width="50" height="50">
<rect x="0" y="0" width="50" height="50" class="rect1"/>
</svg>
<svg width="50" height="50">
<rect x="0" y="0" width="50" height="50" class="rect2"/>
</svg>
<h1>With transformation</h1>
<svg width="50" height="50" class="show-overflow">
<rect x="0" y="0" width="50" height="50" class="rect1"/>
</svg>
<svg width="50" height="50" class="show-overflow">
<rect x="0" y="0" width="50" height="50" class="rect2"/>
</svg>
</body>
</html>
@@ -0,0 +1,61 @@
<!DOCTYPE html><!-- webkit-test-runner [ LayerBasedSVGEngineEnabled=true ] -->
<!-- Based on svg/in-html/inline-svg-non-integer-position-display-inline.html + compositing enabled either for <svg> or <rect> -->
<html>
<head>
<style>
.rect1 {
fill: blue;
}

.rect2 {
fill: green;
}

svg {
display: inline-block;
overflow: hidden;
}

.move-svg {
left: 25px;
position: relative;
}

.show-overflow {
overflow: visible;
}

.composited {
transform: translateZ(0);
}

.scaled {
transform: scale(2);
transform-origin: 0 0;
}
</style>
</head>
<body>
<h1>Enforce non-integer SVG location</h1>
<svg width="50" height="50" class="show-overflow composited">
<rect x="0" y="0" width="50" height="50" class="rect1"/>
</svg>
<svg width="50" height="50" class="show-overflow">
<rect x="0" y="0" width="50" height="50" class="rect2 composited"/>
</svg>
<h1>Effect of overflow: hidden</h1>
<svg width="50" height="50" class="composited">
<rect x="0" y="0" width="50" height="50" class="rect1"/>
</svg>
<svg width="50" height="50">
<rect x="0" y="0" width="50" height="50" class="rect2 composited"/>
</svg>
<h1>With transformation</h1>
<svg width="25" height="25" class="show-overflow composited scaled">
<rect x="0" y="0" width="25" height="25" class="rect1"/>
</svg>
<svg width="25" height="25" class="show-overflow move-svg scaled">
<rect x="0" y="0" width="25" height="25" class="rect2 composited"/>
</svg>
</body>
</html>
@@ -2484,10 +2484,17 @@ LayoutPoint RenderLayer::convertToLayerCoords(const RenderLayer* ancestorLayer,
if (ancestorLayer == this)
return location;

const RenderLayer* currLayer = this;
LayoutPoint locationInLayerCoords = location;
const auto* currLayer = this;
auto locationInLayerCoords = location;
while (currLayer && currLayer != ancestorLayer)
currLayer = accumulateOffsetTowardsAncestor(currLayer, ancestorLayer, locationInLayerCoords, adjustForColumns);

#if ENABLE(LAYER_BASED_SVG_ENGINE)
// Pixel snap the whole SVG subtree as one "block" -- not individual layers down the SVG render tree.
if (renderer().isSVGRoot())
return LayoutPoint(roundPointToDevicePixels(locationInLayerCoords, renderer().document().deviceScaleFactor()));
#endif

return locationInLayerCoords;
}

@@ -1191,11 +1191,11 @@ struct SnappedRectInfo {
LayoutSize m_snapDelta;
};

static SnappedRectInfo snappedGraphicsLayer(const LayoutSize& offset, const LayoutSize& size, float deviceScaleFactor)
static SnappedRectInfo snappedGraphicsLayer(const LayoutSize& offset, const LayoutSize& size, const RenderLayerModelObject& renderer)
{
SnappedRectInfo snappedGraphicsLayer;
LayoutRect graphicsLayerRect = LayoutRect(toLayoutPoint(offset), size);
snappedGraphicsLayer.m_snappedRect = LayoutRect(snapRectToDevicePixels(graphicsLayerRect, deviceScaleFactor));
snappedGraphicsLayer.m_snappedRect = LayoutRect(snapRectToDevicePixelsIfNeeded(graphicsLayerRect, renderer));
snappedGraphicsLayer.m_snapDelta = snappedGraphicsLayer.m_snappedRect.location() - toLayoutPoint(offset);
return snappedGraphicsLayer;
}
@@ -1294,7 +1294,7 @@ LayoutRect RenderLayerBacking::computeParentGraphicsLayerRect(const RenderLayer*
// If the compositing ancestor has a layer to clip children, we parent in that, and therefore position relative to it.
LayoutRect clippingBox = clippingLayerBox(ancestorRenderBox);
LayoutSize clippingBoxOffset = computeOffsetFromAncestorGraphicsLayer(compositedAncestor, clippingBox.location(), deviceScaleFactor());
parentGraphicsLayerRect = snappedGraphicsLayer(clippingBoxOffset, clippingBox.size(), deviceScaleFactor()).m_snappedRect;
parentGraphicsLayerRect = snappedGraphicsLayer(clippingBoxOffset, clippingBox.size(), renderer()).m_snappedRect;
}

if (compositedAncestor->hasCompositedScrollableOverflow()) {
@@ -1402,7 +1402,7 @@ void RenderLayerBacking::updateGeometry(const RenderLayer* compositedAncestor)
clippingBox = clippingLayerBox(renderer());
// Clipping layer is parented in the primary graphics layer.
LayoutSize clipBoxOffsetFromGraphicsLayer = toLayoutSize(clippingBox.location()) + rendererOffset.fromPrimaryGraphicsLayer();
SnappedRectInfo snappedClippingGraphicsLayer = snappedGraphicsLayer(clipBoxOffsetFromGraphicsLayer, clippingBox.size(), deviceScaleFactor);
SnappedRectInfo snappedClippingGraphicsLayer = snappedGraphicsLayer(clipBoxOffsetFromGraphicsLayer, clippingBox.size(), renderer());
clipLayer->setPosition(snappedClippingGraphicsLayer.m_snappedRect.location());
clipLayer->setSize(snappedClippingGraphicsLayer.m_snappedRect.size());
clipLayer->setOffsetFromRenderer(toLayoutSize(clippingBox.location() - snappedClippingGraphicsLayer.m_snapDelta));
@@ -1472,7 +1472,7 @@ void RenderLayerBacking::updateGeometry(const RenderLayer* compositedAncestor)
if (m_overflowControlsContainer) {
LayoutRect overflowControlsBox = overflowControlsHostLayerRect(downcast<RenderBox>(renderer()));
LayoutSize boxOffsetFromGraphicsLayer = toLayoutSize(overflowControlsBox.location()) + rendererOffset.fromPrimaryGraphicsLayer();
SnappedRectInfo snappedBoxInfo = snappedGraphicsLayer(boxOffsetFromGraphicsLayer, overflowControlsBox.size(), deviceScaleFactor);
SnappedRectInfo snappedBoxInfo = snappedGraphicsLayer(boxOffsetFromGraphicsLayer, overflowControlsBox.size(), renderer());

m_overflowControlsContainer->setPosition(snappedBoxInfo.m_snappedRect.location());
m_overflowControlsContainer->setSize(snappedBoxInfo.m_snappedRect.size());
@@ -1558,7 +1558,7 @@ void RenderLayerBacking::adjustOverflowControlsPositionRelativeToAncestor(const
ComputedOffsets rendererOffset(m_owningLayer, &ancestorLayer, { }, parentGraphicsLayerRect, primaryGraphicsLayerRect);

LayoutSize boxOffsetFromGraphicsLayer = toLayoutSize(overflowControlsRect.location()) + rendererOffset.fromParentGraphicsLayer();
SnappedRectInfo snappedBoxInfo = snappedGraphicsLayer(boxOffsetFromGraphicsLayer, overflowControlsRect.size(), deviceScaleFactor());
SnappedRectInfo snappedBoxInfo = snappedGraphicsLayer(boxOffsetFromGraphicsLayer, overflowControlsRect.size(), renderer());

m_overflowControlsContainer->setPosition(snappedBoxInfo.m_snappedRect.location());
m_overflowControlsContainer->setSize(snappedBoxInfo.m_snappedRect.size());
@@ -1644,7 +1644,7 @@ void RenderLayerBacking::updateMaskingLayerGeometry()

// FIXME: Use correct reference box for inlines: https://bugs.webkit.org/show_bug.cgi?id=129047, https://github.com/w3c/csswg-drafts/issues/6383
LayoutRect boundingBox = m_owningLayer.boundingBox(&m_owningLayer);
LayoutRect referenceBoxForClippedInline = LayoutRect(snapRectToDevicePixels(boundingBox, deviceScaleFactor()));
LayoutRect referenceBoxForClippedInline = LayoutRect(snapRectToDevicePixelsIfNeeded(boundingBox, renderer()));
LayoutSize offset = LayoutSize(snapSizeToDevicePixel(-m_subpixelOffsetFromRenderer, LayoutPoint(), deviceScaleFactor()));
auto [clipPath, windRule] = m_owningLayer.computeClipPath(offset, referenceBoxForClippedInline);

@@ -1734,7 +1734,7 @@ void RenderLayerBacking::updateInternalHierarchy()

void RenderLayerBacking::updateContentsRects()
{
m_graphicsLayer->setContentsRect(snapRectToDevicePixels(contentsBox(), deviceScaleFactor()));
m_graphicsLayer->setContentsRect(snapRectToDevicePixelsIfNeeded(contentsBox(), renderer()));

if (is<RenderReplaced>(renderer())) {
FloatRoundedRect contentsClippingRect = downcast<RenderReplaced>(renderer()).roundedContentBoxRect().pixelSnappedRoundedRectForPainting(deviceScaleFactor());
@@ -2028,7 +2028,7 @@ void RenderLayerBacking::updateClippingStackLayerGeometry(LayerAncestorClippingS
auto roundedClipRect = entry.clipData.clipRect;
auto clipRect = roundedClipRect.rect();
LayoutSize clippingOffset = computeOffsetFromAncestorGraphicsLayer(compositedAncestor, clipRect.location() + offsetFromCompositedAncestor, deviceScaleFactor);
LayoutRect snappedClippingLayerRect = snappedGraphicsLayer(clippingOffset, clipRect.size(), deviceScaleFactor).m_snappedRect;
LayoutRect snappedClippingLayerRect = snappedGraphicsLayer(clippingOffset, clipRect.size(), renderer()).m_snappedRect;

auto clippingLayerPosition = toLayoutPoint(snappedClippingLayerRect.location() - lastClipLayerRect.location());
entry.clippingLayer->setPosition(clippingLayerPosition);
@@ -3260,7 +3260,7 @@ void RenderLayerBacking::setContentsNeedDisplayInRect(const LayoutRect& r, Graph

m_owningLayer.invalidateEventRegion(RenderLayer::EventRegionInvalidationReason::Paint);

FloatRect pixelSnappedRectForPainting = snapRectToDevicePixels(r, deviceScaleFactor());
FloatRect pixelSnappedRectForPainting = snapRectToDevicePixelsIfNeeded(r, renderer());
auto& frameView = renderer().view().frameView();
if (m_isMainFrameRenderViewLayer && frameView.isTrackingRepaints())
frameView.addTrackedRepaintRect(pixelSnappedRectForPainting);

0 comments on commit d2c0299

Please sign in to comment.