Skip to content

Commit

Permalink
fix inconsistent scroll restoration behavior (#59366)
Browse files Browse the repository at this point in the history
### What?
While scrolled on a page, and when following a link to a new page and
clicking the browser back button or using `router.back()`, the scroll
position would sometimes restore scroll to the incorrect spot (in the
case of the test added in this PR, it'd scroll you back to the top of
the list)

### Why?
The refactor in #56497 changed the way router actions are processed:
specifically, all actions were assumed to be async, even if they could
be handled synchronously. For most actions this is fine, as most are
currently async. However, `ACTION_RESTORE` (triggered when the
`popstate` event occurs) isn't async, and introducing a small amount of
delay in the handling of this action can cause the browser to not
properly restore the scroll position

### How?
This special-cases `ACTION_RESTORE` to synchronously process the action
and call `setState` when it's received, rather than creating a promise.
To consistently reproduce this behavior, I added an option to our
browser interface that'll allow us to programmatically trigger a CPU
slowdown.

h/t to @alvarlagerlof for isolating the offending commit and sharing a
minimal reproduction.

Closes NEXT-1819
Likely addresses #58899 but the reproduction was too complex to verify.
  • Loading branch information
ztanner authored Dec 7, 2023
1 parent 2874bc0 commit a578cc8
Show file tree
Hide file tree
Showing 9 changed files with 151 additions and 16 deletions.
33 changes: 20 additions & 13 deletions packages/next/src/shared/lib/router/action-queue.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import {
ACTION_REFRESH,
ACTION_SERVER_ACTION,
ACTION_NAVIGATE,
ACTION_RESTORE,
} from '../../../client/components/router-reducer/router-reducer-types'
import type { ReduxDevToolsInstance } from '../../../client/components/use-reducer-with-devtools'
import { reducer } from '../../../client/components/router-reducer/router-reducer'
Expand All @@ -26,7 +27,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
}
Expand Down Expand Up @@ -115,14 +116,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 !== ACTION_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,
Expand All @@ -131,12 +144,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
Expand Down
10 changes: 10 additions & 0 deletions test/e2e/app-dir/navigation/app/scroll-restoration/context.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
import { createContext } from 'react'

export interface Item {
id: number
}

export const ItemsContext = createContext<{
items: Item[]
loadMoreItems: () => void
}>({ items: [], loadMoreItems: () => {} })
27 changes: 27 additions & 0 deletions test/e2e/app-dir/navigation/app/scroll-restoration/layout.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
'use client'
import { useState } from 'react'
import { ItemsContext, type Item } from './context'

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>
)
}
13 changes: 13 additions & 0 deletions test/e2e/app-dir/navigation/app/scroll-restoration/other/page.tsx
Original file line number Diff line number Diff line change
@@ -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>
)
}
23 changes: 23 additions & 0 deletions test/e2e/app-dir/navigation/app/scroll-restoration/page.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
'use client'
import Link from 'next/link'
import React, { useContext } from 'react'
import { ItemsContext } from './context'

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>
)
}
29 changes: 29 additions & 0 deletions test/e2e/app-dir/navigation/navigation.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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)
})
})
}
)
10 changes: 9 additions & 1 deletion test/lib/browsers/base.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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> {}

Expand Down
14 changes: 13 additions & 1 deletion test/lib/browsers/playwright.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Expand Down Expand Up @@ -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
Expand Down
8 changes: 7 additions & 1 deletion test/lib/next-webdriver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ export default async function webdriver(
disableJavaScript?: boolean
headless?: boolean
ignoreHTTPSErrors?: boolean
cpuThrottleRate?: number
}
): Promise<BrowserInterface> {
let CurrentInterface: new () => BrowserInterface
Expand All @@ -87,6 +88,7 @@ export default async function webdriver(
disableJavaScript,
ignoreHTTPSErrors,
headless,
cpuThrottleRate,
} = options

// we import only the needed interface
Expand Down Expand Up @@ -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
Expand Down

0 comments on commit a578cc8

Please sign in to comment.