From dcdd1bcdbbd2a2fb4bd1fc4330259824d0bc8cb9 Mon Sep 17 00:00:00 2001 From: Jessica Janiuk Date: Fri, 31 Oct 2025 08:58:27 -0700 Subject: [PATCH] fix(core): skip leave animations on view swaps We accounted for skipping leave animations during moves, but not swaps. This accounts for the swap cases and updates how we deal with swaps and moves. Now we always queue animations and then essentially dequeue them if we attach them back in the same render pass. fixes: #64818 fixes: #64730 --- packages/core/src/animation/interfaces.ts | 14 +++-- packages/core/src/animation/queue.ts | 21 +++++++- .../src/render3/instructions/control_flow.ts | 46 +++++++++++++--- .../core/src/render3/list_reconciliation.ts | 4 +- .../core/src/render3/node_manipulation.ts | 53 +++++++++---------- .../core/test/acceptance/animation_spec.ts | 41 ++++++++++++++ 6 files changed, 133 insertions(+), 46 deletions(-) diff --git a/packages/core/src/animation/interfaces.ts b/packages/core/src/animation/interfaces.ts index f17a9f0bf3e..3302a6cf192 100644 --- a/packages/core/src/animation/interfaces.ts +++ b/packages/core/src/animation/interfaces.ts @@ -52,7 +52,7 @@ const MAX_ANIMATION_TIMEOUT_DEFAULT = 4000; */ export type AnimationFunction = (event: AnimationCallbackEvent) => void; -export type RunEnterAnimationFn = () => void; +export type RunEnterAnimationFn = VoidFunction; export type RunLeaveAnimationFn = () => {promise: Promise; resolve: VoidFunction}; export interface LongestAnimation { @@ -81,13 +81,11 @@ export interface AnimationLViewData { // We chose to use unknown instead of PromiseSettledResult to avoid requiring the type running?: Promise; - // Skip leave animations - // This flag is solely used when move operations occur. DOM Node move - // operations occur in lists, like `@for` loops, and use the same code - // path during move that detaching or removing does. So to prevent - // unexpected disappearing of moving nodes, we use this flag to skip - // the animations in that case. - skipLeaveAnimations?: boolean; + // Animation functions that have been queued for this view when the view is detached. + // This is used to later remove them from the global animation queue if the view + // is attached before the animation queue runs. This is used in cases where views are + // moved or swapped during list reconciliation. + detachedLeaveAnimationFns?: VoidFunction[]; } /** diff --git a/packages/core/src/animation/queue.ts b/packages/core/src/animation/queue.ts index c430269f28c..4ce0a50fe1d 100644 --- a/packages/core/src/animation/queue.ts +++ b/packages/core/src/animation/queue.ts @@ -8,7 +8,7 @@ import {afterNextRender} from '../render3/after_render/hooks'; import {InjectionToken, Injector} from '../di'; -import {EnterNodeAnimations} from './interfaces'; +import {AnimationLViewData, EnterNodeAnimations} from './interfaces'; export interface AnimationQueue { queue: Set; @@ -36,18 +36,37 @@ export const ANIMATION_QUEUE = new InjectionToken( export function addToAnimationQueue( injector: Injector, animationFns: VoidFunction | VoidFunction[], + animationData?: AnimationLViewData, ) { const animationQueue = injector.get(ANIMATION_QUEUE); if (Array.isArray(animationFns)) { for (const animateFn of animationFns) { animationQueue.queue.add(animateFn); + // If a node is detached, we need to keep track of the queued animation functions + // so we can later remove them from the global animation queue if the view + // is re-attached before the animation queue runs. + animationData?.detachedLeaveAnimationFns?.push(animateFn); } } else { animationQueue.queue.add(animationFns); + // If a node is detached, we need to keep track of the queued animation functions + // so we can later remove them from the global animation queue if the view + // is re-attached before the animation queue runs. + animationData?.detachedLeaveAnimationFns?.push(animationFns); } animationQueue.scheduler && animationQueue.scheduler(injector); } +export function removeFromAnimationQueue(injector: Injector, animationData: AnimationLViewData) { + const animationQueue = injector.get(ANIMATION_QUEUE); + if (animationData.detachedLeaveAnimationFns) { + for (const animationFn of animationData.detachedLeaveAnimationFns) { + animationQueue.queue.delete(animationFn); + } + animationData.detachedLeaveAnimationFns = undefined; + } +} + export function scheduleAnimationQueue(injector: Injector) { const animationQueue = injector.get(ANIMATION_QUEUE); // We only want to schedule the animation queue if it hasn't already been scheduled. diff --git a/packages/core/src/render3/instructions/control_flow.ts b/packages/core/src/render3/instructions/control_flow.ts index 09ab65cc379..2822ad29ccd 100644 --- a/packages/core/src/render3/instructions/control_flow.ts +++ b/packages/core/src/render3/instructions/control_flow.ts @@ -28,6 +28,7 @@ import { DECLARATION_COMPONENT_VIEW, HEADER_OFFSET, HYDRATION, + INJECTOR, LView, TVIEW, TView, @@ -39,15 +40,17 @@ import {NO_CHANGE} from '../tokens'; import {getConstant, getTNode} from '../util/view_utils'; import {createAndRenderEmbeddedLView, shouldAddViewToDom} from '../view_manipulation'; -import {declareNoDirectiveHostTemplate} from './template'; +import {AnimationLViewData} from '../../animation/interfaces'; +import {removeDehydratedViews} from '../../hydration/cleanup'; import { addLViewToLContainer, detachView, getLViewFromLContainer, removeLViewFromLContainer, } from '../view/container'; -import {removeDehydratedViews} from '../../hydration/cleanup'; -import {AnimationLViewData} from '../../animation/interfaces'; +import {declareNoDirectiveHostTemplate} from './template'; +import {removeFromAnimationQueue} from '../../animation/queue'; +import {allLeavingAnimations} from '../../animation/longest_animation'; /** * Creates an LContainer for an ng-template representing a root node @@ -419,10 +422,11 @@ class LiveCollectionLContainerImpl extends LiveCollection< index, shouldAddViewToDom(this.templateTNode, dehydratedView), ); + clearDetachAnimationList(this.lContainer, index); } - override detach(index: number, skipLeaveAnimations?: boolean): LView> { + override detach(index: number): LView> { this.needsIndexUpdate ||= index !== this.length - 1; - if (skipLeaveAnimations) setSkipLeaveAnimations(this.lContainer, index); + maybeInitDetachAnimationList(this.lContainer, index); return detachExistingView>(this.lContainer, index); } override create(index: number, value: unknown): LView> { @@ -570,13 +574,39 @@ function getLContainer(lView: LView, index: number): LContainer { return lContainer; } -function setSkipLeaveAnimations(lContainer: LContainer, index: number): void { +function clearDetachAnimationList(lContainer: LContainer, index: number): void { if (lContainer.length <= CONTAINER_HEADER_OFFSET) return; const indexInContainer = CONTAINER_HEADER_OFFSET + index; const viewToDetach = lContainer[indexInContainer]; - if (viewToDetach && viewToDetach[ANIMATIONS]) { - (viewToDetach[ANIMATIONS] as AnimationLViewData).skipLeaveAnimations = true; + const animations = viewToDetach + ? (viewToDetach[ANIMATIONS] as AnimationLViewData | undefined) + : undefined; + if ( + viewToDetach && + animations && + animations.detachedLeaveAnimationFns && + animations.detachedLeaveAnimationFns.length > 0 + ) { + const injector = viewToDetach[INJECTOR]; + removeFromAnimationQueue(injector, animations); + allLeavingAnimations.delete(viewToDetach); + animations.detachedLeaveAnimationFns = undefined; + } +} + +// Initialize the detach leave animation list for a view about to be detached, but only +// if it has leave animations. +function maybeInitDetachAnimationList(lContainer: LContainer, index: number): void { + if (lContainer.length <= CONTAINER_HEADER_OFFSET) return; + + const indexInContainer = CONTAINER_HEADER_OFFSET + index; + const viewToDetach = lContainer[indexInContainer]; + const animations = viewToDetach + ? (viewToDetach[ANIMATIONS] as AnimationLViewData | undefined) + : undefined; + if (animations && animations.leave && animations.leave.size > 0) { + animations.detachedLeaveAnimationFns = []; } } diff --git a/packages/core/src/render3/list_reconciliation.ts b/packages/core/src/render3/list_reconciliation.ts index 6cf93e89ddb..e2cfb407dea 100644 --- a/packages/core/src/render3/list_reconciliation.ts +++ b/packages/core/src/render3/list_reconciliation.ts @@ -20,7 +20,7 @@ export abstract class LiveCollection { abstract get length(): number; abstract at(index: number): V; abstract attach(index: number, item: T): void; - abstract detach(index: number, skipLeaveAnimations?: boolean): T; + abstract detach(index: number): T; abstract create(index: number, value: V): T; destroy(item: T): void { // noop by default @@ -49,7 +49,7 @@ export abstract class LiveCollection { // DOM nodes, which would trigger `animate.leave` bindings. We need to skip // those animations in the case of a move operation so the moving elements don't // unexpectedly disappear. - this.attach(newIdx, this.detach(prevIndex, true /* skipLeaveAnimations */)); + this.attach(newIdx, this.detach(prevIndex)); } } diff --git a/packages/core/src/render3/node_manipulation.ts b/packages/core/src/render3/node_manipulation.ts index 255d04b03f0..3548ee6468e 100644 --- a/packages/core/src/render3/node_manipulation.ts +++ b/packages/core/src/render3/node_manipulation.ts @@ -84,6 +84,7 @@ import {getLViewParent, getNativeByTNode, unwrapRNode} from './util/view_utils'; import {allLeavingAnimations} from '../animation/longest_animation'; import {Injector} from '../di'; import {addToAnimationQueue, queueEnterAnimations} from '../animation/queue'; +import {RunLeaveAnimationFn} from '../animation/interfaces'; const enum WalkTNodeTreeAction { /** node create in the native environment. Run on initial creation. */ @@ -386,37 +387,35 @@ function runLeaveAnimationsWithCallback( if (animations == null || animations.leave == undefined || !animations.leave.has(tNode.index)) return callback(false); - // this is solely for move operations to prevent leave animations from running - // on the moved nodes, which would have deleted the node. - if (animations.skipLeaveAnimations) { - animations.skipLeaveAnimations = false; - return callback(false); - } - if (lView) allLeavingAnimations.add(lView); - addToAnimationQueue(injector, () => { - // it's possible that in the time between when the leave animation was - // and the time it was executed, the data structure changed. So we need - // to be safe here. - if (animations.leave && animations.leave.has(tNode.index)) { - const leaveAnimationMap = animations.leave; - const leaveAnimations = leaveAnimationMap.get(tNode.index); - const runningAnimations = []; - if (leaveAnimations) { - for (let index = 0; index < leaveAnimations.animateFns.length; index++) { - const animationFn = leaveAnimations.animateFns[index]; - const {promise} = animationFn(); - runningAnimations.push(promise); + addToAnimationQueue( + injector, + () => { + // it's possible that in the time between when the leave animation was + // and the time it was executed, the data structure changed. So we need + // to be safe here. + if (animations.leave && animations.leave.has(tNode.index)) { + const leaveAnimationMap = animations.leave; + const leaveAnimations = leaveAnimationMap.get(tNode.index); + const runningAnimations = []; + if (leaveAnimations) { + for (let index = 0; index < leaveAnimations.animateFns.length; index++) { + const animationFn = leaveAnimations.animateFns[index]; + const {promise} = animationFn() as ReturnType; + runningAnimations.push(promise); + } + animations.detachedLeaveAnimationFns = undefined; } + animations.running = Promise.allSettled(runningAnimations); + runAfterLeaveAnimations(lView!, callback); + } else { + if (lView) allLeavingAnimations.delete(lView); + callback(false); } - animations.running = Promise.allSettled(runningAnimations); - runAfterLeaveAnimations(lView!, callback); - } else { - if (lView) allLeavingAnimations.delete(lView); - callback(false); - } - }); + }, + animations, + ); } function runAfterLeaveAnimations(lView: LView, callback: Function) { diff --git a/packages/core/test/acceptance/animation_spec.ts b/packages/core/test/acceptance/animation_spec.ts index d6592d8c221..dec9fc95260 100644 --- a/packages/core/test/acceptance/animation_spec.ts +++ b/packages/core/test/acceptance/animation_spec.ts @@ -2057,6 +2057,47 @@ describe('Animation', () => { expect(fixture.debugElement.queryAll(By.css('p')).length).toBe(3); })); + it('should not remove elements when swapping or moving nodes', fakeAsync(() => { + const animateSpy = jasmine.createSpy('animateSpy'); + @Component({ + selector: 'test-cmp', + template: ` +
+ @for (item of items; track item.id) { +

{{ item.id }}

+ } +
+ `, + encapsulation: ViewEncapsulation.None, + }) + class TestComponent { + items = [{id: 1}, {id: 2}, {id: 3}]; + private cd = inject(ChangeDetectorRef); + + animate(event: AnimationCallbackEvent) { + animateSpy(); + event.animationComplete(); + } + + shuffle() { + this.items = this.shuffleArray(this.items); + this.cd.markForCheck(); + } + + shuffleArray(array: readonly T[]): T[] { + return [array[1], array[2], array[0]]; + } + } + TestBed.configureTestingModule({animationsEnabled: true}); + + const fixture = TestBed.createComponent(TestComponent); + const cmp = fixture.componentInstance; + cmp.shuffle(); + fixture.detectChanges(); + expect(animateSpy).not.toHaveBeenCalled(); + expect(fixture.debugElement.queryAll(By.css('p')).length).toBe(3); + })); + it('should not remove elements when child element animations finish', fakeAsync(() => { const animateStyles = ` .fade {