Render programmatic text decoration#507
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0e7b65471a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (element.CustomAttributes.TryGetValue(SvgStyleAttributeNames.RawTextDecorationAttributeKey, out var rawValue)) | ||
| { | ||
| return TryParseTextDecorationValue(rawValue, out decorations) && | ||
| HasRenderableDecorations(decorations); |
There was a problem hiding this comment.
When a document was loaded from SVG markup that already had a text-decoration attribute, SvgElementFactory leaves the raw value in CustomAttributes, and the TextDecoration property setter only updates Attributes. Because this branch returns as soon as the raw value exists, a later programmatic assignment is ignored for those parsed elements; for example loading text-decoration="none" and then setting TextDecoration = SvgTextDecoration.LineThrough still renders no decoration, while setting None over an originally underlined element still uses the old raw underline.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in f584514ce2f35838f8ad1b6e2ed0d7519d9b98d8.
The fix keeps the raw text-decoration cache authoritative for freshly parsed SVG, so invalid authored values such as comma-separated decoration lists are still rejected. The stale-cache problem is handled at the local attribute collection layer instead: when an existing text-decoration attribute is set again, the cached __svgskia:text-decoration-raw custom attribute is removed before the change notification. That matches the programmatic SvgText.TextDecoration setter path without changing external submodules.
I added regression tests for both cases called out here:
- parsed
text-decoration="none"followed byTextDecoration = SvgTextDecoration.LineThrough - parsed
text-decoration="line-through"followed byTextDecoration = SvgTextDecoration.None
I also reran the comma-separated resvg text-decoration row that caught the raw parsing edge case.
Validation:
dotnet format Svg.Skia.slnx --no-restore --include src tests samplesdotnet test tests/Svg.Skia.UnitTests/Svg.Skia.UnitTests.csproj -f net10.0 -c Release --no-restore --filter "FullyQualifiedName~SKSvgTests.Save_ProgrammaticTextDecorationLineThrough_RendersDecoration|FullyQualifiedName~SKSvgTests.Save_ParsedTextDecoration|FullyQualifiedName~resvgTests.a_text_decoration"dotnet build Svg.Skia.slnx -c Releasedotnet test Svg.Skia.slnx -c Release
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f584514ce2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (TryUnboxedCheck(oldVal, value)) | ||
| { | ||
| base[attributeName] = value; | ||
| RemoveStaleRawTextDecoration(attributeName); |
There was a problem hiding this comment.
Fresh evidence: the new cleanup runs only in the existing-attribute branch, so parsed elements whose authored text-decoration was kept only as raw text still keep RawTextDecorationAttributeKey after a programmatic assignment. For example, if the converter rejects a value such as text-decoration="underline solid red", Attributes has no text-decoration; setting TextDecoration = SvgTextDecoration.LineThrough takes the insert path, leaves the raw value, and TryGetOwnTextDecoration still returns from that stale raw value before looking at the enum, so the programmatic decoration is ignored. Clear the raw key whenever text-decoration is assigned, including the insert path.
Useful? React with 👍 / 👎.
1 participant
PR Summary
Fixes #278.
Summary
This change makes programmatically assigned
SvgText.TextDecorationvalues render in the Skia scene graph text pipeline. Previously, parsed SVG markup with a rawtext-decorationattribute could flow through the renderer, but documents created in code could setTextDecoration = SvgTextDecoration.LineThrough, serialize correctly astext-decoration="line-through", and still render without the decoration in PNG output.The renderer now checks the normal
text-decorationattribute value when the parser-specific raw text-decoration custom attribute is absent. This preserves the existing raw SVG parsing behavior for authored SVG values while allowing enum-backed programmatic values such asLineThroughto participate in the same decoration drawing pipeline.Implementation Details
text-decorationattribute constant inSvgSceneTextCompiler.TryGetAttribute("text-decoration", ...).line-through.LineThrough.Test Coverage
Added an end-to-end PNG rendering regression test:
SvgDocumentin code.SvgTextDecoration.None.SvgTextDecoration.LineThrough.SKSvg.FromSvgDocument(...).Save(...).Validation
Ran the focused affected test sets on
net10.0:Results:
SKSvgTests: 8 passed, 0 failed.SvgSceneTextCompilerTests: 19 passed, 0 failed.Notes
The full solution build with
--no-restorewas not usable in this fresh worktree before committing because manyproject.assets.jsonfiles were missing, and the local environment also lacks thenet461reference assemblies required by the full solution target graph. The focusednet10.0tests above were restored and then rerun with--no-restore.