Skip to content

Ensure hoisted elements don't break scroll-to-top on page navigations #83108

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
eps1lon wants to merge 1 commit into
base: sebbie/08-27-add_experimental.appnewscrollhandler_
Choose a base branch
from
Draft

Ensure hoisted elements don't break scroll-to-top on page navigations #83108

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
133 changes: 127 additions & 6 deletions packages/next/src/client/components/layout-router.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,14 @@ import {
} from './router-reducer/router-reducer-types'

import React, {
Fragment,
useContext,
use,
startTransition,
Suspense,
useDeferredValue,
useLayoutEffect,
type FragmentInstance,
type JSX,
} from 'react'
import ReactDOM from 'react-dom'
Expand All @@ -46,6 +49,8 @@ const Activity = process.env.__NEXT_ROUTER_BF_CACHE
? (require('react') as typeof import('react')).unstable_Activity
: null!

const enableNewScrollHandler = process.env.__NEXT_APP_NEW_SCROLL_HANDLER

/**
* Add refetch marker to router state at the point of the current layout segment.
* This ensures the response returned is not further down than the current layout segment.
Expand Down Expand Up @@ -153,9 +158,23 @@ function shouldSkipElement(element: HTMLElement) {
/**
* Check if the top corner of the HTMLElement is in the viewport.
*/
function topOfElementInViewport(element: HTMLElement, viewportHeight: number) {
const rect = element.getBoundingClientRect()
return rect.top >= 0 && rect.top <= viewportHeight
function topOfElementInViewport(
instance: HTMLElement | FragmentInstance,
viewportHeight: number
) {
const rects = instance.getClientRects()
if (rects.length === 0) {
// Just to be explicit.
return false
}
let elementTop = Number.POSITIVE_INFINITY
for (let i = 0; i < rects.length; i++) {
const rect = rects[i]
if (rect.top < elementTop) {
elementTop = rect.top
}
}
return elementTop >= 0 && elementTop <= viewportHeight
}

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

Expand Down Expand Up @@ -303,6 +322,108 @@ class InnerScrollAndFocusHandler extends React.Component<ScrollAndFocusHandlerPr
}
}

function InnerScrollAndFocusHandlerNew(props: ScrollAndFocusHandlerProps) {
const childrenRef = React.useRef<FragmentInstance>(null)

useLayoutEffect(
() => {
const { focusAndScrollRef, segmentPath } = props
// Handle scroll and focus, it's only applied once in the first useEffect that triggers that changed.

if (focusAndScrollRef.apply) {
// segmentPaths is an array of segment paths that should be scrolled to
// if the current segment path is not in the array, the scroll is not applied
// unless the array is empty, in which case the scroll is always applied
if (
focusAndScrollRef.segmentPaths.length !== 0 &&
!focusAndScrollRef.segmentPaths.some((scrollRefSegmentPath) =>
segmentPath.every((segment, index) =>
matchSegment(segment, scrollRefSegmentPath[index])
)
)
) {
return
}

let instance: FragmentInstance | HTMLElement | null = null
const hashFragment = focusAndScrollRef.hashFragment

if (hashFragment) {
instance = getHashFragmentDomNode(hashFragment)
}

if (!instance) {
instance = childrenRef.current
}

// If there is no DOM node this layout-router level is skipped. It'll be handled higher-up in the tree.
if (instance === null) {
return
}

// State is mutated to ensure that the focus and scroll is applied only once.
focusAndScrollRef.apply = false
focusAndScrollRef.hashFragment = null
focusAndScrollRef.segmentPaths = []

disableSmoothScrollDuringRouteTransition(
() => {
// In case of hash scroll, we only need to scroll the element into view
if (hashFragment) {
// @ts-expect-error -- Needs `@types/react-dom` update
instance.scrollIntoView()

return
}
// Store the current viewport height because reading `clientHeight` causes a reflow,
// and it won't change during this function.
const htmlElement = document.documentElement
const viewportHeight = htmlElement.clientHeight

// If the element's top edge is already in the viewport, exit early.
if (topOfElementInViewport(instance, viewportHeight)) {
return
}

// Otherwise, try scrolling go the top of the document to be backward compatible with pages
// scrollIntoView() called on `<html/>` element scrolls horizontally on chrome and firefox (that shouldn't happen)
// We could use it to scroll horizontally following RTL but that also seems to be broken - it will always scroll left
// scrollLeft = 0 also seems to ignore RTL and manually checking for RTL is too much hassle so we will scroll just vertically
htmlElement.scrollTop = 0

// Scroll to domNode if domNode is not in viewport when scrolled to top of document
if (!topOfElementInViewport(instance, viewportHeight)) {
// Scroll into view doesn't scroll horizontally by default when not needed
// @ts-expect-error -- Needs `@types/react-dom` update
instance.scrollIntoView()
}
},
{
// We will force layout by querying domNode position
dontForceLayout: true,
onlyHashChange: focusAndScrollRef.onlyHashChange,
}
)

// Mutate after scrolling so that it can be read by `disableSmoothScrollDuringRouteTransition`
focusAndScrollRef.onlyHashChange = false

// Set focus on the element
instance.focus()
}
},
// Used to run on every commit. We may be able to be smarter about this
// but be prepared for lots of manual testing.
undefined
)

return <Fragment ref={childrenRef}>{props.children}</Fragment>
}

const InnerScrollAndFocusHandler = enableNewScrollHandler
? InnerScrollAndFocusHandlerNew
: InnerScrollAndFocusHandlerOld

function ScrollAndFocusHandler({
segmentPath,
children,
Expand Down
32 changes: 20 additions & 12 deletions test/development/acceptance-app/hydration-error.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,14 @@ import path from 'path'
import { outdent } from 'outdent'
import { getToastErrorCount, retry } from 'next-test-utils'

const enableNewScrollHandler = Boolean(
process.env.__NEXT_EXPERIMENTAL_APP_NEW_SCROLL_HANDLER
)

const innerScrollAndFocusHandlerName = enableNewScrollHandler
? 'InnerScrollAndFocusHandlerNew'
: 'InnerScrollAndFocusHandlerOld'

describe('Error overlay for hydration errors in App router', () => {
const { next, isTurbopack } = nextTestSetup({
files: new FileRef(path.join(__dirname, 'fixtures', 'default-template')),
Expand Down Expand Up @@ -74,7 +82,7 @@ describe('Error overlay for hydration errors in App router', () => {
{
"componentStack": "...
<ScrollAndFocusHandler segmentPath={[...]}>
<InnerScrollAndFocusHandler segmentPath={[...]} focusAndScrollRef={{apply:false, ...}}>
<${innerScrollAndFocusHandlerName} segmentPath={[...]} focusAndScrollRef={{apply:false, ...}}>
<ErrorBoundary errorComponent={undefined} errorStyles={undefined} errorScripts={undefined}>
<LoadingBoundary loading={null}>
<HTTPAccessFallbackBoundary notFound={<SegmentViewNode>} forbidden={undefined} unauthorized={undefined}>
Expand Down Expand Up @@ -154,7 +162,7 @@ describe('Error overlay for hydration errors in App router', () => {
{
"componentStack": "...
<ScrollAndFocusHandler segmentPath={[...]}>
<InnerScrollAndFocusHandler segmentPath={[...]} focusAndScrollRef={{apply:false, ...}}>
<${innerScrollAndFocusHandlerName} segmentPath={[...]} focusAndScrollRef={{apply:false, ...}}>
<ErrorBoundary errorComponent={undefined} errorStyles={undefined} errorScripts={undefined}>
<LoadingBoundary loading={null}>
<HTTPAccessFallbackBoundary notFound={<SegmentViewNode>} forbidden={undefined} unauthorized={undefined}>
Expand Down Expand Up @@ -311,7 +319,7 @@ describe('Error overlay for hydration errors in App router', () => {
{
"componentStack": "...
<ScrollAndFocusHandler segmentPath={[...]}>
<InnerScrollAndFocusHandler segmentPath={[...]} focusAndScrollRef={{apply:false, ...}}>
<${innerScrollAndFocusHandlerName} segmentPath={[...]} focusAndScrollRef={{apply:false, ...}}>
<ErrorBoundary errorComponent={undefined} errorStyles={undefined} errorScripts={undefined}>
<LoadingBoundary loading={null}>
<HTTPAccessFallbackBoundary notFound={<SegmentViewNode>} forbidden={undefined} unauthorized={undefined}>
Expand Down Expand Up @@ -372,7 +380,7 @@ describe('Error overlay for hydration errors in App router', () => {
"componentStack": "...
<RenderFromTemplateContext>
<ScrollAndFocusHandler segmentPath={[...]}>
<InnerScrollAndFocusHandler segmentPath={[...]} focusAndScrollRef={{apply:false, ...}}>
<${innerScrollAndFocusHandlerName} segmentPath={[...]} focusAndScrollRef={{apply:false, ...}}>
<ErrorBoundary errorComponent={undefined} errorStyles={undefined} errorScripts={undefined}>
<LoadingBoundary loading={null}>
<HTTPAccessFallbackBoundary notFound={<SegmentViewNode>} forbidden={undefined} unauthorized={undefined}>
Expand Down Expand Up @@ -426,7 +434,7 @@ describe('Error overlay for hydration errors in App router', () => {
"componentStack": "...
<RenderFromTemplateContext>
<ScrollAndFocusHandler segmentPath={[...]}>
<InnerScrollAndFocusHandler segmentPath={[...]} focusAndScrollRef={{apply:false, ...}}>
<${innerScrollAndFocusHandlerName} segmentPath={[...]} focusAndScrollRef={{apply:false, ...}}>
<ErrorBoundary errorComponent={undefined} errorStyles={undefined} errorScripts={undefined}>
<LoadingBoundary loading={null}>
<HTTPAccessFallbackBoundary notFound={<SegmentViewNode>} forbidden={undefined} unauthorized={undefined}>
Expand Down Expand Up @@ -488,7 +496,7 @@ describe('Error overlay for hydration errors in App router', () => {
[
{
"componentStack": "...
<InnerScrollAndFocusHandler segmentPath={[...]} focusAndScrollRef={{apply:false, ...}}>
<${innerScrollAndFocusHandlerName} segmentPath={[...]} focusAndScrollRef={{apply:false, ...}}>
<ErrorBoundary errorComponent={undefined} errorStyles={undefined} errorScripts={undefined}>
<LoadingBoundary loading={null}>
<HTTPAccessFallbackBoundary notFound={<SegmentViewNode>} forbidden={undefined} unauthorized={undefined}>
Expand Down Expand Up @@ -523,7 +531,7 @@ describe('Error overlay for hydration errors in App router', () => {
"componentStack": "...
<RenderFromTemplateContext>
<ScrollAndFocusHandler segmentPath={[...]}>
<InnerScrollAndFocusHandler segmentPath={[...]} focusAndScrollRef={{apply:false, ...}}>
<${innerScrollAndFocusHandlerName} segmentPath={[...]} focusAndScrollRef={{apply:false, ...}}>
<ErrorBoundary errorComponent={undefined} errorStyles={undefined} errorScripts={undefined}>
<LoadingBoundary loading={null}>
<HTTPAccessFallbackBoundary notFound={<SegmentViewNode>} forbidden={undefined} unauthorized={undefined}>
Expand Down Expand Up @@ -582,7 +590,7 @@ describe('Error overlay for hydration errors in App router', () => {
"componentStack": "...
<RenderFromTemplateContext>
<ScrollAndFocusHandler segmentPath={[...]}>
<InnerScrollAndFocusHandler segmentPath={[...]} focusAndScrollRef={{apply:false, ...}}>
<${innerScrollAndFocusHandlerName} segmentPath={[...]} focusAndScrollRef={{apply:false, ...}}>
<ErrorBoundary errorComponent={undefined} errorStyles={undefined} errorScripts={undefined}>
<LoadingBoundary loading={null}>
<HTTPAccessFallbackBoundary notFound={<SegmentViewNode>} forbidden={undefined} unauthorized={undefined}>
Expand Down Expand Up @@ -741,7 +749,7 @@ describe('Error overlay for hydration errors in App router', () => {
"componentStack": "...
<RenderFromTemplateContext>
<ScrollAndFocusHandler segmentPath={[...]}>
<InnerScrollAndFocusHandler segmentPath={[...]} focusAndScrollRef={{apply:false, ...}}>
<${innerScrollAndFocusHandlerName} segmentPath={[...]} focusAndScrollRef={{apply:false, ...}}>
<ErrorBoundary errorComponent={undefined} errorStyles={undefined} errorScripts={undefined}>
<LoadingBoundary loading={null}>
<HTTPAccessFallbackBoundary notFound={<SegmentViewNode>} forbidden={undefined} unauthorized={undefined}>
Expand Down Expand Up @@ -821,7 +829,7 @@ describe('Error overlay for hydration errors in App router', () => {
[
{
"componentStack": "...
<InnerScrollAndFocusHandler segmentPath={[...]} focusAndScrollRef={{apply:false, ...}}>
<${innerScrollAndFocusHandlerName} segmentPath={[...]} focusAndScrollRef={{apply:false, ...}}>
<ErrorBoundary errorComponent={undefined} errorStyles={undefined} errorScripts={undefined}>
<LoadingBoundary loading={null}>
<HTTPAccessFallbackBoundary notFound={<SegmentViewNode>} forbidden={undefined} unauthorized={undefined}>
Expand Down Expand Up @@ -896,7 +904,7 @@ describe('Error overlay for hydration errors in App router', () => {
"componentStack": "...
<RenderFromTemplateContext>
<ScrollAndFocusHandler segmentPath={[...]}>
<InnerScrollAndFocusHandler segmentPath={[...]} focusAndScrollRef={{apply:false, ...}}>
<${innerScrollAndFocusHandlerName} segmentPath={[...]} focusAndScrollRef={{apply:false, ...}}>
<ErrorBoundary errorComponent={undefined} errorStyles={undefined} errorScripts={undefined}>
<LoadingBoundary loading={null}>
<HTTPAccessFallbackBoundary notFound={<SegmentViewNode>} forbidden={undefined} unauthorized={undefined}>
Expand Down Expand Up @@ -972,7 +980,7 @@ describe('Error overlay for hydration errors in App router', () => {
"componentStack": "...
<RenderFromTemplateContext>
<ScrollAndFocusHandler segmentPath={[...]}>
<InnerScrollAndFocusHandler segmentPath={[...]} focusAndScrollRef={{apply:false, ...}}>
<${innerScrollAndFocusHandlerName} segmentPath={[...]} focusAndScrollRef={{apply:false, ...}}>
<ErrorBoundary errorComponent={undefined} errorStyles={undefined} errorScripts={undefined}>
<LoadingBoundary loading={null}>
<HTTPAccessFallbackBoundary notFound={<SegmentViewNode>} forbidden={undefined} unauthorized={undefined}>
Expand Down
18 changes: 13 additions & 5 deletions test/development/app-dir/hydration-error-count/hydration-error-count.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,13 @@
import { nextTestSetup } from 'e2e-utils'

const enableNewScrollHandler = Boolean(
process.env.__NEXT_EXPERIMENTAL_APP_NEW_SCROLL_HANDLER
)

const innerScrollAndFocusHandlerName = enableNewScrollHandler
? 'InnerScrollAndFocusHandlerNew'
: 'InnerScrollAndFocusHandlerOld'

describe('hydration-error-count', () => {
const { next } = nextTestSetup({
files: __dirname,
Expand All @@ -14,7 +22,7 @@ describe('hydration-error-count', () => {
"componentStack": "...
<RenderFromTemplateContext>
<ScrollAndFocusHandler segmentPath={[...]}>
<InnerScrollAndFocusHandler segmentPath={[...]} focusAndScrollRef={{apply:false, ...}}>
<${innerScrollAndFocusHandlerName} segmentPath={[...]} focusAndScrollRef={{apply:false, ...}}>
<ErrorBoundary errorComponent={undefined} errorStyles={undefined} errorScripts={undefined}>
<LoadingBoundary loading={null}>
<HTTPAccessFallbackBoundary notFound={undefined} forbidden={undefined} unauthorized={undefined}>
Expand Down Expand Up @@ -66,7 +74,7 @@ describe('hydration-error-count', () => {
"componentStack": "...
<RenderFromTemplateContext>
<ScrollAndFocusHandler segmentPath={[...]}>
<InnerScrollAndFocusHandler segmentPath={[...]} focusAndScrollRef={{apply:false, ...}}>
<${innerScrollAndFocusHandlerName} segmentPath={[...]} focusAndScrollRef={{apply:false, ...}}>
<ErrorBoundary errorComponent={undefined} errorStyles={undefined} errorScripts={undefined}>
<LoadingBoundary loading={null}>
<HTTPAccessFallbackBoundary notFound={undefined} forbidden={undefined} unauthorized={undefined}>
Expand Down Expand Up @@ -106,7 +114,7 @@ describe('hydration-error-count', () => {
"componentStack": "...
<RenderFromTemplateContext>
<ScrollAndFocusHandler segmentPath={[...]}>
<InnerScrollAndFocusHandler segmentPath={[...]} focusAndScrollRef={{apply:false, ...}}>
<${innerScrollAndFocusHandlerName} segmentPath={[...]} focusAndScrollRef={{apply:false, ...}}>
<ErrorBoundary errorComponent={undefined} errorStyles={undefined} errorScripts={undefined}>
<LoadingBoundary loading={null}>
<HTTPAccessFallbackBoundary notFound={undefined} forbidden={undefined} unauthorized={undefined}>
Expand Down Expand Up @@ -138,7 +146,7 @@ describe('hydration-error-count', () => {
"componentStack": "...
<RenderFromTemplateContext>
<ScrollAndFocusHandler segmentPath={[...]}>
<InnerScrollAndFocusHandler segmentPath={[...]} focusAndScrollRef={{apply:false, ...}}>
<${innerScrollAndFocusHandlerName} segmentPath={[...]} focusAndScrollRef={{apply:false, ...}}>
<ErrorBoundary errorComponent={undefined} errorStyles={undefined} errorScripts={undefined}>
<LoadingBoundary loading={null}>
<HTTPAccessFallbackBoundary notFound={undefined} forbidden={undefined} unauthorized={undefined}>
Expand Down Expand Up @@ -180,7 +188,7 @@ describe('hydration-error-count', () => {
"componentStack": "...
<RenderFromTemplateContext>
<ScrollAndFocusHandler segmentPath={[...]}>
<InnerScrollAndFocusHandler segmentPath={[...]} focusAndScrollRef={{apply:false, ...}}>
<${innerScrollAndFocusHandlerName} segmentPath={[...]} focusAndScrollRef={{apply:false, ...}}>
<ErrorBoundary errorComponent={undefined} errorStyles={undefined} errorScripts={undefined}>
<LoadingBoundary loading={null}>
<HTTPAccessFallbackBoundary notFound={undefined} forbidden={undefined} unauthorized={undefined}>
Expand Down
Loading
Loading