fix(icon): not taking current path after initialization (#13641)

Author: crisbeto

src/lib/icon/icon.spec.ts

@@ -39,8 +39,11 @@ function verifyPathChildElement(element: Element, attributeValue: string): void
 
 
 describe('MatIcon', () => {
+  let fakePath: string;
 
   beforeEach(async(() => {
+    fakePath = '/fake-path';
+
     TestBed.configureTestingModule({
       imports: [HttpClientTestingModule, MatIconModule],
       declarations: [
@@ -55,7 +58,7 @@ describe('MatIcon', () => {
       ],
       providers: [{
         provide: MAT_ICON_LOCATION,
-        useValue: {pathname: '/fake-path'}
+        useValue: {getPathname: () => fakePath}
       }]
     });
 
@@ -608,6 +611,63 @@ describe('MatIcon', () => {
       tick();
     }));
 
+    it('should use latest path when prefixing the `url()` references', fakeAsync(() => {
+      iconRegistry.addSvgIconLiteral('fido', trustHtml(`
+        <svg>
+          <filter id="blur">
+            <feGaussianBlur in="SourceGraphic" stdDeviation="5" />
+          </filter>
+
+          <circle cx="170" cy="60" r="50" fill="green" filter="url('#blur')" />
+        </svg>
+      `));
+
+      let fixture = TestBed.createComponent(IconFromSvgName);
+      fixture.componentInstance.iconName = 'fido';
+      fixture.detectChanges();
+      let circle = fixture.nativeElement.querySelector('mat-icon svg circle');
+
+      expect(circle.getAttribute('filter')).toMatch(/^url\(['"]?\/fake-path#blur['"]?\)$/);
+      tick();
+      fixture.destroy();
+
+      fakePath = '/another-fake-path';
+      fixture = TestBed.createComponent(IconFromSvgName);
+      fixture.componentInstance.iconName = 'fido';
+      fixture.detectChanges();
+      circle = fixture.nativeElement.querySelector('mat-icon svg circle');
+
+      expect(circle.getAttribute('filter')).toMatch(/^url\(['"]?\/another-fake-path#blur['"]?\)$/);
+      tick();
+    }));
+
+    it('should update the `url()` references when the path changes', fakeAsync(() => {
+      iconRegistry.addSvgIconLiteral('fido', trustHtml(`
+        <svg>
+          <filter id="blur">
+            <feGaussianBlur in="SourceGraphic" stdDeviation="5" />
+          </filter>
+
+          <circle cx="170" cy="60" r="50" fill="green" filter="url('#blur')" />
+        </svg>
+      `));
+
+      const fixture = TestBed.createComponent(IconFromSvgName);
+      fixture.componentInstance.iconName = 'fido';
+      fixture.detectChanges();
+      const circle = fixture.nativeElement.querySelector('mat-icon svg circle');
+
+      // We use a regex to match here, rather than the exact value, because different browsers
+      // return different quotes through `getAttribute`, while some even omit the quotes altogether.
+      expect(circle.getAttribute('filter')).toMatch(/^url\(['"]?\/fake-path#blur['"]?\)$/);
+      tick();
+
+      fakePath = '/different-path';
+      fixture.detectChanges();
+
+      expect(circle.getAttribute('filter')).toMatch(/^url\(['"]?\/different-path#blur['"]?\)$/);
+    }));
+
   });
 
   describe('custom fonts', () => {

src/lib/icon/icon.ts

@@ -21,6 +21,8 @@ import {
   InjectionToken,
   inject,
   Inject,
+  OnDestroy,
+  AfterViewChecked,
 } from '@angular/core';
 import {DOCUMENT} from '@angular/common';
 import {CanColor, CanColorCtor, mixinColor} from '@angular/material/core';
@@ -51,14 +53,18 @@ export const MAT_ICON_LOCATION = new InjectionToken<MatIconLocation>('mat-icon-l
  * @docs-private
  */
 export interface MatIconLocation {
-  pathname: string;
+  getPathname: () => string;
 }
 
 /** @docs-private */
 export function MAT_ICON_LOCATION_FACTORY(): MatIconLocation {
   const _document = inject(DOCUMENT);
-  const pathname = (_document && _document.location && _document.location.pathname) || '';
-  return {pathname};
+
+  return {
+    // Note that this needs to be a function, rather than a property, because Angular
+    // will only resolve it once, but we want the current path on each call.
+    getPathname: () => (_document && _document.location && _document.location.pathname) || ''
+  };
 }
 
 
@@ -126,7 +132,9 @@ const funcIriPattern = /^url\(['"]?#(.*?)['"]?\)$/;
   encapsulation: ViewEncapsulation.None,
   changeDetection: ChangeDetectionStrategy.OnPush,
 })
-export class MatIcon extends _MatIconMixinBase implements OnChanges, OnInit, CanColor {
+export class MatIcon extends _MatIconMixinBase implements OnChanges, OnInit, AfterViewChecked,
+  CanColor, OnDestroy {
+
   /**
    * Whether the icon should be inlined, automatically sizing the icon to match the font size of
    * the element the icon is contained in.
@@ -162,6 +170,12 @@ export class MatIcon extends _MatIconMixinBase implements OnChanges, OnInit, Can
   private _previousFontSetClass: string;
   private _previousFontIconClass: string;
 
+  /** Keeps track of the current page path. */
+  private _previousPath?: string;
+
+  /** Keeps track of the elements and attributes that we've prefixed with the current path. */
+  private _elementsWithExternalReferences?: Map<Element, {name: string, value: string}[]>;
+
   constructor(
       elementRef: ElementRef<HTMLElement>,
       private _iconRegistry: MatIconRegistry,
@@ -233,6 +247,31 @@ export class MatIcon extends _MatIconMixinBase implements OnChanges, OnInit, Can
     }
   }
 
+  ngAfterViewChecked() {
+    const cachedElements = this._elementsWithExternalReferences;
+
+    if (cachedElements && this._location && cachedElements.size) {
+      const newPath = this._location.getPathname();
+
+      // We need to check whether the URL has changed on each change detection since
+      // the browser doesn't have an API that will let us react on link clicks and
+      // we can't depend on the Angular router. The references need to be updated,
+      // because while most browsers don't care whether the URL is correct after
+      // the first render, Safari will break if the user navigates to a different
+      // page and the SVG isn't re-rendered.
+      if (newPath !== this._previousPath) {
+        this._previousPath = newPath;
+        this._prependPathToReferences(newPath);
+      }
+    }
+  }
+
+  ngOnDestroy() {
+    if (this._elementsWithExternalReferences) {
+      this._elementsWithExternalReferences.clear();
+    }
+  }
+
   private _usingFontIcon(): boolean {
     return !this.svgIcon;
   }
@@ -251,14 +290,24 @@ export class MatIcon extends _MatIconMixinBase implements OnChanges, OnInit, Can
 
     // Note: we do this fix here, rather than the icon registry, because the
     // references have to point to the URL at the time that the icon was created.
-    this._prependCurrentPathToReferences(svg);
+    if (this._location) {
+      const path = this._location.getPathname();
+      this._previousPath = path;
+      this._cacheChildrenWithExternalReferences(svg);
+      this._prependPathToReferences(path);
+    }
+
     this._elementRef.nativeElement.appendChild(svg);
   }
 
   private _clearSvgElement() {
     const layoutElement: HTMLElement = this._elementRef.nativeElement;
     let childCount = layoutElement.childNodes.length;
 
+    if (this._elementsWithExternalReferences) {
+      this._elementsWithExternalReferences.clear();
+    }
+
     // Remove existing non-element child nodes and SVGs, and add the new SVG element. Note that
     // we can't use innerHTML, because IE will throw if the element has a data binding.
     while (childCount--) {
@@ -317,24 +366,42 @@ export class MatIcon extends _MatIconMixinBase implements OnChanges, OnInit, Can
    * reference. This is required because WebKit browsers require references to be prefixed with
    * the current path, if the page has a `base` tag.
    */
-  private _prependCurrentPathToReferences(element: SVGElement) {
-    // @breaking-change 8.0.0 Remove this null check once `_location` parameter is required.
-    if (!this._location) {
-      return;
+  private _prependPathToReferences(path: string) {
+    const elements = this._elementsWithExternalReferences;
+
+    if (elements) {
+      elements.forEach((attrs, element) => {
+        attrs.forEach(attr => {
+          element.setAttribute(attr.name, `url('${path}#${attr.value}')`);
+        });
+      });
     }
+  }
 
+  /**
+   * Caches the children of an SVG element that have `url()`
+   * references that we need to prefix with the current path.
+   */
+  private _cacheChildrenWithExternalReferences(element: SVGElement) {
     const elementsWithFuncIri = element.querySelectorAll(funcIriAttributeSelector);
-    const path = this._location.pathname ? this._location.pathname.split('#')[0] : '';
+    const elements = this._elementsWithExternalReferences =
+        this._elementsWithExternalReferences || new Map();
 
     for (let i = 0; i < elementsWithFuncIri.length; i++) {
       funcIriAttributes.forEach(attr => {
-        const value = elementsWithFuncIri[i].getAttribute(attr);
+        const elementWithReference = elementsWithFuncIri[i];
+        const value = elementWithReference.getAttribute(attr);
         const match = value ? value.match(funcIriPattern) : null;
 
         if (match) {
-          // Note the quotes inside the `url()`. They're important, because URLs pointing to named
-          // router outlets can contain parentheses which will break if they aren't quoted.
-          elementsWithFuncIri[i].setAttribute(attr, `url('${path}#${match[1]}')`);
+          let attributes = elements.get(elementWithReference);
+
+          if (!attributes) {
+            attributes = [];
+            elements.set(elementWithReference, attributes);
+          }
+
+          attributes!.push({name: attr, value: match[1]});
         }
       });
     }