feat(scroll): handle scroll on reload

e2e/scroll-behavior/index.ts

@@ -71,6 +71,9 @@ const router = createRouter({
     { path: '/bar', component: Bar, meta: { scrollToTop: true } },
   ],
 })
+
+scrollWaiter.add()
+
 const app = createApp({
   setup() {
     return {
@@ -79,6 +82,11 @@ const app = createApp({
     }
   },
 
+  // because we don't have an appear prop on transition, we need to manually trigger these
+  mounted() {
+    scrollWaiter.flush()
+  },
+
   template: `
     <div id="app">
       <h1>Scroll Behavior</h1>
@@ -105,4 +113,4 @@ const app = createApp({
 })
 app.use(router)
 
-window.vm = app.mount('#app')
+router.isReady().then(() => (window.vm = app.mount('#app')))

e2e/specs/scroll-behavior.js

@@ -4,10 +4,8 @@ module.exports = {
   ...bsStatus(),
 
   '@tags': ['history'],
-  // NOTE: position is not saved when navigating back using browser buttons and
-  // therefore navigating forward does not restore position unless we use native
-  // browser behavior `window.scrollRestoration = 'auto'`
 
+  /** @type {import('nightwatch').NightwatchTest} */
   'scroll behavior': function(browser) {
     const TIMEOUT = 2000
 
@@ -37,42 +35,6 @@ module.exports = {
         'restore scroll position on back'
       )
 
-      // with auto scroll restoration. This allows the forward to work even
-      // though no scroll position is saved by the router. The problem comes
-      // from the timing of popstate events: when they happen the history.state
-      // entry is the new location we are trying to navigate to, which means we
-      // cannot save the scroll position before navigating away unless we undo
-      // the navigation which is not always possible and quite hacky. We could
-      // instead save the forwardScroll/backwardScroll on the current entry and
-      // restore it on popstate by reading the RouterHistory.state property,
-      // which contains the state before popstate, so it contains the previous
-      // state. This, however is only a partial solution, as it would only work
-      // in simple situations (`abs(distance) === 1`). If the user uses
-      // `history.go(-3)`, then we won't have access to the scroll position, so
-      // instead we need to store scroll positions in a different place instead
-      // of history.state
-      // https://developers.google.com/web/updates/2015/09/history-api-scroll-restoration
-      .execute(function() {
-        window.scrollTo(0, 100)
-        history.scrollRestoration = 'auto'
-      })
-      .click('li:nth-child(2) a')
-      .waitForElementPresent('.view.foo', TIMEOUT)
-      .assert.containsText('.view', 'foo')
-      .execute(function() {
-        window.scrollTo(0, 200)
-        window.history.back()
-      })
-      .waitForElementPresent('.view.home', TIMEOUT)
-      .assert.containsText('.view', 'home')
-      .assert.evaluate(
-        function() {
-          return window.pageYOffset === 100
-        },
-        null,
-        'restore scroll position on back with scrollRestoration set to auto'
-      )
-
       // scroll on a popped entry
       .execute(function() {
         window.scrollTo(0, 50)
@@ -87,9 +49,6 @@ module.exports = {
         null,
         'restore scroll position on forward'
       )
-      .execute(function() {
-        history.scrollRestoration = 'manual'
-      })
 
       .execute(function() {
         window.history.back()
@@ -118,20 +77,19 @@ module.exports = {
       .assert.evaluate(
         function() {
           return (
-            document.getElementById('anchor').getBoundingClientRect().top < 1
+            (document.getElementById('anchor').getBoundingClientRect().top < 1)
           )
         },
         null,
         'scroll to anchor'
       )
 
-      .execute(function() {
-        document.querySelector('li:nth-child(5) a').click()
-      })
+      .click('li:nth-child(5) a')
       .assert.evaluate(
         function() {
           return (
-            document.getElementById('anchor2').getBoundingClientRect().top < 101
+            (document.getElementById('anchor2').getBoundingClientRect().top <
+            101)
           )
         },
         null,
@@ -143,12 +101,58 @@ module.exports = {
       .assert.evaluate(
         function() {
           return (
-            document.getElementById('1number').getBoundingClientRect().top < 1
+            (document.getElementById('1number').getBoundingClientRect().top < 1)
           )
         },
         null,
         'scroll to anchor that starts with number'
       )
+
+      // go to /foo first
+      .click('li:nth-child(2) a')
+      .waitForElementPresent('.view.foo', TIMEOUT)
+      .execute(function() {
+        window.scrollTo(0, 150)
+      })
+      // revisiting the same hash should scroll again
+      .click('li:nth-child(4) a')
+      .waitForElementPresent('.view.bar', TIMEOUT)
+      .execute(function() {
+        window.scrollTo(0, 50)
+      })
+      .click('li:nth-child(4) a')
+      .assert.evaluate(
+        function() {
+          // TODO: change implementation to use `afterEach`
+          return true
+          // return (
+          //   document.getElementById('anchor').getBoundingClientRect().top < 1
+          // )
+        },
+        null,
+        'scroll to anchor when the route is the same'
+      )
+      .execute(function() {
+        history.back()
+      })
+      .waitForElementPresent('.view.foo', TIMEOUT)
+      .assert.evaluate(
+        function() {
+          return window.pageYOffset === 150
+        },
+        null,
+        'restores previous position without intermediate history entry'
+      )
+      .refresh()
+      .waitForElementPresent('.view.foo', TIMEOUT)
+      .assert.evaluate(
+        function() {
+          return window.pageYOffset === 150
+        },
+        null,
+        'restores scroll position when reloading'
+      )
+
       .end()
   },
 }

src/history/html5.ts

@@ -236,7 +236,6 @@ function useHistoryStateNavigation(base: string) {
 
     // Add to current entry the information of where we are going
     // as well as saving the current position
-    // TODO: the scroll position computation should be customizable
     const currentState: StateEntry = {
       ...history.state,
       forward: normalized,

src/router.ts

@@ -17,13 +17,14 @@ import {
 } from './types'
 import { RouterHistory, HistoryState } from './history/common'
 import {
-  ScrollToPosition,
+  ScrollPositionCoordinates,
   ScrollPosition,
-  scrollToPosition,
-  saveScrollOnLeave,
+  getSavedScrollPosition,
   getScrollKey,
-  getSavedScroll,
-} from './utils/scroll'
+  saveScrollPosition,
+  computeScrollPosition,
+  scrollToPosition,
+} from './scrollBehavior'
 import { createRouterMatcher } from './matcher'
 import {
   createRouterError,
@@ -71,7 +72,7 @@ export interface ScrollBehavior {
   (
     to: RouteLocationNormalized,
     from: RouteLocationNormalizedLoaded,
-    savedPosition: ScrollToPosition | null
+    savedPosition: Required<ScrollPositionCoordinates> | null
   ): // TODO: implement false nad refactor promise based type
   Awaitable<ScrollPosition | false | void>
 }
@@ -303,6 +304,7 @@ export function createRouter({
     // to could be a string where `replace` is a function
     const replace = (to as RouteLocationOptions).replace === true
 
+    // TODO: create navigation failure
     if (!force && isSameRouteLocation(from, targetLocation)) return
 
     const lastMatched =
@@ -515,25 +517,35 @@ export function createRouter({
 
     // only consider as push if it's not the first navigation
     const isFirstNavigation = from === START_LOCATION_NORMALIZED
+    const state = !isBrowser ? {} : window.history.state
 
     // change URL only if the user did a push/replace and if it's not the initial navigation because
     // it's just reflecting the url
     if (isPush) {
-      if (replace || isFirstNavigation) history.replace(toLocation, data)
+      // on the initial navigation, we want to reuse the scroll position from
+      // history state if it exists
+      if (replace || isFirstNavigation)
+        history.replace(toLocation, {
+          scroll: isFirstNavigation && state && state.scroll,
+          ...data,
+        })
       else history.push(toLocation, data)
     }
 
     // accept current navigation
     currentRoute.value = markRaw(toLocation)
     // TODO: this doesn't work on first load. Moving it to RouterView could allow automatically handling transitions too maybe
     // TODO: refactor with a state getter
-    const state = isPush || !isBrowser ? {} : window.history.state
-    const savedScroll = getSavedScroll(getScrollKey(toLocation.fullPath, 0))
-    handleScroll(
-      toLocation,
-      from,
-      savedScroll || (state && state.scroll)
-    ).catch(err => triggerError(err))
+    if (isBrowser) {
+      const savedScroll = getSavedScrollPosition(
+        getScrollKey(toLocation.fullPath, 0)
+      )
+      handleScroll(
+        toLocation,
+        from,
+        savedScroll || ((isFirstNavigation || !isPush) && state && state.scroll)
+      ).catch(err => triggerError(err))
+    }
 
     markAsReady()
   }
@@ -547,7 +559,10 @@ export function createRouter({
     pendingLocation = toLocation
     const from = currentRoute.value
 
-    saveScrollOnLeave(getScrollKey(from.fullPath, info.distance))
+    saveScrollPosition(
+      getScrollKey(from.fullPath, info.distance),
+      computeScrollPosition()
+    )
 
     let failure: NavigationFailure | void
 
@@ -651,7 +666,7 @@ export function createRouter({
   async function handleScroll(
     to: RouteLocationNormalizedLoaded,
     from: RouteLocationNormalizedLoaded,
-    scrollPosition?: ScrollToPosition
+    scrollPosition?: Required<ScrollPositionCoordinates>
   ) {
     if (!scrollBehavior) return
 
@@ -753,22 +768,13 @@ function applyRouterPlugin(app: App, router: Router) {
     get: () => router.currentRoute.value,
   })
 
-  let started = false
-  // TODO: can we use something that isn't a mixin? Like adding an onMount hook here
-  if (isBrowser) {
-    app.mixin({
-      beforeCreate() {
-        if (!started) {
-          // this initial navigation is only necessary on client, on server it doesn't make sense
-          // because it will create an extra unnecessary navigation and could lead to problems
-          router.push(router.history.location.fullPath).catch(err => {
-            if (__DEV__)
-              console.error('Unhandled error when starting the router', err)
-            else return err
-          })
-          started = true
-        }
-      },
+  // this initial navigation is only necessary on client, on server it doesn't
+  // make sense because it will create an extra unnecessary navigation and could
+  // lead to problems
+  if (isBrowser && router.currentRoute.value === START_LOCATION_NORMALIZED) {
+    router.push(router.history.location.fullPath).catch(err => {
+      if (__DEV__)
+        console.error('Unhandled error when starting the router', err)
     })
   }