Skip to content

Commit

Permalink
fix(docs-infra): fix scroll position restoration error in `ScrollServ…
Browse files Browse the repository at this point in the history
…ice` (#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
  • Loading branch information
gkalpak authored and jasonaden committed Apr 2, 2019
1 parent 53be333 commit 0e201ea
Show file tree
Hide file tree
Showing 2 changed files with 12 additions and 19 deletions.
15 changes: 5 additions & 10 deletions aio/src/app/shared/scroll.service.spec.ts
Expand Up @@ -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);
}

Expand All @@ -289,21 +287,18 @@ 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');
location.go('/initial-url2');
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);
}

Expand Down
16 changes: 7 additions & 9 deletions aio/src/app/shared/scroll.service.ts
Expand Up @@ -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) &&
Expand Down Expand Up @@ -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;
}
});
}
Expand Down Expand Up @@ -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;
}
}

/**
Expand Down Expand Up @@ -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;
}

/**
Expand Down

0 comments on commit 0e201ea

Please sign in to comment.