From b00708ef30196193b09dbb510e4d656bd0afdb70 Mon Sep 17 00:00:00 2001 From: Misko Hevery Date: Fri, 1 Nov 2019 21:08:54 -0700 Subject: [PATCH] fix(ivy): handle cases of inserting `LView` into `LContainer` Under some circumstances inserting `LView` into `LContainer` would get inserted in a wrong DOM location resulting in incorrect DOM node order. These two use cases were corrected: - Inserting in front of empty `LView` ``` ` ``` - Inserting in front of nested `Lview` ``` Nested ` ``` --- .../core/src/render3/node_manipulation.ts | 63 ++++++--- .../src/render3/view_engine_compatibility.ts | 6 +- .../test/acceptance/view_insertion_spec.ts | 122 ++++++++++++++++-- 3 files changed, 162 insertions(+), 29 deletions(-) diff --git a/packages/core/src/render3/node_manipulation.ts b/packages/core/src/render3/node_manipulation.ts index 5987eb4dd6f61..ee3567e8c1822 100644 --- a/packages/core/src/render3/node_manipulation.ts +++ b/packages/core/src/render3/node_manipulation.ts @@ -663,27 +663,54 @@ export function appendChild(childEl: RNode | RNode[], childTNode: TNode, current } } -export function getBeforeNodeForView(viewIndexInContainer: number, lContainer: LContainer): RNode| - null { - const nextViewIndex = CONTAINER_HEADER_OFFSET + viewIndexInContainer + 1; - if (nextViewIndex < lContainer.length) { - const lView = lContainer[nextViewIndex] as LView; - ngDevMode && assertDefined(lView[T_HOST], 'Missing Host TNode'); - let tViewNodeChild = (lView[T_HOST] as TViewNode).child; - if (tViewNodeChild !== null) { - if (tViewNodeChild.type === TNodeType.ElementContainer || - tViewNodeChild.type === TNodeType.IcuContainer) { - let currentChild = tViewNodeChild.child; - while (currentChild && (currentChild.type === TNodeType.ElementContainer || - currentChild.type === TNodeType.IcuContainer)) { - currentChild = currentChild.child; - } - tViewNodeChild = currentChild || tViewNodeChild; +/** + * Retrieves `RNode` from `LContainer` to be used for DOM operations which is need as `before` node. + * + * When inserting `LView`s into DOM we often need to know a `before` node to insert into the DOM + * tree into. This function is useful for returning the `RNode` since it retrieves the first `RNode` + * of the `LView` at a given `index` in the `LContainer`. + * + * This method takes into account the fact that the `LView`'s first `TNode` may be another `LView`, + * `IcuContainer`, or `ElementContainer` which requires descending into those locations in order + * to retrieve the `RNode`. + * + * @param lViewIndex Index into the `LContainer` which points to the `LView` of interest + * @param lContainer `LContainer` to look into. + * @returns `RNode` of the `LView` at `index` in the `LContainer` or `LContainer`'s anchor node if + * no `LView` is found at `index`. + */ +export function getFirstRNodeFromLViewInLContainer( + lContainer: LContainer, lViewIndex: number): RNode { + ngDevMode && assertLContainer(lContainer); + // We read the `LView` and look for first `TNode`. However, if we have an empty `TView` (such as + // `) we need to go to the next `LView` to and repeat the process. If + // we run of out of `LView`s to look at we return the `LContainer` anchor. + for (let i = CONTAINER_HEADER_OFFSET + lViewIndex; i < lContainer.length; i++) { + const lView = lContainer[i]; + let tNode = lView[TVIEW].firstChild; + if (tNode !== null) { + let tNodeType = tNode.type; + if (tNodeType === TNodeType.Container) { + // In this case the first `TNode` is another `LContainer` this means we now need to + // recurse + // into it the `LView` and retrieve its `RNode`. + const childLContainer = lView[tNode.index] as LContainer; + ngDevMode && assertLContainer(childLContainer); + return getFirstRNodeFromLViewInLContainer(childLContainer, 0); + } else { + // If we have `ElementContainer` or `IcuContainer` than we need to descend into them and + // find the actual first `Element` + while (tNodeType === TNodeType.ElementContainer || tNodeType === TNodeType.IcuContainer) { + tNode = tNode.child !; + ngDevMode && assertDefined(tNode, 'ElementContainer and IcuContainer must have children'); + tNodeType = tNode.type; } - return getNativeByTNodeOrNull(tViewNodeChild, lView); + return getNativeByTNode(tNode, lView); } } - + } + // If we could not find any `RNode` in any of the `LView`s, than just use the `LContainer`'s + // anchor. return lContainer[NATIVE]; } diff --git a/packages/core/src/render3/view_engine_compatibility.ts b/packages/core/src/render3/view_engine_compatibility.ts index 65aefe14e6f19..caf870f621b08 100644 --- a/packages/core/src/render3/view_engine_compatibility.ts +++ b/packages/core/src/render3/view_engine_compatibility.ts @@ -25,9 +25,9 @@ import {ACTIVE_INDEX, CONTAINER_HEADER_OFFSET, LContainer, VIEW_REFS} from './in import {TContainerNode, TElementContainerNode, TElementNode, TNode, TNodeType, TViewNode} from './interfaces/node'; import {RComment, RElement, isProceduralRenderer} from './interfaces/renderer'; import {isComponentHost, isLContainer, isLView, isRootView} from './interfaces/type_checks'; -import {CONTEXT, DECLARATION_LCONTAINER, LView, LViewFlags, QUERIES, RENDERER, TView, T_HOST} from './interfaces/view'; +import {DECLARATION_LCONTAINER, LView, LViewFlags, QUERIES, RENDERER, TView, T_HOST} from './interfaces/view'; import {assertNodeOfPossibleTypes} from './node_assert'; -import {addRemoveViewFromContainer, appendChild, detachView, getBeforeNodeForView, insertView, nativeInsertBefore, nativeNextSibling, nativeParentNode, removeView} from './node_manipulation'; +import {addRemoveViewFromContainer, appendChild, detachView, getFirstRNodeFromLViewInLContainer, insertView, nativeInsertBefore, nativeNextSibling, nativeParentNode, removeView} from './node_manipulation'; import {getParentInjectorTNode} from './node_util'; import {getLView, getPreviousOrParentTNode} from './state'; import {getParentInjectorView, hasParentInjector} from './util/injector_utils'; @@ -254,9 +254,9 @@ export function createContainerRef( return this.move(viewRef, adjustedIdx); } + const beforeNode = getFirstRNodeFromLViewInLContainer(this._lContainer, adjustedIdx); insertView(lView, this._lContainer, adjustedIdx); - const beforeNode = getBeforeNodeForView(adjustedIdx, this._lContainer); addRemoveViewFromContainer(lView, true, beforeNode); (viewRef as ViewRef).attachToViewContainerRef(this); diff --git a/packages/core/test/acceptance/view_insertion_spec.ts b/packages/core/test/acceptance/view_insertion_spec.ts index b02373d27bed2..bccdce9f1783d 100644 --- a/packages/core/test/acceptance/view_insertion_spec.ts +++ b/packages/core/test/acceptance/view_insertion_spec.ts @@ -7,9 +7,10 @@ */ import {CommonModule} from '@angular/common'; -import {ChangeDetectorRef, Component, EmbeddedViewRef, TemplateRef, ViewChild, ViewContainerRef} from '@angular/core'; +import {ChangeDetectorRef, Component, Directive, EmbeddedViewRef, TemplateRef, ViewChild, ViewContainerRef} from '@angular/core'; import {TestBed} from '@angular/core/testing'; import {By} from '@angular/platform-browser'; +import {ivyEnabled} from '@angular/private/testing'; describe('view insertion', () => { describe('of a simple template', () => { @@ -18,7 +19,7 @@ describe('view insertion', () => { @Component({ selector: 'increment-comp', - template: `created{{counter}}`, + template: `Created{{counter}}`, }) class IncrementComp { counter = _counter++; @@ -27,6 +28,8 @@ describe('view insertion', () => { @Component({ template: ` + Nested +
` }) @@ -37,25 +40,49 @@ describe('view insertion', () => { @ViewChild('simple', {read: TemplateRef, static: true}) simple: TemplateRef = null !; + // This case demonstrates when `LView` contains another `LContainer` and requires + // recursion when we retrieve the insert before `RNode` + @ViewChild('nested', {read: TemplateRef, static: true}) + nested: TemplateRef = null !; + + // This case demonstrates when `LView` contains no `TNode` and we have to go to the next + // `LView` in order to get a real `RNode` + @ViewChild('empty', {read: TemplateRef, static: true}) + empty: TemplateRef = null !; + view0: EmbeddedViewRef = null !; view1: EmbeddedViewRef = null !; view2: EmbeddedViewRef = null !; view3: EmbeddedViewRef = null !; + view4: EmbeddedViewRef = null !; constructor(public changeDetector: ChangeDetectorRef) {} ngAfterViewInit() { - // insert at the front - this.view1 = this.container.createEmbeddedView(this.simple); // "created0" + // insert at the front. Because this is empty and has no `RNode` any insertions in + // front of it need to fall through to the next `LView` to get insertion node. + this.view1 = this.container.createEmbeddedView(this.empty); + expect(fixture.nativeElement.textContent).toBe(''); + + this.view2 = this.container.createEmbeddedView(this.nested); // "nested" + this.view2.detectChanges(); + expect(fixture.nativeElement.textContent).toBe('Nested'); // insert at the front again - this.view0 = this.container.createEmbeddedView(this.simple, {}, 0); // "created1" + debugger; + this.view0 = this.container.createEmbeddedView(this.simple, {}, 0); // "Created0" + this.view0.detectChanges(); + expect(fixture.nativeElement.textContent).toBe('Created0Nested'); // insert at the end - this.view3 = this.container.createEmbeddedView(this.simple); // "created2" + this.view4 = this.container.createEmbeddedView(this.simple); // "Created1" + this.view4.detectChanges(); + expect(fixture.nativeElement.textContent).toBe('Created0NestedCreated1'); // insert in the middle - this.view2 = this.container.createEmbeddedView(this.simple, {}, 2); // "created3" + this.view3 = this.container.createEmbeddedView(this.simple, {}, 3); // "Created2" + this.view3.detectChanges(); + expect(fixture.nativeElement.textContent).toBe('Created0NestedCreated2Created1'); // We need to run change detection here to avoid // ExpressionChangedAfterItHasBeenCheckedError because of the value updating in @@ -75,8 +102,9 @@ describe('view insertion', () => { expect(app.container.indexOf(app.view1)).toBe(1); expect(app.container.indexOf(app.view2)).toBe(2); expect(app.container.indexOf(app.view3)).toBe(3); + expect(app.container.indexOf(app.view4)).toBe(4); // The text in each component differs based on *when* it was created. - expect(fixture.nativeElement.textContent).toBe('created1created0created3created2'); + expect(fixture.nativeElement.textContent).toBe('Created0NestedCreated2Created1'); }); }); @@ -249,4 +277,82 @@ describe('view insertion', () => { expect(fixture.debugElement.queryAll(By.css('div.dynamic')).length).toBe(4); }); }); + + describe('regression', () => { + it('FW-1559: should support inserting in front of nested `LContainers', () => { + @Component({ + selector: 'repeater', + template: ` + + + + + + {{item}} + ` + }) + class Repeater { + rendered: string[] = []; + + trackByFn(index: number, item: string) { return item; } + } + + const fixture = + TestBed.configureTestingModule({declarations: [Repeater]}).createComponent(Repeater); + fixture.componentInstance.rendered = ['b']; + fixture.detectChanges(); + + fixture.componentInstance.rendered = ['a', 'b']; + (global as any).break = true; + fixture.detectChanges(); + expect(fixture.nativeElement.textContent).toEqual('ab'); + }); + + it('FW-1559: should properly render content if vcr.createEmbeddedView is called', () => { + @Directive({selector: '[dir]'}) + class MyDir { + viewRef: any; + + constructor(public template: TemplateRef<{}>, public viewContainerRef: ViewContainerRef) {} + + show() { + this.viewRef = this.viewContainerRef.createEmbeddedView(this.template); + this.viewRef.detectChanges(); + } + } + + @Component({ + selector: 'edit-form', + template: ` + +
Text
+ + `, + }) + class MyComp { + myFlag: boolean = true; + @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; + dirRef.show(); + + // In VE: 3 nodes - 2 comment nodes + 1 div + // In Ivy: 2 comment node (anchor) + const rootNodes = dirRef.viewRef.rootNodes; + expect(rootNodes.length).toBe(ivyEnabled ? 2 : 3); + expect((rootNodes[0] as Comment).textContent).toMatch(/"ng-reflect-ng-if": "true"/); + expect((rootNodes[1] as HTMLElement).outerHTML).toEqual('
Text
'); + }); + + }); });