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

@@ -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<void>; resolve: VoidFunction};
 
 export interface LongestAnimation {
@@ -81,13 +81,11 @@ export interface AnimationLViewData {
   // We chose to use unknown instead of PromiseSettledResult<void> to avoid requiring the type
   running?: Promise<unknown>;
 
-  // 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[];
 }
 
 /**

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<VoidFunction>;
@@ -36,18 +36,37 @@ export const ANIMATION_QUEUE = new InjectionToken<AnimationQueue>(
 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.

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<RepeaterContext<unknown>> {
+  override detach(index: number): LView<RepeaterContext<unknown>> {
     this.needsIndexUpdate ||= index !== this.length - 1;
-    if (skipLeaveAnimations) setSkipLeaveAnimations(this.lContainer, index);
+    maybeInitDetachAnimationList(this.lContainer, index);
     return detachExistingView<RepeaterContext<unknown>>(this.lContainer, index);
   }
   override create(index: number, value: unknown): LView<RepeaterContext<unknown>> {
@@ -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 = [];
   }
 }
 

packages/core/src/render3/list_reconciliation.ts

@@ -20,7 +20,7 @@ export abstract class LiveCollection<T, V> {
   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<T, V> {
     // 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));
   }
 }
 

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<RunLeaveAnimationFn>;
+            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) {

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: `
+          <div>
+            @for (item of items; track item.id) {
+              <p (animate.leave)="animate($event)" #el>{{ item.id }}</p>
+            }
+          </div>
+        `,
+        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<T>(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 {