Skip to content

Commit

Permalink
fix(router): Delay router scroll event until navigated components hav…
Browse files Browse the repository at this point in the history 
…e 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
  • Loading branch information
@atscott @dylhunn
atscott authored and dylhunn committed Oct 19, 2022
1 parent 3d5df57 commit 79e9e8a
Show file tree
Hide file tree
Showing 5 changed files with 67 additions and 31 deletions.
5 changes: 3 additions & 2 deletions packages/router/src/provide_router.ts
View file
Expand Up @@ -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';

Expand Down Expand Up @@ -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);
Expand Down
5 changes: 3 additions & 2 deletions packages/router/src/router_module.ts
View file
Expand Up @@ -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';
Expand Down Expand Up @@ -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);
},
};
}
Expand Down
19 changes: 15 additions & 4 deletions packages/router/src/router_scroller.ts
View file
Expand Up @@ -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';
Expand All @@ -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'
} = {}) {
Expand Down Expand Up @@ -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 */
Expand Down
24 changes: 12 additions & 12 deletions packages/router/test/bootstrap.spec.ts
View file
Expand Up @@ -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();
});
});
Expand Down Expand Up @@ -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);

Expand All @@ -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);
});
Expand Down Expand Up @@ -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']);
Expand Down
45 changes: 34 additions & 11 deletions packages/router/test/router_scroller.spec.ts
View file
Expand Up @@ -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),
Expand All @@ -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]);
});
Expand All @@ -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}));
Expand All @@ -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),
Expand All @@ -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};
Expand Down

1 comment on commit 79e9e8a

@ayyash
Copy link

@ayyash ayyash commented on 79e9e8a Nov 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May be you want to expose scrollToAnchor to let users decide when to do it? With headlessCMS it's almost always out of sync

Please sign in to comment.