fix(fonts): make Google Fonts subsetting CSS text-transform aware - #3577
fix(fonts): make Google Fonts subsetting CSS text-transform aware#3577miga-heygen wants to merge 3 commits into
Conversation
Extends the subset character closure to cover locale/context-sensitive case transforms and non-case CSS text-transform values: - Parse lang attributes from authored HTML and apply toLocaleUpperCase/ toLocaleLowerCase for each detected locale (covers Turkish İ/ı, Azeri, German ẞ, and other locale-dependent casing) - Map ASCII U+0021–U+007E to fullwidth equivalents U+FF01–U+FF5E when full-width appears in the source - Map small hiragana/katakana to full-size equivalents when full-size-kana appears in the source - Preserve the existing 1700-char encoded URL budget and full-font fallback Closes #3496 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Split extractGoogleFontsText into addCaseClosure, addFullwidthVariants, and addFullSizeKanaVariants. Extract subsetTextFor test helper to eliminate repeated URL→text boilerplate. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Reviewed the full deterministicFonts.ts and the whole new test file at 5a50580e, and measured the budget and locale behaviour directly rather than reading the benchmark's verdict. The locale closure is the right idea and the negative tests are well built, but two of the three additions have defects that reach the render path, and the benchmark does not support the claim the description makes from it.
Strengths
collectLangAttributes(:1226-1236) walks[lang]across the whole tree instead of reading<html lang>only, and there is a test pinning the nested case. That is the shape that actually matches authored compositions, where a single foreign-language block carries its ownlang.- The Turkish mapping is real, not assumed —
"i".toLocaleUpperCase("tr")does giveİ, so the premise of #3496 holds. - The negative test is the best thing in this PR. "does not include Turkish İ/ı without a Turkish lang attribute" proves the locale gate actually discriminates, rather than only showing the positive case passing. Most locale changes ship without that half.
addCaseClosure(:1239-1249) keeps #3492's locale-independent pass intact and layers locale variants on top, so the earlier closure's behaviour is preserved rather than replaced.
Blocker — a malformed lang attribute throws RangeError out of font injection
:1233 takes the primary subtag with lang.split("-")[0]!.toLowerCase() and :1245 hands it straight to toLocaleUpperCase(locale). That method throws on a tag that is not structurally valid BCP-47, and the split does not validate. Measured:
lang="en_US" -> RangeError: Incorrect locale information provided
lang="x" -> RangeError: Incorrect locale information provided
lang="123" -> RangeError: Incorrect locale information provided
lang="türkçe" -> RangeError: Incorrect locale information provided
lang="en-US" -> ok lang="tr" -> ok
lang="en_US" with an underscore is a routine authoring mistake, and it now hard-fails a composition that rendered fine before this PR — lang was not read at all previously, so this is new.
It is unguarded end to end. extractGoogleFontsText is called as a bare argument at :1343, and neither call site wraps it: packages/producer/src/services/htmlCompiler.ts:1918 and packages/cli/src/server/studioServer.ts:438. It is also not a FontFetchError, so the failClosedFontFetch typed-error handling cannot classify it. What the author sees is RangeError: Incorrect locale information provided raised from inside font subsetting, with nothing naming the attribute that caused it.
Filtering the tags through Intl.getCanonicalLocales() in a try/catch, or wrapping the per-locale body at :1244-1248 and skipping tags that throw, both fix it. Either way an invalid lang should cost you the locale variants, not the render.
Blocker — html.includes("full-width") over-triggers, and one spurious hit spends a third of the URL budget
:1277 gates the fullwidth expansion on a raw substring search over the entire HTML source. full-width is not a rare string: a CSS class named .full-width, a data-layout="full-width" attribute, a comment, or the words in body prose all satisfy it. The expansion then adds the whole U+FF01–U+FF5E block, and every one of those code points costs 9 characters once percent-encoded — 846 encoded characters, 49.8% of the 1700 budget, on its own.
Measured on one composition, changing nothing but a class name:
class .wide + 100 CJK glyphs -> 798/1700 encoded (46.9%)
class .full-width + 100 CJK glyphs -> 1410/1700 encoded (82.9%) +612 chars
Renaming a CSS class costs 36% of the budget, with no text-transform anywhere in the document. Roughly 32 further distinct glyphs then tip it past the cap, extractGoogleFontsText returns undefined, :1078 omits text=, and the composition silently downloads the full font.
That is the specific outcome #3496's scope said this work must not cause, and the gate is what causes it — the closure itself is fine. Requiring the transform context (matching text-transform adjacency rather than a bare substring) confines the cost to compositions that actually asked for it. :1278 has the same shape for full-size-kana, though at 22 code points it is the cheaper half.
Important — the benchmark does not show what the description says it shows
The description offers the mixed-script benchmark as evidence the budget is safe. Measured on that test's own fixture:
benchmark as written -> 1231/1700 encoded (72.4% used, 469 left)
same composition, transforms off -> 448/1700 encoded (26.4% used)
The three additions nearly triple the subset, and they do it on a fixture holding 86 characters of Latin prose and 18 CJK glyphs. A composition that small consuming 72% of the budget is evidence against the change being cheap, not for it. The assertion passes with about 52 distinct CJK glyphs of margin, which any real Japanese or Chinese composition exceeds immediately.
toBeLessThanOrEqual(1700) is also the wrong assertion for the risk. What needs pinning is the cost the transforms add — assert the delta, or assert that a realistic worst case (several locales plus all three transforms over a few hundred CJK glyphs) still fits. As written the test would stay green through exactly the regression it is named for.
Important — the locale closure runs per source character, including base64 payloads
:1272 feeds the entire HTML source through addCaseClosure, which is deliberate for the character set, but each locale now adds two more case conversions per character. On a 521 KB composition with one embedded base64 image:
0 locales -> 52 ms 1 locale -> 685 ms 3 locales -> 1344 ms
Set size went from 72 entries to 74. That is roughly 1.3 seconds of compile-path work to gain two code points.
Deduplicating before the closure fixes it without changing the result: iterate new Set(chars) instead of chars at :1272-1274. I verified the output set is byte-identical both ways, on the same 41-distinct-character sample where the as-written loop runs 129 iterations.
Nit
The diff drops the comment explaining why raw html is fed into the set at all ("intentional over-approximation: base64, scripts, and class names collapse in the Set"). That rationale is now more load-bearing than before, because it is exactly what makes the budget arithmetic above non-obvious. Worth keeping a line of it.
Scope
Audited: deterministicFonts.ts (read whole, measured the extraction against the repo's own linkedom and postcss), deterministicFonts-textSubset.test.ts (read whole). Trusting: the description's "73 existing tests / 294 assertions pass" — I did not re-run the suite; CI covers it.
Verdict
REQUEST CHANGES. The locale closure is worth landing and its tests are better than most. The two gates need work before it does: validate the lang tags so a typo cannot fail a render, and scope the full-width trigger to the transform so a class name cannot silently push a composition onto the full font. The benchmark should assert the added cost rather than a single small fixture staying under the cap.
Footnote on timing: I held this submit until the pull_request matrix at this head concluded, because a review event can cancel an in-flight matrix in the same concurrency group and nothing re-fires it — which would have left this commit with no test result at all. Worth knowing if a review ever seems slow to appear on a freshly pushed head; pushing during that window restarts the wait.
— Rames Jusso
…nput - Validate lang attributes with Intl.getCanonicalLocales before passing to toLocaleUpperCase — malformed tags (en_US, x, 123) no longer throw RangeError. - Gate fullwidth/kana expansion on text-transform declarations instead of raw html.includes — a CSS class named .full-width no longer eats half the URL budget. - Deduplicate characters before the closure loop (new Set) to avoid redundant locale conversions on base64-heavy compositions. - Benchmark now asserts the transform cost delta, not just that one small fixture fits under the cap. - Restore over-approximation comment. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
miga-heygen
left a comment
miga-heygen
left a comment
There was a problem hiding this comment.
All four points addressed at e6dea112c:
1. Malformed lang → RangeError (blocker): collectLangAttributes now validates each primary subtag through Intl.getCanonicalLocales() in a try/catch. Invalid tags (en_US, x, 123, türkçe) are silently skipped — you lose the locale variants, not the render. New test: "skips invalid lang attributes without crashing".
2. html.includes("full-width") over-triggers (blocker): Replaced with TEXT_TRANSFORM_FULL_WIDTH_RE = /text-transform\s*:[^;]*full-width/ — only fires when there's an actual text-transform declaration containing full-width. A CSS class named .full-width or a data-layout="full-width" attribute no longer triggers the expansion. Same treatment for full-size-kana. New test: "does not trigger fullwidth expansion from a CSS class named full-width".
3. Closure loop performance on base64: Changed the iteration at the entry point from [...Array.from(html), ...Array.from(decodedBodyText)] to new Set([...Array.from(html), ...Array.from(decodedBodyText)]) — deduplicates before the closure runs, so a 521 KB base64-heavy composition runs the locale conversions on ~distinct characters only, not per-source-character.
4. Benchmark assertion: Now asserts the transform cost delta (with-transforms minus without-transforms) stays under 900 encoded chars, plus the absolute cap. This catches the specific regression where transforms silently push realistic compositions onto full-font downloads.
Comment restored: Added the over-approximation rationale back ("raw html includes base64, scripts, and class names, but they collapse in the Set and the budget gate catches bloat").
— Miga
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Re-review at e6dea112c. Three of the four items are genuinely fixed. The text-transform gate is not — the new regex trades the old false positive for a broader one, and I can reproduce the original budget blowout against this head.
I re-derived all four rather than reading the summary, because these implement my own recommendations and that is exactly where a reviewer stops looking.
Fixed, verified
langvalidation (deterministicFonts.ts:1226-1240).Intl.getCanonicalLocales(primary)inside a try/catch is the right guard, and it is exactly as strict as the thing that was throwing. I fuzzed 41 tags through bothIntl.getCanonicalLocalesand"a".toLocaleUpperCase(tag)looking for a gap where validation passes but the call at:1250still throws: zero gaps.en_US,en_us,x,123,türkçe,i,a,abcd,root,x-priv,en--US," "all rejected by both;en-US,tr,zz,und,tlh,zh-Hant,de-DE-1901,en-US-u-ca-buddhistaccepted by both. The crash is closed, not narrowed.- Dedupe before the closure (
:1283). Byte-identical output confirmed at this head —Setpreserves first-occurrence order, so insertion order intouniqueCharactersis unchanged (593 → 593 encoded chars on a mixed-script fixture, strings equal). - Benchmark now asserts the delta (
deterministicFonts-textSubset.test.ts:185-188). Measured against the real fixture:withTransforms1231,withoutTransforms448,transformCost783 against the< 900bound. That pins the cost magnitude, which is what was missing. Note the total is 1231 = 72.4% of the 1700 budget, so the headroom is thinner than "stays within the URL budget" suggests — worth knowing, not worth blocking.
Blocker — the new gate matches across rule boundaries, reintroducing the bug
deterministicFonts.ts:1272-1273:
/text-transform\s*:[^;]*full-width/
[^;]* is stopped only by a semicolon, and a declaration's trailing semicolon is optional. So the match runs from a text-transform: declaration through the end of its rule and into whatever follows — including a class name in a later rule, or class="full-width" in the body markup.
Both of these fire the fullwidth expansion at this head:
<style>h1 { text-transform: uppercase }
.full-width { width: 100% }</style>
<style>h1 { text-transform: uppercase }</style>
<div class="full-width">…</div>Neither page applies text-transform: full-width to anything. On a realistic page (one text-transform: uppercase rule, a .full-width layout class, ~100 chars of Latin copy) the spurious trigger costs 684 encoded characters — 40.2% of the 1700-char budget. That is the same failure I flagged at 5a50580e, reachable through a narrower door: any page combining a text-transform declaration with the string full-width elsewhere.
Omitting the final semicolon in a block is ordinary CSS, and it is what most minifiers emit, so this is not a corner case.
Second defect, same two lines: CSS property names and keyword values are ASCII case-insensitive, but these regexes are not. text-transform: FULL-WIDTH and text-transform: Full-Width both apply the transform in the browser and neither fires the gate, so the fullwidth glyphs are left out of the subset — I confirmed Ａ is absent from the emitted text= for the uppercase spelling. This one is a pre-existing miss (html.includes("full-width") was case-sensitive too), but it lives in the two lines this PR is rewriting and it is the same character of fix. Missing glyphs are a worse outcome than a wasted budget: the render is visibly wrong rather than merely unsubsetted.
One regex resolves both:
const TEXT_TRANSFORM_FULL_WIDTH_RE = /text-transform\s*:\s*[^;{}]*\bfull-width\b/i;
const TEXT_TRANSFORM_FULL_SIZE_KANA_RE = /text-transform\s*:\s*[^;{}]*\bfull-size-kana\b/i;Excluding { and } is what prevents the bridge — a value can never legally contain a brace, so the match cannot leave its own declaration block. I ran 13 cases against both versions: the current regex is wrong on 5 (three genuine-but-uppercase declarations missed; the bridge and the body-markup class spuriously fired), the proposed one is correct on all 13, including !important, minified h1{text-transform:full-width}, a newline inside the value, and an inline style= attribute.
The new test cannot catch this
deterministicFonts-textSubset.test.ts:213-221 is the right idea, but its fixture (:216) contains no text-transform declaration at all — so it passes under a regex that has no anchoring whatsoever. The case that discriminates is a page with both a text-transform declaration and a full-width token elsewhere:
it("does not trigger fullwidth expansion from an unrelated text-transform plus a full-width class", async () => {
const text = await subsetTextFor(
`<!doctype html><html><head><style>
h1 { font-family: "Noto Performance Test", sans-serif; text-transform: uppercase }
.full-width { width: 100% }
</style></head><body><div class="full-width"><h1>ABC</h1></div></body></html>`,
);
expect(text).not.toContain("Ａ");
});Worth a positive companion asserting Ａ is present for text-transform: FULL-WIDTH, so the case-insensitivity fix is pinned too.
CI
Not treating this head as green: at review time Build, Semantic PR title, Test: runtime contract and Typecheck had passed, while Test and Render on windows-latest were still in_progress and Tests on windows-latest and regression had not reported at all (they are roll-ups that do not exist until their shard matrices finish, so absent reads identically to passing unless you check each required name). Zero failures so far. My verdict does not rest on any of it — the two defects above are reproducible from the source.
Heads-up on a flicker this review itself may cause: a pull_request_review event can cancel the in-flight pull_request matrix in the same concurrency group, and review-triggered runs skip their jobs by design. If you see a cancelled run at e6dea112c, that is this review, not a failure — the fix push will re-fire the full matrix, which is why I did not hold the submit this time.
Verdict
REQUEST CHANGES. The lang crash, the dedupe and the benchmark delta are all properly resolved and I would not hold the PR on any of them. The gate rewrite still admits the budget blowout it was meant to remove, and additionally misses genuine uppercase declarations; both close with the one regex above plus a fixture that has a text-transform declaration in it.
— Rames Jusso
2 participants
What
Extends the Google Fonts
text=subset closure to cover locale-sensitiveCSS text transforms that the locale-independent
toUpperCase()/toLowerCase()closure in #3492 explicitly deferred.
Fixes #3496.
Why
A composition with
lang="tr"andtext-transform: uppercaseneeds Turkish İ(U+0130) in its subset —
"i".toUpperCase()gives"I", not"İ". Similarly,text-transform: full-widthandfull-size-kanasynthesize glyphs that theexisting code-point closure cannot reach. Without these variants in the subset,
the browser falls back glyph-by-glyph and the rendered output shows a different
face for the transformed characters.
How
Three additions to
extractGoogleFontsText, continuing the existingover-approximation pattern (safe but not CSS-property-aware):
Locale-aware case closure —
collectLangAttributeswalks the DOM forlangattributes and normalizes to BCP-47 primary subtags. For each detectedlocale,
toLocaleUpperCase(locale)/toLocaleLowerCase(locale)close thecharacter set. Covers Turkish/Azeri İ/ı and German ẞ.
Fullwidth ASCII — when
full-widthappears anywhere in the HTML source,maps ASCII U+0021–U+007E to fullwidth equivalents U+FF01–U+FF5E.
Full-size kana — when
full-size-kanaappears in the source, maps 22small hiragana/katakana to their full-size counterparts via a const lookup.
The URL-length budget (1700 encoded chars) and full-font fallback are preserved.
Test plan
deterministicFonts-textSubset.test.ts:lang="tr"(present) andlang="en"(absent)lang="az"full-widthCSS)— Miga