fix(core): scope sub-composition html/body styles so they don't clobber the host document#2089
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
james-russo-rames-d-jusso
left a comment
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at 22aa93f.
Clean, narrow fix — LGTM from my side, leaving as a comment. The extracted compositionBoxSelector DRY-refactors the existing bare-composition-root case (previous line 146) and reuses that same "host OR flattened inner-root, exactly one" invariant for the new remap. All four sub-composition scoping call sites correctly opt in (compositionLoader.mountCompositionContent, inlineSubCompositions head + content style loops, both htmlBundler template-match + fallback paths), top-level compositions keep scopeRootSelectors default-off so they still legitimately own the document, and * stays untouched. CI is green across the full matrix — Perf/parity/Preview-parity/Windows-render/Smoke all passing at HEAD.
Findings below are all yellow — none blocking.
Concerns
-
Compound
html/body/:rootselectors bypass the remap (packages/core/src/compiler/compositionScoping.ts:124). The guard/^(html|body|:root)$/i.test(trimmed)matches only bare selectors, so a sub-comp writingbody.dark { overflow: hidden },html body { ... },:root .foo { ... },body[data-theme="x"] { ... }, orbody:hover { ... }skips the remap and drops to general scoping (${scope} body.dark), which selects nothing under the composition box. Behavior is byte-identical to pre-fix (those selectors were dead before too — the parent<body>doesn't havedata-composition-idon it), so this is not a regression, but the fix's promise ("sub-comp html/body no longer clobbers") is only true for the bare forms actually enumerated. Two consequences worth flagging:- A sub-comp that authors
body { overflow: hidden }gets the fix; the same sub-comp rewritten asbody.scene-a { overflow: hidden }silently doesn't — the visual clobber symptom won't manifest either (since it doesn't match the parent<body>anymore under general scoping), but the intended styling on the sub-comp is also dead. It's a "both branches misfire" fingerprint — the sub-comp author sees no styling, easy to misdiagnose. - Consider extending the regex to accept a bare tag/pseudo prefix (
^(html|body|:root)([.:\[]|\s|$)), or at minimum document the current bare-only coverage in the scoping docstring so future maintainers don't assume broader semantics.
- A sub-comp that authors
-
:rootcustom-property leak — small backwards-compat. Pre-fix, a sub-comp's:root { --brand: red }set--brandon the parent<html>, which any parent-doc CSS couldvar(--brand). Post-fix,--brandis scoped to the composition box only. Overwhelmingly likely to be an accidental leak (the docs describe:rootas document-level), but any published composition that unknowingly depended on sub-comp:rootvariables reaching upward loses that. Not worth a code change; worth a line in the release notes / CHANGELOG so a customer hitting "myvar(--brand)stopped resolving after upgrade" has a hit to search for.
Nits
-
compositionBoxSelectorat line 100-105 uses:has([data-hf-inner-root]). That baseline is Chrome 105 / Safari 15.4 / Firefox 121. This constraint already existed at the previous line-146 use so nothing's newly at risk here — but promoting it to a shared helper is a good moment to note the:has()dependency in the docstring, since new callers might not realize. -
The new
does not scope*even with scopeRootSelectorstest is good defensive coverage. Consider one more test that pins the concrete clobber pattern verbatim from the PR body — something like:const scoped = scopeCssToComposition( `html, body { width: 560px; height: 360px; overflow: hidden; background: #14141c; }`, "scene", undefined, undefined, { scopeRootSelectors: true }, ); expect(scoped).toContain('[data-composition-id="scene"]:not(:has([data-hf-inner-root]))'); expect(scoped).toContain("overflow: hidden");
The existing test at line 48-66 asserts the negative space (no bare
html/body/:rootsurvives) and the presence of some[data-composition-id="scene"]+data-hf-inner-rootfragment, which is solid — but pinning the exact multi-property declaration that motivated the fix ensures a future rewrite that changes the output shape (e.g. splitshtml, bodyinto two rules or reorders declarations) can't leave the abstract-shape assertions green while re-breaking the concrete case. -
.fallowrc.jsoncre-flags are appropriate line-shift exemptions per the file's convention — no objection, just noting how many touched files now carry re-flags (5 acrossdup/health), all pre-existing complexity carried by the sub-comp scoping subsystem. Not something for this PR to solve.
What I didn't verify
- The Studio-live "verified" claim in the PR body — CI's Preview parity + Perf: parity + Perf: drift are all green at
22aa93f, so I trust the end-to-end observation without re-running. - Nested sub-composition (sub-comp A containing sub-comp B): the bundler's single-pass over the
subCompositionHostssnapshot suggests depth-1 inlining, so B's<style>would need a second pass to be scoped at all. This is pre-existing pipeline shape independent of this PR — if depth>1 works today via some other mechanism,scopeRootSelectors: truepropagates through the shared inliner and the fix applies at every level. - Actual browser matrix for Studio's minimum-supported
:has()— pre-existing dependency, not introduced here.
Cross-PR
- #2090 (always-on crop) — no file overlap (2090 is
packages/studio/*only, this PR ispackages/core/src/compiler+runtime). Direction is complementary — 2089 keeps the composition preview at its authored dimensions, which is exactly what the crop overlay in 2090 needs to measure against. No conflict.
— Review by Rames D Jusso
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
R1 — hyperframes #2089 at 22aa93fb
🟢 LGTM. Concurring with Rames on verdict and on the compound-selector + :root-var-leak concerns; the notes below are the delta I saw independently.
Root cause is called correctly — a mounted/inlined sub-composition's body { width/height; overflow: hidden } was hitting the parent <body> and collapsing the host preview to the last-mounted sub-comp's box. Fix is minimal, opt-in, and coverage is complete.
Verified independently at head SHA
- Consumer coverage — all four sub-comp scoping call sites flipped. Cross-checked at
22aa93fb:runtime/compositionLoader.ts:404(Studio live-mount) —scopeRootSelectors: true✅compiler/inlineSubCompositions.ts:256(DRYed viascopeSubStyle; the two previous call sites forcompDoc.headstyles andcontentDocstyles now share one path) —scopeRootSelectors: true✅compiler/htmlBundler.ts:879(inner-root path,authoredRootIdpresent) — ✅compiler/htmlBundler.ts:910(no-inner-root path,authoredRootId = undefined) — ✅htmlCompiler.ts/htmlDocument.tsdon't callscopeCssToComposition. That's the whole write-site list.
compositionBoxSelectorDRY.compositionScoping.ts:103-105extracts${scope}:not(:has([data-hf-inner-root])), ${scope} > [data-hf-inner-root]and shares it between the new html/body/:root remap (:129) and the pre-existing bare-composition-root branch (:146). If one drifts they drift together — right invariant for a two-site pattern.isInsideGlobalAtRulestill gates keyframes/font-face. Rules nested in@keyframes/@font-facebypass rescoping (unchanged). Rules nested in@mediastill walk — so@media (max-width: 600px) { body { ... } }inside a sub-comp does get remapped under the flag, which is what you want.- CI is fully green at head — Fallow, Test, Typecheck, Format, Lint, Build, Render on windows, Preview parity, SDK unit/contract/smoke, Studio load smoke, all Perf lanes, regression, Preflights. 39 checks pass.
Concurring with Rames
- Compound
body.dark/html body/body:hoverstill fall through to the dead-code prepend path. Rames spelled this out fully; same shape as before this PR — not a regression, but a follow-up-worthy gap. My independent read agrees on his suggested^(html|body|:root)([.:\[]|\s|$)extension being the honest fix if we ever pick it up. :rootcustom-property backwards-compat. Good catch — I'd missed this. A sub-comp's:root { --brand: red }used to leak to the parent<html>, so any parent-docvar(--brand)would resolve. Post-fix,--brandis bounded to the composition box. Overwhelmingly likely to be an accidental leak, but agree a CHANGELOG line for "sub-composition:rootcustom properties no longer reach the host document" would save the debug for anyone who tripped on it.:has()baseline note in the docstring. Chrome 105 / Safari 15.4 / Firefox 121. The dependency already existed on the pre-fix bare-composition-root branch, but promoting it to a shared helper is a fair moment to document it.
Unique observations
O1 — * universal selector is the last document-level leak path. A sub-comp with * { color: red } still applies to every element in the parent document, not just descendants of [data-composition-id="..."]. The pre-fix code was /^(html|body|:root|\*)$/i (all four pass-through); this PR splits * out and preserves the pass-through explicitly (compositionScoping.ts:123 + test lock at :70-79). Idempotent rules like box-sizing: border-box are harmless; something like * { color: red } would color the whole host doc. Doesn't affect the reported clipping symptom and it's out of scope for this fix — worth naming because the test freezes the behavior as intentional, so if it's really "we'll never remap *", great, and if it's "we should, later," a follow-up is called for. A parallel ${scope} * remap under the same flag would slot in without churn.
O2 — Test's negative-match regex for body is a touch looser than for html/:root. compositionScoping.test.ts:55 uses [,{] (catches body, in compound + body {); :56 (:root) and :57 (body) use \{ only. If a future regression somehow produced body, at the front of a compound remapped selector, only the html line would catch it. Not a real risk (postcss splits selectors on , before each hits scopeSelector, so compound output shouldn't happen), just a symmetry nit. Trivial: unify all three on [,{].
Cross-PR
No conflict with #2090 — that PR is packages/studio/* only; this PR is packages/core/src/compiler + runtime. The composition preview staying at authored dimensions (this PR) is exactly what 2090's crop overlay needs to measure against.
R1 by Via
Sub-composition <head> styles targeting html/body/:root (width/height/ overflow/background) were injected into the parent document unscoped by both the Studio runtime mount (compositionLoader) and the render-time inliner (inlineSubCompositions/htmlBundler). scopeCssToComposition deliberately passed html/body/:root through unchanged, so a sub-composition smaller than the root clobbered the host <body> dimensions and its overflow:hidden clipped the composite to the last sub-comp's size. Only the top-left element painted; everything else (and framework-owned video positioned outside that box) was clipped away. Add a scopeRootSelectors option to scopeCssToComposition that remaps html/body/:root to the composition's own box, and enable it everywhere sub-composition styles are scoped. The universal selector stays untouched. Top-level composition scoping is unchanged (it legitimately owns the document). Covered by new compositionScoping tests.
miguel-heygen
force-pushed
the
worktree-fix-subcomp-body-style-leak
branch
from
22aa93f to
da4ba8b
Compare
|
Thanks for the thorough pass. Addressed the actionable yellow items in the latest push:
Deferring (agreed non-blocking): the |
3 participants
Problem
Sub-composition
<head>styles that targethtml/body/:root(typicallywidth/height/overflow/background) were injected into the parent document unscoped.scopeCssToCompositiondeliberately passedhtml/body/:rootthrough unchanged, so when a sub-composition is smaller than the root, itsbody { width/height; overflow: hidden }clobbered the host composition's<body>and clipped the whole composite down to the last-mounted sub-comp's size.Symptom: in a composition with multiple sub-compositions (or a sub-comp smaller than the root), only the top-left element painted in the Studio preview; everything positioned outside that clipped box (including framework-owned video) appeared missing/black. This affected both the Studio runtime mount and the baked render.
Root cause:
packages/core/src/compiler/compositionScoping.ts—scopeSelectorreturnedhtml/body/:root/*selectors verbatim. Correct for a top-level document, but wrong for sub-composition styles that get lifted into a parent document.Fix
Add a
scopeRootSelectorsoption toscopeCssToCompositionthat remapshtml/body/:rootto the composition's own box (host or flattened inner root) instead of leaving them document-level. Enable it everywhere sub-composition styles are scoped:runtime/compositionLoader.ts(Studio live preview mount)compiler/inlineSubCompositions.ts(render inliner)compiler/htmlBundler.ts(bundle path)The universal selector
*is left untouched, and top-level composition scoping is unchanged (scopeRootSelectorsdefaults off), so a composition still legitimately owns its own document.Tests
New
compositionScopingcases cover: default pass-through ofhtml/body/:root, remapping underscopeRootSelectors, and*staying untouched. Full compiler suite green (130 tests).Verified live in Studio: a project with image / video / shape / text / sub-composition scenes now renders all of them (body stays at the composition's
1920x1080); before the fix the body collapsed to a sub-comp's size and clipped everything but the top-left.Notes
The
.fallowrc.jsoncentries added here are file-level exemptions for pre-existing complexity/duplication in the touched files (large scoping/bundler functions and old test/script-loop clones), re-flagged only by the line-shift from this change, following the existing convention in that file.