From f2dc32e5c7e50e6258c10fe34502c565edc8cac1 Mon Sep 17 00:00:00 2001 From: Pawel Kozlowski Date: Mon, 11 Mar 2019 14:26:20 +0100 Subject: [PATCH] fix(core): don't wrap `` and `` elements into a required parent (#29219) BREAKING CHANGE: Certain elements (like `` or ``) require parent elements to be of a certain type by the HTML specification (ex. can only be inside / ). Before this change Angular template parser was auto-correcting "invalid" HTML using the following rules: - `` would be wrapped in `` if not inside ``, `` or ``; - `` would be wrapped in `` if not inside ``. 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 `` / `` are at the root of a component's content, ex.: ```html ... ``` In the above example the `` component culd be "surprised" to see additional `` elements inserted by Angular HTML parser. PR Close #29219 --- packages/compiler/src/ml_parser/html_tags.ts | 38 +-------- packages/compiler/src/ml_parser/parser.ts | 9 --- packages/compiler/src/ml_parser/tags.ts | 4 - .../test/ml_parser/html_parser_spec.ts | 78 +++---------------- packages/core/test/acceptance/query_spec.ts | 27 +++++++ 5 files changed, 42 insertions(+), 114 deletions(-) diff --git a/packages/compiler/src/ml_parser/html_tags.ts b/packages/compiler/src/ml_parser/html_tags.ts index 0b49ab978d..f2dc36a415 100644 --- a/packages/compiler/src/ml_parser/html_tags.ts +++ b/packages/compiler/src/ml_parser/html_tags.ts @@ -12,10 +12,6 @@ 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; @@ -23,12 +19,10 @@ export class HtmlTagDefinition implements TagDefinition { 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, @@ -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; } @@ -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}), diff --git a/packages/compiler/src/ml_parser/parser.ts b/packages/compiler/src/ml_parser/parser.ts index d1fe41608f..88aca6d6f3 100644 --- a/packages/compiler/src/ml_parser/parser.ts +++ b/packages/compiler/src/ml_parser/parser.ts @@ -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); } diff --git a/packages/compiler/src/ml_parser/tags.ts b/packages/compiler/src/ml_parser/tags.ts index 6428f9df45..038f4fe053 100644 --- a/packages/compiler/src/ml_parser/tags.ts +++ b/packages/compiler/src/ml_parser/tags.ts @@ -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; } diff --git a/packages/compiler/test/ml_parser/html_parser_spec.ts b/packages/compiler/test/ml_parser/html_parser_spec.ts index 573001455b..3cc1b89353 100644 --- a/packages/compiler/test/ml_parser/html_parser_spec.ts +++ b/packages/compiler/test/ml_parser/html_parser_spec.ts @@ -113,73 +113,17 @@ import {humanizeDom, humanizeDomSourceSpans, humanizeLineColumn} from './ast_spe ]); }); - it('should add the requiredParent', () => { - expect( - humanizeDom(parser.parse( - '
', - '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( - '
', '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('

', '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( - '
', - '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 ', () => { - expect(humanizeDom(parser.parse('', '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('', 'TestComp'))).toEqual([ - [html.Element, 'tr', 0], + /** + * Certain elements (like or ) require parent elements of a certain type (ex. + * can only be inside / ). 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('
', 'TestComp'))).toEqual([ + [html.Element, 'div', 0], + [html.Element, 'tr', 1], ]); }); diff --git a/packages/core/test/acceptance/query_spec.ts b/packages/core/test/acceptance/query_spec.ts index a3e971ae9a..71005714a3 100644 --- a/packages/core/test/acceptance/query_spec.ts +++ b/packages/core/test/acceptance/query_spec.ts @@ -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; + } + @Component( + {selector: 'test-cmpt', template: ``}) + 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', () => {