fix(router): Scroller should scroll as soon as change detection completes (#55105)

atscott · AndrewKushnir · commit 66ffeca2dea1 · 2024-04-30T09:19:06.000-07:00
Using `setTimeout` to delay scrolling can result in scrolling in the next frame and cause noticeable flicker. This commit scrolls as soon as the next render happens (or in `setTimeout` if a render does not happen before then). fixes #53985 PR Close #55105
diff --git a/packages/router/src/router_scroller.ts b/packages/router/src/router_scroller.ts
@@ -7,7 +7,15 @@
  */
 
 import {ViewportScroller} from '@angular/common';
-import {Injectable, InjectionToken, NgZone, OnDestroy} from '@angular/core';
+import {
+  EnvironmentInjector,
+  Injectable,
+  InjectionToken,
+  NgZone,
+  OnDestroy,
+  afterNextRender,
+  inject,
+} from '@angular/core';
 import {Unsubscribable} from 'rxjs';
 
 import {
@@ -31,6 +39,7 @@ export class RouterScroller implements OnDestroy {
   private lastSource: 'imperative' | 'popstate' | 'hashchange' | undefined = 'imperative';
   private restoredId = 0;
   private store: {[key: string]: [number, number]} = {};
+  private readonly environmentInjector = inject(EnvironmentInjector);
 
   /** @nodoc */
   constructor(
@@ -105,21 +114,32 @@ export class RouterScroller implements OnDestroy {
     routerEvent: NavigationEnd | NavigationSkipped,
     anchor: string | null,
   ): void {
-    this.zone.runOutsideAngular(() => {
-      // The scroll event needs to be delayed until after change detection. Otherwise, we may
+    this.zone.runOutsideAngular(async () => {
+      // The scroll event needs to be delayed until after change detection. Otherwise we may
       // attempt to restore the scroll position before the router outlet has fully rendered the
       // component by executing its update block of the template function.
-      setTimeout(() => {
-        this.zone.run(() => {
-          this.transitions.events.next(
-            new Scroll(
-              routerEvent,
-              this.lastSource === 'popstate' ? this.store[this.restoredId] : null,
-              anchor,
-            ),
-          );
+      await new Promise<void>((resolve) => {
+        // TODO(atscott): Attempt to remove the setTimeout in a future PR.
+        setTimeout(() => {
+          resolve();
         });
-      }, 0);
+        afterNextRender(
+          () => {
+            resolve();
+          },
+          {injector: this.environmentInjector},
+        );
+      });
+
+      this.zone.run(() => {
+        this.transitions.events.next(
+          new Scroll(
+            routerEvent,
+            this.lastSource === 'popstate' ? this.store[this.restoredId] : null,
+            anchor,
+          ),
+        );
+      });
     });
   }
 
diff --git a/packages/router/test/router_scroller.spec.ts b/packages/router/test/router_scroller.spec.ts
@@ -6,40 +6,34 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
-import {fakeAsync, tick} from '@angular/core/testing';
+import {TestBed, fakeAsync, tick} from '@angular/core/testing';
 import {DefaultUrlSerializer, Event, NavigationEnd, NavigationStart} from '@angular/router';
 import {Subject} from 'rxjs';
 import {filter, switchMap, take} from 'rxjs/operators';
 
 import {Scroll} from '../src/events';
 import {RouterScroller} from '../src/router_scroller';
+import {ApplicationRef, ɵNoopNgZone as NoopNgZone} from '@angular/core';
 
 // TODO: add tests that exercise the `withInMemoryScrolling` feature of the provideRouter function
-const fakeZone = {
-  runOutsideAngular: (fn: any) => fn(),
-  run: (fn: any) => fn(),
-};
 describe('RouterScroller', () => {
   it('defaults to disabled', () => {
     const events = new Subject<Event>();
-    const router = <any>{
-      events,
-      parseUrl: (url: any) => new DefaultUrlSerializer().parse(url),
-      triggerEvent: (e: any) => events.next(e),
-    };
-
     const viewportScroller = jasmine.createSpyObj('viewportScroller', [
       'getScrollPosition',
       'scrollToPosition',
       'scrollToAnchor',
       'setHistoryScrollRestoration',
     ]);
     setScroll(viewportScroller, 0, 0);
-    const scroller = new RouterScroller(
-      new DefaultUrlSerializer(),
-      {events} as any,
-      viewportScroller,
-      fakeZone as any,
+    const scroller = TestBed.runInInjectionContext(
+      () =>
+        new RouterScroller(
+          new DefaultUrlSerializer(),
+          {events} as any,
+          viewportScroller,
+          new NoopNgZone(),
+        ),
     );
 
     expect((scroller as any).options.scrollPositionRestoration).toBe('disabled');
@@ -226,12 +220,15 @@ describe('RouterScroller', () => {
     ]);
     setScroll(viewportScroller, 0, 0);
 
-    const scroller = new RouterScroller(
-      new DefaultUrlSerializer(),
-      transitions,
-      viewportScroller,
-      fakeZone as any,
-      {scrollPositionRestoration, anchorScrolling},
+    const scroller = TestBed.runInInjectionContext(
+      () =>
+        new RouterScroller(
+          new DefaultUrlSerializer(),
+          transitions,
+          viewportScroller,
+          new NoopNgZone(),
+          {scrollPositionRestoration, anchorScrolling},
+        ),
     );
     scroller.init();
 
