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

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
  • Loading branch information
@atscott
atscott committed Sep 27, 2022
1 parent 8cef5dd commit 7d9321e
Show file tree
Hide file tree
Showing 2 changed files with 35 additions and 10 deletions.
10 changes: 8 additions & 2 deletions packages/router/src/router_scroller.ts
View file
Expand Up @@ -85,8 +85,14 @@ 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));
// 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.router.triggerEvent(new Scroll(
routerEvent, this.lastSource === 'popstate' ? this.store[this.restoredId] : null,
anchor));
}, 0);
}

/** @nodoc */
Expand Down
35 changes: 27 additions & 8 deletions packages/router/test/router_scroller.spec.ts
View file
Expand Up @@ -7,17 +7,17 @@
*/

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
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 @@ -34,81 +34,97 @@ describe('RouterScroller', () => {
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 +151,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 +183,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 Down

0 comments on commit 7d9321e

Please sign in to comment.