innerHTML causes SVG rendering glitch when inserting nodes
plaidfinch opened this issue · 6 comments
Description
When diffHTML's innerHTML() function is used to modify an <svg> element in the page, diffs which require a new node to be inserted to the SVG's DOM (such as changing an element's tag) will fail to properly re-render the SVG in the browser's view, resulting in a blank image being displayed instead.
I've put together an interactive example demonstrating this behavior on Glitch: https://diffhtml-innerhtml-svg-bug-demo-1.glitch.me. There is a bit of UI and additional explanation there to demonstrate when the bug does and does not occur. In particular, as shown there, updates which only cause an attribute to be altered (such as changing an element's color) will render just fine.
I am very happy to continue to help debug this issue, as we are using diffHTML for a project at my workplace, and our current application runs afoul of this bug. If you have pointers in the right direction to solve this issue, I would be happy to put together a PR if I can get to the root of it.
EDIT: The examples hosted on Glitch have been altered to pin the version to the one which exhibits the bug. The example below, now that the bug is resolved, should now work.
Platforms
This issue has been replicated in Firefox and Chrome on macOS, and Firefox on Linux. I have not yet had a chance to test it on Windows.
Minimal Demonstrating Example
A minimal example showing only the bug is the following complete webpage (also live at https://diffhtml-innerhtml-svg-bug-demo-1-minimal.glitch.me):
<!DOCTYPE html>
<html>
<head>
<style>
.picture {
border: 1px solid black;
width: 100px;
height: 100px;
}
</style>
<script type="module">
import { innerHTML } from "https://diffhtml.org/es";
// A picture of a blue circle, as an SVG
const circle = `
<svg height="100px"
width="100px">
<circle cx="50" cy="50" r="50" stroke="none" fill="blue" />
</svg>
`;
// A picture of a blue square, as an SVG
const square = `
<svg height="100px"
width="100px">
<rect width="100" height="100" stroke="none" fill="blue" />
</svg>
`;
// Use diffHTML to set the picture to a square, then a circle
const pictureUsingDiff = document.getElementById("diff-picture");
innerHTML(pictureUsingDiff, square);
innerHTML(pictureUsingDiff, circle);
// Use .innerHTML = ... to set the picture to a square, then a circle
const pictureUsingSet = document.getElementById("set-picture");
pictureUsingSet.innerHTML = square;
pictureUsingSet.innerHTML = circle;
</script>
</head>
<body>
<div id="diff-picture" class="picture"></div>
<div id="set-picture" class="picture"></div>
</body>
</html>Observed Behavior
At present, this page looks like this:
Expected Behavior
However, I expect the behavior of diffHTML's innerHTML to match that of manually setting the .innerHTML. If the bug is corrected, the page should look like this:
Further Notes
This bug may be tricky to detect using automated testing, because even though the SVG does not correctly display after the alteration of the DOM via innerHTML, the DOM does in fact change, as you can observe in the interactive demonstrator.
Likewise, using "Inspect Element" on the minimal page above, we see that the DOM fragment corresponding to the page's body has been altered by diffHTML to be precisely what we would expect: two identical SVGs, which if rendered normally (without diffHTML) would appear as the latter image above:
<body style="">
<div id="diff-picture" class="picture">
<svg height="100px" width="100px">
<circle cx="50" cy="50" r="50" stroke="none" fill="blue"></circle>
</svg>
</div>
<div id="set-picture" class="picture">
<svg height="100px" width="100px">
<circle cx="50" cy="50" r="50" stroke="none" fill="blue"></circle>
</svg>
</div>
</body>An Attempt at Locating Possible Causes
I am not an expert JavaScript programmer, and I am not deeply familiar with the inner architecture of diffHTML, although I've done my best to understand it in the course of minimizing and reporting this bug. That being said, I'll offer my best guess as to the root cause, by explaining my reasoning as I walk through the codebase. Perhaps much of what I write below will be self-evident, or equally likely, wrong, to a seasoned contributor to this project—but hopefully some of it will save someone time!
My broad hypothesis: Could it be that under some circumstances, createElement is being mistakenly called inside an SVG, instead of createElementNS? According to this StackOverflow answer, the correct method of creating nodes within embedded SVGs is to use createElementNS with the appropriate namespace. The only occurrence of createElementNS in this repository is in the createNode function in packages/diffhtml/lib/node/create.js. This leads me to think that if we can locate a situation that causes isSVG to be erroneously false, we might solve the bug by ensuring the correct branch in the below snippet of createNode is always taken:
// ...
// If no DOM Node was provided by CreateNode hooks, we must create it
// ourselves.
if (domNode === null) {
// Create empty text elements. They will get filled in during the patch
// process.
if (nodeName === '#text') {
domNode = ownerDocument.createTextNode(vTree.nodeValue || '');
}
// Support dynamically creating document fragments.
else if (nodeName === '#document-fragment') {
domNode = ownerDocument.createDocumentFragment();
}
// Support SVG.
else if (isSVG) {
domNode = ownerDocument.createElementNS(namespace, rawNodeName);
}
// If not a Text or SVG Node, then create with the standard method.
else {
domNode = ownerDocument.createElement(rawNodeName);
}
}
// ...The value of isSVG in that function is determined by the logical-or of the input isSVG parameter and whether the node name is the literal string 'svg' (line 43):
isSVG = isSVG || nodeName === 'svg';First thought at possible cause: could it be that nodeName is not always normalized to lowercase? On further investigation, it looks like it is normalized at the creation time of a VTree via createTree:
entry.nodeName = typeof input === 'string' ? input.toLowerCase() : fragmentNameNote that this line does not end with a semi-colon. I don't know enough about JavaScript semantics to know whether this could cause unintended behavior here.
My guess is that the call to createNode we care about comes in the patchNode function:
export default function patchNode(patches, state = {}) {
const promises = [];
const { isSVG, ownerDocument } = state;The only call to patchNode that explicitly initializes its state comes in packages/diffhtml/lib/tasks/patch-node.js in the patch function:
export default function patch(transaction) {
const { domNode, state, state: { measure }, patches } = transaction;
/** @type {HTMLElement | DocumentFragment} */
const { nodeName, namespaceURI, ownerDocument } = (domNode);
const promises = transaction.promises || [];
const namespaceURIString = namespaceURI || '';
state.isSVG = nodeName.toLowerCase() === 'svg' || namespaceURIString.includes('svg');
state.ownerDocument = ownerDocument || document;
measure('patch node');
promises.push(...patchNode(patches, state));
measure('patch node');
transaction.promises = promises;
}Here is where the originating isSVG value is generated: by examining the input domNode in the given transaction, and determining if its toLowerCase()-normalized nodeName is 'svg'. We can then see in packages/diffhtml/lib/node/patch.js how createNode is called with this propagating isSVG value throughout the patchNode function... except in the PATCH_TYPE.NODE_VALUE case, where it is called with only one argument:
// ...
case PATCH_TYPE.NODE_VALUE: {
const vTree = patches[i + 1];
const nodeValue = patches[i + 2];
const oldValue = patches[i + 3];
i += 4;
const domNode = /** @type {HTMLElement} */ (
createNode(vTree)
);
// ...I'm not sure whether this patch type applies in the situation leading to this bug, but if so, could this be related to the issue?
That's about as far as I could get in one reading session before running out of mind-space. Thank you for your help, and for such a great library!
Hi @kwf, thanks for the fantastic issue reporting. This does indeed look like a bug with the isSVG check. When I ran the logger middleware, all the mutations appear correct, but when inspecting the namespaceURI property of both elements, I get different values.
// diff circle
$0.namespaceURI
"http://www.w3.org/1999/xhtml"
// innerHTML circle
$0.namespaceURI
"http://www.w3.org/2000/svg"This is sort of tricky to solve:
- We do not want to maintain a hardcoded list of SVG elements
- We will eagerly create elements when there are attribute / text changes, and may not know where the Node is intended (SVG or HTML element)
I think ultimately it may require a change to the VTree object structure in order to solve this, but I will experiment and get back to you once I find something satisfactory.
I came up with a fairly elegant solution that involves using a Set and tracking the VTree references there. Once it passes, I'll cut a new release as this does not change the public-facing API.
That's fantastic! I'd be very curious to understand how it works, if/when you have the time to write it up. :)
If this solution works, no need to pursue others, of course, but since I just thought of an alternate sketch of a resolution, I'll note it:
We will eagerly create elements when there are attribute / text changes, and may not know where the Node is intended (SVG or HTML element)
If the issue is fundamentally that you don't know what namespace to use when you create the element, but you do right before it's to be inserted, perhaps instead of creating Nodes eagerly, functions of type bool -> Node could be created and propagated, which when supplied with the isSVG context of the insertion site then create the appropriate node. That is, instead of directly returning a Node from the point where they are presently created, we might do something like:
return ((isSVG) => {
if (isSVG) {
// return an SVG node
} else {
// return an HTML node
});I'm not sure what the performance impact would be of allocating and GC-ing these additional closures, or of changing the node creation time (it might change the way the promise scheduling business works?). But it might also avoid the need to track extra information in an auxiliary data structure. Take or leave the thought of course—it's only 10 minutes old :)
Regardless, thank you very much for your swift investigation into this issue. I'm looking forward to slotting this piece into place!
Sweet if you force refresh your example in Glitch you will see it works fine now. Please keep filing issues. The more usage and coverage, the better.
The problem with deferring Node creation is that for transitions (such as attached, detached, replaced), we want to have all attributes and text already set so that you can do a seamless fade in or fade out without having the elements be in a weird state.
Looks good to me! I'll certainly keep filing issues if/when I encounter them. In my testing, I've seen a few sequences of alternations between element.innerHTML = ... and innerHTML(element, ...) that cause the latter method to stop having an effect. I haven't got a minimal example yet, but I'll see if I can make one (seems like it might be related to the cache getting in a weird state when the DOM gets clobbered by the direct assignment to .innerHTML?).
In terms of preventing regressions, perhaps we could make a test that exercises the sequence in the minimal example and checks that the namespaceURI of all sub-elements of the <svg> have the appropriate value.
Ah, I see you've already got those tests in place. Sweet!