fix(compiler): split font-family only on top-level commas - #3067
Conversation
parseFontFamilyValue() split the family stack on every comma, so `font-family: var(--brand-font, inherit)` became two tokens: `var(--brand-font` and `inherit)`. The var() guard from heygen-com#1655 only skips tokens starting with `var(`, so the orphan fragment was treated as a requested family, failed every resolution path, and aborted fail-closed distributed renders with: FontFetchError: [Compiler] Unresolved fonts in fail-closed mode: inherit). Distributed renders require all fonts to be resolvable. Split on top-level commas only, so a var() expression (including a nested one) stays a single token. Quotes are tracked as well, both so parentheses inside a quoted family name cannot skew the depth counter and so a legal quoted comma no longer splits. Closes heygen-com#3066
9d0b305 to
c72870b
Compare
miga-heygen
left a comment
miga-heygen
left a comment
There was a problem hiding this comment.
Review: font-family top-level comma splitting
Is this the best solution?
Yes. The hand-written scanner is the right call for this specific problem. I considered three alternatives:
-
Regex (e.g.
,(?=(?:[^()]*\([^()]*\))*[^()]*$)) — doesn't handle nesting or quotes reliably. The nestedvar(--x, var(--y, "Inter"))case would need a context-free grammar, not a regular expression. -
postcss-value-parser — a real CSS value parser that handles the full grammar. But it's a new dependency for a single utility, and
parseFontFamilyValueis a hot path (font extraction across all stylesheets in every compile). The scanner is ~30 lines, self-contained, and does exactly what's needed. -
Patch the downstream
var(guard to also match fragments likeinherit)— wrong level of abstraction. The bug is in the parser, not the filter. Fixing the parser is SSOT-correct: every consumer gets the right tokens instead of each one learning to work around broken ones.
The scanner is minimal, correct, and fails closed on malformed input (unterminated quote / unbalanced paren → consumes the rest as one token, which is invalid CSS the browser would also discard, so it skips rather than requesting a bogus family). That's the right failure mode.
What I verified
Scanner correctness. Walked all five state transitions:
| Character | Behavior |
|---|---|
\ |
Skip next char (handles \" in quotes, \, in values) |
' or " outside quotes |
Enter quote mode |
| Matching quote | Exit quote mode |
( outside quotes |
depth += 1 |
) outside quotes |
depth = Math.max(0, depth - 1) (clamp prevents negative on stray )) |
, at depth 0, outside quotes |
Split |
| Everything else | Continue |
Priority is correct: escape check first (so \" doesn't exit quotes), then quote check (so ( inside quotes doesn't bump depth).
Trailing backslash edge case. If \ is the last character: index += 1 → value.length, continue → loop increment → value.length + 1, condition fails, loop exits. value.slice(start) captures the trailing backslash in the final piece. Correct — malformed CSS, fails closed.
Post-processing chain. The .trim().replace(/^['"]/, "").replace(/['"]$/, "").trim().filter(nonEmpty) is unchanged. Verified it doesn't corrupt var() tokens — outer quotes aren't present on a var(--x, "Inter") token, so nothing is stripped. The var( guard at line 462 correctly skips the whole expression.
Downstream consumer: resolveFontFamilyDeclarationFamilies. With the fix, families[0] for var(--brand-font, inherit), sans-serif is the complete var(--brand-font, inherit) token. primaryCssVariableName correctly extracts --brand-font via its own paren-aware walk. families.slice(1) holds only the external fallbacks (["sans-serif"]), not the var's internal fallback. Shape is correct.
Downstream consumer: extractRequestedFontFamilies. The normalized.startsWith("var(") guard at line 462 correctly skips the complete var token. No fragments like inherit) survive to the font-request map. Fail-closed mode no longer aborts.
Test coverage
- Parser unit tests (3 new):
var()fallback, nestedvar()fallback, quoted comma — all verify the split produces the expected token list. - Fail-closed integration test (1 new):
var(--brand-font, inherit)in a full HTML document throughinjectDeterministicFontFaceswithfailClosedFontFetch: true— confirms noFontFetchError. - Distributed fixture (
css-var-fonts): changed fromvar(--display-font)tovar(--display-font, "Montserrat"). Render-neutral because--display-fontis defined as"Montserrat"in:root— computed value and embedded faces are identical, existing baseline stays valid. Good coverage addition.
CI note
Only the WIP check has run so far — full CI hasn't triggered yet. The distributed regression lane (css-var-fonts baseline) is the key check to watch.
No blocking concerns. Clean, correct fix at the right level of abstraction.
— Miga
3 participants
What
font-familyvalues on top-level commas only, so avar()expression stays a single tokenvar()fallback, and the distributedcss-var-fontsfixture now exercises the fallback formCloses #3066
Why
parseFontFamilyValue()inpackages/producer/src/services/deterministicFonts.tssplit the family stack on every comma with no parenthesis awareness, so:parsed to two tokens —
var(--brand-fontandinherit).The guard added in #1655 (for #1654) skips only tokens that start with
var(, so the orphan fragmentinherit)survived as a "requested font family". It matches no bundled alias, no Google Fonts family and no system font, so fail-closed distributed renders abort before the first frame:Any fallback argument is affected, not just CSS-wide keywords —
var(--brand-font, "Inter")leaksInter")the same way. #1655 fixed the barevar(--x)form and its regression fixture uses no fallback argument, which is why this shape survived.How
The splitter now walks the value and only cuts at commas seen at paren depth 0, which keeps
var(--x, fallback)— and nestedvar(--x, var(--y, "Inter"))— as one token, so the existingvar(guard andresolveFontFamilyDeclarationFamilies()both see the whole expression. No special-casing ofinheritor any other keyword.Quotes are tracked in the same pass for two reasons: a parenthesis inside a quoted family name would otherwise skew the depth counter, and a legal quoted comma (
"Display, Condensed") no longer splits into two junk names. Trimming, surrounding-quote stripping and empty-entry dropping are unchanged.Adversarial notes, in case they matter to a reviewer:
resolveFontFamilyDeclarationFamilies()is unaffected in shape —families[0]is now the completevar()expression it always meant to be, andfamilies.slice(1)correctly holds only the external fallbacks, not the ones inside the parens.)is harmless: depth is clamped at 0.(, unterminated quote) consumes the rest of the value as one token. That is invalid CSS which the browser would also discard, and it fails closed toward "skip", not toward "request a bogus family". Left as-is rather than growing the change.var()primary is undefined, a concrete font in its fallback argument still isn't pre-embedded — it is skipped like any othervar(). That is fix(compiler): skip CSS var() in font resolver #1655's behaviour, unchanged, and strictly better than today's hard failure.The distributed fixture change (
var(--display-font)→var(--display-font, "Montserrat")) is deliberately render-neutral:--display-fontis defined as"Montserrat"in:root, so the computed value and the embedded faces are identical and the existing baseline mp4 stays valid. Reverting the source fix makes that fixture fail resolution again.Test plan
planValidation.test.ts(var() fallback, nested var() fallback, quoted comma) anddoes NOT throw when font-family uses a CSS var() reference with a fallbackindeterministicFonts-failClosed.test.tsbun testover the touched files: 49 pass / 0 failbun testover everydeterministicFonts*+planValidationtest file: 84 pass / 0 failoxfmt --checkandoxlintclean on the touched filescss-var-fontsbaseline) — not run locally, no render infrastructure; relying on CI