Skip to content

fix(icon): clearing all content when inserting a new SVG #11956

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

Merged
merged 1 commit into from
Jun 28, 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
26 changes: 26 additions & 0 deletions src/lib/icon/icon.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ describe('MatIcon', () => {
IconWithAriaHiddenFalse,
IconWithBindingAndNgIf,
InlineIcon,
SvgIconWithUserContent,
]
});

Expand Down Expand Up @@ -397,6 +398,26 @@ describe('MatIcon', () => {

expect(icon.querySelector('svg')).toBeFalsy();
});

it('should keep non-SVG user content inside the icon element', fakeAsync(() => {
iconRegistry.addSvgIcon('fido', trustUrl('dog.svg'));

const fixture = TestBed.createComponent(SvgIconWithUserContent);
const testComponent = fixture.componentInstance;
const iconElement = fixture.debugElement.nativeElement.querySelector('mat-icon');

testComponent.iconName = 'fido';
fixture.detectChanges();
http.expectOne('dog.svg').flush(FAKE_SVGS.dog);

const userDiv = iconElement.querySelector('div');

expect(userDiv).toBeTruthy();
expect(iconElement.textContent.trim()).toContain('Hello');

tick();
}));

});

describe('Icons from HTML string', () => {
Expand Down Expand Up @@ -706,3 +727,8 @@ class IconWithBindingAndNgIf {
class InlineIcon {
inline = false;
}

@Component({template: `<mat-icon [svgIcon]="iconName"><div>Hello</div></mat-icon>`})
class SvgIconWithUserContent {
iconName: string | undefined = '';
}
13 changes: 9 additions & 4 deletions src/lib/icon/icon.ts
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,6 @@ export const _MatIconMixinBase = mixinColor(MatIconBase);
changeDetection: ChangeDetectionStrategy.OnPush,
})
export class MatIcon extends _MatIconMixinBase implements OnChanges, OnInit, CanColor {

/**
* Whether the icon should be inlined, automatically sizing the icon to match the font size of
* the element the icon is contained in.
Expand Down Expand Up @@ -199,10 +198,16 @@ export class MatIcon extends _MatIconMixinBase implements OnChanges, OnInit, Can
const layoutElement: HTMLElement = this._elementRef.nativeElement;
const childCount = layoutElement.childNodes.length;

// Remove existing child nodes and add the new SVG element. Note that we can't
// use innerHTML, because IE will throw if the element has a data binding.
// 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.
for (let i = 0; i < childCount; i++) {
layoutElement.removeChild(layoutElement.childNodes[i]);
const child = layoutElement.childNodes[i];

Copy link
Member

Choose a reason for hiding this comment

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

It would be good to add a comment that elaborates on why only specific elements are removed.

// 1 corresponds to Node.ELEMENT_NODE. We remove all non-element nodes in order to get rid
// of any loose text nodes, as well as any SVG elements in order to remove any old icons.
if (child.nodeType !== 1 || child.nodeName.toLowerCase() === 'svg') {
layoutElement.removeChild(child);
}
}
}

Expand Down