Skip to content

Commit f2dc32e

Browse files
pkozlowski-opensourcematsko
pkozlowski-opensource
authored and
matsko
committedMar 14, 2019
fix(core): don't wrap <tr> and <col> elements into a required parent (#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
·
zone.js-0.15.08.0.0-beta.9
1 parent 019e65a commit f2dc32e

File tree

5 files changed

+42
-114
lines changed

5 files changed

+42
-114
lines changed
 

‎packages/compiler/src/ml_parser/html_tags.ts

+4-34
Original file line numberDiff line numberDiff line change
@@ -12,23 +12,17 @@ export class HtmlTagDefinition implements TagDefinition {
1212
private closedByChildren: {[key: string]: boolean} = {};
1313

1414
closedByParent: boolean = false;
15-
// TODO(issue/24571): remove '!'.
16-
requiredParents !: {[key: string]: boolean};
17-
// TODO(issue/24571): remove '!'.
18-
parentToAdd !: string;
1915
implicitNamespacePrefix: string|null;
2016
contentType: TagContentType;
2117
isVoid: boolean;
2218
ignoreFirstLf: boolean;
2319
canSelfClose: boolean = false;
2420

2521
constructor(
26-
{closedByChildren, requiredParents, implicitNamespacePrefix,
27-
contentType = TagContentType.PARSABLE_DATA, closedByParent = false, isVoid = false,
28-
ignoreFirstLf = false}: {
22+
{closedByChildren, implicitNamespacePrefix, contentType = TagContentType.PARSABLE_DATA,
23+
closedByParent = false, isVoid = false, ignoreFirstLf = false}: {
2924
closedByChildren?: string[],
3025
closedByParent?: boolean,
31-
requiredParents?: string[],
3226
implicitNamespacePrefix?: string,
3327
contentType?: TagContentType,
3428
isVoid?: boolean,
@@ -39,31 +33,11 @@ export class HtmlTagDefinition implements TagDefinition {
3933
}
4034
this.isVoid = isVoid;
4135
this.closedByParent = closedByParent || isVoid;
42-
if (requiredParents && requiredParents.length > 0) {
43-
this.requiredParents = {};
44-
// The first parent is the list is automatically when none of the listed parents are present
45-
this.parentToAdd = requiredParents[0];
46-
requiredParents.forEach(tagName => this.requiredParents[tagName] = true);
47-
}
4836
this.implicitNamespacePrefix = implicitNamespacePrefix || null;
4937
this.contentType = contentType;
5038
this.ignoreFirstLf = ignoreFirstLf;
5139
}
5240

53-
requireExtraParent(currentParent: string): boolean {
54-
if (!this.requiredParents) {
55-
return false;
56-
}
57-
58-
if (!currentParent) {
59-
return true;
60-
}
61-
62-
const lcParent = currentParent.toLowerCase();
63-
const isParentTemplate = lcParent === 'template' || currentParent === 'ng-template';
64-
return !isParentTemplate && this.requiredParents[lcParent] != true;
65-
}
66-
6741
isClosedByChild(name: string): boolean {
6842
return this.isVoid || name.toLowerCase() in this.closedByChildren;
6943
}
@@ -104,14 +78,10 @@ export function getHtmlTagDefinition(tagName: string): HtmlTagDefinition {
10478
'thead': new HtmlTagDefinition({closedByChildren: ['tbody', 'tfoot']}),
10579
'tbody': new HtmlTagDefinition({closedByChildren: ['tbody', 'tfoot'], closedByParent: true}),
10680
'tfoot': new HtmlTagDefinition({closedByChildren: ['tbody'], closedByParent: true}),
107-
'tr': new HtmlTagDefinition({
108-
closedByChildren: ['tr'],
109-
requiredParents: ['tbody', 'tfoot', 'thead'],
110-
closedByParent: true
111-
}),
81+
'tr': new HtmlTagDefinition({closedByChildren: ['tr'], closedByParent: true}),
11282
'td': new HtmlTagDefinition({closedByChildren: ['td', 'th'], closedByParent: true}),
11383
'th': new HtmlTagDefinition({closedByChildren: ['td', 'th'], closedByParent: true}),
114-
'col': new HtmlTagDefinition({requiredParents: ['colgroup'], isVoid: true}),
84+
'col': new HtmlTagDefinition({isVoid: true}),
11585
'svg': new HtmlTagDefinition({implicitNamespacePrefix: 'svg'}),
11686
'math': new HtmlTagDefinition({implicitNamespacePrefix: 'math'}),
11787
'li': new HtmlTagDefinition({closedByChildren: ['li'], closedByParent: true}),

‎packages/compiler/src/ml_parser/parser.ts

-9
Original file line numberDiff line numberDiff line change
@@ -274,15 +274,6 @@ class _TreeBuilder {
274274
this._elementStack.pop();
275275
}
276276

277-
const tagDef = this.getTagDefinition(el.name);
278-
const {parent, container} = this._getParentElementSkippingContainers();
279-
280-
if (parent && tagDef.requireExtraParent(parent.name)) {
281-
const newParent = new html.Element(
282-
tagDef.parentToAdd, [], [], el.sourceSpan, el.startSourceSpan, el.endSourceSpan);
283-
this._insertBeforeContainer(parent, container, newParent);
284-
}
285-
286277
this._addToParent(el);
287278
this._elementStack.push(el);
288279
}

‎packages/compiler/src/ml_parser/tags.ts

-4
Original file line numberDiff line numberDiff line change
@@ -14,16 +14,12 @@ export enum TagContentType {
1414

1515
export interface TagDefinition {
1616
closedByParent: boolean;
17-
requiredParents: {[key: string]: boolean};
18-
parentToAdd: string;
1917
implicitNamespacePrefix: string|null;
2018
contentType: TagContentType;
2119
isVoid: boolean;
2220
ignoreFirstLf: boolean;
2321
canSelfClose: boolean;
2422

25-
requireExtraParent(currentParent: string): boolean;
26-
2723
isClosedByChild(name: string): boolean;
2824
}
2925

‎packages/compiler/test/ml_parser/html_parser_spec.ts

+11-67
Original file line numberDiff line numberDiff line change
@@ -113,73 +113,17 @@ import {humanizeDom, humanizeDomSourceSpans, humanizeLineColumn} from './ast_spe
113113
]);
114114
});
115115

116-
it('should add the requiredParent', () => {
117-
expect(
118-
humanizeDom(parser.parse(
119-
'<table><thead><tr head></tr></thead><tr noparent></tr><tbody><tr body></tr></tbody><tfoot><tr foot></tr></tfoot></table>',
120-
'TestComp')))
121-
.toEqual([
122-
[html.Element, 'table', 0],
123-
[html.Element, 'thead', 1],
124-
[html.Element, 'tr', 2],
125-
[html.Attribute, 'head', ''],
126-
[html.Element, 'tbody', 1],
127-
[html.Element, 'tr', 2],
128-
[html.Attribute, 'noparent', ''],
129-
[html.Element, 'tbody', 1],
130-
[html.Element, 'tr', 2],
131-
[html.Attribute, 'body', ''],
132-
[html.Element, 'tfoot', 1],
133-
[html.Element, 'tr', 2],
134-
[html.Attribute, 'foot', ''],
135-
]);
136-
});
137-
138-
it('should append the required parent considering ng-container', () => {
139-
expect(humanizeDom(parser.parse(
140-
'<table><ng-container><tr></tr></ng-container></table>', 'TestComp')))
141-
.toEqual([
142-
[html.Element, 'table', 0],
143-
[html.Element, 'tbody', 1],
144-
[html.Element, 'ng-container', 2],
145-
[html.Element, 'tr', 3],
146-
]);
147-
});
148-
149-
it('should append the required parent considering top level ng-container', () => {
150-
expect(humanizeDom(
151-
parser.parse('<ng-container><tr></tr></ng-container><p></p>', 'TestComp')))
152-
.toEqual([
153-
[html.Element, 'ng-container', 0],
154-
[html.Element, 'tr', 1],
155-
[html.Element, 'p', 0],
156-
]);
157-
});
158-
159-
it('should special case ng-container when adding a required parent', () => {
160-
expect(humanizeDom(parser.parse(
161-
'<table><thead><ng-container><tr></tr></ng-container></thead></table>',
162-
'TestComp')))
163-
.toEqual([
164-
[html.Element, 'table', 0],
165-
[html.Element, 'thead', 1],
166-
[html.Element, 'ng-container', 2],
167-
[html.Element, 'tr', 3],
168-
]);
169-
});
170-
171-
it('should not add the requiredParent when the parent is a <ng-template>', () => {
172-
expect(humanizeDom(parser.parse('<ng-template><tr></tr></ng-template>', 'TestComp')))
173-
.toEqual([
174-
[html.Element, 'ng-template', 0],
175-
[html.Element, 'tr', 1],
176-
]);
177-
});
178-
179-
// https://github.com/angular/angular/issues/5967
180-
it('should not add the requiredParent to a template root element', () => {
181-
expect(humanizeDom(parser.parse('<tr></tr>', 'TestComp'))).toEqual([
182-
[html.Element, 'tr', 0],
116+
/**
117+
* Certain elements (like <tr> or <col>) require parent elements of a certain type (ex. <tr>
118+
* can only be inside <tbody> / <thead>). The Angular HTML parser doesn't validate those
119+
* HTML compliancy rules as "problematic" elements can be projected - in such case HTML (as
120+
* written in an Angular template) might be "invalid" (spec-wise) but the resulting DOM will
121+
* still be correct.
122+
*/
123+
it('should not wraps elements in a required parent', () => {
124+
expect(humanizeDom(parser.parse('<div><tr></tr></div>', 'TestComp'))).toEqual([
125+
[html.Element, 'div', 0],
126+
[html.Element, 'tr', 1],
183127
]);
184128
});
185129

‎packages/core/test/acceptance/query_spec.ts

+27
Original file line numberDiff line numberDiff line change
@@ -401,6 +401,33 @@ describe('query logic', () => {
401401

402402
});
403403

404+
describe('descendants', () => {
405+
406+
it('should match directives on elements that used to be wrapped by a required parent in HTML parser',
407+
() => {
408+
409+
@Directive({selector: '[myDef]'})
410+
class MyDef {
411+
}
412+
413+
@Component({selector: 'my-container', template: ``})
414+
class MyContainer {
415+
@ContentChildren(MyDef) myDefs !: QueryList<MyDef>;
416+
}
417+
@Component(
418+
{selector: 'test-cmpt', template: `<my-container><tr myDef></tr></my-container>`})
419+
class TestCmpt {
420+
}
421+
422+
TestBed.configureTestingModule({declarations: [TestCmpt, MyContainer, MyDef]});
423+
const fixture = TestBed.createComponent(TestCmpt);
424+
const cmptWithQuery = fixture.debugElement.children[0].injector.get(MyContainer);
425+
426+
fixture.detectChanges();
427+
expect(cmptWithQuery.myDefs.length).toBe(1);
428+
});
429+
});
430+
404431
describe('observable interface', () => {
405432

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

0 commit comments

Comments
 (0)
Please sign in to comment.