Support CSS hex colors with alpha#510
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0a01d5e0e2
ℹ️ 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".
| var converted = _colourConverter.ConvertFrom(colorValue); | ||
| return converted is Color color ? new SvgColourServer(color) : SvgPaintServer.NotSet; |
There was a problem hiding this comment.
Keep invalid paint values from overriding inheritance
When _colourConverter.ConvertFrom(colorValue) returns SvgPaintServer.NotSet (for example fill="123" or malformed hex), this branch now returns SvgPaintServer.NotSet instead of throwing. In this codebase, SetValue catches conversion exceptions and leaves the attribute unset, which lets invalid declarations be ignored; returning NotSet here instead stores an explicit paint value that resolves as black and overrides inherited fill/stroke. This changes rendering semantics for invalid CSS/SVG paint tokens and is a regression from the previous behavior.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in af561f94d39672783c785f1ef195878414e702f5 (Address PR paint parsing feedback).
The paint factory now keeps the upstream invalid-paint behavior after the new CSS hex-alpha handling. If SvgColourConverter returns SvgPaintServer.NotSet for malformed paint such as fill="123" or fill="#12345", SvgPaintServerFactory.Create throws instead of storing an explicit NotSet paint server. The generated SetValue path catches that conversion failure and leaves the attribute unset, so invalid declarations no longer override inherited fill/stroke.
I added regression coverage in SvgDocumentCompatibilityLoaderTests.FromSvg_InvalidPaintValuesDoNotOverrideInheritedFill for both numeric and malformed-hex paint tokens inheriting fill="lime" from a parent.
There was a problem hiding this comment.
Follow-up pushed in 7370923de after checking current Chrome behavior in headless Chrome.
Chrome keeps the authored invalid text in the DOM attribute, but the computed paint ignores it: fill="123" and fill="#12345" inherit from the parent; with no inherited fill, the initial computed fill is black. For CSS declarations, Chrome also discards the invalid paint declaration before it can win the cascade, so style="fill: lime; fill: 123", #target { fill: lime } #target { fill: 123 }, and rect { fill: lime } #target { fill: 123 } all compute to lime.
7370923de follows that by adding SvgCssPaintDeclarationValidator and invoking it from both inline style and stylesheet declaration ingestion before AddStyle. Direct invalid paint values like 123 and malformed hex are skipped, so earlier or lower-specificity valid paint remains staged. I intentionally leave var(...) values unvalidated at this stage because CSS custom properties resolve later at computed-value time, where falling back to inheritance/initial rather than a lower cascade declaration is the browser behavior.
Added regression coverage for invalid presentation attributes plus inline and stylesheet cascade cases using the Chrome-observed values.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c6748c7fb2
ℹ️ 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".
| // CSS style declarations can legally feed #RGBA/#RRGGBBAA into this factory. | ||
| if (TryParseHexColorWithAlpha(colorValue, out var hexColor)) | ||
| { | ||
| return new SvgColourServer(hexColor); |
There was a problem hiding this comment.
Preserve alpha when interpolating parsed hex colors
When a SMIL animation uses #RGBA/#RRGGBBAA for fill or stroke values, this branch now lets the animation code treat those values as interpolatable colors. The interpolation path later serializes with new SvgColourServer(color).ToString() in SvgAnimationController.TryInterpolateColor, but SvgColourServer.ToString() formats ToArgb().Substring(2), dropping alpha; for example, animating #0000 to #0000 writes #000000 and renders opaque black instead of transparent. Please serialize interpolated colors through the paint converter or another alpha-preserving format when A != 255.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in af561f94d39672783c785f1ef195878414e702f5 (Address PR paint parsing feedback).
SvgAnimationController now serializes synthesized animation colors through a new FormatColor helper. Opaque colors keep the existing SvgColourServer.ToString() output, while colors with alpha are emitted as #RRGGBBAA, which the paint converter can parse back without losing alpha. I replaced the interpolation, additive, and accumulation color paths that previously used new SvgColourServer(color).ToString() and therefore dropped alpha.
I added SvgAnimationControllerTests.CreateAnimatedDocument_PreservesAlphaWhenInterpolatingHexAlphaPaint, which animates fill from #00000000 to #00000080 and asserts the midpoint alpha is 64, not opaque black.
PR Summary: Support CSS Hex Colors With Alpha
Issue
Fixes GitHub issue #309, where Svg.Skia did not support CSS hex color
notations that include an alpha channel. The reported case was that the
library accepted traditional SVG/CSS hex colors such as
#AABBCC, but notalpha forms such as
#AABBCC80.Summary
This change adds support for CSS Color 4 alpha hex color forms in the custom
paint server factory:
#RGBA#RRGGBBAAThe implementation intentionally leaves existing opaque SVG 1.1 hex color
forms (
#RGBand#RRGGBB) on the existingSvgColourConverterpath. Thiskeeps current behavior for supported opaque colors while adding only the
missing alpha-capable formats.
Implementation Details
SvgPaintServerFactory.Createnow checks for CSS alpha hex values beforefalling back to the upstream color converter. When a valid alpha hex value is
found, it creates an
SvgColourServerwith the parsedSystem.Drawing.Color.The parser supports:
#abc8, expanded to red0xAA, green0xBB, blue0xCC, alpha0x88#AABBCC80, parsed as red0xAA, green0xBB, blue0xCC, alpha0x80Invalid alpha hex values fall through to the existing converter behavior.
SvgPaintServerFactory.ConvertTonow serializes non-opaqueSvgColourServervalues as
#RRGGBBAA, preserving alpha when a paint server is converted backto a string.
Tests
Added compatibility loader coverage for both CSS style declarations and direct
SVG attributes:
#AABBCC80#abc8#AABBCC80#abc8Added render coverage through
SKSvg.Saveto verify that an alpha hex fill ispreserved in the generated PNG output.
Validation
Commands run:
Results:
git diff --check: passedSvgAnimationControllerTests.CreateAnimatedDocument_SaturatesLargeRepeatIterationCountsCommits
Fix CSS alpha hex paint parsingAdd alpha hex color regression tests