Skip to content

Commit 66b72bf

Browse files
marclavalmhevery
authored andcommitted
fix(ivy): ViewContainerRef.destroy should properly clean the DOM (angular#29414)
PR Close angular#29414
1 parent 0007564 commit 66b72bf

File tree

8 files changed

+264
-26
lines changed

8 files changed

+264
-26
lines changed

packages/common/test/directives/ng_switch_spec.ts

Lines changed: 48 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77
*/
88

99
import {CommonModule} from '@angular/common';
10-
import {Attribute, Component, Directive} from '@angular/core';
10+
import {Attribute, Component, Directive, TemplateRef, ViewChild} from '@angular/core';
1111
import {ComponentFixture, TestBed} from '@angular/core/testing';
1212
import {expect} from '@angular/platform-browser/testing/src/matchers';
1313

@@ -26,7 +26,7 @@ import {expect} from '@angular/platform-browser/testing/src/matchers';
2626

2727
beforeEach(() => {
2828
TestBed.configureTestingModule({
29-
declarations: [TestComponent],
29+
declarations: [TestComponent, ComplexComponent],
3030
imports: [CommonModule],
3131
});
3232
});
@@ -171,6 +171,20 @@ import {expect} from '@angular/platform-browser/testing/src/matchers';
171171
getComponent().switchValue = 'b';
172172
detectChangesAndExpectText('when b1;when b2;');
173173
});
174+
175+
it('should support nested NgSwitch on ng-container with ngTemplateOutlet', () => {
176+
fixture = TestBed.createComponent(ComplexComponent);
177+
detectChangesAndExpectText('Foo');
178+
179+
fixture.componentInstance.state = 'case2';
180+
detectChangesAndExpectText('Bar');
181+
182+
fixture.componentInstance.state = 'notACase';
183+
detectChangesAndExpectText('Default');
184+
185+
fixture.componentInstance.state = 'case1';
186+
detectChangesAndExpectText('Foo');
187+
});
174188
});
175189
});
176190
}
@@ -182,6 +196,38 @@ class TestComponent {
182196
when2: any = null;
183197
}
184198

199+
@Component({
200+
selector: 'complex-cmp',
201+
template: `
202+
<div [ngSwitch]="state">
203+
<ng-container *ngSwitchCase="'case1'" [ngSwitch]="true">
204+
<ng-container *ngSwitchCase="true" [ngTemplateOutlet]="foo"></ng-container>
205+
<span *ngSwitchDefault>Should never render</span>
206+
</ng-container>
207+
<ng-container *ngSwitchCase="'case2'" [ngSwitch]="true">
208+
<ng-container *ngSwitchCase="true" [ngTemplateOutlet]="bar"></ng-container>
209+
<span *ngSwitchDefault>Should never render</span>
210+
</ng-container>
211+
<ng-container *ngSwitchDefault [ngSwitch]="false">
212+
<ng-container *ngSwitchCase="true" [ngTemplateOutlet]="foo"></ng-container>
213+
<span *ngSwitchDefault>Default</span>
214+
</ng-container>
215+
</div>
216+
217+
<ng-template #foo>
218+
<span>Foo</span>
219+
</ng-template>
220+
<ng-template #bar>
221+
<span>Bar</span>
222+
</ng-template>
223+
`
224+
})
225+
class ComplexComponent {
226+
@ViewChild('foo') foo !: TemplateRef<any>;
227+
@ViewChild('bar') bar !: TemplateRef<any>;
228+
state: string = 'case1';
229+
}
230+
185231
function createTestComponent(template: string): ComponentFixture<TestComponent> {
186232
return TestBed.overrideComponent(TestComponent, {set: {template: template}})
187233
.createComponent(TestComponent);

packages/core/src/render3/instructions/instructions.ts

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1952,7 +1952,7 @@ function generateInitialInputs(
19521952
*/
19531953
export function createLContainer(
19541954
hostNative: RElement | RComment | StylingContext | LView, currentView: LView, native: RComment,
1955-
isForViewContainerRef?: boolean): LContainer {
1955+
tNode: TNode, isForViewContainerRef?: boolean): LContainer {
19561956
ngDevMode && assertDomNode(native);
19571957
ngDevMode && assertLView(currentView);
19581958
const lContainer: LContainer = [
@@ -1962,8 +1962,9 @@ export function createLContainer(
19621962
currentView, // parent
19631963
null, // next
19641964
null, // queries
1965-
[], // views
1965+
tNode, // t_host
19661966
native, // native
1967+
[], // views
19671968
];
19681969
ngDevMode && attachLContainerDebug(lContainer);
19691970
return lContainer;
@@ -2037,7 +2038,8 @@ function containerInternal(
20372038
const comment = lView[RENDERER].createComment(ngDevMode ? 'container' : '');
20382039
ngDevMode && ngDevMode.rendererCreateComment++;
20392040
const tNode = createNodeAtIndex(index, TNodeType.Container, comment, tagName, attrs);
2040-
const lContainer = lView[adjustedIndex] = createLContainer(lView[adjustedIndex], lView, comment);
2041+
const lContainer = lView[adjustedIndex] =
2042+
createLContainer(lView[adjustedIndex], lView, comment, tNode);
20412043

20422044
appendChild(comment, tNode, lView);
20432045

packages/core/src/render3/interfaces/container.ts

Lines changed: 16 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -6,10 +6,12 @@
66
* found in the LICENSE file at https://angular.io/license
77
*/
88

9+
import {TNode} from './node';
910
import {LQueries} from './query';
1011
import {RComment, RElement} from './renderer';
1112
import {StylingContext} from './styling';
12-
import {HOST, LView, NEXT, PARENT, QUERIES} from './view';
13+
import {HOST, LView, NEXT, PARENT, QUERIES, T_HOST} from './view';
14+
1315

1416
/**
1517
* Special location which allows easy identification of type. If we have an array which was
@@ -23,10 +25,10 @@ export const TYPE = 1;
2325
* Uglify will inline these when minifying so there shouldn't be a cost.
2426
*/
2527
export const ACTIVE_INDEX = 2;
26-
// PARENT, NEXT, and QUERIES are indices 3, 4, and 5.
28+
// PARENT, NEXT, QUERIES and T_HOST are indices 3, 4, 5 and 6.
2729
// As we already have these constants in LView, we don't need to re-create them.
28-
export const VIEWS = 6;
2930
export const NATIVE = 7;
31+
export const VIEWS = 8;
3032

3133
/**
3234
* The state associated with a container.
@@ -83,17 +85,22 @@ export interface LContainer extends Array<any> {
8385
// `[QUERIES]` in it which are not needed for `LContainer` (only needed for Template)
8486

8587
/**
86-
* A list of the container's currently active child views. Views will be inserted
87-
* here as they are added and spliced from here when they are removed. We need
88-
* to keep a record of current views so we know which views are already in the DOM
89-
* (and don't need to be re-added) and so we can remove views from the DOM when they
90-
* are no longer required.
88+
* Pointer to the `TNode` which represents the host of the container.
9189
*/
92-
[VIEWS]: LView[];
90+
[T_HOST]: TNode;
9391

9492
/** The comment element that serves as an anchor for this LContainer. */
9593
readonly[NATIVE]:
9694
RComment; // TODO(misko): remove as this value can be gotten by unwrapping `[HOST]`
95+
96+
/**
97+
*A list of the container's currently active child views. Views will be inserted
98+
*here as they are added and spliced from here when they are removed. We need
99+
*to keep a record of current views so we know which views are already in the DOM
100+
*(and don't need to be re-added) and so we can remove views from the DOM when they
101+
*are no longer required.
102+
*/
103+
[VIEWS]: LView[];
97104
}
98105

99106
// Note: This hack is necessary so we don't erroneously get a circular dependency

packages/core/src/render3/node_manipulation.ts

Lines changed: 26 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -91,14 +91,22 @@ function walkTNodeTree(
9191
let tNode: TNode|null = rootTNode.child as TNode;
9292
while (tNode) {
9393
let nextTNode: TNode|null = null;
94-
if (tNode.type === TNodeType.Element) {
94+
if (tNode.type === TNodeType.Element || tNode.type === TNodeType.ElementContainer) {
9595
executeNodeAction(
9696
action, renderer, renderParent, getNativeByTNode(tNode, currentView), tNode, beforeNode);
9797
const nodeOrContainer = currentView[tNode.index];
9898
if (isLContainer(nodeOrContainer)) {
9999
// This element has an LContainer, and its comment needs to be handled
100100
executeNodeAction(
101101
action, renderer, renderParent, nodeOrContainer[NATIVE], tNode, beforeNode);
102+
if (nodeOrContainer[VIEWS].length) {
103+
currentView = nodeOrContainer[VIEWS][0];
104+
nextTNode = currentView[TVIEW].node;
105+
106+
// When the walker enters a container, then the beforeNode has to become the local native
107+
// comment node.
108+
beforeNode = nodeOrContainer[NATIVE];
109+
}
102110
}
103111
} else if (tNode.type === TNodeType.Container) {
104112
const lContainer = currentView ![tNode.index] as LContainer;
@@ -133,9 +141,8 @@ function walkTNodeTree(
133141
nextTNode = currentView[TVIEW].data[head.index] as TNode;
134142
}
135143
}
136-
137144
} else {
138-
// Otherwise, this is a View or an ElementContainer
145+
// Otherwise, this is a View
139146
nextTNode = tNode.child;
140147
}
141148

@@ -145,7 +152,14 @@ function walkTNodeTree(
145152
currentView = projectionNodeStack[projectionNodeIndex--] as LView;
146153
tNode = projectionNodeStack[projectionNodeIndex--] as TNode;
147154
}
148-
nextTNode = (tNode.flags & TNodeFlags.isProjected) ? tNode.projectionNext : tNode.next;
155+
156+
if (tNode.flags & TNodeFlags.isProjected) {
157+
nextTNode = tNode.projectionNext;
158+
} else if (tNode.type === TNodeType.ElementContainer) {
159+
nextTNode = tNode.child || tNode.next;
160+
} else {
161+
nextTNode = tNode.next;
162+
}
149163

150164
/**
151165
* Find the next node in the TNode tree, taking into account the place where a node is
@@ -172,19 +186,26 @@ function walkTNodeTree(
172186
* chain until:
173187
* - we find an lView with a next pointer
174188
* - or find a tNode with a parent that has a next pointer
189+
* - or find a lContainer
175190
* - or reach root TNode (in which case we exit, since we traversed all nodes)
176191
*/
177192
while (!currentView[NEXT] && currentView[PARENT] &&
178193
!(tNode.parent && tNode.parent.next)) {
179194
if (tNode === rootTNode) return;
180195
currentView = currentView[PARENT] as LView;
196+
if (isLContainer(currentView)) {
197+
tNode = currentView[T_HOST] !;
198+
currentView = currentView[PARENT];
199+
beforeNode = currentView[tNode.index][NATIVE];
200+
break;
201+
}
181202
tNode = currentView[T_HOST] !;
182203
}
183204
if (currentView[NEXT]) {
184205
currentView = currentView[NEXT] as LView;
185206
nextTNode = currentView[T_HOST];
186207
} else {
187-
nextTNode = tNode.next;
208+
nextTNode = tNode.type === TNodeType.ElementContainer && tNode.child || tNode.next;
188209
}
189210
} else {
190211
nextTNode = tNode.next;

packages/core/src/render3/view_engine_compatibility.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -321,7 +321,7 @@ export function createContainerRef(
321321
}
322322

323323
hostView[hostTNode.index] = lContainer =
324-
createLContainer(slotValue, hostView, commentNode, true);
324+
createLContainer(slotValue, hostView, commentNode, hostTNode, true);
325325

326326
addToViewTree(hostView, lContainer);
327327
}

0 commit comments

Comments
 (0)