refactor onBrowserHistoryNavigation (#1925)

vike/client/client-routing-runtime/history.ts

@@ -1,5 +1,5 @@
 export {
-  initHistoryState,
+  enhanceHistoryState,
   getHistoryState,
   pushHistory,
   type ScrollPosition,
@@ -27,28 +27,20 @@ type StateNotInitialized =
   // Already enhanced
   | StateVikeEnhanced
 
-// `window.history.state` can be uninitialized (i.e. `null`):
+// `window.history.state === null` when:
 // - The very first render
-// - The user's code runs `location.hash = '#section'`
-// - The user clicks on an anchor link `<a href="#section">Section</a>` (Vike's `initOnLinkClick()` handler skips hash links).
-function initHistoryState() {
+// - Click on `<a href="#some-hash" />`
+// - `location.hash = 'some-hash'`
+function enhanceHistoryState() {
   const stateNotInitialized: StateNotInitialized = window.history.state
-
-  const stateVikeEnhanced = enhanceState(stateNotInitialized)
-
+  if (isVikeEnhanced(stateNotInitialized)) return
+  const stateVikeEnhanced = enhance(stateNotInitialized)
   replaceHistoryState(stateVikeEnhanced)
 }
-
-function enhanceState(stateNotInitialized: StateNotInitialized): StateVikeEnhanced {
-  // Already enhanced
-  if (isVikeEnhanced(stateNotInitialized)) {
-    return stateNotInitialized
-  }
-
+function enhance(stateNotInitialized: StateNotInitialized): StateVikeEnhanced {
   const timestamp = getTimestamp()
   const scrollPosition = getScrollPosition()
   const triggeredBy = 'browser'
-
   let stateVikeEnhanced: StateVikeEnhanced
   if (!stateNotInitialized) {
     stateVikeEnhanced = {
@@ -66,7 +58,6 @@ function enhanceState(stateNotInitialized: StateNotInitialized): StateVikeEnhanc
       _isVikeEnhanced: true
     }
   }
-
   assert(isVikeEnhanced(stateVikeEnhanced))
   return stateVikeEnhanced
 }

vike/client/client-routing-runtime/initClientRouter.ts

@@ -1,7 +1,7 @@
 export { initClientRouter }
 
 import { assert } from './utils.js'
-import { initHistoryState, monkeyPatchHistoryPushState } from './history.js'
+import { enhanceHistoryState, monkeyPatchHistoryPushState } from './history.js'
 import { getRenderCount, renderPageClientSide } from './renderPageClientSide.js'
 import { onBrowserHistoryNavigation } from './onBrowserHistoryNavigation.js'
 import { initOnLinkClick } from './initOnLinkClick.js'
@@ -37,7 +37,7 @@ async function renderFirstPage() {
 
 function initHistoryAndScroll() {
   setupNativeScrollRestoration()
-  initHistoryState()
+  enhanceHistoryState()
   autoSaveScrollPosition()
   monkeyPatchHistoryPushState()
   // Handle back-/forward navigation

vike/client/client-routing-runtime/onBrowserHistoryNavigation.ts

@@ -1,8 +1,8 @@
 export { onBrowserHistoryNavigation }
 export { updateState }
 
-import { getCurrentUrl, getGlobalObject } from './utils.js'
-import { initHistoryState, getHistoryState } from './history.js'
+import { assert, getCurrentUrl, getGlobalObject } from './utils.js'
+import { enhanceHistoryState, getHistoryState } from './history.js'
 import { renderPageClientSide } from './renderPageClientSide.js'
 import { type ScrollTarget, setScrollPosition } from './setScrollPosition.js'
 
@@ -14,49 +14,55 @@ function onBrowserHistoryNavigation() {
   //     - By user clicking on his browser's back-/forward navigation (or using a shortcut)
   //     - By JavaScript: `history.back()` / `history.forward()`
   //   - URL hash change.
-  //     - By user clicking on a hash link `<a href="#some-hash" />`
+  //     - Click on `<a href="#some-hash" />`
   //       - The popstate event is *only* triggered if `href` starts with '#' (even if `href` is '/#some-hash' while the current URL's pathname is '/' then the popstate still isn't triggered)
-  //     - By JavaScript: `location.hash = 'some-hash'`
+  //     - `location.hash = 'some-hash'`
   // - The `event` argument of `window.addEventListener('popstate', (event) => /*...*/)` is useless: the History API doesn't provide the previous state (the popped state), see https://stackoverflow.com/questions/48055323/is-history-state-always-the-same-as-popstate-event-state
   window.addEventListener('popstate', async (): Promise<undefined> => {
+    const { previousState } = globalObject
     const currentState = getState()
+    globalObject.previousState = currentState
 
     const scrollTarget: ScrollTarget = currentState.historyState?.scrollPosition || undefined
 
     const isUserLandPushStateNavigation = currentState.historyState?.triggeredBy === 'user'
 
-    const isHashNavigation = currentState.urlWithoutHash === globalObject.previousState.urlWithoutHash
+    const isHashNavigation = currentState.urlWithoutHash === previousState.urlWithoutHash
 
     const isBackwardNavigation =
-      !currentState.historyState?.timestamp || !globalObject.previousState.historyState?.timestamp
+      !currentState.historyState?.timestamp || !previousState.historyState?.timestamp
         ? null
-        : currentState.historyState.timestamp < globalObject.previousState.historyState.timestamp
+        : currentState.historyState.timestamp < previousState.historyState.timestamp
 
-    globalObject.previousState = currentState
+    // - `history.state === null` when:
+    //   - Click on `<a href="#some-hash" />` (note that Vike's `initOnLinkClick()` handler skips hash links)
+    //   - `location.hash = 'some-hash'`
+    // - `history.state !== null` when `popstate` was triggered by the user clicking on his browser's forward/backward history button.
+    let isHashNavigationNew = isHashNavigation && window.history.state === null
+    if (window.history.state === null) {
+      assert(isHashNavigation)
+      // The browser already scrolled to `#${hash}` => the current scroll position is the right one => we save it with `enhanceHistoryState()`.
+      enhanceHistoryState()
+      globalObject.previousState = getState()
+    }
 
+    // We have to scroll ourselves because we use `window.history.scrollRestoration = 'manual'`. So far this seems to work. Alternatives in case it doesn't work:
+    // - Alternative: we use `window.history.scrollRestoration = 'auto'`
+    //   - Problem: I don't think it's possbible to set `window.history.scrollRestoration = 'auto'` only for hash navigation and not for non-hash navigations?
+    //   - Problem: inconsistencies between browsers? For example specification says that setting `window.history.scrollRestoration` only affects the current entry in the session history but this contradicts what people are experiencing in practice.
+    //     - Specification: https://html.spec.whatwg.org/multipage/history.html#the-history-interface
+    //     - Practice: https://stackoverflow.com/questions/70188241/history-scrollrestoration-manual-doesnt-prevent-safari-from-restoring-scrol
+    // - Alternative: we completely take over hash navigation and reproduce the browser's native behavior upon hash navigation.
+    //   - By using the `hashchange` event.
+    //   - Problem: conflict if user wants to override the browser's default behavior? E.g. for smooth scrolling, or when using hashes for saving states of some fancy animations.
     if (isHashNavigation && !isUserLandPushStateNavigation) {
-      // - `history.state` is uninitialized (`null`) when:
-      //   - The user's code runs `window.location.hash = '#section'`.
-      //   - The user clicks on an anchor link `<a href="#section">Section</a>` (because Vike's `initOnLinkClick()` handler skips hash links).
-      // - Alternatively, we completely take over hash navigation and reproduce the browser's native behavior upon hash navigation.
-      //   - Problem: we cannot intercept `window.location.hash = '#section'`. (Or maybe we can with the `hashchange` event?)
-      //   - Other potential problem: would there be a conflict when the user wants to override the browser's default behavior? E.g. for smooth scrolling, or when using hashes for saving states of some fancy animations.
-      // - Another alternative: we use the browser's scroll restoration mechanism (see `browserNativeScrollRestoration_enable()` below).
-      //   - Problem: not clear when to call `browserNativeScrollRestoration_disable()`/`browserNativeScrollRestoration_enable()`
-      //   - Other potential problem are inconsistencies between browsers: specification says that setting `window.history.scrollRestoration` only affects the current entry in the session history. But this seems to contradict what folks saying.
-      //     - Specification: https://html.spec.whatwg.org/multipage/history.html#the-history-interface
-      //     - https://stackoverflow.com/questions/70188241/history-scrollrestoration-manual-doesnt-prevent-safari-from-restoring-scrol
-      if (window.history.state === null) {
-        // The browser already scrolled to `#${hash}` => the current scroll position is the right one => we save it with `initHistoryState()`.
-        initHistoryState()
-        globalObject.previousState = getState()
-      } else {
-        // If `history.state !== null` then it means that `popstate` was triggered by the user clicking on his browser's forward/backward history button.
+      if (!isHashNavigationNew) {
         setScrollPosition(scrollTarget)
       }
-    } else {
-      await renderPageClientSide({ scrollTarget, isBackwardNavigation, isUserLandPushStateNavigation })
+      return
     }
+
+    await renderPageClientSide({ scrollTarget, isBackwardNavigation, isUserLandPushStateNavigation })
   })
 }