fix(router): skip scroll saving for unknown pop direction (fix #1431) (#2780)

* fix(router): skip scroll saving for unknown pop direction

* test: no internals

* style: format

Fix #1431

---------

Co-authored-by: Eduardo San Martin Morote <posva13@gmail.com>

Author: ifer47

packages/router/__tests__/router.spec.ts

@@ -698,6 +698,7 @@ describe('Router', () => {
 
     beforeEach(() => {
       scrollTo.mockClear()
+      window.history.replaceState(null, '', '/')
     })
 
     afterAll(() => {
@@ -713,6 +714,46 @@ describe('Router', () => {
       '/p/a': 2000,
     }
 
+    it('does not restore scroll positions for pop navigations with unknown direction', async () => {
+      const scrollBehavior = vi.fn()
+      const { router } = await newRouter({
+        history: createWebHashHistory(),
+        scrollBehavior,
+      })
+      scrollBehavior.mockClear()
+
+      // Plain `<a href="#...">` links and manual `location.hash` writes create
+      // a history entry with no state, so the popstate fires with
+      // `state: null` and the router cannot compute a direction (delta 0).
+      // happy-dom does not fire popstate on hash changes, so dispatch it like
+      // a browser would.
+      function changeHash(hash: string) {
+        window.location.hash = hash
+        window.dispatchEvent(new PopStateEvent('popstate', { state: null }))
+      }
+
+      changeHash('#/foo')
+      await nextNavigation(router)
+      changeHash('#/')
+      await nextNavigation(router)
+
+      expect(scrollBehavior).toHaveBeenCalledTimes(2)
+      expect(scrollBehavior).toHaveBeenNthCalledWith(
+        1,
+        expect.objectContaining({ path: '/foo' }),
+        expect.objectContaining({ path: '/' }),
+        null
+      )
+      // a stale position saved under the unknown-direction key would show up
+      // here as a non-null savedPosition and override the target anchor
+      expect(scrollBehavior).toHaveBeenNthCalledWith(
+        2,
+        expect.objectContaining({ path: '/' }),
+        expect.objectContaining({ path: '/foo' }),
+        null
+      )
+    })
+
     it('ignores the scroll of navigations superseded before they finish', async () => {
       // scrollBehavior resolves asynchronously, simulating waiting for the DOM
       const scrollBehavior = vi.fn((to: { path: string }) => {

packages/router/e2e/specs/scroll-behavior.spec.ts

@@ -23,6 +23,35 @@ const anchorTop = (page: Page, id: string) =>
   )
 
 test.describe('scroll-behavior', () => {
+  test('does not restore saved positions for manual hash changes', async ({
+    page,
+  }) => {
+    await page.goto('/scroll-behavior/bar')
+    await expect(page.locator('.view.bar')).toBeVisible()
+
+    await page.evaluate(() => {
+      window.location.hash = '#anchor'
+    })
+    await expect.poll(() => anchorTop(page, 'anchor')).toBeLessThan(1)
+
+    await page.evaluate(() => {
+      window.location.hash = '#anchor2'
+    })
+    await expect.poll(() => anchorTop(page, 'anchor2')).toBeLessThan(101)
+
+    // Make a stale saved position visibly different from the anchor target.
+    await page.evaluate(() => {
+      window.scrollTo(0, 150)
+      window.location.hash = '#anchor'
+    })
+    await expect.poll(() => anchorTop(page, 'anchor')).toBeLessThan(1)
+
+    await page.evaluate(() => {
+      window.location.hash = '#anchor2'
+    })
+    await expect.poll(() => anchorTop(page, 'anchor2')).toBeLessThan(101)
+  })
+
   test('scroll behavior', async ({ page }) => {
     await page.goto('/scroll-behavior/')
     await expect(

packages/router/src/experimental/router.spec.ts

@@ -1002,6 +1002,7 @@ describe('Experimental Router', () => {
 
     beforeEach(() => {
       scrollTo.mockClear()
+      window.history.replaceState(null, '', '/')
     })
 
     afterAll(() => {
@@ -1017,6 +1018,46 @@ describe('Experimental Router', () => {
       '/p/a': 2000,
     }
 
+    it('does not restore scroll positions for pop navigations with unknown direction', async () => {
+      const scrollBehavior = vi.fn()
+      const { router } = await newRouter({
+        history: createWebHashHistory(),
+        scrollBehavior,
+      })
+      scrollBehavior.mockClear()
+
+      // Plain `<a href="#...">` links and manual `location.hash` writes create
+      // a history entry with no state, so the popstate fires with
+      // `state: null` and the router cannot compute a direction (delta 0).
+      // happy-dom does not fire popstate on hash changes, so dispatch it like
+      // a browser would.
+      function changeHash(hash: string) {
+        window.location.hash = hash
+        window.dispatchEvent(new PopStateEvent('popstate', { state: null }))
+      }
+
+      changeHash('#/foo')
+      await nextNavigation(router)
+      changeHash('#/')
+      await nextNavigation(router)
+
+      expect(scrollBehavior).toHaveBeenCalledTimes(2)
+      expect(scrollBehavior).toHaveBeenNthCalledWith(
+        1,
+        expect.objectContaining({ path: '/foo' }),
+        expect.objectContaining({ path: '/' }),
+        null
+      )
+      // a stale position saved under the unknown-direction key would show up
+      // here as a non-null savedPosition and override the target anchor
+      expect(scrollBehavior).toHaveBeenNthCalledWith(
+        2,
+        expect.objectContaining({ path: '/' }),
+        expect.objectContaining({ path: '/foo' }),
+        null
+      )
+    })
+
     it('ignores the scroll of navigations superseded before they finish', async () => {
       // scrollBehavior resolves asynchronously, simulating waiting for the DOM
       const scrollBehavior = vi.fn((to: { path: string }) => {

packages/router/src/experimental/router.ts

@@ -1107,8 +1107,9 @@ export function experimental_createRouter(
       pendingLocation = toLocation
       const from = currentRoute.value
 
+      // Unknown-direction navigations cannot be tied to a history entry.
       // TODO: should be moved to web history?
-      if (isBrowser) {
+      if (isBrowser && info.delta) {
         saveScrollPosition(getScrollKey(from.fullPath, info.delta))
       }
 

packages/router/src/router.ts

@@ -814,8 +814,9 @@ export function createRouter(options: RouterOptions): Router {
       pendingLocation = toLocation
       const from = currentRoute.value
 
+      // Unknown-direction navigations cannot be tied to a history entry.
       // TODO: should be moved to web history?
-      if (isBrowser) {
+      if (isBrowser && info.delta) {
         saveScrollPosition(getScrollKey(from.fullPath, info.delta))
       }