From 5393a6495f055cf22fe8a6747627455aeec5bdbc Mon Sep 17 00:00:00 2001 From: sis0k0 Date: Tue, 11 Jul 2017 11:02:54 +0300 Subject: [PATCH 1/5] fix(renderer): attach `CommentNode`s to visual tree fixes #872 --- .../directives/list-view-comp.ts | 4 +- nativescript-angular/directives/tab-view.ts | 4 +- nativescript-angular/element-registry.ts | 51 +++++++++++++++++-- nativescript-angular/renderer.ts | 6 +-- nativescript-angular/view-util.ts | 22 +++----- 5 files changed, 61 insertions(+), 26 deletions(-) diff --git a/nativescript-angular/directives/list-view-comp.ts b/nativescript-angular/directives/list-view-comp.ts index cbf9aa46f..d9fbed07c 100644 --- a/nativescript-angular/directives/list-view-comp.ts +++ b/nativescript-angular/directives/list-view-comp.ts @@ -25,7 +25,7 @@ import { ObservableArray } from "tns-core-modules/data/observable-array"; import { LayoutBase } from "tns-core-modules/ui/layouts/layout-base"; import { profile } from "tns-core-modules/profiling"; -import { CommentNode } from "../element-registry"; +import { InvisibleNode } from "../element-registry"; import { isListLikeIterable } from "../collection-facade"; import { listViewLog, listViewError } from "../trace"; @@ -217,7 +217,7 @@ export class ListViewComponent implements DoCheck, OnDestroy, AfterContentInit { } function getSingleViewRecursive(nodes: Array, nestLevel: number): View { - const actualNodes = nodes.filter(node => !(node instanceof CommentNode)); + const actualNodes = nodes.filter(node => !(node instanceof InvisibleNode)); if (actualNodes.length === 0) { throw new Error(`No suitable views found in list template! ` + diff --git a/nativescript-angular/directives/tab-view.ts b/nativescript-angular/directives/tab-view.ts index 3b578be54..a945531f9 100644 --- a/nativescript-angular/directives/tab-view.ts +++ b/nativescript-angular/directives/tab-view.ts @@ -9,7 +9,7 @@ import { } from "@angular/core"; import { TabView, TabViewItem } from "tns-core-modules/ui/tab-view"; -import { CommentNode } from "../element-registry"; +import { InvisibleNode } from "../element-registry"; import { rendererLog } from "../trace"; import { isBlank } from "../lang-facade"; @@ -105,7 +105,7 @@ export class TabViewItemDirective implements OnInit { const viewRef = this.viewContainer.createEmbeddedView(this.templateRef); // Filter out text nodes and comments const realViews = viewRef.rootNodes.filter(node => - !(node instanceof CommentNode)); + !(node instanceof InvisibleNode)); if (realViews.length > 0) { this.item.view = realViews[0]; diff --git a/nativescript-angular/element-registry.ts b/nativescript-angular/element-registry.ts index 6cf0c6cf0..7c6a44165 100644 --- a/nativescript-angular/element-registry.ts +++ b/nativescript-angular/element-registry.ts @@ -1,7 +1,7 @@ import { View } from "tns-core-modules/ui/core/view"; export type NgView = (View & ViewExtensions); -export type NgElement = NgView | CommentNode; +export type NgElement = NgView | InvisibleNode; export interface ViewExtensions { nodeType: number; @@ -15,12 +15,55 @@ export interface ViewClass { new (): View; } -// used for creating comments and text nodes in the renderer -export class CommentNode { - meta: { skipAddToDom: true }; +export abstract class InvisibleNode extends View implements ViewExtensions { + meta: { skipAddToDom: boolean }; templateParent: NgView; + nodeType: number; + nodeName: string; + ngCssClasses: Map; + + constructor() { + super(); + + this.nodeType = 1; + this.nodeName = getClassName(this); + } + + toString() { + return `${this.nodeName}(${this.id})`; + } } +export class CommentNode extends InvisibleNode { + protected static id = 0; + + constructor() { + super(); + + this.meta = { + skipAddToDom: false, + }; + this.id = CommentNode.id.toString(); + CommentNode.id += 1; + } +} + +export class TextNode extends InvisibleNode { + protected static id = 0; + + constructor() { + super(); + + this.meta = { + skipAddToDom: true, + }; + this.id = TextNode.id.toString(); + TextNode.id += 1; + } +} + +const getClassName = instance => instance.constructor.name; + export interface ViewClassMeta { skipAddToDom?: boolean; insertChild?: (parent: NgView, child: NgView, atIndex: number) => void; diff --git a/nativescript-angular/renderer.ts b/nativescript-angular/renderer.ts index b95318bc3..38f034877 100644 --- a/nativescript-angular/renderer.ts +++ b/nativescript-angular/renderer.ts @@ -13,7 +13,7 @@ import { profile } from "tns-core-modules/profiling"; import { APP_ROOT_VIEW, DEVICE, getRootPage } from "./platform-providers"; import { isBlank } from "./lang-facade"; import { ViewUtil } from "./view-util"; -import { NgView, CommentNode } from "./element-registry"; +import { NgView, InvisibleNode } from "./element-registry"; import { rendererLog as traceLog } from "./trace"; // CONTENT_ATTR not exported from NativeScript_renderer - we need it for styles application. @@ -120,7 +120,7 @@ export class NativeScriptRenderer extends Renderer2 { } @profile - createComment(_value: any): CommentNode { + createComment(_value: any): InvisibleNode { traceLog(`NativeScriptRenderer.createComment ${_value}`); return this.viewUtil.createComment(); } @@ -132,7 +132,7 @@ export class NativeScriptRenderer extends Renderer2 { } @profile - createText(_value: string): CommentNode { + createText(_value: string): InvisibleNode { traceLog(`NativeScriptRenderer.createText ${_value}`); return this.viewUtil.createText(); } diff --git a/nativescript-angular/view-util.ts b/nativescript-angular/view-util.ts index 6349c203f..99b196197 100644 --- a/nativescript-angular/view-util.ts +++ b/nativescript-angular/view-util.ts @@ -5,8 +5,10 @@ import { ContentView } from "tns-core-modules/ui/content-view"; import { LayoutBase } from "tns-core-modules/ui/layouts/layout-base"; import { CommentNode, + InvisibleNode, NgElement, NgView, + TextNode, ViewExtensions, getViewClass, getViewMeta, @@ -52,9 +54,8 @@ export class ViewUtil { } public insertChild(parent: any, child: NgElement, atIndex: number = -1) { - if (child instanceof CommentNode) { + if (child instanceof InvisibleNode) { child.templateParent = parent; - return; } if (!parent || isDetachedElement(child)) { @@ -79,16 +80,11 @@ export class ViewUtil { parent.content = child; } else if (parent && parent._addChildFromBuilder) { parent._addChildFromBuilder(child.nodeName, child); - } else { - // throw new Error("Parent can"t contain children: " + parent.nodeName + ", " + parent); } } public removeChild(parent: any, child: NgElement) { - if (!parent || - child instanceof CommentNode || - isDetachedElement(child)) { - + if (!parent || isDetachedElement(child)) { return; } @@ -102,8 +98,6 @@ export class ViewUtil { } } else if (isView(parent)) { parent._removeView(child); - } else { - // throw new Error("Unknown parent type: " + parent); } } @@ -112,17 +106,15 @@ export class ViewUtil { return parent.getChildIndex(child); } else if (isContentView(parent)) { return child === parent.content ? 0 : -1; - } else { - // throw new Error("Parent can"t contain children: " + parent); } } - public createComment(): CommentNode { + public createComment(): InvisibleNode { return new CommentNode(); } - public createText(): CommentNode { - return new CommentNode(); + public createText(): InvisibleNode { + return new TextNode(); } public createView(name: string): NgView { From 9b40316e88a1a22a0ebf9cc6b60770f68bdddf25 Mon Sep 17 00:00:00 2001 From: sis0k0 Date: Tue, 11 Jul 2017 15:05:37 +0300 Subject: [PATCH 2/5] tests(renderer): update structural directives tests --- tests/app/tests/renderer-tests.ts | 112 +++++++++++++++++++++++------- 1 file changed, 88 insertions(+), 24 deletions(-) diff --git a/tests/app/tests/renderer-tests.ts b/tests/app/tests/renderer-tests.ts index fecbaf982..6efc463c8 100644 --- a/tests/app/tests/renderer-tests.ts +++ b/tests/app/tests/renderer-tests.ts @@ -94,6 +94,39 @@ export class NgIfLabel { } } +@Component({ + selector: "ng-if-two-elements", + template: ` + + + + + ` +}) +export class NgIfTwoElements { + public show: boolean = false; + constructor(public elementRef: ElementRef) { + } +} + +@Component({ + selector: "ng-if-multiple", + template: ` + + + + + + + + ` +}) +export class NgIfMultiple { + public show: boolean = false; + constructor(public elementRef: ElementRef) { + } +} + @Component({ selector: "ng-if-else", template: ` @@ -154,7 +187,9 @@ describe("Renderer E2E", () => { LayoutWithLabel, LabelCmp, LabelContainer, ProjectableCmp, ProjectionContainer, StyledLabelCmp, StyledLabelCmp2, - NgIfLabel, NgIfElseComponent, NgIfThenElseComponent, + NgIfLabel, NgIfThenElseComponent, NgIfMultiple, + NgIfTwoElements, NgIfMultiple, + NgIfElseComponent, NgIfThenElseComponent, NgForLabel, ]).then((app) => { testApp = app; @@ -256,7 +291,7 @@ describe("Renderer E2E", () => { it("ngIf hides component when false", () => { return testApp.loadComponent(NgIfLabel).then((componentRef) => { const componentRoot = componentRef.instance.elementRef.nativeElement; - assert.equal("(ProxyViewContainer)", dumpView(componentRoot)); + assert.equal("(ProxyViewContainer (CommentNode))", dumpView(componentRoot)); }); }); @@ -267,7 +302,43 @@ describe("Renderer E2E", () => { component.show = true; testApp.appRef.tick(); - assert.equal("(ProxyViewContainer (Label))", dumpView(componentRoot)); + assert.equal("(ProxyViewContainer (CommentNode), (Label))", dumpView(componentRoot)); + }); + }); + + it("ngIf shows elements in correct order when two are rendered", () => { + return testApp.loadComponent(NgIfTwoElements).then((componentRef) => { + const component = componentRef.instance; + const componentRoot = component.elementRef.nativeElement; + + component.show = true; + testApp.appRef.tick(); + assert.equal( + "(ProxyViewContainer (StackLayout (CommentNode), (Label), (Button)))", + dumpView(componentRoot)); + }); + }); + + + it("ngIf shows elements in correct order when multiple are rendered and there's *ngIf", () => { + return testApp.loadComponent(NgIfMultiple).then((componentRef) => { + const component = componentRef.instance; + const componentRoot = component.elementRef.nativeElement; + + component.show = true; + testApp.appRef.tick(); + assert.equal( + "(ProxyViewContainer " + + "(StackLayout " + + "(Label[text=1]), " + + "(Label[text=2]), " + + "(Label[text=3]), " + + "(CommentNode), " + // ng-reflect comment + "(Label[text=4]), " + // the content to be displayed and its anchor + "(Label[text=5])" + + ")" + + ")", + dumpView(componentRoot, true)); }); }); @@ -280,7 +351,8 @@ describe("Renderer E2E", () => { assert.equal( "(ProxyViewContainer " + "(StackLayout " + - "(Label[text=If])))", // the content to be displayed + "(CommentNode), " + // ng-reflect comment + "(Label[text=If]), (CommentNode)))", // the content to be displayed and its anchor dumpView(componentRoot, true)); }); @@ -296,7 +368,8 @@ describe("Renderer E2E", () => { assert.equal( "(ProxyViewContainer " + "(StackLayout " + - "(Label[text=Else])))", // the content to be displayed + "(CommentNode), " + // ng-reflect comment + "(Label[text=Else]), (CommentNode)))", // the content to be displayed and its anchor dumpView(componentRoot, true)); }); @@ -311,7 +384,10 @@ describe("Renderer E2E", () => { assert.equal( "(ProxyViewContainer " + "(StackLayout " + - "(Label[text=Then])))", // the content to be displayed + "(CommentNode), " + // ng-reflect comment + "(Label[text=Then]), (CommentNode), " + // the content to be displayed and its anchor + "(CommentNode)))", // the anchor for the else template + dumpView(componentRoot, true)); }); }); @@ -327,7 +403,9 @@ describe("Renderer E2E", () => { assert.equal( "(ProxyViewContainer " + "(StackLayout " + - "(Label[text=Else])))", // the content to be displayed + "(CommentNode), " + // the content to be displayed + "(Label[text=Else]), (CommentNode), " + // the content to be displayed + "(CommentNode)))", // the content to be displayed dumpView(componentRoot, true)); }); @@ -337,7 +415,7 @@ describe("Renderer E2E", () => { return testApp.loadComponent(NgForLabel).then((componentRef) => { const componentRoot = componentRef.instance.elementRef.nativeElement; assert.equal( - "(ProxyViewContainer (Label[text=one]), (Label[text=two]), (Label[text=three]))", + "(ProxyViewContainer (CommentNode), (Label[text=one]), (Label[text=two]), (Label[text=three]))", dumpView(componentRoot, true)); }); }); @@ -351,7 +429,7 @@ describe("Renderer E2E", () => { testApp.appRef.tick(); assert.equal( - "(ProxyViewContainer (Label[text=one]), (Label[text=three]))", + "(ProxyViewContainer (CommentNode), (Label[text=one]), (Label[text=three]))", dumpView(componentRoot, true)); }); }); @@ -365,7 +443,7 @@ describe("Renderer E2E", () => { testApp.appRef.tick(); assert.equal( - "(ProxyViewContainer " + + "(ProxyViewContainer (CommentNode), " + "(Label[text=one]), (Label[text=new]), (Label[text=two]), (Label[text=three]))", dumpView(componentRoot, true)); }); @@ -464,19 +542,5 @@ describe("Renderer attach/detach", () => { assert.equal(parent.getChildrenCount(), 0); assert.isUndefined(button.parent); }); - - it("attaching and detaching comment anchor to content view does not affect its content", () => { - const parent = renderer.createElement("ContentView"); - const button =