fix(platform-browser): wait until animation completion before destroying renderer (#50860)

alan-agius4 · AndrewKushnir · commit a797f41d1bcf · 2023-06-27T06:27:16.000-07:00
Prior to this commit, the renderer destroy method was being called before the animation complete. This is problematic when using `REMOVE_STYLES_ON_COMPONENT_DESTROY` as it causes the styles to be removed too early. This commit, updates this destroy logic to be call the render destroy once the animations complete. This has been reported internally in: - http://b/271251353#comment12 - http://b/282004950#comment5 PR Close #50860
diff --git a/packages/animations/browser/src/render/animation_engine_next.ts b/packages/animations/browser/src/render/animation_engine_next.ts
@@ -113,4 +113,8 @@ export class AnimationEngine {
   whenRenderingDone(): Promise<any> {
     return this._transitionEngine.whenRenderingDone();
   }
+
+  afterFlushAnimationsDone(cb: VoidFunction): void {
+    this._transitionEngine.afterFlushAnimationsDone(cb);
+  }
 }
diff --git a/packages/platform-browser/animations/src/animation_renderer.ts b/packages/platform-browser/animations/src/animation_renderer.ts
@@ -138,19 +138,26 @@ export class AnimationRendererFactory implements RendererFactory2 {
 export class BaseAnimationRenderer implements Renderer2 {
   constructor(
       protected namespaceId: string, public delegate: Renderer2, public engine: AnimationEngine,
-      private _onDestroy?: () => void) {
-    this.destroyNode = this.delegate.destroyNode ? (n) => delegate.destroyNode!(n) : null;
-  }
+      private _onDestroy?: () => void) {}
 
   get data() {
     return this.delegate.data;
   }
 
-  destroyNode: ((n: any) => void)|null;
+  destroyNode(node: any): void {
+    this.delegate.destroyNode?.(node);
+  }
 
   destroy(): void {
     this.engine.destroy(this.namespaceId, this.delegate);
-    this.delegate.destroy();
+    this.engine.afterFlushAnimationsDone(() => {
+      // Call the renderer destroy method after the animations has finished as otherwise
+      // styles will be removed too early which will cause an unstyled animation.
+      queueMicrotask(() => {
+        this.delegate.destroy();
+      });
+    });
+
     this._onDestroy?.();
   }
 
diff --git a/packages/platform-browser/test/dom/dom_renderer_spec.ts b/packages/platform-browser/test/dom/dom_renderer_spec.ts
@@ -144,50 +144,50 @@ import {expect} from '@angular/platform-browser/testing/src/matchers';
     });
 
     describe('should not cleanup styles of destroyed components by default', () => {
-      it('works for components without encapsulation emulated', () => {
+      it('works for components without encapsulation emulated', async () => {
         const fixture = TestBed.createComponent(SomeAppForCleanUp);
         const compInstance = fixture.componentInstance;
         compInstance.showEmulatedComponents = true;
 
         fixture.detectChanges();
         // verify style is in DOM
-        expect(styleCount(fixture, '.emulated')).toBe(1);
+        expect(await styleCount(fixture, '.emulated')).toBe(1);
 
         // Remove a single instance of the component.
         compInstance.componentOneInstanceHidden = true;
         fixture.detectChanges();
         // Verify style is still in DOM
-        expect(styleCount(fixture, '.emulated')).toBe(1);
+        expect(await styleCount(fixture, '.emulated')).toBe(1);
 
         // Hide all instances of the component
         compInstance.componentTwoInstanceHidden = true;
         fixture.detectChanges();
 
         // Verify style is still in DOM
-        expect(styleCount(fixture, '.emulated')).toBe(1);
+        expect(await styleCount(fixture, '.emulated')).toBe(1);
       });
 
-      it('works for components without encapsulation none', () => {
+      it('works for components without encapsulation none', async () => {
         const fixture = TestBed.createComponent(SomeAppForCleanUp);
         const compInstance = fixture.componentInstance;
         compInstance.showEmulatedComponents = false;
 
         fixture.detectChanges();
         // verify style is in DOM
-        expect(styleCount(fixture, '.none')).toBe(1);
+        expect(await styleCount(fixture, '.none')).toBe(1);
 
         // Remove a single instance of the component.
         compInstance.componentOneInstanceHidden = true;
         fixture.detectChanges();
         // Verify style is still in DOM
-        expect(styleCount(fixture, '.none')).toBe(1);
+        expect(await styleCount(fixture, '.none')).toBe(1);
 
         // Hide all instances of the component
         compInstance.componentTwoInstanceHidden = true;
         fixture.detectChanges();
 
         // Verify style is still in DOM
-        expect(styleCount(fixture, '.none')).toBe(1);
+        expect(await styleCount(fixture, '.none')).toBe(1);
       });
     });
 
@@ -212,56 +212,61 @@ import {expect} from '@angular/platform-browser/testing/src/matchers';
             });
           });
 
-          it('works for components without encapsulation emulated', () => {
+          it('works for components without encapsulation emulated', async () => {
             const fixture = TestBed.createComponent(SomeAppForCleanUp);
             const compInstance = fixture.componentInstance;
             compInstance.showEmulatedComponents = true;
-
             fixture.detectChanges();
             // verify style is in DOM
-            expect(styleCount(fixture, '.emulated')).toBe(1);
+            expect(await styleCount(fixture, '.emulated')).toBe(1);
 
             // Remove a single instance of the component.
             compInstance.componentOneInstanceHidden = true;
             fixture.detectChanges();
             // Verify style is still in DOM
-            expect(styleCount(fixture, '.emulated')).toBe(1);
+            expect(await styleCount(fixture, '.emulated')).toBe(1);
 
             // Hide all instances of the component
             compInstance.componentTwoInstanceHidden = true;
             fixture.detectChanges();
 
             // Verify style is not in DOM
-            expect(styleCount(fixture, '.emulated')).toBe(0);
+            expect(await styleCount(fixture, '.emulated')).toBe(0);
           });
 
-          it('works for components without encapsulation none', () => {
+          it('works for components without encapsulation none', async () => {
             const fixture = TestBed.createComponent(SomeAppForCleanUp);
             const compInstance = fixture.componentInstance;
             compInstance.showEmulatedComponents = false;
 
             fixture.detectChanges();
             // verify style is in DOM
-            expect(styleCount(fixture, '.none')).toBe(1);
+            expect(await styleCount(fixture, '.none')).toBe(1);
 
             // Remove a single instance of the component.
             compInstance.componentOneInstanceHidden = true;
             fixture.detectChanges();
             // Verify style is still in DOM
-            expect(styleCount(fixture, '.none')).toBe(1);
+            expect(await styleCount(fixture, '.none')).toBe(1);
 
             // Hide all instances of the component
             compInstance.componentTwoInstanceHidden = true;
             fixture.detectChanges();
 
             // Verify style is not in DOM
-            expect(styleCount(fixture, '.emulated')).toBe(0);
+            expect(await styleCount(fixture, '.emulated')).toBe(0);
           });
         });
   });
 }
 
-function styleCount(fixture: ComponentFixture<unknown>, cssContentMatcher: string): number {
+async function styleCount(
+    fixture: ComponentFixture<unknown>, cssContentMatcher: string): Promise<number> {
+  // flush
+  await new Promise<void>(resolve => {
+    setTimeout(() => resolve(), 0);
+  });
+
   const html = fixture.debugElement.parent?.parent;
   const debugElements = html?.queryAll(By.css('style'));
 

Original file line number	Diff line number	Diff line change
`@@ -113,4 +113,8 @@ export class AnimationEngine {`
`113`	`113`	`whenRenderingDone(): Promise<any> {`
`114`	`114`	`return this._transitionEngine.whenRenderingDone();`
`115`	`115`	`}`
	`116`	`+`
	`117`	`+ afterFlushAnimationsDone(cb: VoidFunction): void {`
	`118`	`+ this._transitionEngine.afterFlushAnimationsDone(cb);`
	`119`	`+ }`
`116`	`120`	`}`