From 66b72bfa586a795be924805f6c1e67b988230d6c Mon Sep 17 00:00:00 2001 From: Marc Laval Date: Wed, 20 Mar 2019 15:26:48 +0100 Subject: [PATCH] fix(ivy): ViewContainerRef.destroy should properly clean the DOM (#29414) PR Close #29414 --- .../common/test/directives/ng_switch_spec.ts | 50 +++++- .../src/render3/instructions/instructions.ts | 8 +- .../core/src/render3/interfaces/container.ts | 25 ++- .../core/src/render3/node_manipulation.ts | 31 +++- .../src/render3/view_engine_compatibility.ts | 2 +- .../acceptance/view_container_ref_spec.ts | 166 +++++++++++++++++- packages/core/test/render3/view_utils_spec.ts | 5 +- packages/private/testing/src/goog_get_msg.ts | 3 +- 8 files changed, 264 insertions(+), 26 deletions(-) diff --git a/packages/common/test/directives/ng_switch_spec.ts b/packages/common/test/directives/ng_switch_spec.ts index 9d78c6f6d9..f7042859e5 100644 --- a/packages/common/test/directives/ng_switch_spec.ts +++ b/packages/common/test/directives/ng_switch_spec.ts @@ -7,7 +7,7 @@ */ import {CommonModule} from '@angular/common'; -import {Attribute, Component, Directive} from '@angular/core'; +import {Attribute, Component, Directive, TemplateRef, ViewChild} from '@angular/core'; import {ComponentFixture, TestBed} from '@angular/core/testing'; import {expect} from '@angular/platform-browser/testing/src/matchers'; @@ -26,7 +26,7 @@ import {expect} from '@angular/platform-browser/testing/src/matchers'; beforeEach(() => { TestBed.configureTestingModule({ - declarations: [TestComponent], + declarations: [TestComponent, ComplexComponent], imports: [CommonModule], }); }); @@ -171,6 +171,20 @@ import {expect} from '@angular/platform-browser/testing/src/matchers'; getComponent().switchValue = 'b'; detectChangesAndExpectText('when b1;when b2;'); }); + + it('should support nested NgSwitch on ng-container with ngTemplateOutlet', () => { + fixture = TestBed.createComponent(ComplexComponent); + detectChangesAndExpectText('Foo'); + + fixture.componentInstance.state = 'case2'; + detectChangesAndExpectText('Bar'); + + fixture.componentInstance.state = 'notACase'; + detectChangesAndExpectText('Default'); + + fixture.componentInstance.state = 'case1'; + detectChangesAndExpectText('Foo'); + }); }); }); } @@ -182,6 +196,38 @@ class TestComponent { when2: any = null; } +@Component({ + selector: 'complex-cmp', + template: ` +
+ + + Should never render + + + + Should never render + + + + Default + +
+ + + Foo + + + Bar + +` +}) +class ComplexComponent { + @ViewChild('foo') foo !: TemplateRef; + @ViewChild('bar') bar !: TemplateRef; + state: string = 'case1'; +} + function createTestComponent(template: string): ComponentFixture { return TestBed.overrideComponent(TestComponent, {set: {template: template}}) .createComponent(TestComponent); diff --git a/packages/core/src/render3/instructions/instructions.ts b/packages/core/src/render3/instructions/instructions.ts index e3c3e78934..a5e32252b6 100644 --- a/packages/core/src/render3/instructions/instructions.ts +++ b/packages/core/src/render3/instructions/instructions.ts @@ -1952,7 +1952,7 @@ function generateInitialInputs( */ export function createLContainer( hostNative: RElement | RComment | StylingContext | LView, currentView: LView, native: RComment, - isForViewContainerRef?: boolean): LContainer { + tNode: TNode, isForViewContainerRef?: boolean): LContainer { ngDevMode && assertDomNode(native); ngDevMode && assertLView(currentView); const lContainer: LContainer = [ @@ -1962,8 +1962,9 @@ export function createLContainer( currentView, // parent null, // next null, // queries - [], // views + tNode, // t_host native, // native + [], // views ]; ngDevMode && attachLContainerDebug(lContainer); return lContainer; @@ -2037,7 +2038,8 @@ function containerInternal( const comment = lView[RENDERER].createComment(ngDevMode ? 'container' : ''); ngDevMode && ngDevMode.rendererCreateComment++; const tNode = createNodeAtIndex(index, TNodeType.Container, comment, tagName, attrs); - const lContainer = lView[adjustedIndex] = createLContainer(lView[adjustedIndex], lView, comment); + const lContainer = lView[adjustedIndex] = + createLContainer(lView[adjustedIndex], lView, comment, tNode); appendChild(comment, tNode, lView); diff --git a/packages/core/src/render3/interfaces/container.ts b/packages/core/src/render3/interfaces/container.ts index c562cab613..3d1a61a967 100644 --- a/packages/core/src/render3/interfaces/container.ts +++ b/packages/core/src/render3/interfaces/container.ts @@ -6,10 +6,12 @@ * found in the LICENSE file at https://angular.io/license */ +import {TNode} from './node'; import {LQueries} from './query'; import {RComment, RElement} from './renderer'; import {StylingContext} from './styling'; -import {HOST, LView, NEXT, PARENT, QUERIES} from './view'; +import {HOST, LView, NEXT, PARENT, QUERIES, T_HOST} from './view'; + /** * Special location which allows easy identification of type. If we have an array which was @@ -23,10 +25,10 @@ export const TYPE = 1; * Uglify will inline these when minifying so there shouldn't be a cost. */ export const ACTIVE_INDEX = 2; -// PARENT, NEXT, and QUERIES are indices 3, 4, and 5. +// PARENT, NEXT, QUERIES and T_HOST are indices 3, 4, 5 and 6. // As we already have these constants in LView, we don't need to re-create them. -export const VIEWS = 6; export const NATIVE = 7; +export const VIEWS = 8; /** * The state associated with a container. @@ -83,17 +85,22 @@ export interface LContainer extends Array { // `[QUERIES]` in it which are not needed for `LContainer` (only needed for Template) /** - * A list of the container's currently active child views. Views will be inserted - * here as they are added and spliced from here when they are removed. We need - * to keep a record of current views so we know which views are already in the DOM - * (and don't need to be re-added) and so we can remove views from the DOM when they - * are no longer required. + * Pointer to the `TNode` which represents the host of the container. */ - [VIEWS]: LView[]; + [T_HOST]: TNode; /** The comment element that serves as an anchor for this LContainer. */ readonly[NATIVE]: RComment; // TODO(misko): remove as this value can be gotten by unwrapping `[HOST]` + + /** +*A list of the container's currently active child views. Views will be inserted +*here as they are added and spliced from here when they are removed. We need +*to keep a record of current views so we know which views are already in the DOM +*(and don't need to be re-added) and so we can remove views from the DOM when they +*are no longer required. +*/ + [VIEWS]: LView[]; } // Note: This hack is necessary so we don't erroneously get a circular dependency diff --git a/packages/core/src/render3/node_manipulation.ts b/packages/core/src/render3/node_manipulation.ts index 717fa06d73..14ad1e7541 100644 --- a/packages/core/src/render3/node_manipulation.ts +++ b/packages/core/src/render3/node_manipulation.ts @@ -91,7 +91,7 @@ function walkTNodeTree( let tNode: TNode|null = rootTNode.child as TNode; while (tNode) { let nextTNode: TNode|null = null; - if (tNode.type === TNodeType.Element) { + if (tNode.type === TNodeType.Element || tNode.type === TNodeType.ElementContainer) { executeNodeAction( action, renderer, renderParent, getNativeByTNode(tNode, currentView), tNode, beforeNode); const nodeOrContainer = currentView[tNode.index]; @@ -99,6 +99,14 @@ function walkTNodeTree( // This element has an LContainer, and its comment needs to be handled executeNodeAction( action, renderer, renderParent, nodeOrContainer[NATIVE], tNode, beforeNode); + if (nodeOrContainer[VIEWS].length) { + currentView = nodeOrContainer[VIEWS][0]; + nextTNode = currentView[TVIEW].node; + + // When the walker enters a container, then the beforeNode has to become the local native + // comment node. + beforeNode = nodeOrContainer[NATIVE]; + } } } else if (tNode.type === TNodeType.Container) { const lContainer = currentView ![tNode.index] as LContainer; @@ -133,9 +141,8 @@ function walkTNodeTree( nextTNode = currentView[TVIEW].data[head.index] as TNode; } } - } else { - // Otherwise, this is a View or an ElementContainer + // Otherwise, this is a View nextTNode = tNode.child; } @@ -145,7 +152,14 @@ function walkTNodeTree( currentView = projectionNodeStack[projectionNodeIndex--] as LView; tNode = projectionNodeStack[projectionNodeIndex--] as TNode; } - nextTNode = (tNode.flags & TNodeFlags.isProjected) ? tNode.projectionNext : tNode.next; + + if (tNode.flags & TNodeFlags.isProjected) { + nextTNode = tNode.projectionNext; + } else if (tNode.type === TNodeType.ElementContainer) { + nextTNode = tNode.child || tNode.next; + } else { + nextTNode = tNode.next; + } /** * Find the next node in the TNode tree, taking into account the place where a node is @@ -172,19 +186,26 @@ function walkTNodeTree( * chain until: * - we find an lView with a next pointer * - or find a tNode with a parent that has a next pointer + * - or find a lContainer * - or reach root TNode (in which case we exit, since we traversed all nodes) */ while (!currentView[NEXT] && currentView[PARENT] && !(tNode.parent && tNode.parent.next)) { if (tNode === rootTNode) return; currentView = currentView[PARENT] as LView; + if (isLContainer(currentView)) { + tNode = currentView[T_HOST] !; + currentView = currentView[PARENT]; + beforeNode = currentView[tNode.index][NATIVE]; + break; + } tNode = currentView[T_HOST] !; } if (currentView[NEXT]) { currentView = currentView[NEXT] as LView; nextTNode = currentView[T_HOST]; } else { - nextTNode = tNode.next; + nextTNode = tNode.type === TNodeType.ElementContainer && tNode.child || tNode.next; } } else { nextTNode = tNode.next; diff --git a/packages/core/src/render3/view_engine_compatibility.ts b/packages/core/src/render3/view_engine_compatibility.ts index 7bba5c33d7..67c544a58c 100644 --- a/packages/core/src/render3/view_engine_compatibility.ts +++ b/packages/core/src/render3/view_engine_compatibility.ts @@ -321,7 +321,7 @@ export function createContainerRef( } hostView[hostTNode.index] = lContainer = - createLContainer(slotValue, hostView, commentNode, true); + createLContainer(slotValue, hostView, commentNode, hostTNode, true); addToViewTree(hostView, lContainer); } diff --git a/packages/core/test/acceptance/view_container_ref_spec.ts b/packages/core/test/acceptance/view_container_ref_spec.ts index cc5b963612..1e921e55fc 100644 --- a/packages/core/test/acceptance/view_container_ref_spec.ts +++ b/packages/core/test/acceptance/view_container_ref_spec.ts @@ -6,15 +6,25 @@ * found in the LICENSE file at https://angular.io/license */ -import {Component, QueryList, TemplateRef, ViewChild, ViewChildren, ViewContainerRef} from '@angular/core'; +import {Component, Directive, NO_ERRORS_SCHEMA, QueryList, TemplateRef, ViewChild, ViewChildren, ViewContainerRef} from '@angular/core'; import {TestBed} from '@angular/core/testing'; import {expect} from '@angular/platform-browser/testing/src/matchers'; -import {ivyEnabled, onlyInIvy} from '@angular/private/testing'; +import {ivyEnabled, onlyInIvy, polyfillGoogGetMsg} from '@angular/private/testing'; describe('ViewContainerRef', () => { + const TRANSLATIONS: any = { + 'Bar': 'o', + '{$startTagBefore}{$closeTagBefore}{$startTagDiv}{$startTagInside}{$closeTagInside}{$closeTagDiv}{$startTagAfter}{$closeTagAfter}': + 'F{$startTagDiv}{$closeTagDiv}o', + '{$startTagBefore}{$closeTagBefore}{$startTagDiv}{$startTagIn}{$closeTagIn}{$closeTagDiv}{$startTagAfter}{$closeTagAfter}': + '{$startTagDiv}{$closeTagDiv}{$startTagBefore}{$closeTagBefore}' + }; + beforeEach(() => { - TestBed.configureTestingModule({declarations: [ViewContainerRefComp, ViewContainerRefApp]}); + polyfillGoogGetMsg(TRANSLATIONS); + TestBed.configureTestingModule( + {declarations: [StructDir, ViewContainerRefComp, ViewContainerRefApp, DestroyCasesComp]}); }); describe('insert', () => { @@ -80,6 +90,142 @@ describe('ViewContainerRef', () => { }); }); + describe('destroy should clean the DOM in all cases:', () => { + function executeTest(template: string) { + TestBed.overrideTemplate(DestroyCasesComp, template).configureTestingModule({ + schemas: [NO_ERRORS_SCHEMA] + }); + + const fixture = TestBed.createComponent(DestroyCasesComp); + fixture.detectChanges(); + const initial = fixture.nativeElement.innerHTML; + + const structDirs = fixture.componentInstance.structDirs.toArray(); + + structDirs.forEach(structDir => structDir.create()); + fixture.detectChanges(); + expect(fixture.nativeElement).toHaveText('Foo'); + + structDirs.forEach(structDir => structDir.destroy()); + fixture.detectChanges(); + expect(fixture.nativeElement.innerHTML).toEqual(initial); + } + + it('when nested ng-container', () => { + executeTest(` + + + + + + Foo + + + + + `); + }); + + it('when ViewContainerRef is on a ng-container', () => { + executeTest(` + + Foo + + + + + + + + + `); + }); + + it('when ViewContainerRef is on an element', () => { + executeTest(` + + Foo + + + + +
+ +
+ + `); + }); + + it('when ViewContainerRef is on a ng-template', () => { + executeTest(` + + Foo + + + + + + + `); + }); + + it('when ViewContainerRef is on an element inside a ng-container', () => { + executeTest(` + + Foo + + + + + + +
+ +
+ + + + `); + }); + + onlyInIvy('Ivy i18n logic') + .it('when ViewContainerRef is on an element inside a ng-container with i18n', () => { + executeTest(` + + Bar + + + + + + +
+ +
+ + + + `); + }); + + onlyInIvy('Ivy i18n logic') + .it('when ViewContainerRef is on an element, and i18n is on the parent ViewContainerRef', + () => { + executeTest(` + + Foo + + + + +
+ +
+ + `); + }); + }); + }); @Component({ @@ -105,3 +251,17 @@ class ViewContainerRefComp { class ViewContainerRefApp { @ViewChild(ViewContainerRefComp) vcrComp !: ViewContainerRefComp; } + +@Directive({selector: '[structDir]'}) +export class StructDir { + constructor(private vcref: ViewContainerRef, private tplRef: TemplateRef) {} + + create() { this.vcref.createEmbeddedView(this.tplRef); } + + destroy() { this.vcref.clear(); } +} + +@Component({selector: 'destroy-cases', template: ` `}) +class DestroyCasesComp { + @ViewChildren(StructDir) structDirs !: QueryList; +} diff --git a/packages/core/test/render3/view_utils_spec.ts b/packages/core/test/render3/view_utils_spec.ts index e8ea579dcf..1603faad8d 100644 --- a/packages/core/test/render3/view_utils_spec.ts +++ b/packages/core/test/render3/view_utils_spec.ts @@ -6,7 +6,7 @@ * found in the LICENSE file at https://angular.io/license */ -import {createLContainer, createLView, createTView} from '@angular/core/src/render3/instructions/all'; +import {createLContainer, createLView, createTNode, createTView} from '@angular/core/src/render3/instructions/all'; import {createEmptyStylingContext} from '@angular/core/src/render3/styling/util'; import {isLContainer, isLView, isStylingContext, unwrapLContainer, unwrapLView, unwrapRNode, unwrapStylingContext} from '@angular/core/src/render3/util/view_utils'; @@ -15,7 +15,8 @@ describe('view_utils', () => { const div = document.createElement('div'); const tView = createTView(0, null, 0, 0, null, null, null, null); const lView = createLView(null, tView, {}, 0, div, null, {} as any, {} as any, null, null); - const lContainer = createLContainer(lView, lView, div, true); + const tNode = createTNode(null, 3, 0, 'div', []); + const lContainer = createLContainer(lView, lView, div, tNode, true); const styleContext = createEmptyStylingContext(lContainer, null, null, null); expect(unwrapRNode(styleContext)).toBe(div); diff --git a/packages/private/testing/src/goog_get_msg.ts b/packages/private/testing/src/goog_get_msg.ts index 2547839cd8..829dcb70ca 100644 --- a/packages/private/testing/src/goog_get_msg.ts +++ b/packages/private/testing/src/goog_get_msg.ts @@ -17,7 +17,8 @@ export function polyfillGoogGetMsg(translations: {[key: string]: string} = {}): const glob = (global as any); glob.goog = glob.goog || {}; glob.goog.getMsg = function(input: string, placeholders: {[key: string]: string} = {}) { - if (typeof translations[input] !== 'undefined') { // to account for empty string + if (typeof translations[input] !== 'undefined') { // to account for + // empty string input = translations[input]; } return Object.keys(placeholders).length ?