From 0e201ea9d86d941be14f169267ae5f7353576a54 Mon Sep 17 00:00:00 2001 From: George Kalpakas Date: Tue, 2 Apr 2019 13:46:12 +0300 Subject: [PATCH] fix(docs-infra): fix scroll position restoration error in `ScrollService` (#29658) Based on Google Analytics error report, the following error happens occasionally (15% or total errors for 2019-03): ``` Cannot read property '0' of null TypeError: at t.scrollToPosition@main.js ``` This was a result of calling [ViewportScroller#scrollToPosition()][1] with `null`, which in turn happens when calling [ScrollService#scrollToPosition()][2] while `this.scrollPosition` is `null`. This can be a result of a `popstate` event without an associated history state. This commit fixes the error by checking whether `this.scrollPosition` is `null`, before using it with `scrollToPosition()`. (It also refactors away the unneeded internal `popStateFired` property.) [1]: https://github.com/angular/angular/blob/deca6a60d/packages/common/src/viewport_scroller.ts#L101-L105 [2]: https://github.com/angular/angular/blob/deca6a60d/aio/src/app/shared/scroll.service.ts#L158-L161 PR Close #29658 --- aio/src/app/shared/scroll.service.spec.ts | 15 +++++---------- aio/src/app/shared/scroll.service.ts | 16 +++++++--------- 2 files changed, 12 insertions(+), 19 deletions(-) diff --git a/aio/src/app/shared/scroll.service.spec.ts b/aio/src/app/shared/scroll.service.spec.ts index a27b500260..5b45d40986 100644 --- a/aio/src/app/shared/scroll.service.spec.ts +++ b/aio/src/app/shared/scroll.service.spec.ts @@ -264,16 +264,14 @@ describe('ScrollService', () => { location.go('/initial-url2'); location.back(); - expect(scrollService.popStateFired).toBe(true); - expect(scrollService.scrollPosition).toEqual([2000, 0]); + expect(scrollService.poppedStateScrollPosition).toEqual([2000, 0]); expect(scrollService.needToFixScrollPosition()).toBe(true); } else { location.go('/initial-url1'); location.go('/initial-url2'); location.back(); - expect(scrollService.popStateFired).toBe(false); // popStateFired is always false - expect(scrollService.scrollPosition).toEqual([0, 0]); // scrollPosition always equals [0, 0] + expect(scrollService.poppedStateScrollPosition).toBe(null); expect(scrollService.needToFixScrollPosition()).toBe(false); } @@ -289,12 +287,10 @@ describe('ScrollService', () => { location.replaceState('/initial-url1', 'hack', {scrollPosition: [2000, 0]}); location.back(); - scrollService.popStateFired = false; - scrollService.scrollPosition = [0, 0]; + scrollService.poppedStateScrollPosition = [0, 0]; location.forward(); - expect(scrollService.popStateFired).toBe(true); - expect(scrollService.scrollPosition).toEqual([2000, 0]); + expect(scrollService.poppedStateScrollPosition).toEqual([2000, 0]); expect(scrollService.needToFixScrollPosition()).toBe(true); } else { location.go('/initial-url1'); @@ -302,8 +298,7 @@ describe('ScrollService', () => { location.back(); location.forward(); - expect(scrollService.popStateFired).toBe(false); // popStateFired is always false - expect(scrollService.scrollPosition).toEqual([0, 0]); // scrollPosition always equals [0, 0] + expect(scrollService.poppedStateScrollPosition).toBe(null); expect(scrollService.needToFixScrollPosition()).toBe(false); } diff --git a/aio/src/app/shared/scroll.service.ts b/aio/src/app/shared/scroll.service.ts index 2e319507d4..4c0d864141 100644 --- a/aio/src/app/shared/scroll.service.ts +++ b/aio/src/app/shared/scroll.service.ts @@ -19,11 +19,8 @@ export class ScrollService { private _topOffset: number | null; private _topOfPageElement: Element; - // Whether a `popstate` event has been fired (but the associated scroll position is not yet - // restored). - popStateFired = false; // The scroll position which has to be restored, after a `popstate` event. - scrollPosition: ScrollPosition = [0, 0]; + poppedStateScrollPosition: ScrollPosition | null = null; // Whether the browser supports the necessary features for manual scroll restoration. supportManualScrollRestoration: boolean = !!window && ('scrollTo' in window) && ('scrollX' in window) && ('scrollY' in window) && @@ -72,8 +69,7 @@ export class ScrollService { this.removeStoredScrollPosition(); // The `popstate` event is always triggered by a browser action such as clicking the // forward/back button. It can be followed by a `hashchange` event. - this.popStateFired = true; - this.scrollPosition = event.state ? event.state['scrollPosition'] : null; + this.poppedStateScrollPosition = event.state ? event.state.scrollPosition : null; } }); } @@ -160,8 +156,10 @@ export class ScrollService { } scrollToPosition() { - this.viewportScroller.scrollToPosition(this.scrollPosition); - this.popStateFired = false; + if (this.poppedStateScrollPosition) { + this.viewportScroller.scrollToPosition(this.poppedStateScrollPosition); + this.poppedStateScrollPosition = null; + } } /** @@ -191,7 +189,7 @@ export class ScrollService { * Check if the scroll position need to be manually fixed after popState event */ needToFixScrollPosition(): boolean { - return this.popStateFired && this.scrollPosition && this.supportManualScrollRestoration; + return this.supportManualScrollRestoration && !!this.poppedStateScrollPosition; } /**