Skip to content

fix(core): namespace SVG ids during composition inline to prevent cross-scene collisions - #3494

Open
miga-heygen wants to merge 2 commits into
mainfrom
fix/svg-id-collision-nested-compositions
Open

fix(core): namespace SVG ids during composition inline to prevent cross-scene collisions#3494
miga-heygen wants to merge 2 commits into
mainfrom
fix/svg-id-collision-nested-compositions

Conversation

@miga-heygen

Copy link
Copy Markdown
Contributor

Summary

  • Prefix SVG element ids with the composition's document-unique runtime id during sub-composition inline, so <clipPath id="clip">, <symbol id="shape">, <filter id="fx">, etc. no longer collide once two nested scenes are merged into one render/preview document.
  • Rewrite every same-document reference to a renamed id to match: DOM attributes (href, xlink:href, any url(#id) value — clip-path, filter, mask, fill, stroke, marker-start/mid/end, including inside style) and the composition's own extracted <style> text (both #id selectors and url(#id) declaration values).
  • Renamed elements keep their original id on data-hf-authored-id (the same attribute fix(core): scoped getElementById fails with duplicate element IDs across sub-compositions #646 added for the composition root), so an inline script's own document.getElementById(originalId) keeps resolving via the existing __hfGetElementById scoping shim — the fix doesn't regress the already-fixed getElementById scoping.
  • Extracted selectorIdTokens.ts from compositionScoping.ts's existing single-id selector scan so the new many-id rewrite reuses the same guarded-region (quote/bracket) logic instead of a second copy.

Why not the media-id / getElementById approach?

getElementById was already scoped per composition in #646, and media pipeline ids got a parallel data-hf-render-id attribute in #3340 — both work by adding a side-channel attribute without touching the real id. That doesn't work here: url(#id) and href="#id" are resolved by the browser's native SVG/CSS engine, which always binds to the first element in document order carrying that literal id attribute. No JS proxy can intercept native resolution, so the id attribute itself has to become document-unique.

Closes #3490

Test plan

  • packages/core/src/compiler/svgIdNamespacing.test.ts (new, 13 tests): unit coverage for id renaming, url()/href rewriting across clip-path/filter/mask/fill/stroke/marker-*/style, xlink:href, and CSS selector/declaration rewriting.
  • packages/core/src/compiler/inlineSubCompositions.test.ts (new suite): end-to-end repro of the issue — two sibling scenes reusing #clip/#shape/#fx get distinct non-colliding ids that still resolve correctly; the same catalog block used twice in one scene is disambiguated; getElementById(originalId) still resolves for an inline script after rename.
  • bun run typecheck — clean.
  • oxlint — clean.
  • Full existing suites re-run clean: compositionScoping.test.ts (50), htmlBundler.test.ts (58, exercises inlineSubCompositions end-to-end via the preview bundler) — no regressions.

Co-Authored-By: Miga noreply@anthropic.com

…ss-scene collisions

Two nested compositions that each declare their own SVG ids (`<clipPath
id="clip">`, `<symbol id="shape">`, `<filter id="fx">`) are legal per
file and pass `hyperframes check`, but collide once both are inlined
into one render/preview document. `url(#id)` funcrefs (`clip-path`,
`filter`, `mask`, `fill`, `stroke`, `marker-start/mid/end`) and
fragment `href`/`xlink:href` refs (`<use href="#id">`) are resolved by
the browser's native SVG/CSS engine, which always binds to the first
matching id in document order — so the later scene either clips to
nothing or paints the earlier scene's content.

`getElementById` was already scoped per composition in #646, and media
pipeline ids got a parallel `data-hf-render-id` attribute in #3340.
Neither covers this: native `url(#id)`/`href="#id"` resolution can't be
intercepted by a JS proxy, so the `id` attribute itself has to become
document-unique.

Add `svgIdNamespacing.ts`: during `inlineSubCompositions`, every id
declared on an `<svg>`-subtree element is prefixed with the
composition's document-unique runtime id, and every same-document
reference to it — DOM attributes (`href`, `xlink:href`, any `url(#id)`
value including inside `style`) and the composition's own extracted
`<style>` text (both `#id` selectors and `url(#id)` declaration
values) — is rewritten to match. Renamed elements keep their original
id on `data-hf-authored-id`, the same attribute #646 already added for
the composition root, so an inline script's own
`document.getElementById(originalId)` keeps resolving via the existing
`__hfGetElementById` scoping shim.

Extract `selectorIdTokens.ts` from `compositionScoping.ts`'s existing
single-id selector scan so the new many-id rewrite reuses the same
guarded-region logic instead of a second copy.

Closes #3490

Co-Authored-By: Miga <noreply@anthropic.com>

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

R1 adversarial — SVG ID collision namespace pass

Summary: DOM walk over svg[id]/svg [id] renames each declared id to ${sanitizedNamespace}--${originalId}, records the authored id on data-hf-authored-id, and rewrites same-element attribute refs via a url(#...) regex + suffix :href detector. <style> text goes through a postcss pass + a shared selectorIdTokens scan (extracted from the existing root-id rewrite so both callers share one quote/bracket state machine).

Blockers (P0/P1):

  • (none)

Concerns (non-blocking):

  • ARIA id-refs not rewrittenaria-labelledby, aria-describedby, aria-controls, aria-owns, aria-flowto all carry bare id lists (no #), and <svg><title id=\"chart-title\"> referenced by aria-labelledby=\"chart-title\" is standard SVG a11y. Post-rename the id is scene-a--chart-title; the aria attr still says chart-title and either resolves to another scene's title in the merged doc or dangles. Not a first-order render bug (matches the ticket's url(#)/href scope) but worth a followup — same class of native-resolver reference that motivated this PR.
  • [id=\"foo\"] attribute selectors in <style> are silently skippedselectorIdTokens.ts:markUnguardedOffsets deliberately masks out bracket regions (correct for # disambiguation), so rewriteSvgIdReferencesInCss never touches [id=\"clip\"]. Rare in author CSS, but any composition that reaches for the attribute form instead of #clip will silently stop matching after rename. Worth a test asserting current behavior + a doc note.
  • Idempotency not guarded — a second namespaceSvgIds(root, ns) call double-prefixes (scene-a--scene-a--clip) AND overwrites data-hf-authored-id with the already-namespaced id, destroying the original. inlineSubCompositions looks single-pass per instance so this is latent, but no if (el.hasAttribute(SVG_AUTHORED_ID_ATTR)) skip guard exists to catch a future re-entry (e.g. nested inline of a fragment that was itself pre-inlined).
  • No golden-frame / render-pixel test — three unit tests confirm the id map is correct, but nothing asserts "two scenes reusing #fx actually paint their own filter after the compiler ran end-to-end." The repro in #3490 is visual; the failure mode is silent misresolution. A single regression-shard scene mirroring the ticket repro would lock the fix in against future refactors of the URL regex.
  • sanitizeNamespaceSegment folds special chars to - without a collision guardfoo! and foo? both become foo-, so any two runtime ids that differ only in special chars produce the same prefix. Runtime ids from assignBundledRuntimeCompositionIds are compiler-generated and almost certainly alphanumeric, but the sanitizer is a public-ish contract; a defensive test asserting the runtime-id shape would prevent surprise later.

Verified clean:

  • URL fragment coverage: url(#id), url(\"#id\"), url('#id'), url( #id ), and all funcref attrs (clip-path, filter, mask, fill, stroke, marker-start/mid/end) reach the same regex via the generic attribute walk — no per-attr whitelist to drift.
  • Fragment-href: isHrefAttrName catches href and every foo:href suffix (covers xlink:href regardless of DOM impl), and rewriteHrefValue short-circuits on non-# values so https://example.com/#clip and asset URLs stay untouched (explicit test).
  • Longer-id-shadowing: selectorIdTokens.ts sorts candidates longest-first and gates on isSelectorNameChar boundary, so #clip in the id map never eats #clip2. Explicit test in both suites.
  • No-SVG fast path: querySelectorAll(\"svg [id], svg[id]\") returns empty → idMap.size === 0 → early return, no attribute walk.
  • Anonymous-host guard: empty namespace → early return with empty map, no mutation. Matches the same guard scopeCssToComposition and wrapScopedCompositionScript apply — consistent with existing scoping-primitive contract.
  • getElementById(originalId) for author scripts: data-hf-authored-id is set on every renamed element; __hfGetElementById shim (from #646) already checks this attr as fallback. Explicit test asserts scene-a root still resolves symbol by authored id shape after rename.
  • SVG_AUTHORED_ID_ATTR shares the exact string constant (\"data-hf-authored-id\") with AUTHORED_ROOT_ID_ATTR in compositionScoping.ts — the shim's fallback path already reads it, so no third attr introduced.
  • Shared scanner extraction: replaceAuthoredRootIdSelectors is now a thin wrapper over replaceSelectorIdTokens; behavior preserved (single-form → one-element candidate list), one state machine to maintain instead of two.
  • Perf: single tree walk + O(N) attr scan + one postcss parse per composition — no visible O(N²).

CI: Preflight/lint/format/typecheck/unit/producer-integration/SDK/perf-drift/parity/fps/load/scrub/preview-parity/Fallow all green. regression-shards shards 1-9, Smoke: global install, Render/Tests on windows-latest, Test, Analyze (javascript-typescript) still pending — the render-parity signal is exactly what would exercise the visual repro so worth waiting on before merge.

Signature: — Via

* character-by-character state machine.
*/
const GUARDED_SELECTOR_SEGMENT_RE =
/"(?:[^"\\]|\\.)*"|'(?:[^'\\]|\\.)*'|\[(?:"(?:[^"\\]|\\.)*"|'(?:[^'\\]|\\.)*'|[^\]])*\]/g;
SVG ids referenced exclusively by JavaScript (e.g. GSAP's
`tl.to("#cut-1")`) must not be renamed — global libraries access
`document` directly and bypass the composition-scoped querySelector
Proxy, so renamed ids break animation targeting.

Now namespaceSvgIds pre-scans for url(#id) funcrefs and href="#id"
fragment refs (both attribute values and <style> text content) and
only renames ids that appear in at least one native reference. Ids
with no native reference (only used by JS) keep their original
names.

Fixes style-17-prod regression where GSAP selectors targeted
#cut-1/#tear-path-1 etc. which were unreachable after rename.

Co-Authored-By: miga-heygen <miguel.sierra_miga@heygen.com>

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

COMMENT — R1 SVG id namespacing @ 75b84322586c06a4f8b9c84415a9e9a8465639a6. No P0/P1 blockers; three orthogonal follow-ups. Holding off APPROVE only because regression-shards (esp. shard-5 which contains style-17-prod) and preview-parity are still in-flight and are the load-bearing signals for both this fix AND the regression it patches.

Concurring with @vibook-bot's R1 (unchanged at head):

  • ARIA id-refs (aria-labelledby / -describedby / -controls / -owns / -flowto) still not tracked. Same class as url(#) / href="#" — native browser resolution — worth a follow-up.
  • [id="foo"] attribute-selector inside <style> intentionally skipped by the bracket-guard mask.
  • No idempotency guard on SVG_AUTHORED_ID_ATTR — a re-entry double-prefixes and clobbers the original.
  • No golden-frame test locking the #3490 repro; leaning on regression-shards + preview-parity to catch it.

Independent concerns (differentiated):

  1. Regression-fix trade-off worth documenting in-code. The 75b8432 pivot to "only rename ids that have a native url()/href reference" is the right call for style-17-prod (GSAP → document.querySelector bypasses the scoped shim, so a renamed id is unreachable). But it introduces a residual cross-comp collision path: two composition instances that both animate the SAME JS-only id (e.g. two catalog scenes each doing tl.to("#cut-1")) now BOTH keep id="cut-1" → merged doc has two id="cut-1" → scene B's GSAP targets scene A's element — the exact document-order-resolution bug this PR fixes, now residual for the JS-only subset. Sound trade-off (less common than the always-broken JS case), but the module doc should call it out so a future reader doesn't broaden the pre-scan and silently re-break style-17. Suggest a comment near collectNativelyReferencedIds naming this residual class.

  2. Test gap paired with #1. No test asserts the two-comps-sharing-JS-only-id case — either as the "we accept this collision" contract or a scoped mitigation. Would lock in the current design.

  3. Runtime-injected url(#id) isn't pre-scanned. A <script> that later does el.setAttribute("clip-path", "url(#foo)") as the ONLY reference to #foo misses the pre-scan → foo isn't renamed → same-class collision if duplicated across comps. Unlikely in HF composition authoring, but a known blind spot.

Verified clean at 75b8432:

  • url(#..) (bare/quoted/single-quoted/spaced), all funcref attrs (clip-path/filter/mask/fill/stroke/marker-*), inline style attr, and <style> text content — all routed through the same URL_HASH_REF_RE. No per-attr whitelist to drift.
  • href / foo:href (covers xlink:href regardless of DOM impl); non-fragment hrefs (https://example.com/#clip) untouched, explicit test.
  • Pre-scan scope is per-comp (namespaceSvgIds(innerRoot ?? contentDoc, ns)) — no cross-comp poisoning path.
  • Multi-instance same-src block: assignBundledRuntimeCompositionIds gives each instance a distinct runtime id → distinct namespace. End-to-end test locks it.
  • data-hf-authored-id reuses the exact string constant from compositionScoping.ts__hfGetElementById fallback already checks it, no third attr introduced.
  • Shared selectorIdTokens.ts scanner: replaceAuthoredRootIdSelectors is now a thin wrapper — one state machine to maintain, longest-first sort preserved (#clip never eats #clip2).
  • <style> extraction pipeline: sub-comp <style> textContent (including SVG-inline <style>) is extracted via plan.styleSources and rewritten by rewriteSvgIdReferencesInCss before scopeCssToComposition scopes it. Ordering is correct (rename → asset URL rewrite → composition scope).

CI: many required checks in-progress at snapshot; nothing failing. regression-shards (esp. shard-5 containing style-17-prod) and preview-parity are the load-bearing signals for both the primary fix AND the regression it patches — worth waiting on before merge. Happy to bump to APPROVE once those settle green.

— Review by tai (pr-review)

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.

Nested compositions that reuse SVG ids bind url(#…) / <use href="#"> to the first scene after inline

5 participants