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

fix(ivy): ViewRef.rootNodes returns rootNodes from child views as well #33457

Closed
wants to merge 5 commits into from
Closed

fix(ivy): ViewRef.rootNodes returns rootNodes from child views as well #33457

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
8d371fa
fix(ivy): `ViewRef.rootNodes` returns rootNodes from child views as w…
mhevery Oct 28, 2019
dc424ff
fixup! fix(ivy): `ViewRef.rootNodes` returns rootNodes from child vie…
mhevery Oct 29, 2019
6a4f953
fixup! fix(ivy): `ViewRef.rootNodes` returns rootNodes from child vie…
mhevery Oct 29, 2019
7cfeec9
fixup! fix(ivy): `ViewRef.rootNodes` returns rootNodes from child vie…
mhevery Oct 29, 2019
f27a91e
fixup! fix(ivy): `ViewRef.rootNodes` returns rootNodes from child vie…
mhevery Oct 29, 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
39 changes: 27 additions & 12 deletions packages/core/src/render3/view_ref.ts
View file
Open in desktop
Expand Up @@ -10,10 +10,13 @@ import {ApplicationRef} from '../application_ref';
import {ChangeDetectorRef as viewEngine_ChangeDetectorRef} from '../change_detection/change_detector_ref';
import {ViewContainerRef as viewEngine_ViewContainerRef} from '../linker/view_container_ref';
import {EmbeddedViewRef as viewEngine_EmbeddedViewRef, InternalViewRef as viewEngine_InternalViewRef} from '../linker/view_ref';
import {assertDefined} from '../util/assert';

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 {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';
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,39 @@ 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;

while (tNodeChild) {
const nativeNode = getNativeByTNodeOrNull(tNodeChild, lView);
function collectNativeNodes(lView: LView, tNode: TNode | null, result: any[]): any[] {
while (tNode) {
ngDevMode && assertNodeOfPossibleTypes(
tNode, TNodeType.Element, TNodeType.Container, TNodeType.Projection,
TNodeType.ElementContainer);
Copy link
Member

Choose a reason for hiding this comment

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

I believe that we need to handle a ICU container, see: https://github.com/angular/angular/pull/33457/files#r340598750

const nativeNode = getNativeByTNodeOrNull(tNode, lView);
nativeNode && result.push(nativeNode);
if (tNodeChild.type === TNodeType.ElementContainer) {
collectNativeNodes(lView, tNodeChild, result);
} else if (tNodeChild.type === TNodeType.Projection) {

if (tNode.type === TNodeType.ElementContainer) {
Copy link
Member

Choose a reason for hiding this comment

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

I don't see a test for the TNodeType.IcuContainer in the series of node type checks. If I recall properly ICU containers "behave" similarly to <ng-container>s. I'm not sure if we can bump into ICU containers here, but:

  • if no, we should add an assert;
  • if yes, we should add a test + handle this case.

Copy link
Member

Choose a reason for hiding this comment

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

Also, I would add an assert to verify that we don't bump into tNode being TNodeType.View (just for completeness, to make sure that we are not missing handling of any TNode type).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • I have added asserts
  • Added tests and was not able to get Icu node to show up here.

collectNativeNodes(lView, tNode.child, result);
} else if (tNode.type === TNodeType.Container) {
const lContainer = lView[tNode.index];
const containerTView = tNode.tViews !as TView;
ngDevMode && assertDefined(containerTView, 'TView expected');
Copy link
Member

Choose a reason for hiding this comment

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

I don't think that this assert is correct for 2 reasons:

  • it refers to a TView defined by this <ng-template> and not inserted into a ViewContainerRef asked on this <ng-template>;
  • many different types of TViews can be inserted into one container so we can't assume that all inserted views share the same TView
  • it can be null when the definition is empty <ng-template></ng-template>, AFAIK

There is a confusion between an embedded view defintion and the the inserted instance here.

const containerFirstChildTNode = containerTView.firstChild !;
if (containerFirstChildTNode !== null) {
for (let i = CONTAINER_HEADER_OFFSET; i < lContainer.length; i++) {
collectNativeNodes(lContainer[i], containerFirstChildTNode, result);
}
}
} else if (tNode.type === 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) {
result.push(getNativeByTNode(currentProjectedNode, parentView));
currentProjectedNode = currentProjectedNode.next;
}
}
tNodeChild = tNodeChild.next;
tNode = tNode.next;
}

return result;
Expand Down
61 changes: 60 additions & 1 deletion packages/core/test/acceptance/view_container_ref_spec.ts
View file
Open in desktop
Expand Up @@ -8,7 +8,7 @@

import {CommonModule, DOCUMENT} from '@angular/common';
import {computeMsgId} from '@angular/compiler';
import {Compiler, Component, ComponentFactoryResolver, Directive, DoCheck, ElementRef, EmbeddedViewRef, ErrorHandler, NO_ERRORS_SCHEMA, NgModule, OnInit, Pipe, PipeTransform, QueryList, RendererFactory2, RendererType2, Sanitizer, TemplateRef, ViewChild, ViewChildren, ViewContainerRef} from '@angular/core';
import {Compiler, Component, ComponentFactoryResolver, Directive, DoCheck, ElementRef, EmbeddedViewRef, ErrorHandler, NO_ERRORS_SCHEMA, NgModule, OnInit, Pipe, PipeTransform, QueryList, RendererFactory2, RendererType2, Sanitizer, TemplateRef, ViewChild, ViewChildren, ViewContainerRef, ViewRef} from '@angular/core';
import {Input} from '@angular/core/src/metadata';
import {ngDevModeResetPerfCounters} from '@angular/core/src/util/ng_dev_mode';
import {TestBed, TestComponentRenderer} from '@angular/core/testing';
Expand Down Expand Up @@ -872,6 +872,65 @@ describe('ViewContainerRef', () => {
'<child vcref="">**A**</child><child>**C**</child><child>**C**</child><child>**B**</child>');
});

it('should recurs into child views when computing rootNodes elements', () => {
class MyDirEmbedded {
// This is a marker for easier debugging.
}

@Directive({selector: '[dir]'})
class MyDir {
viewRef: EmbeddedViewRef<{}>|null = null;

@Input() dir: any;

constructor(public template: TemplateRef<{}>, public viewContainerRef: ViewContainerRef) {
this.viewRef =
this.viewContainerRef.createEmbeddedView(this.template, new MyDirEmbedded());
this.viewRef.detectChanges();
}
}

@Component({
selector: 'edit-form',
template: `
<ng-template [dir]="true">
<div *ngIf="true">Text</div>
Copy link
Member

Choose a reason for hiding this comment

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

Could we add few more test cases:

  • nested <ng-container>s at the root of an embedded view;
  • ICU inside <ng-container> - I'm not sure if we can have them at the root of a view;
  • a directive that asks for a ViewContainerRef at the root of a view (you've got a case where a VCRef is asked on a container);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added test cases

Copy link
Member

Choose a reason for hiding this comment

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

I haven't seen a test for the case where a ViewContainerRef is injected on a regular element.

<ng-container>container-content</ng-container>
<div i18n>assert TNodeType.IcuContainer is never a root node</div>
Copy link
Member

Choose a reason for hiding this comment

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

This is not enough, you need to have an actual ICU expression inside an element with the i18n attribute. Also, having div at the root is not enough as a <div> is a root node so no need to descend any further. An example way of testing it would be <ng-container i18n>Updated {minutes, plural, =0 {just now} =1 {one minute ago} other {several minutes ago}}</ng-container> and here we need to descent into the ICU container.

tl;dr; I still believe that we need to handle the case of a ICU container.

<ng-container i18n>assert TNodeType.IcuContainer is never a root node</ng-container>
</ng-template>
`,
})
class MyComp {
@ViewChild(MyDir, {static: true}) dir !: MyDir;
}

TestBed.configureTestingModule({
declarations: [MyComp, MyDir],
imports: [CommonModule],
});

const fixture = TestBed.createComponent(MyComp);
fixture.detectChanges();

const dirRef = fixture.componentInstance.dir;

// Expecting:
// - One comment node for ngIf anchor.
// - One <div>Text</div> node for the ngIf content.
// - VE ONLY: Extra comment node It is unclear why the VE adds the last one.
const rootNodes = dirRef.viewRef !.rootNodes;
expect(rootNodes.length).toBe(7);
expect(rootNodes[0].textContent).toBe('bindings={\n "ng-reflect-ng-if": "true"\n}');
expect(rootNodes[1].outerHTML).toBe('<div>Text</div>');
expect(rootNodes[2].textContent).toBe(ivyEnabled ? 'ng-container' : '');
expect(rootNodes[3].textContent).toBe('container-content');
expect(rootNodes[4].outerHTML)
.toBe('<div>assert TNodeType.IcuContainer is never a root node</div>');
expect(rootNodes[5].textContent).toBe(ivyEnabled ? 'ng-container' : '');
expect(rootNodes[6].textContent).toBe('assert TNodeType.IcuContainer is never a root node');
});

});

describe('createComponent', () => {
Expand Down