Skip to content

Commit

Permalink
fix(compiler): incorrectly inferring namespace for HTML nodes inside …
Browse files Browse the repository at this point in the history
…SVG (#38477)

The HTML parser gets an element's namespace either from the tag name
(e.g. `<svg:rect>`) or from its parent element `<svg><rect></svg>`) which
breaks down when an element is inside of an SVG `foreignElement`,
because foreign elements allow nodes from a different namespace to be
inserted into an SVG.

These changes add another flag to the tag definitions which tells child
nodes whether to try to inherit their namespaces from their parents.
It also adds a definition for `foreignObject` with the new flag,
allowing elements placed inside it to infer their namespaces instead.

Fixes #37218.

PR Close #38477
  • Loading branch information
crisbeto authored and josephperrott committed Aug 31, 2020
1 parent 5e4aeaa commit 0dda97e
Show file tree
Hide file tree
Showing 5 changed files with 78 additions and 5 deletions.
49 changes: 49 additions & 0 deletions packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts
Expand Up @@ -2017,6 +2017,28 @@ export declare class AnimationEvent {
expect(diags.length).toBe(0);
});

it('should allow HTML elements without explicit namespace inside SVG foreignObject', () => {
env.write('test.ts', `
import {Component, NgModule} from '@angular/core';
@Component({
template: \`
<svg>
<foreignObject>
<div>Hello</div>
</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';
Expand All @@ -2042,6 +2064,33 @@ export declare class AnimationEvent {
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.`);
});

it('should check for unknown elements without explicit namespace inside an SVG foreignObject',
() => {
env.write('test.ts', `
import {Component, NgModule} from '@angular/core';
@Component({
selector: 'blah',
template: \`
<svg>
<foreignObject>
<foo>Hello</foo>
</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 Down
24 changes: 21 additions & 3 deletions packages/compiler/src/ml_parser/html_tags.ts
Expand Up @@ -17,21 +17,24 @@ export class HtmlTagDefinition implements TagDefinition {
isVoid: boolean;
ignoreFirstLf: boolean;
canSelfClose: boolean = false;
preventNamespaceInheritance: boolean;

constructor({
closedByChildren,
implicitNamespacePrefix,
contentType = TagContentType.PARSABLE_DATA,
closedByParent = false,
isVoid = false,
ignoreFirstLf = false
ignoreFirstLf = false,
preventNamespaceInheritance = false
}: {
closedByChildren?: string[],
closedByParent?: boolean,
implicitNamespacePrefix?: string,
contentType?: TagContentType,
isVoid?: boolean,
ignoreFirstLf?: boolean
ignoreFirstLf?: boolean,
preventNamespaceInheritance?: boolean
} = {}) {
if (closedByChildren && closedByChildren.length > 0) {
closedByChildren.forEach(tagName => this.closedByChildren[tagName] = true);
Expand All @@ -41,6 +44,7 @@ export class HtmlTagDefinition implements TagDefinition {
this.implicitNamespacePrefix = implicitNamespacePrefix || null;
this.contentType = contentType;
this.ignoreFirstLf = ignoreFirstLf;
this.preventNamespaceInheritance = preventNamespaceInheritance;
}

isClosedByChild(name: string): boolean {
Expand Down Expand Up @@ -88,6 +92,17 @@ export function getHtmlTagDefinition(tagName: string): HtmlTagDefinition {
'th': new HtmlTagDefinition({closedByChildren: ['td', 'th'], closedByParent: true}),
'col': new HtmlTagDefinition({isVoid: true}),
'svg': new HtmlTagDefinition({implicitNamespacePrefix: 'svg'}),
'foreignObject': new HtmlTagDefinition({
// Usually the implicit namespace here would be redundant since it will be inherited from
// the parent `svg`, but we have to do it for `foreignObject`, because the way the parser
// works is that the parent node of an end tag is its own start tag which means that
// the `preventNamespaceInheritance` on `foreignObject` would have it default to the
// implicit namespace which is `html`, unless specified otherwise.
implicitNamespacePrefix: 'svg',
// We want to prevent children of foreignObject from inheriting its namespace, because
// the point of the element is to allow nodes from other namespaces to be inserted.
preventNamespaceInheritance: true,
}),
'math': new HtmlTagDefinition({implicitNamespacePrefix: 'math'}),
'li': new HtmlTagDefinition({closedByChildren: ['li'], closedByParent: true}),
'dt': new HtmlTagDefinition({closedByChildren: ['dt', 'dd']}),
Expand All @@ -111,5 +126,8 @@ export function getHtmlTagDefinition(tagName: string): HtmlTagDefinition {
{contentType: TagContentType.ESCAPABLE_RAW_TEXT, ignoreFirstLf: true}),
};
}
return TAG_DEFINITIONS[tagName.toLowerCase()] || _DEFAULT_TAG_DEFINITION;
// We have to make both a case-sensitive and a case-insesitive lookup, because
// HTML tag names are case insensitive, whereas some SVG tags are case sensitive.
return TAG_DEFINITIONS[tagName] ?? TAG_DEFINITIONS[tagName.toLowerCase()] ??
_DEFAULT_TAG_DEFINITION;
}
8 changes: 6 additions & 2 deletions packages/compiler/src/ml_parser/parser.ts
Expand Up @@ -10,7 +10,7 @@ import {ParseError, ParseSourceSpan} from '../parse_util';

import * as html from './ast';
import * as lex from './lexer';
import {getNsPrefix, isNgContainer, mergeNsAndName, TagDefinition} from './tags';
import {getNsPrefix, mergeNsAndName, splitNsName, TagDefinition} from './tags';

export class TreeError extends ParseError {
static create(elementName: string|null, span: ParseSourceSpan, msg: string): TreeError {
Expand Down Expand Up @@ -353,7 +353,11 @@ class _TreeBuilder {
if (prefix === '') {
prefix = this.getTagDefinition(localName).implicitNamespacePrefix || '';
if (prefix === '' && parentElement != null) {
prefix = getNsPrefix(parentElement.name);
const parentTagName = splitNsName(parentElement.name)[1];
const parentTagDefinition = this.getTagDefinition(parentTagName);
if (!parentTagDefinition.preventNamespaceInheritance) {
prefix = getNsPrefix(parentElement.name);
}
}
}

Expand Down
1 change: 1 addition & 0 deletions packages/compiler/src/ml_parser/tags.ts
Expand Up @@ -19,6 +19,7 @@ export interface TagDefinition {
isVoid: boolean;
ignoreFirstLf: boolean;
canSelfClose: boolean;
preventNamespaceInheritance: boolean;

isClosedByChild(name: string): boolean;
}
Expand Down
1 change: 1 addition & 0 deletions packages/compiler/src/ml_parser/xml_tags.ts
Expand Up @@ -20,6 +20,7 @@ export class XmlTagDefinition implements TagDefinition {
isVoid: boolean = false;
ignoreFirstLf: boolean = false;
canSelfClose: boolean = true;
preventNamespaceInheritance: boolean = false;

requireExtraParent(currentParent: string): boolean {
return false;
Expand Down

0 comments on commit 0dda97e

Please sign in to comment.