Namespaced SVG function breaks existing classes in SVGs

[jhealey5]

I do a lot of animation with svgs, the recent craft update broke lots of things because classes have now got namespaces https://github.com/craftcms/cms/blob/develop/CHANGELOG-v3.md#3026---2018-09-29

It needs to make sure to not interfere with any existing classes (add a space).

What is the reasoning behind it in the first place? Why would those namespaced classes ever get used?

[brandonkelly]

The point is to avoid conflicts between the SVG’s classes (and IDs, which the function already was namespacing before that update) and other elements on the page.

It updates the class names and their references, so in theory shouldn’t actually have an effect on the visual appearance of the SVG. I may have overlooked something though; please upload your affected SVG image and I can look into it.

[meyerweb]

Here’s a basic example I ran into just this morning. I have an SVG like this (the real one is more complicated, but you’ll get the idea):

<svg version="1.1" id="Layer_1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" x="0px" y="0px" viewBox="0 0 300 41" enable-background="new 0 0 300 41" xml:space="preserve">
<defs>
<style>
.letters {fill: #222;}
</style>
</defs>
<g>
	<path class="letters" d="M65.455,31.615h-6.754l-1.967-5.688h-9.918l-1.924,5.688h-6.755l9.491-26.721h8.508L65.455,31.615z M51.817,10.152l-3.548,10.475h6.968L51.817,10.152z"/>
</g>
</svg>

On certain pages, I override the fill of the letters with something like:

header#main svg .letters {fill: #FFF;}

This got broken because the svg() function now turns all the classes into something like:

<path class="rpyvibhune-letters" d="M65.455,31.615h-6.754l-1.967-5.688h-9.918l-1.924,5.688h-6.755l9.491-26.721h8.508L65.455,31.615z    M51.817,10.152l-3.548,10.475h6.968L51.817,10.152z"></path>

So the selector no longer matches, and I end up with dark letters on a dark background instead of the intended white letters on a dark background.

I can work around this particular problem with attribute selectors, but I object to the behavior here. Craft should not be screwing around with attribute values like this, at least not by default. Anyone who did anything vaguely resembling what I did will have their CSS broken by it, as jhealey5 did. As he said, CSS-driven animations will be especially vulnerable to this, as will anyone’s attempt to style an SVG by referencing its ID.

[meyerweb]

Followup: I’d also like to understand the rationale here, since I assume this is driven by a different sort of need. Perhaps we can work out a path (pun totally intended) that satisfies everyone!

[brandonkelly]

Ah interesting… @jhealey5 are you referencing SVG classes in separate <style> tags or CSS files as well? That isn’t something I anticipated and certainly would be affected by the change.

@meyerweb The reason the function does this is because SVGs tend to be generated by apps like Illustrator or Sketch, which tend to reuse the same class names and IDs for the nodes. If you have a page that includes multiple SVGs generated by the same app, there’s a good chance that their classes/IDs will conflict with each other, so styles that were defined “in” SVG A and meant for nodes within that, may end up applying to nodes within SVG B as well. Namespacing the IDs and class names helps avoid that. (I put “in” in quotes because once you embed an SVG into HTML, it’s really just part of the rest of the HTML doc, which is why the styles can end up overreaching).

Anyway, clearly there needs to be a way to disable the auto-namespacing. I’ll add a parameter to do just that…

[brandonkelly]

…and done. As of the next release you’ll be able to set namespace=false to prevent svg() from namespacing your IDs/classes.

{{ svg('@webroot/path/to/image.svg', namespace=false) }}

If you’re comfortable with Terminal/Composer, you can get this change right away by updating your craftcms/cms version requirement in composer.json:

"require": {
    "craftcms/cms": "dev-develop#ae61eb2d9020ff0d028b6e81bdbc611cd641734c as 3.0.26.1",
    "...": "..."
},

Then go to your project in Terminal and run composer update.

[jhealey5]

@brandonkelly Thanks for the fix! I primarily manually add classes to svgs for animations, so I can target paths/etc with JS/CSS, and/or overwrite things on certain pages.

Though I second what @meyerweb said, whilst it is a nice idea, I don't think it's Crafts place to change SVG's, any duplicate attributes should probably be handled by the developer (or Sketch needs to sort out namespacing). That being said, I've not seen Sketch use classes at all, just ID's for referencing and styling.

[brandonkelly]

I don't think it's Crafts place to change SVG's, any duplicate attributes should probably be handled by the developer (or Sketch needs to sort out namespacing).

@jhealey5 The svg() method is usually used to embed user-supplied SVGs (e.g. uploaded via an Assets field somewhere), where the developer doesn’t have a chance to go in and set unique class names/IDs. (That’s also why the function runs SVGs through svg-sanitize by default – because there’s a chance that someone uploaded a malicious SVG that contains scripts that could attempt an XSS attack.)

[jhealey5]

@brandonkelly Ah that makes sense, I didn't think of user uploaded svgs. Nicely done then (now we have an optional parameter) :)

[brandonkelly]

Thought a bit more about this and came up with a better solution. Now if a file path (or an alias) is passed in, the svg() function will not sanitize or namespace the SVG document by default, as there’s a pretty good chance that the file can be trusted in that case.

[meyerweb]

Now if a file path (or an alias) is passed in, the svg() function will not sanitize or namespace the SVG document by default, as there’s a pretty good chance that the file can be trusted in that case.

Hm, interesting. Given this, will the default of namespace change to false?

[brandonkelly]

@meyerweb It changes to null, which means the function should decide for itself whether to apply a namespace, based on whether a file path or actual SVG file/contents were passed in.

cms/src/web/twig/Extension.php

Lines 818 to 830 in 2bfdad1

	/**
	* Returns the contents of a given SVG file.
	*
	* @param string|Asset $svg An SVG asset, a file path, or raw SVG markup
	* @param bool|null $sanitize Whether the SVG should be sanitized of potentially
	* malicious scripts. By default the SVG will only be sanitized if an asset
	* or markup is passed in. (File paths are assumed to be safe.)
	* @param bool|null $namespace Whether class names and IDs within the SVG
	* should be namespaced to avoid conflicts with other elements in the DOM.
	* By default the SVG will only be namespaced if an asset or markup is passed in.
	* @return \Twig_Markup|string
	*/
	public function svgFunction($svg, bool $sanitize = null, bool $namespace = null)

You can still explicitly enable/disable namespacing by adding namespace=true or namespace=false, but I’m guessing the new default behavior will work for people 99% of the time.