Material: svg-icon: id mangling on cache destroys xhref references

[Lykathia]

When a svg with internal def ID references is created in an ephemeral element (dialog, bottomsheet, etc), every subsequent cached load causes the id references to fail lookup (as the original svg is no longer present to reference).

Raw SVG:

<svg viewBox="0 0 100 100" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" width="100" height="100">
<defs>
 <g id="bg" fill="#000000"><path d="M50,100C22.43,100,0,77.57,0,50C0,22.43,22.43,0,50,0c27.57,0,50,22.43,50,50C100,77.57,77.57,100,50,100z M50,2C23.533,2,2,23.533,2,50c0,26.468,21.533,48,48,48c26.468,0,48-21.532,48-48C98,23.533,76.468,2,50,2z"/></g> 
</defs>
<use x="0" y="0" class="bg" xlink:href="#bg"/>
</svg>

After material caching:

<svg viewBox="0 0 100 100" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" width="100%" height="100%" fit="" preserveAspectRatio="xMidYMid meet" focusable="false">
<defs>
 <g id="bg_cache53" fill="#000000"><path d="M50,100C22.43,100,0,77.57,0,50C0,22.43,22.43,0,50,0c27.57,0,50,22.43,50,50C100,77.57,77.57,100,50,100z M50,2C23.533,2,2,23.533,2,50c0,26.468,21.533,48,48,48c26.468,0,48-21.532,48-48C98,23.533,76.468,2,50,2z"></path></g> 
</defs>
<use x="0" y="0" class="bg" xlink:href="#bg"></use>
</svg>

Note that the xlink:href tags now point to an id that no longer exists.

Angular Versions:

• Angular: 1.5.5
• Material: 1.1.0-rc.5

Specifically, it's this bit of code:
https://github.com/angular/material/blob/master/src/components/icon/js/iconService.js#L450,L456

[devversion]

@topherfangio We already discussed about that on Slack.

Let's continue searching for some better possibilities.

[topherfangio]

@DevVersion Agreed; let's try to find a good solution. That said, if a user wants to provide an id that conflicts with another id in a separate SVG, I'm not sure there is much we can do.

[devversion]

I think, we should stick with our current solution.
It fixed accessibility issues with multiple unique ids, so it's actually desired.

My approach would be, introducing a new attribute, to disable this functionality.

[Lykathia]

That said, if a user wants to provide an id that conflicts with another id in a separate SVG, I'm not sure there is much we can do.

Having non-unique ID's is against spec. It would seem more of a user problem rather than something that requires library consideration. SVG's can always be refactored to utilize symbol/def and use references if there are conflicts.

<svg><use xmlns:xlink="http://www.w3.org/1999/xlink" xlink:href="#theUniqueId"></use></svg>

I was a little shocked at the current solution. It seems very counter-intuitive to mutate unique ID's to make them "more unique".

EDIT: That being said:

[topherfangio]

@Lykathia Indeed, it is indeed possible to extract the <def> tags into a separate SVG file, but with the way SVGs are defined for most icons, this would require a ton of work on the designer's behalf to extract out the similar parts. Also, you can't really guarantee that icons you find on the internet and want to use follow this pattern properly separating any <def>s into a separate file.

I think we're looking for a compromise that allows for users to do what they normally do without causing massive issues.

@DevVersion Can you link to the accessibility issues that were fixed by providing separate IDs?

[devversion]

@topherfangio Sure, here are some related issues / commits.

• Pre-Commit: https://github.com/angular/material/commit/810c4f3309b76b3337a155b177398ac2dd8ad0b0
• Commit: https://github.com/angular/material/commit/67bd35def212335b8dd1d5b1b03faf4573ea6b4f
• PR: https://github.com/angular/material/issues/7467
• Related Issues: https://github.com/angular/material/issues/7467 https://github.com/angular/material/issues/7381

[Lykathia]

@Lykathia Indeed, it is indeed possible to extract the tags into a separate SVG file, but with the way SVGs are defined for most icons, this would require a ton of work on the designer's behalf to extract out the similar parts. Also, you can't really guarantee that icons you find on the internet and want to use follow this pattern properly separating any s into a separate file.

Spitballing, as I'm certain this has flaws, but what if <md-icon> set unique id's on the cached svg's (top level, if missing), and then instead of inline-ing the svg directly, just inlined a use reference to that id.

e.g.:

<html>

<body>
  <svg id='foo'>
    <line x1="0" y1="0" x2="200" y2="0" style="stroke:rgb(255,0,0);stroke-width:2">
  </svg>

  <svg><use xlink:href='#foo'></use></svg>
  <svg><use xlink:href='#foo'></use></svg>
  <svg><use xlink:href='#foo'></use></svg>
  <svg><use xlink:href='#foo'></use></svg>
</body>

</html>

[topherfangio]

So, I totally thought of that. We could extract the <def>s out and cache them ourselves at the top. The problem with this approach is the following:

Suppose two icons use the same id for totally different purposes and we cache it at the top; now it totally breaks.

Honestly, I'm thinking that part of our caching might just need to check the rest of the <svg> for any uses of/references to that id and update them as well...

@DevVersion What are your thoughts on this approach?

[Lykathia]

Suppose two icons use the same id for totally different purposes and we cache it at the top; now it totally breaks.

I wasn't suggesting extracting subsets, but rather referencing the entire svg tag by ID. I'm not confident there aren't edge cases when single documents are added that define multiple tags (and sadly, do not have the time to properly investigate).

Honestly, I'm thinking that part of our caching might just need to check the rest of the

Slippery slope. Since you can, by design, reference elements in other SVG's inside ... or even outside the local DOM. https://www.w3.org/TR/SVG/linking.html#processingIRI

[Lykathia]

Suffering from a bit of insomnia, so figured I'd poke around a bit. Did the change solve #7467 and #7381? From using the test case plunkr in #7381 it does not appear to be fixed. Perhaps this is another issue:

Since the icon set in that plunkr is registered as the default icon set, all ID lookups for transformation fail the check. The fallback to check for icons in the default set doesn't happen until later. Moving the ID manipulation up to the top causes the cache to fire as expected.

Even when correcting for this behavior by specifying a named icon set, the problem as described in those tickets still appears. Firefox-Aurora, 48.0a2 (2016-05-03). 1.1.0-rc.5 tag.

[devversion]

@topherfangio I think, extracting the <def> elements out, and move them into the document top (or elsewhere) isn't a very elegant solution (as you said)

Checking for any usages, will make more sense to me, but it will also impact the performance (I guess)

And as @Lykathia said, it will be really difficult to detect those references.

[BrianLenzo]

We just ran into this issue with a gradient becoming broken after the image was reloaded from cache. +1 on @topherfangio suggestion of checking usages and updating them.

Tough problem, this seems like the least intrusive solution.

[BarrensZeppelin]

While this issue is getting fixed, we worked around it by placing all the icons in a div with position: absolute and left: -1000px. The icons that have mangled ids can now reference the original ones off-screen.

[carpaij]

+1
I'm facing the same problem where some of my svg's include fill="url(#dummie)" values which work fine the first time (after disabeling the <base> tag in index.html) the page loads. But subsequent calls hit the cached icons and hence the id's inside the fill attributes won't be referencing existing id's anymore.

[dstibbe]

Is there an option to just have md-svg-icon not touch anything in our svg? And,just ... show it as is?

[topherfangio]

@dstibbe That would certainly impact performance, but might be a decent/quick temporary solution. We could update $mdIconProvider with a new method to enable/disable caching. Let me talk to the rest of the team and get their thoughts.

[dstibbe]

@topherfangio that would be great.

[RBleyenberg]

@topherfangio That would be awesome ak a method for enable/disable caching +1

[msuozzo]

+1
Adding a method to disable caching (maybe even an annotation per-icon :smile:) would unblock us from this issue.

[ghost]

@topherfangio any updates? i am facing the same problem of id caching.

[topherfangio]

Sorry guys; I’m no longer actively working on the Material project.
On Tue, Nov 7, 2017 at 9:45 AM Jatin Tiwari [e-mail]
wrote:

@topherfangio https://github.com/topherfangio any updates? i am facing
the same problem of id caching.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/angular/material/issues/8689#issuecomment-342524273,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AADUYouGFQlltUgDN98GgfKC6VKEgcppks5s0HslgaJpZM4Iv9_I
.

[Splaktar]

I just wanted to make a note here, in addition to the label, that we need a CodePen demo of this problem so that we can debug it.

[Lykathia]

This issue broke the camels back for me, and I've long since moved away from using this library -> but the link in the original post points incorrectly as it should have been linked off of a tag.

https://github.com/angular/material/blob/v1.1.0-rc.5/src/components/icon/js/iconService.js#L450-L456

[ystreibel]

This issue is for all id references in svg!

Before caching :

<svg height="150" width="400">
  <defs>
    <linearGradient id="grad2" x1="0%" y1="0%" x2="0%" y2="100%">
      <stop offset="0%" style="stop-color:rgb(255,0,0);stop-opacity:1" />
      <stop offset="100%" style="stop-color:rgb(255,255,0);stop-opacity:1" />
    </linearGradient>
  </defs>
  <ellipse cx="200" cy="70" rx="85" ry="55" fill="url(#grad2)" />
</svg>

After caching:

<svg height="150" width="400">
  <defs>
    <linearGradient id="grad2_cache7" x1="0%" y1="0%" x2="0%" y2="100%">
      <stop offset="0%" style="stop-color:rgb(255,0,0);stop-opacity:1" />
      <stop offset="100%" style="stop-color:rgb(255,255,0);stop-opacity:1" />
    </linearGradient>
  </defs>
  <ellipse cx="200" cy="70" rx="85" ry="55" fill="url(#grad2)" />
</svg>

fill="url(#grad2)"loosing the new reference of linear linearGradient tag that is now grad2_cache7.

This is du to those lines

This is my proposual to fix this for all impact attributes:

if (clone.id) clone.id += cacheSuffix;
    angular.forEach(clone.querySelectorAll('[id]'), function(item) {
      angular.forEach(clone.querySelectorAll('[a="url(#'+ item.id +')"], [altGlyph="url(#'+ item.id +')"], [animate="url(#'+ item.id +')"], [animateColor="url(#'+ item.id +')"], [animateMotion="url(#'+ item.id +')"], [animateTransform="url(#'+ item.id +')"], [clip-path="url(#'+ item.id +')"], [color-profile="url(#'+ item.id +')"], [src="url(#'+ item.id +')"], [cursor="url(#'+ item.id +')"], [feImage="url(#'+ item.id +')"], [fill="url(#'+ item.id +')"], [filter="url(#'+ item.id +')"], [image="url(#'+ item.id +')"], [linearGradient="url(#'+ item.id +')"], [marker="url(#'+ item.id +')"], [marker-smart="url(#'+ item.id +')"], [marker-mid="url(#'+ item.id +')"], [marker-end="url(#'+ item.id +')"], [mask="url(#'+ item.id +')"], [pattern="url(#'+ item.id +')"], [radialGradient="url(#'+ item.id +')"], [script="url(#'+ item.id +')"], [stroke="url(#'+ item.id +')"], [textPath="url(#'+ item.id +')"], [tref="url(#'+ item.id +')"], [set="url(#'+ item.id +')"], [use="url(#'+ item.id +')"]'), function(refItem) {
        refItem.outerHTML = refItem.outerHTML.replace("url(#" + item.id + ")", "url(#" + item.id + cacheSuffix + ")");
      });
      item.id += cacheSuffix;
    });

Thanks

[Splaktar]

@ystreibel that looks pretty complex but does appear that it may solve this. Can you please open a PR that includes unit tests to verify this functionality?

[ystreibel]

@Splaktar done! PR #11315