Repository navigation
fix(core): preserve SVG icons in loaded compositions - #5096
miguel-heygen merged 5 commits into
Conversation
Edit accuracy: accurate 2055 (base branch 2055), smooth 1603 of thoseThe gate passes. Quarantined, measured but not gated (0) |
terencecho
left a comment
There was a problem hiding this comment.
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 inwrapScopedCompositionScript; external<script src>payloads are injected directly at lines 574–575. With two scenes definingclipPath#clipand only external scripts, a deferredsvg.querySelector('#clip')in the second scene returnsnullafter its ID is renamed. In an identical Chromium fixture against the PR parent0d5e395, it returnedclip; at this head the result isnullandrefreshInstalledis 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">tohref="#grid--symbol", but the selector shim deliberately does not rewrite tokens inside[href="#symbol"]. A deferred wrapped inline script'suse[href="#symbol"]lookup returned true against the parent and false at this head in Chromium (while its#cliplookup 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).mountCompositionContentexecutes scripts beforeloadExternalCompositions/loadInlineTemplateCompositionsresolve, then this new pass renames IDs. A script that capturesclipPath.idduring mount and writesclip-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 atsvgIdNamespacing.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#fxselector to the new SVG ID. The div retainsid="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)
…cenes rename them
|
Thanks for the four repros. Addressed at
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
left a comment
There was a problem hiding this comment.
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:
- External-only scripts lose authored-ID lookups: resolved. The alias service is now a standalone module (
compiler/svgSelectorAliases.ts). The loader callsrefreshSvgSelectorAliases()at the end of everynamespaceMountedSvgIdspass (compositionLoader.ts:919), whether or not a wrapped inline script ran. Covered by the Chromium case "external-only scripts retain authored SVG selectors". - Rewritten
hrefbreaks[href="#symbol"]lookups: resolved. Each reference write is recorded indata-hf-svg-reference-aliases, andrewriteSvgSelectorsexpands a matching attribute predicate to:is(<authored>, [href="<renamed>"])for both thequerySelectorproxy and<style>text. Covered by "authored native-reference attribute selectors work in scripts and CSS", plus the operator, flag, escape and namespace cases. - Scripts capture an ID the later pass renames: resolved by ordering, not patching.
mountCompositionContentnow returnsrunScriptsinstead of running scripts during mount.loadCompositionsnamespaces every initial mount, external and inline, before any scene script runs, andfinalizedIdskeeps those elements fixed in the post-script pass. Covered by "scripts capture the final SVG id before a deferred reference write", using the HTMLdiv#clipkeeper from the original trace. - Scope-wide CSS rewrite drops rules for an unchanged HTML id: resolved.
#fxnow 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.
namespaceCollidingSvgIdsused to return early when nothing collided (hasAnyCollision). It now always runsapplyRenames.rewriteElementIdReferencesthen records an identity alias for everyhref="#…"/url(#…)reference, even whenbefore === after. A probe at this head, with one scope, no collisions and an empty id map, wrotedata-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:decideReferenceWritederivesbeforefrom the current value when there is no prior record, andrefresh()filtersbefore === afteranyway. 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
left a comment
There was a problem hiding this comment.
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#idlookups 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)
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 bycheck:svg-selector-aliasesin coretest). Its text no longer depends on the transpiler that runs the compiler, so atsxrun 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
@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@namespacerules 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.<link rel="stylesheet">that selects a renamed SVG ID orhrefstill 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.querySelectorandquerySelectorAllon elements follow renamed references.closest,matchesand document-level queries from unwrapped external scripts see the renamed values.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.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.tsx, neither the installer artifact nor a wrapped scene script contains__name(."use strict"reach the authored script, letting malformed alias JSON throw, and accepting a wrong-typed alias entry.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.
After
All 16 icons render. Each tile's clipping path now resolves to its own renamed definition in the visible grid scene.