Skip to content

Commit

Permalink
fix(ivy): incorrectly validating html foreign objects inside svg
Browse files Browse the repository at this point in the history 
Fixes ngtsc incorrectly logging an unknown element diagnostic for HTML elements that are inside an SVG `foreignObject` with the `xhtml` namespace.

Fixes #34171.
  • Loading branch information
@crisbeto
crisbeto committed Dec 2, 2019
1 parent 168abc6 commit 4405bb8
Show file tree
Hide file tree
Showing 3 changed files with 116 additions and 29 deletions.
16 changes: 11 additions & 5 deletions packages/compiler-cli/src/ngtsc/typecheck/src/dom.ts
View file
Expand Up @@ -14,6 +14,7 @@ import {ErrorCode} from '../../diagnostics';
import {TcbSourceResolver, makeTemplateDiagnostic} from './diagnostics';

const REGISTRY = new DomElementSchemaRegistry();
const REMOVE_XHTML_REGEX = /^:xhtml:/;

/**
* Checks every non-Angular element/property processed in a template and potentially produces
Expand Down Expand Up @@ -69,15 +70,20 @@ export class RegistryDomSchemaChecker implements DomSchemaChecker {
constructor(private resolver: TcbSourceResolver) {}

checkElement(id: string, element: TmplAstElement, schemas: SchemaMetadata[]): void {
if (!REGISTRY.hasElement(element.name, schemas)) {
// HTML elements inside an SVG `foreignObject` are declared in the `xhtml` namespace.
// We need to strip it before handing it over to the registry because all HTML tag names
// in the registry are without a namespace.
const name = element.name.replace(REMOVE_XHTML_REGEX, '');

if (!REGISTRY.hasElement(name, schemas)) {
const mapping = this.resolver.getSourceMapping(id);

let errorMsg = `'${element.name}' is not a known element:\n`;
let errorMsg = `'${name}' is not a known element:\n`;
errorMsg +=
`1. If '${element.name}' is an Angular component, then verify that it is part of this module.\n`;
if (element.name.indexOf('-') > -1) {
`1. If '${name}' is an Angular component, then verify that it is part of this module.\n`;
if (name.indexOf('-') > -1) {
errorMsg +=
`2. If '${element.name}' is a Web Component then add 'CUSTOM_ELEMENTS_SCHEMA' to the '@NgModule.schemas' of this component to suppress this message.`;
`2. If '${name}' is a Web Component then add 'CUSTOM_ELEMENTS_SCHEMA' to the '@NgModule.schemas' of this component to suppress this message.`;
} else {
errorMsg +=
`2. To allow any element add 'NO_ERRORS_SCHEMA' to the '@NgModule.schemas' of this component.`;
Expand Down
97 changes: 73 additions & 24 deletions packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts
View file
Expand Up @@ -100,18 +100,18 @@ export declare class AnimationEvent {
{fullTemplateTypeCheck: true, strictInputTypes: true, strictAttributeTypes: true});
env.write('test.ts', `
import {Component, Directive, NgModule, Input} from '@angular/core';
@Component({
selector: 'test',
template: '<div dir foo="2"></div>',
})
class TestCmp {}
@Directive({selector: '[dir]'})
class TestDir {
@Input() foo: number;
}
@NgModule({
declarations: [TestCmp, TestDir],
})
Expand All @@ -128,15 +128,15 @@ export declare class AnimationEvent {
{fullTemplateTypeCheck: true, strictInputTypes: true, strictOutputEventTypes: true});
env.write('test.ts', `
import {Component, Directive, NgModule, EventEmitter} from '@angular/core';
@Component({
selector: 'test',
template: '<div dir [some-input.xs]="2" (some-output)="handleEvent($event)"></div>',
})
class TestCmp {
handleEvent(event: number): void {}
}
@Directive({
selector: '[dir]',
inputs: ['some-input.xs'],
Expand All @@ -146,7 +146,7 @@ export declare class AnimationEvent {
'some-input.xs': string;
'some-output': EventEmitter<string>;
}
@NgModule({
declarations: [TestCmp, TestDir],
})
Expand All @@ -164,20 +164,20 @@ export declare class AnimationEvent {
env.tsconfig({fullTemplateTypeCheck: true, strictOutputEventTypes: true});
env.write('test.ts', `
import {Component, Directive, EventEmitter, NgModule, Output} from '@angular/core';
@Component({
selector: 'test',
template: '<div dir (update)="update($event); updated = true" (focus)="update($event); focused = true"></div>',
})
class TestCmp {
update(data: string) {}
}
@Directive({selector: '[dir]'})
class TestDir {
@Output() update = new EventEmitter<number>();
}
@NgModule({
declarations: [TestCmp, TestDir],
})
Expand Down Expand Up @@ -589,15 +589,15 @@ export declare class AnimationEvent {
beforeEach(() => {
env.write('test.ts', `
import {Component, NgModule} from '@angular/core';
@Component({
selector: 'test',
template: '<div (focus)="invalid; update($event)"></div>',
})
class TestCmp {
update(data: string) {}
}
@NgModule({
declarations: [TestCmp],
})
Expand Down Expand Up @@ -915,7 +915,7 @@ export declare class AnimationEvent {
env.write('test.ts', `
import {CommonModule} from '@angular/common';
import {Component, NgModule} from '@angular/core';
@Component({
selector: 'test',
template: \`<div *ngFor="let foo of foos as foos">
Expand All @@ -926,7 +926,7 @@ export declare class AnimationEvent {
export class TestCmp {
foos: {name: string}[];
}
@NgModule({
declarations: [TestCmp],
imports: [CommonModule],
Expand Down Expand Up @@ -1015,7 +1015,7 @@ export declare class AnimationEvent {
static ɵdir: i0.ɵɵDirectiveDefWithMeta<BaseDir, '[base]', never, {'fromBase': 'fromBase'}, never, never>;
}
export declare class ExternalModule {
static ɵmod: i0.ɵɵNgModuleDefWithMeta<ExternalModule, [typeof BaseDir], never, [typeof BaseDir]>;
}
Expand All @@ -1024,20 +1024,20 @@ export declare class AnimationEvent {
env.write('test.ts', `
import {Component, Directive, Input, NgModule} from '@angular/core';
import {BaseDir, ExternalModule} from 'external';
@Directive({
selector: '[child]',
})
class ChildDir extends BaseDir {
@Input() fromChild!: boolean;
}
@Component({
selector: 'test',
template: '<div child [fromAbstract]="true" [fromBase]="3" [fromChild]="4"></div>',
})
class TestCmp {}
@NgModule({
declarations: [TestCmp, ChildDir],
imports: [ExternalModule],
Expand Down Expand Up @@ -1260,13 +1260,13 @@ export declare class AnimationEvent {
() => {
env.write('test.ts', `
import {Component, NgModule, CUSTOM_ELEMENTS_SCHEMA} from '@angular/core';
@Component({
selector: 'blah',
template: '<custom-element [foo]="1">test</custom-element>',
})
export class FooCmp {}
@NgModule({
declarations: [FooCmp],
schemas: [CUSTOM_ELEMENTS_SCHEMA],
Expand All @@ -1280,13 +1280,13 @@ export declare class AnimationEvent {
it('should not produce diagnostics when using the NO_ERRORS_SCHEMA', () => {
env.write('test.ts', `
import {Component, NgModule, NO_ERRORS_SCHEMA} from '@angular/core';
@Component({
selector: 'blah',
template: '<foo [bar]="1"></foo>',
})
export class FooCmp {}
@NgModule({
declarations: [FooCmp],
schemas: [NO_ERRORS_SCHEMA],
Expand All @@ -1296,6 +1296,55 @@ export declare class AnimationEvent {
const diags = env.driveDiagnostics();
expect(diags).toEqual([]);
});

it('should allow HTML elements inside SVG foreignObject', () => {
env.write('test.ts', `
import {Component, NgModule} from '@angular/core';
@Component({
selector: 'blah',
template: \`
<svg>
<svg:foreignObject>
<xhtml:div>Hello</xhtml:div>
</svg:foreignObject>
</svg>
\`,
})
export class FooCmp {}
@NgModule({
declarations: [FooCmp],
})
export class FooModule {}
`);
const diags = env.driveDiagnostics();
expect(diags.length).toBe(0);
});

it('should check for unknown elements inside an SVG foreignObject', () => {
env.write('test.ts', `
import {Component, NgModule} from '@angular/core';
@Component({
selector: 'blah',
template: \`
<svg>
<svg:foreignObject>
<xhtml:foo>Hello</xhtml:foo>
</svg:foreignObject>
</svg>
\`,
})
export class FooCmp {}
@NgModule({
declarations: [FooCmp],
})
export class FooModule {}
`);
const diags = env.driveDiagnostics();
expect(diags.length).toBe(1);
expect(diags[0].messageText).toBe(`'foo' is not a known element:
1. If 'foo' is an Angular component, then verify that it is part of this module.
2. To allow any element add 'NO_ERRORS_SCHEMA' to the '@NgModule.schemas' of this component.`);
});
});

// Test both sync and async compilations, see https://github.com/angular/angular/issues/32538
Expand All @@ -1314,7 +1363,7 @@ export declare class AnimationEvent {
it('should be correct for direct templates', async() => {
env.write('test.ts', `
import {Component, NgModule} from '@angular/core';
@Component({
selector: 'test',
template: \`<p>
Expand All @@ -1334,7 +1383,7 @@ export declare class AnimationEvent {
it('should be correct for indirect templates', async() => {
env.write('test.ts', `
import {Component, NgModule} from '@angular/core';
const TEMPLATE = \`<p>
{{user.does_not_exist}}
</p>\`;
Expand All @@ -1360,7 +1409,7 @@ export declare class AnimationEvent {
</p>`);
env.write('test.ts', `
import {Component, NgModule} from '@angular/core';
@Component({
selector: 'test',
Expand Down
32 changes: 32 additions & 0 deletions packages/core/test/acceptance/ng_module_spec.ts
View file
Expand Up @@ -311,5 +311,37 @@ describe('NgModule', () => {
}).not.toThrow();
});

onlyInIvy('Unknown property warning logged instead of throwing an error')
.it('should not warn about HTML elements inside an SVG foreignObject', () => {
@Component({
selector: 'my-comp',
template: `
<svg>
<svg:foreignObject>
<xhtml:div>Hello</xhtml:div>
</svg:foreignObject>
</svg>
`,
})
class MyComp {
condition = true;
}

@NgModule({
imports: [CommonModule],
declarations: [MyComp],
})
class MyModule {
}

TestBed.configureTestingModule({imports: [MyModule]});

const spy = spyOn(console, 'warn');
const fixture = TestBed.createComponent(MyComp);
fixture.detectChanges();
expect(spy).not.toHaveBeenCalled();
});


});
});

0 comments on commit 4405bb8

Please sign in to comment.