Skip to content

Commit

Permalink
fix(common): Allow scrolling when browser supports scrollTo (#38468)
Browse files Browse the repository at this point in the history
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 bb88c9f, 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
  • Loading branch information
atscott committed Aug 14, 2020
1 parent d5e09f4 commit b32126c
Show file tree
Hide file tree
Showing 2 changed files with 23 additions and 6 deletions.
14 changes: 11 additions & 3 deletions packages/common/src/viewport_scroller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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];
Expand All @@ -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]);
}
}
Expand All @@ -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) {
Expand Down Expand Up @@ -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 {
Expand Down
15 changes: 12 additions & 3 deletions packages/common/test/viewport_scroller_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand Down

0 comments on commit b32126c

Please sign in to comment.