Skip to content

Commit

Permalink
fix(core): don't wrap <tr> and <col> elements into a required par…
Browse files Browse the repository at this point in the history 
…ent (#29219)

BREAKING CHANGE:

Certain elements (like `<tr>` or `<col>`) require parent elements to be of a certain type by the HTML specification
(ex. <tr> can only be inside <tbody> / <thead>). Before this change Angular template parser was auto-correcting
"invalid" HTML using the following rules:
- `<tr>` would be wrapped in `<tbody>` if not inside `<tbody>`, `<tfoot>` or `<thead>`;
- `<col>` would be wrapped in `<colgroup>` if not inside `<colgroup>`.

This meachanism of automatic wrapping / auto-correcting was problematic for several reasons:
- it is non-obvious and arbitrary (ex. there are more HTML elements that has rules for parent type);
- it is incorrect for cases where `<tr>` / `<col>` are at the root of a component's content, ex.:

```html
<projecting-tr-inside-tbody>
  <tr>...</tr>
</projecting-tr-inside-tbody>
```

In the above example the `<projecting-tr-inside-tbody>` component culd be "surprised" to see additional
`<tbody>` elements inserted by Angular HTML parser.

PR Close #29219
  • Loading branch information
@pkozlowski-opensource @matsko
pkozlowski-opensource authored and matsko committed Mar 14, 2019
1 parent 019e65a commit f2dc32e
Show file tree
Hide file tree
Showing 5 changed files with 42 additions and 114 deletions.
38 changes: 4 additions & 34 deletions packages/compiler/src/ml_parser/html_tags.ts
View file
Expand Up @@ -12,23 +12,17 @@ export class HtmlTagDefinition implements TagDefinition {
private closedByChildren: {[key: string]: boolean} = {};

closedByParent: boolean = false;
// TODO(issue/24571): remove '!'.
requiredParents !: {[key: string]: boolean};
// TODO(issue/24571): remove '!'.
parentToAdd !: string;
implicitNamespacePrefix: string|null;
contentType: TagContentType;
isVoid: boolean;
ignoreFirstLf: boolean;
canSelfClose: boolean = false;

constructor(
{closedByChildren, requiredParents, implicitNamespacePrefix,
contentType = TagContentType.PARSABLE_DATA, closedByParent = false, isVoid = false,
ignoreFirstLf = false}: {
{closedByChildren, implicitNamespacePrefix, contentType = TagContentType.PARSABLE_DATA,
closedByParent = false, isVoid = false, ignoreFirstLf = false}: {
closedByChildren?: string[],
closedByParent?: boolean,
requiredParents?: string[],
implicitNamespacePrefix?: string,
contentType?: TagContentType,
isVoid?: boolean,
Expand All @@ -39,31 +33,11 @@ export class HtmlTagDefinition implements TagDefinition {
}
this.isVoid = isVoid;
this.closedByParent = closedByParent || isVoid;
if (requiredParents && requiredParents.length > 0) {
this.requiredParents = {};
// The first parent is the list is automatically when none of the listed parents are present
this.parentToAdd = requiredParents[0];
requiredParents.forEach(tagName => this.requiredParents[tagName] = true);
}
this.implicitNamespacePrefix = implicitNamespacePrefix || null;
this.contentType = contentType;
this.ignoreFirstLf = ignoreFirstLf;
}

requireExtraParent(currentParent: string): boolean {
if (!this.requiredParents) {
return false;
}

if (!currentParent) {
return true;
}

const lcParent = currentParent.toLowerCase();
const isParentTemplate = lcParent === 'template' || currentParent === 'ng-template';
return !isParentTemplate && this.requiredParents[lcParent] != true;
}

isClosedByChild(name: string): boolean {
return this.isVoid || name.toLowerCase() in this.closedByChildren;
}
Expand Down Expand Up @@ -104,14 +78,10 @@ export function getHtmlTagDefinition(tagName: string): HtmlTagDefinition {
'thead': new HtmlTagDefinition({closedByChildren: ['tbody', 'tfoot']}),
'tbody': new HtmlTagDefinition({closedByChildren: ['tbody', 'tfoot'], closedByParent: true}),
'tfoot': new HtmlTagDefinition({closedByChildren: ['tbody'], closedByParent: true}),
'tr': new HtmlTagDefinition({
closedByChildren: ['tr'],
requiredParents: ['tbody', 'tfoot', 'thead'],
closedByParent: true
}),
'tr': new HtmlTagDefinition({closedByChildren: ['tr'], closedByParent: true}),
'td': new HtmlTagDefinition({closedByChildren: ['td', 'th'], closedByParent: true}),
'th': new HtmlTagDefinition({closedByChildren: ['td', 'th'], closedByParent: true}),
'col': new HtmlTagDefinition({requiredParents: ['colgroup'], isVoid: true}),
'col': new HtmlTagDefinition({isVoid: true}),
'svg': new HtmlTagDefinition({implicitNamespacePrefix: 'svg'}),
'math': new HtmlTagDefinition({implicitNamespacePrefix: 'math'}),
'li': new HtmlTagDefinition({closedByChildren: ['li'], closedByParent: true}),
Expand Down
9 changes: 0 additions & 9 deletions packages/compiler/src/ml_parser/parser.ts
View file
Expand Up @@ -274,15 +274,6 @@ class _TreeBuilder {
this._elementStack.pop();
}

const tagDef = this.getTagDefinition(el.name);
const {parent, container} = this._getParentElementSkippingContainers();

if (parent && tagDef.requireExtraParent(parent.name)) {
const newParent = new html.Element(
tagDef.parentToAdd, [], [], el.sourceSpan, el.startSourceSpan, el.endSourceSpan);
this._insertBeforeContainer(parent, container, newParent);
}

this._addToParent(el);
this._elementStack.push(el);
}
Expand Down
4 changes: 0 additions & 4 deletions packages/compiler/src/ml_parser/tags.ts
View file
Expand Up @@ -14,16 +14,12 @@ export enum TagContentType {

export interface TagDefinition {
closedByParent: boolean;
requiredParents: {[key: string]: boolean};
parentToAdd: string;
implicitNamespacePrefix: string|null;
contentType: TagContentType;
isVoid: boolean;
ignoreFirstLf: boolean;
canSelfClose: boolean;

requireExtraParent(currentParent: string): boolean;

isClosedByChild(name: string): boolean;
}

Expand Down
78 changes: 11 additions & 67 deletions packages/compiler/test/ml_parser/html_parser_spec.ts
View file
Expand Up @@ -113,73 +113,17 @@ import {humanizeDom, humanizeDomSourceSpans, humanizeLineColumn} from './ast_spe
]);
});

it('should add the requiredParent', () => {
expect(
humanizeDom(parser.parse(
'<table><thead><tr head></tr></thead><tr noparent></tr><tbody><tr body></tr></tbody><tfoot><tr foot></tr></tfoot></table>',
'TestComp')))
.toEqual([
[html.Element, 'table', 0],
[html.Element, 'thead', 1],
[html.Element, 'tr', 2],
[html.Attribute, 'head', ''],
[html.Element, 'tbody', 1],
[html.Element, 'tr', 2],
[html.Attribute, 'noparent', ''],
[html.Element, 'tbody', 1],
[html.Element, 'tr', 2],
[html.Attribute, 'body', ''],
[html.Element, 'tfoot', 1],
[html.Element, 'tr', 2],
[html.Attribute, 'foot', ''],
]);
});

it('should append the required parent considering ng-container', () => {
expect(humanizeDom(parser.parse(
'<table><ng-container><tr></tr></ng-container></table>', 'TestComp')))
.toEqual([
[html.Element, 'table', 0],
[html.Element, 'tbody', 1],
[html.Element, 'ng-container', 2],
[html.Element, 'tr', 3],
]);
});

it('should append the required parent considering top level ng-container', () => {
expect(humanizeDom(
parser.parse('<ng-container><tr></tr></ng-container><p></p>', 'TestComp')))
.toEqual([
[html.Element, 'ng-container', 0],
[html.Element, 'tr', 1],
[html.Element, 'p', 0],
]);
});

it('should special case ng-container when adding a required parent', () => {
expect(humanizeDom(parser.parse(
'<table><thead><ng-container><tr></tr></ng-container></thead></table>',
'TestComp')))
.toEqual([
[html.Element, 'table', 0],
[html.Element, 'thead', 1],
[html.Element, 'ng-container', 2],
[html.Element, 'tr', 3],
]);
});

it('should not add the requiredParent when the parent is a <ng-template>', () => {
expect(humanizeDom(parser.parse('<ng-template><tr></tr></ng-template>', 'TestComp')))
.toEqual([
[html.Element, 'ng-template', 0],
[html.Element, 'tr', 1],
]);
});

// https://github.com/angular/angular/issues/5967
it('should not add the requiredParent to a template root element', () => {
expect(humanizeDom(parser.parse('<tr></tr>', 'TestComp'))).toEqual([
[html.Element, 'tr', 0],
/**
* Certain elements (like <tr> or <col>) require parent elements of a certain type (ex. <tr>
* can only be inside <tbody> / <thead>). The Angular HTML parser doesn't validate those
* HTML compliancy rules as "problematic" elements can be projected - in such case HTML (as
* written in an Angular template) might be "invalid" (spec-wise) but the resulting DOM will
* still be correct.
*/
it('should not wraps elements in a required parent', () => {
expect(humanizeDom(parser.parse('<div><tr></tr></div>', 'TestComp'))).toEqual([
[html.Element, 'div', 0],
[html.Element, 'tr', 1],
]);
});

Expand Down
27 changes: 27 additions & 0 deletions packages/core/test/acceptance/query_spec.ts
View file
Expand Up @@ -401,6 +401,33 @@ describe('query logic', () => {

});

describe('descendants', () => {

it('should match directives on elements that used to be wrapped by a required parent in HTML parser',
() => {

@Directive({selector: '[myDef]'})
class MyDef {
}

@Component({selector: 'my-container', template: ``})
class MyContainer {
@ContentChildren(MyDef) myDefs !: QueryList<MyDef>;
}
@Component(
{selector: 'test-cmpt', template: `<my-container><tr myDef></tr></my-container>`})
class TestCmpt {
}

TestBed.configureTestingModule({declarations: [TestCmpt, MyContainer, MyDef]});
const fixture = TestBed.createComponent(TestCmpt);
const cmptWithQuery = fixture.debugElement.children[0].injector.get(MyContainer);

fixture.detectChanges();
expect(cmptWithQuery.myDefs.length).toBe(1);
});
});

describe('observable interface', () => {

it('should allow observing changes to query list', () => {
Expand Down

0 comments on commit f2dc32e

Please sign in to comment.