From bb88c9fa3daac80086efbda951d81c159e3840f4 Mon Sep 17 00:00:00 2001 From: marcvincenti Date: Thu, 23 May 2019 14:44:46 +0200 Subject: [PATCH] fix(common): ensure scrollRestoration is writable (#30630) Some specialised browsers that do not support scroll restoration (e.g. some web crawlers) do not allow `scrollRestoration` to be writable. We already sniff the browser to see if it has the `window.scrollTo` method, so now we also check whether `window.history.scrollRestoration` is writable too. Fixes #30629 PR Close #30630 --- aio/src/app/shared/scroll.service.spec.ts | 20 +++++++++++++++++++ aio/src/app/shared/scroll.service.ts | 20 +++++++++++++++++-- goldens/size-tracking/aio-payloads.json | 4 ++-- packages/common/src/viewport_scroller.ts | 14 ++++++++++++- .../common/test/viewport_scroller_spec.ts | 17 +++++++++++++++- 5 files changed, 69 insertions(+), 6 deletions(-) diff --git a/aio/src/app/shared/scroll.service.spec.ts b/aio/src/app/shared/scroll.service.spec.ts index fd694f6930..bd3ffd71f1 100644 --- a/aio/src/app/shared/scroll.service.spec.ts +++ b/aio/src/app/shared/scroll.service.spec.ts @@ -80,6 +80,26 @@ describe('ScrollService', () => { expect(updateScrollPositionInHistorySpy).toHaveBeenCalledTimes(1); })); + it('should not support `manual` scrollRestoration when it is not writable', () => { + const original = Object.getOwnPropertyDescriptor(window.history, 'scrollRestoration'); + try { + Object.defineProperty(window.history, 'scrollRestoration', { + value: 'auto', + configurable: true, + }); + scrollService = createScrollService( + document, platformLocation as PlatformLocation, viewportScrollerStub, location); + + expect(scrollService.supportManualScrollRestoration).toBe(false); + } finally { + if (original !== undefined) { + Object.defineProperty(window.history, 'scrollRestoration', original); + } else { + delete window.history.scrollRestoration; + } + } + }); + it('should set `scrollRestoration` to `manual` if supported', () => { if (scrollService.supportManualScrollRestoration) { expect(window.history.scrollRestoration).toBe('manual'); diff --git a/aio/src/app/shared/scroll.service.ts b/aio/src/app/shared/scroll.service.ts index 3664bbc130..ab5f299b66 100644 --- a/aio/src/app/shared/scroll.service.ts +++ b/aio/src/app/shared/scroll.service.ts @@ -24,8 +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) && !!history && - ('scrollRestoration' in history); + ('scrollX' in window) && ('scrollY' in window) && isScrollRestorationWritable(); // Offset from the top of the document to bottom of any static elements // at the top (e.g. toolbar) + some margin @@ -243,3 +242,20 @@ export class ScrollService implements OnDestroy { return decodeURIComponent(this.platformLocation.hash.replace(/^#/, '')); } } + +/** + * We need to check whether we can write to `history.scrollRestoration` + * + * We do this by checking the property descriptor of the property, but + * it might actually be defined on the `history` prototype not the instance. + * + * In this context "writable" means either than the property is a `writable` + * data file or a property that has a setter. + */ +function isScrollRestorationWritable() { + const scrollRestorationDescriptor = + Object.getOwnPropertyDescriptor(history, 'scrollRestoration') || + Object.getOwnPropertyDescriptor(Object.getPrototypeOf(history), 'scrollRestoration'); + return scrollRestorationDescriptor !== undefined && + !!(scrollRestorationDescriptor.writable || scrollRestorationDescriptor.set); +} diff --git a/goldens/size-tracking/aio-payloads.json b/goldens/size-tracking/aio-payloads.json index b91c2891d6..8ebdb7da24 100755 --- a/goldens/size-tracking/aio-payloads.json +++ b/goldens/size-tracking/aio-payloads.json @@ -21,9 +21,9 @@ "master": { "uncompressed": { "runtime-es2015": 3097, - "main-es2015": 429885, + "main-es2015": 430239, "polyfills-es2015": 52195 } } } -} \ No newline at end of file +} diff --git a/packages/common/src/viewport_scroller.ts b/packages/common/src/viewport_scroller.ts index c2def3b91b..cea5ca5d30 100644 --- a/packages/common/src/viewport_scroller.ts +++ b/packages/common/src/viewport_scroller.ts @@ -165,13 +165,25 @@ export class BrowserViewportScroller implements ViewportScroller { */ private supportScrollRestoration(): boolean { try { - return !!this.window && !!this.window.scrollTo; + if (!this.window || !this.window.scrollTo) { + return false; + } + // The `scrollRestoration` property could be on the `history` instance or its prototype. + const scrollRestorationDescriptor = getScrollRestorationProperty(this.window.history) || + getScrollRestorationProperty(Object.getPrototypeOf(this.window.history)); + // We can write to the `scrollRestoration` property if it is a writable data field or it has a + // setter function. + return !!scrollRestorationDescriptor && + !!(scrollRestorationDescriptor.writable || scrollRestorationDescriptor.set); } catch { return false; } } } +function getScrollRestorationProperty(obj: any): PropertyDescriptor|undefined { + return Object.getOwnPropertyDescriptor(obj, 'scrollRestoration'); +} /** * Provides an empty implementation of the viewport scroller. This will diff --git a/packages/common/test/viewport_scroller_spec.ts b/packages/common/test/viewport_scroller_spec.ts index f3b6c907a8..2ac9328fc1 100644 --- a/packages/common/test/viewport_scroller_spec.ts +++ b/packages/common/test/viewport_scroller_spec.ts @@ -23,10 +23,25 @@ import {BrowserViewportScroller, ViewportScroller} from '../src/viewport_scrolle describe('BrowserViewportScroller', () => { let scroller: ViewportScroller; let documentSpy: any; + let windowSpy: any; + beforeEach(() => { + windowSpy = jasmine.createSpyObj('window', ['history']); + windowSpy.scrollTo = 1; + windowSpy.history.scrollRestoration = 'auto'; + documentSpy = jasmine.createSpyObj('document', ['querySelector']); - scroller = new BrowserViewportScroller(documentSpy, {scrollTo: 1}, null!); + scroller = new BrowserViewportScroller(documentSpy, windowSpy, null!); }); + + it('should not crash when scrollRestoration is not writable', () => { + Object.defineProperty(windowSpy.history, 'scrollRestoration', { + value: 'auto', + configurable: true, + }); + expect(() => scroller.setHistoryScrollRestoration('manual')).not.toThrow(); + }); + it('escapes invalid characters selectors', () => { const invalidSelectorChars = `"' :.[],=`; // Double escaped to make sure we match the actual value passed to `querySelector`