From b358a4a516df3f17b350e97ae13fd22e80ca6bb9 Mon Sep 17 00:00:00 2001 From: Andrew Scott Date: Tue, 27 Sep 2022 11:56:48 -0700 Subject: [PATCH] fix(router): Delay router scroll event until navigated components have rendered (#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 #24547 PR Close #47563 --- packages/router/src/provide_router.ts | 5 ++- packages/router/src/router_module.ts | 5 ++- packages/router/src/router_scroller.ts | 19 +++++++-- packages/router/test/bootstrap.spec.ts | 24 +++++------ packages/router/test/router_scroller.spec.ts | 45 +++++++++++++++----- 5 files changed, 67 insertions(+), 31 deletions(-) diff --git a/packages/router/src/provide_router.ts b/packages/router/src/provide_router.ts index c65e1b61d2aa8..945a0e69eb658 100644 --- a/packages/router/src/provide_router.ts +++ b/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); diff --git a/packages/router/src/router_module.ts b/packages/router/src/router_module.ts index f47b9e098ffff..42b066dbefb26 100644 --- a/packages/router/src/router_module.ts +++ b/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); }, }; } diff --git a/packages/router/src/router_scroller.ts b/packages/router/src/router_scroller.ts index 5fcf25eafcb5a..068a64a04330a 100644 --- a/packages/router/src/router_scroller.ts +++ b/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 */ diff --git a/packages/router/test/bootstrap.spec.ts b/packages/router/test/bootstrap.spec.ts index 59a0977611c39..111ae7099358d 100644 --- a/packages/router/test/bootstrap.spec.ts +++ b/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((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']); diff --git a/packages/router/test/router_scroller.spec.ts b/packages/router/test/router_scroller.spec.ts index 15b6a5578ae16..c55a87a3f257a 100644 --- a/packages/router/test/router_scroller.spec.ts +++ b/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(); + const events = new Subject(); const router = { 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): Promise { + 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(); + const events = new Subject(); const router = { 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};