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: angular#64818
fixes: angular#64730

Author: thePunderWoman

packages/core/src/animation/interfaces.ts

@@ -52,23 +52,10 @@ const MAX_ANIMATION_TIMEOUT_DEFAULT = 4000;
  */
 export type AnimationFunction = (event: AnimationCallbackEvent) => void;
 
-export type AnimationEventFunction = (
-  el: Element,
-  value: AnimationFunction,
-) => AnimationRemoveFunction;
-export type AnimationClassFunction = (
-  el: Element,
-  value: Set<string> | null,
-  resolvers: Function[] | undefined,
-) => AnimationRemoveFunction;
-export type AnimationRemoveFunction = (removeFn: VoidFunction) => void;
+export type RunEnterAnimationFn = VoidFunction;
+export type RunLeaveAnimationFn = () => {promise: Promise<void>; resolve: VoidFunction};
 
-export interface AnimationDetails {
-  classes: Set<string> | null;
-  classFns?: Function[];
-  animateFn: AnimationRemoveFunction;
-  isEventBinding: boolean;
-}
+export type RunAnimationFn = RunEnterAnimationFn | RunLeaveAnimationFn;
 
 export interface LongestAnimation {
   animationName: string | undefined;
@@ -77,7 +64,7 @@ export interface LongestAnimation {
 }
 
 export interface NodeAnimations {
-  animateFns: Function[];
+  animateFns: RunAnimationFn[];
   resolvers?: VoidFunction[];
 }
 
@@ -92,11 +79,9 @@ 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.
+  detachFnQueue?: VoidFunction[];
 }

packages/core/src/animation/queue.ts

@@ -8,10 +8,10 @@
 
 import {afterNextRender} from '../render3/after_render/hooks';
 import {InjectionToken, Injector} from '../di';
-import {NodeAnimations} from './interfaces';
+import {AnimationLViewData, NodeAnimations} from './interfaces';
 
 export interface AnimationQueue {
-  queue: Set<Function>;
+  queue: Set<VoidFunction>;
   isScheduled: boolean;
   scheduler: Function | null;
 }
@@ -33,18 +33,40 @@ export const ANIMATION_QUEUE = new InjectionToken<AnimationQueue>(
   },
 );
 
-export function addToAnimationQueue(injector: Injector, animationFns: Function | Function[]) {
+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?.detachFnQueue?.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?.detachFnQueue?.push(animationFns);
   }
   animationQueue.scheduler && animationQueue.scheduler(injector);
 }
 
+export function removeFromAnimationQueue(injector: Injector, animationData: AnimationLViewData) {
+  const animationQueue = injector.get(ANIMATION_QUEUE);
+  if (animationData.detachFnQueue) {
+    for (const animationFn of animationData.detachFnQueue) {
+      animationQueue.queue.delete(animationFn);
+    }
+    animationData.detachFnQueue = 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/animation/utils.ts

@@ -7,7 +7,7 @@
  */
 
 import {stringify} from '../util/stringify'; // Adjust imports as per actual location
-import {ANIMATIONS_DISABLED, LongestAnimation, NodeAnimations} from './interfaces';
+import {ANIMATIONS_DISABLED, LongestAnimation, NodeAnimations, RunAnimationFn} from './interfaces';
 import {INJECTOR, LView, DECLARATION_LCONTAINER, ANIMATIONS} from '../render3/interfaces/view';
 import {RuntimeError, RuntimeErrorCode} from '../errors';
 import {Renderer} from '../render3/interfaces/renderer';
@@ -255,7 +255,7 @@ export function isLongestAnimation(
 export function addAnimationToLView(
   animations: Map<number, NodeAnimations>,
   tNode: TNode,
-  fn: Function,
+  fn: RunAnimationFn,
 ) {
   const nodeAnimations = animations.get(tNode.index) ?? {animateFns: []};
   nodeAnimations.animateFns.push(fn);

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

@@ -28,6 +28,7 @@ import {
   DECLARATION_COMPONENT_VIEW,
   HEADER_OFFSET,
   HYDRATION,
+  INJECTOR,
   LView,
   TVIEW,
   TView,
@@ -48,6 +49,8 @@ import {
   removeLViewFromLContainer,
 } from '../view/container';
 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),
     );
+    clearDetachFlag(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);
+    setDetachFlag(this.lContainer, index);
     return detachExistingView<RepeaterContext<unknown>>(this.lContainer, index);
   }
   override create(index: number, value: unknown): LView<RepeaterContext<unknown>> {
@@ -570,13 +574,35 @@ function getLContainer(lView: LView, index: number): LContainer {
   return lContainer;
 }
 
-function setSkipLeaveAnimations(lContainer: LContainer, index: number): void {
+function clearDetachFlag(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).detachFnQueue &&
+    (viewToDetach[ANIMATIONS] as AnimationLViewData).detachFnQueue!.length > 0
+  ) {
+    const animations = viewToDetach[ANIMATIONS] as AnimationLViewData;
+    if (animations.detachFnQueue && animations.detachFnQueue.length > 0) {
+      const injector = viewToDetach[INJECTOR];
+      removeFromAnimationQueue(injector, animations);
+      allLeavingAnimations.delete(viewToDetach);
+      animations.detachFnQueue = undefined;
+    }
+  }
+}
+
+function setDetachFlag(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[ANIMATIONS] as AnimationLViewData;
+    animations.detachFnQueue = [];
   }
 }
 

packages/core/src/render3/list_reconciliation.ts

@@ -21,7 +21,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
@@ -50,7 +50,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));
   }
 }
 
@@ -131,6 +131,7 @@ export function reconcile<T, V>(
       // compare from the beginning
       const liveStartValue = liveCollection.at(liveStartIdx);
       const newStartValue = newCollection[liveStartIdx];
+      const newLastValue = newCollection[newEndIdx];
 
       if (ngDevMode) {
         recordDuplicateKeys(duplicateKeys!, trackByFn(liveStartIdx, newStartValue), liveStartIdx);
@@ -180,6 +181,7 @@ export function reconcile<T, V>(
       const liveStartKey = trackByFn(liveStartIdx, liveStartValue);
       const liveEndKey = trackByFn(liveEndIdx, liveEndValue);
       const newStartKey = trackByFn(liveStartIdx, newStartValue);
+
       if (Object.is(newStartKey, liveEndKey)) {
         const newEndKey = trackByFn(newEndIdx, newEndValue);
         // detect swap on both ends;

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.detachFnQueue = 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 {