refactor(core): Ensure animations are flushed before running render h…

…ooks (#55564) This commit ensures we flush animations by calling renderFactory begin/end in cases where the ApplicationRef._tick happens in a mode that skips straight to the render hooks. PR Close #55564
angular · Apr 30, 2024 · 024e9bf · 024e9bf
1 parent 3312727
commit 024e9bf
Show file tree

Hide file tree

Showing 13 changed files with 34 additions and 79 deletions.
diff --git a/packages/animations/browser/src/create_engine.ts b/packages/animations/browser/src/create_engine.ts
@@ -6,33 +6,17 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
-import {ɵChangeDetectionScheduler as ChangeDetectionScheduler} from '@angular/core';
-
 import {NoopAnimationStyleNormalizer} from './dsl/style_normalization/animation_style_normalizer';
 import {WebAnimationsStyleNormalizer} from './dsl/style_normalization/web_animations_style_normalizer';
 import {NoopAnimationDriver} from './render/animation_driver';
 import {AnimationEngine} from './render/animation_engine_next';
 import {WebAnimationsDriver} from './render/web_animations/web_animations_driver';
 
-export function createEngine(
-  type: 'animations' | 'noop',
-  doc: Document,
-  scheduler: ChangeDetectionScheduler | null,
-): AnimationEngine {
+export function createEngine(type: 'animations' | 'noop', doc: Document): AnimationEngine {
   // TODO: find a way to make this tree shakable.
   if (type === 'noop') {
-    return new AnimationEngine(
-      doc,
-      new NoopAnimationDriver(),
-      new NoopAnimationStyleNormalizer(),
-      scheduler,
-    );
+    return new AnimationEngine(doc, new NoopAnimationDriver(), new NoopAnimationStyleNormalizer());
   }
 
-  return new AnimationEngine(
-    doc,
-    new WebAnimationsDriver(),
-    new WebAnimationsStyleNormalizer(),
-    scheduler,
-  );
+  return new AnimationEngine(doc, new WebAnimationsDriver(), new WebAnimationsStyleNormalizer());
 }
diff --git a/packages/animations/browser/src/render/animation_engine_next.ts b/packages/animations/browser/src/render/animation_engine_next.ts
@@ -6,7 +6,6 @@
  * found in the LICENSE file at https://angular.io/license
  */
 import {AnimationMetadata, AnimationPlayer, AnimationTriggerMetadata} from '@angular/animations';
-import {ɵChangeDetectionScheduler as ChangeDetectionScheduler} from '@angular/core';
 
 import {TriggerAst} from '../dsl/animation_ast';
 import {buildAnimationAst} from '../dsl/animation_ast_builder';
@@ -33,14 +32,8 @@ export class AnimationEngine {
     doc: Document,
     private _driver: AnimationDriver,
     private _normalizer: AnimationStyleNormalizer,
-    scheduler: ChangeDetectionScheduler | null,
   ) {
-    this._transitionEngine = new TransitionAnimationEngine(
-      doc.body,
-      _driver,
-      _normalizer,
-      scheduler,
-    );
+    this._transitionEngine = new TransitionAnimationEngine(doc.body, _driver, _normalizer);
     this._timelineEngine = new TimelineAnimationEngine(doc.body, _driver, _normalizer);
 
     this._transitionEngine.onRemovalComplete = (element: any, context: any) =>

diff --git a/packages/animations/browser/src/render/transition_animation_engine.ts b/packages/animations/browser/src/render/transition_animation_engine.ts
@@ -14,11 +14,7 @@ import {
   ɵPRE_STYLE as PRE_STYLE,
   ɵStyleDataMap,
 } from '@angular/animations';
-import {
-  ɵChangeDetectionScheduler as ChangeDetectionScheduler,
-  ɵNotificationSource as NotificationSource,
-  ɵWritable as Writable,
-} from '@angular/core';
+import {ɵWritable as Writable} from '@angular/core';
 
 import {AnimationTimelineInstruction} from '../dsl/animation_timeline_instruction';
 import {AnimationTransitionFactory} from '../dsl/animation_transition_factory';
@@ -625,7 +621,6 @@ export class TransitionAnimationEngine {
     public bodyNode: any,
     public driver: AnimationDriver,
     private _normalizer: AnimationStyleNormalizer,
-    private readonly scheduler: ChangeDetectionScheduler | null,
   ) {}
 
   get queuedPlayers(): TransitionAnimationPlayer[] {
@@ -817,7 +812,6 @@ export class TransitionAnimationEngine {
 
   removeNode(namespaceId: string, element: any, context: any): void {
     if (isElementNode(element)) {
-      this.scheduler?.notify(NotificationSource.AnimationQueuedNodeRemoval);
       const ns = namespaceId ? this._fetchNamespace(namespaceId) : null;
       if (ns) {
         ns.removeNode(element, context);

diff --git a/packages/animations/browser/test/render/transition_animation_engine_spec.ts b/packages/animations/browser/test/render/transition_animation_engine_spec.ts
@@ -57,7 +57,6 @@ const DEFAULT_NAMESPACE_ID = 'id';
         getBodyNode(),
         driver,
         normalizer || new NoopAnimationStyleNormalizer(),
-        null,
       );
       engine.createNamespace(DEFAULT_NAMESPACE_ID, element);
       return engine;

diff --git a/packages/core/src/application/application_ref.ts b/packages/core/src/application/application_ref.ts
@@ -21,7 +21,7 @@ import {inject} from '../di';
 import {Injectable} from '../di/injectable';
 import {InjectionToken} from '../di/injection_token';
 import {Injector} from '../di/injector';
-import {EnvironmentInjector} from '../di/r3_injector';
+import {EnvironmentInjector, type R3Injector} from '../di/r3_injector';
 import {ErrorHandler, INTERNAL_APPLICATION_ERROR_HANDLER} from '../error_handler';
 import {formatRuntimeError, RuntimeError, RuntimeErrorCode} from '../errors';
 import {Type} from '../interface/type';
@@ -30,6 +30,7 @@ import {ComponentFactoryResolver} from '../linker/component_factory_resolver';
 import {NgModuleRef} from '../linker/ng_module_factory';
 import {ViewRef} from '../linker/view_ref';
 import {PendingTasks} from '../pending_tasks';
+import {RendererFactory2} from '../render/api';
 import {AfterRenderEventManager} from '../render3/after_render_hooks';
 import {ComponentFactory as R3ComponentFactory} from '../render3/component_ref';
 import {isStandalone} from '../render3/definition';
@@ -311,6 +312,9 @@ export class ApplicationRef {
   private beforeRender = new Subject<boolean>();
   /** @internal */
   afterTick = new Subject<void>();
+  private get allViews() {
+    return [...this.externalTestViews.keys(), ...this._views];
+  }
 
   /**
    * Indicates whether this instance was destroyed.
@@ -564,6 +568,11 @@ export class ApplicationRef {
   }
 
   private detectChangesInAttachedViews(refreshViews: boolean) {
+    let rendererFactory: RendererFactory2 | null = null;
+    if (!(this._injector as R3Injector).destroyed) {
+      rendererFactory = this._injector.get(RendererFactory2, null, {optional: true});
+    }
+
     let runs = 0;
     const afterRenderEffectManager = this.afterRenderEffectManager;
     while (runs < MAXIMUM_REFRESH_RERUNS) {
@@ -578,28 +587,25 @@ export class ApplicationRef {
             this.zonelessEnabled,
           );
         }
+      } else {
+        // If we skipped refreshing views above, there might still be unflushed animations
+        // because we never called `detectChangesInternal` on the views.
+        rendererFactory?.begin?.();
+        rendererFactory?.end?.();
       }
       runs++;
 
       afterRenderEffectManager.executeInternalCallbacks();
       // If we have a newly dirty view after running internal callbacks, recheck the views again
       // before running user-provided callbacks
-      if (
-        [...this.externalTestViews.keys(), ...this._views].some(({_lView}) =>
-          requiresRefreshOrTraversal(_lView),
-        )
-      ) {
+      if (this.allViews.some(({_lView}) => requiresRefreshOrTraversal(_lView))) {
         continue;
       }
 
       afterRenderEffectManager.execute();
       // If after running all afterRender callbacks we have no more views that need to be refreshed,
       // we can break out of the loop
-      if (
-        ![...this.externalTestViews.keys(), ...this._views].some(({_lView}) =>
-          requiresRefreshOrTraversal(_lView),
-        )
-      ) {
+      if (!this.allViews.some(({_lView}) => requiresRefreshOrTraversal(_lView))) {
         break;
       }
     }
@@ -701,11 +707,7 @@ export class ApplicationRef {
       );
     }
 
-    // This is a temporary type to represent an instance of an R3Injector, which can be destroyed.
-    // The type will be replaced with a different one once destroyable injector type is available.
-    type DestroyableInjector = Injector & {destroy?: Function; destroyed?: boolean};
-
-    const injector = this._injector as DestroyableInjector;
+    const injector = this._injector as R3Injector;
 
     // Check that this injector instance supports destroy operation.
     if (injector.destroy && !injector.destroyed) {

diff --git a/packages/core/src/change_detection/scheduling/zoneless_scheduling.ts b/packages/core/src/change_detection/scheduling/zoneless_scheduling.ts
@@ -21,9 +21,6 @@ export const enum NotificationSource {
   DebugApplyChanges,
   // ChangeDetectorRef.markForCheck indicates the component is dirty/needs to refresh.
   MarkForCheck,
-  // Node removal is queued in animation code and needs change detection to flush.
-  // TODO(atscott): We should not have to refresh views in order to flush animations.
-  AnimationQueuedNodeRemoval,
 
   // Bound listener callbacks execute and can update state without causing other notifications from
   // above.

diff --git a/packages/core/src/change_detection/scheduling/zoneless_scheduling_impl.ts b/packages/core/src/change_detection/scheduling/zoneless_scheduling_impl.ts
@@ -120,7 +120,6 @@ export class ChangeDetectionSchedulerImpl implements ChangeDetectionScheduler {
       case NotificationSource.MarkAncestorsForTraversal:
       case NotificationSource.MarkForCheck:
       case NotificationSource.Listener:
-      case NotificationSource.AnimationQueuedNodeRemoval:
       case NotificationSource.SetInput: {
         this.shouldRefreshViews = true;
         break;

diff --git a/packages/core/test/acceptance/renderer_factory_spec.ts b/packages/core/test/acceptance/renderer_factory_spec.ts
@@ -98,7 +98,7 @@ describe('renderer factory lifecycle', () => {
 
   it('should work with a component', () => {
     const fixture = TestBed.createComponent(SomeComponent);
-    fixture.detectChanges();
+    fixture.componentRef.changeDetectorRef.detectChanges();
     expect(logs).toEqual([
       'create',
       'create',
@@ -108,14 +108,14 @@ describe('renderer factory lifecycle', () => {
       'end',
     ]);
     logs = [];
-    fixture.detectChanges();
+    fixture.componentRef.changeDetectorRef.detectChanges();
     expect(logs).toEqual(['begin', 'some_component update', 'end']);
   });
 
   it('should work with a component which throws', () => {
     expect(() => {
       const fixture = TestBed.createComponent(SomeComponentWhichThrows);
-      fixture.detectChanges();
+      fixture.componentRef.changeDetectorRef.detectChanges();
     }).toThrow();
     expect(logs).toEqual(['create', 'create', 'begin', 'end']);
   });
@@ -377,12 +377,7 @@ function getAnimationRendererFactory2(document: Document): RendererFactory2 {
   const fakeNgZone: NgZone = new NoopNgZone();
   return new ɵAnimationRendererFactory(
     getRendererFactory2(document),
-    new ɵAnimationEngine(
-      document,
-      new MockAnimationDriver(),
-      new ɵNoopAnimationStyleNormalizer(),
-      null,
-    ),
+    new ɵAnimationEngine(document, new MockAnimationDriver(), new ɵNoopAnimationStyleNormalizer()),
     fakeNgZone,
   );
 }

diff --git a/packages/core/test/change_detection_scheduler_spec.ts b/packages/core/test/change_detection_scheduler_spec.ts
@@ -366,7 +366,7 @@ describe('Angular with zoneless enabled', () => {
 
       const component2 = createComponent(DynamicCmp, {environmentInjector});
       // TODO(atscott): Only needed because renderFactory will not run if ApplicationRef has no
-      // views This should likely be fixed in ApplicationRef
+      // views. This should likely be fixed in ApplicationRef
       appRef.attachView(component2.hostView);
       appRef.detachView(component.hostView);
       // DOM is not synchronously removed because change detection hasn't run

diff --git a/packages/platform-browser/animations/async/src/async_animation_renderer.ts b/packages/platform-browser/animations/async/src/async_animation_renderer.ts
@@ -45,11 +45,7 @@ export class AsyncAnimationRendererFactory implements OnDestroy, RendererFactory
     private zone: NgZone,
     private animationType: 'animations' | 'noop',
     private moduleImpl?: Promise<{
-      ɵcreateEngine: (
-        type: 'animations' | 'noop',
-        doc: Document,
-        scheduler: ChangeDetectionScheduler | null,
-      ) => AnimationEngine;
+      ɵcreateEngine: (type: 'animations' | 'noop', doc: Document) => AnimationEngine;
       ɵAnimationRendererFactory: typeof AnimationRendererFactory;
     }>,
   ) {}
@@ -84,7 +80,7 @@ export class AsyncAnimationRendererFactory implements OnDestroy, RendererFactory
       .then(({ɵcreateEngine, ɵAnimationRendererFactory}) => {
         // We can't create the renderer yet because we might need the hostElement and the type
         // Both are provided in createRenderer().
-        this._engine = ɵcreateEngine(this.animationType, this.doc, this.scheduler);
+        this._engine = ɵcreateEngine(this.animationType, this.doc);
         const rendererFactory = new ɵAnimationRendererFactory(
           this.delegate,
           this._engine,

diff --git a/packages/platform-browser/animations/async/test/animation_renderer_spec.ts b/packages/platform-browser/animations/async/test/animation_renderer_spec.ts
@@ -64,11 +64,7 @@ type AnimationBrowserModule = typeof import('@angular/animations/browser');
               engine: MockAnimationEngine,
             ) => {
               const animationModule = {
-                ɵcreateEngine: (
-                  _: 'animations' | 'noop',
-                  _2: Document,
-                  _3: ChangeDetectionScheduler | null,
-                ): AnimationEngine => engine,
+                ɵcreateEngine: (_: 'animations' | 'noop', _2: Document): AnimationEngine => engine,
                 ɵAnimationEngine: MockAnimationEngine as any,
                 ɵAnimationRenderer: AnimationRenderer,
                 ɵBaseAnimationRenderer: BaseAnimationRenderer,

diff --git a/packages/platform-browser/animations/src/providers.ts b/packages/platform-browser/animations/src/providers.ts
@@ -39,7 +39,7 @@ export class InjectableAnimationEngine extends AnimationEngine implements OnDest
     driver: AnimationDriver,
     normalizer: AnimationStyleNormalizer,
   ) {
-    super(doc, driver, normalizer, inject(ChangeDetectionScheduler, {optional: true}));
+    super(doc, driver, normalizer);
   }
 
   ngOnDestroy(): void {

diff --git a/packages/platform-browser/animations/test/animation_renderer_spec.ts b/packages/platform-browser/animations/test/animation_renderer_spec.ts
@@ -351,11 +351,11 @@ import {el} from '../../testing/src/browser_util';
       const cmp = fixture.componentInstance;
 
       renderer.log = [];
-      fixture.detectChanges();
+      fixture.changeDetectorRef.detectChanges();
       expect(renderer.log).toEqual(['begin', 'end']);
 
       renderer.log = [];
-      fixture.detectChanges();
+      fixture.changeDetectorRef.detectChanges();
       expect(renderer.log).toEqual(['begin', 'end']);
     });
   });