Skip to content

Commit b4f726a

Browse files
eps1loneps1lon
committed
Fix scroll restoration bugs
1 parent fdb93c7 commit b4f726a

File tree

7 files changed

+246
-58
lines changed

7 files changed

+246
-58
lines changed

‎packages/next/src/client/components/layout-router.tsx‎

Lines changed: 127 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -16,11 +16,14 @@ import {
1616
} from './router-reducer/router-reducer-types'
1717

1818
import React, {
19+
Fragment,
1920
useContext,
2021
use,
2122
startTransition,
2223
Suspense,
2324
useDeferredValue,
25+
useLayoutEffect,
26+
type FragmentInstance,
2427
type JSX,
2528
} from 'react'
2629
import ReactDOM from 'react-dom'
@@ -46,6 +49,8 @@ const Activity = process.env.__NEXT_ROUTER_BF_CACHE
4649
? (require('react') as typeof import('react')).unstable_Activity
4750
: null!
4851

52+
const enableNewScrollHandler = process.env.__NEXT_APP_NEW_SCROLL_HANDLER
53+
4954
/**
5055
* Add refetch marker to router state at the point of the current layout segment.
5156
* This ensures the response returned is not further down than the current layout segment.
@@ -153,9 +158,23 @@ function shouldSkipElement(element: HTMLElement) {
153158
/**
154159
* Check if the top corner of the HTMLElement is in the viewport.
155160
*/
156-
function topOfElementInViewport(element: HTMLElement, viewportHeight: number) {
157-
const rect = element.getBoundingClientRect()
158-
return rect.top >= 0 && rect.top <= viewportHeight
161+
function topOfElementInViewport(
162+
instance: HTMLElement | FragmentInstance,
163+
viewportHeight: number
164+
) {
165+
const rects = instance.getClientRects()
166+
if (rects.length === 0) {
167+
// Just to be explicit.
168+
return false
169+
}
170+
let elementTop = Number.POSITIVE_INFINITY
171+
for (let i = 0; i < rects.length; i++) {
172+
const rect = rects[i]
173+
if (rect.top < elementTop) {
174+
elementTop = rect.top
175+
}
176+
}
177+
return elementTop >= 0 && elementTop <= viewportHeight
159178
}
160179

161180
/**
@@ -182,7 +201,7 @@ interface ScrollAndFocusHandlerProps {
182201
children: React.ReactNode
183202
segmentPath: FlightSegmentPath
184203
}
185-
class InnerScrollAndFocusHandler extends React.Component<ScrollAndFocusHandlerProps> {
204+
class InnerScrollAndFocusHandlerOld extends React.Component<ScrollAndFocusHandlerProps> {
186205
handlePotentialScroll = () => {
187206
// Handle scroll and focus, it's only applied once.
188207
const { focusAndScrollRef, segmentPath } = this.props
@@ -227,9 +246,9 @@ class InnerScrollAndFocusHandler extends React.Component<ScrollAndFocusHandlerPr
227246
while (!(domNode instanceof HTMLElement) || shouldSkipElement(domNode)) {
228247
if (process.env.NODE_ENV !== 'production') {
229248
if (domNode.parentElement?.localName === 'head') {
230-
// TODO: We enter this state when metadata was rendered as part of the page or via Next.js.
249+
// We enter this state when metadata was rendered as part of the page or via Next.js.
231250
// This is always a bug in Next.js and caused by React hoisting metadata.
232-
// We need to replace `findDOMNode` in favor of Fragment Refs (when available) so that we can skip over metadata.
251+
// Fixed with `experimental.appNewScrollHandler`
233252
}
234253
}
235254

@@ -303,6 +322,108 @@ class InnerScrollAndFocusHandler extends React.Component<ScrollAndFocusHandlerPr
303322
}
304323
}
305324

325+
function InnerScrollAndFocusHandlerNew(props: ScrollAndFocusHandlerProps) {
326+
const childrenRef = React.useRef<FragmentInstance>(null)
327+
328+
useLayoutEffect(
329+
() => {
330+
const { focusAndScrollRef, segmentPath } = props
331+
// Handle scroll and focus, it's only applied once in the first useEffect that triggers that changed.
332+
333+
if (focusAndScrollRef.apply) {
334+
// segmentPaths is an array of segment paths that should be scrolled to
335+
// if the current segment path is not in the array, the scroll is not applied
336+
// unless the array is empty, in which case the scroll is always applied
337+
if (
338+
focusAndScrollRef.segmentPaths.length !== 0 &&
339+
!focusAndScrollRef.segmentPaths.some((scrollRefSegmentPath) =>
340+
segmentPath.every((segment, index) =>
341+
matchSegment(segment, scrollRefSegmentPath[index])
342+
)
343+
)
344+
) {
345+
return
346+
}
347+
348+
let instance: FragmentInstance | HTMLElement | null = null
349+
const hashFragment = focusAndScrollRef.hashFragment
350+
351+
if (hashFragment) {
352+
instance = getHashFragmentDomNode(hashFragment)
353+
}
354+
355+
if (!instance) {
356+
instance = childrenRef.current
357+
}
358+
359+
// If there is no DOM node this layout-router level is skipped. It'll be handled higher-up in the tree.
360+
if (instance === null) {
361+
return
362+
}
363+
364+
// State is mutated to ensure that the focus and scroll is applied only once.
365+
focusAndScrollRef.apply = false
366+
focusAndScrollRef.hashFragment = null
367+
focusAndScrollRef.segmentPaths = []
368+
369+
disableSmoothScrollDuringRouteTransition(
370+
() => {
371+
// In case of hash scroll, we only need to scroll the element into view
372+
if (hashFragment) {
373+
// @ts-expect-error -- Needs `@types/react-dom` update
374+
instance.scrollIntoView()
375+
376+
return
377+
}
378+
// Store the current viewport height because reading `clientHeight` causes a reflow,
379+
// and it won't change during this function.
380+
const htmlElement = document.documentElement
381+
const viewportHeight = htmlElement.clientHeight
382+
383+
// If the element's top edge is already in the viewport, exit early.
384+
if (topOfElementInViewport(instance, viewportHeight)) {
385+
return
386+
}
387+
388+
// Otherwise, try scrolling go the top of the document to be backward compatible with pages
389+
// scrollIntoView() called on `<html/>` element scrolls horizontally on chrome and firefox (that shouldn't happen)
390+
// We could use it to scroll horizontally following RTL but that also seems to be broken - it will always scroll left
391+
// scrollLeft = 0 also seems to ignore RTL and manually checking for RTL is too much hassle so we will scroll just vertically
392+
htmlElement.scrollTop = 0
393+
394+
// Scroll to domNode if domNode is not in viewport when scrolled to top of document
395+
if (!topOfElementInViewport(instance, viewportHeight)) {
396+
// Scroll into view doesn't scroll horizontally by default when not needed
397+
// @ts-expect-error -- Needs `@types/react-dom` update
398+
instance.scrollIntoView()
399+
}
400+
},
401+
{
402+
// We will force layout by querying domNode position
403+
dontForceLayout: true,
404+
onlyHashChange: focusAndScrollRef.onlyHashChange,
405+
}
406+
)
407+
408+
// Mutate after scrolling so that it can be read by `disableSmoothScrollDuringRouteTransition`
409+
focusAndScrollRef.onlyHashChange = false
410+
411+
// Set focus on the element
412+
instance.focus()
413+
}
414+
},
415+
// Used to run on every commit. We may be able to be smarter about this
416+
// but be prepared for lots of manual testing.
417+
undefined
418+
)
419+
420+
return <Fragment ref={childrenRef}>{props.children}</Fragment>
421+
}
422+
423+
const InnerScrollAndFocusHandler = enableNewScrollHandler
424+
? InnerScrollAndFocusHandlerNew
425+
: InnerScrollAndFocusHandlerOld
426+
306427
function ScrollAndFocusHandler({
307428
segmentPath,
308429
children,

‎test/development/acceptance-app/hydration-error.test.ts‎

Lines changed: 20 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,14 @@ import path from 'path'
55
import { outdent } from 'outdent'
66
import { getToastErrorCount, retry } from 'next-test-utils'
77

8+
const enableNewScrollHandler = Boolean(
9+
process.env.__NEXT_EXPERIMENTAL_APP_NEW_SCROLL_HANDLER
10+
)
11+
12+
const innerScrollAndFocusHandlerName = enableNewScrollHandler
13+
? 'InnerScrollAndFocusHandlerNew'
14+
: 'InnerScrollAndFocusHandlerOld'
15+
816
describe('Error overlay for hydration errors in App router', () => {
917
const { next, isTurbopack } = nextTestSetup({
1018
files: new FileRef(path.join(__dirname, 'fixtures', 'default-template')),
@@ -74,7 +82,7 @@ describe('Error overlay for hydration errors in App router', () => {
7482
{
7583
"componentStack": "...
7684
<ScrollAndFocusHandler segmentPath={[...]}>
77-
<InnerScrollAndFocusHandler segmentPath={[...]} focusAndScrollRef={{apply:false, ...}}>
85+
<${innerScrollAndFocusHandlerName} segmentPath={[...]} focusAndScrollRef={{apply:false, ...}}>
7886
<ErrorBoundary errorComponent={undefined} errorStyles={undefined} errorScripts={undefined}>
7987
<LoadingBoundary loading={null}>
8088
<HTTPAccessFallbackBoundary notFound={<SegmentViewNode>} forbidden={undefined} unauthorized={undefined}>
@@ -154,7 +162,7 @@ describe('Error overlay for hydration errors in App router', () => {
154162
{
155163
"componentStack": "...
156164
<ScrollAndFocusHandler segmentPath={[...]}>
157-
<InnerScrollAndFocusHandler segmentPath={[...]} focusAndScrollRef={{apply:false, ...}}>
165+
<${innerScrollAndFocusHandlerName} segmentPath={[...]} focusAndScrollRef={{apply:false, ...}}>
158166
<ErrorBoundary errorComponent={undefined} errorStyles={undefined} errorScripts={undefined}>
159167
<LoadingBoundary loading={null}>
160168
<HTTPAccessFallbackBoundary notFound={<SegmentViewNode>} forbidden={undefined} unauthorized={undefined}>
@@ -311,7 +319,7 @@ describe('Error overlay for hydration errors in App router', () => {
311319
{
312320
"componentStack": "...
313321
<ScrollAndFocusHandler segmentPath={[...]}>
314-
<InnerScrollAndFocusHandler segmentPath={[...]} focusAndScrollRef={{apply:false, ...}}>
322+
<${innerScrollAndFocusHandlerName} segmentPath={[...]} focusAndScrollRef={{apply:false, ...}}>
315323
<ErrorBoundary errorComponent={undefined} errorStyles={undefined} errorScripts={undefined}>
316324
<LoadingBoundary loading={null}>
317325
<HTTPAccessFallbackBoundary notFound={<SegmentViewNode>} forbidden={undefined} unauthorized={undefined}>
@@ -372,7 +380,7 @@ describe('Error overlay for hydration errors in App router', () => {
372380
"componentStack": "...
373381
<RenderFromTemplateContext>
374382
<ScrollAndFocusHandler segmentPath={[...]}>
375-
<InnerScrollAndFocusHandler segmentPath={[...]} focusAndScrollRef={{apply:false, ...}}>
383+
<${innerScrollAndFocusHandlerName} segmentPath={[...]} focusAndScrollRef={{apply:false, ...}}>
376384
<ErrorBoundary errorComponent={undefined} errorStyles={undefined} errorScripts={undefined}>
377385
<LoadingBoundary loading={null}>
378386
<HTTPAccessFallbackBoundary notFound={<SegmentViewNode>} forbidden={undefined} unauthorized={undefined}>
@@ -426,7 +434,7 @@ describe('Error overlay for hydration errors in App router', () => {
426434
"componentStack": "...
427435
<RenderFromTemplateContext>
428436
<ScrollAndFocusHandler segmentPath={[...]}>
429-
<InnerScrollAndFocusHandler segmentPath={[...]} focusAndScrollRef={{apply:false, ...}}>
437+
<${innerScrollAndFocusHandlerName} segmentPath={[...]} focusAndScrollRef={{apply:false, ...}}>
430438
<ErrorBoundary errorComponent={undefined} errorStyles={undefined} errorScripts={undefined}>
431439
<LoadingBoundary loading={null}>
432440
<HTTPAccessFallbackBoundary notFound={<SegmentViewNode>} forbidden={undefined} unauthorized={undefined}>
@@ -488,7 +496,7 @@ describe('Error overlay for hydration errors in App router', () => {
488496
[
489497
{
490498
"componentStack": "...
491-
<InnerScrollAndFocusHandler segmentPath={[...]} focusAndScrollRef={{apply:false, ...}}>
499+
<${innerScrollAndFocusHandlerName} segmentPath={[...]} focusAndScrollRef={{apply:false, ...}}>
492500
<ErrorBoundary errorComponent={undefined} errorStyles={undefined} errorScripts={undefined}>
493501
<LoadingBoundary loading={null}>
494502
<HTTPAccessFallbackBoundary notFound={<SegmentViewNode>} forbidden={undefined} unauthorized={undefined}>
@@ -523,7 +531,7 @@ describe('Error overlay for hydration errors in App router', () => {
523531
"componentStack": "...
524532
<RenderFromTemplateContext>
525533
<ScrollAndFocusHandler segmentPath={[...]}>
526-
<InnerScrollAndFocusHandler segmentPath={[...]} focusAndScrollRef={{apply:false, ...}}>
534+
<${innerScrollAndFocusHandlerName} segmentPath={[...]} focusAndScrollRef={{apply:false, ...}}>
527535
<ErrorBoundary errorComponent={undefined} errorStyles={undefined} errorScripts={undefined}>
528536
<LoadingBoundary loading={null}>
529537
<HTTPAccessFallbackBoundary notFound={<SegmentViewNode>} forbidden={undefined} unauthorized={undefined}>
@@ -582,7 +590,7 @@ describe('Error overlay for hydration errors in App router', () => {
582590
"componentStack": "...
583591
<RenderFromTemplateContext>
584592
<ScrollAndFocusHandler segmentPath={[...]}>
585-
<InnerScrollAndFocusHandler segmentPath={[...]} focusAndScrollRef={{apply:false, ...}}>
593+
<${innerScrollAndFocusHandlerName} segmentPath={[...]} focusAndScrollRef={{apply:false, ...}}>
586594
<ErrorBoundary errorComponent={undefined} errorStyles={undefined} errorScripts={undefined}>
587595
<LoadingBoundary loading={null}>
588596
<HTTPAccessFallbackBoundary notFound={<SegmentViewNode>} forbidden={undefined} unauthorized={undefined}>
@@ -741,7 +749,7 @@ describe('Error overlay for hydration errors in App router', () => {
741749
"componentStack": "...
742750
<RenderFromTemplateContext>
743751
<ScrollAndFocusHandler segmentPath={[...]}>
744-
<InnerScrollAndFocusHandler segmentPath={[...]} focusAndScrollRef={{apply:false, ...}}>
752+
<${innerScrollAndFocusHandlerName} segmentPath={[...]} focusAndScrollRef={{apply:false, ...}}>
745753
<ErrorBoundary errorComponent={undefined} errorStyles={undefined} errorScripts={undefined}>
746754
<LoadingBoundary loading={null}>
747755
<HTTPAccessFallbackBoundary notFound={<SegmentViewNode>} forbidden={undefined} unauthorized={undefined}>
@@ -821,7 +829,7 @@ describe('Error overlay for hydration errors in App router', () => {
821829
[
822830
{
823831
"componentStack": "...
824-
<InnerScrollAndFocusHandler segmentPath={[...]} focusAndScrollRef={{apply:false, ...}}>
832+
<${innerScrollAndFocusHandlerName} segmentPath={[...]} focusAndScrollRef={{apply:false, ...}}>
825833
<ErrorBoundary errorComponent={undefined} errorStyles={undefined} errorScripts={undefined}>
826834
<LoadingBoundary loading={null}>
827835
<HTTPAccessFallbackBoundary notFound={<SegmentViewNode>} forbidden={undefined} unauthorized={undefined}>
@@ -896,7 +904,7 @@ describe('Error overlay for hydration errors in App router', () => {
896904
"componentStack": "...
897905
<RenderFromTemplateContext>
898906
<ScrollAndFocusHandler segmentPath={[...]}>
899-
<InnerScrollAndFocusHandler segmentPath={[...]} focusAndScrollRef={{apply:false, ...}}>
907+
<${innerScrollAndFocusHandlerName} segmentPath={[...]} focusAndScrollRef={{apply:false, ...}}>
900908
<ErrorBoundary errorComponent={undefined} errorStyles={undefined} errorScripts={undefined}>
901909
<LoadingBoundary loading={null}>
902910
<HTTPAccessFallbackBoundary notFound={<SegmentViewNode>} forbidden={undefined} unauthorized={undefined}>
@@ -972,7 +980,7 @@ describe('Error overlay for hydration errors in App router', () => {
972980
"componentStack": "...
973981
<RenderFromTemplateContext>
974982
<ScrollAndFocusHandler segmentPath={[...]}>
975-
<InnerScrollAndFocusHandler segmentPath={[...]} focusAndScrollRef={{apply:false, ...}}>
983+
<${innerScrollAndFocusHandlerName} segmentPath={[...]} focusAndScrollRef={{apply:false, ...}}>
976984
<ErrorBoundary errorComponent={undefined} errorStyles={undefined} errorScripts={undefined}>
977985
<LoadingBoundary loading={null}>
978986
<HTTPAccessFallbackBoundary notFound={<SegmentViewNode>} forbidden={undefined} unauthorized={undefined}>

‎test/development/app-dir/hydration-error-count/hydration-error-count.test.ts‎

Lines changed: 13 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,13 @@
11
import { nextTestSetup } from 'e2e-utils'
22

3+
const enableNewScrollHandler = Boolean(
4+
process.env.__NEXT_EXPERIMENTAL_APP_NEW_SCROLL_HANDLER
5+
)
6+
7+
const innerScrollAndFocusHandlerName = enableNewScrollHandler
8+
? 'InnerScrollAndFocusHandlerNew'
9+
: 'InnerScrollAndFocusHandlerOld'
10+
311
describe('hydration-error-count', () => {
412
const { next } = nextTestSetup({
513
files: __dirname,
@@ -14,7 +22,7 @@ describe('hydration-error-count', () => {
1422
"componentStack": "...
1523
<RenderFromTemplateContext>
1624
<ScrollAndFocusHandler segmentPath={[...]}>
17-
<InnerScrollAndFocusHandler segmentPath={[...]} focusAndScrollRef={{apply:false, ...}}>
25+
<${innerScrollAndFocusHandlerName} segmentPath={[...]} focusAndScrollRef={{apply:false, ...}}>
1826
<ErrorBoundary errorComponent={undefined} errorStyles={undefined} errorScripts={undefined}>
1927
<LoadingBoundary loading={null}>
2028
<HTTPAccessFallbackBoundary notFound={undefined} forbidden={undefined} unauthorized={undefined}>
@@ -66,7 +74,7 @@ describe('hydration-error-count', () => {
6674
"componentStack": "...
6775
<RenderFromTemplateContext>
6876
<ScrollAndFocusHandler segmentPath={[...]}>
69-
<InnerScrollAndFocusHandler segmentPath={[...]} focusAndScrollRef={{apply:false, ...}}>
77+
<${innerScrollAndFocusHandlerName} segmentPath={[...]} focusAndScrollRef={{apply:false, ...}}>
7078
<ErrorBoundary errorComponent={undefined} errorStyles={undefined} errorScripts={undefined}>
7179
<LoadingBoundary loading={null}>
7280
<HTTPAccessFallbackBoundary notFound={undefined} forbidden={undefined} unauthorized={undefined}>
@@ -106,7 +114,7 @@ describe('hydration-error-count', () => {
106114
"componentStack": "...
107115
<RenderFromTemplateContext>
108116
<ScrollAndFocusHandler segmentPath={[...]}>
109-
<InnerScrollAndFocusHandler segmentPath={[...]} focusAndScrollRef={{apply:false, ...}}>
117+
<${innerScrollAndFocusHandlerName} segmentPath={[...]} focusAndScrollRef={{apply:false, ...}}>
110118
<ErrorBoundary errorComponent={undefined} errorStyles={undefined} errorScripts={undefined}>
111119
<LoadingBoundary loading={null}>
112120
<HTTPAccessFallbackBoundary notFound={undefined} forbidden={undefined} unauthorized={undefined}>
@@ -138,7 +146,7 @@ describe('hydration-error-count', () => {
138146
"componentStack": "...
139147
<RenderFromTemplateContext>
140148
<ScrollAndFocusHandler segmentPath={[...]}>
141-
<InnerScrollAndFocusHandler segmentPath={[...]} focusAndScrollRef={{apply:false, ...}}>
149+
<${innerScrollAndFocusHandlerName} segmentPath={[...]} focusAndScrollRef={{apply:false, ...}}>
142150
<ErrorBoundary errorComponent={undefined} errorStyles={undefined} errorScripts={undefined}>
143151
<LoadingBoundary loading={null}>
144152
<HTTPAccessFallbackBoundary notFound={undefined} forbidden={undefined} unauthorized={undefined}>
@@ -180,7 +188,7 @@ describe('hydration-error-count', () => {
180188
"componentStack": "...
181189
<RenderFromTemplateContext>
182190
<ScrollAndFocusHandler segmentPath={[...]}>
183-
<InnerScrollAndFocusHandler segmentPath={[...]} focusAndScrollRef={{apply:false, ...}}>
191+
<${innerScrollAndFocusHandlerName} segmentPath={[...]} focusAndScrollRef={{apply:false, ...}}>
184192
<ErrorBoundary errorComponent={undefined} errorStyles={undefined} errorScripts={undefined}>
185193
<LoadingBoundary loading={null}>
186194
<HTTPAccessFallbackBoundary notFound={undefined} forbidden={undefined} unauthorized={undefined}>

0 commit comments

Comments
 (0)