fix(router): Delay router scroll event until navigated components have rendered (angular#47563)

Currently, the scroll event is fired immediately after the
`NavigationEnd`. However, this is problematic because a change detection
has not been able to run, meaning that Angular will not yet have run the update
block of the component templates being rendered as part of the navigation.

This change delays the scroll event using a `setTimeout`, which will
allow Angular's change detection to run before the scroll restoration is
performed.

fixes angular#24547

PR Close angular#47563

Author: atscott

packages/router/src/provide_router.ts

@@ -7,7 +7,7 @@
  */
 
 import {LOCATION_INITIALIZED, ViewportScroller} from '@angular/common';
-import {APP_BOOTSTRAP_LISTENER, APP_INITIALIZER, ApplicationRef, ComponentRef, ENVIRONMENT_INITIALIZER, EnvironmentProviders, inject, InjectFlags, InjectionToken, Injector, makeEnvironmentProviders, Provider, Type} from '@angular/core';
+import {APP_BOOTSTRAP_LISTENER, APP_INITIALIZER, ApplicationRef, ComponentRef, ENVIRONMENT_INITIALIZER, EnvironmentProviders, inject, InjectFlags, InjectionToken, Injector, makeEnvironmentProviders, NgZone, Provider, Type} from '@angular/core';
 import {of, Subject} from 'rxjs';
 import {filter, map, take} from 'rxjs/operators';
 
@@ -155,7 +155,8 @@ export function withInMemoryScrolling(options: InMemoryScrollingOptions = {}):
     useFactory: () => {
       const router = inject(Router);
       const viewportScroller = inject(ViewportScroller);
-      return new RouterScroller(router, viewportScroller, options);
+      const zone = inject(NgZone);
+      return new RouterScroller(router, viewportScroller, zone, options);
     },
   }];
   return routerFeature(RouterFeatureKind.InMemoryScrollingFeature, providers);

packages/router/src/router_module.ts

@@ -7,7 +7,7 @@
  */
 
 import {HashLocationStrategy, Location, LocationStrategy, PathLocationStrategy, ViewportScroller} from '@angular/common';
-import {APP_BOOTSTRAP_LISTENER, ComponentRef, inject, Inject, InjectionToken, ModuleWithProviders, NgModule, NgProbeToken, Optional, Provider, SkipSelf, ɵRuntimeError as RuntimeError} from '@angular/core';
+import {APP_BOOTSTRAP_LISTENER, ComponentRef, inject, Inject, InjectionToken, ModuleWithProviders, NgModule, NgProbeToken, NgZone, Optional, Provider, SkipSelf, ɵRuntimeError as RuntimeError} from '@angular/core';
 
 import {EmptyOutletComponent} from './components/empty_outlet';
 import {RouterLink} from './directives/router_link';
@@ -154,11 +154,12 @@ export function provideRouterScroller(): Provider {
     useFactory: () => {
       const router = inject(Router);
       const viewportScroller = inject(ViewportScroller);
+      const zone = inject(NgZone);
       const config: ExtraOptions = inject(ROUTER_CONFIGURATION);
       if (config.scrollOffset) {
         viewportScroller.setOffset(config.scrollOffset);
       }
-      return new RouterScroller(router, viewportScroller, config);
+      return new RouterScroller(router, viewportScroller, zone, config);
     },
   };
 }

packages/router/src/router_scroller.ts

@@ -7,7 +7,7 @@
  */
 
 import {ViewportScroller} from '@angular/common';
-import {Injectable, InjectionToken, OnDestroy} from '@angular/core';
+import {Injectable, InjectionToken, NgZone, OnDestroy} from '@angular/core';
 import {Unsubscribable} from 'rxjs';
 
 import {NavigationEnd, NavigationStart, Scroll} from './events';
@@ -29,7 +29,8 @@ export class RouterScroller implements OnDestroy {
 
   constructor(
       private router: Router,
-      /** @docsNotRequired */ public readonly viewportScroller: ViewportScroller, private options: {
+      /** @docsNotRequired */ public readonly viewportScroller: ViewportScroller,
+      private readonly zone: NgZone, private options: {
         scrollPositionRestoration?: 'disabled'|'enabled'|'top',
         anchorScrolling?: 'disabled'|'enabled'
       } = {}) {
@@ -85,8 +86,18 @@ export class RouterScroller implements OnDestroy {
   }
 
   private scheduleScrollEvent(routerEvent: NavigationEnd, anchor: string|null): void {
-    this.router.triggerEvent(new Scroll(
-        routerEvent, this.lastSource === 'popstate' ? this.store[this.restoredId] : null, anchor));
+    this.zone.runOutsideAngular(() => {
+      // 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.router.triggerEvent(new Scroll(
+              routerEvent, this.lastSource === 'popstate' ? this.store[this.restoredId] : null,
+              anchor));
+        });
+      }, 0);
+    });
   }
 
   /** @nodoc */

packages/router/test/bootstrap.spec.ts

@@ -205,21 +205,13 @@ describe('bootstrap', () => {
       schemas: [CUSTOM_ELEMENTS_SCHEMA]
     })
     class TestModule {
-      constructor(router: Router) {
-        log.push('TestModule');
-        router.events.subscribe(e => log.push(e.constructor.name));
-      }
+      constructor(router: Router) {}
     }
 
     platformBrowserDynamic([]).bootstrapModule(TestModule).then(res => {
       const router = res.injector.get(Router);
       const data = router.routerState.snapshot.root.firstChild!.data;
       expect(data['test']).toEqual('test-data');
-      expect(log).toEqual([
-        'TestModule', 'NavigationStart', 'RoutesRecognized', 'GuardsCheckStart',
-        'ChildActivationStart', 'ActivationStart', 'GuardsCheckEnd', 'ResolveStart', 'ResolveEnd',
-        'RootCmp', 'ActivationEnd', 'ChildActivationEnd', 'NavigationEnd', 'Scroll'
-      ]);
       done();
     });
   });
@@ -432,6 +424,14 @@ describe('bootstrap', () => {
     class TestModule {
     }
 
+    function resolveAfter(milliseconds: number) {
+      return new Promise<void>((resolve) => {
+        setTimeout(() => {
+          resolve();
+        }, milliseconds);
+      });
+    }
+
     const res = await platformBrowserDynamic([]).bootstrapModule(TestModule);
     const router = res.injector.get(Router);
 
@@ -447,13 +447,16 @@ describe('bootstrap', () => {
     expect(window.pageYOffset).toEqual(3000);
 
     await router.navigateByUrl('/cc');
+    await resolveAfter(100);
     expect(window.pageYOffset).toEqual(0);
 
     await router.navigateByUrl('/aa#marker2');
+    await resolveAfter(100);
     expect(window.pageYOffset).toBeGreaterThanOrEqual(5900);
     expect(window.pageYOffset).toBeLessThan(6000);  // offset
 
     await router.navigateByUrl('/aa#marker3');
+    await resolveAfter(100);
     expect(window.pageYOffset).toBeGreaterThanOrEqual(8900);
     expect(window.pageYOffset).toBeLessThan(9000);
   });
@@ -506,9 +509,6 @@ describe('bootstrap', () => {
     (async () => {
       const res = await platformBrowserDynamic([]).bootstrapModule(TestModule);
       const router = res.injector.get(Router);
-      router.events.subscribe(() => {
-        expect(router.getCurrentNavigation()?.id).toBeDefined();
-      });
       router.events.subscribe(async (e) => {
         if (e instanceof NavigationEnd && e.url === '/b') {
           await router.navigate(['a']);

packages/router/test/router_scroller.spec.ts

@@ -7,17 +7,21 @@
  */
 
 import {fakeAsync, tick} from '@angular/core/testing';
-import {DefaultUrlSerializer, NavigationEnd, NavigationStart, RouterEvent} from '@angular/router';
+import {DefaultUrlSerializer, Event, NavigationEnd, NavigationStart} from '@angular/router';
 import {Subject} from 'rxjs';
-import {filter, switchMap} from 'rxjs/operators';
+import {filter, switchMap, take} from 'rxjs/operators';
 
 import {Scroll} from '../src/events';
 import {RouterScroller} from '../src/router_scroller';
 
 // 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<RouterEvent>();
+    const events = new Subject<Event>();
     const router = <any>{
       events,
       parseUrl: (url: any) => new DefaultUrlSerializer().parse(url),
@@ -28,87 +32,103 @@ describe('RouterScroller', () => {
         'viewportScroller',
         ['getScrollPosition', 'scrollToPosition', 'scrollToAnchor', 'setHistoryScrollRestoration']);
     setScroll(viewportScroller, 0, 0);
-    const scroller = new RouterScroller(router, router);
+    const scroller = new RouterScroller(router, router, fakeZone as any);
 
     expect((scroller as any).options.scrollPositionRestoration).toBe('disabled');
     expect((scroller as any).options.anchorScrolling).toBe('disabled');
   });
 
+  function nextScrollEvent(events: Subject<Event>): Promise<Scroll> {
+    return events.pipe(filter((e): e is Scroll => e instanceof Scroll), take(1)).toPromise();
+  }
+
   describe('scroll to top', () => {
-    it('should scroll to the top', () => {
+    it('should scroll to the top', async () => {
       const {events, viewportScroller} =
           createRouterScroller({scrollPositionRestoration: 'top', anchorScrolling: 'disabled'});
 
       events.next(new NavigationStart(1, '/a'));
       events.next(new NavigationEnd(1, '/a', '/a'));
+      await nextScrollEvent(events);
       expect(viewportScroller.scrollToPosition).toHaveBeenCalledWith([0, 0]);
 
       events.next(new NavigationStart(2, '/a'));
       events.next(new NavigationEnd(2, '/b', '/b'));
+      await nextScrollEvent(events);
       expect(viewportScroller.scrollToPosition).toHaveBeenCalledWith([0, 0]);
 
       events.next(new NavigationStart(3, '/a', 'popstate'));
       events.next(new NavigationEnd(3, '/a', '/a'));
+      await nextScrollEvent(events);
       expect(viewportScroller.scrollToPosition).toHaveBeenCalledWith([0, 0]);
     });
   });
 
   describe('scroll to the stored position', () => {
-    it('should scroll to the stored position on popstate', () => {
+    it('should scroll to the stored position on popstate', async () => {
       const {events, viewportScroller} =
           createRouterScroller({scrollPositionRestoration: 'enabled', anchorScrolling: 'disabled'});
 
       events.next(new NavigationStart(1, '/a'));
       events.next(new NavigationEnd(1, '/a', '/a'));
+      await nextScrollEvent(events);
       setScroll(viewportScroller, 10, 100);
       expect(viewportScroller.scrollToPosition).toHaveBeenCalledWith([0, 0]);
 
       events.next(new NavigationStart(2, '/b'));
       events.next(new NavigationEnd(2, '/b', '/b'));
+      await nextScrollEvent(events);
       setScroll(viewportScroller, 20, 200);
       expect(viewportScroller.scrollToPosition).toHaveBeenCalledWith([0, 0]);
 
       events.next(new NavigationStart(3, '/a', 'popstate', {navigationId: 1}));
       events.next(new NavigationEnd(3, '/a', '/a'));
+      await nextScrollEvent(events);
       expect(viewportScroller.scrollToPosition).toHaveBeenCalledWith([10, 100]);
     });
   });
 
   describe('anchor scrolling', () => {
-    it('should work (scrollPositionRestoration is disabled)', () => {
+    it('should work (scrollPositionRestoration is disabled)', async () => {
       const {events, viewportScroller} =
           createRouterScroller({scrollPositionRestoration: 'disabled', anchorScrolling: 'enabled'});
       events.next(new NavigationStart(1, '/a#anchor'));
       events.next(new NavigationEnd(1, '/a#anchor', '/a#anchor'));
+      await nextScrollEvent(events);
       expect(viewportScroller.scrollToAnchor).toHaveBeenCalledWith('anchor');
 
       events.next(new NavigationStart(2, '/a#anchor2'));
       events.next(new NavigationEnd(2, '/a#anchor2', '/a#anchor2'));
+      await nextScrollEvent(events);
       expect(viewportScroller.scrollToAnchor).toHaveBeenCalledWith('anchor2');
       viewportScroller.scrollToAnchor.calls.reset();
 
       // we never scroll to anchor when navigating back.
       events.next(new NavigationStart(3, '/a#anchor', 'popstate'));
       events.next(new NavigationEnd(3, '/a#anchor', '/a#anchor'));
+      await nextScrollEvent(events);
       expect(viewportScroller.scrollToAnchor).not.toHaveBeenCalled();
       expect(viewportScroller.scrollToPosition).not.toHaveBeenCalled();
     });
 
-    it('should work (scrollPositionRestoration is enabled)', () => {
+    it('should work (scrollPositionRestoration is enabled)', async () => {
       const {events, viewportScroller} =
           createRouterScroller({scrollPositionRestoration: 'enabled', anchorScrolling: 'enabled'});
       events.next(new NavigationStart(1, '/a#anchor'));
       events.next(new NavigationEnd(1, '/a#anchor', '/a#anchor'));
+      await nextScrollEvent(events);
       expect(viewportScroller.scrollToAnchor).toHaveBeenCalledWith('anchor');
 
       events.next(new NavigationStart(2, '/a#anchor2'));
       events.next(new NavigationEnd(2, '/a#anchor2', '/a#anchor2'));
+      await nextScrollEvent(events);
       expect(viewportScroller.scrollToAnchor).toHaveBeenCalledWith('anchor2');
       viewportScroller.scrollToAnchor.calls.reset();
 
       // we never scroll to anchor when navigating back
       events.next(new NavigationStart(3, '/a#anchor', 'popstate', {navigationId: 1}));
       events.next(new NavigationEnd(3, '/a#anchor', '/a#anchor'));
+      await nextScrollEvent(events);
       expect(viewportScroller.scrollToAnchor).not.toHaveBeenCalled();
       expect(viewportScroller.scrollToPosition).toHaveBeenCalledWith([0, 0]);
     });
@@ -135,14 +155,17 @@ describe('RouterScroller', () => {
 
          events.next(new NavigationStart(1, '/a'));
          events.next(new NavigationEnd(1, '/a', '/a'));
+         tick();
          setScroll(viewportScroller, 10, 100);
 
          events.next(new NavigationStart(2, '/b'));
          events.next(new NavigationEnd(2, '/b', '/b'));
+         tick();
          setScroll(viewportScroller, 20, 200);
 
          events.next(new NavigationStart(3, '/c'));
          events.next(new NavigationEnd(3, '/c', '/c'));
+         tick();
          setScroll(viewportScroller, 30, 300);
 
          events.next(new NavigationStart(4, '/a', 'popstate', {navigationId: 1}));
@@ -164,7 +187,7 @@ describe('RouterScroller', () => {
     scrollPositionRestoration: 'disabled'|'enabled'|'top',
     anchorScrolling: 'disabled'|'enabled'
   }) {
-    const events = new Subject<RouterEvent>();
+    const events = new Subject<Event>();
     const router = <any>{
       events,
       parseUrl: (url: any) => new DefaultUrlSerializer().parse(url),
@@ -176,8 +199,8 @@ describe('RouterScroller', () => {
         ['getScrollPosition', 'scrollToPosition', 'scrollToAnchor', 'setHistoryScrollRestoration']);
     setScroll(viewportScroller, 0, 0);
 
-    const scroller =
-        new RouterScroller(router, viewportScroller, {scrollPositionRestoration, anchorScrolling});
+    const scroller = new RouterScroller(
+        router, viewportScroller, fakeZone as any, {scrollPositionRestoration, anchorScrolling});
     scroller.init();
 
     return {events, viewportScroller, router};