Fix intrinsic sizing for SVG elements - #736
Open
nicoburns wants to merge 2 commits into
Open
Conversation
Contributor
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
- Treat viewport-relative units (vw/vh/vmin/vmax) on the root svg width/height as non-intrinsic, like percentages - Track zero/negative root width/height attributes: they are a zero intrinsic dimension and disable rendering of the SVG content, while the usvg tree is still built (with the degenerate attributes blanked out) so CSS-sized boxes can exist - Remove the usvg tree-size fallback from intrinsic dimensions and aspect ratio: without a viewBox, the intrinsic ratio exists only when both width and height are absolute lengths - Resolve SVG object-fit painting via the CSS default sizing algorithm with the content box as the default object size, and honor the actual object-fit property instead of hardcoding contain - Transfer a missing natural dimension through the aspect-ratio property (accounting for box-sizing) in replaced layout
staging-devin-ai-integration
Bot
force-pushed
the
devin/1786973137-svg-intrinsic-sizing
branch
from
August 17, 2026 16:23
fce3b7d to
0b81519
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes CSS intrinsic sizing for SVG per SVG2 "Sizing SVG in CSS" / css-images-3, closing the gaps left by #726/#730/#732:
vw/vh/vmin/vmax) on the rootwidth/heightcontribute no intrinsic dimension (like percentages). They fail to parse assvgtypes::Length, soSvgIntrinsicDimensionsnow simply records them as absent; intrinsic dims are resolved from the declared length via a newresolve_absolutehelper (usvg unit factors) instead of trustingusvg::Tree::size().width/height: a zero intrinsic dimension (matching browsers), and rendering of the SVG content is disabled (SvgImageData::rendering_disabled(), checked in bothdraw_svgand background painting alongsidedegenerate_view_box). usvg refuses to parse non-positive sizes, so the offending attributes are blanked out (via theirroxmltreebyte ranges) before building the tree, keeping the image usable when CSS sizes it.aspect_ratio()now returnsOption<f32>— declared width/height ratio only when both are positive absolute lengths, else theviewBoxratio, elseNone. The old fallback tousvg::Tree::size()ratio (and the layout fallback feeding tree size as intrinsic dims) is gone; layout now feedsIntrinsicSizes { width, height, ratio }straight from the accessors with the standard 300×150 default object size.draw_svgpreviously hardcodedobject-fit: containagainst the usvg tree size. It now resolves the natural object size withconcrete_object_size(default = content box)(so a dimensionless SVG fills its box) and honors the actualobject-fitproperty.aspect-ratioproperty (converting throughbox-sizing), not just the intrinsic ratio:<svg>painting is now viewport-aware: the CSS box is the SVG viewport, so percentage lengths inside the SVG and theviewBox/preserveAspectRatiomapping must resolve against it.SvgImageData::tree_for_viewport(w, h)re-parses the (cached) source with the rootwidth/heightoverridden to the box size, and inline painting renders that tree atscale * effective_zoom(zoomscales the CSS box but not user units). SVG-as-image without aviewBoxuses the same path with the concrete object size; with aviewBoxthe plain tree-size scale is kept.@namespace svgandsvg|svg { overflow: hidden }per the SVG2 UA sheet (the baresvgselector was matching the HTML default namespace, so inline SVG overflow was never clipped).Inline
<svg>width/heightattrs map to CSS width/height as presentational hints instylo.rs;resolved_width/resolved_heightare unused by layout.WPT results (local per-test comparison vs base
06c4a704, WPT_DIR=~/repos/wpt)Net +58 / -1 across these suites. The earlier CI-reported replaced/filter/mask regressions came from the first pushed revision (inline SVG was sized via presentational hints but still painted with the old tree-size mapping); the viewport-aware painting above fixes all of them.
The single remaining regression is
svg/render/reftests/nested-svg-overflow-clip.html: it passed on main only by accident (usvg sized the tree to its 250px content and blitz scaled it down into the 100px box). usvg flattens nested<svg>without implementing its viewport clipping, so fixing it properly requires upstream usvg support.cargo test --workspace,cargo fmt, andcargo clippy --workspaceare clean; unit tests cover viewport units, zero/negative dimensions, and the no-viewBox no-ratio case.Link to Devin session: https://dioxus.staging.devinenterprise.com/sessions/961449e5b9814de886799ea728946822
Requested by: @nicoburns
WPT results
56 newly passing, 1 newly failing (net +55).
Full diff (57 changed tests)
Generated by the WPT workflow.