From 71079ce47e974df4139fb7f93cca5cc4da237a9d Mon Sep 17 00:00:00 2001 From: Andrew Scott Date: Fri, 14 Aug 2020 10:32:55 -0700 Subject: [PATCH] fix(common): Allow scrolling when browser supports scrollTo (#38468) This commit fixes a regression from "fix(common): ensure scrollRestoration is writable (#30630)" that caused scrolling to not happen at all in browsers that do not support scroll restoration. The issue was that `supportScrollRestoration` was updated to return `false` if a browser did not have a writable `scrollRestoration`. However, the previous behavior was that the function would return `true` if `window.scrollTo` was defined. Every scrolling function in the `ViewportScroller` used `supportScrollRestoration` and, with the update in bb88c9fa3daac80086efbda951d81c159e3840f4, no scrolling would be performed if a browser did not have writable `scrollRestoration` but _did_ have `window.scrollTo`. Note, that this failure was detected in the saucelabs tests. IE does not support scroll restoration so IE tests were failing. PR Close #38468 --- packages/common/src/viewport_scroller.ts | 14 +++++++++++--- packages/common/test/viewport_scroller_spec.ts | 15 ++++++++++++--- 2 files changed, 23 insertions(+), 6 deletions(-) diff --git a/packages/common/src/viewport_scroller.ts b/packages/common/src/viewport_scroller.ts index 355b90ecc79d6..a7c9093426938 100644 --- a/packages/common/src/viewport_scroller.ts +++ b/packages/common/src/viewport_scroller.ts @@ -88,7 +88,7 @@ export class BrowserViewportScroller implements ViewportScroller { * @returns The position in screen coordinates. */ getScrollPosition(): [number, number] { - if (this.supportScrollRestoration()) { + if (this.supportsScrolling()) { return [this.window.scrollX, this.window.scrollY]; } else { return [0, 0]; @@ -100,7 +100,7 @@ export class BrowserViewportScroller implements ViewportScroller { * @param position The new position in screen coordinates. */ scrollToPosition(position: [number, number]): void { - if (this.supportScrollRestoration()) { + if (this.supportsScrolling()) { this.window.scrollTo(position[0], position[1]); } } @@ -110,7 +110,7 @@ export class BrowserViewportScroller implements ViewportScroller { * @param anchor The ID of the anchor element. */ scrollToAnchor(anchor: string): void { - if (this.supportScrollRestoration()) { + if (this.supportsScrolling()) { const elSelected = this.document.getElementById(anchor) || this.document.getElementsByName(anchor)[0]; if (elSelected) { @@ -163,6 +163,14 @@ export class BrowserViewportScroller implements ViewportScroller { return false; } } + + private supportsScrolling(): boolean { + try { + return !!this.window.scrollTo; + } catch { + return false; + } + } } function getScrollRestorationProperty(obj: any): PropertyDescriptor|undefined { diff --git a/packages/common/test/viewport_scroller_spec.ts b/packages/common/test/viewport_scroller_spec.ts index 510a24bad5b7f..006702e985038 100644 --- a/packages/common/test/viewport_scroller_spec.ts +++ b/packages/common/test/viewport_scroller_spec.ts @@ -15,21 +15,30 @@ describe('BrowserViewportScroller', () => { let windowSpy: any; beforeEach(() => { - windowSpy = jasmine.createSpyObj('window', ['history']); - windowSpy.scrollTo = 1; + windowSpy = jasmine.createSpyObj('window', ['history', 'scrollTo']); windowSpy.history.scrollRestoration = 'auto'; documentSpy = jasmine.createSpyObj('document', ['getElementById', 'getElementsByName']); scroller = new BrowserViewportScroller(documentSpy, windowSpy, null!); }); describe('setHistoryScrollRestoration', () => { - it('should not crash when scrollRestoration is not writable', () => { + function createNonWritableScrollRestoration() { Object.defineProperty(windowSpy.history, 'scrollRestoration', { value: 'auto', configurable: true, }); + } + + it('should not crash when scrollRestoration is not writable', () => { + createNonWritableScrollRestoration(); expect(() => scroller.setHistoryScrollRestoration('manual')).not.toThrow(); }); + + it('should still allow scrolling if scrollRestoration is not writable', () => { + createNonWritableScrollRestoration(); + scroller.scrollToPosition([10, 10]); + expect(windowSpy.scrollTo as jasmine.Spy).toHaveBeenCalledWith(10, 10); + }); }); describe('scrollToAnchor', () => {