Skip to content

fix(core): preserve SVG icons in loaded compositions - #5096

Merged
miguel-heygen merged 5 commits into
mainfrom
fix/preview-namespaces-svg-ids-in-loaded-sub-compositions
Oct 6, 2026
Merged

miguel-heygen merged 5 commits into
mainfrom
fix/preview-namespaces-svg-ids-in-loaded-sub-compositions

Conversation

@miguel-heygen

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

Copy link
Copy Markdown
Collaborator

What

Keep SVG icons, masks and gradients attached to their own nested composition when files load in the browser, while preserving authored script and CSS selectors.

Why

Two scene files can reuse SVG definition IDs. A later scene's clipping path can resolve into an earlier, hidden scene and erase an icon. Renaming those collisions must also preserve deferred lookups, captured IDs and rules that match unchanged HTML elements.

How

Stage the initial external and inline content, then finalize already native-referenced SVG definitions before scripts run. After initial scripts settle, the shared collision pass handles definitions newly referenced by those scripts. Earlier eligible definitions, composition identities and local target bindings stay fixed. The alias service installs independently of inline script wrappers and preserves authored ID and native-reference attribute selectors in scripts and CSS.

Compiled scene scripts carry the alias installer as a prebuilt artifact (build:svg-selector-aliases, same builder as the position-edits render artifact, kept in sync by check:svg-selector-aliases in core test). Its text no longer depends on the transpiler that runs the compiler, so a tsx run cannot inject __name(...) helpers into page code, and each scene script carries about 7 KB instead of about 15 KB. The installer runs in its own function, so its "use strict" never applies to the authored script.

The comment ratchet now skips packages/<pkg>/src/generated/: a generated file's header comments come from its generator, and the committed artifact would otherwise fail the new-file share check.

The catalog fetch mirror is re-recorded. Font subset requests are built from every character in the compiled page, scripts included, so the installer's characters change each Google Fonts subset URL.

Compatibility limits

  • JS-only SVG IDs stay unchanged. An ID string captured while its definition is still JS-only can become stale if an initial script adds its first native reference and the second pass must rename it. The fix does not broaden the existing contract for libraries that query the real document directly.
  • Attribute selectors that use a declared namespace prefix (@namespace xl url(...) with [xl|href="#id"]) follow renamed references through the browser's live stylesheet before scene scripts run, in both runtime and compiled output. Only the browser knows which @namespace rules a stylesheet actually kept, so compiled CSS text leaves these selectors as authored. Compiled output therefore needs JavaScript for this narrow case: with scripts off, or before the runtime starts, such a selector matches only references that were not renamed. Unprefixed and *| attribute selectors and ID selectors stay rewritten in the compiled CSS text.
  • A root stylesheet linked with <link rel="stylesheet"> that selects a renamed SVG ID or href still matches both scenes in the compiled render, but in the runtime preview it matches only the scene that kept the original ID. Runtime rewriting covers <style> text; extending it to linked sheets is a follow-up.
  • Only querySelector and querySelectorAll on elements follow renamed references. closest, matches and document-level queries from unwrapped external scripts see the renamed values.
  • Inline-template scenes now mount before external scene scripts run, so every ID is final before any script reads it. An unscoped selector in an external script can therefore also reach an inline-template scene.

Validation

Run at 2e042dac. The two later commits change only the generated-file banner and the catalog fetch mirror; at the final head, both artifact sync checks, the generated-artifact tests, the catalog drift check (no unmirrored fetches), comment checks and formatting pass.

  • Chromium contract suite (coreRuntimeBrowser.test.ts): 47/47, with runtime and compiled cases for each of the four review points. The declared-namespace case passed three runs in a row. It covers a rule nested in @media, an author edit to the repaired rule, a second refresh, and a foreign-namespace attribute that carries the renamed value and must not match.
  • Core: selector lexer 15, SVG namespacing 27, composition scoping 76 (three runs), inline sub-compositions 39, composition loader 66, installer artifact 2 (three runs). Producer compile/runtime parity 13. Comment ratchet unit tests pass.
  • Under tsx, neither the installer artifact nor a wrapped scene script contains __name(.
  • Mutations, each failing with an assertion: removing the live stylesheet repair (opacity 1 instead of 0.4, runtime and compiled), ignoring an author edit to a repaired rule, skipping nested rules, letting the installer's "use strict" reach the authored script, letting malformed alias JSON throw, and accepting a wrong-typed alias entry.
  • Format, lint, typecheck, artifact sync, comment checks, comment ratchet and Fallow audit: clean.
  • The Studio capture below comes from a fresh build of 2e042dac, at four seconds on the 16-tile fixture.

Before

In the synthetic 16-tile fixture, tiles 1, 2 and 16 have no icon at four seconds. Their clipping paths resolve into the hidden intro scene.

Studio before: three icons are missing

After

All 16 icons render. Each tile's clipping path now resolves to its own renamed definition in the visible grid scene.

Studio after: all sixteen icons render

@github-actions

github-actions Bot commented Oct 6, 2026 •

Copy link
Copy Markdown

Edit accuracy: accurate 2055 (base branch 2055), smooth 1603 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)

@miguel-heygen
miguel-heygen marked this pull request as ready for review October 6, 2026 00:59

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

Request changes at 6336ab5a386936f33e012c8900874343cca9f30b. Reusing the shared SVG namespacing pass is the right direction, and the new collision tests cover the original blank-tile case. The new runtime path still changes existing scenes in these cases:

  • Blocker — external scripts lose authored-ID lookups (packages/core/src/runtime/compositionLoader.ts:811). The refresh is optional, but the only installer lives in wrapScopedCompositionScript; external <script src> payloads are injected directly at lines 574–575. With two scenes defining clipPath#clip and only external scripts, a deferred svg.querySelector('#clip') in the second scene returns null after its ID is renamed. In an identical Chromium fixture against the PR parent 0d5e395, it returned clip; at this head the result is null and refreshInstalled is false. Install/refresh the alias independently of whether a wrapped inline script happened to execute.

  • Blocker — rewritten references invalidate attribute selectors (packages/core/src/runtime/compositionLoader.ts:792). The pass changes the second scene's <use href="#symbol"> to href="#grid--symbol", but the selector shim deliberately does not rewrite tokens inside [href="#symbol"]. A deferred wrapped inline script's use[href="#symbol"] lookup returned true against the parent and false at this head in Chromium (while its #clip lookup still succeeded). CSS rules using the same attribute selector also stop matching. Preserve these authored selectors when rewriting the references, and add coverage for both script and CSS consumers.

  • Blocker — scripts can retain an ID that the later pass changes (packages/core/src/runtime/init.ts:3395). mountCompositionContent executes scripts before loadExternalCompositions/loadInlineTemplateCompositions resolve, then this new pass renames IDs. A script that captures clipPath.id during mount and writes clip-path: url(#capturedId) from a later event handler keeps the old ID. When a later unrenamable HTML element shares that ID, the helper keeps the HTML ID and renames the earlier SVG definition; the later write now points at the HTML element. This follows the execution order and rename rule at svgIdNamespacing.ts:281–304; the parent did not rename the SVG. Namespace before script execution or preserve the captured-ID contract, with a deferred-handler test.

  • Blocker — scope-wide CSS rewrite changes rules for unchanged HTML IDs (packages/core/src/runtime/compositionLoader.ts:807). If a mounted scene has an SVG <filter id="fx"> followed by <div id="fx"> and <style>#fx { color: red }</style>, the helper keeps the unrenamable div and renames the SVG definition, then maps every #fx selector to the new SVG ID. The div retains id="fx" but loses its authored rule. The parent leaves the earlier SVG ID and the rule applies to the div. Scope CSS rewriting to references for the renamed definition rather than all same-text selectors, and test an HTML/SVG duplicate within one mount.

The first two failures were reproduced in Chromium against built runtime bundles from the parent and this exact head; the last two are code-path traces that need regression tests. I did not treat the pre-existing linked-stylesheet and same-scope duplicate-definition limitations as new regressions.

Verdict: REQUEST CHANGES
Reasoning: The original collision is addressed, but the runtime pass breaks existing deferred lookups and can redirect later ID and CSS references in affected scenes.

— Review by tai (pr-review)

@miguel-heygen

Copy link
Copy Markdown
Collaborator Author

Thanks for the four repros. Addressed at 2e042dac:

  1. External scripts: the shared alias service installs at runtime entry and refreshes at composition lifecycle boundaries. It no longer depends on a wrapped inline script. The external-only deferred lookup regression passes in Chromium.

  2. Attribute selectors: the shared selector transformer preserves authored predicates alongside the current native values written by namespacing. Script and CSS coverage includes href, namespaced href, URL references, attribute operators, and unrelated later writes that must stop matching. Compiled and lazy-loading paths are exercised. Selectors with a declared namespace prefix ([xl|href="#symbol"]) are repaired in the live CSSOM from the stylesheet's own @namespace bindings before scripts run, so compiled output needs JavaScript for that case; a foreign-namespace attribute carrying the renamed value does not match.

  3. Captured IDs: initial content is staged before already-referenced SVG definitions are finalized and scripts run. Both the same-host HTML collision and a later-mounted HTML collision retain the captured final ID in deferred handlers. A second pass namespaces definitions newly referenced by initial scripts. JS-only IDs and the global-library contract retain their existing behavior. An ID string captured while its definition is still JS-only can become stale if an initial script adds its first native reference and the second pass must rename it.

  4. HTML and SVG CSS matches: rewritten ID selectors preserve the authored keeper and the renamed SVG targets. A same-host HTML/SVG duplicate retains its red rule on both elements. Root styles also retain every intended renamed definition.

Verification: the four original reproductions failed with assertions against the unchanged runtime before the fixes and pass now, in runtime and compiled output (Chromium suite 47/47 at this head). Compiled scene scripts now carry the alias installer as a prebuilt artifact, so the emitted code no longer depends on the transpiler running the compiler. Counts, mutation checks and remaining limits are in the PR body.

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

Reviewed at 8edc51cc. I scoped to the delta since 6336ab5a (where @terencecho requested changes) plus the full svgSelectorAliases.ts, svgIdNamespacing.ts, the compositionLoader.ts load sequence and the compositionScoping.ts wrapper change. I trusted the 16 re-recorded catalog mirror .bin files and the generated artifact.

The four findings from the earlier change request, checked against the code at this head:

  1. External-only scripts lose authored-ID lookups: resolved. The alias service is now a standalone module (compiler/svgSelectorAliases.ts). The loader calls refreshSvgSelectorAliases() at the end of every namespaceMountedSvgIds pass (compositionLoader.ts:919), whether or not a wrapped inline script ran. Covered by the Chromium case "external-only scripts retain authored SVG selectors".
  2. Rewritten href breaks [href="#symbol"] lookups: resolved. Each reference write is recorded in data-hf-svg-reference-aliases, and rewriteSvgSelectors expands a matching attribute predicate to :is(<authored>, [href="<renamed>"]) for both the querySelector proxy and <style> text. Covered by "authored native-reference attribute selectors work in scripts and CSS", plus the operator, flag, escape and namespace cases.
  3. Scripts capture an ID the later pass renames: resolved by ordering, not patching. mountCompositionContent now returns runScripts instead of running scripts during mount. loadCompositions namespaces every initial mount, external and inline, before any scene script runs, and finalizedIds keeps those elements fixed in the post-script pass. Covered by "scripts capture the final SVG id before a deferred reference write", using the HTML div#clip keeper from the original trace.
  4. Scope-wide CSS rewrite drops rules for an unchanged HTML id: resolved. #fx now becomes :is(#fx, #<scope>--fx) instead of being swapped outright, so the unrenamed HTML element keeps its rule. Covered by "CSS id rules keep unchanged HTML and renamed SVG matches".

On the root-cause question: this is a real fix, not a per-render patch. The cause is that SVG url(#id) and href="#id" resolve document-wide, so two mounted scenes that reuse IDs collide. The compiled path already namespaced those collisions. This PR runs the same shared namespaceCollidingSvgIds pass in the runtime loader. The "repaired at runtime" part is narrow and necessary: @namespace-prefixed attribute selectors can only be resolved against the live CSSOM's namespace bindings, and the PR body documents that limit.

Reuse and simplicity: this is a net win. The roughly 120-line hand-written selector tokenizer and Element.prototype shim inlined in wrapScopedCompositionScript is deleted. The runtime, the compiled scene scripts (through the prebuilt svg-selector-aliases-inline artifact) and the CSS rewriter now share one TypeScript implementation backed by selectorIdTokens.ts. There's also an artifact sync check.

Non-blocking:

  • Collision-free documents are now annotated. namespaceCollidingSvgIds used to return early when nothing collided (hasAnyCollision). It now always runs applyRenames. rewriteElementIdReferences then records an identity alias for every href="#…" / url(#…) reference, even when before === after. A probe at this head, with one scope, no collisions and an empty id map, wrote data-hf-svg-reference-aliases='[{"name":"fill",…,"before":"url(#g)","after":"url(#g)"}, …]' onto every referencing <rect> and <use>. That adds about 130 bytes per reference to compiled HTML and the live DOM for every composition that uses SVG references, including ones with no collision. As far as I can tell the identity records carry no information: decideReferenceWrite derives before from the current value when there is no prior record, and refresh() filters before === after anyway. Writing only real changes, or keeping the early return for the first pass, would restore "no collisions, no DOM change". Worth a follow-up unless something reads them that I missed.
  • The ordering change is a behaviour change, and the PR states it: inline-template scenes now mount before external scene scripts run, so an unscoped selector in an external script can also reach an inline scene. That's acceptable, but it's the part most likely to surprise an existing project.

Gate state: @terencecho's CHANGES_REQUESTED at 6336ab5a is still live, so this approval alone does not open the merge gate. It needs to be cleared by that reviewer.

Verdict: APPROVE
Reasoning: All four earlier blockers are fixed at their root (ordering, recorded reference aliases, non-destructive CSS rewrites, a standalone alias service), each has a Chromium regression case, and the change consolidates two selector-rewrite implementations into one. The remaining note is a size/cleanliness follow-up, not a correctness issue.

— Rames Jusso

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

Re-review at 8edc51cc168d0b6b7f2f4b0c0fbcb85afdc3eab3 after my changes request on 6336ab5a:

  • The external-script-only alias installer now runs independently of an inline wrapper (packages/core/src/runtime/entry.ts:69, compositionLoader.ts:919), so deferred authored #id lookups survive the initial collision pass.
  • Recorded native-reference aliases preserve authored [href="#symbol"] lookups in scripts and CSS (packages/core/src/compiler/svgSelectorAliases.ts:43-65). The Chromium contracts cover both consumers, including an author-edited rule.
  • The loader stages external and initial inline scenes and finalizes their SVG IDs before running scene scripts (compositionLoader.ts:846-880); its post-script pass retains those initial IDs. This addresses the captured-ID timing case.
  • CSS ID selectors retain the original match alongside the renamed SVG match rather than redirecting away from unchanged HTML elements (svgSelectorAliases.ts:68-91). The browser tests cover that keeper case.

I checked the implementations and at-head Chromium coverage for all four original findings. The restored catalog mirror has the same 23 keys as before its accidental pruning. All 11 required checks are green at this head; I did not run the repository test suite locally because the isolated checkout lacks dependencies.

Non-blocking follow-ups: A standalone Chromium probe of the committed alias installer found that a same-origin stylesheet imported through <style>@import ...</style> is not traversed by the new live CSSOM repair: an imported @namespace xl rule with [xl|href="#symbol"] loses its match when the SVG reference becomes #grid--symbol (opacity 0.4 before, 1 after), whereas the direct inline rule stays at 0.4. CSSImportRule.styleSheet.cssRules is accessible but not in document.styleSheets. This is adjacent to the documented external linked-stylesheet limitation, rather than a regression in the four addressed inline/scene-script cases; if imported CSS must be supported, recurse into the imported sheet with its own namespace bindings and cover it in the full loader. Separately, collision-free documents now receive identity data-hf-svg-reference-aliases attributes on otherwise unchanged references; these add size without an established behavioral failure.

Verdict: APPROVE. The four blockers in my earlier review have been addressed at this exact head; this approval supersedes my previous request for changes. No merge action taken.

— Review by tai (pr-review)

@miguel-heygen
miguel-heygen added this pull request to the merge queue Oct 6, 2026
Merged via the queue into main with commit eff7863 Oct 6, 2026
94 checks passed
@miguel-heygen
miguel-heygen deleted the fix/preview-namespaces-svg-ids-in-loaded-sub-compositions branch October 6, 2026 11:20
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.

3 participants