From 5a52990b91000ce70955cfc444b886e840dc2bb1 Mon Sep 17 00:00:00 2001 From: Pawel Kozlowski Date: Fri, 29 Nov 2019 21:59:57 +0100 Subject: [PATCH] fix(ivy): allow insertion of views attached to a different container (#34156) Fixes #34152 PR Close #34156 --- .../src/render3/view_engine_compatibility.ts | 51 ++++++++++++------- .../acceptance/view_container_ref_spec.ts | 41 +++++++++++++++ 2 files changed, 74 insertions(+), 18 deletions(-) diff --git a/packages/core/src/render3/view_engine_compatibility.ts b/packages/core/src/render3/view_engine_compatibility.ts index fda2a4b0cf..2424e0a5c6 100644 --- a/packages/core/src/render3/view_engine_compatibility.ts +++ b/packages/core/src/render3/view_engine_compatibility.ts @@ -16,16 +16,16 @@ import {ViewContainerRef as ViewEngine_ViewContainerRef} from '../linker/view_co import {EmbeddedViewRef as viewEngine_EmbeddedViewRef, ViewRef as viewEngine_ViewRef} from '../linker/view_ref'; import {Renderer2} from '../render/api'; import {addToArray, removeFromArray} from '../util/array_utils'; -import {assertDefined, assertGreaterThan, assertLessThan} from '../util/assert'; +import {assertDefined, assertEqual, assertGreaterThan, assertLessThan} from '../util/assert'; import {assertLContainer} from './assert'; import {NodeInjector, getParentInjectorLocation} from './di'; import {addToViewTree, createLContainer, createLView, renderView} from './instructions/shared'; import {ActiveIndexFlag, CONTAINER_HEADER_OFFSET, LContainer, VIEW_REFS} from './interfaces/container'; -import {TContainerNode, TElementContainerNode, TElementNode, TNode, TNodeType, TViewNode} from './interfaces/node'; +import {TContainerNode, TDirectiveHostNode, 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 {DECLARATION_COMPONENT_VIEW, DECLARATION_LCONTAINER, LView, LViewFlags, QUERIES, RENDERER, TView, T_HOST} from './interfaces/view'; +import {DECLARATION_COMPONENT_VIEW, DECLARATION_LCONTAINER, LView, LViewFlags, PARENT, 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 {getParentInjectorTNode} from './node_util'; @@ -240,18 +240,40 @@ export function createContainerRef( } insert(viewRef: viewEngine_ViewRef, index?: number): viewEngine_ViewRef { + const lView = (viewRef as ViewRef)._lView !; + if (viewRef.destroyed) { throw new Error('Cannot insert a destroyed View in a ViewContainer!'); } + this.allocateContainerIfNeeded(); - const lView = (viewRef as ViewRef)._lView !; if (viewAttachedToContainer(lView)) { - // If view is already attached, fall back to move() so we clean up - // references appropriately. - // Note that we "shift" -1 because the move will involve inserting - // one view but also removing one view. - return this.move(viewRef, this._adjustIndex(index, -1)); + // If view is already attached, detach it first so we clean up references appropriately. + + const prevIdx = this.indexOf(viewRef); + + // A view might be attached either to this or a different container. The `prevIdx` for + // those cases will be: + // equal to -1 for views attached to this ViewContainerRef + // >= 0 for views attached to a different ViewContainerRef + if (prevIdx !== -1) { + this.detach(prevIdx); + } else { + const prevLContainer = lView[PARENT] as LContainer; + ngDevMode && assertEqual( + isLContainer(prevLContainer), true, + 'An attached view should have its PARENT point to a container.'); + + + // We need to re-create a R3ViewContainerRef instance since those are not stored on + // LView (nor anywhere else). + const prevVCRef = new R3ViewContainerRef( + prevLContainer, prevLContainer[T_HOST] as TDirectiveHostNode, + prevLContainer[PARENT]); + + prevVCRef.detach(prevVCRef.indexOf(viewRef)); + } } const adjustedIdx = this._adjustIndex(index); @@ -270,14 +292,7 @@ export function createContainerRef( if (viewRef.destroyed) { throw new Error('Cannot move a destroyed View in a ViewContainer!'); } - const index = this.indexOf(viewRef); - if (index === -1) { - this.insert(viewRef, newIndex); - } else if (index !== newIndex) { - this.detach(index); - this.insert(viewRef, newIndex); - } - return viewRef; + return this.insert(viewRef, newIndex); } indexOf(viewRef: viewEngine_ViewRef): number { @@ -307,7 +322,7 @@ export function createContainerRef( return this.length + shift; } if (ngDevMode) { - assertGreaterThan(index, -1, 'index must be positive'); + assertGreaterThan(index, -1, `ViewRef index must be positive, got ${index}`); // +1 because it's legal to insert at the end. assertLessThan(index, this.length + 1 + shift, 'index'); } diff --git a/packages/core/test/acceptance/view_container_ref_spec.ts b/packages/core/test/acceptance/view_container_ref_spec.ts index ade2e74f31..192b126730 100644 --- a/packages/core/test/acceptance/view_container_ref_spec.ts +++ b/packages/core/test/acceptance/view_container_ref_spec.ts @@ -220,6 +220,47 @@ describe('ViewContainerRef', () => { viewContainerRef.insert(ref0); expect(fixture.nativeElement.textContent).toEqual('0'); }); + + it('should insert a view already inserted into another container', () => { + + @Component({ + selector: 'test-cmpt', + template: ` + content + before||middle||after + ` + }) + class TestComponent { + @ViewChild('t', {static: true}) t !: TemplateRef<{}>; + @ViewChild('c1', {static: true, read: ViewContainerRef}) c1 !: ViewContainerRef; + @ViewChild('c2', {static: true, read: ViewContainerRef}) c2 !: ViewContainerRef; + } + + TestBed.configureTestingModule({declarations: [TestComponent]}); + const fixture = TestBed.createComponent(TestComponent); + fixture.detectChanges(); + const cmpt = fixture.componentInstance; + const native = fixture.nativeElement; + + expect(native.textContent.trim()).toEqual('before||middle||after'); + + // create and insert an embedded view into the c1 container + const viewRef = cmpt.c1.createEmbeddedView(cmpt.t, {}); + expect(native.textContent.trim()).toEqual('before|content|middle||after'); + expect(cmpt.c1.indexOf(viewRef)).toBe(0); + expect(cmpt.c2.indexOf(viewRef)).toBe(-1); + + // move the existing embedded view into the c2 container + cmpt.c2.insert(viewRef); + expect(native.textContent.trim()).toEqual('before||middle|content|after'); + + // VE has a bug where a view moved between containers is not correctly detached from the + // previous container. Check https://github.com/angular/angular/issues/20824 for more details. + if (ivyEnabled) { + expect(cmpt.c1.indexOf(viewRef)).toBe(-1); + } + expect(cmpt.c2.indexOf(viewRef)).toBe(0); + }); }); describe('move', () => {