fix(runtime): keep stamped flow children in document flow in preview#1702
Conversation
xiayewang-heygen
left a comment
There was a problem hiding this comment.
Approve. The root cause framing is right — position: relative elements resolve computed top/left to "0px" (not "auto"), so the pre-existing explicit-anchor heuristic conflated them with deliberately-anchored clips and stripped their layout position when promoting to absolute. The fix is a clean two-pass:
- Pass 1 preserves the legacy-anchored-defaults normalization (no mutation of flow).
- Pass 2 freezes the in-flow geometry of every promotable clip before mutating any of them — the comment block explains why correctly: promoting clip A shifts where in-flow sibling B sits, so measurements can't interleave with mutations.
Subtle correctness checks that all hold:
(!shouldForceAbsolute && (computed.top !== "auto" || …))is the actual surgical change — only trust the computed anchor when we're NOT promoting. Otherwise use the frozen offset. ✓offset*is transform-independent, so a frame-0 GSAP entrance transform on the clip doesn't pollute the measurement. ✓offsetParent !== rootElfilter correctly skips nested clips → they fall back totop:0/left:0(preserves prior behavior for non-root cases). ✓- jsdom / hidden zero-box elements skipped via the
width <= 0 || height <= 0guard → also fall back. ✓ - Frozen widths/heights propagated so flex-driven layouts (
justify-content: space-betweenetc.) survive the promotion. ✓
Cross-check vs the distributed-renderer compatibility class: this lands inside packages/core/src/runtime/init.ts — the common runtime — so the fix applies identically to the single-container preview and FrameCapture sidecar paths. No EF-side gap reopened (those are typography / window.__hf exposure, orthogonal).
Two non-blocking observations:
- Test scope. The new test pins the right contract (header + footer in flex column stay anchored after promotion) but doesn't explicitly exercise the
offsetParent !== rootElearly-continue or the zero-box fallback. The implicit coverage is acceptable for a layout fix this surgical; an explicit case would be nice if there's a follow-up. - Two passes call
getComputedStyle. Necessary because measurements must precede mutations — can't be collapsed without breaking the freeze-before-promote invariant. Negligible perf cost given typical root-clip counts.
No inline reviewer comments at read; CI green except for regression-shards still running (the normal HF cadence, not a blocker for a layout-runtime fix this targeted).
— WXai
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Layered second-pass on Xiaye's approve. I agree with the root-cause framing and the fix shape — position: relative resolving computed.top/left to "0px" (not "auto") is the actual mechanism; the (!shouldForceAbsolute && (computed.top !== "auto" || ...)) change at packages/core/src/runtime/init.ts:382-384, 389-391 is the minimal surgical fix and the pre-mutation measurement pass is correctly ordered. SHA reviewed 0131fe0b.
A few cross-cutting concerns not load-bearing enough to block, but worth surfacing:
Concerns
• packages/core/src/runtime/init.ts:2296-2298 — applyClipLayout runs on the transport tick every 20 RAF frames (~333ms @60fps), unconditional on isPlaying(). The new pre-pass adds 5 forced layout reads per clip per call (getComputedStyle().position, offsetParent, offsetWidth, offsetHeight, offsetTop, offsetLeft) — and offset* flushes pending style mutations. Old applyClipLayout was style-only (zero offset* reads). For a typical Vision Board comp with ~3-5 root clips this is negligible; for compositions with many root-level clips it's a recurring forced-reflow every ~333ms while idle. Worth eyeballing the Player perf job's Perf: load / Perf: parity deltas vs base, and worth considering: can the freeze happen once on init / signature-change instead of every call? Possible cheap mitigation: skip the pre-pass when clipTreeSignature hasn't changed AND every clip's style.position is already "absolute" (steady state after first call).
• packages/core/src/runtime/init.ts:60-61 — the file's own preamble says this runtime runs in both preview and render contexts, but the PR body asserts "final render is correct." If both paths execute applyClipLayout, why does render escape the bug? Two non-mutually-exclusive hypotheses: (a) the render-side host injects a stage wrapper whose flow doesn't trigger the relative-anchor heuristic, or (b) render-time frame capture happens AFTER postTimeline has run enough times that authored stylesheets re-stamp position — but neither is verified. If render IS structurally immune for a reason that's load-bearing here, a one-line code comment in applyClipLayout documenting it would help the next reader who has to re-derive this.
• Test scope — overlaps Xiaye's note. The new test covers the happy path (two relative clips, both anchored to frozen offsets). Not covered: (a) the el.offsetParent !== rootEl early-continue (nested or fixed-positioned clips); (b) the width <= 0 || height <= 0 zero-box fallback path (the headline jsdom-safety claim from the PR body); (c) a clip authored with position: absolute; top: 50px keeping its authored offset (regression coverage on the unchanged path). The fix is well-scoped enough that this is a "would be nicer if" rather than a blocker — but the jsdom-fallback claim being load-bearing for every other init.test.ts test passing makes (b) the highest-value follow-up.
Questions
• packages/core/src/runtime/init.ts:357 — position === "absolute" || === "fixed" skip-list explicitly excludes position: sticky. A sticky clip resolves computed top/left similarly to relative (the spec sticky offset cap is per-spec but still resolves to a px value, not "auto"), so under the new code a sticky root-level clip would get measured + anchored as absolute — silently stripping its sticky behavior. git grep position.*sticky across the HF packages shows zero sticky users today, so this is theoretical. Intentional defer, or a case worth narrowing to "anything not relative/static" being the criterion instead?
Nits
• packages/core/src/runtime/init.ts:366 — const tag = el.tagName.toLowerCase() is recomputed in the second loop but only consumed for the tag !== "audio" branch at line 395. The filter at line 289-293 already lowercases tag once per clip; could be hoisted into a struct or skipped where unused, but inlining clarity wins over micro-opt here.
Coverage observation (no action implied)
The preview-regression CI job passed, which is real evidence the change is non-regressive for existing fixtures. The symptom Jerry reported (Vision Board iframe preview header/footer collapse) is repro'd in the unit test logically but not pixel-wise in any fixture under preview-regression — if there's bandwidth post-merge, adding a flex-column flow composition to that suite would convert this whole class of bug from "live customer reports it" to "preview-regression CI catches it." Not a blocker for this PR.
Cross-stack scan
• applyClipLayout is module-private to packages/core/src/runtime/init.ts (no other consumer). Callers within the file are postTimeline only — which is itself called from 6 sites including the rAF transport tick (init.ts:2297), the metadata-rebind flow (init.ts:1399, 1426), and post-render-ready publish (init.ts:2096, 2633). No external repo imports this symbol; no Pacific / EF / lambda consumer touches it.
• Last touch to init.ts on main was Miguel's #1662 (1 day ago, descendants-of-inactive-render hide). No coherence concern with that change — the new code lives in applyClipLayout which #1662 didn't touch.
Stamp posture
Net 🟡 — comment-only. The fix is correct and Xiaye's approve already gives Miguel forward motion; my concerns are cross-cutting (perf telemetry, render-path symmetry doc, test coverage of fallback path) and shouldn't gate. If preview-perf job deltas look clean, this is a clean land.
— Rames D Jusso
vanceingalls
left a comment
There was a problem hiding this comment.
Targeted runtime fix in applyClipLayout: when the legacy explicit-anchor heuristic sees a position: relative root-level clip — whose computed top/left resolve to "0px" rather than "auto" — it used to confuse it with a deliberately CSS-anchored clip, promote it to absolute without an anchor, and let it collapse onto the root's top-left. This PR freezes the in-flow offset* box of every promotable clip in a pre-pass (before any mutation, so promoting clip A doesn't shift sibling B's measurement), then pins top/left (and width/height if no data-* override) to that frozen box. Surgical, two-pass, transform-independent — matches Jerry's Vision Board preview repro (relative <header>/<footer> in a flex column) exactly.
Verdict: LGTM
Findings (numbered, with severity tag):
- nit — Test coverage is implicit on two of the new branches:
packages/core/src/runtime/init.ts:359(theoffsetParent !== rootElearly-continue for nested clips) andpackages/core/src/runtime/init.ts:362(thewidth <= 0 || height <= 0zero-box fallback for jsdom / hidden elements). The single new test (header + footer happy path) exercises the working flow; the fallback paths are covered by the existing 556 tests passing unchanged, but an explicit "nested clip falls back to top:0/left:0" case would tighten the contract. Author-discretion follow-up — not blocking.
Verified:
- Freeze-before-mutate invariant.
packages/core/src/runtime/init.ts:356-364measures every promotable clip'soffsetTop/offsetLeft/offsetWidth/offsetHeightbefore the apply loop at:366runs anyel.style.position = "absolute"write. Promoting sibling A can no longer pollute sibling B's frozen box. - Transform-independence.
offset*metrics are spec'd transform-independent, so a GSAP entrance transform present at frame 0 (cross-PR family with #1692immediateRender:true) doesn't pollute the freeze. The inline comment at:347-350calls this out correctly. - Surgical anchor-heuristic change. The
!shouldForceAbsolute && (...)gate at:384and:391is the load-bearing edit: only trust the pre-promotion computedtop/leftwhen we are NOT promoting. For promoted clips, fall through to the frozen offset. Preserves the prior behavior for clips that were alreadyabsolute/fixed(deliberately CSS-anchored —shouldForceAbsolute=falsekeeps the computed-anchor short-circuit). - Inline-style short-circuit preserved.
Boolean(el.style.top) || Boolean(el.style.bottom)at:382-383still wins over the frozen value, so an author setting inlinestyle.topdirectly is untouched (existing contract). - Width/height pin gated correctly.
:404-408else if (frozen && !el.style.width)only pins when there's no author-inline width AND nodata-widthoverride.data-widthstill wins viaforcedWidthat:400-403. The full-width footer withjustify-content: space-betweenMiguel cites is covered by thefrozen.widthfallback. - Idempotency on re-runs.
applyClipLayoutruns frompostTimeline(:1678) which fires on init, sub-comp load/unload (:1399,:1426,:2096,:2292,:2297,:2633). On a second run, promoted clips'positionisabsolute→ skipped in the freeze loop (frozen map empty for them);Boolean(el.style.top)is now truthy from the prior run → anchor preserved. No drift. - No defensive try/catch (Pattern #4): the freeze and apply loops are bare — DOM exceptions would propagate, which is the right call for a layout-critical pass.
- Single promotion path.
grep 'position = "absolute"'returns one hit at:375. No Pattern #3 silent scope gap. - No duplicate anchor logic (Pattern #1): only one
offsetTop/offsetLeftwrite site in the file. - Title matches diff (Pattern #6): "anchor in-flow clips when forcing them absolute" — diff exactly does this; no silent additions to display/right/bottom.
- Customer-impact reach. Jerry's specific repro is a relative
<header>/<footer>in a flex column on Vision Board's<hyperframes-player>cross-origin preview. The new test mirrors this exact shape (relative header at offsetTop:80, relative footer at offsetTop:968) and asserts the frozen anchors. The fix lands inpackages/core/src/runtime/init.ts— the common runtime shared by the single-container preview and theFrameCapturesidecar path — so VA board, Studio editor, and producer renders all hit the same code path. Producer was already correct per the PR body (final server render was fine), so unconditional pinning doesn't regress producer; the preview-only collapse is the only behavioral delta. - Cross-PR family non-regression.
- #1618 (page-side compositor handshake) —
applyClipLayoutruns insidepostTimeline, separate fromprepareComposite/settled-seek. No inline-style writes during the handshake window. - #1656 (capture screenshot gate flip) — producer's screenshot path runs after the runtime has stabilized; the frozen pin sets static inline styles that the screenshot sees as final DOM. No race.
- #1662 (descendant-hide on inactive clips) — descendant-hides target children of inactive clips, not the clip element itself. The freeze captures the clip's own
offset*, which remains valid. Also,postTimelinefires before activation/inactivation logic, so #1662's hides haven't run yet at freeze time. - #1686 (GSAP transition overlay flagging) — orthogonal: that PR flags transition overlays, not root-level clip layout.
- #1692 (runtime
immediateRender:true+ array timeline norm) — frame-0 GSAP transforms are transform-independent foroffset*, so the freeze captures the unmodified flow position. Inline comment at:347-350explicitly addresses this.
- #1618 (page-side compositor handshake) —
- Legacy normalization interaction. The pre-pass at
:297-333(legacy anchored defaults + centering transform) runs BEFORE the freeze loop and never takes elements out of flow — its inline comment at:295-296correctly explains why. Verified the only mutations are inlinetop/left/width/heightclears (and conditional restores), nopositionwrites. - jsdom / hidden fallback.
width <= 0 || height <= 0at:362skips the freeze, and the apply loop falls back toel.style.top = "0"/el.style.left = "0"— preserving prior behavior for non-laying-out environments.
CI state at HEAD 0131fe0b435d1d37fc0605f536235c8ec3a413b3: All required checks green (CI Build/Lint/Typecheck/Test, Format, Semantic PR title, Fallow audit, CodeQL × 3, Player perf × 5, Preview parity, Windows render verify, runtime contract, Studio load smoke, SDK unit+contract+smoke, CLI smoke). regression-shards 1-8 still IN_PROGRESS — the standard HF cadence, none failing. No regression failures observed.
Prior reviewer state: Xiaye Wang (xiayewang-heygen, MEMBER) APPROVED at 2026-06-24T21:18:42Z on the same SHA. His findings converge with mine: correct root-cause framing, freeze-before-mutate invariant verified, transform-independence of offset* confirmed, offsetParent !== rootEl and zero-box guards behave as intended, frozen width preserves flex layouts, common-runtime delta means single-container preview and FrameCapture sidecar share the fix. He flags the same two non-blocking observations I see (implicit test coverage on the early-continue / zero-box paths, and the two getComputedStyle passes — necessary because measurement must precede mutation). Rames has not posted at read time.
Review by Via
0131fe0 to
8d73cb4
Compare
Studio/preview stamps `data-start` onto ID'd and GSAP-targeted flow children (eg. a <header>/<footer> in a flex column) so the design panel can discover them. applyClipLayout then force-absolutized those stamped elements as if they were authored overlay clips, collapsing the layout: the footer shrink-wrapped and its `justify-content: space-between` clustered into the top-left, while the rendered video — which never stamps (production renders run as the top-level page, not in an iframe) — stayed correct. Mark runtime-stamped clips with `data-hf-autostamped` and skip them in applyClipLayout so they remain in document flow. The preview now matches the rendered video (true WYSIWYG). Authored overlay clips are unchanged, so the golden regression suite is unaffected.
8d73cb4 to
a35268f
Compare
What
In Studio/preview, root-level flow children that the runtime auto-stamps with
data-start(so the design panel can discover ID'd / GSAP-targeted elements) were being force-absolutized byapplyClipLayoutas if they were authored overlay clips. That collapsed document-flow layouts: a flex-column<footer>shrink-wrapped and itsjustify-content: space-betweenclustered into the top-left, overlapping the header.This marks runtime-stamped clips with
data-hf-autostampedand skips them inapplyClipLayout, leaving them in document flow.Why
Production renders run as the top-level page and never stamp, so the rendered video was always correct — only the in-iframe preview diverged. Skipping stamped clips makes the preview match the rendered output (WYSIWYG). Authored overlay clips are untouched, so existing behavior and the golden regression suite are unaffected.
How
applyClipLayout: skip any clip carryingdata-hf-autostamped(one guard, after the existingdata-startcheck). Authored clips take the unchanged path.data-startonto GSAP-targeted and ID'd children withdata-hf-autostamped.Test plan
Unit (
init.test.ts): authoreddata-startclip still force-absolutized; stamped clip left in flow.Local golden regression (in-iframe stamping never runs in the producer, so this path is unchanged — confirmed):
Manual — flow-layout composition (header / table / footer column), measured in the player's iframe:
position: absolute, width 854position: relative(in flow), width 1760position: absolute, top 118, width 595position: relative, top 966, width 1760[80, 388, 601](clustered)[80, 971, 1766](spread full-width)