fix(fonts): consolidate composition-aware compilation#2406
Conversation
vanceingalls
left a comment
There was a problem hiding this comment.
Lens split: runtime-interop + cross-layer contract (Rames covering test + observability). First pass + Rule 15 adversarial-user pass on 41fcd438. Consolidation of #2353 (Google Fonts text= subsetting) and #2322 (TTC direct embed) — proposed contracts from both source PRs are preserved in shape.
Strengths
fontFormatHintatdeterministicFonts.ts:435is a clean single-point-of-truth for theformat(...)hint. Threads correctly through Path 1 (bundled fill), Path 2 (Google), and Path 3 (system-capture) viabuildFontFaceRule, and pairsdata:font/collection;withformat("collection")— the exact combo Chromium accepts and matches the prior art atstudio-server/src/routes/fonts.ts:155-156where TTC →font/collectionwas already the served MIME.- Empty-HTML edge is safe:
extractGoogleFontsText("")returns""(falsy), sofetchGoogleFontskips&text=and falls back to the pre-PR full-subset request. - Determinism holds —
Array.from(html)iterates codepoints in order andSetpreserves insertion order per spec, so thetext=param is deterministic across runs. - URL-length ceiling (~1700 encoded chars for
text+ ~140 char prefix for family and wght list) sits comfortably under Google Fonts' effective 2 KB CSS API URL cap; the fall-back-to-full-subset above the ceiling is the right escape.
Important
-
deterministicFonts.ts:857-864—extractGoogleFontsTextcovers HTML source characters ([...new Set(Array.from(html))]), not rendered characters. Legitimate authoring patterns miss the subset:- HTML entity refs — source
旅renders旅at DOM but only&,#, digits,;reach the char set. - JS-computed text —
el.textContent = String.fromCharCode(0x672C)in a<script>block puts本on screen but not in the source string. - CSS generated content —
content: "\65E5"orcontent: attr(data-label)where the attribute value contains the rendered chars.
I verified Google Fonts'
text=response empirically:curl "…&text=abc"returns a face withunicode-range: U+61-63. So the browser doesn't render tofu on out-of-subset codepoints — it falls back to the CSSfont-familychain (typicallysans-serif→ host system font). On distributed / Lambda headless-shell hosts where system CJK coverage differs from the author's machine, this substitutes a different font silently. The regression only bites CJK-capable Google fonts (Noto Sans JP/TC/SC/KR, etc.) where the pre-PR full-subset fetch used to cover the runtime-injected codepoints.In-tree compositions don't exercise this pattern (grep on
packages/producer/testsforString.fromCharCodeand&#returns zero) — but user-authored compositions can. The PR's headline win is exactly this family of fonts.Cheap mitigation: also fold
parseHTML(html).document.body?.textContent ?? ""into the char set (already usinglinkedomin this file), which resolves entity refs. JS-computed remains a documented limitation. Alternatively, droptext=(keep full unicode) when the char-set-derived-from-source is under some minimum threshold — a signal that the compositon likely populates text at runtime. - HTML entity refs — source
-
deterministicFonts.ts:624(subsetToken) +:798(call site) — cache-key drift across compositions.subsetToken(woff2Url) = sha1(woff2Url).slice(0,12); post-PR the woff2 URL fortext=-scoped requests looks likehttps://fonts.gstatic.com/l/font?kit=<subset-specific-key>&skey=<hash>&v=v20(verified via curl). Different compositions with differenttext=params get different woff2 URLs → different cache filenames.Pre-PR: cache warmed on
Inter/400covered every subsequent composition using Inter/400.
Post-PR: each unique composition-text-set produces its own cache file. Distributed workers effectively cold-hit Google Fonts per composition. On Lambda whereresolveFontCacheRoot()points at/tmp(default 512 MB), a bursty fleet rendering high-variety compositions can grow the cache linearly with composition count.Suggested fix: rewrite the cache-key derivation to hash
(family, weight, style, unicode-range)rather than the woff2 URL — the URL is now content-addressed per text-subset so hashing it defeats sharing.unicode-rangeis captured by the parse regex at:785and available at cache-write time.
Nits
deterministicFonts-textSubset.test.ts:1-25— mocks Google with HTTP 400 and asserts each character of旅行ランキングappears in the constructed URL. That confirms URL construction. It does not confirm Google returns a woff2 covering those glyphs (mocked away), nor that the browser renders the composition correctly with the injected@font-face. There is no headless-Chrome / puppeteer assertion in this PR's tests. This is a boundary between "does the URL look right" and "does the render look right" — worth a follow-up positive-pin test that boots a browser against a fixture, especially given (1) above.htmlCompiler.ts:1602-1651(embedLocalFontFaces) — pre-existing gap, adjacent to this PR. It replaces the URL of a composition-authored@font-faceblock viaresult.replaceAll(localPath, dataUri)but leaves the surroundingformat(...)declaration untouched. Author-writtensrc: url("./foo.ttc") format("woff2")now points at adata:font/collection;URI afterfontToDataUri(post-PR) returns raw TTC. Chromium content-sniffs data URIs so this mostly works, but the point offontFormatHintis to make format/URI agreement invariant — running it here at rewrite time (or normalizing the enclosingformat(...)when we swap the URI) closes the last branch.
What I did not find
- No parallel-capture race:
injectDeterministicFontFacesruns entirely during compile time incompileForRenderathtmlCompiler.ts:1782, before any puppeteer capture stage starts. - Path 3 (system-capture) with TTC is consistent —
fontToDataUrireturnsdata:font/collection;,buildFontFaceRuleusesfontFormatHint→format("collection"). Match. - Path 1 (bundle + Google fill) skips Google faces for weights the bundle covers. If the bundle's weight-400 face lacks CJK and the composition needs CJK at weight 400, the CJK codepoints have no coverage — but this is pre-existing behavior, not introduced by this PR.
Verdict: COMMENT
Reasoning: Both important findings are real risks but affect user-authored composition patterns the in-tree tests don't exercise. Not gating merge, but recommend addressing (1) at minimum via the linkedom-textContent fold before shipping — that's the direct regression surface for the CJK subsetting win the PR is built around.
— Via
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at 41fcd438681429ed83f16283b34a354e5b29a8a1 — consolidates #2353 (Google Fonts text subsetting) + #2322 (TTC collection embedding).
Summary
Two orthogonal producer-side wins for font handling:
- Text subsetting for Google Fonts requests —
extractGoogleFontsText(html)extracts unique code points from the composition HTML, threads them throughbuildFontFaceCss→fetchGoogleFontas a&text=...query param, and Google Fonts returns a subset containing only the glyphs actually used. Under the 1,700-encoded-char cap the request URL stays inside the ~2 KB browser/proxy limit. - Raw TTC embedding —
fontCompression.ts:82-86short-circuits.ttccollections to a rawdata:font/collection;base64,...URI, skipping the slow (and previously botched) WOFF2 compression path.fontFormatHint(src)on the read side inspects the data URI and emitsformat("collection")vsformat("woff2")to match.
Verification
- Text-subset request contains the composition's characters —
.textSubset.test.ts:5-25renders a composition with CJK content (旅行ランキング) and asserts every character appears in the outgoingtextquery param. Fetch stubbed to 400 so no external call. fontFormatHintmatches the MIME map —RAW_MIME_TYPES.ttc = "font/collection"atfontCompression.ts:18aligns withdata:font/collection;prefix thatfontFormatHintdetects.format("collection")is the CSS Fonts Level 4 keyword; Chromium/Safari/Firefox accept it.- TTC embed skips the
try { woff2 compress } catch { fall back }slow path — new test verifiesconsole.warnis silent (no compression warning emitted) and the output is the raw base64. Removes a real latency hit for compositions using variable / collection fonts. - 1,700-char cap is a safe floor — encoded length after
encodeURIComponent, not raw length. Cap-exceeded fallback dropstextentirely and falls back to the full-weight family fetch (via existingunicode-rangesplits at CSS level). No hard failure mode.
Adversarial pass
- Static-only text extraction.
extractGoogleFontsTextscanshtmlfor characters — for a composition that reveals characters via GSAP text animation, the characters need to be present in the DOM at capture time. HF studio-emitted compositions do write the full text into text nodes (masked or transformed by animation), so this holds. Doesn't regress vs prior full-fetch behavior since the OLD path also relied on Google'sunicode-rangesplits and the same DOM shape. new Set(Array.from(html))iterates code points — correctly handles astral chars (e.g. emoji) viaArray.from. Good.- Cap-exceeded →
undefined→ notextparam — safe fallback, verified by the URL-building code (fontText ? \&text=…` : ""`). No off-by-one that would produce a malformed URL. fontFormatHintfalse-positive risk — only "collection" if the src literally begins withdata:font/collection;. Any bundled bytes that end up in a different format would needRAW_MIME_TYPESto route differently. Consistent by construction.
Verdict framing
Two clean wins with proper test coverage on both. LGTM from my side.
|
Addressed the direct CJK/entity regression in Via’s review at 9ebd396: Google Fonts text subsetting now unions raw source characters with browser-decoded body text, with a regression for numeric entity references resolving to visible CJK glyphs. Focused font tests and producer typecheck/lint pass. I did not replace the text-scoped cache key with family/weight/style/unicode-range: distinct text= requests can contain different glyph payloads while exposing no unicode-range, so that key would alias incompatible WOFF2 files; URL-derived identity remains correctness-preserving. |
vanceingalls
left a comment
There was a problem hiding this comment.
Approving on behalf of vance for merge. Prior R1 CJK/entity union fix now on 9ebd396; URL-only cache-key retained per Magi's rationale that text-scoped Google CSS returns distinct payloads without unicode-range — accepted as trade-off.
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at 9ebd396d21466741a17f44eb97f340766c083fa4 — R2 delta from my R1 (41fcd438).
R2 delta scope
Two-part fix at packages/producer/src/services/deterministicFonts.ts:860-863:
extractGoogleFontsTextnow unions raw-source characters ANDparseHTML(html).document.body?.textContent— i.e. adds browser-decoded body text alongside the R1 raw-source scan.- New regression at
deterministicFonts-textSubset.test.ts:26-43feeds<h1>旅行</h1>(numeric entities for旅行) and asserts the?text=query on the Google Fonts request contains the DECODED CJK glyphs (旅行), not just旅行character parts.
Verification
- Union captures both source and decoded forms —
[...new Set([...Array.from(html), ...Array.from(decodedBodyText)])]merges raw-source chars (which now include the CJK glyphs' escape sequence characters&,#,x, digits,;) with the decoded glyphs themselves. Without this,旅行never appears in the subset URL and Google's?text=returns a font stripped of those glyphs — CJK captions render as boxes. - Regression is load-bearing — the R1-only implementation (
Array.from(html)alone) would fail this assertion:旅行is nowhere in the raw HTML source (only in the entity-encoded form). The test would fail at thetext.includes(character)check. R2 passes. - Encoded-length ceiling still enforced —
GOOGLE_FONTS_TEXT_MAX_ENCODED_LENGTH = 1_700guard unchanged, applied to the merged character set. If merging body text overflows the cap, subsetting falls back toundefinedand the full font downloads. Correct fallback path. - Body-missing fallback —
document.body?.textContent ?? ""degrades to R1 raw-source-only behavior when the composition lacks a body element. No regression for edge shapes.
Adversarial pass
parseHTML(html)cost — R1 wasArray.from(html)(O(N) string scan). R2 adds a full HTML parse per composition. Realistic forinjectDeterministicFontFaceswhich is called once per composition per render, not per frame. Amortized fine.- Text-scoped Google Fonts cache key — per Magi's message, the URL-only cache key is intentionally retained: two compositions with different text scopes yield distinct glyph payloads even without a
unicode-rangedescriptor, so URL-keyed cache produces the correct-per-composition subset. Consistent with the fix framing. - What about
data-*attribute text? — decodedtextContentwalks all visible text nodes, so text inside inline<span>/<a>etc. is included. Alt-text and aria-labels are NOT text nodes and won't appear; if a caption glyph exists only in analt=attribute, it'd miss the subset. Realistic risk low — captions are typically text nodes. - Character iteration over multi-code-point emoji —
Array.from("🇯🇵")splits regional indicators into separate code points. Google's text subsetting resolves at glyph level, so this doesn't lose data — worst case a couple of extra scalars in the request URL. Not a regression from R1's identicalArray.frombehavior. document.body?.textContent— an empty body yields"". Union with the source characters produces the R1 set. Consistent.
Verdict framing
Clean R2 — Via's direct CJK/entity finding is closed with the decoded-body union and a load-bearing regression that would fail against the R1 raw-only path. LGTM from my side.
Summary
Verification