Multiple of the same ids in a differents files

[pastjean]

What if there are multiple of the same IDs in a different file, there would be duplicate ids

[pastjean]

svgo does something similar https://github.com/svg/svgo/blob/master/plugins/cleanupIDs.js

[w0rm]

@pastjean can we accomplish similar with cheerio?

[pastjean]

I guess so, (i've just started playing with cheerio yesterday while fixing my regression on svgstore :tongue:

something like getting all elements with id's with id's $('[id]') all element with xlink:href with $('[xlink:href]) and elements with style that references url (probably a style parser would be needed here) then rename the ids and all xlinkhref,style accordingly.

[w0rm]

Yeah, having to parse the styles kinda holds me back from implementing this feature. I also checked svgo code, if only they allowed to specify prefix for the ids. Maybe we shouldn't solve this issue, but rather somehow create a new plugin for svgo and run it as a gulp task before passing to gulp-svgstore.

[pastjean]

I would rather do that! Sometimes you want the duplicated ids so you can reuse some elements inside of others or you don't want the renaming to occur

Say

file 1 :

<svg>
  <symbol id="niceshape" />
    <rect/>
  </symbol>
  <use xlink:href="niceshape"/>
</svg>

file 2 (redefines same symbol):

<svg>
  <symbol id="niceshape" />
    <rect/>
  </symbol>
  <use xlink:href="niceshape"/>
</svg>

then you filter with cheerio in gulp to remove duplicate symbols

renaming aggressively would make this impossible (of course could be an option but then... better a separate gulp plugin)

[Jakobud]

What do you think of grunt-svgstore's approach for solving this problem? They append a file hash to each ID in order to make sure its unique within each SVG symbol.

[w0rm]

@Jakobud this is not an issue, a hash or a filename will make ids unique. The most difficult task is to change all the references in styles and attributes.

[Jakobud]

It wouldn't be hard to do a find/replace in the content. Just find all unique IDs, append hash to each one, and then go through and replace the string everywhere they are used. Would you be interested in a PR that does this? Honestly this would be fairly easy to whip up if you are interested.

[w0rm]

@Jakobud it's not that simple, these occurrences should be aware of the context, you definitely don't want to replace it inside the text node.

[Jakobud]

Maybe we are talking about different issues here... I'm not sure. What I'm talking about is when you have something like a <linearGradient id="whatever".... in more than one of your SVGs. After svgstore combines everything you now have multiple linearGradients with the same id and when you try to use them like fill="url(#whatever)". And the SVGs end up using the wrong linear gradient since IDs need to be unique. What exactly are you talking about regarding text nodes?

[w0rm]

whatever may occur in different places, but not all of them have to be replaced

[Jakobud]

Can you give an example of where #whatever is used but you wouldn't want to change it? Do you mean like

<text>This is my #whatever text</text>

? If we can come up with a small list of examples it wouldn't be too hard to write this and tests for it as well.

[w0rm]

I think it is easier to list all the places where it should be changed (or just check what svgo does). But for this we need to parse inline css. I still doubt if it should be a part of svgstore, or a separate svgo plugin similar to https://github.com/svg/svgo/blob/master/plugins/cleanupIDs.js

[Jakobud]

Looks like grunt-svgstore fixes this problem like this:

https://github.com/FWeinb/grunt-svgstore/blob/master/tasks/svgstore.js#L143

It finds url(#...) and xlink:href=# and replaces the id's with a unique id.

And here

https://github.com/FWeinb/grunt-svgstore/blob/master/tasks/svgstore.js#L140

is there it changes the id of the element to the unique id.

[Jakobud]

I still doubt if it should be a part of svgstore, or a separate svgo plugin

I have to disagree with this I think. The problem that we are facing is a direct result of concatenating multiple SVGs together via svgstore. Any one out there who uses a vector program like Illustrator to make multiple SVGs that have elements with ids in them will face the same issue once they use svgstore to combine them. Solving the problem by making a plugin for svgo seems like you would be requiring the user to install another dependency just to fix a problem in svgstore.

This is a great gulp package. I really like it a lot and I'm personally and professionally relying on it for web projects. I just want to do everything I can to make it perfect. The grunt version has this issue figured out and I think the same could be applied here. I'm going to work on it this weekend and see what you think.

[Jakobud]

Okay I threw this together tonight

https://github.com/Jakobud/gulp-svgstore/commit/fed910afd5c57aa74f38a585796aa9d7efc49275

It's actually really simple. For each SVG, before concatenation, it looks for any id's that are referenced using url(#someid). It then replaces all instances of someid with idAttr-someid. So it basically just appends the filename to the front of the id. So for circle-with-gradient.svg that has <linearGradient id="SVGID_1_" ... every instance of SVGID_1_ gets replaced with circle-with-gradient-SVGID_1_. This ensures that the element with that ID will only be referenced within that symbol after all the SVGs are concatenated.

This approach is also nice because it does not blatantly replace ids that are not referenced elsewhere. For example if you had <circle id=mycircle...> as long as mycircle isn't referenced elsewhere in the SVG, then it's left alone. This is important for when you want to select/alter/animate individual SVG elements by it's ID in a webpage. Obviously altering that id would be undesirable.

It seems to work great for everything I have thrown at it so far. I have tried it with gradients, filters, etc. Can you guys think of any other test cases I can throw at this? I'm working on putting together a test for it.

Thoughts? Opinions? I think it's a pretty simple but solid approach.

[w0rm]

@Jakobud what if there is a <style> tag that styles element based on its id? Maybe we can ignore this, but then we need to provide a hint to the user. Lets start with tests, but include false positive matches, e.g. same id, but in the places where it shouldn't be replaced. Your <text>This is my #whatever text</text> is a good example. Also, some attributes where arbitrary text may be put, e.g. xlink:title.

[Jakobud]

I think the <text> case won't be difficult to handle. I just need to change my global search/replace to instead just search/replace on element attributes instead. The style tag could be tricky. For example, can you do this?

<linearGradient id="mygradient"...>...</linearGradient>
<style>
  circle {
    fill: url(#mygradient);
  }
</style>

That would be tricky to take care of without introducing a css parser I think.... I'll mess around with this.

[Jakobud]

https://github.com/Jakobud/gulp-svgstore/commit/a245467b3a02efa2ad35b14a219f884d0ff758ce

Okay added an update that takes care of text in <text> and id's in <style> tags. Still investigating use cases of xlink:title and trying to figure out your tests. Some tests on the master branch were failing for me before I made any changes.

[Jakobud]

Okay so I think any type of thing that uses xlink:title should be covered because it's an attribute just like any other attribute:

id="#someid"
fill="url(#someid)"
style="fill:url(#someid)"
etc

[Jakobud]

Okay I made some changes and added several tests

https://github.com/Jakobud/gulp-svgstore/commits/feature/unique-element-ids

The tests are:

Should rename referenced ids in element attributes Should rename referenced ids from definition tags Should rename referenced ids in style tags Should not rename id in text elements Should not rename un-referenced ids

It's working very well so far. Do you guys see any other coverage I need to take care of?

[w0rm]

@Jakobud I am really worried of merging this change, because it may potentially break svg.

1. What if the id that you want to rename is only a part of the actual id, e.g. you need to replace SVGID_1_ with circle-with-id-SVGID_1_, but there is already SVGID_1_something, that will be mistakenly replaced with circle-with-id-SVGID_1_something.
2. If the id is url, will then fill="url(#SVGID_1_)" become fill="circle-with-id-url(#SVGID_1_)"?
3. The intention of only replacing referenced ids contradicts with the task of making all ids unique, i.e. we may get duplicated ids in the result, if they were un-referenced.

I suggest you to implement a separate npm package that exports one function renameIds that accepts svg as cheerio object, and rename function to be called on each id. Then you can pipe it through gulp-cheerio like this:

gulp.task('svg', function () {

  return gulp
    .src('test/src/*.svg')
    .pipe(cheerio({
      run: function ($, file) {
        renameIds($, function (id) {
          return path.basename(file.relative, path.extname(file.relative)) + '-' + id;
        })
      },
      parserOptions: { xmlMode: true }
    }))
    .pipe(svgstore())
    .pipe(gulp.dest('test/dest'))

})

As soon as it is stable enough, we can use it inside gulp-svgstore.

[Jakobud]

1. That is a use case I need to adjust the code for. Thanks,
2. No, it renamed it correctly as you can see in the tests
3. This is maybe worth a discussion but I don't think changing un-referenced ids makes sense. Un-referenced id's don't cause any problems that I can see. It's the non-unique referenced id's that svgstore doesn't handle well, which is what I'm trying to fix. Also, when someone is using an svg with un-referenced ids, they may be planning on using those ids to programmatically animate those elements using CSS or something like that (I know I have done this). So changing those might be frustrating to the user cause now they have to go into their source and change them there too. Changing referenced ids seems to be fine though because it's something that the user never needs to know or care about...

One possible exception I can think of is if a user was programmatically changing an element fromfill="url(#SVGID_1_)" to fill="url(#SVGID_2_)" or something like that. If svgstore changes the ids then the user would need to know about that ahead of time. I think thats an edge case but it's probably worth a discussion.

I'm a little frustrated here. You seem to be pretty hesitant to want to solve this problem. I mean it's your project and that's fine, but do you understand that svgstore generates broken svgs in the case of non-unique referenced ids? I don't even know if it's valid to the svg spec to have multiple referenced elements with the same id in one svg. Take a look here for a simple example:

http://jsbin.com/wecekodeqo/edit?html,output

The first circle is the wrong color because the second linearGradient had the same id as the first one and therefore takes priority. The ids are used globally within the svg, not just within each symbol.

When anyone uses something like Adobe Illustrator or Inkscape or whatever to generate SVGs, things like <linearGradient>s are always (automatically) named stuff like, SVGID_1_, SVGID_2_, SVGID_3_, etc. So when svgstore combined multiple files with these same exact ids, you have problems. The absolutely only solution is to manually go through an SVG in an editor and rename the IDs to something unique every time you generate a new one. This especially sucks when you have to make a change to an SVG in Illustrator and save out a new one. Time to go manually rename all those ids again so that they work...

Like I said, it's your project and you can do whatever you want with it. It's a great gulp plugin. But I'm going to make some more improvements to what I'm working on but I'm pretty much done as it is passing my tests and working perfectly for everything I have been throwing at it so far. I'll just be using my branch for my projects. Thank you for the great Gulp plugin.

[w0rm]

@Jakobud I'm really sorry for frustrating you. I completely understand the issue that you're trying to solve. My only concern is merging something unstable into the project, and having to maintain this afterwards for other use cases, like point 1. Thats why I suggested a safer path of doing this, by splitting functionality and maintaining it separately.

What about doing it the way I proposed, and I will integrate such module in gulp-svgstore from the day 1, but make it optional?

[w0rm]

Because svgo already has cleanupIDs plugin that supports prefixes, this seems the best way of solving the issue without having to reinvent the bicycle:

var gulp = require('gulp');
var SVGO = require('svgo');
var svgstore = require('gulp-svgstore');
var stream = require('stream');
var path = require('path');

function prefixIds () {
    var st = new stream.Transform({objectMode: true});
    st._transform = function (file, enc, done) {
        var data = file.contents.toString();
        var prefix = path.basename(file.relative, path.extname(file.relative));
        var svgo = new SVGO({
            plugins: [{
                cleanupIDs: {
                    prefix: prefix + '-',
                    minify: true
                }
            }]
        });
        svgo.optimize(data, function (result) {
            file.contents = new Buffer(result.data);
            done(null, file);
        });
    }
    return st;
}

gulp.task('default', function () {
    gulp.src('svg/*.svg')
        .pipe(prefixIds())
        .pipe(svgstore())
        .pipe(gulp.dest('.'))
});

Bonus point, this will not only prefix ids, but can also minify them. Below is the output when using svgs from @Jakobud tests:

<svg xmlns="http://www.w3.org/2000/svg">
  <defs>
    <linearGradient id="circle-a" y2="100%">
      <stop offset="0" stop-color="#FFF"/>
      <stop offset="1"/>
    </linearGradient>
    <linearGradient id="square-a" y2="100%">
      <stop offset="0" stop-color="#FFF"/>
      <stop offset="1"/>
    </linearGradient>
  </defs>
  <symbol id="circle">
    <circle fill="url(#circle-a)" cx="2" cy="2" r="1"/>
  </symbol>
  <symbol id="square">
    <path fill="url(#square-a)" d="M1 1h2v2H1z"/>
  </symbol>
</svg>

[Jakobud]

Yeah that looks like a good way of doing this. I didn't realize it had the potential to append to ids. Good call!

[w0rm]

@Jakobud yeah svgo is really powerful and minifying step is needed anyway in svg pipeline. I wonder if this should be in its own gulp plugin, because it needs to configure svgo on a per-file basis.

Can be done in a pull request to @ben-eb gulp-svgmin, that allows passing a function that receives file and returns svgo options, so something like this would be possible:

gulp.src('svg/*.svg')
.pipe(svgmin(function (file) {
    var prefix = path.basename(file.relative, path.extname(file.relative));
    return {
        plugins: [{
            cleanupIDs: {
                prefix: prefix + '-',
                minify: true
            }
        }]
    }
}))
.pipe(svgstore())
.pipe(gulp.dest('.'))

[ben-eb]

Sure, why not. Send a PR with the necessary improvements. :smile:

[Jakobud]

Hmmm is there no way to grab the basename of the current file in the stream to pass it to a gulp plugin? Seems like there would be a way to do something like that...

[w0rm]

@Jakobud plugin creates a transform stream with predefined svgo instance, it needs a callback per transformation for a custom behavior. I see no other way of doing this

[w0rm]

@ben-eb I sent PR, will update gulp-svgstore readme when you publish it, and then close this issue.

[Jakobud]

Thanks, sorry I didn't have a chance to put together a PR for that. I was trying to figure out a way to pass a closure as the prefix name and pull in the current source file basename. But I guess maybe you can't do that with gulp.

[Jakobud]

By the way, does that cleanupIDs plugin rename ALL ids? Even non-referenced ones?

[w0rm]

@Jakobud I think that by default it even removes unused ids. It can be turned off, but then it will minify and prefix all of them. You can try to implement your own svgo plugin, based on https://github.com/svg/svgo/blob/master/plugins/cleanupIDs.js

[Jakobud]

Ah okay. Yeah removing unused ids could be undesirable in the case that you are trying to select certain elements to color or animate them using CSS.

[w0rm]

That's a bit strange use case, do you inject combined file into html and then style it from page's CSS?

I'd rather not alter symbols' contents with CSS, there are several options, you can remove fills, and they will inherit from svg tag that contains use tag. If you need to color different elements, you may separate them into files that will become symbols, and then put multiple use tags inside your svg tag.

[Jakobud]

Yeah just pull in your SVG sprite into your html via ajax (and optionally store/load it in localstorage).

Also, you don't need to remove fills if you do stuff like use path { fill: inherit; }. That will allow you to override the fills that are defined in the symbol.

http://tympanus.net/codrops/2015/07/16/styling-svg-use-content-css/

[w0rm]

@Jakobud restyling all paths isn't flexible either, sometimes I need to change the color of some paths, but keep the rest. It will also break when in the future all browsers will support linking to symbols from external svg.

[Jakobud]

ids and classes are how I target specific paths and elements.

[w0rm]

Depends on what do you want to style, if you target contents of svgstored symbols from page's css, then you won't be able to change fill later in different places where you use them.