fix(createNode): recreate id-bearing nodes with createElementNS so inline SVG survives morphs#154
Open
guoliu wants to merge 1 commit into
Open
Conversation
…line SVG survives morphs createNode has two insertion paths. The stateless branch clones with document.importNode, which preserves the source node's namespace. The id-preserving branch, however, rebuilt its dummy with document.createElement(newChild.tagName), which ALWAYS creates an HTML-namespaced element. A rebuilt inline <svg> is therefore not an SVGElement, so morphAttributes' plain setAttribute folds its case-sensitive names (viewBox -> viewbox, clipPathUnits, ...) and the graphic renders blank even though it still shows in the DOM inspector. This only bites id-bearing SVG subtrees, which is a sharp trap: the library's own guidance (bigskysoftware#57) is to ADD an id to preserve state across a morph, precisely what can route a node into this branch. Fix: recreate the node in its OWN namespace with document.createElementNS(newChild.namespaceURI, newChild.localName). - namespaceURI: SVG/MathML get their real namespace, so setAttribute no longer folds case-sensitive names. - localName, not tagName: createElementNS does NOT case-normalize its qualified-name argument, and tagName is UPPERCASE for HTML ("NAV"). localName is correct in every namespace, and createElementNS(htmlNS, "nav") yields an ordinary HTMLElement identical to createElement("nav"), so HTML behavior is unchanged. morphdom fixed the same class of bug with namespace-aware node creation (morphdom bigskysoftware#34 / PR bigskysoftware#61). dist/ is intentionally left untouched: this repo regenerates dist/ only at release time (see RELEASE.md), so contributor changes touch src/ only.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Inline SVG (and any foreign/MathML content) can silently lose its namespace when it is rebuilt through
createNode's id-preserving branch. The<svg>becomes an HTML-namespaced element (anHTMLUnknownElement), somorphAttributes' plainsetAttributefolds its case-sensitive attribute names (viewBox→viewbox,clipPathUnits,preserveAspectRatio, …) and the graphic renders blank even though it still appears in the DOM inspector.It only bites id-bearing SVG subtrees, which is a sharp trap: the library's own guidance (#57) is to add an
idto preserve state across a morph — precisely what can route a node into this branch.Root cause
createNodehas two insertion paths:The stateless branch uses
importNode, which preserves the source node's namespace — so most inline SVG survives. The id branch rebuilt the dummy withdocument.createElement(tagName), which always creates an HTML-namespaced element. A rebuilt<svg>is therefore not anSVGElement, andsetAttributeon an HTML-namespaced element folds camelCase names.Fix
Recreate the node in its own namespace:
Two details are load-bearing:
namespaceURI— read off the already-parsed source node; SVG/MathML get their real namespace, so the element is a properSVGElement/MathMLElementandsetAttributeno longer folds case-sensitive names.localName, nottagName—createElementNSdoes not case-normalize its qualified-name argument (unlikecreateElement).tagNameis UPPERCASE for HTML elements ("NAV"), so keying on it would create uppercase-localName HTML nodes (<NAV>) that miss lowercase type-selector CSS.localNameis correct in every namespace:"nav"for HTML,"clipPath"for SVG.createElementNS(htmlNS, "nav")yields an ordinaryHTMLElementidentical tocreateElement("nav"), so HTML behavior is unchanged.This mirrors how morphdom handles the same class of bug (namespace-aware node creation; morphdom #34 / PR #61).
Verification — read this carefully
I want to be precise about where this reproduces, because it matters for how to treat the added test.
<svg>containing a<clipPath id="…">went invisible after the first morph and stayed gone until a full reload; with this patch itsviewBoxsurvives repeated morphs and the icon renders. This is the concrete failure that motivated the fix.web-test-runner(headless Chrome) and Playwright WebKit — and instrumented the id branch. In both engines an id-bearing inline<svg>is id-matched and moved (or cloned viaimportNode) rather than rebuilt through thecreateElementdummy; only the surrounding HTML wrapper (<section>) actually takes the dummy path, and that is harmlessly HTML-namespaced (http://www.w3.org/1999/xhtml). So thecreateElement → HTML-namespacefold is real but latent in those engines with the fixtures I could construct — it surfaced in the WKWebView app.importNodeand jsdom doesn't foldviewBox). A jsdom/headless regression test would pass with or without the fix and would be misleading, so I did not add one.Tests
npm run typecheck— passes.npx prettier --checkon the changed files — passes.web-test-runner(headless Chrome) andweb-test-runner --playwright --browsers webkit— the added test passes; the rest of the suite is green except 4 pre-existing failures in "Option to forcibly restore focus after morph" that also fail on unmodifiedmainin this environment (headless-Chrome selection-state behavior), i.e. unrelated to this change.test/core.js(describe("Foreign (SVG) namespace")) asserting an id-bearing inline<svg>keepsnamespaceURI === "http://www.w3.org/2000/svg"and itsviewBoxacross a morph. Its comment states plainly that it does not discriminate the fix in the headless suites (it passes with and without the patch there) and documents why — it stands as an executable statement of the intended invariant and a guard for any engine that does route foreign content through the dummy. Happy to drop or relocate it if you'd prefer.dist/Left untouched on purpose.
dist/in this repo is regenerated only at release time (RELEASE.mdstep 3,npm run dist); recent bugfix commits (e.g.8c80098,afd144f) touchsrc/only, and the committeddist/is already a fewsrc/commits behindmain. Rebuilding it here would pull unrelated changes into this PR, so I've kept this tosrc/+ the test to match the project's convention. Glad to regeneratedist/if you'd rather it ship in the PR.DOM references
Document.createElementalways produces an element in the HTML namespace and lowercases its argument;Document.createElementNS(ns, qualifiedName)uses the given namespace and does not case-normalize (MDNDocument.createElementNS).setAttributelowercases the qualified name only when the element is in the HTML namespace in an HTML document — which is exactly why an HTML-namespaced<svg>foldsviewBox.Related
document.bodywhen morphing withignoreActiveValue: true#34 / PR Move matching nodes, rather than deleting up to them #61 — the same class of fix in a peer library.