[fragment-scroll] Ensure hoisted elements don't break scroll-to-top on page navigations (#83108)

Author: eps1lon

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

@@ -12,10 +12,13 @@ import type { FocusAndScrollRef } from './router-reducer/router-reducer-types'
 
 import React, {
   Activity,
+  Fragment,
   useContext,
   use,
   Suspense,
   useDeferredValue,
+  useLayoutEffect,
+  type FragmentInstance,
   type JSX,
   type ActivityProps,
 } from 'react'
@@ -45,6 +48,8 @@ import { getParamValueFromCacheKey } from '../route-params'
 import type { Params } from '../../server/request/params'
 import { isDeferredRsc } from './router-reducer/ppr-navigations'
 
+const enableNewScrollHandler = process.env.__NEXT_APP_NEW_SCROLL_HANDLER
+
 const __DOM_INTERNALS_DO_NOT_USE_OR_WARN_USERS_THEY_CANNOT_UPGRADE = (
   ReactDOM as any
 ).__DOM_INTERNALS_DO_NOT_USE_OR_WARN_USERS_THEY_CANNOT_UPGRADE
@@ -96,9 +101,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
+): boolean {
+  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
 }
 
 /**
@@ -125,7 +144,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
@@ -170,9 +189,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`
           }
         }
 
@@ -246,6 +265,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) {
+              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
+              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 but don't scroll since we already did that.
+        // The focus might have targetted a deep element outside of the instances
+        // top edge.
+        instance.focus({ preventScroll: true })
+      }
+    },
+    // 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,

test/development/browser-logs/browser-logs.test.ts

@@ -6,6 +6,11 @@ import stripAnsi from 'strip-ansi'
 import { retry } from 'next-test-utils'
 
 const bundlerName = process.env.IS_TURBOPACK_TEST ? 'Turbopack' : 'Webpack'
+const enableNewScrollHandler =
+  process.env.__NEXT_EXPERIMENTAL_APP_NEW_SCROLL_HANDLER === 'true'
+const innerScrollAndFocusHandlerName = enableNewScrollHandler
+  ? 'InnerScrollAndFocusHandlerNew'
+  : 'InnerScrollAndFocusHandlerOld'
 
 function setupLogCapture() {
   const logs: string[] = []
@@ -332,7 +337,7 @@ describe(`Terminal Logging (${bundlerName})`, () => {
          ...
            <RenderFromTemplateContext>
              <ScrollAndFocusHandler segmentPath={[...]}>
-               <InnerScrollAndFocusHandler segmentPath={[...]} focusAndScrollRef={{apply:false, ...}}>
+               <${innerScrollAndFocusHandlerName} segmentPath={[...]} focusAndScrollRef={{apply:false, ...}}>
                  <ErrorBoundary errorComponent={undefined} errorStyles={undefined} errorScripts={undefined}>
                    <LoadingBoundary name="hydration-..." loading={null}>
                      <HTTPAccessFallbackBoundary notFound={undefined} forbidden={undefined} unauthorized={undefined}>

test/e2e/app-dir/router-autoscroll/app/hoisted/page.tsx

@@ -0,0 +1,13 @@
+export default function Page() {
+  return (
+    <>
+      <style precedence="alpha" href="custom-stylesheet"></style>
+      {
+        // Repeat 500 elements
+        Array.from({ length: 500 }, (_, i) => (
+          <div key={i}>{i}</div>
+        ))
+      }
+    </>
+  )
+}

test/e2e/app-dir/router-autoscroll/app/page.tsx

@@ -35,6 +35,10 @@ export default function Page() {
       <div>
         <Link href="/new-metadata">To new metadata</Link>
       </div>
+
+      <div>
+        <Link href="/hoisted">To hoisted</Link>
+      </div>
     </>
   )
 }

test/e2e/app-dir/router-autoscroll/router-autoscroll.test.ts

@@ -2,6 +2,9 @@ import type { Playwright } from 'next-webdriver'
 import { nextTestSetup } from 'e2e-utils'
 import { check, assertNoConsoleErrors, retry } from 'next-test-utils'
 
+const enableNewScrollHandler =
+  process.env.__NEXT_EXPERIMENTAL_APP_NEW_SCROLL_HANDLER === 'true'
+
 describe('router autoscrolling on navigation', () => {
   const { next, isNextDev } = nextTestSetup({
     files: __dirname,
@@ -237,4 +240,27 @@ describe('router autoscrolling on navigation', () => {
       })
     })
   })
+
+  it('should scroll to top even if React hoists children', async () => {
+    const browser = await next.browser('/')
+
+    // scroll to bottom
+    await browser.eval(
+      `window.scrollTo(0, ${await browser.eval('document.documentElement.scrollHeight')})`
+    )
+    // Just need to scroll by something
+    expect(await getTopScroll(browser)).toBeGreaterThan(0)
+
+    await browser.elementByCss('[href="/hoisted"]').click()
+    expect(
+      await browser.eval('document.documentElement.scrollHeight')
+    ).toBeGreaterThan(0)
+    if (enableNewScrollHandler) {
+      await waitForScrollToComplete(browser, { x: 0, y: 0 })
+    } else {
+      await expect(
+        waitForScrollToComplete(browser, { x: 0, y: 0 })
+      ).rejects.toThrow()
+    }
+  })
 })

test/lib/next-test-utils.ts

@@ -1486,7 +1486,8 @@ const nextjsClientComponentNames = [
   'HTTPAccessFallbackBoundary',
   'HTTPAccessFallbackErrorBoundary',
   'InnerLayoutRouter',
-  'InnerScrollAndFocusHandler',
+  'InnerScrollAndFocusHandlerOld',
+  'InnerScrollAndFocusHandlerNew',
   'RedirectBoundary',
   'RedirectErrorBoundary',
   'RenderFromTemplateContext',