Skip to content

Commit bb88c9f

Browse files
marcvincentiAndrewKushnir
authored andcommitted
fix(common): ensure scrollRestoration is writable (angular#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 angular#30629 PR Close angular#30630
1 parent 8227b56 commit bb88c9f

File tree

5 files changed

+69
-6
lines changed

5 files changed

+69
-6
lines changed

aio/src/app/shared/scroll.service.spec.ts

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,26 @@ describe('ScrollService', () => {
8080
expect(updateScrollPositionInHistorySpy).toHaveBeenCalledTimes(1);
8181
}));
8282

83+
it('should not support `manual` scrollRestoration when it is not writable', () => {
84+
const original = Object.getOwnPropertyDescriptor(window.history, 'scrollRestoration');
85+
try {
86+
Object.defineProperty(window.history, 'scrollRestoration', {
87+
value: 'auto',
88+
configurable: true,
89+
});
90+
scrollService = createScrollService(
91+
document, platformLocation as PlatformLocation, viewportScrollerStub, location);
92+
93+
expect(scrollService.supportManualScrollRestoration).toBe(false);
94+
} finally {
95+
if (original !== undefined) {
96+
Object.defineProperty(window.history, 'scrollRestoration', original);
97+
} else {
98+
delete window.history.scrollRestoration;
99+
}
100+
}
101+
});
102+
83103
it('should set `scrollRestoration` to `manual` if supported', () => {
84104
if (scrollService.supportManualScrollRestoration) {
85105
expect(window.history.scrollRestoration).toBe('manual');

aio/src/app/shared/scroll.service.ts

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -24,8 +24,7 @@ export class ScrollService implements OnDestroy {
2424
poppedStateScrollPosition: ScrollPosition|null = null;
2525
// Whether the browser supports the necessary features for manual scroll restoration.
2626
supportManualScrollRestoration: boolean = !!window && ('scrollTo' in window) &&
27-
('scrollX' in window) && ('scrollY' in window) && !!history &&
28-
('scrollRestoration' in history);
27+
('scrollX' in window) && ('scrollY' in window) && isScrollRestorationWritable();
2928

3029
// Offset from the top of the document to bottom of any static elements
3130
// at the top (e.g. toolbar) + some margin
@@ -243,3 +242,20 @@ export class ScrollService implements OnDestroy {
243242
return decodeURIComponent(this.platformLocation.hash.replace(/^#/, ''));
244243
}
245244
}
245+
246+
/**
247+
* We need to check whether we can write to `history.scrollRestoration`
248+
*
249+
* We do this by checking the property descriptor of the property, but
250+
* it might actually be defined on the `history` prototype not the instance.
251+
*
252+
* In this context "writable" means either than the property is a `writable`
253+
* data file or a property that has a setter.
254+
*/
255+
function isScrollRestorationWritable() {
256+
const scrollRestorationDescriptor =
257+
Object.getOwnPropertyDescriptor(history, 'scrollRestoration') ||
258+
Object.getOwnPropertyDescriptor(Object.getPrototypeOf(history), 'scrollRestoration');
259+
return scrollRestorationDescriptor !== undefined &&
260+
!!(scrollRestorationDescriptor.writable || scrollRestorationDescriptor.set);
261+
}

goldens/size-tracking/aio-payloads.json

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,9 +21,9 @@
2121
"master": {
2222
"uncompressed": {
2323
"runtime-es2015": 3097,
24-
"main-es2015": 429885,
24+
"main-es2015": 430239,
2525
"polyfills-es2015": 52195
2626
}
2727
}
2828
}
29-
}
29+
}

packages/common/src/viewport_scroller.ts

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -165,13 +165,25 @@ export class BrowserViewportScroller implements ViewportScroller {
165165
*/
166166
private supportScrollRestoration(): boolean {
167167
try {
168-
return !!this.window && !!this.window.scrollTo;
168+
if (!this.window || !this.window.scrollTo) {
169+
return false;
170+
}
171+
// The `scrollRestoration` property could be on the `history` instance or its prototype.
172+
const scrollRestorationDescriptor = getScrollRestorationProperty(this.window.history) ||
173+
getScrollRestorationProperty(Object.getPrototypeOf(this.window.history));
174+
// We can write to the `scrollRestoration` property if it is a writable data field or it has a
175+
// setter function.
176+
return !!scrollRestorationDescriptor &&
177+
!!(scrollRestorationDescriptor.writable || scrollRestorationDescriptor.set);
169178
} catch {
170179
return false;
171180
}
172181
}
173182
}
174183

184+
function getScrollRestorationProperty(obj: any): PropertyDescriptor|undefined {
185+
return Object.getOwnPropertyDescriptor(obj, 'scrollRestoration');
186+
}
175187

176188
/**
177189
* Provides an empty implementation of the viewport scroller. This will

packages/common/test/viewport_scroller_spec.ts

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,10 +23,25 @@ import {BrowserViewportScroller, ViewportScroller} from '../src/viewport_scrolle
2323
describe('BrowserViewportScroller', () => {
2424
let scroller: ViewportScroller;
2525
let documentSpy: any;
26+
let windowSpy: any;
27+
2628
beforeEach(() => {
29+
windowSpy = jasmine.createSpyObj('window', ['history']);
30+
windowSpy.scrollTo = 1;
31+
windowSpy.history.scrollRestoration = 'auto';
32+
2733
documentSpy = jasmine.createSpyObj('document', ['querySelector']);
28-
scroller = new BrowserViewportScroller(documentSpy, {scrollTo: 1}, null!);
34+
scroller = new BrowserViewportScroller(documentSpy, windowSpy, null!);
2935
});
36+
37+
it('should not crash when scrollRestoration is not writable', () => {
38+
Object.defineProperty(windowSpy.history, 'scrollRestoration', {
39+
value: 'auto',
40+
configurable: true,
41+
});
42+
expect(() => scroller.setHistoryScrollRestoration('manual')).not.toThrow();
43+
});
44+
3045
it('escapes invalid characters selectors', () => {
3146
const invalidSelectorChars = `"' :.[],=`;
3247
// Double escaped to make sure we match the actual value passed to `querySelector`

0 commit comments

Comments
 (0)