Skip to content

fix(runtime): keep stamped flow children in document flow in preview#1702

Merged
miguel-heygen merged 1 commit into
mainfrom
fix/clip-layout-absolute-anchor
Jun 25, 2026
Merged

fix(runtime): keep stamped flow children in document flow in preview#1702
miguel-heygen merged 1 commit into
mainfrom
fix/clip-layout-absolute-anchor

Conversation

@miguel-heygen

@miguel-heygen miguel-heygen commented Jun 24, 2026

Copy link
Copy Markdown
Collaborator

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 by applyClipLayout as if they were authored overlay clips. That collapsed document-flow layouts: a flex-column <footer> shrink-wrapped and its justify-content: space-between clustered into the top-left, overlapping the header.

This marks runtime-stamped clips with data-hf-autostamped and skips them in applyClipLayout, 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 carrying data-hf-autostamped (one guard, after the existing data-start check). Authored clips take the unchanged path.
  • Stamping pass: tag the two sites that inject data-start onto GSAP-targeted and ID'd children with data-hf-autostamped.

Test plan

  • Unit tests added/updated
  • Manual testing performed
  • Documentation updated (if applicable)

Unit (init.test.ts): authored data-start clip 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):

Fixture Result
style-3-prod PASS (PSNR ≥ 30)
style-8-prod PASS
overlay-montage-prod PASS

Manual — flow-layout composition (header / table / footer column), measured in the player's iframe:

Element Before After
header position: absolute, width 854 position: relative (in flow), width 1760
footer position: absolute, top 118, width 595 position: relative, top 966, width 1760
footer children (x) [80, 388, 601] (clustered) [80, 971, 1766] (spread full-width)

@xiayewang-heygen xiayewang-heygen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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:

  1. Pass 1 preserves the legacy-anchored-defaults normalization (no mutation of flow).
  2. 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 !== rootEl filter correctly skips nested clips → they fall back to top:0/left:0 (preserves prior behavior for non-root cases). ✓
  • jsdom / hidden zero-box elements skipped via the width <= 0 || height <= 0 guard → also fall back. ✓
  • Frozen widths/heights propagated so flex-driven layouts (justify-content: space-between etc.) 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 !== rootEl early-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 james-russo-rames-d-jusso left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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-2298applyClipLayout 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:357position === "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:366const 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 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.

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):

  1. nit — Test coverage is implicit on two of the new branches: packages/core/src/runtime/init.ts:359 (the offsetParent !== rootEl early-continue for nested clips) and packages/core/src/runtime/init.ts:362 (the width <= 0 || height <= 0 zero-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-364 measures every promotable clip's offsetTop/offsetLeft/offsetWidth/offsetHeight before the apply loop at :366 runs any el.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 #1692 immediateRender:true) doesn't pollute the freeze. The inline comment at :347-350 calls this out correctly.
  • Surgical anchor-heuristic change. The !shouldForceAbsolute && (...) gate at :384 and :391 is the load-bearing edit: only trust the pre-promotion computed top/left when we are NOT promoting. For promoted clips, fall through to the frozen offset. Preserves the prior behavior for clips that were already absolute/fixed (deliberately CSS-anchored — shouldForceAbsolute=false keeps the computed-anchor short-circuit).
  • Inline-style short-circuit preserved. Boolean(el.style.top) || Boolean(el.style.bottom) at :382-383 still wins over the frozen value, so an author setting inline style.top directly is untouched (existing contract).
  • Width/height pin gated correctly. :404-408 else if (frozen && !el.style.width) only pins when there's no author-inline width AND no data-width override. data-width still wins via forcedWidth at :400-403. The full-width footer with justify-content: space-between Miguel cites is covered by the frozen.width fallback.
  • Idempotency on re-runs. applyClipLayout runs from postTimeline (:1678) which fires on init, sub-comp load/unload (:1399, :1426, :2096, :2292, :2297, :2633). On a second run, promoted clips' position is absolute → 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/offsetLeft write 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 in packages/core/src/runtime/init.ts — the common runtime shared by the single-container preview and the FrameCapture sidecar 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) — applyClipLayout runs inside postTimeline, separate from prepareComposite/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, postTimeline fires 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 for offset*, so the freeze captures the unmodified flow position. Inline comment at :347-350 explicitly addresses this.
  • 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-296 correctly explains why. Verified the only mutations are inline top/left/width/height clears (and conditional restores), no position writes.
  • jsdom / hidden fallback. width <= 0 || height <= 0 at :362 skips the freeze, and the apply loop falls back to el.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

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.
@miguel-heygen miguel-heygen force-pushed the fix/clip-layout-absolute-anchor branch from 8d73cb4 to a35268f Compare June 24, 2026 23:32
@miguel-heygen miguel-heygen changed the title fix(runtime): anchor in-flow clips when forcing them absolute fix(runtime): keep stamped flow children in document flow in preview Jun 24, 2026
@miguel-heygen miguel-heygen merged commit 9bf1e1c into main Jun 25, 2026
59 of 66 checks passed
@miguel-heygen miguel-heygen deleted the fix/clip-layout-absolute-anchor branch June 25, 2026 00:12
@miguel-heygen miguel-heygen mentioned this pull request Jun 25, 2026
3 tasks
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.

4 participants