From 354e66efad728b39026fe1664903b55b65fc6786 Mon Sep 17 00:00:00 2001 From: Alessandro Date: Thu, 16 Jul 2020 10:58:17 +0200 Subject: [PATCH] refactor(common): use getElementById in ViewportScroller.scrollToAnchor (#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 #28960 PR Close #30143 --- .../size-tracking/integration-payloads.json | 8 +- packages/common/src/viewport_scroller.ts | 24 +----- .../common/test/viewport_scroller_spec.ts | 75 +++++++++++-------- 3 files changed, 51 insertions(+), 56 deletions(-) diff --git a/goldens/size-tracking/integration-payloads.json b/goldens/size-tracking/integration-payloads.json index 2f69e3ce40..9a036944cf 100644 --- a/goldens/size-tracking/integration-payloads.json +++ b/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 @@ } } } -} \ No newline at end of file +} diff --git a/packages/common/src/viewport_scroller.ts b/packages/common/src/viewport_scroller.ts index cea5ca5d30..355b90ecc7 100644 --- a/packages/common/src/viewport_scroller.ts +++ b/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); } } } diff --git a/packages/common/test/viewport_scroller_spec.ts b/packages/common/test/viewport_scroller_spec.ts index 2ac9328fc1..510a24bad5 100644 --- a/packages/common/test/viewport_scroller_spec.ts +++ b/packages/common/test/viewport_scroller_spec.ts @@ -6,34 +6,23 @@ * 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', @@ -41,15 +30,37 @@ import {BrowserViewportScroller, ViewportScroller} from '../src/viewport_scrolle }); 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}']`); + 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(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(scroller, 'scrollToElement'); + scroller.scrollToAnchor(anchor); + expect(documentSpy.getElementById).toHaveBeenCalledWith(anchor); + expect(documentSpy.getElementsByName).toHaveBeenCalledWith(anchor); + expect((scroller as any).scrollToElement).toHaveBeenCalledWith(el); + }); + + it('should not call scrollToElement when an element is not found by its id or its name', () => { + documentSpy.getElementsByName.and.returnValue([]); + spyOn(scroller, 'scrollToElement'); + scroller.scrollToAnchor(anchor); + expect(documentSpy.getElementById).toHaveBeenCalledWith(anchor); + expect(documentSpy.getElementsByName).toHaveBeenCalledWith(anchor); + expect((scroller as any).scrollToElement).not.toHaveBeenCalled(); }); }); -} +});