Bug 237554 – Unify 'transform-box' reference box computation

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Nikolas Zimmermann]

Currently 'transform-box' reference box computation is sprinkled over multiple places: CSSComputedStyleDeclaration, SVGRenderSupport, RenderBox, parts in RenderLayer.
Before adding any new features, the code should be cleaned up -- this is a preparation to add SVG/CSS transform support within LBSE.

[Nikolas Zimmermann]

Created attachment 454027 [details]
Patch, v1

[Nikolas Zimmermann]

237553 needs to land, before EWS can process the patch.

[Nikolas Zimmermann]

Created attachment 454033 [details]
Patch, v1

[Nikolas Zimmermann]

Created attachment 454216 [details]
Patch, v2

[Nikolas Zimmermann]

Created attachment 454342 [details]
Patch, v3

[Nikolas Zimmermann]

Created attachment 454354 [details]
Patch, v4

[Nikolas Zimmermann]

Created attachment 454405 [details]
Patch, v5

[Nikolas Zimmermann]

After discussing once more with Rob, the conclusion is that the LayoutRect -> FloatRect roundtrips should be avoided - a possible idea is a covariant return type, a la FloatOrLayoutRect, that knows what it is and helps us to encapsulate a method that returns a FloatRect for SVG and a LayoutRect for CSS, without relying on potential error-prone explicit FloatRect -> LayoutRect conversions.

[Nikolas Zimmermann]

Created attachment 454560 [details]
Patch, v6

The latest iteration of the patch introduces FloatOrLayoutRect, which either holds a FloatRect or LayoutRect (realized via variant<FloatRect, LayoutRect>) -- this avoids all LayoutRect(fromFloatRect) hacks, present in previous iterations of the patch and guarantees type-safety again. As benefit it allows us to detect if we need to pixel snap or not (SVG stores FloatRects, no pixel snapping should occour), simplifying quite a few parts in RenderLayer

[Rob Buis]

Comment on attachment 454560 [details]
Patch, v6

View in context: https://bugs.webkit.org/attachment.cgi?id=454560&action=review

> Source/WebCore/ChangeLog:78
> [phone]  Nikolas Zimmermann  <[e-mail]>

Double entry.

> Source/WebCore/platform/graphics/FloatOrLayoutRect.h:40
> +        : m_data { rect }

are move semantics possible?

> Source/WebCore/platform/graphics/FloatOrLayoutRect.h:59
> +    FloatRect rectForPainting(float deviceScaleFactor) const

Probably better floatRectForPainting to make it explicit.

[Nikolas Zimmermann]

(In reply to Rob Buis from comment #10)
> Comment on attachment 454560 [details]
> Patch, v6
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=454560&action=review
> 
> > Source/WebCore/ChangeLog:78
> > [phone]  Nikolas Zimmermann  <[e-mail]>
> 
> Double entry.
Fixed.

> > Source/WebCore/platform/graphics/FloatOrLayoutRect.h:40
> > +        : m_data { rect }
> 
> are move semantics possible?
I experimented a bit further, and adapted FloatOrLayoutRect to construct the variant in-place from FloatRect/LayoutRect rvalue references. This should have the lowest possible overhead, when using variants.

I'll add a new "Patch, v7" which tidies up FloatOrLayoutRect even further: constexpr usage, generic private is()/as() helpers to remove boilerplate. This is an important aspect of LBSE in future, so it's fine to take the time and get it right :-)

> 
> > Source/WebCore/platform/graphics/FloatOrLayoutRect.h:59
> > +    FloatRect rectForPainting(float deviceScaleFactor) const
> 
> Probably better floatRectForPainting to make it explicit.
Fair enough, will change.

[Nikolas Zimmermann]

Created attachment 454582 [details]
Patch, v7

[Nikolas Zimmermann]

Created attachment 454584 [details]
Patch, v7

[Rob Buis]

Comment on attachment 454584 [details]
Patch, v7

View in context: https://bugs.webkit.org/attachment.cgi?id=454584&action=review

> Source/WebCore/platform/graphics/FloatOrLayoutRect.h:36
> +class FloatOrLayoutRect final {

Is there a particular benefit to not moving this to the private section at the bottom?

> Source/WebCore/platform/graphics/FloatOrLayoutRect.h:47
> +    constexpr T& as() { return std::get<T>(m_data); }

I like these intuitive method names :)

> Source/WebCore/platform/graphics/FloatOrLayoutRect.h:86
> +    void move(const LayoutSize& size)

I was actually triggered to think about move semantics because of this method name. Maybe it is possible to rename slightly to distinguish from std::move? moveLocation? movePosition?

> Source/WebCore/platform/graphics/FloatOrLayoutRect.h:106
> +static_assert(std::is_trivially_copyable<FloatOrLayoutRect>::value, "'FloatOrLayoutRect' must be trivially coypable.");

Tyop.

[Nikolas Zimmermann]

(In reply to Rob Buis from comment #14)
> Comment on attachment 454584 [details]
> Patch, v7
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=454584&action=review
> 
> > Source/WebCore/platform/graphics/FloatOrLayoutRect.h:36
> > +class FloatOrLayoutRect final {
> 
> Is there a particular benefit to not moving this to the private section at
> the bottom?
The constexpr inline as()/is() helpers are otherwise undefined in the first public method that uses them…
> 
> > Source/WebCore/platform/graphics/FloatOrLayoutRect.h:47
> > +    constexpr T& as() { return std::get<T>(m_data); }
> 
> I like these intuitive method names :)
:-)

> 
> > Source/WebCore/platform/graphics/FloatOrLayoutRect.h:86
> > +    void move(const LayoutSize& size)
> 
> I was actually triggered to think about move semantics because of this
> method name. Maybe it is possible to rename slightly to distinguish from
> std::move? moveLocation? movePosition?
This needs to keep the same FloatRect/LayoutRect naming scheme, otherwise I have to adapt call-sites.

> > Source/WebCore/platform/graphics/FloatOrLayoutRect.h:106
> > +static_assert(std::is_trivially_copyable<FloatOrLayoutRect>::value, "'FloatOrLayoutRect' must be trivially coypable.");
> 
> Tyop.
Will fxi.

[Radar WebKit Bug Importer]

<rdar://problem/90248560>

[Rob Buis]

The patch now looks great IMHO. But I think Simon should have a look as well.

[Simon Fraser (smfr)]

Comment on attachment 454584 [details]
Patch, v7

View in context: https://bugs.webkit.org/attachment.cgi?id=454584&action=review

> Source/WebCore/rendering/RenderBox.h:171
> +    FloatOrLayoutRect referenceBoxRect(CSSBoxType) const final;

Rather than the unwieldy FloatOrLayoutRect, can we just templatize this function?

[zalan]

Comment on attachment 454584 [details]
Patch, v7

View in context: https://bugs.webkit.org/attachment.cgi?id=454584&action=review

> Source/WebCore/ChangeLog:20
> +        For SVGs needs referenceBoxRect() needs to return a FloatRect, whereas for CSS/HTML LayoutRect
> +        is the right choice. Introduce a 'FloatOrLayoutRect' helper that can be used as return value/argument

Could you elaborate on the "SVGs needs referenceBoxRect() needs to return a FloatRect" part? Does SVG need higher precision than what LayoutRect can provide?

[Nikolas Zimmermann]

(In reply to Simon Fraser (smfr) from comment #18)
> Comment on attachment 454584 [details]
> Patch, v7
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=454584&action=review
> 
> > Source/WebCore/rendering/RenderBox.h:171
> > +    FloatOrLayoutRect referenceBoxRect(CSSBoxType) const final;
> 
> Rather than the unwieldy FloatOrLayoutRect, can we just templatize this
> function?

If we do that, we cannot have one virtual function and therefore have no unique way to access the transform reference box rect from the render tree, e.g. from RenderLayer. One would need to cast to RenderBox, to get the CSS LayoutRect reference box (and to RenderSVGModelObject to get the SVG FloatRect reference box in LBSE) -- that's what I want to avoid.

[Nikolas Zimmermann]

(In reply to zalan from comment #19)
> Could you elaborate on the "SVGs needs referenceBoxRect() needs to return a
> FloatRect" part? Does SVG need higher precision than what LayoutRect can
> provide?
Sure, SVG needs arbitrary floating-point precision: you can define a SVG fragment that is infinitesimal small, only occupying e.g. 0.00001 x 0.00001px in user-coordinates. I've seen 'normalized' documents in the wild that have all content defined as fraction of 1px -- that's not an edge case, but common. Sure, there are potential issues when painting off device-pixels, but that's the price SVG pays for it's flexible unconstrained user coordinate system approach.

The LBSE downstream branch has some new test cases covering sub-LayoutUnit precision alignment for SVG renderers, here's an example reftest, illustrating the issue:

diff --git a/LayoutTests/svg/transforms/layout-tiny-elements-in-scaled-group-expected.svg b/LayoutTests/svg/transforms/layout-tiny-elements-in-scaled-group-expected.svg
new file mode 100644
index 000000000000..3b2839839ad8
--- /dev/null
+++ b/LayoutTests/svg/transforms/layout-tiny-elements-in-scaled-group-expected.svg
@@ -0,0 +1,8 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1 Tiny//EN" "http://www.w3.org/Graphics/SVG/1.1/DTD/svg11-tiny.dtd">
+<svg xmlns="http://www.w3.org/2000/svg" width="100%" height="100%" viewBox="0 0 48 36">
+<rect x="0" y="0" width="15" height="15" fill="red" />
+<rect x="20" y="0" width="15" height="15" fill="green" />
+<rect x="0" y="20" width="15" height="15" fill="yellow" />
+<rect x="20" y="20" width="15" height="15" fill="blue" />
+</svg>

diff --git a/LayoutTests/svg/transforms/layout-tiny-elements-in-scaled-group.svg b/LayoutTests/svg/transforms/layout-tiny-elements-in-scaled-group.svg
new file mode 100644
index 000000000000..d50ea8236696
--- /dev/null
+++ b/LayoutTests/svg/transforms/layout-tiny-elements-in-scaled-group.svg
@@ -0,0 +1,10 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1 Tiny//EN" "http://www.w3.org/Graphics/SVG/1.1/DTD/svg11-tiny.dtd">
+<svg xmlns="http://www.w3.org/2000/svg" width="100%" height="100%" viewBox="0 0 48 36">
+<g transform="scale(10000, 10000)">
+<rect x="0" y="0" width="0.0015" height="0.0015" fill="red" />
+<rect x="0.002" y="0" width="0.0015" height="0.0015" fill="green" />
+<rect x="0" y="0.002" width="0.0015" height="0.0015" fill="yellow" />
+<rect x="0.002" y="0.002" width="0.0015" height="0.0015" fill="blue" />
+</g>
+</svg>

[zalan]

(In reply to Nikolas Zimmermann from comment #21)
> (In reply to zalan from comment #19)
> > Could you elaborate on the "SVGs needs referenceBoxRect() needs to return a
> > FloatRect" part? Does SVG need higher precision than what LayoutRect can
> > provide?
> Sure, SVG needs arbitrary floating-point precision: you can define a SVG
> fragment that is infinitesimal small, only occupying e.g. 0.00001 x
> 0.00001px in user-coordinates. I've seen 'normalized' documents in the wild
> that have all content defined as fraction of 1px -- that's not an edge case,
> but common. Sure, there are potential issues when painting off
> device-pixels, but that's the price SVG pays for it's flexible unconstrained
> user coordinate system approach.
ok, thanks for the explanation. In that case, I'd rather just have a FloatRect referenceBox(). This API is not used extensively so probably won't be too confusing (and with LFC this may not even be a thing anymore).

[Nikolas Zimmermann]

(In reply to zalan from comment #22)
> ok, thanks for the explanation. In that case, I'd rather just have a
> FloatRect referenceBox(). This API is not used extensively so probably won't
> be too confusing (and with LFC this may not even be a thing anymore).

Patch v6 took that approach - however you need to know at which times you need to convert the FloatRect returned by transformReferenceBox() into a LayoutRect, to apply pixel-snapping, to get a device-pixel aligned FloatRect again. This was quite confusing -- one needs to know the type of the renderer (e.g. RenderBox actually returns a LayoutRect, converted to a FloatRect, and e.g. RenderSVGModelObject does return a FloatRect, and won't need pixel snapping).
...

I do think that the latest iteration does have some advantages, especially as we can hide the whole pixel snapping into the 'floatRectForPainting' method, and only conditionally apply it for non-SVG renderers.

Maybe you can have another look at Patch v6 and see if you agree with. my argumentation. Thanks for your time Zalan & Simon & Rob.

[Nikolas Zimmermann]

(In reply to zalan from comment #22)
> FloatRect referenceBox(). This API is not used extensively so probably won't
> be too confusing (and with LFC this may not even be a thing anymore).

Forgot to ask: what will differ with LFC regarding referenceBox() & co?

[zalan]

(In reply to Nikolas Zimmermann from comment #24)
> (In reply to zalan from comment #22)
> > FloatRect referenceBox(). This API is not used extensively so probably won't
> > be too confusing (and with LFC this may not even be a thing anymore).
> 
> Forgot to ask: what will differ with LFC regarding referenceBox() & co?
Haven't really thought about it yet, since referenceBox is not a layout concept, but (for this exact reason) it certainly won't be a public function on a Box class like this. Layout::Boxes do not have any geometry related APIs (since they don't know anything about position/size) and Display::Box is a lower level concept. Probably on a helper class used by painting/hittest etc? -and maybe we'll discover that FloatRect is the right type for all the use cases.

[Nikolas Zimmermann]

(In reply to zalan from comment #25)
> (In reply to Nikolas Zimmermann from comment #24)
> > (In reply to zalan from comment #22)
> > > FloatRect referenceBox(). This API is not used extensively so probably won't
> > > be too confusing (and with LFC this may not even be a thing anymore).
> > 
> > Forgot to ask: what will differ with LFC regarding referenceBox() & co?
> Haven't really thought about it yet, since referenceBox is not a layout
> concept, but (for this exact reason) it certainly won't be a public function
> on a Box class like this. Layout::Boxes do not have any geometry related
> APIs (since they don't know anything about position/size) and Display::Box
> is a lower level concept.
Ok.

> Probably on a helper class used by
> painting/hittest etc? -and maybe we'll discover that FloatRect is the right
> type for all the use cases.

Hmmm, that confuses me -- do you have plans to move away from LayoutUnit & co. for CSS geometry?

If not, I don't understand the comment: The CSS "border box" for an element, returned by RenderBox borderBoxRect() uses LayoutRect, as the whole geometry is based on fixed-point arithmetic. Therefore, if I need to pick a "reference box" (== borderBoxRect() if "transform-box: border-box" or contentBoxRect() if "transform-box: content-box", etc.), I have no choice but to represent it as LayoutRect.

When painting CSS content, we decide to align the painting to device-pixels (taking device scale factors for HiDPI into account), which avoids unwanted anti-aliasing effects, blurry borders, etc.

Therefore we need to take the LayoutRects, and apply various operations, such
as snapRectToDevicePixels(someLayoutRect, deviceScaleFactor) to extract a FloatRect that can be passed down to the graphics stack, that we prepared in such a way that painting happens on device-pixel boundaries, resulting at most in off-by-one pixel errors, but sharp, non-blurry rendering.

All this logic is not wanted for SVG painting, as we don't generally know a sensible 'bin size' to quantize the coordinate system axes (such as 1/64th pixel for CSS), as SVG allows arbitrary user-coordinate systems (e.g. (0,0) - (1e-6, 1e-4))...

For me the distinction between FloatRect/LayoutRect is therefore natural: CSS related geometry information will in general use LayoutRect, and SVG FloatRect.

Sure, you can return FloatRect(someLayoutRect) everywhere, to assure everything is encoded in FloatRects, but then you also need to keep track whether the FloatRect you have, came from a SVG or CSS source (to decide upon painting, if you need to snap or not, among other things).

Please enlighten me :-)

[zalan]

>Hmmm, that confuses me -- do you have plans to move away from LayoutUnit & co. for CSS geometry?
It depends on what you mean by CSS geometry. LayoutUnit is going to be reserved for CSS layout operations only (stays within the layout formatting context boundaries) and a LayoutUnit -> float conversion happens when producing the final display boxes (you can see it on trunk at InlineDisplayLineBuilder/InlineDisplayContentBuilder). So if you mean CSS geometry within layout, then the answer is no, we are not moving away from LayoutUnit. However if by CSS geometry you mean whatever painting/hittesting and friends are going to be based on, then the answer is yes. Now since reference box applies to transformable elements, which is a painting concept, it will operate on the display box side of things. It's going to return a float based value (since it won't even have access to LayoutUnit based geometries) and whether this value is aligned with some final output (e.g. physical pixel size) or not (probably not) I have not thought about yet.

[Nikolas Zimmermann]

Created attachment 455224 [details]
Patch, v8

[Rob Buis]

Comment on attachment 455224 [details]
Patch, v8

View in context: https://bugs.webkit.org/attachment.cgi?id=455224&action=review

> Source/WebCore/ChangeLog:13
> +        'referenceBoxFloatRect()' methods  implementing the computation for both HTML/CSS and SVG.

Double spacing.

> Source/WebCore/rendering/RenderBox.cpp:788
> +    return std::nullopt;

I am not sure why this returns optional but also does not think returning null opt should happen?

[Nikolas Zimmermann]

(In reply to zalan from comment #27)
> >Hmmm, that confuses me -- do you have plans to move away from LayoutUnit & co. for CSS geometry?
> It depends on what you mean by CSS geometry. LayoutUnit is going to be
> reserved for CSS layout operations only (stays within the layout formatting
> context boundaries) and a LayoutUnit -> float conversion happens when
> producing the final display boxes (you can see it on trunk at
> InlineDisplayLineBuilder/InlineDisplayContentBuilder). So if you mean CSS
> geometry within layout, then the answer is no, we are not moving away from
> LayoutUnit. However if by CSS geometry you mean whatever painting/hittesting
> and friends are going to be based on, then the answer is yes. Now since
> reference box applies to transformable elements, which is a painting
> concept, it will operate on the display box side of things. It's going to
> return a float based value (since it won't even have access to LayoutUnit
> based geometries) and whether this value is aligned with some final output
> (e.g. physical pixel size) or not (probably not) I have not thought about
> yet.


Thanks for the explanation - I'll keep that in mind for future patches. Keeping Layout* an implementation detail makes sense, and it's good to see that LFC improves this.

Coming back to this patch:
Given your comments, the slack discussions and Simons preference towards two dedicated methods to query the 'reference box': one for HTML/CSS returning LayoutRect (as it is right now), and another one for SVG returning a 'FloatRect' I have updated the patch once again (see v8).

However just adding a new method to RenderSVGModelObject leads to clumsy code like the example below in LBSE, since for SVG we not only need a different return type (FloatRect vs. LayoutRect) but also disable pixel snapping within the SVG subtree.

Places like RenderLayer::setupClipPath() that need access to both the device-pixel aligned FloatRect and the unsnapped LayoutRect then need to contain explicit checks for the type of the renderer and logic to handle them:

FloatRect referenceBoxRect;
FloatRect referenceBoxRectForPainting;
if (is<RenderBox>(someRenderer)) {
    LayoutRect referenceBox = downcast<RenderBox>(someRenderer).referenceBoxRect();
    referenceBoxRect = referenceBox;
    referenceBoxRectForPainting = snapRectToDevicePixels(referenceBox, someRenderer.document().deviceScaleFactor());
} else if (is<RenderSVGModelObject>(someRenderer)) {
    referenceBoxRect = downcast<RenderSVGModelObject>(someRenderer).referenceBoxRect();
    referenceBoxRectForPainting = referenceBoxRect;
}

This is quite a burden on the call site, and easy to miss certain renderers in all the different places, that currently contain such checks.
To clean this up, I added the following methods to RenderElement to compute the 'reference box' (CSS Transformations Module Level 1):

virtual std::optional<LayoutRect> referenceBoxLayoutRect(CSSBoxType) const { return std::nullopt; }
virtual std::optional<FloatRect> referenceBoxFloatRect(CSSBoxType) const;

Plus helpers to query the 'transform reference box', based on these:

    std::optional<FloatRect> transformReferenceBoxFloatRect(const RenderStyle& style) const { return referenceBoxFloatRect(transformBoxToCSSBoxType(style.transformBox())); }
    std::optional<FloatRect> transformReferenceBoxFloatRect() const { return transformReferenceBoxFloatRect(style()); }
...


RenderBox provides a referenceBoxLayoutRect() implementation (== the current RenderBox::referenceBox() method), and always returns std::nullopt for referenceBoxFloatRect().
RenderSVGModelObject keeps the default referenceBoxLayoutRect() implementation to return std::nullopt, whereas referenceBoxFloatRect() contains the reference box computation code currently present in SVGRenderSupport::transformReferenceBox() (added in this patch, but only used in LBSE -- omitted here).

RenderLayer needs access to the 'transform reference box' to resolve e.g. percentage values in the 'transform-origin' property and needs to pixel-snap these LayoutRects for painting, respecting the device scale factor. Currently it handles explicit RenderBox type checks in dozens of places that can be avoided.

Using the methods above one can easily define a method that fulfills the aformentioned requirements (no pixel snapping for SVG, no explicit type checking, etc.):

FloatRect RenderLayer::transformReferenceBoxRectForPainting(const RenderStyle& style) const
{
    if (auto referenceBoxRect = renderer().transformReferenceBoxFloatRect(style))
        return referenceBoxRect.value();
    if (auto referenceBoxRect = renderer().transformReferenceBoxLayoutRect(style))
        return snapRectToDevicePixels(referenceBoxRect.value(), renderer().document().deviceScaleFactor());
    return { };
}

That looks more readable to me and makes other call sites more readable and less branchy:

     if (hasTransform) {
         m_transform->makeIdentity();
        renderer().applyTransform(*m_transform, transformReferenceBoxRectForPainting(renderer().style()));
         makeMatrixRenderable(*m_transform, canRender3DTransforms());
     }

If you're fine with the std::optional<> approach let me know.

[Nikolas Zimmermann]

(In reply to Rob Buis from comment #29)
> Comment on attachment 455224 [details]

> > Source/WebCore/ChangeLog:13
> > +        'referenceBoxFloatRect()' methods  implementing the computation for both HTML/CSS and SVG.
> 
> Double spacing.
Will fix.

> > Source/WebCore/rendering/RenderBox.cpp:788
> > +    return std::nullopt;
> 
> I am not sure why this returns optional but also does not think returning
> null opt should happen?
Nope it cannot, to documentthat I've added the ASSERT_NOT_REACHED(). If we don't handle a certain type in the switch, the compiler will warn us, there is no 'default' case given.

Why it returns a std::optional<>? See my length comment for why I tried that approach: it simplifies call sites by removing explicit type checks.

[Rob Buis]

Comment on attachment 455224 [details]
Patch, v8

View in context: https://bugs.webkit.org/attachment.cgi?id=455224&action=review

Patch now LGTM, minus the small ChangeLog nit.

>>> Source/WebCore/rendering/RenderBox.cpp:788
>>> +    return std::nullopt;
>> 
>> I am not sure why this returns optional but also does not think returning null opt should happen?
> 
> Nope it cannot, to documentthat I've added the ASSERT_NOT_REACHED(). If we don't handle a certain type in the switch, the compiler will warn us, there is no 'default' case given.
> 
> Why it returns a std::optional<>? See my length comment for why I tried that approach: it simplifies call sites by removing explicit type checks.

Thanks, now it makes sense!

[Nikolas Zimmermann]

Created attachment 456616 [details]
Patch, v9

[Nikolas Zimmermann]

A new iteration, w/o std::optional<>, as discussed on Slack yesterday with smfr.

[Nikolas Zimmermann]

Created attachment 456751 [details]
Patch, v10

[Simon Fraser (smfr)]

Comment on attachment 456751 [details]
Patch, v10

View in context: https://bugs.webkit.org/attachment.cgi?id=456751&action=review

> Source/WebCore/rendering/RenderLayer.h:701
> +    // Returns the 'reference box' used for clipping operations (different rules for inlines, wrt. to boxes).

"for clipping" makes me think of the overflow and clip properties, but I think this is only about clipPath? So maybe referenceBoxRectForClipPath() would be better? Not sure if motion path would also use it.

> Source/WebCore/rendering/RenderLayer.h:927
> +    bool rendererNeedsPixelSnapping() const;
> +    FloatRect snapRectToDevicePixelsIfNeeded(const LayoutRect&) const;
> +    FloatRect snapRectToDevicePixelsIfNeeded(const FloatRect&) const;

I think these should be free functions somewhere, and not functions of RenderLayer.

[Nikolas Zimmermann]

(In reply to Simon Fraser (smfr) from comment #36)
> Comment on attachment 456751 [details]
> Patch, v10
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=456751&action=review
> 
> > Source/WebCore/rendering/RenderLayer.h:701
> > +    // Returns the 'reference box' used for clipping operations (different rules for inlines, wrt. to boxes).
> 
> "for clipping" makes me think of the overflow and clip properties, but I
> think this is only about clipPath? So maybe referenceBoxRectForClipPath()
> would be better?
Correct, that's a better term - will fix.

> Not sure if motion path would also use it.
Motion path is resolved against the boundingBox, passed to RenderStyle::applyTransform() -- which is usually the 'transform reference box', no special handling for inlines needed.

> 
> > Source/WebCore/rendering/RenderLayer.h:927
> > +    bool rendererNeedsPixelSnapping() const;
> > +    FloatRect snapRectToDevicePixelsIfNeeded(const LayoutRect&) const;
> > +    FloatRect snapRectToDevicePixelsIfNeeded(const FloatRect&) const;
> 
> I think these should be free functions somewhere, and not functions of
> RenderLayer.
I checked all call-sites that currently use snapRectToDevicePixels():

There are users of that function in RenderBox/RenderBoxModelObject/RenderElement -- all in HTML/CSS specific code that cannot be invoked through SVG. Therefore it's safe to stay with the snapRectToDevicePixels() method, as it is right now.

The places in RenderLayer that use snapRectToDevicePixels() were patched to use snapRectToDevicePixelsIfNeeded(). There's only one place that's also relevant: RenderObject::addFocusRingQuads, would in principle need to avoid pixel-snapping for SVG -- however that function is broken with transformed descandants anyhow, unaware of SVG, so I'm ignoring this.

Therefore RenderLayerModelObject.h seems to be ok to place the new free-functions. If you'd like to see this moved to another maybe new header file, let me know, and I'll fix that post-landing.

Furthermore one last minute fix: I've changes the pixel snapping condition, to only ever affect something if LBSE is not only compiled, but also activated:

+bool rendererNeedsPixelSnapping(const RenderLayerModelObject& renderer)
+{
+#if ENABLE(LAYER_BASED_SVG_ENGINE)
+    if (renderer.document().settings().layerBasedSVGEngineEnabled() && renderer.isSVGLayerAwareRenderer() && !renderer.isSVGRoot())
+        return false;
+#endif
+    return true;
+}

This was previously missing the SVGEngineEnabled() check.

[Nikolas Zimmermann]

Committed r292525 (249363@trunk): <https://commits.webkit.org/249363@trunk>

[Nikolas Zimmermann]

Landed an additional build fix right after in https://trac.webkit.org/changeset/290882/webkit, to silence an unused parameter warning in non-lbse enabled builds (default setting).

[Nikolas Zimmermann]

There are regressions...

https://build.webkit.org/results/Apple-BigSur-Release-AppleSilicon-WK1-Tests/r292525%20(8691)/results.html

compositing/geometry/scroller-with-clipping-and-foreground-layers.html
imported/w3c/web-platform-tests/css/css-contain/contain-body-overflow-002.html

I will investigate -- will likely take some hours as I'll have a break in 1h for a few hours. If okay, leave it in the tree, and I'll come back tonight with a fix.

[Nikolas Zimmermann]

(In reply to Nikolas Zimmermann from comment #40)
> There are regressions...
> 
> https://build.webkit.org/results/Apple-BigSur-Release-AppleSilicon-WK1-Tests/
> r292525%20(8691)/results.html
> 
> compositing/geometry/scroller-with-clipping-and-foreground-layers.html
> imported/w3c/web-platform-tests/css/css-contain/contain-body-overflow-002.
> html
> 
> I will investigate -- will likely take some hours as I'll have a break in 1h
> for a few hours. If okay, leave it in the tree, and I'll come back tonight
> with a fix.

It's only compositing/geometry/scroller-with-clipping-and-foreground-layers.html.
https://build.webkit.org/results/Apple-BigSur-Release-AppleSilicon-WK1-Tests/r292525%20(8691)/results.html

Cannot reproduce locally at present, but I have additional patches on top, let's see.

[Nikolas Zimmermann]

Later builds apparently had no issue - let's see, if this comes back or if it was a fluke.