Skip to content

fix(core): timed svg clips hide outside their window - #5661

Merged
miguel-heygen merged 3 commits into
mainfrom
fix/runtime-svg-clip-visibility
Oct 15, 2026
Merged

miguel-heygen merged 3 commits into
mainfrom
fix/runtime-svg-clip-visibility

Conversation

@miguel-heygen

@miguel-heygen miguel-heygen commented Oct 15, 2026 •

Copy link
Copy Markdown
Collaborator

What

A timed <svg> clip now hides outside its window like any other clip. Before, every <svg data-start> stayed visible at every time, so a seek or a snapshot showed the last full-frame SVG layer instead of the one whose window holds that time. <div> clips in the same composition were fine.

Why

The runtime's visibility pass skipped every element that is not an HTML element (if (!isHtmlElement(rawNode)) continue;). An <svg> and its shapes are in the SVG namespace, so the pass never set their visibility. Once the first-pass hide rule lifted, they all showed. The walk that keeps a clip hidden while an ancestor clip is hidden had the same guard, so an SVG group clip could not hide a timed shape inside it.

How

Both guards now use the existing isStylableElement (HTML or SVG, any realm), so every element kind a timeline clip can be gets hidden and shown the same way.

isStylableElement now narrows to Element & ElementCSSInlineStyle: TypeScript narrows an Element by HTMLElement | SVGElement to HTMLElement alone, which hid that the visibility helpers were still typed HTML-only. Those helpers, and isRuntimeElementVisibleAt, now take Element or the style-bearing type.

Verification

  • New test: three full-frame <svg> clips (0-4 s, 4-12 s, 12-20 s), each holding a timed shape that outlives it, seeked to 1 s, 5 s and 16 s; exactly one layer and its shape are visible at each time. On main it fails: layers at 1s: expected [ '', '', '' ] to deeply equal [ 'visible', 'hidden', 'hidden' ].
  • With only the ancestor guard reverted, the same test fails on the shapes: shapes at 1s: expected [ 'visible', 'visible', 'visible' ] to deeply equal [ 'visible', 'hidden', 'hidden' ].
  • A real hyperframes snapshot --at 1,5,16 of a composition with three full-frame SVG layers (red, green, blue) through the built CLI on Linux: the center pixel reads red at 1 s, green at 5 s and blue at 16 s.
  • packages/core: init.test.ts and domRealm.test.ts green 3 runs in a row (248 tests), tsc clean, oxlint and oxfmt clean on the changed files.

Ships alone: it is a self-contained runtime fix in packages/core; the only other open change from this lane is an unrelated CLI publish fix in another package, and folding them together would mix two reviews.

@github-actions

github-actions Bot commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

Edit accuracy: accurate 271 (base branch 271), smooth 163 of those

The gate passes.
Smoothness is reported in the artifact, not gated. A case fails only if it fails 2 of 3 runs.

Quarantined, measured but not gated (0)

@terencecho terencecho 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.

Source review at 531313c4b15fc14fe25692f072d5a8aa3b6644c8.

The two guard changes correctly address both layer visibility and ancestor gating, and the added test exercises timed SVG descendants as well as their layers (packages/core/src/runtime/init.test.ts:1475).

Blocker [P2] — Widen the shared element contracts along with the guard

packages/core/src/runtime/init.ts:2806 now narrows rawNode to HTMLElement | SVGElement, but the next calls still require HTMLElement: dataHiddenDisplayNodes.has/add at 2809–2810 (the set is WeakSet<HTMLElement> at 2616), hideByDisplay/restoreDisplay, nodeAffectsAudio, visibleAt, dueSoon, and the flow/leaf helpers. The other changed guard at 2694 similarly passes the new union directly to visibleAt, whose parameter remains HTMLElement at 2676. There is no intervening HTML narrowing.

These are incompatible calls in the dedicated runtime TypeScript program: packages/core/tsconfig.runtime.json:7 includes this source, and typecheck:runtime checks that project. The ordinary core project excludes runtime, so a bare tsc result does not establish this program is clean. The previous HTML guards satisfied these contracts.

Please widen element-only helpers to Element and style-bearing helpers/collection keys to HTMLElement | SVGElement, propagating the visibility contract through packages/core/src/runtime/timeline.ts:35; do not hide the mismatch with HTML casts.

Audited: both changed files fully, legacy then HEAD, plus relevant helper, timing, lifecycle, producer and source-embedding paths; an independent targeted adversarial pass corroborated this finding. Limits: source-only; no compiler, tests, browser or native execution. This is a source-established type mismatch, not an observed diagnostic or a demonstrated rendered failure. No second concrete runtime regression was established.

Verdict: REQUEST CHANGES
Reasoning: The SVG behavior change leaves its callers incompatible with the unchanged HTML-only contracts in the runtime typecheck.

— tai

@miguel-heygen

Copy link
Copy Markdown
Collaborator Author

Fixed at 279a3602, at the root of the type mismatch rather than with casts.

Root: TypeScript narrows an Element by the guard value is HTMLElement | SVGElement to HTMLElement alone, so after the guard an <svg> was typed as an HTMLElement and the runtime typecheck passed while the contracts were wrong. I confirmed it with a probe: const h: HTMLElement = rawNode compiled after the guard.

  • isStylableElement now narrows to StylableElement = Element & ElementCSSInlineStyle (domRealm.ts), which TypeScript keeps as is. With it, tsc -p tsconfig.runtime.json listed every HTML-only contract on the path.
  • Widened to Element: isRuntimeElementVisibleAt (timeline.ts), the timedVisibilityAt closure, dueSoon, nodeAffectsAudio, isTimedClipInFlow, isTimedClipLeaf.
  • Widened to StylableElement: dataHiddenDisplayNodes, displayBeforeHide, hideByDisplay, restoreDisplay.
  • positionEdits.ts drops its local HTMLElement | SVGElement alias for the shared type, and compositionReadiness.ts drops its as HTMLElement cast.

Checked: tsc -p tsconfig.runtime.json clean; compositionReadiness, init, domRealm, positionEdits and timeline tests green 3 runs in a row (377 tests); oxlint and oxfmt clean on the changed files.

@terencecho terencecho 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.

Source re-review at exact HEAD 6a3000bada7328b64a8bea9a911bf3a4d8679817, effective base 0e9f380c2e2295db4b88945a77cfa81001ac5c27.

Correction to my earlier review: I withdraw its assertion that the old union necessarily produced incompatible-call errors in the runtime TypeScript program. I did not establish that compiler failure; no compiler was run in either of my reviews. The author's narrowing probe and reported compiler results are external receipts, not my executions. This correction is separate from the now-aligned source contracts.

The source concern is addressed by 279a36020a30a72bbccd9e54137948776b6faf89: StylableElement = Element & ElementCSSInlineStyle is shared by the guard, style helpers and collection keys; attribute-only timing/ancestor/flow helpers and isRuntimeElementVisibleAt accept Element. Readiness no longer needs the HTML cast, and position edits imports the same structural type. Current HEAD retains those changes and refreshes the generated position-edits artifact; its changes are consistent identifier renaming, not a new runtime behavior.

Both SVG admission and ancestor gating are covered by the retained regression: each rect deliberately outlives its SVG layer, and the assertions check the real visibility pass at 1, 5 and 16 seconds. The timeline double does not replace the visibility predicate. This is meaningful source-level coverage, not a claim that I executed the test or verified rendered pixels.

Audited: all seven effective changed files, legacy and HEAD, relevant real timing/seek/lifecycle, CSS/readiness, display restoration, audio, position-edit/reset and generated-source integration, plus bundled-change pairs. An independent adversarial pass completed all seven BASE files before HEAD conclusions. The primary pass was initially partial and its test gaps were completed by two bounded supplemental readers; I do not retroactively claim its original ordering was complete. Parent independently reconstructed both immutable trees, rebound all 14 changed blobs and checked the combined test-coverage intervals.

Trusting/not exercised: repository/compiler/test/build/CI execution, GSAP/jsdom/Chromium native behavior, actual SVG pixels, cross-realm browser behavior, SDK/native embedding and external providers. Dependency/caller coverage is scoped, not whole-repository semantic completeness. Author execution receipts remain attributed reports.

Verdict: APPROVE
Reasoning: No introduced, reachable blocker established in the reviewed scope; the previous source-contract concern is addressed, with the compiler-error overstatement corrected above.

— tai

@miguel-heygen
miguel-heygen added this pull request to the merge queue Oct 15, 2026
Merged via the queue into main with commit 1ee6754 Oct 15, 2026
122 checks passed
@miguel-heygen
miguel-heygen deleted the fix/runtime-svg-clip-visibility branch October 15, 2026 23:42
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