Fix #6950, work around IE missing innerHTML on SVG nodes #6982
Conversation
ghost
added
|
This looks generally correct. Can we detect when .innerHTML is missing and only do this workaround in that case? Otherwise I think this is right. It might be hard to cover this with our unit test setup. |
|
That sounds a lot better, and makes things neater. I only added the |
| // the target node | ||
| if (typeof SVGElement !== 'undefined' && node instanceof SVGElement) { | ||
| reusableSVGContainer = reusableSVGContainer || document.createElement('div'); | ||
| reusableSVGContainer.innerHTML = '<svg>' + html + '</svg>'; |
There was a problem hiding this comment.
Am I missing something, why not create the svg directly and drop the div? EDIT: It seems to have innerHTML, but perhaps it doesn't actually work?
There was a problem hiding this comment.
IE does not support innerHTML on SVG Elements, so we wrap the markup to make it a 'complete' svg string, pop that into a temp container node, then copy across the children of the <svg>.
|
@joshhunt updated the pull request. |
|
@joshhunt updated the pull request. |
| @@ -28,7 +31,20 @@ var createMicrosoftUnsafeLocalFunction = require('createMicrosoftUnsafeLocalFunc | |||
| */ | |||
| var setInnerHTML = createMicrosoftUnsafeLocalFunction( | |||
| function(node, html) { | |||
| node.innerHTML = html; | |||
| if ('innerHTML' in node) { | |||
There was a problem hiding this comment.
@spicyj Perhaps it makes sense to check for the SVG namespace as well?
There was a problem hiding this comment.
Yeah let's do…
if (node.namespaceURI === DOMNamespaces.svg && !('innerHTML' in node)) {
// workaround
} else {
node.innerHTML = html;
}
Then I think this looks good if you have done manual testing that reproduces the original issue and confirms this fixes it.
There was a problem hiding this comment.
Hmm... is it possible that this could break due to improper namespaces or does HTML enforce the SVG namespace on descendants of an SVG-element? I think not? But perhaps the element does not work correctly either without a proper namespace?
There was a problem hiding this comment.
I don't believe SVG elements work with an incorrect namespace, and the parser should assign it automatically in most cases.
There was a problem hiding this comment.
what would be the advantage of checking the namespace as opposed to just checking if the node is a SVGElement?
There was a problem hiding this comment.
Either would be fine. Namespace works better in jsdom but if you prefer instanceof SVGElement that seems fine too.
|
@joshhunt updated the pull request. |
|
@joshhunt updated the pull request. |
|
@joshhunt updated the pull request. |
|
Looks great, thank you! |
…ebook#6982) * Workaround IE lacking innerHTML on SVG elements * Add tests for setInnerHTML * Correctly check if node has innerHTML property * Ensure tests for setInnerHTML actually tests both codepaths * Provide mock element for setInnerHTML tests * Only use SVG setInnerHTML workaround for SVG elements (cherry picked from commit 99d8524)
* Workaround IE lacking innerHTML on SVG elements * Add tests for setInnerHTML * Correctly check if node has innerHTML property * Ensure tests for setInnerHTML actually tests both codepaths * Provide mock element for setInnerHTML tests * Only use SVG setInnerHTML workaround for SVG elements (cherry picked from commit 99d8524)
| var newNodes = reusableSVGContainer.firstChild.childNodes; | ||
| for (var i = 0; i < newNodes.length; i++) { | ||
| node.appendChild(newNodes[i]); | ||
| } |
There was a problem hiding this comment.
As @jp7837 pointed out in #7358 (comment) - this is going to get every other child - childNodes is a live NodeList. This should be a while loop. I can confirm this is what is happening in IE11.
Fixes #6950, where
dangerouslySetInnerHTMLdoesn't work on any SVG tag. I updatedsetInnerHTMLto use the method hashed out by @spicyj, and updated DOMLazyTree's use ofnode.innerHTMLto usesetInnerHTMLinstead.I was hesitant about updating DOMLazyTree to use the
setInnerHTMLutil, but it seemed to make the most sense. If it's not appropriate I'll just make the workaround local to DOMLazyTree.If this looks good I'll go ahead and add some tests to make sure it continues to work.