From 024e9bf54d7dcab2637dd6f653b0ebdba61014a2 Mon Sep 17 00:00:00 2001 From: Andrew Scott Date: Fri, 26 Apr 2024 12:33:13 -0700 Subject: [PATCH] refactor(core): Ensure animations are flushed before running render hooks (#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 --- .../animations/browser/src/create_engine.ts | 22 ++---------- .../src/render/animation_engine_next.ts | 9 +---- .../src/render/transition_animation_engine.ts | 8 +---- .../transition_animation_engine_spec.ts | 1 - .../core/src/application/application_ref.ts | 34 ++++++++++--------- .../scheduling/zoneless_scheduling.ts | 3 -- .../scheduling/zoneless_scheduling_impl.ts | 1 - .../test/acceptance/renderer_factory_spec.ts | 13 +++---- .../test/change_detection_scheduler_spec.ts | 2 +- .../async/src/async_animation_renderer.ts | 8 ++--- .../async/test/animation_renderer_spec.ts | 6 +--- .../animations/src/providers.ts | 2 +- .../test/animation_renderer_spec.ts | 4 +-- 13 files changed, 34 insertions(+), 79 deletions(-) diff --git a/packages/animations/browser/src/create_engine.ts b/packages/animations/browser/src/create_engine.ts index e0a991a942494..ab67368b60ff1 100644 --- 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 index 6183ebd5b6d84..eb1dd5df3377a 100644 --- 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 index 73b8ac7eb59b6..f64e5bfa3ce95 100644 --- 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 index 2fd1e42f2f51b..de787772d32c9 100644 --- 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 index 4b097b827b69f..5a1cfe65e2555 100644 --- 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(); /** @internal */ afterTick = new Subject(); + 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 index 0fede95715fab..04b5207e88940 100644 --- 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 index 369940fc9593c..d6f2b2b53cae9 100644 --- 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 index f8e288426972c..fed5b0a9f7560 100644 --- 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 index b5cc8752aecf2..a43be4141f4c3 100644 --- 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 index e0f764f05cdf4..06e8c87d4af5b 100644 --- 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 index 0fe65678a9e54..0755c0961ca17 100644 --- 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 index 1313efc5cbae8..0259222efb80d 100644 --- 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 index 6afc4cbd3a086..516617b43444b 100644 --- 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']); }); });