From a20fec17265522109a17f56615051dcfcf8845ed Mon Sep 17 00:00:00 2001 From: WilliamKoza Date: Sun, 20 Jan 2019 14:12:52 +0100 Subject: [PATCH 1/2] fix(common): Prefer to use pageXOffset / pageYOffset instance of scrollX / scrollY This fix ensures a better cross-browser compatibility. This fix has been used for angular.io. --- aio/src/app/shared/scroll.service.ts | 2 +- packages/common/src/viewport_scroller.ts | 6 +++--- packages/common/test/viewport_scroller_spec.ts | 3 ++- packages/router/test/bootstrap.spec.ts | 3 ++- 4 files changed, 8 insertions(+), 6 deletions(-) diff --git a/aio/src/app/shared/scroll.service.ts b/aio/src/app/shared/scroll.service.ts index ab5f299b66e7c..20b133055bf08 100644 --- a/aio/src/app/shared/scroll.service.ts +++ b/aio/src/app/shared/scroll.service.ts @@ -24,7 +24,7 @@ export class ScrollService implements OnDestroy { 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) && isScrollRestorationWritable(); + ('pageXOffset' in window) && isScrollRestorationWritable(); // Offset from the top of the document to bottom of any static elements // at the top (e.g. toolbar) + some margin diff --git a/packages/common/src/viewport_scroller.ts b/packages/common/src/viewport_scroller.ts index bbcf53ebbc898..d3ff85d6fde3a 100644 --- a/packages/common/src/viewport_scroller.ts +++ b/packages/common/src/viewport_scroller.ts @@ -89,7 +89,7 @@ export class BrowserViewportScroller implements ViewportScroller { */ getScrollPosition(): [number, number] { if (this.supportsScrolling()) { - return [this.window.scrollX, this.window.scrollY]; + return [this.window.pageXOffset, this.window.pageYOffset]; } else { return [0, 0]; } @@ -149,7 +149,7 @@ export class BrowserViewportScroller implements ViewportScroller { */ private supportScrollRestoration(): boolean { try { - if (!this.window || !this.window.scrollTo) { + if (!this.supportsScrolling()) { return false; } // The `scrollRestoration` property could be on the `history` instance or its prototype. @@ -166,7 +166,7 @@ export class BrowserViewportScroller implements ViewportScroller { private supportsScrolling(): boolean { try { - return !!this.window.scrollTo; + return !!this.window && !!this.window.scrollTo && 'pageXOffset' in this.window; } catch { return false; } diff --git a/packages/common/test/viewport_scroller_spec.ts b/packages/common/test/viewport_scroller_spec.ts index 006702e985038..90713e62fda99 100644 --- a/packages/common/test/viewport_scroller_spec.ts +++ b/packages/common/test/viewport_scroller_spec.ts @@ -15,7 +15,8 @@ describe('BrowserViewportScroller', () => { let windowSpy: any; beforeEach(() => { - windowSpy = jasmine.createSpyObj('window', ['history', 'scrollTo']); + windowSpy = + jasmine.createSpyObj('window', ['history', 'scrollTo', 'pageXOffset', 'pageYOffset']); windowSpy.history.scrollRestoration = 'auto'; documentSpy = jasmine.createSpyObj('document', ['getElementById', 'getElementsByName']); scroller = new BrowserViewportScroller(documentSpy, windowSpy, null!); diff --git a/packages/router/test/bootstrap.spec.ts b/packages/router/test/bootstrap.spec.ts index f3ee2c51df20a..a3c5232256be6 100644 --- a/packages/router/test/bootstrap.spec.ts +++ b/packages/router/test/bootstrap.spec.ts @@ -349,7 +349,8 @@ describe('bootstrap', () => { window.scrollTo(0, 5000); // IE 11 uses non-standard pageYOffset instead of scrollY - const getScrollY = () => window.scrollY !== undefined ? window.scrollY : window.pageYOffset; + // For cross-browser compatibility, prefer window.pageYOffset instead of window.scrollY + const getScrollY = () => window.pageYOffset; await router.navigateByUrl('/fail'); expect(getScrollY()).toEqual(5000); From 9755ef29b1dfc06a323f3ca3c6941a6f0fa1a129 Mon Sep 17 00:00:00 2001 From: William Koza Date: Sun, 15 Nov 2020 10:47:12 +0100 Subject: [PATCH 2/2] fixup! fix(common): Prefer to use pageXOffset / pageYOffset instance of scrollX / scrollY --- packages/router/test/bootstrap.spec.ts | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/packages/router/test/bootstrap.spec.ts b/packages/router/test/bootstrap.spec.ts index a3c5232256be6..5c76c6b8e2c9b 100644 --- a/packages/router/test/bootstrap.spec.ts +++ b/packages/router/test/bootstrap.spec.ts @@ -348,28 +348,24 @@ describe('bootstrap', () => { await router.navigateByUrl('/aa'); window.scrollTo(0, 5000); - // IE 11 uses non-standard pageYOffset instead of scrollY - // For cross-browser compatibility, prefer window.pageYOffset instead of window.scrollY - const getScrollY = () => window.pageYOffset; - await router.navigateByUrl('/fail'); - expect(getScrollY()).toEqual(5000); + expect(window.pageYOffset).toEqual(5000); await router.navigateByUrl('/bb'); window.scrollTo(0, 3000); - expect(getScrollY()).toEqual(3000); + expect(window.pageYOffset).toEqual(3000); await router.navigateByUrl('/cc'); - expect(getScrollY()).toEqual(0); + expect(window.pageYOffset).toEqual(0); await router.navigateByUrl('/aa#marker2'); - expect(getScrollY()).toBeGreaterThanOrEqual(5900); - expect(getScrollY()).toBeLessThan(6000); // offset + expect(window.pageYOffset).toBeGreaterThanOrEqual(5900); + expect(window.pageYOffset).toBeLessThan(6000); // offset await router.navigateByUrl('/aa#marker3'); - expect(getScrollY()).toBeGreaterThanOrEqual(8900); - expect(getScrollY()).toBeLessThan(9000); + expect(window.pageYOffset).toBeGreaterThanOrEqual(8900); + expect(window.pageYOffset).toBeLessThan(9000); done(); });