refactor(common): use getElementById in ViewportScroller.scrollToAnchor (angular#30143)

This commit uses getElementById and getElementsByName when an anchor scroll happens,
to avoid escaping the anchor and wrapping the code in a try/catch block.

Related to angular#28960

PR Close angular#30143

Author: aleesaan

goldens/size-tracking/integration-payloads.json

@@ -49,9 +49,9 @@
     "master": {
       "uncompressed": {
         "runtime-es2015": 2289,
-        "main-es2015": 221897,
-        "polyfills-es2015": 36938,
-        "5-es2015": 779
+        "main-es2015": 221939,
+        "polyfills-es2015": 36723,
+        "5-es2015": 781
       }
     }
   },
@@ -66,4 +66,4 @@
       }
     }
   }
-}
+}

packages/common/src/viewport_scroller.ts

@@ -111,26 +111,10 @@ export class BrowserViewportScroller implements ViewportScroller {
    */
   scrollToAnchor(anchor: string): void {
     if (this.supportScrollRestoration()) {
-      // Escape anything passed to `querySelector` as it can throw errors and stop the application
-      // from working if invalid values are passed.
-      if (this.window.CSS && this.window.CSS.escape) {
-        anchor = this.window.CSS.escape(anchor);
-      } else {
-        anchor = anchor.replace(/(\"|\'\ |:|\.|\[|\]|,|=)/g, '\\$1');
-      }
-      try {
-        const elSelectedById = this.document.querySelector(`#${anchor}`);
-        if (elSelectedById) {
-          this.scrollToElement(elSelectedById);
-          return;
-        }
-        const elSelectedByName = this.document.querySelector(`[name='${anchor}']`);
-        if (elSelectedByName) {
-          this.scrollToElement(elSelectedByName);
-          return;
-        }
-      } catch (e) {
-        this.errorHandler.handleError(e);
+      const elSelected =
+          this.document.getElementById(anchor) || this.document.getElementsByName(anchor)[0];
+      if (elSelected) {
+        this.scrollToElement(elSelected);
       }
     }
   }

packages/common/test/viewport_scroller_spec.ts

@@ -6,50 +6,61 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
-
-
-/**
- * @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
- */
-
 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;
+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, windowSpy, null!);
-    });
+  beforeEach(() => {
+    windowSpy = jasmine.createSpyObj('window', ['history']);
+    windowSpy.scrollTo = 1;
+    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', () => {
       Object.defineProperty(windowSpy.history, 'scrollRestoration', {
         value: 'auto',
         configurable: true,
       });
       expect(() => scroller.setHistoryScrollRestoration('manual')).not.toThrow();
     });
+  });
+
+  describe('scrollToAnchor', () => {
+    const anchor = 'anchor';
+    const el = document.createElement('a');
+
+    it('should only call getElementById when an element is found by id', () => {
+      documentSpy.getElementById.and.returnValue(el);
+      spyOn<any>(scroller, 'scrollToElement');
+      scroller.scrollToAnchor(anchor);
+      expect(documentSpy.getElementById).toHaveBeenCalledWith(anchor);
+      expect(documentSpy.getElementsByName).not.toHaveBeenCalled();
+      expect((scroller as any).scrollToElement).toHaveBeenCalledWith(el);
+    });
+
+    it('should call getElementById and getElementsByName when an element is found by name', () => {
+      documentSpy.getElementsByName.and.returnValue([el]);
+      spyOn<any>(scroller, 'scrollToElement');
+      scroller.scrollToAnchor(anchor);
+      expect(documentSpy.getElementById).toHaveBeenCalledWith(anchor);
+      expect(documentSpy.getElementsByName).toHaveBeenCalledWith(anchor);
+      expect((scroller as any).scrollToElement).toHaveBeenCalledWith(el);
+    });
 
-    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 not call scrollToElement when an element is not found by its id or its name', () => {
+      documentSpy.getElementsByName.and.returnValue([]);
+      spyOn<any>(scroller, 'scrollToElement');
+      scroller.scrollToAnchor(anchor);
+      expect(documentSpy.getElementById).toHaveBeenCalledWith(anchor);
+      expect(documentSpy.getElementsByName).toHaveBeenCalledWith(anchor);
+      expect((scroller as any).scrollToElement).not.toHaveBeenCalled();
     });
   });
-}
+});