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 1559 - more cases #33493

Closed
wants to merge 5 commits into from
Closed

Fw 1559 - more cases #33493

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5abf9ea
fix(ivy): descend into view containers on ng-template when collecting…
pkozlowski-opensource Oct 30, 2019
aa1c67b
fix(ivy): descend into view containers on elements when collecting ro…
pkozlowski-opensource Oct 30, 2019
d97d5fd
fix(ivy): descend into view containers on ng-container when collectin…
pkozlowski-opensource Oct 30, 2019
9f23b75
fix(ivy): descend into ICU containers when collecting rootNodes
pkozlowski-opensource Oct 30, 2019
87d23cc
test(ivy): remove code duplication from the EmbeddedView.rootNodes tests
pkozlowski-opensource Oct 30, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
50 changes: 36 additions & 14 deletions packages/core/src/render3/view_ref.ts
View file
Open in desktop
Expand Up @@ -12,11 +12,14 @@ import {ViewContainerRef as viewEngine_ViewContainerRef} from '../linker/view_co
import {EmbeddedViewRef as viewEngine_EmbeddedViewRef, InternalViewRef as viewEngine_InternalViewRef} from '../linker/view_ref';

import {checkNoChangesInRootView, checkNoChangesInternal, detectChangesInRootView, detectChangesInternal, markViewDirty, storeCleanupFn} from './instructions/shared';
import {CONTAINER_HEADER_OFFSET} from './interfaces/container';
import {TElementNode, TNode, TNodeType, TViewNode} from './interfaces/node';
import {CONTEXT, FLAGS, HOST, LView, LViewFlags, T_HOST} from './interfaces/view';
import {isLContainer} from './interfaces/type_checks';
import {CONTEXT, FLAGS, HOST, LView, LViewFlags, TVIEW, T_HOST} from './interfaces/view';
import {assertNodeOfPossibleTypes} from './node_assert';
import {destroyLView, renderDetachView} from './node_manipulation';
import {findComponentView, getLViewParent} from './util/view_traversal_utils';
import {getNativeByTNode, getNativeByTNodeOrNull} from './util/view_utils';
import {getNativeByTNode, unwrapRNode} from './util/view_utils';



Expand All @@ -38,7 +41,7 @@ export class ViewRef<T> implements viewEngine_EmbeddedViewRef<T>, viewEngine_Int
get rootNodes(): any[] {
if (this._lView[HOST] == null) {
const tView = this._lView[T_HOST] as TViewNode;
return collectNativeNodes(this._lView, tView, []);
return collectNativeNodes(this._lView, tView.child, []);
}
return [];
}
Expand Down Expand Up @@ -299,27 +302,46 @@ export class RootViewRef<T> extends ViewRef<T> {
get context(): T { return null !; }
}

function collectNativeNodes(lView: LView, parentTNode: TNode, result: any[]): any[] {
let tNodeChild = parentTNode.child;
function collectNativeNodes(lView: LView, tNode: TNode | null, result: any[]): any[] {
while (tNode !== null) {
ngDevMode && assertNodeOfPossibleTypes(
tNode, TNodeType.Element, TNodeType.Container, TNodeType.Projection,
TNodeType.ElementContainer, TNodeType.IcuContainer);

while (tNodeChild) {
const nativeNode = getNativeByTNodeOrNull(tNodeChild, lView);
nativeNode && result.push(nativeNode);
if (tNodeChild.type === TNodeType.ElementContainer) {
collectNativeNodes(lView, tNodeChild, result);
} else if (tNodeChild.type === TNodeType.Projection) {
const lNode = lView[tNode.index];
if (lNode !== null) {
result.push(unwrapRNode(lNode));
}

// A given lNode can represent either a native node or a LContainer (when it is a host of a
// ViewContainerRef). When we find a LContainer we need to descend into it to collect root nodes
// from the views in this container.
if (isLContainer(lNode)) {
for (let i = CONTAINER_HEADER_OFFSET; i < lNode.length; i++) {
const lViewInAContainer = lNode[i];
const lViewFirstChildTNode = lViewInAContainer[TVIEW].firstChild;
if (lViewFirstChildTNode !== null) {
collectNativeNodes(lViewInAContainer, lViewFirstChildTNode, result);
}
}
}

const tNodeType = tNode.type;
if (tNodeType === TNodeType.ElementContainer || tNodeType === TNodeType.IcuContainer) {
collectNativeNodes(lView, tNode.child, result);
} else if (tNodeType === TNodeType.Projection) {
const componentView = findComponentView(lView);
const componentHost = componentView[T_HOST] as TElementNode;
const parentView = getLViewParent(componentView);
let currentProjectedNode: TNode|null =
(componentHost.projection as(TNode | null)[])[tNodeChild.projection as number];
(componentHost.projection as(TNode | null)[])[tNode.projection as number];

while (currentProjectedNode && parentView) {
while (currentProjectedNode !== null && parentView !== null) {
result.push(getNativeByTNode(currentProjectedNode, parentView));
currentProjectedNode = currentProjectedNode.next;
}
}
tNodeChild = tNodeChild.next;
tNode = tNode.next;
}

return result;
Expand Down
229 changes: 122 additions & 107 deletions packages/core/test/acceptance/template_ref_spec.ts
View file
Open in desktop
Expand Up @@ -9,19 +9,61 @@
import {Component, TemplateRef, ViewChild, ViewContainerRef} from '@angular/core';
import {TestBed} from '@angular/core/testing';
import {expect} from '@angular/platform-browser/testing/src/matchers';
import {onlyInIvy} from '@angular/private/testing';
import {ivyEnabled, onlyInIvy} from '@angular/private/testing';

describe('TemplateRef', () => {
describe('rootNodes', () => {
it('should include projected nodes in rootNodes', () => {

@Component({template: `<ng-template #templateRef></ng-template>`})
class App {
@ViewChild('templateRef', {static: true})
templateRef !: TemplateRef<any>;
minutes = 0;
}

function getRootNodes(template: string): any[] {
TestBed.configureTestingModule({
declarations: [App],
});
TestBed.overrideTemplate(App, template);
const fixture = TestBed.createComponent(App);
fixture.detectChanges();

const embeddedView = fixture.componentInstance.templateRef.createEmbeddedView({});
embeddedView.detectChanges();

return embeddedView.rootNodes;
}


it('should return root render nodes for an embedded view instance', () => {
const rootNodes =
getRootNodes(`<ng-template #templateRef><div></div>some text<span></span></ng-template>`);
expect(rootNodes.length).toBe(3);
});

/**
* This is different as compared to the view engine implementation which returns a comment node
* in this case:
* https://stackblitz.com/edit/angular-uiqry6?file=src/app/app.component.ts
*
* Returning a comment node for a template ref with no nodes is wrong is fixed in Ivy.
*/
onlyInIvy('Fixed: Ivy no longer adds a comment node in this case.')
.it('should return an empty array for embedded view with no nodes', () => {
const rootNodes = getRootNodes('<ng-template #templateRef></ng-template>');
expect(rootNodes.length).toBe(0);
});

it('should include projected nodes', () => {
@Component({
selector: 'menu-content',
template: `
<ng-template>
Header
<ng-content></ng-content>
</ng-template>
`,
<ng-template>
Header
<ng-content></ng-content>
</ng-template>
`,
exportAs: 'menuContent'
})
class MenuContent {
Expand All @@ -30,11 +72,11 @@ describe('TemplateRef', () => {

@Component({
template: `
<menu-content #menu="menuContent">
<button>Item one</button>
<button>Item two</button>
</menu-content>
`
<menu-content #menu="menuContent">
<button>Item one</button>
<button>Item two</button>
</menu-content>
`
})
class App {
@ViewChild(MenuContent) content !: MenuContent;
Expand All @@ -54,116 +96,89 @@ describe('TemplateRef', () => {
expect(rootNodeTextContent).toEqual(['Header', 'Item one', 'Item two']);
});

it('should return root render nodes for an embedded view instance', () => {
@Component({
template: `
<ng-template #templateRef>
<div></div>
some text
<span></span>
</ng-template>
`
})
class App {
@ViewChild('templateRef', {static: true})
templateRef !: TemplateRef<any>;
}

TestBed.configureTestingModule({
declarations: [App],
});
const fixture = TestBed.createComponent(App);
fixture.detectChanges();

const embeddedView = fixture.componentInstance.templateRef.createEmbeddedView({});
expect(embeddedView.rootNodes.length).toBe(3);
it('should descend into view containers on ng-template', () => {
/**
* NOTE: In VE, if `SUFFIX` text node below is _not_ present, VE will add an
* additional `<!---->` comment, thus being slightly different than Ivy.
* (resulting in 1 root node in Ivy and 2 in VE).
*/
const rootNodes = getRootNodes(`
<ng-template #templateRef>
<ng-template [ngIf]="true">text|</ng-template>SUFFIX
</ng-template>`);

expect(rootNodes.length).toBe(3);
expect(rootNodes[0].nodeType).toBe(Node.COMMENT_NODE);
expect(rootNodes[1].nodeType).toBe(Node.TEXT_NODE);
expect(rootNodes[2].nodeType).toBe(Node.TEXT_NODE);
});

/**
* This is different as compared to the view engine implementation which returns a comment node
* in this case:
* https://stackblitz.com/edit/angular-uiqry6?file=src/app/app.component.ts
*
* Returning a comment node for a template ref with no nodes is wrong is fixed in Ivy.
*/
onlyInIvy('Fixed: Ivy no longer adds a comment node in this case.')
.it('should return an empty array for embedded view with no nodes', () => {
@Component({
template: `
<ng-template #templateRef></ng-template>
`
})
class App {
@ViewChild('templateRef', {static: true})
templateRef !: TemplateRef<any>;
}

TestBed.configureTestingModule({
declarations: [App],
});
const fixture = TestBed.createComponent(App);
fixture.detectChanges();

const embeddedView = fixture.componentInstance.templateRef.createEmbeddedView({});
expect(embeddedView.rootNodes.length).toBe(0);
});

it('should not descend into containers when retrieving root nodes', () => {
it('should descend into view containers on an element', () => {
/**
* NOTE: In VE, if `SUFFIX` text node below is _not_ present, VE will add an
* additional `<!---->` comment, thus being slightly different than Ivy.
* (resulting in 1 root node in Ivy and 2 in VE).
*/
@Component({
template: `
<ng-template #templateRef><ng-template [ngIf]="true">text</ng-template>SUFFIX</ng-template>
`
})
class App {
@ViewChild('templateRef', {static: true})
templateRef !: TemplateRef<any>;
}

TestBed.configureTestingModule({
declarations: [App],
});
const fixture = TestBed.createComponent(App);
fixture.detectChanges();
const rootNodes = getRootNodes(`
<ng-template #dynamicTpl>text</ng-template>
<ng-template #templateRef>
<div [ngTemplateOutlet]="dynamicTpl"></div>SUFFIX
</ng-template>
`);

expect(rootNodes.length).toBe(3);
expect(rootNodes[0].nodeType).toBe(Node.ELEMENT_NODE);
expect(rootNodes[1].nodeType).toBe(Node.TEXT_NODE);
expect(rootNodes[2].nodeType).toBe(Node.TEXT_NODE);
});

const embeddedView = fixture.componentInstance.templateRef.createEmbeddedView({});
expect(embeddedView.rootNodes.length).toBe(2);
expect(embeddedView.rootNodes[0].nodeType).toBe(Node.COMMENT_NODE);
expect(embeddedView.rootNodes[1].nodeType).toBe(Node.TEXT_NODE);
it('should descend into view containers on ng-container', () => {
/**
* NOTE: In VE, if `SUFFIX` text node below is _not_ present, VE will add an
* additional `<!---->` comment, thus being slightly different than Ivy.
* (resulting in 1 root node in Ivy and 2 in VE).
*/
const rootNodes = getRootNodes(`
<ng-template #dynamicTpl>text</ng-template>
<ng-template #templateRef><ng-container [ngTemplateOutlet]="dynamicTpl"></ng-container>SUFFIX</ng-template>
`);

expect(rootNodes.length).toBe(3);
expect(rootNodes[0].nodeType).toBe(Node.COMMENT_NODE);
expect(rootNodes[1].nodeType).toBe(Node.TEXT_NODE);
expect(rootNodes[2].nodeType).toBe(Node.TEXT_NODE);
});

/**
* Contrary to containers (<ng-template>) we _do_ descend into element containers
* (<ng-container>)
*/
it('should descend into element containers when retrieving root nodes', () => {
@Component({
template: `
it('should descend into element containers', () => {
const rootNodes = getRootNodes(`
<ng-template #templateRef>
<ng-container>text</ng-container>
</ng-template>
`
})
class App {
@ViewChild('templateRef', {static: true})
templateRef !: TemplateRef<any>;
}

TestBed.configureTestingModule({
declarations: [App],
});
const fixture = TestBed.createComponent(App);
fixture.detectChanges();
`);

const embeddedView = fixture.componentInstance.templateRef.createEmbeddedView({});
expect(rootNodes.length).toBe(2);
expect(rootNodes[0].nodeType).toBe(Node.COMMENT_NODE);
expect(rootNodes[1].nodeType).toBe(Node.TEXT_NODE);
});

expect(embeddedView.rootNodes.length).toBe(2);
expect(embeddedView.rootNodes[0].nodeType).toBe(Node.COMMENT_NODE);
expect(embeddedView.rootNodes[1].nodeType).toBe(Node.TEXT_NODE);
it('should descend into ICU containers', () => {
const rootNodes = getRootNodes(`
<ng-template #templateRef>
<ng-container i18n>Updated {minutes, select, =0 {just now} other {some time ago}}</ng-container>
</ng-template>
`);

if (ivyEnabled) {
expect(rootNodes.length).toBe(4);
expect(rootNodes[0].nodeType).toBe(Node.COMMENT_NODE); // ng-container
expect(rootNodes[1].nodeType).toBe(Node.TEXT_NODE); // "Updated " text
expect(rootNodes[2].nodeType).toBe(Node.COMMENT_NODE); // ICU container
expect(rootNodes[3].nodeType).toBe(Node.TEXT_NODE); // "one minute ago" text
} else {
// ViewEngine seems to produce very different DOM structure as compared to ivy
// when it comes to ICU containers - this needs more investigation / fix.
expect(rootNodes.length).toBe(7);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AndrewKushnir it seems like VE and ivy are generating very different DOM when it comes to ICU expressions. Not sure if this is intended and if not, are we tracking it anywhere?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be expected for ICU expressions.

}
});
});
});