fix inconsistent scroll restoration behavior

Author: ztanner

packages/next/src/shared/lib/router/action-queue.ts

@@ -26,7 +26,7 @@ export type AppRouterActionQueue = {
 export type ActionQueueNode = {
   payload: ReducerActions
   next: ActionQueueNode | null
-  resolve: (value: PromiseLike<AppRouterState> | AppRouterState) => void
+  resolve: (value: ReducerState) => void
   reject: (err: Error) => void
   discarded?: boolean
 }
@@ -115,14 +115,26 @@ function dispatchAction(
   setState: DispatchStatePromise
 ) {
   let resolvers: {
-    resolve: (value: AppRouterState | PromiseLike<AppRouterState>) => void
+    resolve: (value: ReducerState) => void
     reject: (reason: any) => void
-  }
+  } = { resolve: setState, reject: () => {} }
+
+  // most of the action types are async with the exception of restore
+  // it's important that restore is handled quickly since it's fired on the popstate event
+  // and we don't want to add any delay on a back/forward nav
+  // this only creates a promise for the async actions
+  if (payload.type !== 'restore') {
+    // Create the promise and assign the resolvers to the object.
+    const deferredPromise = new Promise<AppRouterState>((resolve, reject) => {
+      resolvers = { resolve, reject }
+    })
 
-  // Create the promise and assign the resolvers to the object.
-  const deferredPromise = new Promise<AppRouterState>((resolve, reject) => {
-    resolvers = { resolve, reject }
-  })
+    startTransition(() => {
+      // we immediately notify React of the pending promise -- the resolver is attached to the action node
+      // and will be called when the associated action promise resolves
+      setState(deferredPromise)
+    })
+  }
 
   const newAction: ActionQueueNode = {
     payload,
@@ -131,12 +143,6 @@ function dispatchAction(
     reject: resolvers!.reject,
   }
 
-  startTransition(() => {
-    // we immediately notify React of the pending promise -- the resolver is attached to the action node
-    // and will be called when the associated action promise resolves
-    setState(deferredPromise)
-  })
-
   // Check if the queue is empty
   if (actionQueue.pending === null) {
     // The queue is empty, so add the action and start it immediately

test/e2e/app-dir/navigation/app/scroll-restoration/layout.tsx

@@ -0,0 +1,35 @@
+'use client'
+import { useState, createContext } from 'react'
+
+interface Item {
+  id: number
+}
+
+export const ItemsContext = createContext<{
+  items: Item[]
+  loadMoreItems: () => void
+}>({ items: [], loadMoreItems: () => {} })
+
+const createItems = (start: number, end: number): Item[] => {
+  const items: Item[] = []
+  for (let i = start; i <= end; i++) {
+    items.push({ id: i })
+  }
+  return items
+}
+
+export default function Layout({ children }: { children: React.ReactNode }) {
+  const [items, setItems] = useState<Item[]>(createItems(1, 50))
+
+  const loadMoreItems = () => {
+    const start = items.length + 1
+    const end = start + 50 - 1
+    setItems((prevItems) => [...prevItems, ...createItems(start, end)])
+  }
+
+  return (
+    <ItemsContext.Provider value={{ items, loadMoreItems }}>
+      {children}
+    </ItemsContext.Provider>
+  )
+}

test/e2e/app-dir/navigation/app/scroll-restoration/other/page.tsx

@@ -0,0 +1,13 @@
+'use client'
+import { useRouter } from 'next/navigation'
+
+export default function Page() {
+  const router = useRouter()
+  return (
+    <div>
+      <button onClick={() => router.back()} id="back-button">
+        Go Back
+      </button>
+    </div>
+  )
+}

test/e2e/app-dir/navigation/app/scroll-restoration/page.tsx

@@ -0,0 +1,23 @@
+'use client'
+import Link from 'next/link'
+import React, { useContext, useState } from 'react'
+import { ItemsContext } from './layout'
+
+export default function Page() {
+  const { items, loadMoreItems } = useContext(ItemsContext)
+
+  return (
+    <div>
+      <h1>Page</h1>
+      <ul>
+        {items.map((item) => (
+          <li key={item.id}>Item {item.id}</li>
+        ))}
+      </ul>
+      <button id="load-more" onClick={loadMoreItems}>
+        Load More
+      </button>
+      <Link href="/scroll-restoration/other">Go to Other</Link>
+    </div>
+  )
+}

test/e2e/app-dir/navigation/navigation.test.ts

@@ -732,5 +732,34 @@ createNextDescribe(
         }
       })
     })
+
+    describe('scroll restoration', () => {
+      it('should restore original scroll position when navigating back', async () => {
+        const browser = await next.browser('/scroll-restoration', {
+          // throttling the CPU to rule out flakiness based on how quickly the page loads
+          cpuThrottleRate: 6,
+        })
+        const body = await browser.elementByCss('body')
+        expect(await body.text()).toContain('Item 50')
+        await browser.elementById('load-more').click()
+        await browser.elementById('load-more').click()
+        await browser.elementById('load-more').click()
+        expect(await body.text()).toContain('Item 200')
+
+        // scroll to the bottom of the page
+        await browser.eval('window.scrollTo(0, document.body.scrollHeight)')
+
+        // grab the current position
+        const scrollPosition = await browser.eval('window.pageYOffset')
+
+        await browser.elementByCss("[href='/scroll-restoration/other']").click()
+        await browser.elementById('back-button').click()
+
+        const newScrollPosition = await browser.eval('window.pageYOffset')
+
+        // confirm that the scroll position was restored
+        await check(() => scrollPosition === newScrollPosition, true)
+      })
+    })
   }
 )

test/lib/browsers/base.ts

@@ -125,7 +125,15 @@ export abstract class BrowserInterface implements PromiseLike<any> {
   off(event: Event, cb: (...args: any[]) => void) {}
   async loadPage(
     url: string,
-    { disableCache: boolean, beforePageLoad: Function }
+    {
+      disableCache,
+      cpuThrottleRate,
+      beforePageLoad,
+    }: {
+      disableCache?: boolean
+      cpuThrottleRate?: number
+      beforePageLoad?: Function
+    }
   ): Promise<void> {}
   async get(url: string): Promise<void> {}
 

test/lib/browsers/playwright.ts

@@ -128,7 +128,11 @@ export class Playwright extends BrowserInterface {
 
   async loadPage(
     url: string,
-    opts?: { disableCache: boolean; beforePageLoad?: (...args: any[]) => void }
+    opts?: {
+      disableCache: boolean
+      cpuThrottleRate: number
+      beforePageLoad?: (...args: any[]) => void
+    }
   ) {
     if (this.activeTrace) {
       const traceDir = path.join(__dirname, '../../traces')
@@ -182,6 +186,14 @@ export class Playwright extends BrowserInterface {
       session.send('Network.setCacheDisabled', { cacheDisabled: true })
     }
 
+    if (opts.cpuThrottleRate) {
+      const session = await context.newCDPSession(page)
+      // https://chromedevtools.github.io/devtools-protocol/tot/Emulation/#method-setCPUThrottlingRate
+      session.send('Emulation.setCPUThrottlingRate', {
+        rate: opts.cpuThrottleRate,
+      })
+    }
+
     page.on('websocket', (ws) => {
       if (tracePlaywright) {
         page

test/lib/next-webdriver.ts

@@ -68,6 +68,7 @@ export default async function webdriver(
     disableJavaScript?: boolean
     headless?: boolean
     ignoreHTTPSErrors?: boolean
+    cpuThrottleRate?: number
   }
 ): Promise<BrowserInterface> {
   let CurrentInterface: new () => BrowserInterface
@@ -87,6 +88,7 @@ export default async function webdriver(
     disableJavaScript,
     ignoreHTTPSErrors,
     headless,
+    cpuThrottleRate,
   } = options
 
   // we import only the needed interface
@@ -124,7 +126,11 @@ export default async function webdriver(
 
   console.log(`\n> Loading browser with ${fullUrl}\n`)
 
-  await browser.loadPage(fullUrl, { disableCache, beforePageLoad })
+  await browser.loadPage(fullUrl, {
+    disableCache,
+    cpuThrottleRate,
+    beforePageLoad,
+  })
   console.log(`\n> Loaded browser with ${fullUrl}\n`)
 
   // Wait for application to hydrate