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.

Sign up for GitHub

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

feat(material/icon): SEO friendly ligature icons #24578

Merged
merged 1 commit into from
Jun 27, 2022
Merged

feat(material/icon): SEO friendly ligature icons #24578

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 1 addition & 0 deletions src/components-examples/material/icon/icon-harness/icon-harness-example.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
<mat-icon fontSet="fontIcons" fontIcon="fontIcon"></mat-icon>
<mat-icon svgIcon="svgIcons:svgIcon"></mat-icon>
<mat-icon inline>ligature_icon</mat-icon>
<mat-icon fontIcon="ligature_icon_by_attribute"></mat-icon>
8 changes: 4 additions & 4 deletions src/components-examples/material/icon/icon-harness/icon-harness-example.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,24 +32,24 @@ describe('IconHarnessExample', () => {

it('should load all icon harnesses', async () => {
const icons = await loader.getAllHarnesses(MatIconHarness);
expect(icons.length).toBe(3);
expect(icons.length).toBe(4);
});

it('should get the name of an icon', async () => {
const icons = await loader.getAllHarnesses(MatIconHarness);
const names = await parallel(() => icons.map(icon => icon.getName()));
expect(names).toEqual(['fontIcon', 'svgIcon', 'ligature_icon']);
expect(names).toEqual(['fontIcon', 'svgIcon', 'ligature_icon', 'ligature_icon_by_attribute']);
});

it('should get the namespace of an icon', async () => {
const icons = await loader.getAllHarnesses(MatIconHarness);
const namespaces = await parallel(() => icons.map(icon => icon.getNamespace()));
expect(namespaces).toEqual(['fontIcons', 'svgIcons', null]);
expect(namespaces).toEqual(['fontIcons', 'svgIcons', null, null]);
});

it('should get whether an icon is inline', async () => {
const icons = await loader.getAllHarnesses(MatIconHarness);
const inlineStates = await parallel(() => icons.map(icon => icon.isInline()));
expect(inlineStates).toEqual([false, false, true]);
expect(inlineStates).toEqual([false, false, true, false]);
});
});
2 changes: 1 addition & 1 deletion src/components-examples/material/icon/icon-overview/icon-overview-example.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
<mat-icon aria-hidden="false" aria-label="Example home icon">home</mat-icon>
<mat-icon aria-hidden="false" aria-label="Example home icon" fontIcon="home"></mat-icon>
7 changes: 6 additions & 1 deletion src/dev-app/icon/icon-demo.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,12 @@
</p>

<p>
Ligature from Material Icons font:
Ligature from Material Icons font by attribute:
<mat-icon fontIcon="home"></mat-icon>
</p>

<p>
Ligature from Material Icons font by content:
<mat-icon>home</mat-icon>
</p>

Expand Down
27 changes: 20 additions & 7 deletions src/material/icon/icon-registry.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,12 @@ import {
Inject,
Injectable,
InjectionToken,
OnDestroy,
Optional,
SecurityContext,
SkipSelf,
OnDestroy,
} from '@angular/core';
import {DomSanitizer, SafeResourceUrl, SafeHtml} from '@angular/platform-browser';
import {DomSanitizer, SafeHtml, SafeResourceUrl} from '@angular/platform-browser';
import {forkJoin, Observable, of as observableOf, throwError as observableThrow} from 'rxjs';
import {catchError, finalize, map, share, tap} from 'rxjs/operators';
import {TrustedHTML, trustedHTMLFromString} from './trusted-types';
Expand Down Expand Up @@ -149,7 +149,7 @@ export class MatIconRegistry implements OnDestroy {
* specified. The default 'material-icons' value assumes that the material icon font has been
* loaded as described at http://google.github.io/material-design-icons/#icon-font-for-the-web
*/
private _defaultFontSetClass = ['material-icons'];
private _defaultFontSetClass = ['material-icons', 'mat-ligature-font'];
Copy link
Member

Choose a reason for hiding this comment

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

I think that adding it to the defaults may break existing clients. We should document in icon.md how people can opt into it instead. A live example would be good as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@crisbeto, sorry for the unusually late reply.

I don't think it is a breaking change, because even though the new class is always added, it only has any effect if and only if the fontIcon attribute is also used. And since that attribute did not have any meaning for the material-icons font previously, there is no reason to have that in existing code. And even then if a user incorrectly duplicated the icon name, like <mat-icon fontIcon="home">home</mat-icon>, then it would only show the icon over each other. So even in case of past incorrect use, there is not visual difference at all.

Also I don't think that having a new class (which does nothing) on a element is considered breaking, because classes are prefixed with mat- specifically to avoid class name conflicts.

Below is an extract of the dev-app where you can see that new usage style (by attribute), and the old usage style (by content) are both pixel perfect, without any changes for the old usage:

image

What exactly did you have in mind when you mentioned breaking change ?


constructor(
@Optional() private _httpClient: HttpClient,
Expand Down Expand Up @@ -281,15 +281,28 @@ export class MatIconRegistry implements OnDestroy {
}

/**
* Defines an alias for a CSS class name to be used for icon fonts. Creating an matIcon
* Defines an alias for CSS class names to be used for icon fonts. Creating an matIcon
* component with the alias as the fontSet input will cause the class name to be applied
* to the `<mat-icon>` element.
*
* If the registered font is a ligature font, then don't forget to also include the special
* class `mat-ligature-font` to allow the usage via attribute. So register like this:
*
* ```ts
* iconRegistry.registerFontClassAlias('f1', 'font1 mat-ligature-font');
* ```
*
* And use like this:
*
* ```html
* <mat-icon fontSet="f1" fontIcon="home"></mat-icon>
* ```
*
* @param alias Alias for the font.
* @param className Class name override to be used instead of the alias.
* @param classNames Class names override to be used instead of the alias.
*/
registerFontClassAlias(alias: string, className: string = alias): this {
this._fontCssClassesByAlias.set(alias, className);
registerFontClassAlias(alias: string, classNames: string = alias): this {
this._fontCssClassesByAlias.set(alias, classNames);
return this;
}

Expand Down
4 changes: 4 additions & 0 deletions src/material/icon/icon.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,10 @@ $size: 24px !default;
line-height: inherit;
width: inherit;
}

&.mat-ligature-font[fontIcon]::before {
content: attr(fontIcon);
}
}

// Icons that will be mirrored in RTL.
Expand Down
77 changes: 72 additions & 5 deletions src/material/icon/icon.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ describe('MatIcon', () => {
declarations: [
IconWithColor,
IconWithLigature,
IconWithLigatureByAttribute,
IconWithCustomFontCss,
IconFromSvgName,
IconWithAriaHiddenFalse,
Expand Down Expand Up @@ -125,6 +126,7 @@ describe('MatIcon', () => {
fixture.detectChanges();
expect(sortedClassNames(matIconElement)).toEqual([
'mat-icon',
'mat-ligature-font',
'mat-primary',
'material-icons',
'notranslate',
Expand All @@ -143,6 +145,7 @@ describe('MatIcon', () => {
expect(sortedClassNames(matIconElement)).toEqual([
'mat-icon',
'mat-icon-no-color',
'mat-ligature-font',
'material-icons',
'notranslate',
]);
Expand Down Expand Up @@ -179,7 +182,7 @@ describe('MatIcon', () => {
});

describe('Ligature icons', () => {
it('should add material-icons class by default', () => {
it('should add material-icons and mat-ligature-font class by default', () => {
const fixture = TestBed.createComponent(IconWithLigature);

const testComponent = fixture.componentInstance;
Expand All @@ -189,13 +192,14 @@ describe('MatIcon', () => {
expect(sortedClassNames(matIconElement)).toEqual([
'mat-icon',
'mat-icon-no-color',
'mat-ligature-font',
'material-icons',
'notranslate',
]);
});

it('should use alternate icon font if set', () => {
iconRegistry.setDefaultFontSetClass('myfont');
iconRegistry.setDefaultFontSetClass('myfont', 'mat-ligature-font');

const fixture = TestBed.createComponent(IconWithLigature);

Expand All @@ -206,6 +210,7 @@ describe('MatIcon', () => {
expect(sortedClassNames(matIconElement)).toEqual([
'mat-icon',
'mat-icon-no-color',
'mat-ligature-font',
'myfont',
'notranslate',
]);
Expand All @@ -223,7 +228,7 @@ describe('MatIcon', () => {
});

it('should be able to provide multiple alternate icon set classes', () => {
iconRegistry.setDefaultFontSetClass('myfont', 'myfont-48x48');
iconRegistry.setDefaultFontSetClass('myfont', 'mat-ligature-font', 'myfont-48x48');

let fixture = TestBed.createComponent(IconWithLigature);

Expand All @@ -234,6 +239,62 @@ describe('MatIcon', () => {
expect(sortedClassNames(matIconElement)).toEqual([
'mat-icon',
'mat-icon-no-color',
'mat-ligature-font',
'myfont',
'myfont-48x48',
'notranslate',
]);
});
});

describe('Ligature icons by attribute', () => {
it('should add material-icons and mat-ligature-font class by default', () => {
const fixture = TestBed.createComponent(IconWithLigatureByAttribute);

const testComponent = fixture.componentInstance;
const matIconElement = fixture.debugElement.nativeElement.querySelector('mat-icon');
testComponent.iconName = 'home';
fixture.detectChanges();
expect(sortedClassNames(matIconElement)).toEqual([
'mat-icon',
'mat-icon-no-color',
'mat-ligature-font',
'material-icons',
'notranslate',
]);
});

it('should use alternate icon font if set', () => {
iconRegistry.setDefaultFontSetClass('myfont', 'mat-ligature-font');

const fixture = TestBed.createComponent(IconWithLigatureByAttribute);

const testComponent = fixture.componentInstance;
const matIconElement = fixture.debugElement.nativeElement.querySelector('mat-icon');
testComponent.iconName = 'home';
fixture.detectChanges();
expect(sortedClassNames(matIconElement)).toEqual([
'mat-icon',
'mat-icon-no-color',
'mat-ligature-font',
'myfont',
'notranslate',
]);
});

it('should be able to provide multiple alternate icon set classes', () => {
iconRegistry.setDefaultFontSetClass('myfont', 'mat-ligature-font', 'myfont-48x48');

let fixture = TestBed.createComponent(IconWithLigatureByAttribute);

const testComponent = fixture.componentInstance;
const matIconElement = fixture.debugElement.nativeElement.querySelector('mat-icon');
testComponent.iconName = 'home';
fixture.detectChanges();
expect(sortedClassNames(matIconElement)).toEqual([
'mat-icon',
'mat-icon-no-color',
'mat-ligature-font',
'myfont',
'myfont-48x48',
'notranslate',
Expand Down Expand Up @@ -1042,17 +1103,18 @@ describe('MatIcon', () => {
it('should handle values with extraneous spaces being passed in to `fontIcon`', () => {
const fixture = TestBed.createComponent(IconWithCustomFontCss);
const matIconElement = fixture.debugElement.nativeElement.querySelector('mat-icon');
fixture.componentInstance.fontSet = 'f1';

expect(() => {
fixture.componentInstance.fontIcon = 'font icon';
fixture.detectChanges();
}).not.toThrow();

expect(sortedClassNames(matIconElement)).toEqual([
'f1',
'font',
'mat-icon',
'mat-icon-no-color',
'material-icons',
'notranslate',
]);

Expand All @@ -1063,9 +1125,9 @@ describe('MatIcon', () => {

expect(sortedClassNames(matIconElement)).toEqual([
'changed',
'f1',
'mat-icon',
'mat-icon-no-color',
'material-icons',
'notranslate',
]);
});
Expand Down Expand Up @@ -1311,6 +1373,11 @@ class IconWithLigature {
iconName = '';
}

@Component({template: `<mat-icon [fontIcon]="iconName"></mat-icon>`})
class IconWithLigatureByAttribute {
iconName = '';
}

@Component({template: `<mat-icon [color]="iconColor">{{iconName}}</mat-icon>`})
class IconWithColor {
iconName = '';
Expand Down
18 changes: 13 additions & 5 deletions src/material/icon/icon.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,13 +114,18 @@ const funcIriPattern = /^url\(['"]?#(.*?)['"]?\)$/;
* `<mat-icon svgIcon="left-arrow"></mat-icon>
* <mat-icon svgIcon="animals:cat"></mat-icon>`
*
* - Use a font ligature as an icon by putting the ligature text in the content of the `<mat-icon>`
* component. By default the Material icons font is used as described at
* - Use a font ligature as an icon by putting the ligature text in the `fontIcon` attribute or the
* content of the `<mat-icon>` component. If you register a custom font class, don't forget to also
* include the special class `mat-ligature-font`. It is recommended to use the attribute alternative
* to prevent the ligature text to be selectable and to appear in search engine results.
* By default, the Material icons font is used as described at
* http://google.github.io/material-design-icons/#icon-font-for-the-web. You can specify an
* alternate font by setting the fontSet input to either the CSS class to apply to use the
* desired font, or to an alias previously registered with MatIconRegistry.registerFontClassAlias.
* Examples:
* `<mat-icon>home</mat-icon>
* `<mat-icon fontIcon="home"></mat-icon>
* <mat-icon>home</mat-icon>
* <mat-icon fontSet="myfont" fontIcon="sun"></mat-icon>
* <mat-icon fontSet="myfont">sun</mat-icon>`
*
* - Specify a font glyph to be included via CSS rules by setting the fontSet input to specify the
Expand Down Expand Up @@ -359,15 +364,18 @@ export class MatIcon extends _MatIconBase implements OnInit, AfterViewChecked, C
const elem: HTMLElement = this._elementRef.nativeElement;
const fontSetClasses = (
this.fontSet
? [this._iconRegistry.classNameForFontAlias(this.fontSet)]
? this._iconRegistry.classNameForFontAlias(this.fontSet).split(/ +/)
: this._iconRegistry.getDefaultFontSetClass()
).filter(className => className.length > 0);

this._previousFontSetClass.forEach(className => elem.classList.remove(className));
fontSetClasses.forEach(className => elem.classList.add(className));
this._previousFontSetClass = fontSetClasses;

if (this.fontIcon !== this._previousFontIconClass) {
if (
this.fontIcon !== this._previousFontIconClass &&
!fontSetClasses.includes('mat-ligature-font')
) {
if (this._previousFontIconClass) {
elem.classList.remove(this._previousFontIconClass);
}
Expand Down
15 changes: 8 additions & 7 deletions src/material/icon/testing/shared.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ export function runHarnessTests(

it('should load all icon harnesses', async () => {
const icons = await loader.getAllHarnesses(iconHarness);
expect(icons.length).toBe(3);
expect(icons.length).toBe(4);
});

it('should filter icon harnesses based on their type', async () => {
Expand All @@ -47,7 +47,7 @@ export function runHarnessTests(
]);

expect(svgIcons.length).toBe(1);
expect(fontIcons.length).toBe(2);
expect(fontIcons.length).toBe(3);
});

it('should filter icon harnesses based on their name', async () => {
Expand All @@ -69,31 +69,31 @@ export function runHarnessTests(

expect(regexFilterResults.length).toBe(1);
expect(stringFilterResults.length).toBe(1);
expect(nullFilterResults.length).toBe(1);
expect(nullFilterResults.length).toBe(2);
});

it('should get the type of each icon', async () => {
const icons = await loader.getAllHarnesses(iconHarness);
const types = await parallel(() => icons.map(icon => icon.getType()));
expect(types).toEqual([IconType.FONT, IconType.SVG, IconType.FONT]);
expect(types).toEqual([IconType.FONT, IconType.SVG, IconType.FONT, IconType.FONT]);
});

it('should get the name of an icon', async () => {
const icons = await loader.getAllHarnesses(iconHarness);
const names = await parallel(() => icons.map(icon => icon.getName()));
expect(names).toEqual(['fontIcon', 'svgIcon', 'ligature_icon']);
expect(names).toEqual(['fontIcon', 'svgIcon', 'ligature_icon', 'ligature_icon_by_attribute']);
});

it('should get the namespace of an icon', async () => {
const icons = await loader.getAllHarnesses(iconHarness);
const namespaces = await parallel(() => icons.map(icon => icon.getNamespace()));
expect(namespaces).toEqual(['fontIcons', 'svgIcons', null]);
expect(namespaces).toEqual(['fontIcons', 'svgIcons', null, null]);
});

it('should get whether an icon is inline', async () => {
const icons = await loader.getAllHarnesses(iconHarness);
const inlineStates = await parallel(() => icons.map(icon => icon.isInline()));
expect(inlineStates).toEqual([false, false, true]);
expect(inlineStates).toEqual([false, false, true, false]);
});
}

Expand All @@ -102,6 +102,7 @@ export function runHarnessTests(
<mat-icon fontSet="fontIcons" fontIcon="fontIcon"></mat-icon>
<mat-icon svgIcon="svgIcons:svgIcon"></mat-icon>
<mat-icon inline>ligature_icon</mat-icon>
<mat-icon fontIcon="ligature_icon_by_attribute"></mat-icon>
`,
})
class IconHarnessTest {}
Loading