Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(icon): handle references for pages with base tag #12428

Merged
merged 1 commit into from
Oct 5, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 30 additions & 2 deletions src/lib/icon/icon.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import {inject, async, fakeAsync, tick, TestBed} from '@angular/core/testing';
import {SafeResourceUrl, DomSanitizer, SafeHtml} from '@angular/platform-browser';
import {HttpClientTestingModule, HttpTestingController} from '@angular/common/http/testing';
import {Component} from '@angular/core';
import {MatIconModule} from './index';
import {MatIconModule, MAT_ICON_LOCATION} from './index';
import {MatIconRegistry, getMatIconNoHttpProviderError} from './icon-registry';
import {FAKE_SVGS} from './fake-svgs';
import {wrappedErrorMessage} from '@angular/cdk/testing';
Expand Down Expand Up @@ -52,7 +52,11 @@ describe('MatIcon', () => {
IconWithBindingAndNgIf,
InlineIcon,
SvgIconWithUserContent,
]
],
providers: [{
provide: MAT_ICON_LOCATION,
useValue: {pathname: '/fake-path'}
}]
});

TestBed.compileComponents();
Expand Down Expand Up @@ -580,6 +584,30 @@ describe('MatIcon', () => {

tick();
}));

it('should prepend the current path to attributes with `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>
`));

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();
}));

});

describe('custom fonts', () => {
Expand Down
90 changes: 89 additions & 1 deletion src/lib/icon/icon.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,12 @@ import {
OnInit,
SimpleChanges,
ViewEncapsulation,
Optional,
InjectionToken,
inject,
Inject,
} from '@angular/core';
import {DOCUMENT} from '@angular/common';
import {CanColor, CanColorCtor, mixinColor} from '@angular/material/core';
import {coerceBooleanProperty} from '@angular/cdk/coercion';
import {MatIconRegistry} from './icon-registry';
Expand All @@ -31,6 +36,53 @@ export class MatIconBase {
export const _MatIconMixinBase: CanColorCtor & typeof MatIconBase =
mixinColor(MatIconBase);

/**
* Injection token used to provide the current location to `MatIcon`.
* Used to handle server-side rendering and to stub out during unit tests.
* @docs-private
*/
export const MAT_ICON_LOCATION = new InjectionToken<MatIconLocation>('mat-icon-location', {
providedIn: 'root',
factory: MAT_ICON_LOCATION_FACTORY
});

/**
* Stubbed out location for `MatIcon`.
* @docs-private
*/
export interface MatIconLocation {
pathname: string;
}

/** @docs-private */
export function MAT_ICON_LOCATION_FACTORY(): MatIconLocation {
const _document = inject(DOCUMENT);
const pathname = (_document && _document.location && _document.location.pathname) || '';
return {pathname};
}


/** SVG attributes that accept a FuncIRI (e.g. `url(<something>)`). */
const funcIriAttributes = [
'clip-path',
'color-profile',
'src',
'cursor',
'fill',
'filter',
'marker',
'marker-start',
'marker-mid',
'marker-end',
'mask',
'stroke'
];

/** Selector that can be used to find all elements that are using a `FuncIRI`. */
const funcIriAttributeSelector = funcIriAttributes.map(attr => `[${attr}]`).join(', ');

/** Regex that can be used to extract the id out of a FuncIRI. */
const funcIriPattern = /^url\(['"]?#(.*?)['"]?\)$/;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a link we could put here for some spec for this format? Should the # be optional?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could link to the SVG spec or MDN, but the link is longer than the description of the format: https://developer.mozilla.org/en-US/docs/Web/SVG/Content_type#FuncIRI. As for the #, it shouldn't be optional, because we only want to target references by ID.


/**
* Component to display an icon. It can be used in the following ways:
Expand Down Expand Up @@ -113,7 +165,12 @@ export class MatIcon extends _MatIconMixinBase implements OnChanges, OnInit, Can
constructor(
elementRef: ElementRef<HTMLElement>,
private _iconRegistry: MatIconRegistry,
@Attribute('aria-hidden') ariaHidden: string) {
@Attribute('aria-hidden') ariaHidden: string,
/**
* @deprecated `location` parameter to be made required.
* @breaking-change 8.0.0
*/
@Optional() @Inject(MAT_ICON_LOCATION) private _location?: MatIconLocation) {
super(elementRef);

// If the user has not explicitly set aria-hidden, mark the icon as hidden, as this is
Expand Down Expand Up @@ -192,6 +249,9 @@ export class MatIcon extends _MatIconMixinBase implements OnChanges, OnInit, Can
styleTags[i].textContent += ' ';
}

// 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);
this._elementRef.nativeElement.appendChild(svg);
}

Expand Down Expand Up @@ -251,4 +311,32 @@ export class MatIcon extends _MatIconMixinBase implements OnChanges, OnInit, Can
private _cleanupFontValue(value: string) {
return typeof value === 'string' ? value.trim().split(' ')[0] : value;
}

/**
* Prepends the current path to all elements that have an attribute pointing to a `FuncIRI`
* 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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Last thing: should we just restrict this to webkit, then? I suspect it's not hugely costly, but if we can easily/reliably cut out the extra work, we might as well.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went with doing it on all browsers since it shouldn't be too costly (unless an individual icon has hundreds of elements that all have one of the attributes) and to avoid having too many of these forks in the logic where we sniff out the browser and do special things just for one vendor.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking a little closer- could we do this prepending at the icon-registry level so that it only has to happen once per svg? I didn't realize earlier it was happening for every directive instance.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a comment for it a bit further down. Having it in the registry would be ideal, but we want the path at the time the icon was created, rather than when it was fetched. If we did it in the registry, it would be cached with the initial path.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the path in <base> normally change?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't the icon registry not request an SVG until it's used anyway?

I'm just hesitant to go forward with adding the extra work for pages that might have multiple icons in something like a large data table or option list.

Copy link
Member Author

@crisbeto crisbeto Aug 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does, but it also caches it afterwards. If performance is a concern, an alternative can be to have some kind of inexpensive check against the string representation of the icon before we do the querySelector and we walk through all the attributes. E.g. adding a MatIconRegistry.hasFuncIRI(iconName) method which just does a regex match or an indexOf.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not the check so much as actually updating each uri that I'm worried about.

Copy link
Member Author

@crisbeto crisbeto Aug 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure whether it would be that expensive, compared to what we're doing for each icon already (cloning the cached SVG node).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fair.

return;
}

const elementsWithFuncIri = element.querySelectorAll(funcIriAttributeSelector);
const path = this._location.pathname ? this._location.pathname.split('#')[0] : '';

for (let i = 0; i < elementsWithFuncIri.length; i++) {
funcIriAttributes.forEach(attr => {
const value = elementsWithFuncIri[i].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]}')`);
}
});
}
}
}