[SVG] Fix `<feImage>` rendering when referenced node contains hidden containers [chromium/src : main]
Ragvesh Sharma (Gerrit)
Ragvesh Sharma added 58 comments![]( "Open in Gerrit")
[SVG] Fix feImage rendering when referenced node contains hidden containersRagvesh Sharmanit: <feImage>
Acknowledged
When an feImage references a node (via href) that contains hiddenRagvesh Sharmanit: <feImage>
Acknowledged
When an feImage references a node (via href) that contains hiddenRagvesh Sharmanit: `href`
Acknowledged
The root cause was that SourceToDestinationTransform() usedRagvesh Sharmanit: add function name with quotes
Acknowledged
HasViewportDependence() to decide whether to apply a viewport adjustmentRagvesh Sharmanit: add function name with quotes
Acknowledged
scale. HasViewportDependence() propagates up from all children,Ragvesh Sharmanit: add function name with quotes
Acknowledged
SelfHasRelativeLengths() == true, causing the parent <g> to haveRagvesh Sharmanit: add function name with quotes
Acknowledged
ComputeViewportAdjustmentScale(), replacing the normal filter scale withRagvesh Sharmanit: add function name with quotes
Acknowledged
The fix introduces HasRenderedViewportDependence() which skips hiddenRagvesh Sharmanit: add function name with quotes
Acknowledged
visual geometry of the referenced element.Ragvesh SharmaThe entire problem is not solved in this case.
`ComputeViewportAdjustmentScale()` applies a single uniform scale to the entire referenced subtree. This correctly maps percentage-based lengths (e.g. width="100%") to the filter primitive subregion but distorts sibling elements using absolute units (e.g. width="60"). A proper fix would re-resolve percentage lengths per-element against the filter region rather than scaling the whole paint.
Consider the below case:
```
<g id="referenced">
<rect width="100%" height="100%" fill="lightblue"/> <!-- percentage → needs scaling -->
<rect x="50" y="40" width="60" height="100" fill="black"/> <!-- absolute → gets distorted -->
</g>
```
Agreed — this is the pre-existing limitation tracked by `TODO(crbug/260709)`. Our CL correctly determines when to apply `ComputeViewportAdjustmentScale()` but doesn't change how it scales. Fixing the per-element re-resolution is a larger effort and out of scope for this bug fix
static bool HasRenderedViewportDependence(const LayoutObject& object) {Ragvesh Sharmanit: "Rendered" could be confused with "has a LayoutObject" in Blink terminology. Can we consider `HasVisibleViewportDependence` or `HasPaintedViewportDependence` to more clearly contrast with hidden containers?
Acknowledged
static bool HasRenderedViewportDependence(const LayoutObject& object) {Ragvesh SharmaAny reason why we are making this static?
The function has been removed entirely in the updated patchset. The tree walk was replaced by a `HasVisibleViewportDependence()` bitfield on `LayoutObject`, computed during layout at O(1) cost.
static bool HasRenderedViewportDependence(const LayoutObject& object) {Ragvesh Sharmanit: every other static function uses `layout_object` as parameter probably good to follow the same naming convention.
`HasRenderedViewportDependence()` has been removed. The tree walk is replaced by a `HasVisibleViewportDependence()` bitfield on `LayoutObject`, so there's no static function or parameter anymore.
if (!object.HasViewportDependence() || object.IsSVGHiddenContainer()) {Fredrik SöderquistIf the root layout object is a hidden container (e.g., <g> containing a <rect> plus a hidden gradient), this early return skips checking children entirely even if the children include visible elements with viewport-relative lengths.
Ragvesh SharmaHidden containers will never end up producing any content though, so skipping children seems reasonable?
As fs@ confirmed, hidden containers never produce any visible content — `LayoutSVGHiddenContainer::Paint()` is a final no-op. The early return was correct.
This is no longer applicable in the updated patchset — the tree walk has been replaced by the `HasVisibleViewportDependence()` bitfield, which is computed during layout in `SVGContentContainer::Layout()`. Hidden containers are excluded at propagation time, not at query time.
if (!object.SlowFirstChild()) {
return true;
}
// Check the root element's own relative lengths.
if (auto* element = DynamicTo<SVGElement>(object.GetNode())) {
if (element->SelfHasRelativeLengths()) {
return true;
}
}Ragvesh SharmaI think you could fold this into the loop (perhaps the above as well). Just start at the "root" instead of. This would avoid duplicating the predicate.
Acknowledged. This is no longer applicable in the updated patchset — the tree walk function has been removed entirely. Viewport dependence excluding hidden containers is now computed as a `HasVisibleViewportDependence()` bitfield on `LayoutObject` during layout in `SVGContentContainer::Layout()`, making the check O(1) at read time.
current = current->IsSVGHiddenContainer()Fredrik Söderquistnit: We could also skip subtrees where `HasViewportDependence()` is already false, since this flag propagates upward from children, if a node doesn't have it, no descendant can either:
```
current = (current->IsSVGHiddenContainer() || !current->HasViewportDependence())
? current->NextInPreOrderAfterChildren(&object)
: current->NextInPreOrder(&object)
```This avoids descending into subtrees whose aggregate flag is already false. Not critical for this code path, but makes the intent clearer.
Ragvesh Sharma👍
I suspect that this will read easier (and not repeat itself that much) if written as a `while`-loop.
Both incorporated — but in a different way than suggested. The tree walk has been removed entirely in the updated patchset. Instead, `HasVisibleViewportDependence()` is now a `bitfield` on `LayoutObject`, computed during layout in `SVGContentContainer::Layout()` alongside the existing `HasViewportDependence()`.
This makes the check O(1) at paint time, eliminating the need for any loop, subtree skipping, or while-loop restructuring.
current = current->IsSVGHiddenContainer()
? current->NextInPreOrderAfterChildren(&object)
: current->NextInPreOrder(&object)) {Ragvesh Sharmawe have the check `current->IsSVGHiddenContainer()` at L:103 so why have this ternary operator?
Acknowledged
if (auto* element = DynamicTo<SVGElement>(current->GetNode())) {
if (element->SelfHasRelativeLengths()) {
return true;
}
}Ragvesh SharmaI think it would be better to have this in the LayoutObject so that we can keep it in sync with what the layout code uses (and `SelfHasRelativeLengths()` is being dismantled). Since we're assuming containers here perhaps we could do something like:
```
auto* container = DynamicTo<LayoutSVGContainer>(*current);
// For containers, check the if it has dependencies by itself, else descend
// into it. For non-containers we know that they have viewport dependencies.
if (!container || container->SelfHasViewportDependence()) {
return true;
}
```
?
(where `SelfHasViewportDependence()` is a new function). And the `SlowFirstChild()` check could be eliminated.
Done. `HasVisibleViewportDependence()` is now a bitfield on `LayoutObject`, computed during layout in `SVGContentContainer::Layout()` alongside the existing `HasViewportDependence()`. Hidden containers are excluded at propagation time. The tree walk function, `SelfHasRelativeLengths()` call, and `SlowFirstChild()` check have all been eliminated. Thanks for the suggestion — it led to a much cleaner O(1) solution.
if (HasRenderedViewportDependence(layout_object)) {Ragvesh SharmaRuntime flag??
Divyansh MangalThis is a correctness fix for a rendering bug open since 2013 — it only affects cases where hidden containers (like <radialGradient>) falsely trigger viewport-dependent scaling. Elements without hidden container children are unaffected. Since this is strictly a bug fix with no intentional behavior change, I don't think a runtime flag is needed here. WDYT?
Vinay SinghBut the tree traversal will happen for most cases correct (apart from the early returns)?
I also agree a runtime-feature flag what be good to have here.
Ragvesh SharmaThere is a performance regression angle here because of traversal that we are doing.
The runtime flag is a way to control the blast radius should that pose a problem for any niche use case that we are not considering now.
Acknowledged
if (HasRenderedViewportDependence(layout_object)) {Vinay SinghSince the bit `HasViewportDependence()` contains information of viewport dependency from all children, including hidden containers. Should we instead create a new bit field, lets say `HasViewportDependenceExcludingHiddenContainers`
which excludes hidden containers?what I am suggesting instead of a tree traversal like done here, we can cache the value needed like just what `HasViewportDependence` does. Benefit of that we won't require the tree traversal here. But let me know your thoughts on this we can ask owners as well for thier opinions as well.
Ragvesh Sharma+1
The argument here would be that for an SVG element deeply embeeded into a large SVG will invoke the tree traversal aggresively multiple times for say a JS animated SVG.
We should focus on retaining this information in a quickly accesible single fields like a bitfield.
Acknowledged
Ragvesh SharmaConsider if you can make the test so that the reference does not need to use `<filter>` at all. (if it could just be a green 100x100 that'd be great.)
Done
<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink"Ragvesh SharmaRemove this.
Acknowledged
<filter filterUnits="userSpaceOnUse" primitiveUnits="userSpaceOnUse"Ragvesh SharmaPlease remove this.
Acknowledged
id="flt1" x="0" y="0" width="160" height="180">Ragvesh SharmaNot needed.
Actually, x="0" y="0" are not the default values for the <filter> element. Per the Filter Effects spec (https://www.w3.org/TR/filter-effects-1/) and MDN (
https://developer.mozilla.org/en-US/docs/Web/SVG/Reference/Element/filter), the defaults are x="-10%" and y="-10%" (with width="120%" and height="120%"). This padding exists
so that filter effects like <feGaussianBlur> aren't clipped at the element boundary.
Without explicit x="0" y="0", the filter region shifts to a negative offset (e.g., x=-40 for a 400px viewport), which mispositions the <feImage> output. We need these
attributes to anchor the filter region at the origin so it matches the reference.
<feImage xlink:href="#img1"/>Ragvesh Sharma`href`
Acknowledged
<!-- Reference 1 (no hidden container) -->
<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink"
width="400" height="180">
<filter filterUnits="userSpaceOnUse" primitiveUnits="userSpaceOnUse"
id="flt1" x="0" y="0" width="160" height="180">
<feImage xlink:href="#img1"/>
</filter>
<rect filter="url(#flt1)" width="160" height="180"/>
<g transform="translate(200 0)">
<g id="img1">
<rect width="160" height="180" fill="lightblue"/>
<rect x="50" y="40" width="60" height="100" fill="black"/>
</g>
</g>
</svg>Ragvesh SharmaThis can be updated as (if my above comments are accepted):
```
<!-- Reference 1 (no hidden container) -->
<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" height="180">
<g>
<rect width="160" height="180" fill="lightblue"/>
<rect x="50" y="40" width="60" height="100" fill="black"/>
</g>
</svg>
```
Done
<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink"Ragvesh SharmaRemove the same as above test for all the refs as well.
Acknowledged
<!-- Reference 2 (no hidden container) -->
<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink"
width="400" height="180">
<filter filterUnits="userSpaceOnUse" primitiveUnits="userSpaceOnUse"
id="flt2" x="0" y="0" width="160" height="180">
<feImage xlink:href="#img2"/>
</filter>
<rect filter="url(#flt2)" width="160" height="180"/>
<g transform="translate(200 0)">
<g id="img2">
<rect width="160" height="180" fill="lightblue"/>
<rect x="50" y="40" width="60" height="100" fill="black"/>
</g>
</g>
</svg>Ragvesh Sharmasame as above
Acknowledged
<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink"
width="400" height="180">
<filter filterUnits="userSpaceOnUse" primitiveUnits="userSpaceOnUse"
id="flt3" x="0" y="0" width="160" height="180">
<feImage xlink:href="#img3"/>
</filter>
<rect filter="url(#flt3)" width="160" height="180"/>
<g transform="translate(200 0)">
<g id="img3">
<rect width="160" height="180" fill="lightblue"/>
<rect x="50" y="40" width="60" height="100" fill="black"/>
</g>
</g>
</svg>Ragvesh Sharmasame as above
Acknowledged
<!-- Reference 4 (no hidden container, but has viewport-relative length) -->
<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink"
width="400" height="180">
<filter filterUnits="userSpaceOnUse" primitiveUnits="userSpaceOnUse"
id="flt4" x="0" y="0" width="160" height="180">
<feImage xlink:href="#img4"/>
</filter>
<rect filter="url(#flt4)" width="160" height="180"/>
<g transform="translate(200 0)">
<g id="img4">
<rect width="100%" height="100%" fill="lightblue"/>
<rect x="50" y="40" width="60" height="100" fill="black"/>
</g>
</g>
</svg>Ragvesh Sharmasame as above
Acknowledged
Ragvesh SharmaI guess this would sort under `css/filter-effects/` - not `svg/`?
Done
<!DOCTYPE html>Ragvesh SharmaDo we require test for `pattern`/`mask` elements?
Acknowledged
<link rel="help" href="https://www.w3.org/TR/filter-effects-1/#feImageElement">
<link rel="match" href="feImage-with-hidden-container-ref.html">Ragvesh Sharmaadd an author link like this
`<link rel="author" title="Divyansh Mangal" href="mailto:dma...@microsoft.com">`
Acknowledged
<!-- Test 1: radialGradient -->Ragvesh SharmaCan we add different test cases in different files for simplicity?
Acknowledged
<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink"Ragvesh SharmaRemove this.
Acknowledged
<filter filterUnits="userSpaceOnUse" primitiveUnits="userSpaceOnUse"Ragvesh SharmaThis is not needed as it is the default value.
Done. Removed — the default for `primitiveUnits` is indeed `userSpaceOnUse` per the Filter Effects spec (https://drafts.fxtf.org/filter-effects/#FilterElement) and MDN (
https://developer.mozilla.org/en-US/docs/Web/SVG/Reference/Attribute/primitiveUnits).
id="flt1" x="0" y="0" width="160" height="180">Ragvesh SharmaThis is not adding any value. Remove this.
Actually, x="0" y="0" are not the default values for the <filter> element. Per the Filter Effects spec (https://www.w3.org/TR/filter-effects-1/) and MDN (
https://developer.mozilla.org/en-US/docs/Web/SVG/Reference/Element/filter), the defaults are x="-10%" and y="-10%" (with width="120%" and height="120%"). This padding exists
so that filter effects like <feGaussianBlur> aren't clipped at the element boundary.
Without explicit x="0" y="0", the filter region shifts to a negative offset (e.g., x=-40 for a 400px viewport), which mispositions the <feImage> output. We need these
attributes to anchor the filter region at the origin so it matches the reference.
<feImage xlink:href="#img1"/>Ragvesh SharmaCan we just use `href`?
Acknowledged
<g transform="translate(200 0)">
<g id="img1">
<radialGradient/>
<rect width="160" height="180" fill="lightblue"/>
<rect x="50" y="40" width="60" height="100" fill="black"/>
</g>
</g>Ragvesh SharmaI think we can simply these tests, by just not rendering these rectnagles
For example this can be simplified as:
```
<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" height="180">
<defs>
<g id="img1">
<radialGradient/>
<rect width="160" height="180" fill="lightblue"/>
<rect x="50" y="40" width="60" height="100" fill="black"/>
</g>
</defs>
<filter filterUnits="userSpaceOnUse" primitiveUnits="userSpaceOnUse"
id="flt1" x="0" y="0" width="160" height="180">
<feImage xlink:href="#img1"/>
</filter>
<rect filter="url(#flt1)" width="160" height="180"/>
</svg>
```You can then simplify portion in ref file by simply not having the filter code at all.
Done. Tests now use `<defs>` to wrap the referenced `<g>` and `<filter>`, rendering only via `<rect filter="url(#flt)">`. The reference files have been simplified to plain `<rect>` elements with no `<filter>` code.
<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink"
width="400" height="180">
<filter filterUnits="userSpaceOnUse" primitiveUnits="userSpaceOnUse"
id="flt1" x="0" y="0" width="160" height="180">
<feImage xlink:href="#img1"/>
</filter>
<rect filter="url(#flt1)" width="160" height="180"/>
<g transform="translate(200 0)">
<g id="img1">
<radialGradient/>
<rect width="160" height="180" fill="lightblue"/>
<rect x="50" y="40" width="60" height="100" fill="black"/>
</g>
</g>Fredrik SöderquistA second observation that I observed these behave differently in Firefox, it seems
The rectangle using the filter does not even gets rendered in Firefox, is that a bug on their end? what happens in Safari?
Ragvesh SharmaIIRC, Gecko does not support `feImage` which reference an element.
Acknowledged. As fs@ noted, Gecko does not support `<feImage>` referencing an element at all. Safari/WebKit has partial support for element references, but it's known to be buggy with viewport-dependent gaps and artifacts (WebKit Bug #266295 (https://bugs.webkit.org/show_bug.cgi?id=266295)).
<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink"Ragvesh SharmaSee above.
Acknowledged
<filter filterUnits="userSpaceOnUse" primitiveUnits="userSpaceOnUse"Ragvesh SharmaSee above.
Acknowledged
id="flt2" x="0" y="0" width="160" height="180">Ragvesh SharmaThis is not adding any value. Remove this.
Actually, x="0" y="0" are not the default values for the <filter> element. Per the Filter Effects spec (https://www.w3.org/TR/filter-effects-1/) and MDN (
https://developer.mozilla.org/en-US/docs/Web/SVG/Reference/Element/filter), the defaults are x="-10%" and y="-10%" (with width="120%" and height="120%"). This padding exists
so that filter effects like <feGaussianBlur> aren't clipped at the element boundary.
Without explicit x="0" y="0", the filter region shifts to a negative offset (e.g., x=-40 for a 400px viewport), which mispositions the <feImage> output. We need these
attributes to anchor the filter region at the origin so it matches the reference.
<feImage xlink:href="#img2"/>Ragvesh Sharma`href`
Acknowledged
<!-- Test 2: linearGradient -->
<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink"
width="400" height="180">
<filter filterUnits="userSpaceOnUse" primitiveUnits="userSpaceOnUse"
id="flt2" x="0" y="0" width="160" height="180">
<feImage xlink:href="#img2"/>
</filter>
<rect filter="url(#flt2)" width="160" height="180"/>
<g transform="translate(200 0)">
<g id="img2">
<linearGradient/>
<rect width="160" height="180" fill="lightblue"/>
<rect x="50" y="40" width="60" height="100" fill="black"/>
</g>
</g>
</svg>Ragvesh Sharmaplease see above
Done
<!-- Test 3: filter -->Ragvesh SharmaConsider adding a test for <pattern> i.e another hidden container with percentage-based defaults.
Acknowledged
<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink"Ragvesh SharmaRemove this.
Acknowledged
<filter filterUnits="userSpaceOnUse" primitiveUnits="userSpaceOnUse"Ragvesh SharmaRemove this.
Acknowledged
id="flt3" x="0" y="0" width="160" height="180">Ragvesh SharmaThis is not adding any value. Remove this.
Actually, x="0" y="0" are not the default values for the <filter> element. Per the Filter Effects spec (https://www.w3.org/TR/filter-effects-1/) and MDN (
https://developer.mozilla.org/en-US/docs/Web/SVG/Reference/Element/filter), the defaults are x="-10%" and y="-10%" (with width="120%" and height="120%"). This padding exists
so that filter effects like <feGaussianBlur> aren't clipped at the element boundary.
Without explicit x="0" y="0", the filter region shifts to a negative offset (e.g., x=-40 for a 400px viewport), which mispositions the <feImage> output. We need these
attributes to anchor the filter region at the origin so it matches the reference.
<feImage xlink:href="#img3"/>Ragvesh Sharma`href`
Acknowledged
<!-- Test 3: filter -->
<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink"
width="400" height="180">
<filter filterUnits="userSpaceOnUse" primitiveUnits="userSpaceOnUse"
id="flt3" x="0" y="0" width="160" height="180">
<feImage xlink:href="#img3"/>
</filter>
<rect filter="url(#flt3)" width="160" height="180"/>
<g transform="translate(200 0)">
<g id="img3">
<filter/>
<rect width="160" height="180" fill="lightblue"/>
<rect x="50" y="40" width="60" height="100" fill="black"/>
</g>
</g>
</svg>Ragvesh Sharmaplease see above
Done
<!-- Test 4: hidden container + visible element with genuine viewport-relative length -->Ragvesh SharmaCan we add a negative case where SVG viewport size is different from filter primitive subregion?
Done. The `feImage-with-hidden-container-viewport-dependent.html` test covers this — SVG viewport is `400×180` while the filter primitive subregion is `160×180`. The referenced `<g>` contains both a `<radialGradient/>` (hidden container) and a `<rect width="100%">` (visible, viewport-dependent). This verifies that `ComputeViewportAdjustmentScale()` is correctly triggered by the visible element's percentage lengths while the hidden container's viewport dependence is excluded.
<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink"Ragvesh SharmaRemove this.
Acknowledged
<filter filterUnits="userSpaceOnUse" primitiveUnits="userSpaceOnUse"Ragvesh SharmaRemove this.
Acknowledged
id="flt4" x="0" y="0" width="160" height="180">Ragvesh SharmaThis is not adding any value. Remove this.
Actually, x="0" y="0" are not the default values for the <filter> element. Per the Filter Effects spec (https://www.w3.org/TR/filter-effects-1/) and MDN (
https://developer.mozilla.org/en-US/docs/Web/SVG/Reference/Element/filter), the defaults are x="-10%" and y="-10%" (with width="120%" and height="120%"). This padding exists
so that filter effects like <feGaussianBlur> aren't clipped at the element boundary.
Without explicit x="0" y="0", the filter region shifts to a negative offset (e.g., x=-40 for a 400px viewport), which mispositions the <feImage> output. We need these
attributes to anchor the filter region at the origin so it matches the reference.
<feImage xlink:href="#img4"/>Ragvesh Sharma`href`
Acknowledged
<radialGradient/>Ragvesh SharmaShould we also test a case `radialGradient` is defined in <defs>, a very common use to consider?
Acknowledged
<!-- Test 4: hidden container + visible element with genuine viewport-relative length -->
<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink"
width="400" height="180">
<filter filterUnits="userSpaceOnUse" primitiveUnits="userSpaceOnUse"
id="flt4" x="0" y="0" width="160" height="180">
<feImage xlink:href="#img4"/>
</filter>
<rect filter="url(#flt4)" width="160" height="180"/>
<g transform="translate(200 0)">
<g id="img4">
<radialGradient/>
<rect width="100%" height="100%" fill="lightblue"/>
<rect x="50" y="40" width="60" height="100" fill="black"/>
</g>
</g>
</svg>Ragvesh Sharmaplease see above
Done
Related details
- Divyansh Mangal
- Fredrik Söderquist
- Philip Rogers
- Vinay Singh
- Virali Purbey
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fredrik Söderquist (Gerrit)
Fredrik Söderquist added 4 comments![]( "Open in Gerrit")
if (HasRenderedViewportDependence(layout_object)) {Ragvesh SharmaRuntime flag??
Divyansh MangalThis is a correctness fix for a rendering bug open since 2013 — it only affects cases where hidden containers (like <radialGradient>) falsely trigger viewport-dependent scaling. Elements without hidden container children are unaffected. Since this is strictly a bug fix with no intentional behavior change, I don't think a runtime flag is needed here. WDYT?
Vinay SinghBut the tree traversal will happen for most cases correct (apart from the early returns)?
I also agree a runtime-feature flag what be good to have here.
Ragvesh SharmaThere is a performance regression angle here because of traversal that we are doing.
The runtime flag is a way to control the blast radius should that pose a problem for any niche use case that we are not considering now.
Acknowledged
I think this thread missed to weigh using up a bit on _all_ LayoutObjects to fix this fairly limited case. Having an additional bit also means more bookkeeping. And now there's also an additional virtual call in the main container layout function - which is _much_ more commonly called than the `<feImage>` code-path. Is this a good trade-off based on known quantities?
I'd still suggest making the test simpler. I'.e there should be no need to have multiple `<rect>`s if they both are of the same type. And if using multiple ones they could still be arranged such that the reference doesn't need to have multiple ones (or even any SVG at all - it could just be a div). As mentioned previously, I'd recommend just having all tests produce a 100x100 green square and either no text or text to match a known reference.
Is this test actually correct? I think it just codifies our currently (incorrect/buggy) behavior, no? In that case it shouldn't be added to WPT. At least not with an incorrect reference.
Related details
- Divyansh Mangal
- Philip Rogers
- Ragvesh Sharma
- Vinay Singh
- Virali Purbey
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ragvesh Sharma (Gerrit)
Ragvesh Sharma voted and added 4 comments![]( "Open in Gerrit")
Votes added by Ragvesh Sharma
| Auto-Submit | +1 |
| Commit-Queue | +2 |
4 comments
if (HasRenderedViewportDependence(layout_object)) {Ragvesh SharmaRuntime flag??
Divyansh MangalThis is a correctness fix for a rendering bug open since 2013 — it only affects cases where hidden containers (like <radialGradient>) falsely trigger viewport-dependent scaling. Elements without hidden container children are unaffected. Since this is strictly a bug fix with no intentional behavior change, I don't think a runtime flag is needed here. WDYT?
Vinay SinghBut the tree traversal will happen for most cases correct (apart from the early returns)?
I also agree a runtime-feature flag what be good to have here.
Ragvesh SharmaThere is a performance regression angle here because of traversal that we are doing.
The runtime flag is a way to control the blast radius should that pose a problem for any niche use case that we are not considering now.
Fredrik SöderquistAcknowledged
I think this thread missed to weigh using up a bit on _all_ LayoutObjects to fix this fairly limited case. Having an additional bit also means more bookkeeping. And now there's also an additional virtual call in the main container layout function - which is _much_ more commonly called than the `<feImage>` code-path. Is this a good trade-off based on known quantities?
Agreed — using a bit on all LayoutObjects for this limited case isn't a good trade-off. The updated patchset removes the bitfield and all the layout bookkeeping. The check is now localized to `svg_fe_image.cc` — a walk of immediate children only when `HasViewportDependence()` is true, which keeps the fix contained to the `<feImage>` code-path with zero impact on the general layout path.
I'd still suggest making the test simpler. I'.e there should be no need to have multiple `<rect>`s if they both are of the same type. And if using multiple ones they could still be arranged such that the reference doesn't need to have multiple ones (or even any SVG at all - it could just be a div). As mentioned previously, I'd recommend just having all tests produce a 100x100 green square and either no text or text to match a known reference.
Acknowledged
Is this test actually correct? I think it just codifies our currently (incorrect/buggy) behavior, no? In that case it shouldn't be added to WPT. At least not with an incorrect reference.
- Divyansh Mangal
- Fredrik Söderquist
- Philip Rogers
- Vinay Singh
- Virali Purbey
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fredrik Söderquist (Gerrit)
Fredrik Söderquist added 2 comments![]( "Open in Gerrit")
// Non-container leaf or a container with self-viewport-dependence.But this we technically don't know? (Except if the container is empty.) I think this may need the the additional function that was brought up previously.
Related details
- Divyansh Mangal
- Philip Rogers
- Ragvesh Sharma
- Vinay Singh
- Virali Purbey
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ragvesh Sharma (Gerrit)
Ragvesh Sharma voted and added 1 comment![]( "Open in Gerrit")
Votes added by Ragvesh Sharma
| Auto-Submit | +1 |
1 comment
// Non-container leaf or a container with self-viewport-dependence.But this we technically don't know? (Except if the container is empty.) I think this may need the the additional function that was brought up previously.
Good catch. Updated — the `!SlowFirstChild()` has been replaced with an explicit `SelfHasRelativeLengths()` check for every visible, viewport-dependent node. This reads directly from the element's DOM attributes, so it can't be stale from previously-removed children.
Related details
- Divyansh Mangal
- Fredrik Söderquist
- Philip Rogers
- Vinay Singh
- Virali Purbey
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fredrik Söderquist (Gerrit)
Fredrik Söderquist added 1 comment![]( "Open in Gerrit")
if (auto* element = DynamicTo<SVGElement>(current->GetNode())) {
if (element->SelfHasRelativeLengths()) {
return true;
}
}This should really go in the `LayoutObject`, because it has a more complete picture of the dependencies. This function only covers dependencies from attributes. (And as mentioned previously, it's slated to go away.)
Related details
- Divyansh Mangal
- Philip Rogers
- Ragvesh Sharma
- Vinay Singh
- Virali Purbey
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ragvesh Sharma (Gerrit)
Ragvesh Sharma voted and added 1 comment![]( "Open in Gerrit")
Votes added by Ragvesh Sharma
| Auto-Submit | +1 |
1 comment
if (auto* element = DynamicTo<SVGElement>(current->GetNode())) {
if (element->SelfHasRelativeLengths()) {
return true;
}
}This should really go in the `LayoutObject`, because it has a more complete picture of the dependencies. This function only covers dependencies from attributes. (And as mentioned previously, it's slated to go away.)
- Divyansh Mangal
- Fredrik Söderquist
- Philip Rogers
- Vinay Singh
- Virali Purbey
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Philip Rogers (Gerrit)
Philip Rogers added 1 comment![]( "Open in Gerrit")
if (!current->SlowFirstChild()) {Does this loop have a bug for text and foreign object? These types have children but are not containers. For example:
```
<!DOCTYPE html>
<title>feImage should be affected by text with viewport dependence</title>
<link rel="help" href="https://www.w3.org/TR/filter-effects-1/#feImageElement">
<link rel="match" href="reference/green-200x200.html">
<svg xmlns="http://www.w3.org/2000/svg" width="200" height="200" style="background: red">
<defs>
<filter filterUnits="userSpaceOnUse" id="flt" x="0" y="0" width="200" height="200">
<feImage href="#img"/>
</filter>
</defs>
<svg width="100" height="100" overflow="visible">
<g id="img">
<rect width="100" height="100" fill="green"/>
<text x="50%" fill="transparent">a</text>
</g>
</svg>
<rect filter="url(#flt)" width="200" height="200"/>
</svg>
```
and
```
<!DOCTYPE html>
<title>feImage should be affected by foreignObject with viewport dependence</title>
<link rel="help" href="https://www.w3.org/TR/filter-effects-1/#feImageElement">
<link rel="match" href="reference/green-200x200.html">
<svg xmlns="http://www.w3.org/2000/svg" width="200" height="200" style="background: red">
<defs>
<filter filterUnits="userSpaceOnUse" id="flt" x="0" y="0" width="200" height="200">
<feImage href="#img"/>
</filter>
</defs>
<svg width="100" height="100" overflow="visible">
<g id="img">
<rect width="100" height="100" fill="green"/>
<foreignObject width="100%" height="100%">
<div xmlns="http://www.w3.org/1999/xhtml">a</div>
</foreignObject>
</g>
</svg>
<rect filter="url(#flt)" width="200" height="200"/>
</svg>
```
With reference:
```
<!DOCTYPE html>
<div style="width: 200px; height: 200px; background-color: green"></div>
```
Related details
- Divyansh Mangal
- Fredrik Söderquist
- Ragvesh Sharma
- Vinay Singh
- Virali Purbey
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |