Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Skip unsupported nodes even if they have text value #113

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

skozin
Copy link

@skozin skozin commented Feb 1, 2018

This PR fixes #112 and #97. It also removes the code added in #76 to fix #84, as it's no longer needed.

The main cause of both issues is that inspectNode() function fails to exclude children of unsupported node types when they have some value. This leads to nodes of type #text (#84) and #comment (#97, #112) being included into the resulting document as raw text, which, in turn, produces the Raw text cannot be used outside of a <Text> tag error.

Here's what happens: in the very beginning of inspectNode() function, there is a check for inclusion of the node type into ACCEPTED_SVG_ELEMENTS array. If it's not inside the array, null is returned. In a loop inside the same function, there is another check that doesn't include child into the resulting array if recursive call to inspectNode() returned null, so nodes of unsupported types get skipped.

But, inside the same loop, there is a code that adds child node's value into the children array if the node has nodeValue property set. It runs before recursing into inspectNode(), so nodes with nodeValue set get included into resulting array even if their type is not inside ACCEPTED_SVG_ELEMENTS array.

This PR fixes the issue by making sure that the check for whether node type is supported runs for all child nodes, regardless of whether they have nodeValue set.

@compojoom
Copy link

Just tested it and it works. Hope that it gets merged soon

@skozin skozin mentioned this pull request Feb 15, 2018
@rori4
Copy link

rori4 commented Feb 19, 2018

This still has issue with the rendering of text. When it goes to the inspectNode function it returns null because of the check of allowed elements
if (!ACCEPTED_SVG_ELEMENTS.includes(node.nodeName)) { return null; }
So i still get no render of text svg elements. A quick fix for this would be
if (!ACCEPTED_SVG_ELEMENTS.includes(node.nodeName) && !(node.parentNode.localName==="text")) { return null; }
I hope this helps someone as I was struggling for 2 hours to understand why i don't get text to render.

@skozin
Copy link
Author

skozin commented Feb 25, 2018

@rori4 can you please provide an example of SVG that doesn't render correctly? It will help me fixing this.

@HenryQz
Copy link

HenryQz commented Mar 12, 2018

please merge this commit early. i have a issue relative it 😓

@ammichael
Copy link

Why it isn't merged yet? It's causing trouble for bunch of people

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.

Error: raw text cannot be used inside of a <Text> tag Need updates
6 participants