fix(core): prevent animations renderer from impacting animate.leave (#63921)

thePunderWoman · thePunderWoman · commit e78451cf8a48 · 2025-09-19T21:26:20.000Z
This adds an optional flag to the renderer on `removeChild` called `requireSynchronousElementRemoval`, which can tell any downstream renderer that elements need to be removed synchronously. This gets passed down to the legacy animation renderer to ensure that any elements that set this flag aren't impacted by that renderers changes to timing. fixes: #63893 PR Close #63921
diff --git a/goldens/public-api/core/index.api.md b/goldens/public-api/core/index.api.md
@@ -1579,7 +1579,7 @@ export abstract class Renderer2 {
     abstract nextSibling(node: any): any;
     abstract parentNode(node: any): any;
     abstract removeAttribute(el: any, name: string, namespace?: string | null): void;
-    abstract removeChild(parent: any, oldChild: any, isHostElement?: boolean): void;
+    abstract removeChild(parent: any, oldChild: any, isHostElement?: boolean, requireSynchronousElementRemoval?: boolean): void;
     abstract removeClass(el: any, name: string): void;
     abstract removeStyle(el: any, style: string, flags?: RendererStyleFlags2): void;
     abstract selectRootElement(selectorOrNode: string | any, preserveContent?: boolean): any;
diff --git a/packages/animations/browser/src/render/renderer.ts b/packages/animations/browser/src/render/renderer.ts
@@ -78,7 +78,20 @@ export class BaseAnimationRenderer implements Renderer2 {
     this.engine.onInsert(this.namespaceId, newChild, parent, isMove);
   }
 
-  removeChild(parent: any, oldChild: any, isHostElement?: boolean): void {
+  // TODO(thePunderWoman): remove the requireSynchronousElementRemoval flag after the
+  // animations package has been fully deleted post v23.
+  removeChild(
+    parent: any,
+    oldChild: any,
+    isHostElement?: boolean,
+    requireSynchronousElementRemoval?: boolean,
+  ): void {
+    // Elements using the new `animate.leave` API require synchronous removal and should
+    // skip the rest of the legacy animation behaviors.
+    if (requireSynchronousElementRemoval) {
+      this.delegate.removeChild(parent, oldChild, isHostElement);
+      return;
+    }
     // Prior to the changes in #57203, this method wasn't being called at all by `core` if the child
     // doesn't have a parent. There appears to be some animation-specific downstream logic that
     // depends on the null check happening before the animation engine. This check keeps the old
diff --git a/packages/core/src/render/api.ts b/packages/core/src/render/api.ts
@@ -125,9 +125,15 @@ export abstract class Renderer2 {
    * @param parent The parent node.
    * @param oldChild The child node to remove.
    * @param isHostElement Optionally signal to the renderer whether this element is a host element
-   * or not
+   * @param requireSynchronousElementRemoval Optionally signal to the renderer whether this element
+   * needs synchronous removal
    */
-  abstract removeChild(parent: any, oldChild: any, isHostElement?: boolean): void;
+  abstract removeChild(
+    parent: any,
+    oldChild: any,
+    isHostElement?: boolean,
+    requireSynchronousElementRemoval?: boolean,
+  ): void;
   /**
    * Implement this callback to prepare an element to be bootstrapped
    * as a root element, and return the element instance.
diff --git a/packages/core/src/render3/dom_node_manipulation.ts b/packages/core/src/render3/dom_node_manipulation.ts
@@ -81,9 +81,16 @@ export function nativeAppendOrInsertBefore(
  * @param renderer A renderer to be used
  * @param rNode The native node that should be removed
  * @param isHostElement A flag indicating if a node to be removed is a host of a component.
+ * @param requireSynchronousElementRemoval A flag indicating if a node requires synchronous
+ * removal from the DOM.
  */
-export function nativeRemoveNode(renderer: Renderer, rNode: RNode, isHostElement?: boolean): void {
-  renderer.removeChild(null, rNode, isHostElement);
+export function nativeRemoveNode(
+  renderer: Renderer,
+  rNode: RNode,
+  isHostElement?: boolean,
+  requireSynchronousElementRemoval?: boolean,
+): void {
+  renderer.removeChild(null, rNode, isHostElement, requireSynchronousElementRemoval);
 }
 
 /**
diff --git a/packages/core/src/render3/interfaces/renderer.ts b/packages/core/src/render3/interfaces/renderer.ts
@@ -44,7 +44,13 @@ export interface Renderer {
   destroyNode?: ((node: RNode) => void) | null;
   appendChild(parent: RElement, newChild: RNode): void;
   insertBefore(parent: RNode, newChild: RNode, refChild: RNode | null, isMove?: boolean): void;
-  removeChild(parent: RElement | null, oldChild: RNode, isHostElement?: boolean): void;
+  // TODO(thePunderWoman): remove the requireSynchronousElementRemoval flag once the animations package has been deleted after v23.
+  removeChild(
+    parent: RElement | null,
+    oldChild: RNode,
+    isHostElement?: boolean,
+    requireSynchronousElementRemoval?: boolean,
+  ): void;
   selectRootElement(selectorOrNode: string | any, preserveContent?: boolean): RElement;
 
   parentNode(node: RNode): RElement | null;
diff --git a/packages/core/src/render3/node_manipulation.ts b/packages/core/src/render3/node_manipulation.ts
@@ -139,8 +139,11 @@ function applyToElementOrContainer(
     } else if (action === WalkTNodeTreeAction.Insert && parent !== null) {
       nativeInsertBefore(renderer, parent, rNode, beforeNode || null, true);
     } else if (action === WalkTNodeTreeAction.Detach) {
-      runLeaveAnimationsWithCallback(parentLView, () => {
-        nativeRemoveNode(renderer, rNode, isComponent);
+      runLeaveAnimationsWithCallback(parentLView, (nodeHasLeaveAnimations: boolean) => {
+        // the nodeHasLeaveAnimations indicates to the renderer that the element needs to
+        // be removed synchronously and sets the requireSynchronousElementRemoval flag in
+        // the renderer.
+        nativeRemoveNode(renderer, rNode, isComponent, nodeHasLeaveAnimations);
       });
     } else if (action === WalkTNodeTreeAction.Destroy) {
       runLeaveAnimationsWithCallback(parentLView, () => {
@@ -350,6 +353,10 @@ function cleanUpView(tView: TView, lView: LView): void {
   }
 }
 
+function hasLeaveAnimations(lView: LView | undefined): boolean {
+  return lView !== undefined && lView[ANIMATIONS] !== null && lView[ANIMATIONS].leave !== undefined;
+}
+
 function runLeaveAnimationsWithCallback(lView: LView | undefined, callback: Function) {
   if (lView && lView[ANIMATIONS] && lView[ANIMATIONS].leave) {
     if (lView[ANIMATIONS].skipLeaveAnimations) {
@@ -375,11 +382,11 @@ function runAfterLeaveAnimations(lView: LView | undefined, callback: Function) {
         lView[ANIMATIONS].running = undefined;
       }
       allLeavingAnimations.delete(lView);
-      callback();
+      callback(true);
     });
     return;
   }
-  callback();
+  callback(false);
 }
 
 /** Removes listeners and unsubscribes from output subscriptions */
diff --git a/packages/core/test/acceptance/animation_spec.ts b/packages/core/test/acceptance/animation_spec.ts
@@ -23,8 +23,8 @@ import {fakeAsync, TestBed, tick} from '@angular/core/testing';
 import {By} from '@angular/platform-browser';
 import {isNode} from '@angular/private/testing';
 import {tickAnimationFrames} from '../animation_utils/tick_animation_frames';
-import {NoopAnimationsModule} from '@angular/platform-browser/animations';
 import {BrowserTestingModule, platformBrowserTesting} from '@angular/platform-browser/testing';
+import {NoopAnimationsModule} from '@angular/platform-browser/animations';
 
 @NgModule({
   providers: [provideZonelessChangeDetection()],
@@ -37,11 +37,19 @@ describe('Animation', () => {
     return;
   }
 
-  beforeAll(() => {
+  beforeEach(() => {
     TestBed.resetTestEnvironment();
     TestBed.initTestEnvironment([BrowserTestingModule, TestModule], platformBrowserTesting());
   });
 
+  afterEach(() => {
+    TestBed.resetTestEnvironment();
+    TestBed.initTestEnvironment(
+      [BrowserTestingModule, NoopAnimationsModule, TestModule],
+      platformBrowserTesting(),
+    );
+  });
+
   afterAll(() => {
     TestBed.resetTestEnvironment();
     TestBed.initTestEnvironment(
@@ -709,6 +717,70 @@ describe('Animation', () => {
       expect(fixture.nativeElement.outerHTML).not.toContain('slide-out fade ');
       expect(fixture.debugElement.query(By.css('child-cmp'))).toBeNull();
     }));
+
+    describe('legacy animations compatibility', () => {
+      beforeAll(() => {
+        TestBed.resetTestEnvironment();
+        TestBed.initTestEnvironment(
+          [BrowserTestingModule, NoopAnimationsModule, TestModule],
+          platformBrowserTesting(),
+        );
+      });
+
+      const styles = `
+      .fade {
+        animation: fade-out 1ms;
+      }
+      @keyframes fade-out {
+        from {
+          opacity: 1;
+        }
+        to {
+          opacity: 0;
+        }
+      }
+      `;
+
+      it('should have the same exact timing when AnimationsModule is present', fakeAsync(() => {
+        const logSpy = jasmine.createSpy('logSpy');
+        @Component({
+          selector: 'test-cmp',
+          styles: styles,
+          template:
+            '<div>@if (show()) {<p animate.leave="fade" (animationend)="logMe($event)">I should fade</p>}</div>',
+          encapsulation: ViewEncapsulation.None,
+        })
+        class TestComponent {
+          show = signal(true);
+
+          logMe(event: AnimationEvent) {
+            logSpy();
+          }
+        }
+
+        TestBed.configureTestingModule({animationsEnabled: true});
+
+        const fixture = TestBed.createComponent(TestComponent);
+        const cmp = fixture.componentInstance;
+        fixture.detectChanges();
+        const paragragh = fixture.debugElement.query(By.css('p'));
+
+        expect(fixture.nativeElement.outerHTML).not.toContain('class="fade"');
+        cmp.show.set(false);
+        fixture.detectChanges();
+        tickAnimationFrames(1);
+        expect(cmp.show()).toBeFalsy();
+        fixture.detectChanges();
+        expect(fixture.nativeElement.outerHTML).toContain('class="fade"');
+        fixture.detectChanges();
+        paragragh.nativeElement.dispatchEvent(
+          new AnimationEvent('animationend', {animationName: 'fade-out'}),
+        );
+        tick();
+        expect(fixture.nativeElement.outerHTML).not.toContain('class="fade"');
+        expect(logSpy).toHaveBeenCalled();
+      }));
+    });
   });
 
   describe('animate.enter', () => {
