feat(fonts): prefer a registered real face over the bundled substitute (provider precedence)#3640
Merged
Merged
Conversation
…e (provider precedence) The resolver picked the bundled clone (Calibri->Carlito) even when the document registered a real Calibri face via fonts.add, so a customer-provided font was ignored unless they explicitly mapped around it. Add a registered_face step to the face-aware ladder: explicit fonts.map override > registered real face for the logical family > bundled metric-compatible substitute > as-requested. resolveFace/resolvePhysicalFamilyForFace consult hasFace for the logical family before the clone, and preload follows the same precedence so it loads the real family, not the clone. Adds the public FontResolutionReason value 'registered_face' so diagnostics distinguish 'rendered the real registered font' from a bundled substitute or browser fallback. Embedded DOCX fonts becoming registry entries is a separate follow-up; this only changes resolution precedence for already-registered faces.
superdoc-bot
Bot
added
review: thorough
review: careful
and removed
review: thorough
labels
![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: 0bc7dc4b92
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
The 'four-face clone substitutes' test passed an all-true hasFace oracle, which marks the logical family (Calibri) itself registered - so provider precedence correctly resolves it to registered_face (Calibri) instead of the bundled clone, and the test's expected Carlito faces no longer matched. Use a clone-only oracle (Carlito registered, Calibri not), matching the test's intent: the normal bundled-substitute case.
…probe matches A quoted CSS primary like '"Calibri"' kept its quotes through splitStack, while hasFace normalizes quotes and reports the family registered - so resolveFace returned physicalFamily '"Calibri"'. The load/preload path feeds that into faceProbe, which quotes it again, so the browser probes a literal "Calibri" and never matches the registered face (a false fallback_used + reflow), worst for multi-word families. Add a case-preserving stripFamilyQuotes() (distinct from normalizeFamilyKey, which lowercases for keying) and apply it to the structured resolveFace/resolveFontFamily physicalFamily, which is a load/report family not a CSS stack. The CSS paint variants (resolvePhysicalFamily/resolvePhysicalFamilyForFace) keep the quoted stack, which is valid CSS, so measure and paint stay consistent.
The root vitest projects list omitted shared/font-system, and the other-packages CI job's --filter list omits @superdoc/font-system, so the package's resolver/registry/report/os2 tests never ran in CI - which is how a quoted-family regression and two mutually contradictory resolver tests went unnoticed. Add ./shared/font-system to the vitest projects so the (node-env, @font-system) suite runs under the existing other-packages job.
…weight/clone-pin) Four correctness fixes to the provider-precedence ladder, all from the #3640 review: (A) hasFace now consults a register-only #providerFaceKeys set instead of #facesByFamily, which the await path also fills - so an awaited as_requested family no longer masquerades as registered_face on the next plan. (2) A registered face whose asset terminally fails drops out of hasFace (timed_out does not), and the gate replans once per newly-failed required face (fire-once, factored #scheduleAvailabilityReflow) so it demotes to the bundled clone instead of rendering broken forever. (3) An explicit map to the bundled clone is now STORED as a custom_mapping override (identity self-map still drops, compared with the resolver's family normalization), so the pin beats a registered real face. (4) The FontFace is built with the bucketed weight/style the key uses, so an off-bucket 500 face no longer answers the 400 query and renders at 500. Tests + the two contradictory resolver/controller map-to-default tests updated to the new contract.
After provider precedence, map({ Calibri: 'Carlito' }) is honored as a pin that outranks a registered real Calibri (custom_mapping > registered_face), not the old map-to-default no-op. Update the public fonts.map JSDoc and the resolver map() JSDoc so reviewers/consumers do not assume the prior behavior; a no-op is now only an identity self-map or a redundant same-target map.
… report fake The face report fake registered a face when it only meant to give it a load status, so after the registered_face ladder rung a pass-through Georgia 700 (Gelasio lacks bold) reported registered_face instead of fallback_face_absent. Add setAwaitedFaceStatus (status, not registration) and use it for the pass-through, matching the real registry where awaiting a face does not make it a provider. Surfaced now that shared/font-system runs in CI.
A no-op is a self-map or a mapping identical to an already-stored override - not 'the target it already has', which read as if map-to-bundled-default were still a no-op. Clone pins are never no-ops.
Contributor
|
🎉 This PR is included in superdoc-cli v0.16.0 The release is available on GitHub release |
Contributor
|
🎉 This PR is included in superdoc-sdk v1.15.0 |
Contributor
|
🎉 This PR is included in @superdoc-dev/mcp v0.11.0 The release is available on GitHub release |
Contributor
|
🎉 This PR is included in superdoc v1.39.0 The release is available on GitHub release |
Contributor
|
🎉 This PR is included in @superdoc-dev/react v1.10.0 The release is available on GitHub release |
Contributor
|
🎉 This PR is included in vscode-ext v2.11.0 |
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
2 participants
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.
Makes the per-document font resolver prefer a real registered face for a logical family over the bundled metric-compatible clone. Before this, a document that registered a real Calibri via
fonts.addstill rendered Carlito unless the user explicitly mapped around it.The face-aware resolution now follows one precedence ladder, applied per face:
fonts.mapoverride — when the mapped family actually provides the faceregistered_face)fallback_face_absent) when a provider was configured but none could supply the faceA subtle case the ladder fixes: if you map
Calibri -> Tinos, Tinos lacks Bold, and a real Calibri Bold is registered, the resolver now renders the real Calibri Bold (registered_face) instead of reporting it missing. The override only consumes a face it can supply; otherwise the rest of the ladder still applies.Adds the public
FontResolutionReasonvalueregistered_faceso the report distinguishes "rendered the real registered font" from a bundled substitute or a browser fallback.preloadfollows the same precedence, so it loads the real family rather than the clone.Embedded DOCX fonts are not registered with the font system yet — the converter still injects them as
@font-faceCSS — so this PR changes precedence only for already-registered faces (fonts.add). Making embedded fonts first-class registry entries (with object-URL lifecycle and licensing handling) is a separate follow-up that will then flow through this same ladder.