Skip to content

fix(producer): preserve inner root element during sub-composition inlining#1342

Open
miguel-heygen wants to merge 15 commits into
mainfrom
fix/producer-inner-root-flatten
Open

fix(producer): preserve inner root element during sub-composition inlining#1342
miguel-heygen wants to merge 15 commits into
mainfrom
fix/producer-inner-root-flatten

Conversation

@miguel-heygen

Copy link
Copy Markdown
Collaborator

Summary

The producer's inlineSubCompositions call did not pass flattenInnerRoot, so when a sub-composition had a wrapper element with classes (e.g. .scene_1-root), the producer path used innerHTML which stripped the wrapper — losing the class. The scoped CSS still targeted .scene_1-root, so nothing matched and styles/GSAP/JS all broke.

The bundler path already passed prepareFlattenedInnerRoot, which is why hyperframes preview worked correctly but hyperframes render (producer) didn't.

Fix

Pass prepareFlattenedInnerRoot to the producer's inlineSubCompositions call, aligning it with the bundler path. The inner root element is now preserved as a child of the host with its classes intact.

Test plan

  • Added regression test: flattenInnerRoot preserves .scene_N-root class so scoped CSS resolves
  • Added characterization test: without flattenInnerRoot, the class is lost (documents the pre-fix behavior)
  • All 11 inlineSubCompositions tests pass

Reported by James in the team channel.

…ining

The producer's inlineSubCompositions call did not pass flattenInnerRoot,
so it used innerHTML which strips the wrapper element (and its classes
like .scene_N-root). The bundler path already passed it, which is why
preview worked but renders broke.

Pass prepareFlattenedInnerRoot to align producer with bundler, preserving
inner root classes so scoped CSS selectors resolve correctly.
…ass preservation

Adds a minimal composition with a .scene_1-root class wrapper that
exercises the flattenInnerRoot path. Without the fix, the class would be
stripped and CSS selectors would not resolve, rendering a black frame.
…mpositions/ dir

Restructure to match existing sub-comp-id-selector pattern: sub-comp
HTML lives directly in src/, not in a compositions/ subdirectory.

@vanceingalls vanceingalls left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review

The fix is correct for the stated bug — the inner root element's class is now preserved during producer inlining. The diagnosis and bundler-vs-producer alignment story are accurate. But there's a blocker-level regression introduced by keeping compoundAuthoredRoot: true alongside the new flattenInnerRoot.


BLOCKER: compoundAuthoredRoot: true breaks #id-based CSS selectors when id != compId

The old producer path (no flattenInnerRoot) relied on innerHTML-based inlining, which collapses the inner root onto the host. With that path:

  • inlineSubCompositions propagated data-hf-authored-id = authoredRootId directly onto the host
  • compoundAuthoredRoot: true generated [data-composition-id="scene_1"][data-hf-authored-id="root"] — a compound selector targeting the host, which had both attributes ✓

After this fix:

  • prepareFlattenedInnerRoot places data-hf-authored-id = "root" on the child inner root element, and does NOT set it on the host
  • The producer's post-inlining loop sets data-hf-authored-id on the host only if not already present, and uses compId as the value — so the host ends up with data-hf-authored-id = "scene_1", not "root"
  • CSS scoping (run during inlineSubCompositions itself, before the post-loop) used authoredRootId = "root" and compoundAuthoredRoot: true to emit [data-composition-id="scene_1"][data-hf-authored-id="root"]
  • At runtime that selector matches nothing: the host has the wrong authored-id value, and the child has no data-composition-id

This only fires when id != compId on the inner root element (e.g. id="root" with data-composition-id="scene_1"). But that's exactly the fixture shape in the new test ("root" vs "scene_1") — the test just doesn't assert on CSS selector resolution for #root-style selectors.

The new tests are silent on this because they only check DOM structure (class preserved), not scoped CSS output for #id selectors.

Fix options:

  1. Drop compoundAuthoredRoot: true from the producer call. With flattenInnerRoot, the inner root is a child of the host — not merged onto it — so descendant selectors are correct and compound is no longer needed.
  2. If compoundAuthoredRoot must be retained for some other reason, the post-loop needs to set data-hf-authored-id from authoredRootId (not compId) on the host, so the compound selector has something to match.

IMPORTANT: The new tests use a stub that doesn't exercise prepareFlattenedInnerRoot

The new regression tests define their own inline flattenInnerRoot stub that omits timing-attr stripping, width/height style injection, and data-composition-id removal — all of which prepareFlattenedInnerRoot does. Tests that pass against a simplified stub can miss interactions that only appear with the production function. Recommend switching the new tests to call prepareFlattenedInnerRoot directly (it's already exported from ./htmlBundler).


NIT: Stale comment in producer wrapper

The comment on the post-loop in htmlCompiler.ts says "Unlike flattenInnerRoot (which changes DOM structure and breaks baselines), this preserves the existing innerHTML-based inlining..." — directly contradicted by the new line above it. Should be updated to reflect that the loop exists to align data-hf-authored-id on the host for the CSS scoping contract.


What's solid

  • Root cause diagnosis is correct and the 1-line fix is the right direction.
  • The characterization test (pre-fix behavior) is a good addition.
  • The type cast (prepareFlattenedInnerRoot as (el: Element) => Element) is the standard linkedom bridge pattern used throughout the file — not a concern.
  • CI is still running; regression-shards (shard-7, sub-composition-video ...) would catch this if any existing test composition has id != compId on its inner root.

— Vai

…repareFlattenedInnerRoot in tests

- Drop compoundAuthoredRoot: true since flattenInnerRoot preserves the
  inner root as a child (descendant selectors are correct, compound
  selectors would target the wrong element when id != compId)
- Replace inline flattenInnerRoot stubs with the real
  prepareFlattenedInnerRoot in tests
- Update stale comment on the post-loop

@vanceingalls vanceingalls left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All three R1 blockers are resolved.

  1. compoundAuthoredRoot: true dropped — replaced with flattenInnerRoot: prepareFlattenedInnerRoot. Descendant selectors in the CSS scoping now work correctly because the inner root is a proper child of the host. The compound-selector breakage path is gone.

  2. Inline stub removed — the hand-rolled flattenInnerRoot approximation in the test is deleted; the test now imports and calls the real prepareFlattenedInnerRoot from htmlBundler. Regression test exercises the exact code path that was broken in production.

  3. Stale comment updated — the misleading "Unlike flattenInnerRoot (which changes DOM structure and breaks baselines)..." comment is gone, replaced with a clean one-liner.

The end-to-end fixture (src/scene_1.html → compiled.html) demonstrates the fix visually. The characterization test that documents the pre-fix behavior is a nice touch.

— Vai

…pId combination

Three new tests document the interaction between compoundAuthoredRoot
and flattenInnerRoot when authoredRootId differs from compId:
1. Without flattenInnerRoot: compound selector works (both attrs on host)
2. With flattenInnerRoot: descendant selectors work (attrs on separate elements)
3. With both: compound selector matches nothing (documents the bug)
…lay-montage-prod

The flattenInnerRoot change alters DOM structure (inner root preserved as
child), causing compilation validation to flag descendant composition id
mismatches. Regenerated baselines inside Docker.
… avoid runtime-inline dependency

The test import of prepareFlattenedInnerRoot from htmlBundler pulled in
the generated/runtime-inline module which doesn't exist in CI test runs
(only after build). Moving the function to inlineSubCompositions breaks
the transitive dependency chain while keeping the re-export from
htmlBundler for backwards compatibility.
…flattenInnerRoot

When the host element lacks data-composition-id and flattenInnerRoot is
used, prepareFlattenedInnerRoot strips the attribute from the inner root.
Scoped CSS targeting [data-composition-id="X"] then has nothing to match,
causing styles to disappear.

Fix: when flattenInnerRoot is active and compId is null (host doesn't have
it), propagate the inferred composition ID from the inner root to the host
element. Regenerated baselines for missing-host-comp-id and
overlay-montage-prod inside Docker.
The previous fix (propagating comp-id to host) broke flex centering: CSS
like display:flex;align-items:center targets the host, but .label is a
grandchild (host > inner-root > label) so flex centering doesn't cascade.

Better approach: only apply flattenInnerRoot when compId is present on the
host. When compId is null, the old outerHTML path already preserved the
inner root with all its original attributes (including data-composition-id),
so flattenInnerRoot is unnecessary and harmful.

This preserves the original fix for James's bug (inner root classes lost
when host HAS compId and innerHTML strips the wrapper) while avoiding
regressions when the host lacks compId (30 existing regression tests).
…DOM change

style-7-prod hosts have data-composition-id, so flattenInnerRoot applies
and changes the DOM structure. Regenerated baseline inside Docker.
The bundler's prepareFlattenedInnerRoot injects pixel dimensions
(width:Xpx;height:Ypx) on the inner root wrapper, which overrides
the host's flex/grid centering. The producer needs a lighter version
that strips timing/composition attrs and converts id to
data-hf-authored-id, but preserves the original inline styles so CSS
layout (flex centering, absolute positioning) remains intact.

This fixes James's bug (inner root classes like .scene_1-root lost
during producer inlining) without regressing existing compositions
that use flex/grid centering on the host element.
flattenInnerRoot in ANY form (bundler's prepareFlattenedInnerRoot or a
light version without pixel dims) adds an inner root wrapper div that
breaks the host's flex/grid centering: children are no longer direct
flex items. This regressed chat (73 failed frames), style-7-prod (15),
and overlay-montage-prod (14).

The producer now uses the original innerHTML/outerHTML path with no
flattenInnerRoot. James's inner root class preservation bug needs a
different approach (CSS scoping change) tracked separately.

The sub-comp-inner-root-class regression test is kept — it documents
the current behavior and will validate the future CSS scoping fix.
… wrapper

Root cause: when the producer uses innerHTML to strip the inner root,
classes like .scene_1-root are lost. Scoped CSS targeting
[data-composition-id="X"] .scene_1-root has nothing to match.

Fix (two parts):
1. Copy inner root classes to the host element before innerHTML strips it
2. Emit both compound and descendant CSS selectors for inner root classes:
   - [scope].scene_1-root { ... }  (matches host with class)
   - [scope] .scene_1-root { ... } (matches child with class — bundler path)

This preserves CSS layout (no wrapper div that breaks flex/grid) while
making class-based selectors work in both bundler and producer paths.
…scoping

After innerHTML strips the inner root, both data-composition-id and
data-hf-authored-id end up on the host element. Scoped CSS needs
compound selectors ([scope][authored-id]) to match a single element —
descendant selectors ([scope] [authored-id]) fail because there's no
parent-child relationship.

This was already needed before this PR but the baseline was stale.
Regenerated sub-comp-t0 and sub-comp-inner-root-class baselines.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants