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};