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`

diff --git a/__tests__/scrollBehavior.spec.ts b/__tests__/scrollBehavior.spec.ts
new file mode 100644 (file)
index 0000000..96b5dd1
--- /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 7d16aced085df647611d01ee28b56ba5021e3303..aa809d07f1cf53ff1fba8a5b6d629dbfb92ff288 100644 (file)
--- 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 92eb7df203c9ba6c866257bc8d2cd252e5391a75..066b2f879e50f16417df96b0fb4c22f6a167de62 100644 (file)
--- 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
+    )
   }
 }