Skip to content

Commit 0806ee3

Browse files
mattlewis92mattlewis92
authored
fix(core): prevent animated element duplication with dynamic components in zoneless mode
When using ViewContainerRef to rapidly toggle animated elements in zoneless mode (e.g. CDK Overlay menus), multiple copies of the element could appear in the DOM. This happened because leave animations were queued but not yet executed, and the existing `cancelLeavingNodes` mechanism could not find the leaving element to cancel it — it ran during template execution before the new element was in the DOM, and used the declaration container's anchor which doesn't work for overlay/portal patterns where elements are moved to separate containers.
1 parent 83da928 commit 0806ee3

File tree

14 files changed

+105
-70
lines changed

14 files changed

+105
-70
lines changed

‎packages/core/src/animation/queue.ts‎

Lines changed: 0 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -103,17 +103,3 @@ export function queueEnterAnimations(
103103
addToAnimationQueue(injector, nodeAnimations.animateFns);
104104
}
105105
}
106-
107-
export function removeAnimationsFromQueue(
108-
injector: Injector,
109-
animationFns: VoidFunction | VoidFunction[],
110-
) {
111-
const animationQueue = injector.get(ANIMATION_QUEUE);
112-
if (Array.isArray(animationFns)) {
113-
for (const animateFn of animationFns) {
114-
animationQueue.queue.delete(animateFn);
115-
}
116-
} else {
117-
animationQueue.queue.delete(animationFns);
118-
}
119-
}

‎packages/core/src/animation/utils.ts‎

Lines changed: 38 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -16,12 +16,11 @@ import {
1616
LeaveNodeAnimations,
1717
AnimationClassBindingFn,
1818
} from './interfaces';
19-
import {INJECTOR, LView, DECLARATION_LCONTAINER, ANIMATIONS} from '../render3/interfaces/view';
19+
import {INJECTOR, LView, ANIMATIONS} from '../render3/interfaces/view';
2020
import {RuntimeError, RuntimeErrorCode} from '../errors';
2121
import {Renderer} from '../render3/interfaces/renderer';
2222
import {RElement} from '../render3/interfaces/renderer_dom';
2323
import {TNode} from '../render3/interfaces/node';
24-
import {getBeforeNodeForView} from '../render3/node_manipulation';
2524

2625
const DEFAULT_ANIMATIONS_DISABLED = false;
2726

@@ -131,26 +130,40 @@ export function clearLeavingNodes(tNode: TNode, el: HTMLElement): void {
131130
}
132131

133132
/**
134-
* In the case that we have an existing node that's animating away, like when
135-
* an `@if` toggles quickly, we need to end the animation for the former node
136-
* and remove it right away to prevent duplicate nodes showing up.
133+
* In the case that we have an existing node that's animating away in a
134+
* different DOM parent (e.g. a CDK overlay menu that renders each instance
135+
* in its own overlay pane), we need to end the animation for the former
136+
* node and remove it right away to prevent duplicate nodes showing up.
137+
*
138+
* Leaving elements in the same parent are left alone — their leave
139+
* animation will complete naturally and remove them from the DOM.
137140
*/
138-
export function cancelLeavingNodes(tNode: TNode, lView: LView): void {
139-
const leavingEl = leavingNodes.get(tNode)?.shift();
140-
const lContainer = lView[DECLARATION_LCONTAINER];
141-
if (lContainer) {
142-
// this is the insertion point for the new TNode element.
143-
// it will be inserted before the declaring containers anchor.
144-
const beforeNode = getBeforeNodeForView(tNode.index, lContainer);
145-
// here we need to check the previous sibling of that anchor. The first
146-
// previousSibling node will be the new element added. The second
147-
// previousSibling will be the one that's being removed.
148-
const previousNode = beforeNode?.previousSibling;
149-
// We really only want to cancel animations if the leaving node is the
150-
// same as the node before where the new node will be inserted. This is
151-
// the control flow scenario where an if was toggled.
152-
if (leavingEl && previousNode && leavingEl === previousNode) {
141+
export function cancelLeavingNodes(tNode: TNode, newElement: HTMLElement): void {
142+
const nodes = leavingNodes.get(tNode);
143+
if (!nodes || nodes.length === 0) return;
144+
145+
const newParent = newElement.parentNode;
146+
const prevSibling = newElement.previousSibling;
147+
148+
for (let i = nodes.length - 1; i >= 0; i--) {
149+
const leavingEl = nodes[i];
150+
const leavingParent = leavingEl.parentNode;
151+
// Cancel if the leaving element is:
152+
// - The direct previousSibling of the new element. This is reliable
153+
// because Angular inserts new elements at the same position (before
154+
// the container anchor) where the leaving element was, making them
155+
// always adjacent. Covers @if toggling and same-VCR toggling.
156+
// - In a different DOM parent (overlay/portal case where each instance
157+
// renders in its own container, e.g. CDK Overlay).
158+
// Leaving elements in the same parent that are NOT the previousSibling
159+
// are left alone (e.g. @for items animating out at different positions).
160+
if (
161+
(prevSibling && leavingEl === prevSibling) ||
162+
(leavingParent && newParent && leavingParent !== newParent)
163+
) {
164+
nodes.splice(i, 1);
153165
leavingEl.dispatchEvent(new CustomEvent('animationend', {detail: {cancel: true}}));
166+
leavingEl.parentNode?.removeChild(leavingEl);
154167
}
155168
}
156169
}
@@ -164,8 +177,11 @@ export function trackLeavingNodes(tNode: TNode, el: HTMLElement): void {
164177
// We need to track this tNode's element just to be sure we don't add
165178
// a new RNode for this TNode while this one is still animating away.
166179
// once the animation is complete, we remove this reference.
167-
if (leavingNodes.has(tNode)) {
168-
leavingNodes.get(tNode)?.push(el);
180+
const nodes = leavingNodes.get(tNode);
181+
if (nodes) {
182+
if (!nodes.includes(el)) {
183+
nodes.push(el);
184+
}
169185
} else {
170186
leavingNodes.set(tNode, [el]);
171187
}

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

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,6 @@ import {
2929
assertAnimationTypes,
3030
assertElementNodes,
3131
cancelAnimationsIfRunning,
32-
cancelLeavingNodes,
3332
cleanupAfterLeaveAnimations,
3433
cleanupEnterClassData,
3534
clearLeavingNodes,
@@ -69,7 +68,6 @@ export function ɵɵanimateEnter(value: string | AnimationClassBindingFn): typeo
6968
}
7069

7170
const tNode = getCurrentTNode()!;
72-
cancelLeavingNodes(tNode, lView);
7371

7472
// Capture NgZone eagerly while the injector is still valid. The animation
7573
// function runs later from the queue, at which point the lView injector
@@ -205,7 +203,6 @@ export function ɵɵanimateEnterListener(value: AnimationFunction): typeof ɵɵa
205203
return ɵɵanimateEnterListener;
206204
}
207205
const tNode = getCurrentTNode()!;
208-
cancelLeavingNodes(tNode, lView);
209206

210207
addAnimationToLView(getLViewEnterAnimations(lView), tNode, () =>
211208
runEnterAnimationFunction(lView, tNode, value),
@@ -259,7 +256,6 @@ export function ɵɵanimateLeave(value: string | AnimationClassBindingFn): typeo
259256
}
260257

261258
const tNode = getCurrentTNode()!;
262-
cancelLeavingNodes(tNode, lView);
263259

264260
// Capture NgZone eagerly while the injector is still valid. The animation
265261
// function runs later from the queue, at which point the lView injector
@@ -397,7 +393,6 @@ export function ɵɵanimateLeaveListener(value: AnimationFunction): typeof ɵɵa
397393

398394
const lView = getLView();
399395
const tNode = getCurrentTNode()!;
400-
cancelLeavingNodes(tNode, lView);
401396

402397
allLeavingAnimations.add(lView[ID]);
403398

‎packages/core/src/render3/node_animations.ts‎

Lines changed: 1 addition & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -12,11 +12,7 @@ import {
1212
AnimationLViewData,
1313
} from '../animation/interfaces';
1414
import {allLeavingAnimations} from '../animation/longest_animation';
15-
import {
16-
queueEnterAnimations,
17-
addToAnimationQueue,
18-
removeAnimationsFromQueue,
19-
} from '../animation/queue';
15+
import {queueEnterAnimations, addToAnimationQueue} from '../animation/queue';
2016
import {Injector, INJECTOR} from '../di';
2117
import {CONTAINER_HEADER_OFFSET} from './interfaces/container';
2218
import {TNode, TNodeType} from './interfaces/node';
@@ -55,11 +51,6 @@ export function runLeaveAnimationsWithCallback(
5551

5652
const animations = lView?.[ANIMATIONS];
5753

58-
// regarding the TNode index to see if it is the same element.
59-
if (animations?.enter?.has(tNode.index)) {
60-
removeAnimationsFromQueue(injector, animations.enter.get(tNode.index)!.animateFns);
61-
}
62-
6354
// get all nodes in the current view that are descendants of tNode and have leave animations
6455
const nodesWithExitAnimations = aggregateDescendantAnimations(lView, tNode, animations);
6556

‎packages/core/src/render3/node_manipulation.ts‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,7 @@ import {assertTNodeType} from './node_assert';
8282
import {profiler} from './profiler';
8383
import {ProfilerEvent} from '../../primitives/devtools';
8484
import {getLViewParent, getNativeByTNode, unwrapRNode} from './util/view_utils';
85+
import {cancelLeavingNodes, trackLeavingNodes} from '../animation/utils';
8586
import {Injector} from '../di';
8687
import {maybeQueueEnterAnimation, runLeaveAnimationsWithCallback} from './node_animations';
8788

@@ -144,7 +145,11 @@ function applyToElementOrContainer(
144145
} else if (action === WalkTNodeTreeAction.Insert && parent !== null) {
145146
maybeQueueEnterAnimation(parentLView, parent, tNode, injector);
146147
nativeInsertBefore(renderer, parent, rNode, beforeNode || null, true);
148+
cancelLeavingNodes(tNode, rNode as HTMLElement);
147149
} else if (action === WalkTNodeTreeAction.Detach) {
150+
if (parentLView?.[ANIMATIONS]?.leave?.has(tNode.index)) {
151+
trackLeavingNodes(tNode, rNode as HTMLElement);
152+
}
148153
runLeaveAnimationsWithCallback(
149154
parentLView,
150155
tNode,

‎packages/core/test/acceptance/animation_spec.ts‎

Lines changed: 37 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -2373,7 +2373,7 @@ describe('Animation', () => {
23732373
});
23742374

23752375
describe('animation element duplication', () => {
2376-
it('should not duplicate elements when using dynamic components', async () => {
2376+
it('should not duplicate elements when using dynamic components in overlay-like containers', fakeAsync(() => {
23772377
const animateStyles = `
23782378
.example-menu {
23792379
display: inline-flex;
@@ -2415,6 +2415,7 @@ describe('Animation', () => {
24152415
@ViewChild('menu') menuTpl!: TemplateRef<unknown>;
24162416
vcr = inject(ViewContainerRef);
24172417
opened = false;
2418+
private currentPane: HTMLElement | null = null;
24182419

24192420
toggle() {
24202421
if (this.opened) {
@@ -2426,12 +2427,23 @@ describe('Animation', () => {
24262427

24272428
open() {
24282429
this.opened = true;
2429-
this.vcr.createEmbeddedView(this.menuTpl);
2430+
const viewRef = this.vcr.createEmbeddedView(this.menuTpl);
2431+
// Simulate CDK DomPortalOutlet: after creating the view, move
2432+
// the root nodes to a new "overlay pane" div, just like CDK
2433+
// Overlay does with outletElement.appendChild(rootNode).
2434+
const pane = document.createElement('div');
2435+
pane.className = 'overlay-pane';
2436+
document.body.appendChild(pane);
2437+
for (const node of viewRef.rootNodes) {
2438+
pane.appendChild(node);
2439+
}
2440+
this.currentPane = pane;
24302441
}
24312442

24322443
close() {
24332444
this.opened = false;
24342445
this.vcr.clear();
2446+
// CDK Overlay may or may not remove the pane immediately
24352447
}
24362448
}
24372449

@@ -2449,22 +2461,36 @@ describe('Animation', () => {
24492461

24502462
const cmp = fixture.debugElement.query(By.css('dynamic-menu')).componentInstance;
24512463

2452-
const delay = (ms: number) => new Promise((resolve) => setTimeout(resolve, ms));
2464+
// Query from document since overlay panes are appended to body
2465+
const countMenus = () => document.querySelectorAll('.example-menu').length;
2466+
2467+
// Helper to complete the leave animation for all leaving menu elements
2468+
const finishLeaveAnimations = () => {
2469+
tickAnimationFrames(1);
2470+
document.querySelectorAll('.example-menu.close').forEach((el) => {
2471+
el.dispatchEvent(new AnimationEvent('animationend', {animationName: 'open'}));
2472+
});
2473+
tick();
2474+
};
24532475

2454-
// Toggle the menu quickly multiple times
2476+
// Simulate rapid clicking with CD between each toggle
24552477
for (let i = 0; i < 20; i++) {
24562478
cmp.toggle();
2457-
// Wait 1ms to allow some logic to run but less than animation duration
2458-
await delay(1);
24592479
fixture.detectChanges();
2480+
tickAnimationFrames(1);
2481+
// At no point should there be more than one menu element
2482+
expect(countMenus()).toBeLessThanOrEqual(1);
24602483
}
24612484

2462-
// Finish remaining animations (wait 100ms real time which is > 10ms animation)
2463-
await delay(200);
2485+
// Complete any remaining leave animations
2486+
finishLeaveAnimations();
24642487
fixture.detectChanges();
24652488

2466-
const menus = fixture.debugElement.nativeElement.querySelectorAll('.example-menu');
2467-
expect(menus.length).toBe(0);
2468-
});
2489+
// 20 toggles (even) = closed = 0 elements
2490+
expect(countMenus()).toBe(0);
2491+
2492+
// Clean up overlay panes
2493+
document.querySelectorAll('.overlay-pane').forEach((p) => p.remove());
2494+
}));
24692495
});
24702496
});

‎packages/core/test/bundling/animations-standalone/bundle.golden_symbols.json‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -364,6 +364,7 @@
364364
"callHook",
365365
"callHookInternal",
366366
"callHooks",
367+
"cancelLeavingNodes",
367368
"captureError",
368369
"checkStable",
369370
"classIndexOf",
@@ -661,6 +662,7 @@
661662
"leaveSkipHydrationBlock",
662663
"leaveView",
663664
"leaveViewLight",
665+
"leavingNodes",
664666
"linkTNodeInTView",
665667
"listenOnPlayer",
666668
"locateDirectiveOrProvider",
@@ -760,7 +762,6 @@
760762
"relativePath",
761763
"rememberChangeHistoryAndInvokeOnChangesHook",
762764
"remove",
763-
"removeAnimationsFromQueue",
764765
"removeClass",
765766
"removeElements",
766767
"removeFromArray",
@@ -839,6 +840,7 @@
839840
"timeoutProvider",
840841
"toInputRefArray",
841842
"toOutputRefArray",
843+
"trackLeavingNodes",
842844
"trackMovedView",
843845
"transition",
844846
"transitionFailed",

‎packages/core/test/bundling/create_component/bundle.golden_symbols.json‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -284,6 +284,7 @@
284284
"callHook",
285285
"callHookInternal",
286286
"callHooks",
287+
"cancelLeavingNodes",
287288
"captureError",
288289
"checkStable",
289290
"cleanUpView",
@@ -550,6 +551,7 @@
550551
"leaveDI",
551552
"leaveView",
552553
"leaveViewLight",
554+
"leavingNodes",
553555
"linkTNodeInTView",
554556
"listenToDomEvent",
555557
"listenToOutput",
@@ -623,7 +625,6 @@
623625
"relativePath",
624626
"rememberChangeHistoryAndInvokeOnChangesHook",
625627
"remove",
626-
"removeAnimationsFromQueue",
627628
"removeElements",
628629
"removeFromArray",
629630
"removeLViewOnDestroy",
@@ -696,6 +697,7 @@
696697
"timeoutProvider",
697698
"toInputRefArray",
698699
"toOutputRefArray",
700+
"trackLeavingNodes",
699701
"trackMovedView",
700702
"uniqueIdCounter",
701703
"unregisterLView",

‎packages/core/test/bundling/defer/bundle.golden_symbols.json‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -327,6 +327,7 @@
327327
"callHook",
328328
"callHookInternal",
329329
"callHooks",
330+
"cancelLeavingNodes",
330331
"captureError",
331332
"checkStable",
332333
"classIndexOf",
@@ -593,6 +594,7 @@
593594
"leaveSkipHydrationBlock",
594595
"leaveView",
595596
"leaveViewLight",
597+
"leavingNodes",
596598
"linkTNodeInTView",
597599
"locateDirectiveOrProvider",
598600
"locateHostElement",
@@ -660,7 +662,6 @@
660662
"registerPreOrderHooks",
661663
"rememberChangeHistoryAndInvokeOnChangesHook",
662664
"remove",
663-
"removeAnimationsFromQueue",
664665
"removeFromArray",
665666
"removeLViewFromLContainer",
666667
"removeLViewOnDestroy",
@@ -734,6 +735,7 @@
734735
"timeoutProvider",
735736
"toInputRefArray",
736737
"toOutputRefArray",
738+
"trackLeavingNodes",
737739
"trackMovedView",
738740
"triggerDeferBlock",
739741
"triggerResourceLoading",

0 commit comments

Comments
 (0)