Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

FW-1141 (requiredParents insertion mechanism) #29219

Closed
wants to merge 2 commits into from
Closed

FW-1141 (requiredParents insertion mechanism) #29219

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4f472d6
fix(core): don't wrap `<tr>` and `<col>` elements into a required parent
pkozlowski-opensource Mar 11, 2019
d6bb919
test(ivy): enable passing Material tests
pkozlowski-opensource Mar 11, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 4 additions & 34 deletions packages/compiler/src/ml_parser/html_tags.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
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
Open in desktop
Original file line number Diff line number Diff line change
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
Open in desktop
Original file line number Diff line number Diff line change
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
Open in desktop
Original file line number Diff line number Diff line change
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
Open in desktop
Original file line number Diff line number Diff line change
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
62 changes: 1 addition & 61 deletions tools/material-ci/angular_material_test_blocklist.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,38 +17,6 @@
// tslint:disable

window.testBlocklist = {
"CdkTable should be able to render multiple header and footer rows": {
"error": "Error: Missing definitions for header, footer, and row; cannot determine which columns should be rendered.",
"notes": "FW-1141: Direct ContentChildren not found for <tr> tags without a <tbody>"
},
"CdkTable should render correctly when using native HTML tags": {
"error": "Error: Missing definitions for header, footer, and row; cannot determine which columns should be rendered.",
"notes": "FW-1141: Direct ContentChildren not found for <tr> tags without a <tbody>"
},
"CdkTable with sticky positioning on native table layout should stick and unstick headers": {
"error": "Error: Missing definitions for header, footer, and row; cannot determine which columns should be rendered.",
"notes": "FW-1141: Direct ContentChildren not found for <tr> tags without a <tbody>"
},
"CdkTable with sticky positioning on native table layout should stick and unstick footers": {
"error": "Error: Missing definitions for header, footer, and row; cannot determine which columns should be rendered.",
"notes": "FW-1141: Direct ContentChildren not found for <tr> tags without a <tbody>"
},
"CdkTable with sticky positioning on native table layout should stick tfoot when all rows are stuck": {
"error": "Error: Missing definitions for header, footer, and row; cannot determine which columns should be rendered.",
"notes": "FW-1141: Direct ContentChildren not found for <tr> tags without a <tbody>"
},
"CdkTable with sticky positioning on native table layout should stick and unstick left columns": {
"error": "Error: Missing definitions for header, footer, and row; cannot determine which columns should be rendered.",
"notes": "FW-1141: Direct ContentChildren not found for <tr> tags without a <tbody>"
},
"CdkTable with sticky positioning on native table layout should stick and unstick right columns": {
"error": "Error: Missing definitions for header, footer, and row; cannot determine which columns should be rendered.",
"notes": "FW-1141: Direct ContentChildren not found for <tr> tags without a <tbody>"
},
"CdkTable with sticky positioning on native table layout should stick and unstick combination of sticky header, footer, and columns": {
"error": "Error: Missing definitions for header, footer, and row; cannot determine which columns should be rendered.",
"notes": "FW-1141: Direct ContentChildren not found for <tr> tags without a <tbody>"
},
"CdkTree flat tree should initialize should be able to use units different from px for the indentation": {
"error": "Error: Failed: Expected node level to be 15rem but was 28px",
"notes": "Breaking change: Change detection follows insertion tree only, not declaration tree (CdkTree is OnPush)"
Expand Down Expand Up @@ -93,30 +61,10 @@ window.testBlocklist = {
"error": "TypeError: Cannot read property 'nativeElement' of undefined",
"notes": "Unknown"
},
"MatStepper basic stepper should not do anything when pressing the ENTER key with a modifier": {
"error": "Error: Expected 0 to be 1, 'Expected index of focused step to increase by 1 after pressing the next key.'.",
"notes": "FW-1146: Components should be able to inherit view queries from directives"
},
"MatStepper basic stepper should not do anything when pressing the SPACE key with a modifier": {
"error": "Error: Expected 0 to be 1, 'Expected index of focused step to increase by 1 after pressing the next key.'.",
"notes": "FW-1146: Components should be able to inherit view queries from directives"
},
"MatStepper linear stepper should not move to next step if current step is pending": {
"error": "TypeError: Cannot read property 'nativeElement' of undefined",
"notes": "Unknown"
},
"MatStepper vertical stepper should support using the left/right arrows to move focus": {
"error": "Error: Expected 0 to be 1, 'Expected index of focused step to increase by 1 after pressing the next key.'.",
"notes": "FW-1146: Components should be able to inherit view queries from directives"
},
"MatStepper vertical stepper should support using the up/down arrows to move focus": {
"error": "Error: Expected 0 to be 1, 'Expected index of focused step to increase by 1 after pressing the next key.'.",
"notes": "FW-1146: Components should be able to inherit view queries from directives"
},
"MatStepper vertical stepper should reverse arrow key focus in RTL mode": {
"error": "Error: Expected 0 to be 1.",
"notes": "FW-1146: Components should be able to inherit view queries from directives"
},
"MatStepper stepper with error state should show error state": {
"error": "TypeError: Cannot read property 'nativeElement' of undefined",
"notes": "Unknown"
Expand All @@ -130,7 +78,7 @@ window.testBlocklist = {
"notes": "Unknown"
},
"MatSidenav should be fixed position when in fixed mode": {
"error": "Error: Expected ng-tns-c23383-0 ng-trigger ng-trigger-transform mat-drawer mat-sidenav mat-drawer-over ng-star-inserted to contain 'mat-sidenav-fixed'.",
"error": "Error: Expected ng-tns-c22979-0 ng-trigger ng-trigger-transform mat-drawer mat-sidenav mat-drawer-over ng-star-inserted to contain 'mat-sidenav-fixed'.",
"notes": "FW-1132: Host class bindings don't work if super class has host class bindings"
},
"MatSidenav should set fixed bottom and top when in fixed mode": {
Expand Down Expand Up @@ -208,14 +156,6 @@ window.testBlocklist = {
"MatTooltip special cases should clear the `-webkit-user-drag` on draggable elements": {
"error": "Error: Expected 'none' to be falsy.",
"notes": "FW-1133: Inline styles are not applied before constructor is run"
},
"MatTable should be able to render a table correctly with native elements": {
"error": "Error: Missing definitions for header, footer, and row; cannot determine which columns should be rendered.",
"notes": "FW-1141: Direct ContentChildren not found for <tr> tags without a <tbody>"
},
"MatTable should apply custom sticky CSS class to sticky cells": {
"error": "Error: Missing definitions for header, footer, and row; cannot determine which columns should be rendered.",
"notes": "FW-1141: Direct ContentChildren not found for <tr> tags without a <tbody>"
}
};
// clang-format on