Repository navigation
Conversation
Identical to the helper in fix/svg-transform-order so the branches merge cleanly.
SvgGlyphSource parsed the root <svg> element's transform and applied it
more than once: it seeded Walk with it, so it was baked into every
PaintedLayer.Transform, and it was also stored in PaintedCanvasMetadata,
which PaintedGlyphMetrics.ComputeSourceToUpem applied again. When the
document had no glyph{gid} element, Walk started at the root itself and
parsed the same transform a third time as the node's own transform.
SVG 2 applies a transform on an <svg> element as if it were set on a
parent element, so it acts in the parent space outside the viewBox
mapping. Only the canvas metadata can place it there, so the root
transform now lives only on the canvas: Walk starts from identity, and
the no-glyph-id fallback walks the root's children with the root's
resolved paint and opacity instead of walking the root element itself.
ComputeSourceToUpem now applies the root transform after the viewBox
mapping and before the Y-flip, rather than before the viewBox mapping.
COLR sources always pass an identity root transform, so they are
unaffected.
Add SvgTestFonts.CreateFontFromDocument for tests that need full control
of the document, and SvgRootTransformTests covering a root transform
with a glyph{gid} group, without a glyph id, and combined with a viewBox.
A gradient that references another through href/xlink:href inherits an attribute, or its stops, from the referenced gradient only when it does not specify it itself (SVG 1.1 13.2.2/13.2.3 xlink:href; SVG 2 paint server templates). BuildLinearGradient and BuildRadialGradient got this wrong in three ways: - gradientTransform: every transform in the chain was multiplied together instead of the nearest specified one being used alone. - gradientUnits and spreadMethod: reassigned on every step, so the referenced (outermost) gradient overrode the referencing one. - Stops: the walk stopped at the first element with stops, so a gradient with its own stops never inherited any attribute from the gradient it references. Track units, spread and transform as nullable and take the first value found walking outward, keep collecting stops only until some are found, and keep walking the chain for the remaining attributes. An attribute counts as specified when present; an unrecognized value resolves to its initial value (objectBoundingBox, pad, identity), as the existing parsers already do, and still hides the referenced gradient's value. Coordinate attributes already resolved first-defined-wins and are unchanged.
Follow-up to the href inheritance fix, which keeps walking the chain past the element that supplies the stops: - A template must be a linearGradient or radialGradient element (SVG 1.1 13.2.2/13.2.3 xlink:href; SVG 2 gradient href). A reference to any other element is now ignored and ends the chain, so a gradient no longer takes x1/y1/x2/y2 from a <line> or cx/cy/r from a <circle>. - Geometry attributes are read only from elements of the gradient's own type. A linear template supplies a radial gradient only the common attributes (gradientUnits, spreadMethod, gradientTransform) and the stops, and vice versa. - Stops are inherited only when the gradient has no stop children, not when none of its stop children parse. - The visited set is seeded with the starting gradient's id, so a chain that leads back to it ends there instead of processing it twice.
Moving the root <svg> transform onto the canvas left PaintedGlyph.Bounds without it, and GetDesignBounds applied it by transforming that cached axis-aligned box. A rotation or skew in the root transform therefore produced the box around the rotated box, inflating ink bounds used for measurement, line metrics and the box passed to renderers. GetDesignBounds keeps the cheap cached-box path while the source-to-UPEM mapping keeps the axes aligned (scale, translation, Y-flip), where it is exact, and otherwise measures the geometry under the full mapping through the new PaintedGlyph.GetBounds. This keeps the root transform in a single place, after the viewBox mapping, without the layers needing to know unitsPerEm and independently of how Walk composes nested transforms. Replace the no-glyph-id special case in TryGetPaintedGlyph, which duplicated Walk's container handling, with one rule in Walk: when the node is the document root, its children start from the parent transform rather than from one that includes the root's own transform, since that transform is applied once on the canvas. Add a test that a rotate(45) root transform gives the same measured and renderer bounds as the equivalent pre-rotated geometry.
The geometry pass for a rotating or skewing root transform ran on every GetDesignBounds call; the glyph and canvas are fixed per instance, so cache the result. Also treat transforms that swap the axes, and zero terms carrying trigonometric rounding noise, as axis-aligned so rotate(90/180/270) keep transforming the cached box.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #573 +/- ##
======================================
Coverage 80% 81%
======================================
Files 408 408
Lines 35802 35831 +29
Branches 5542 5552 +10
======================================
+ Hits 28885 29095 +210
+ Misses 5488 5316 -172
+ Partials 1429 1420 -9
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Drop two branches that could never run: a gradient is only built through the id it was referenced by, so pass that id in rather than re-reading an attribute that is always present, and detect the document root as the element without a parent rather than through a nullable Document. Add tests for the paths the patch left untested: quarter-turn and skewed root transforms, cubic curves and arcs under a rotated root transform, root fill="none" with a filled child, a radial gradient's own focal radius, and radial defaults following inherited userSpaceOnUse units.
This branch has not been deployed
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.
Prerequisites
Description
Fixes #572.
Root
<svg>transformPaintedCanvasMetadata.RootTransform, andComputeSourceToUpemapplies it in this order: viewBox mapping, then root transform, then Y-flip. Before, the root transform came first. This follows SVG 2 §8.6. The OpenType spec references SVG 1.1, which doesn't allowtransformon<svg>at all.Walk.TryGetPaintedGlyphnow startsWalkfrom identity. InsideWalk, the document root's owntransformis never put into the layers, however the walk reaches the root.GetDesignBoundscan't only transform the cached box when the root transform rotates or skews. The box would grow around the rotated corners. In that case it measures the actual geometry through a newPaintedGlyph.GetBounds(Matrix3x2)and caches the result per instance.Gradient
hrefchainsBuildLinearGradientandBuildRadialGradientnow resolve inherited values first-defined-wins (SVG 1.1 §13.2.2/§13.2.3, SVG 2 templates):gradientTransform,gradientUnits,spreadMethod: the nearest gradient that specifies one wins. Transforms are no longer multiplied along the chain. A present but unrecognised value counts as specified and falls back to the initial value, matching how the existing parsers treat unknown values.<stop>children keeps them. The walk now continues past it to pick up attributes it didn't set itself.x1…y2,cx…fr): taken only from gradients of the same type. A linear template passes on only the shared attributes and stops to a radial gradient, and the other way round.hrefthat points at anything other than a gradient element.Tests
These use
SvgTestFonts, which adds anSVGtable toSixLaborsSampleAB.ttf. Each test compares a document that uses the feature with an equivalent document that doesn't.SvgRootTransformTests(4 tests): the root transform with aglyph{gid}group, without one, combined with aviewBox, and rotated with tight bounds. All 4 fail onmain. Before the fix, the translate was applied 2× with a glyph group and 3× without one.SvgGradientInheritanceTests(29 tests, linear and radial):main.main, becauseTryParseStopnever rejects a stop and the first-wins change makes a repeat visit harmless. They pin the rule.Transforms in the tests are single operations or
matrix(...), so they don't depend on #571, which fixes transform-list order. I applied #571'ssrcdiff on top of this branch: it applies cleanly and all tests still pass.Verification
HarfBuzzCorpusTests(2 tests) andTextShaperTests.ZawgyiScriptOverrideAppliesThroughTextOptionsAndTextRun. Those fail the same way on a cleanmainin my environment.SUPPORTS_DRAWINGturned on locally, the SVGIssues_462snapshot differences are identical tomainto four decimal places. The Noto Color Emoji output doesn't change.Merging with #569 / #571
tests/SixLabors.Fonts.Tests/SvgTestFonts.csis also added by #571. This branch's copy has one extra method,CreateFontFromDocument. Whichever of the two merges second will get an add/add conflict in that file; resolve it by keeping this version.Noticed but not changed
These already exist on
mainand are outside this fix:<use>cycles: a<use>that references one of its own ancestors (including the root<svg>) makesWalkrecurse until the stack overflows.Walk'susecase has no cycle guard.glyph{gid}: transforms and presentation attributes (fill,opacity) on elements between the root and a nestedglyph{gid}element are ignored.IncludeArcboxes the arc and then transforms the box.userSpaceOnUsedefaults:x2defaults to 0 instead of 100%, andcx/cy/rdefault to 0 instead of 50%.ApproximateFloatComparer.Equals: theIEnumerable<Vector2>overload takes both enumerators fromx, so it comparesxwith itself.