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

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}),

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);
   }

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;
 }
 

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(
-                  '<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],
+        /**
+         * 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],
-                [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],
           ]);
         });
 

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<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', () => {