From ab8a01c0a6eda1bafc293b39cb6c77ed10fb359e Mon Sep 17 00:00:00 2001 From: Eduardo San Martin Morote Date: Fri, 12 Jun 2020 17:25:39 +0200 Subject: [PATCH] feat(scroll): replace selector with el BREAKING CHANGE: this change follows the RFC at https://github.com/vuejs/rfcs/pull/176: - `selector` is renamed into `el` - `el` also accepts an `Element` - `left` and `top` are passed along `el` instead of inside an object passed as `offset` --- __tests__/scrollBehavior.spec.ts | 197 +++++++++++++++++++++++++++++++ e2e/scroll-behavior/index.ts | 15 +-- src/scrollBehavior.ts | 56 +++++---- 3 files changed, 238 insertions(+), 30 deletions(-) create mode 100644 __tests__/scrollBehavior.spec.ts diff --git a/__tests__/scrollBehavior.spec.ts b/__tests__/scrollBehavior.spec.ts new file mode 100644 index 000000000..96b5dd1c1 --- /dev/null +++ b/__tests__/scrollBehavior.spec.ts @@ -0,0 +1,197 @@ +import { JSDOM } from 'jsdom' +import { scrollToPosition } from '../src/scrollBehavior' +import { createDom } from './utils' +import { mockWarn } from 'jest-mock-warn' + +describe('scrollBehavior', () => { + mockWarn() + let dom: JSDOM + let scrollTo: jest.SpyInstance + let getElementById: jest.SpyInstance + let querySelector: jest.SpyInstance + + beforeAll(() => { + dom = createDom() + scrollTo = jest.spyOn(window, 'scrollTo').mockImplementation(() => {}) + getElementById = jest.spyOn(document, 'getElementById') + querySelector = jest.spyOn(document, 'querySelector') + + // #text + let el = document.createElement('div') + el.id = 'text' + document.documentElement.appendChild(el) + + // [data-scroll] + el = document.createElement('div') + el.setAttribute('data-scroll', 'true') + document.documentElement.appendChild(el) + + // #special~characters + el = document.createElement('div') + el.id = 'special~characters' + document.documentElement.appendChild(el) + + // #text .container + el = document.createElement('div') + let child = document.createElement('div') + child.classList.add('container') + el.id = 'text' + el.append(child) + document.documentElement.appendChild(el) + + // .container #1 + el = document.createElement('div') + child = document.createElement('div') + el.classList.add('container') + child.id = '1' + el.append(child) + document.documentElement.appendChild(el) + }) + + beforeEach(() => { + scrollTo.mockClear() + getElementById.mockClear() + querySelector.mockClear() + __DEV__ = false + }) + + afterAll(() => { + __DEV__ = true + }) + + afterAll(() => { + dom.window.close() + scrollTo.mockRestore() + getElementById.mockRestore() + querySelector.mockRestore() + }) + + describe('left and top', () => { + it('scrolls to a position', () => { + scrollToPosition({ left: 10, top: 100 }) + expect(getElementById).not.toHaveBeenCalled() + expect(getElementById).not.toHaveBeenCalled() + expect(scrollTo).toHaveBeenCalledWith({ + left: 10, + top: 100, + behavior: undefined, + }) + }) + + it('scrolls to a partial position top', () => { + scrollToPosition({ top: 10 }) + expect(getElementById).not.toHaveBeenCalled() + expect(getElementById).not.toHaveBeenCalled() + expect(scrollTo).toHaveBeenCalledWith({ + top: 10, + behavior: undefined, + }) + }) + + it('scrolls to a partial position left', () => { + scrollToPosition({ left: 10 }) + expect(getElementById).not.toHaveBeenCalled() + expect(getElementById).not.toHaveBeenCalled() + expect(scrollTo).toHaveBeenCalledWith({ + left: 10, + behavior: undefined, + }) + }) + }) + + describe('el option', () => { + it('scrolls to an id', () => { + scrollToPosition({ el: '#text' }) + expect(getElementById).toHaveBeenCalledWith('text') + expect(querySelector).not.toHaveBeenCalled() + expect(scrollTo).toHaveBeenCalledWith({ + left: 0, + top: 0, + behavior: undefined, + }) + }) + + it('scrolls to an element using querySelector', () => { + scrollToPosition({ el: '[data-scroll=true]' }) + expect(querySelector).toHaveBeenCalledWith('[data-scroll=true]') + expect(getElementById).not.toHaveBeenCalled() + expect(scrollTo).toHaveBeenCalledWith({ + left: 0, + top: 0, + behavior: undefined, + }) + }) + + it('scrolls to an id with special characters', () => { + scrollToPosition({ el: '#special~characters' }) + expect(getElementById).toHaveBeenCalledWith('special~characters') + expect(querySelector).not.toHaveBeenCalled() + expect(scrollTo).toHaveBeenCalledWith({ + left: 0, + top: 0, + behavior: undefined, + }) + }) + + it('scrolls to an id with special characters', () => { + scrollToPosition({ el: '#special~characters' }) + expect(getElementById).toHaveBeenCalledWith('special~characters') + expect(querySelector).not.toHaveBeenCalled() + expect(scrollTo).toHaveBeenCalledWith({ + left: 0, + top: 0, + behavior: undefined, + }) + }) + + it('accepts a raw element', () => { + scrollToPosition({ el: document.getElementById('special~characters')! }) + expect(getElementById).toHaveBeenCalledWith('special~characters') + expect(querySelector).not.toHaveBeenCalled() + expect(scrollTo).toHaveBeenCalledWith({ + left: 0, + top: 0, + behavior: undefined, + }) + }) + + describe('warnings', () => { + beforeEach(() => { + __DEV__ = true + }) + + it('warns if element cannot be found with id', () => { + scrollToPosition({ el: '#not-found' }) + expect( + `Couldn't find element using selector "#not-found"` + ).toHaveBeenWarned() + }) + + it('warns if element cannot be found with selector', () => { + scrollToPosition({ el: '.not-found' }) + expect( + `Couldn't find element using selector ".not-found"` + ).toHaveBeenWarned() + }) + + it('warns if element cannot be found with id but can with selector', () => { + scrollToPosition({ el: '#text .container' }) + expect( + `selector "#text .container" should be passed as "el: document.querySelector('#text .container')"` + ).toHaveBeenWarned() + }) + + it('warns if element cannot be found with id but can with selector', () => { + scrollToPosition({ el: '#text .container' }) + expect( + `selector "#text .container" should be passed as "el: document.querySelector('#text .container')"` + ).toHaveBeenWarned() + }) + + it('warns if querySelector throws', () => { + scrollToPosition({ el: '.container #1' }) + expect(`selector ".container #1" is invalid`).toHaveBeenWarned() + }) + }) + }) +}) diff --git a/e2e/scroll-behavior/index.ts b/e2e/scroll-behavior/index.ts index 7d16aced0..aa809d07f 100644 --- a/e2e/scroll-behavior/index.ts +++ b/e2e/scroll-behavior/index.ts @@ -40,20 +40,15 @@ const scrollBehavior: ScrollBehavior = async function ( // scroll to anchor by returning the selector if (to.hash) { - position = { selector: decodeURI(to.hash), offset: { behavior } } + position = { el: decodeURI(to.hash), behavior } // specify offset of the element if (to.hash === '#anchor2') { - position.offset = { top: 100, behavior } + position.top = 100 + position.behavior = behavior } - if (document.querySelector(position.selector)) { - return position - } - - // if the returned position is falsy or an empty object, - // will retain current scroll position. - return false + return position } // check if any matched route config has meta that requires scrolling to top @@ -86,7 +81,7 @@ const app = createApp({ setup() { return { smoothScroll, - hashWithNumber: { path: '/bar', hash: '#\\31 number' }, + hashWithNumber: { path: '/bar', hash: '#1number' }, flushWaiter: scrollWaiter.flush, setupWaiter: scrollWaiter.add, } diff --git a/src/scrollBehavior.ts b/src/scrollBehavior.ts index 92eb7df20..066b2f879 100644 --- a/src/scrollBehavior.ts +++ b/src/scrollBehavior.ts @@ -30,7 +30,7 @@ export type _ScrollPositionNormalized = { top: number } -export interface ScrollPositionElement { +export interface ScrollPositionElement extends ScrollToOptions { /** * A valid CSS selector. Note some characters must be escaped in id selectors (https://mathiasbynens.be/notes/css-escapes). * @example @@ -43,11 +43,7 @@ export interface ScrollPositionElement { * - `#marker.with.dot`: selects `class="with dot" id="marker"`, not `id="marker.with.dot"` * */ - selector: string - /** - * Relative offset to the `selector` in {@link ScrollPositionCoordinates} - */ - offset?: ScrollPositionCoordinates + el: string | Element } export type ScrollPosition = ScrollPositionCoordinates | ScrollPositionElement @@ -85,7 +81,10 @@ export const computeScrollPosition = () => export function scrollToPosition(position: ScrollPosition): void { let scrollToOptions: ScrollPositionCoordinates - if ('selector' in position) { + if ('el' in position) { + let positionEl = position.el + const isIdSelector = + typeof positionEl === 'string' && positionEl.startsWith('#') /** * `id`s can accept pretty much any characters, including CSS combinators * like `>` or `~`. It's still possible to retrieve elements using @@ -107,24 +106,39 @@ export function scrollToPosition(position: ScrollPosition): void { * https://mathiasbynens.be/notes/html5-id-class. * - Practical example: https://mathiasbynens.be/demo/html5-id */ - if (__DEV__) { - try { - document.querySelector(position.selector) - } catch { - warn( - `The selector "${position.selector}" is invalid. If you are using an id selector, make sure to escape it. You can find more information about escaping characters in selectors at https://mathiasbynens.be/notes/css-escapes.` - ) + if (__DEV__ && typeof position.el === 'string') { + if (!isIdSelector || !document.getElementById(position.el.slice(1))) { + try { + let foundEl = document.querySelector(position.el) + if (isIdSelector && foundEl) { + warn( + `The selector "${position.el}" should be passed as "el: document.querySelector('${position.el}')" because it starts with "#".` + ) + // return to avoid other warnings + return + } + } catch { + warn( + `The selector "${position.el}" is invalid. If you are using an id selector, make sure to escape it. You can find more information about escaping characters in selectors at https://mathiasbynens.be/notes/css-escapes or use CSS.escape (https://developer.mozilla.org/en-US/docs/Web/API/CSS/escape).` + ) + // return to avoid other warnings + return + } } } - const el = document.querySelector(position.selector) + const el = + typeof positionEl === 'string' + ? isIdSelector + ? document.getElementById(positionEl.slice(1)) + : document.querySelector(positionEl) + : positionEl if (!el) { - __DEV__ && - warn(`Couldn't find element with selector "${position.selector}"`) + __DEV__ && warn(`Couldn't find element using selector "${position.el}"`) return } - scrollToOptions = getElementPosition(el, position.offset || {}) + scrollToOptions = getElementPosition(el, position) } else { scrollToOptions = position } @@ -132,8 +146,10 @@ export function scrollToPosition(position: ScrollPosition): void { if ('scrollBehavior' in document.documentElement.style) window.scrollTo(scrollToOptions) else { - // TODO: pass the current value instead of 0 using computeScroll - window.scrollTo(scrollToOptions.left || 0, scrollToOptions.top || 0) + window.scrollTo( + scrollToOptions.left != null ? scrollToOptions.left : window.pageXOffset, + scrollToOptions.top != null ? scrollToOptions.top : window.pageYOffset + ) } }