fix(common): Prefer to use pageXOffset / pageYOffset instance of scro…

…llX / scrollY. This fix ensures a better cross-browser compatibility. This fix has been used for angular.io.
angular · Nov 11, 2020 · 50f8eeb · 50f8eeb
1 parent 39e8dbc
commit 50f8eeb
Show file tree

Hide file tree

Showing 3 changed files with 32 additions and 28 deletions.
diff --git a/aio/src/app/shared/scroll.service.ts b/aio/src/app/shared/scroll.service.ts
@@ -24,7 +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) && isScrollRestorationWritable();
+      ('pageXOffset' in window) && ('pageYOffset' in window) && isScrollRestorationWritable();
 
   // Offset from the top of the document to bottom of any static elements
   // at the top (e.g. toolbar) + some margin

diff --git a/packages/common/src/viewport_scroller.ts b/packages/common/src/viewport_scroller.ts
@@ -86,7 +86,7 @@ export class BrowserViewportScroller implements ViewportScroller {
    */
   getScrollPosition(): [number, number] {
     if (this.supportsScrolling()) {
-      return [this.window.scrollX, this.window.scrollY];
+      return [this.window.pageXOffset, this.window.pageYOffset];
     } else {
       return [0, 0];
     }
@@ -146,7 +146,7 @@ export class BrowserViewportScroller implements ViewportScroller {
    */
   private supportScrollRestoration(): boolean {
     try {
-      if (!this.window || !this.window.scrollTo) {
+      if (!this.supportsScrolling()) {
         return false;
       }
       // The `scrollRestoration` property could be on the `history` instance or its prototype.
@@ -163,7 +163,8 @@ export class BrowserViewportScroller implements ViewportScroller {
 
   private supportsScrolling(): boolean {
     try {
-      return !!this.window.scrollTo;
+      return !!this.window && !!this.window.scrollTo && !!this.window.pageXOffset &&
+          !!this.window.pageYOffset;
     } catch {
       return false;
     }

diff --git a/packages/common/test/viewport_scroller_spec.ts b/packages/common/test/viewport_scroller_spec.ts
@@ -6,35 +6,38 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
+import {describe, expect, it} from '@angular/core/testing/src/testing_internal';
+import {BrowserViewportScroller, ViewportScroller} from '../src/viewport_scroller';
 
+describe('BrowserViewportScroller', () => {
+  let scroller: ViewportScroller;
+  let documentSpy: any;
+  let windowSpy: any;
 
-/**
- * @license
- * Copyright Google LLC All Rights Reserved.
- *
- * Use of this source code is governed by an MIT-style license that can be
- * found in the LICENSE file at https://angular.io/license
- */
+  beforeEach(() => {
+    windowSpy = jasmine.createSpyObj('window', ['history', 'scrollTo']);
+    windowSpy.history.scrollRestoration = 'auto';
+    documentSpy = jasmine.createSpyObj('document', ['getElementById', 'getElementsByName']);
+    scroller = new BrowserViewportScroller(documentSpy, windowSpy, null!);
+  });
 
-import {describe, expect, it} from '@angular/core/testing/src/testing_internal';
-import {BrowserViewportScroller, ViewportScroller} from '../src/viewport_scroller';
+  describe('setHistoryScrollRestoration', () => {
+    function createNonWritableScrollRestoration() {
+      Object.defineProperty(windowSpy.history, 'scrollRestoration', {
+        value: 'auto',
+        configurable: true,
+      });
+    }
 
-{
-  describe('BrowserViewportScroller', () => {
-    let scroller: ViewportScroller;
-    let documentSpy: any;
-    beforeEach(() => {
-      documentSpy = jasmine.createSpyObj('document', ['querySelector']);
-      scroller = new BrowserViewportScroller(documentSpy, {scrollTo: 1}, null!);
+    it('should not crash when scrollRestoration is not writable', () => {
+      createNonWritableScrollRestoration();
+      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`
-      const escapedInvalids = `\\"\\' \\:\\.\\[\\]\\,\\=`;
-      scroller.scrollToAnchor(`specials=${invalidSelectorChars}`);
-      expect(documentSpy.querySelector).toHaveBeenCalledWith(`#specials\\=${escapedInvalids}`);
-      expect(documentSpy.querySelector)
-          .toHaveBeenCalledWith(`[name='specials\\=${escapedInvalids}']`);
+
+    it('should still allow scrolling if scrollRestoration is not writable', () => {
+      createNonWritableScrollRestoration();
+      scroller.scrollToPosition([10, 10]);
+      expect(windowSpy.scrollTo as jasmine.Spy).toHaveBeenCalledWith(10, 10);
     });
   });