fix(core): set explicit dimensions on data-hf-inner-root wrapper#1050
Conversation
jrusso1020
left a comment
There was a problem hiding this comment.
Verdict: COMMENT (would-be APPROVE)
Minimal fix at the right scope. Root-cause analysis matches the bug shape, fix applied to both compiler + runtime paths (parity ✓ on the primary case), regression fixture pins prior behavior. One parity gap worth flagging before merge. Holding for James's stamp greenlight.
Regression source traced
prepareFlattenedInnerRoot + the data-hf-inner-root wrapper attribute were introduced in commit 581e7a7e ("refactor: extract shared inlineSubCompositions from bundler and producer"). Before that, sub-composition flattening didn't insert this wrapper, so authored CSS using height: 100% resolved directly against the composition host's explicit pixel height. The wrapper broke the percentage-resolution chain — height: 100% resolved against a dimensionless wrapper → 0px → descendants collapse → flex centering renders at top edge.
That maps cleanly to Miguel's "regression for old templates" framing: any template authored before 581e7a7e (or after, but using the percentage-height + flex-centering pattern) was silently broken. Setting explicit pixel dimensions on the wrapper restores the chain.
Parity gap — compiler vs runtime fallback diverges
The two paths now have different fallback behavior when data-width / data-height are absent:
Compiler (htmlBundler.ts:402-406):
const widthVal = w ? `${w}px` : "100%";
const heightVal = h ? `${h}px` : "100%";
prepared.setAttribute("style", existingStyle ? `${existingStyle};${fill}` : fill);Falls back to 100% when attributes are missing.
Runtime (compositionLoader.ts:88-89):
if (w) prepared.style.width = `${w}px`;
if (h) prepared.style.height = `${h}px`;Sets nothing when attributes are missing.
For templates with data-width / data-height present (the common case + the regression fixture), both paths produce identical output. ✓
For templates without those attributes on the inner root, the paths diverge:
- Compiler emits
<div data-hf-inner-root="true" style="width:100%;height:100%"> - Runtime emits
<div data-hf-inner-root="true">(no style)
height: 100% is the fix that addresses the regression. The runtime path, without that fallback, still has the regression for any old template missing data-width/data-height on the inner root — exactly the population the PR title targets. This is the "preview-vs-render parity bug" shape that's hit hyperframes repeatedly (hf#965, #978, #981, #986, etc.) — the runtime path skips a defensive case the compiler path covers.
Suggested fix (one line in compositionLoader.ts):
prepared.style.width = w ? `${w}px` : "100%";
prepared.style.height = h ? `${h}px` : "100%";Or, if the deliberate intent is "trust that data-width/data-height are always present" (per CLAUDE.md framework contract), drop the conditional on both sides and assert presence — the patch would simplify to:
prepared.style.width = `${prepared.getAttribute("data-width")}px`;
prepared.style.height = `${prepared.getAttribute("data-height")}px`;…and the lint pass would catch missing attributes upstream.
Either resolution works; the current shape (compiler defensive, runtime not) is the worst-of-both.
Test coverage observation
The new sub-comp-height-percent fixture pins the prior-behavior on the with-attributes case (the regression's primary repro). It doesn't exercise the divergent fallback path I just flagged — a follow-up fixture with data-width/data-height omitted from the inner root would catch the runtime-vs-compiler divergence in CI. Optional; not a blocker since the framework contract says those attributes should always be present.
Sanity checks (no concerns)
- Style attribute append shape:
${existingStyle};${fill}correctly handles empty / whitespaceexistingStylevia the.trim()+ ternary. No leading-semicolon bug. ✓ - CI: 30 success / 0 in_progress / 0 failures.
- Parity scope: only two consumers of
prepareFlattenedInnerRootexist (htmlBundler.ts+compositionLoader.ts— the bundler/runtime pair). No third path missed. The studio iframe DOM patch path (patchIframeDomTimingand similar fromreference_preview_render_parity_check.md) doesn't reconstruct the inner root, so it's not affected. ✓ - Regression-source attribution: tracing back through commits,
581e7a7eintroduced the wrapper without dimensions; this PR repairs it. Net effect on the bug: closed.
Summary
Fix is the right shape and the right scope. One ask before merge: align the compiler's "100%" fallback into the runtime path (or drop the fallback on both sides). The current divergence leaves the runtime path with the same regression for the old-template population the PR title targets.
— Rames Jusso
vanceingalls
left a comment
There was a problem hiding this comment.
Correct fix, clean execution. Root cause and scope are both well understood.
Strengths
packages/core/src/compiler/htmlBundler.ts:401-408— the append-not-replace style logic (existingStyle ? ${existingStyle};${fill} : fill) is exactly right; it preserves any authored inline style on the inner-root element while injecting the missing dimensions.- Producer regression test with a visual assertion closes the loop on the bug scenario.
minPsnr: 20gives a meaningful floor without being brittle. - PR description includes a precise CSS causality chain (percentage height resolving against a 0px parent) — that's the level of detail that makes future archaeology much easier.
Important
htmlBundler.ts and compositionLoader.ts diverge on the no-dimensions fallback:
// htmlBundler.ts (compiler path)
const widthVal = w ? `${w}px` : "100%"; // always sets style
const heightVal = h ? `${h}px` : "100%";
// compositionLoader.ts (runtime path)
if (w) prepared.style.width = `${w}px`; // only sets when present; silent no-op otherwise
if (h) prepared.style.height = `${h}px`;
Both functions are named prepareFlattenedInnerRoot and are meant to produce equivalent DOM. Every real sub-composition fixture I checked carries explicit data-width/data-height, so this divergence doesn't affect the regression being fixed — but the two paths now have different contracts for a hypothetical dimensionless inner root. Either document why the runtime path is intentionally a no-op when dimensions are absent, or unify the fallback to match.
Nit
The regression test exercises the fix end-to-end (full render pipeline, visual diff), which is high-confidence but heavyweight. The transform itself — "inner root with data-width/data-height gets matching inline dimensions; without them, bundler falls back to 100%" — is a pure DOM mutation that could be pinned with a 3-line unit test in the existing htmlBundler test suite. Wouldn't replace the producer test, but would give faster signal on future regressions at this site.
CI: all required checks pass, including regression-shards (8 shards) and perf suite.
Verdict: APPROVE
Reasoning: The fix is correct for the full affected population (all real sub-compositions carry explicit dimensions), the regression test catches the failure case, and CI is fully green. The fallback asymmetry is important enough to track but doesn't gate this fix.
— Vai
The prepareFlattenedInnerRoot function creates a wrapper div when inlining sub-compositions. This wrapper had no width/height, which broke CSS height:100% chains — any sub-composition using percentage heights with flexbox centering would collapse to 0px and render content at the top instead of centered. Read data-width/data-height from the inner root and set matching pixel dimensions on the wrapper's inline style. Applied in both the compiler (server-side bundling) and the runtime (browser-side composition loader). Adds a producer regression test with a centered card sub-composition that fails without this fix.
23cd385 to
3983d4e
Compare
Summary
height: 100%+ flexbox centering rendered content stuck at the top instead of centeredprepareFlattenedInnerRootcreates a<div data-hf-inner-root>wrapper that had no explicit dimensions, breaking the CSS percentage height chaindata-width/data-heightfrom the inner root and set matching pixel dimensions on the wrapper's inline stylehtmlBundler.ts) and runtime (compositionLoader.ts)Reproduction
Any sub-composition with this pattern was affected:
The
.wrapper'sheight: 100%resolved against the dimensionlessdata-hf-inner-rootdiv (→ 0px), collapsing all descendant heights.Test plan
sub-comp-height-percent— centered card sub-composition