fix(icon): remove IDs from source icon set from rendered output (#8266)

Author: jelbourn

src/lib/icon/fake-svgs.ts

@@ -8,39 +8,40 @@
 
 /**
  * Fake URLs and associated SVG documents used by tests.
+ * The ID attribute is used to load the icons, the name attribute is only used for testing.
  * @docs-private
  */
 export const FAKE_SVGS = {
-  cat: '<svg><path id="meow"></path></svg>',
-  dog: '<svg><path id="woof"></path></svg>',
+  cat: '<svg><path id="meow" name="meow"></path></svg>',
+  dog: '<svg><path id="woof" name="woof"></path></svg>',
   farmSet1: `
     <svg>
       <defs>
-        <g id="pig"><path id="oink"></path></g>
-        <g id="cow"><path id="moo"></path></g>
+        <g id="pig" name="pig"><path name="oink"></path></g>
+        <g id="cow" name="cow"><path name="moo"></path></g>
       </defs>
     </svg>
   `,
   farmSet2: `
     <svg>
       <defs>
-        <g id="cow"><path id="moo moo"></path></g>
-        <g id="sheep"><path id="baa"></path></g>
+        <g id="cow" name="cow"><path name="moo moo"></path></g>
+        <g id="sheep" name="sheep"><path name="baa"></path></g>
       </defs>
     </svg>
   `,
   farmSet3: `
     <svg>
-      <symbol id="duck">
-        <path id="quack"></path>
+      <symbol id="duck" name="duck">
+        <path id="quack" name="quack"></path>
       </symbol>
     </svg>
   `,
   arrows: `
     <svg>
       <defs>
-        <svg id="left-arrow"><path id="left"></path></svg>
-        <svg id="right-arrow"><path id="right"></path></svg>
+        <svg id="left-arrow" name="left-arrow"><path name="left"></path></svg>
+        <svg id="right-arrow" name="right-arrow"><path name="right"></path></svg>
       </defs>
     </svg>  `
 };

src/lib/icon/icon-registry.ts

@@ -365,23 +365,28 @@ export class MatIconRegistry {
    * returns it. Returns null if no matching element is found.
    */
   private _extractSvgIconFromSet(iconSet: SVGElement, iconName: string): SVGElement | null {
-    const iconNode = iconSet.querySelector('#' + iconName);
+    const iconSource = iconSet.querySelector('#' + iconName);
 
-    if (!iconNode) {
+    if (!iconSource) {
       return null;
     }
 
+    // Clone the element and remove the ID to prevent multiple elements from being added
+    // to the page with the same ID.
+    const iconElement = iconSource.cloneNode(true) as Element;
+    iconElement.id = '';
+
     // If the icon node is itself an <svg> node, clone and return it directly. If not, set it as
     // the content of a new <svg> node.
-    if (iconNode.tagName.toLowerCase() === 'svg') {
-      return this._setSvgAttributes(iconNode.cloneNode(true) as SVGElement);
+    if (iconElement.nodeName.toLowerCase() === 'svg') {
+      return this._setSvgAttributes(iconElement as SVGElement);
     }
 
     // If the node is a <symbol>, it won't be rendered so we have to convert it into <svg>. Note
     // that the same could be achieved by referring to it via <use href="#id">, however the <use>
     // tag is problematic on Firefox, because it needs to include the current page path.
-    if (iconNode.nodeName.toLowerCase() === 'symbol') {
-      return this._setSvgAttributes(this._toSvgElement(iconNode));
+    if (iconElement.nodeName.toLowerCase() === 'symbol') {
+      return this._setSvgAttributes(this._toSvgElement(iconElement));
     }
 
     // createElement('SVG') doesn't work as expected; the DOM ends up with
@@ -391,7 +396,7 @@ export class MatIconRegistry {
     // http://stackoverflow.com/questions/23003278/svg-innerhtml-in-firefox-can-not-display
     const svg = this._svgElementFromString('<svg></svg>');
     // Clone the node so we don't remove it from the parent icon set element.
-    svg.appendChild(iconNode.cloneNode(true));
+    svg.appendChild(iconElement);
 
     return this._setSvgAttributes(svg);
   }
@@ -400,8 +405,6 @@ export class MatIconRegistry {
    * Creates a DOM element from the given SVG string.
    */
   private _svgElementFromString(str: string): SVGElement {
-    // TODO: Is there a better way than innerHTML? Renderer doesn't appear to have a method for
-    // creating an element from an HTML string.
     const div = document.createElement('DIV');
     div.innerHTML = str;
     const svg = div.querySelector('svg') as SVGElement;

src/lib/icon/icon.spec.ts

@@ -17,6 +17,7 @@ function sortedClassNames(element: Element): string[] {
  * Verifies that an element contains a single <svg> child element, and returns that child.
  */
 function verifyAndGetSingleSvgChild(element: SVGElement): SVGElement {
+  expect(element.id).toBeFalsy();
   expect(element.childNodes.length).toBe(1);
   const svgChild = element.childNodes[0] as SVGElement;
   expect(svgChild.tagName.toLowerCase()).toBe('svg');
@@ -31,7 +32,9 @@ function verifyPathChildElement(element: Element, attributeValue: string): void
   expect(element.childNodes.length).toBe(1);
   const pathElement = element.childNodes[0] as SVGPathElement;
   expect(pathElement.tagName.toLowerCase()).toBe('path');
-  expect(pathElement.getAttribute('id')).toBe(attributeValue);
+
+  // The testing data SVGs have the name attribute set for verification.
+  expect(pathElement.getAttribute('name')).toBe(attributeValue);
 }
 
 
@@ -190,7 +193,7 @@ describe('MatIcon', () => {
       svgChild = svgElement.childNodes[0];
       // The first <svg> child should be the <g id="pig"> element.
       expect(svgChild.tagName.toLowerCase()).toBe('g');
-      expect(svgChild.getAttribute('id')).toBe('pig');
+      expect(svgChild.getAttribute('name')).toBe('pig');
       verifyPathChildElement(svgChild, 'oink');
 
       // Change the icon, and the SVG element should be replaced.
@@ -200,7 +203,7 @@ describe('MatIcon', () => {
       svgChild = svgElement.childNodes[0];
       // The first <svg> child should be the <g id="cow"> element.
       expect(svgChild.tagName.toLowerCase()).toBe('g');
-      expect(svgChild.getAttribute('id')).toBe('cow');
+      expect(svgChild.getAttribute('name')).toBe('cow');
       verifyPathChildElement(svgChild, 'moo');
     });
 
@@ -224,7 +227,8 @@ describe('MatIcon', () => {
       svgChild = svgElement.childNodes[0];
       // The <svg> child should be the <g id="pig"> element.
       expect(svgChild.tagName.toLowerCase()).toBe('g');
-      expect(svgChild.getAttribute('id')).toBe('pig');
+      expect(svgChild.getAttribute('name')).toBe('pig');
+      expect(svgChild.getAttribute('id')).toBe('');
       expect(svgChild.childNodes.length).toBe(1);
       verifyPathChildElement(svgChild, 'oink');
 
@@ -237,7 +241,7 @@ describe('MatIcon', () => {
       svgChild = svgElement.childNodes[0];
       // The first <svg> child should be the <g id="cow"> element.
       expect(svgChild.tagName.toLowerCase()).toBe('g');
-      expect(svgChild.getAttribute('id')).toBe('cow');
+      expect(svgChild.getAttribute('name')).toBe('cow');
       expect(svgChild.childNodes.length).toBe(1);
       verifyPathChildElement(svgChild, 'moo moo');
     });
@@ -259,7 +263,7 @@ describe('MatIcon', () => {
       expect(svgElement.querySelector('symbol')).toBeFalsy();
       expect(svgElement.childNodes.length).toBe(1);
       expect(firstChild.nodeName.toLowerCase()).toBe('path');
-      expect((firstChild as HTMLElement).getAttribute('id')).toBe('quack');
+      expect((firstChild as HTMLElement).getAttribute('name')).toBe('quack');
     });
 
     it('should not wrap <svg> elements in icon sets in another svg tag', () => {