Preserve @font-face descriptors outside the hardcoded set - #223
Merged
FlorianRappl merged 1 commit intoAug 13, 2026
Merged
Conversation
CssFontFaceRule kept only the seven descriptors named in its private ContainedProperties set; CssDeclarationRule discarded everything else silently and without consulting CssParserOptions, so IsIncludingUnknownDeclarations had no effect inside @font-face even though it is what keeps unrecognized declarations alive in style rules. Standard CSS Fonts Level 4 descriptors (font-display, size-adjust, ascent-override, font-feature-settings, ...) and vendor descriptors (mso-*) were both lost, and ToCss emitted a well-formed looking rule so a caller round-tripping a stylesheet had no way to notice. Non-descriptor declarations now fall through to the same IsAllowingUnknownDeclarations gate that style rules use, which also covers @counter-style, @font-feature-values and @Viewport - the sibling rules on the same base class. Register the standard descriptors that had no declaration at all (size-adjust, ascent-override, descent-override, line-gap-override, font-feature-settings) with real value grammars, add a percentage converter, and extend ContainedProperties so they are kept, typed, by default rather than only under the opt-in. Along the way: - font-variation-settings accepted only `normal`, so `"wght" 400` was rejected in style rules and would have been rejected in the newly preserved @font-face. It now implements normal | [<string> <number>]#. - SetValue added properties without checking they parsed, so an invalid descriptor serialized as a malformed `size-adjust: ;`. Invalid values are now ignored and leave an existing valid declaration standing, matching CssStyleDeclaration. - ICssFontFaceRule.Features was a String.Empty/no-op stub despite its featureSettings DOM name; it now maps to font-feature-settings. Fixes the silent loss reported downstream in mganss/HtmlSanitizer#541.
There was a problem hiding this comment.
Pull request overview
This PR fixes loss of @font-face (and other declaration-rule) descriptors by honoring CssParserOptions.IsIncludingUnknownDeclarations in CssDeclarationRule, and registers several CSS Fonts Level 4 descriptors so they are preserved/typed by default. It also improves declaration assignment behavior by ignoring invalid values (preventing malformed serialization) and wires ICssFontFaceRule.Features to font-feature-settings.
Changes:
- Update
CssDeclarationRuleto preserve non-contained descriptors only when unknown declarations are enabled, and to ignore invalid values instead of storing empty declarations. - Add converters, keywords, property names, initial values, and factory registrations for missing font descriptors (e.g.
size-adjust,*-override,font-feature-settings) and expandfont-variation-settingsparsing. - Add targeted NUnit tests covering preservation, round-tripping, overwrite semantics, and value validation for these descriptors.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/AngleSharp.Css/ValueConverters.cs | Adds percent-only parsing and new converters for font descriptors / variation settings. |
| src/AngleSharp.Css/Factories/DefaultDeclarationFactory.cs | Registers new descriptor declarations so they’re recognized/typed. |
| src/AngleSharp.Css/Dom/Internal/Rules/CssFontFaceRule.cs | Expands the contained descriptor set and implements Features mapping. |
| src/AngleSharp.Css/Dom/Internal/Rules/CssDeclarationRule.cs | Preserves unknown declarations per parser option and ignores invalid values. |
| src/AngleSharp.Css/Declarations/SizeAdjustDeclaration.cs | Adds declaration metadata for size-adjust. |
| src/AngleSharp.Css/Declarations/LineGapOverrideDeclaration.cs | Adds declaration metadata for line-gap-override. |
| src/AngleSharp.Css/Declarations/FontFeatureSettingsDeclaration.cs | Adds declaration metadata for font-feature-settings. |
| src/AngleSharp.Css/Declarations/DescentOverrideDeclaration.cs | Adds declaration metadata for descent-override. |
| src/AngleSharp.Css/Declarations/AscentOverrideDeclaration.cs | Adds declaration metadata for ascent-override. |
| src/AngleSharp.Css/Constants/PropertyNames.cs | Adds constants for new @font-face descriptor names. |
| src/AngleSharp.Css/Constants/InitialValues.cs | Adds initial values for the new descriptors. |
| src/AngleSharp.Css/Constants/CssKeywords.cs | Adds on / off keywords used by font-feature-settings. |
| src/AngleSharp.Css/BrowsingContextExtensions.cs | Makes unknown-declaration option check reusable (internal). |
| src/AngleSharp.Css.Tests/Rules/FontFaceDescriptors.cs | Adds rule-level tests for preservation, serialization, and overwrite semantics. |
| src/AngleSharp.Css.Tests/Declarations/CssFontDescriptorProperty.cs | Adds declaration parsing tests for legal/illegal descriptor values. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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
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
Please make sure you can check the following two boxes:
Contribution Type
What types of changes does your code introduce? Put an
xin all the boxes that apply:Description
Preserve
@font-facedescriptors outside the hardcoded set of sevenFixes the silent descriptor loss reported downstream in mganss/HtmlSanitizer#541.
Problem
CssFontFaceRulekept only the seven descriptors named in its privateContainedPropertiesset.CssDeclarationRule.CreateNewPropertyreturnednullfor everything else, andSetValuethen skipped the declaration entirely - without ever consulting the parser options:So
CssParserOptions.IsIncludingUnknownDeclarations- the switch that keeps unrecognized declarations alive in ordinary style rules - had no effect inside@font-face. Standard CSS Fonts Level 4 descriptors (font-display,size-adjust,ascent-override,font-feature-settings, …) and vendor descriptors (mso-*, common in email HTML) were both lost. The drop is silent: no exception, no diagnostic, andToCss()emits a rule that looks well-formed, so a caller round-tripping a stylesheet has no way to notice.CssViewportRule,CssCounterStyleRuleandCssFontFeatureValuesRulederive from the same base class. The latter two pass an empty contained set, so they were dropping every declaration:Before
Output was byte-identical with
IsIncludingUnknownDeclarations = false.After
Changes
1. Honour
IsIncludingUnknownDeclarationsinCssDeclarationRuleDeclarations outside a rule's descriptor set now fall through to the same
IsAllowingUnknownDeclarations()gate that style rules already use, instead of being dropped unconditionally.@font-faceand style rules now agree in every configuration. This applies to@counter-style,@font-feature-valuesand@viewporttoo, since they share the base class.BrowsingContextExtensions.IsAllowingUnknownDeclarationschanged fromprivatetointernalto make this reusable. No public API change.2. Register the missing standard descriptors
Of the dropped descriptors, only
font-displaywas actually a known declaration.size-adjust,ascent-override,descent-override,line-gap-overrideandfont-feature-settingshad no declaration registered at all - extendingContainedPropertiesalone would not have saved them, since they would still be flaggedPropertyFlags.Unknownand gated by the option.They are now registered with real value grammars, so they are kept - typed - by default:
size-adjust<percentage>100%ascent-overridenormal | <percentage>normaldescent-overridenormal | <percentage>normalline-gap-overridenormal | <percentage>normalfont-feature-settingsnormal | [ <string> [ <integer> | on | off ]? ]#normalSupporting additions:
OnlyPercentConverter/PercentConverter(there was no percentage-only converter -LengthOrPercentConverterwould have accepted10pxforsize-adjust), and theon/offkeywords.CssFontFaceRule.ContainedPropertiesgains all seven Fonts L4 descriptors.3. Three defects this exposed
font-variation-settingsaccepted onlynormal. Its converter wasAssign(CssKeywords.Normal, …), so"wght" 400was rejected in style rules and would have been rejected in the newly preserved@font-face. Now implementsnormal | [<string> <number>]#.CssDeclarationRule.SetValueadded properties without checking they parsed, so@font-face { size-adjust: 10px }serialized as@font-face { size-adjust: ; }. Invalid values are now ignored and leave an existing valid declaration standing (font-weight: 400; font-weight: boguskeeps400), matchingCssStyleDeclaration's behaviour.ICssFontFaceRule.Featureswas a stub -get => String.Empty; set { }- despite itsfeatureSettingsDOM name. It now maps tofont-feature-settings.Behaviour summary
@font-faceIsIncludingUnknownDeclarations)font-family,src,font-style,font-weight,font-stretch,font-variant,unicode-rangefont-displaysize-adjust,ascent-override,descent-override,line-gap-overridefont-feature-settings,font-variation-settingsmso-generic-font-family,--custom-thing,colorScope
@counter-styleand@font-feature-valuesare only fixed under the opt-in. Their descriptors (system,symbols,suffix, …) are still unregistered, so by default they remain dropped. Registering the ~10 counter-style descriptors is a separate piece of work.CssDeclarationRule.ToCsspassesnullas the prelude, so@counter-style thumbs { … }serializes as@counter-style { … }even when the declarations survive. Separate defect, worth its own issue.One pre-existing quirk worth noting, unchanged here:
IsAllowingUnknownDeclarationsresolves viaGetProvider<CssParser>(), which misses a factory-registered parser that has not been resolved yet and then defaults to permissive. That is why the original report saw identical output fortrueandfalse. This PR reuses that same gate rather than working around it, so the@font-faceand style-rule paths stay consistent whatever it resolves to.Testing
src/AngleSharp.Css.Tests/Rules/FontFaceDescriptors.cs- descriptor preservation, vendor/custom descriptors under both option values, invalid-value handling, overwrite semantics,Featuresmapping,ToCssround-trip, and sibling-rule coverage for@counter-style/@viewport.src/AngleSharp.Css.Tests/Declarations/CssFontDescriptorProperty.cs- legal and illegal values for each new grammar.