fix: stop Dynamic Type from discarding the configured fontType - #79
Merged
Conversation
Dynamic Type is expressed with the CSS `font` shorthand
(`html { font: -apple-system-body; }`), and a shorthand resets every longhand it
covers, including `font-family` and `font-style`. Those rules live in
`customCSS`, which is emitted after the generated rules, so they always won the
cascade. The result was that `supportsDynamicType: true` silently threw away
`fontType`, and `.monospaced`, `.italic` and `.customName` all rendered in the
default family.
Add `Configuration.resolvedCustomCSS`, which appends a font override after the
Dynamic Type rules, and use it everywhere `customCSS` was emitted. `customCSS`
itself is unchanged, so anything reading it keeps the current behaviour.
Also pins down the underlying #63 fix that shipped in v3.0.0: font names must be
valid CSS families. The pre-3.0 implementation returned raw PostScript names
such as `.AppleSystemUIFontMonospaced-Regular`, which WebKit could not parse as
an unquoted family, so it dropped the declaration and fell back to serif.
Refs #63
There was a problem hiding this comment.
🟡 Changes recommended
The added override selector is not specific enough to beat Dynamic Type’s p.subheadline/etc rules and may also introduce an unintended cascade behavior change when fontType is the default system font.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR fixes an interaction where enabling Dynamic Type (implemented via CSS font shorthand rules appended to customCSS) unintentionally reset font-family/font-style, causing the configured fontType (e.g., .monospaced, .italic, .customName) to be lost in the final cascade.
Changes:
- Added
Configuration.resolvedCustomCSSto append a framework-managed font override intended to win after Dynamic Type shorthand rules. - Switched CSS emission sites (
WebView.generateCSS()andConfiguration.generateCompleteCSS()) fromcustomCSStoresolvedCustomCSS. - Added tests to validate CSS-safe font family strings and to assert ordering between Dynamic Type shorthands and font overrides.
File summaries
| File | Description |
|---|---|
| Tests/RichTextTests/RichTextSwiftTestingTests.swift | Adds regression tests around CSS font name formatting and Dynamic Type + fontType behavior. |
| Sources/RichText/Views/Webview.swift | Emits resolvedCustomCSS in generated HTML/CSS so overrides apply in the WebView. |
| Sources/RichText/Models/Configuration.swift | Introduces resolvedCustomCSS and updates complete CSS generation to use it. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+113
to
+123
| public var resolvedCustomCSS: String { | ||
| guard supportsDynamicType else { | ||
| return customCSS | ||
| } | ||
|
|
||
| let fontOverrideCSS = """ | ||
| html, body, h1, h2, h3, h4, h5, h6, p { font-family: \(fontType.name); \(fontType.additionalCSSProperties) } | ||
| """ | ||
|
|
||
| return customCSS + "\n" + fontOverrideCSS | ||
| } |
Comment on lines
+179
to
+190
| let shorthandRange = css.range(of: "font: -apple-system-body") | ||
| let familyRange = css.range(of: "font-family: \(fontType.name)", options: .backwards) | ||
|
|
||
| #expect(shorthandRange != nil) | ||
| #expect(familyRange != nil) | ||
|
|
||
| // The `font` shorthand resets `font-family`, so the family has to be re-applied | ||
| // after the Dynamic Type rules to survive the cascade. | ||
| if let shorthandRange, let familyRange { | ||
| #expect(familyRange.lowerBound > shorthandRange.lowerBound) | ||
| } | ||
| } |
Addresses Copilot review feedback on #79. Two problems with the first version of the override: 1. It used a bare `p` selector, specificity (0,0,1). `RichTextConstants.dynamicTypeCSS` styles `p.subheadline`, `p.footnote`, `p.caption1` and `p.caption2` at (0,1,1), so those four paragraph classes still won the cascade and kept resetting `font-family` no matter how late the override was emitted. Name the classes explicitly so specificity ties and source order decides. 2. It was emitted unconditionally. For the default system font the override is a no-op against the Dynamic Type shorthands, but it is appended after `customCSS`, so it would clobber a caller's own `font-family` rule for no benefit. Emit it only when the configured font actually differs, either in family or in the extra properties `.italic` and `.monospaced` add. Tests now compare against the last Dynamic Type shorthand rather than the first, assert the paragraph classes are covered, and assert that no override is emitted for `.system`.
There was a problem hiding this comment.
🟢 Approval recommended
The fix is localized, aligns with the stated root cause (shorthand resetting longhands), and is backed by targeted regression tests for cascade order and selector specificity.
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Lite
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.
Problem
Dynamic Type is expressed with the CSS
fontshorthand:A shorthand resets every longhand it covers, which includes
font-familyandfont-style. Those rules are appended tocustomCSS, andcustomCSSis emitted after the generated rules, so they always won the cascade.The result: setting
supportsDynamicType: truesilently threw awayfontType..monospaced,.italicand.customNameall rendered in the default family, with no warning and nothing in the generated CSS that looked wrong at a glance.Changes
Add
Configuration.resolvedCustomCSS, which appends a font override after the Dynamic Type rules:and use it everywhere
customCSSwas previously emitted (WebView.generateCSS()andConfiguration.generateCompleteCSS()).customCSSitself is untouched, so anything reading that property keeps its current value and behaviour.Note on #63
The originally reported symptom was fixed in v3.0.0 and this PR pins it down with a test. Root cause there:
FontType.nameused to return the raw PostScript name, e.g..AppleSystemUIFontMonospaced-Regular. A CSS family starting with.is not a valid unquoted identifier, so WebKit discarded the wholefont-familydeclaration and fell back to its default serif face, which is exactly what the reporter saw. v3.0.0 returns real CSS stacks and quotes custom names; the new test asserts that font names never start with..The Dynamic Type interaction above was the one part of #63 still broken on 3.x.
Fixes #63