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

CDATA in SVG script causing trouble #88

Closed
cyqsimon opened this issue Jul 21, 2020 · 6 comments · Fixed by #122
Closed

CDATA in SVG script causing trouble #88

cyqsimon opened this issue Jul 21, 2020 · 6 comments · Fixed by #122
Labels

Comments

@cyqsimon
Copy link

cyqsimon commented Jul 21, 2020

The version of svgo used by htmlnano has issues with parsing script tags.
When using both minifyJS, and minifySvg, there's an issue with parsing script tags inside svg tags.
Specifically, CDATA declaration is removed when it is necessary:

<script>//<![CDATA[  //]]></script>

is parsed into:

<script></script>

This causes SVGs like this to fail validation:

<svg xmlns='http://www.w3.org/2000/svg'>
  <script>//<![CDATA[
    const x = '<>';
  //]]></script>
</svg>

with the error:

Trace: Error in parsing SVG: Unencoded <

This is not an issue of svgo:

svgo -s "<svg xmlns='http://www.w3.org/2000/svg'><script>//<![CDATA[const x='<>';//]]></script></svg>"

returns:

<svg xmlns="http://www.w3.org/2000/svg"><script>//<![CDATA[const x='<>';//]]></script></svg>
@cyqsimon
Copy link
Author

It is also possible that htmlnano removed CDATA before invoking svgo. However I cannot be certain of this without reading your code.

@cyqsimon cyqsimon changed the title [Bug] Please update svgo [Bug] CDATA in SVG script causing trouble Jul 21, 2020
@cyqsimon
Copy link
Author

Update: I think I found the issue. It actually has nothing to do with svgo.
minifyJs is invoked before minifySvg, but minifyJS seems to look inside svg tags for script tags as well. So it is actually minifyJs that's removing CDATA, thereby causing minifySvg to throw.

A potential solution is to exclude svg tags from the application scope of minifyJs module, if that's possible.

@maltsev
Copy link
Member

maltsev commented Jul 21, 2020

Thanks for the detailed info! I'll look into it in the following days.

@maltsev
Copy link
Member

maltsev commented Jul 25, 2020

For now, I've found no better solution than check for //<![CDATA[ //]] and then add it manually after the JS minification.

@Scrum if I have a node (e.g., <script>) is it possible to detect if one of its ancestors is <svg>?

@Scrum
Copy link
Member

Scrum commented Jul 27, 2020

@maltsev Hi, from the options I can suggest making a match for the svg element and in this match make a match on script

let me know if it's not very clear, I'll sketch an example

@cyqsimon
Copy link
Author

It might be a good idea to check for namespace switching instead of hard-coding for svg only.

I'm thinking of the possibility of some other element using a strict XML syntax for its internal DOM just like SVG, although I cannot come up with a concrete example at the moment.

@maltsev maltsev added the bug label Aug 8, 2020
@maltsev maltsev changed the title [Bug] CDATA in SVG script causing trouble CDATA in SVG script causing trouble Aug 8, 2020
SukkaW added a commit to SukkaW/htmlnano that referenced this issue Nov 15, 2020
SukkaW added a commit to SukkaW/htmlnano that referenced this issue Nov 15, 2020
maltsev added a commit that referenced this issue Nov 16, 2020
Fix: #88 & CDATA inside script/style
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants