Repository navigation
fix(core): timed svg clips hide outside their window - #5661
Conversation
Edit accuracy: accurate 271 (base branch 271), smooth 163 of thoseThe gate passes. Quarantined, measured but not gated (0) |
terencecho
left a comment
There was a problem hiding this comment.
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
|
Fixed at Root: TypeScript narrows an
Checked: |
terencecho
left a comment
There was a problem hiding this comment.
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
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 theirvisibility. 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.isStylableElementnow narrows toElement & ElementCSSInlineStyle: TypeScript narrows anElementbyHTMLElement | SVGElementtoHTMLElementalone, which hid that the visibility helpers were still typed HTML-only. Those helpers, andisRuntimeElementVisibleAt, now takeElementor the style-bearing type.Verification
<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' ].shapes at 1s: expected [ 'visible', 'visible', 'visible' ] to deeply equal [ 'visible', 'hidden', 'hidden' ].hyperframes snapshot --at 1,5,16of 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.tsanddomRealm.test.tsgreen 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.