Skip to content

Commit

Permalink
fix(common): ensure scrollRestoration is writable (#30630) (#38357)
Browse files Browse the repository at this point in the history
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

PR Close #38357
  • Loading branch information
marcvincenti authored and AndrewKushnir committed Aug 6, 2020
1 parent 3974312 commit 58f4b3a
Show file tree
Hide file tree
Showing 5 changed files with 69 additions and 6 deletions.
20 changes: 20 additions & 0 deletions aio/src/app/shared/scroll.service.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down
20 changes: 18 additions & 2 deletions aio/src/app/shared/scroll.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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);
}
4 changes: 2 additions & 2 deletions goldens/size-tracking/aio-payloads.json
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,8 @@
"master": {
"uncompressed": {
"runtime-es2015": 3097,
"main-es2015": 428653,
"polyfills-es2015": 52261
"main-es2015": 429110,
"polyfills-es2015": 52195
}
}
}
Expand Down
14 changes: 13 additions & 1 deletion packages/common/src/viewport_scroller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
17 changes: 16 additions & 1 deletion packages/common/test/viewport_scroller_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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`
Expand Down

0 comments on commit 58f4b3a

Please sign in to comment.