iSharkFly-Docs
/
angular-docs-cn
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

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
This commit is contained in:
George Kalpakas 2019-04-02 13:46:12 +03:00 committed by Jason Aden
parent 53be333439
commit 0e201ea9d8
2 changed files with 12 additions and 19 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

15
aio/src/app/shared/scroll.service.spec.ts
View File

@ -264,16 +264,14 @@ describe('ScrollService', () => {
location.go('/initial-url2'); location.go('/initial-url2');
location.back(); location.back();
expect(scrollService.popStateFired).toBe(true); expect(scrollService.poppedStateScrollPosition).toEqual([2000, 0]);
expect(scrollService.scrollPosition).toEqual([2000, 0]);
expect(scrollService.needToFixScrollPosition()).toBe(true); expect(scrollService.needToFixScrollPosition()).toBe(true);
} else { } else {
location.go('/initial-url1'); location.go('/initial-url1');
location.go('/initial-url2'); location.go('/initial-url2');
location.back(); location.back();
expect(scrollService.popStateFired).toBe(false); // popStateFired is always false expect(scrollService.poppedStateScrollPosition).toBe(null);
expect(scrollService.scrollPosition).toEqual([0, 0]); // scrollPosition always equals [0, 0]
expect(scrollService.needToFixScrollPosition()).toBe(false); expect(scrollService.needToFixScrollPosition()).toBe(false);
} }
@ -289,12 +287,10 @@ describe('ScrollService', () => {
location.replaceState('/initial-url1', 'hack', {scrollPosition: [2000, 0]}); location.replaceState('/initial-url1', 'hack', {scrollPosition: [2000, 0]});
location.back(); location.back();
scrollService.popStateFired = false; scrollService.poppedStateScrollPosition = [0, 0];
scrollService.scrollPosition = [0, 0];
location.forward(); location.forward();
expect(scrollService.popStateFired).toBe(true); expect(scrollService.poppedStateScrollPosition).toEqual([2000, 0]);
expect(scrollService.scrollPosition).toEqual([2000, 0]);
expect(scrollService.needToFixScrollPosition()).toBe(true); expect(scrollService.needToFixScrollPosition()).toBe(true);
} else { } else {
location.go('/initial-url1'); location.go('/initial-url1');
@ -302,8 +298,7 @@ describe('ScrollService', () => {
location.back(); location.back();
location.forward(); location.forward();
expect(scrollService.popStateFired).toBe(false); // popStateFired is always false expect(scrollService.poppedStateScrollPosition).toBe(null);
expect(scrollService.scrollPosition).toEqual([0, 0]); // scrollPosition always equals [0, 0]
expect(scrollService.needToFixScrollPosition()).toBe(false); expect(scrollService.needToFixScrollPosition()).toBe(false);
} }

16
aio/src/app/shared/scroll.service.ts
View File

@ -19,11 +19,8 @@ export class ScrollService {
private _topOffset: number | null; private _topOffset: number | null;
private _topOfPageElement: Element; 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. // 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. // Whether the browser supports the necessary features for manual scroll restoration.
supportManualScrollRestoration: boolean = supportManualScrollRestoration: boolean =
!!window && ('scrollTo' in window) && ('scrollX' in window) && ('scrollY' in window) && !!window && ('scrollTo' in window) && ('scrollX' in window) && ('scrollY' in window) &&
@ -72,8 +69,7 @@ export class ScrollService {
this.removeStoredScrollPosition(); this.removeStoredScrollPosition();
// The `popstate` event is always triggered by a browser action such as clicking the // 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. // forward/back button. It can be followed by a `hashchange` event.
this.popStateFired = true; this.poppedStateScrollPosition = event.state ? event.state.scrollPosition : null;
this.scrollPosition = event.state ? event.state['scrollPosition'] : null;
} }
}); });
} }
@ -160,8 +156,10 @@ export class ScrollService {
} }
scrollToPosition() { scrollToPosition() {
this.viewportScroller.scrollToPosition(this.scrollPosition); if (this.poppedStateScrollPosition) {
this.popStateFired = false; 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 * Check if the scroll position need to be manually fixed after popState event
*/ */
needToFixScrollPosition(): boolean { needToFixScrollPosition(): boolean {
return this.popStateFired && this.scrollPosition && this.supportManualScrollRestoration; return this.supportManualScrollRestoration && !!this.poppedStateScrollPosition;
} }
/** /**