test(fonts): font availability + curation behavior matrix (SD-3441)#3733
Closed
caio-pizzol wants to merge 9 commits into
Closed
test(fonts): font availability + curation behavior matrix (SD-3441)#3733caio-pizzol wants to merge 9 commits into
caio-pizzol wants to merge 9 commits into
Conversation
Drive the bundled-font modes through the existing behavior harness via a `fonts` config option (no-pack / curation / malformed raw / bad base), and serve the built dist/fonts at /fonts/ so face loads are assertable. Covers: no-pack baseline (exact + rich-absent), include drops the built-in baseline, exclude removes only the named family, applying Calibri loads its substitute (200) and stores the logical name not Carlito, and malformed raw fonts.bundled warns once without crashing. Default mode keeps the rich pack, so existing toolbar specs are unaffected.
…on (SD-3441) Load a Calibri document with no pack configured and assert it fetches no bundled .woff2, keeps the logical Calibri name on the run, and still lists Calibri in the toolbar as a document font. Guards the 1.40 regression: preserving a document's font name must not activate a substitute the app never configured.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Add a `package` harness mode that imports superdocFonts from @superdoc-dev/fonts (aliased to source in the harness, so no dep churn) and asserts applying Calibri loads Carlito 200 from the package's bundler-emitted asset (new URL(import.meta.url)), with no assetBaseUrl. Proves the import-and-go DX the assetBaseUrl harness does not cover.
…nts/ (SD-3441) The /fonts/ middleware made bundled substitutes load for every spec (the harness default assetBaseUrl is /fonts/), so specs reading a rendered font - e.g. list-marker-font-inheritance - saw Liberation Serif instead of Times New Roman. Serve the assets at a distinct /bundled-fonts/ base used only by the pack font mode, leaving the default /fonts/ unserved (its prior state) so substitutes are advertised but not loaded.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6a19d5e5d1
ℹ️ 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".
The package-import spec resolves faces to packages/fonts/assets via the source alias. That dir is gitignored and generated by the fonts package's prepare/build, which behavior CI skips (--ignore-scripts; root build only builds superdoc). The spec passes in CI today via an ambient workspace mechanism, but relying on it is fragile - so sync the assets explicitly in the Playwright webServer command. Verified cold: cleared assets + Vite cache -> sync writes 65 faces -> Carlito loads 200.
…st the count (SD-3441) The shared-registry first-config-wins contract (#14) is already covered here; tighten the assertion so it checks the warning explains WHY the later config was dropped, not merely that console.warn fired once.
…-3441) bad-url: a configured-but-broken assetBaseUrl still advertises the rich set (presence-gated) and requests faces from exactly that base, staying graceful (run keeps the logical name). Note: SuperDoc does not emit its own load-failure warning - the browser reports the decode failure - and the Vite harness returns an SPA fallback rather than a 404, so this asserts base-honoring + graceful fallback, not a 404. programmatic apply (no pack): applying a bundled font via the editor command keeps the logical name and fetches no substitute - the resolver gate, UI-aside.
Add 'custom' / 'custom-toolbar' harness modes that register a licensed font
('Brand Sans', sourced from a served woff2) via fonts.families, optionally
listing it in modules.toolbar.fonts. Two specs lock the contract: registration
alone renders the font when applied but does NOT add a toolbar option;
listing it in modules.toolbar.fonts makes it selectable. Also fix the now-stale
npm-font-availability header.
Trim the wordiest comment to the why (base-honored; no 404/SuperDoc warning - browser reports the decode failure, Vite serves an SPA fallback). Other comments encode real contracts and stay.
Contributor
|
This pull request landed on |
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.
Behavior coverage for the config-gated bundled fonts from SD-3441, so the npm and CDN paths are verified end to end instead of by inspection.
It drives the bundled-font modes through the existing behavior harness with a new
fontsconfig option, plus a/fonts/middleware that serves the built pack so real face loads are assertable. The default mode keeps the rich pack, so existing toolbar specs are unaffected.Covered so far (npm, green locally on chromium):
includedrops the built-in baseline,excluderemoves only the named family.fonts.bundledwarns once and falls back to the full pack, never crashing init.Still to come, and why this is a draft:
@superdoc-dev/fontsimport smoke test (the package'sresolveAssetUrlflow, distinct from theassetBaseUrlharness)Review: the harness change must stay backward-compatible (no
fontsparam = rich pack); confirmed against the existing font dropdown spec.Verified:
playwright test tests/fonts --project=chromiumgreen; existingfont-dropdown-document-options.spec.tsstill green.