fix: respect CSS generic font-family fallback chain#403
Open
fix: respect CSS generic font-family fallback chain#403
Conversation
set_font_families() ignores the configured generic family options (sansSerifFamily, serifFamily, etc.) and sets ALL generic families to the first font loaded into fontdb. On this system that's "Font Awesome 6 Brands" — an icon font missing text glyphs — causing font-family chains like "'Berkeley Mono', 'Noto Sans', sans-serif" to render "7.4B" as "74B" (missing the dot). This test verifies that sansSerifFamily is respected: font-family="sans-serif" with sansSerifFamily='Liberation Sans' should render identically to font-family="Liberation Sans" directly. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
set_font_families() was ignoring the user-configured generic family
options (serifFamily, sansSerifFamily, cursiveFamily, fantasyFamily,
monoSpaceFamily) and blindly setting ALL generic families to
default_font_family — which was often empty ("") or the first font
loaded into fontdb (e.g. "Font Awesome 6 Brands").
This caused font-family fallback chains like
"'Berkeley Mono', 'Noto Sans', sans-serif"
to resolve sans-serif to an icon font, dropping glyphs for "." ":" etc.
The fix:
- New resolve_generic_family() helper uses the configured value if it
exists in fontdb, otherwise falls back to the first available font
- set_font_families() and set_wasm_font_families() now use per-generic
configured values instead of a single default_font_family
- find_and_debug_font_path() no longer overrides generic families
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The previous commit respected user-configured generic family values, but the defaults (Arial, Times New Roman, etc.) are Windows fonts that don't exist on Linux. When sans-serif defaulted to "Arial" and Arial wasn't installed, resolve_generic_family fell back to the first font in fontdb — still "Font Awesome 6 Brands" on this system. Now resolve_generic_family tries a list of well-known alternatives for each generic family (Arial → Liberation Sans → Noto Sans → DejaVu Sans → ...) before falling back to the first available font. This mirrors browser behavior. Also adds a direct repro test: rendering "7.4B" vs "74B" with font-family="sans-serif" must produce different pixels (proving the dot renders). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@wakamex is attempting to deploy a commit to the yisibl's projects Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
Pull request overview
This PR fixes generic CSS font-family resolution so sans-serif/serif/etc respect the configured generic-family options and fall back through a browser-like chain of well-known fonts, avoiding cases where a generic family accidentally resolves to an icon font without text glyphs.
Changes:
- Added generic-family resolution logic that probes configured values, then OS-common fallback families, then the first available font in
fontdb. - Updated both native and wasm font-family setup to use the generic-family resolver (and reduced default-font debug behavior).
- Added tests validating
sansSerifFamilycontrolssans-serifresolution and that punctuation like.renders (detects the “7.4B → 74B” regression).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/fonts.rs |
Introduces generic-family resolver + fallback lists; updates native/wasm family setup to avoid generic families resolving to the first loaded face. |
__test__/index.spec.ts |
Adds regression tests for sans-serif resolution and punctuation rendering. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Address PR review comments: - Build a HashSet of available family names once instead of scanning all fontdb faces per candidate (resolve_generic_family is gone, replaced by a closure over the shared set) - Apply cargo fmt to fix formatting Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Instead of hardcoding "Liberation Sans" (unavailable on macOS/Windows), probe for an available sans-serif font by comparing against a non-existent font baseline to detect per-character fallback. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Author
|
thanks for the review! tests pass on my fork https://github.com/wakamex/resvg-js/actions/runs/22752685120 |
Member
|
Thank you for your contribution. I don’t expect to have time to continue reviewing this PR until April. |
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.
Summary
set_font_families()now respects the configured generic family options (sansSerifFamily,serifFamily, etc.) instead of setting all generic families to the first font loaded into fontdb — which could be "Font Awesome 6 Brands" or another icon font lacking text glyphs.font-family="'Berkeley Mono', 'Noto Sans', sans-serif"rendered7.4Bas74B(invisible dot) becausesans-serifresolved to an icon font.Test plan
sansSerifFamilyoption controlssans-serifresolution (pixel-exact match against direct font name)sans-serifrenders "." in "7.4B" (compares pixel output of "7.4B" vs "74B" — must differ)/tmp/repro_fixed.pngshows7.4Bwith dot🤖 Generated with Claude Code