Ensure metadata doesn't break scroll-to-top on navigation (#74748)

vercel · Jan 10, 2025 · f8afd61 · f8afd61
1 parent 5bd025d
commit f8afd61
Show file tree

Hide file tree

Showing 7 changed files with 100 additions and 45 deletions.
diff --git a/packages/next/src/client/components/layout-router.tsx b/packages/next/src/client/components/layout-router.tsx
@@ -215,6 +215,14 @@ class InnerScrollAndFocusHandler extends React.Component<ScrollAndFocusHandlerPr
       // Verify if the element is a HTMLElement and if we want to consider it for scroll behavior.
       // If the element is skipped, try to select the next sibling and try again.
       while (!(domNode instanceof HTMLElement) || shouldSkipElement(domNode)) {
+        if (process.env.NODE_ENV !== 'production') {
+          if (domNode.parentElement?.localName === 'head') {
+            console.error(
+              'Tried to scroll to a head element. This is a bug in Next.js.'
+            )
+          }
+        }
+
         // No siblings found that match the criteria are found, so handle scroll higher up in the tree instead.
         if (domNode.nextElementSibling === null) {
           return

diff --git a/packages/next/src/server/app-render/create-component-tree.tsx b/packages/next/src/server/app-render/create-component-tree.tsx
@@ -399,25 +399,25 @@ async function createComponentTreeInternal({
 
   const notFoundElement = NotFound ? (
     <>
+      <NotFound />
       {metadata}
       {notFoundStyles}
-      <NotFound />
     </>
   ) : undefined
 
   const forbiddenElement = Forbidden ? (
     <>
+      <Forbidden />
       {metadata}
       {forbiddenStyles}
-      <Forbidden />
     </>
   ) : undefined
 
   const unauthorizedElement = Unauthorized ? (
     <>
+      <Unauthorized />
       {metadata}
       {unauthorizedStyles}
-      <Unauthorized />
     </>
   ) : undefined
 
@@ -669,8 +669,13 @@ async function createComponentTreeInternal({
     return [
       actualSegment,
       <React.Fragment key={cacheNodeKey}>
-        {metadata}
         {pageElement}
+        {/*
+         * The order here matters since a parent might call findDOMNode().
+         * findDOMNode() will return the first child if multiple children are rendered.
+         * But React will hoist metadata into <head> which breaks scroll handling.
+         */}
+        {metadata}
         {layerAssets}
         <OutletBoundary>
           <MetadataOutlet ready={getViewportReady} />
@@ -805,12 +810,12 @@ async function createComponentTreeInternal({
             notFound={
               NotFound ? (
                 <>
-                  {metadata}
                   {layerAssets}
                   <SegmentComponent params={params}>
                     {notFoundStyles}
                     <NotFound />
                   </SegmentComponent>
+                  {metadata}
                 </>
               ) : undefined
             }

diff --git a/test/development/acceptance-app/hydration-error.test.ts b/test/development/acceptance-app/hydration-error.test.ts
@@ -374,7 +374,9 @@ describe('Error overlay for hydration errors in App router', () => {
     const { session, browser } = sandbox
     await session.openRedbox()
 
-    expect(await getRedboxTotalErrorCount(browser)).toBe(2)
+    await retry(async () => {
+      expect(await getRedboxTotalErrorCount(browser)).toBe(2)
+    })
 
     // FIXME: Should also have "text nodes cannot be a child of tr"
     expect(await session.getRedboxDescription()).toMatchInlineSnapshot(
@@ -439,7 +441,6 @@ describe('Error overlay for hydration errors in App router', () => {
                        <RedirectBoundary>
                          <RedirectErrorBoundary router={{...}}>
                            <InnerLayoutRouter url="/" tree={[...]} cacheNode={{lazyData:null, ...}} segmentPath={[...]}>
-                             <__next_metadata_boundary__>
                              <ClientPageRoot Component={function Page} searchParams={{}} params={{}}>
                                <Page params={Promise} searchParams={Promise}>
      >                           <table>
@@ -567,7 +568,9 @@ describe('Error overlay for hydration errors in App router', () => {
     const { session, browser } = sandbox
     await session.openRedbox()
 
-    expect(await getRedboxTotalErrorCount(browser)).toBe(2)
+    await retry(async () => {
+      expect(await getRedboxTotalErrorCount(browser)).toBe(2)
+    })
 
     const description = await session.getRedboxDescription()
     expect(description).toContain(
@@ -665,7 +668,9 @@ describe('Error overlay for hydration errors in App router', () => {
     const { session, browser } = sandbox
     await session.openRedbox()
 
-    expect(await getRedboxTotalErrorCount(browser)).toBe(2)
+    await retry(async () => {
+      expect(await getRedboxTotalErrorCount(browser)).toBe(2)
+    })
 
     const description = await session.getRedboxDescription()
     expect(description).toEqual(outdent`
@@ -718,7 +723,9 @@ describe('Error overlay for hydration errors in App router', () => {
     const { session, browser } = sandbox
     await session.openRedbox()
 
-    expect(await getRedboxTotalErrorCount(browser)).toBe(2)
+    await retry(async () => {
+      expect(await getRedboxTotalErrorCount(browser)).toBe(2)
+    })
 
     const description = await session.getRedboxDescription()
     expect(description).toContain(
@@ -895,19 +902,18 @@ describe('Error overlay for hydration errors in App router', () => {
            <RedirectBoundary>
              <RedirectErrorBoundary router={{...}}>
                <InnerLayoutRouter url="/" tree={[...]} cacheNode={{lazyData:null, ...}} segmentPath={[...]}>
-                 <__next_metadata_boundary__>
-                   <ClientPageRoot Component={function Page} searchParams={{}} params={{}}>
-                     <Page params={Promise} searchParams={Promise}>
+                 <ClientPageRoot Component={function Page} searchParams={{}} params={{}}>
+                   <Page params={Promise} searchParams={Promise}>
+                     <div>
                        <div>
                          <div>
                            <div>
-                             <div>
-                               <Mismatch>
-                                 <p>
-                                   <span>
-                                     ...
-       +                              client
-       -                              server"
+                             <Mismatch>
+                               <p>
+                                 <span>
+                                   ...
+       +                            client
+       -                            server"
       `)
     } else {
       expect(fullPseudoHtml).toMatchInlineSnapshot(`
@@ -916,19 +922,18 @@ describe('Error overlay for hydration errors in App router', () => {
            <RedirectBoundary>
              <RedirectErrorBoundary router={{...}}>
                <InnerLayoutRouter url="/" tree={[...]} cacheNode={{lazyData:null, ...}} segmentPath={[...]}>
-                 <__next_metadata_boundary__>
-                   <ClientPageRoot Component={function Page} searchParams={{}} params={{}}>
-                     <Page params={Promise} searchParams={Promise}>
+                 <ClientPageRoot Component={function Page} searchParams={{}} params={{}}>
+                   <Page params={Promise} searchParams={Promise}>
+                     <div>
                        <div>
                          <div>
                            <div>
-                             <div>
-                               <Mismatch>
-                                 <p>
-                                   <span>
-                                     ...
-       +                              client
-       -                              server"
+                             <Mismatch>
+                               <p>
+                                 <span>
+                                   ...
+       +                            client
+       -                            server"
       `)
     }
   })

diff --git a/test/e2e/app-dir/router-autoscroll/app/new-metadata/page.tsx b/test/e2e/app-dir/router-autoscroll/app/new-metadata/page.tsx
@@ -0,0 +1,17 @@
+import type { Metadata } from 'next'
+
+export const metadata: Metadata = {
+  keywords: ['new-metadata'],
+}
+export default function Page() {
+  return (
+    <>
+      {
+        // Repeat 500 elements
+        Array.from({ length: 500 }, (_, i) => (
+          <div key={i}>{i}</div>
+        ))
+      }
+    </>
+  )
+}
diff --git a/test/e2e/app-dir/router-autoscroll/app/page.tsx b/test/e2e/app-dir/router-autoscroll/app/page.tsx
@@ -31,6 +31,10 @@ export default function Page() {
           To sticky first element
         </Link>
       </div>
+
+      <div>
+        <Link href="/new-metadata">To new metadata</Link>
+      </div>
     </>
   )
 }
diff --git a/test/e2e/app-dir/router-autoscroll/router-autoscroll.test.ts b/test/e2e/app-dir/router-autoscroll/router-autoscroll.test.ts
@@ -1,31 +1,29 @@
-import webdriver from 'next-webdriver'
+import webdriver, { type BrowserInterface } from 'next-webdriver'
 import { nextTestSetup } from 'e2e-utils'
-import { check } from 'next-test-utils'
+import { check, assertNoConsoleErrors, retry } from 'next-test-utils'
 
 describe('router autoscrolling on navigation', () => {
   const { next, isNextDev } = nextTestSetup({
     files: __dirname,
   })
 
-  type BrowserInterface = Awaited<ReturnType<typeof webdriver>>
-
   const getTopScroll = async (browser: BrowserInterface) =>
     await browser.eval('document.documentElement.scrollTop')
 
   const getLeftScroll = async (browser: BrowserInterface) =>
     await browser.eval('document.documentElement.scrollLeft')
 
-  const waitForScrollToComplete = (
-    browser,
+  const waitForScrollToComplete = async (
+    browser: BrowserInterface,
     options: { x: number; y: number }
-  ) =>
-    check(async () => {
+  ) => {
+    await retry(async () => {
       const top = await getTopScroll(browser)
       const left = await getLeftScroll(browser)
-      return top === options.y && left === options.x
-        ? 'success'
-        : JSON.stringify({ top, left })
-    }, 'success')
+      expect({ top, left }).toEqual({ top: options.y, left: options.x })
+    })
+    await assertNoConsoleErrors(browser)
+  }
 
   const scrollTo = async (
     browser: BrowserInterface,
@@ -93,6 +91,23 @@ describe('router autoscrolling on navigation', () => {
       await browser.eval(`window.router.push("/10/100/100/1000/page2")`)
       await waitForScrollToComplete(browser, { x: 0, y: 0 })
     })
+
+    it('should scroll to top of document with new metadata', async () => {
+      const browser = await webdriver(next.url, '/')
+
+      // 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="/new-metadata"]').click()
+      expect(
+        await browser.eval('document.documentElement.scrollHeight')
+      ).toBeGreaterThan(0)
+      await waitForScrollToComplete(browser, { x: 0, y: 0 })
+    })
   })
 
   describe('horizontal scroll', () => {
@@ -153,7 +168,7 @@ describe('router autoscrolling on navigation', () => {
           return (
             content +
             `
-      \\\\ Add this meaningless comment to force refresh
+      // Add this meaningless comment to force refresh
       `
           )
         })

diff --git a/test/lib/next-test-utils.ts b/test/lib/next-test-utils.ts
@@ -1553,8 +1553,9 @@ export async function assertNoConsoleErrors(browser: BrowserInterface) {
       log.source === 'warning' ||
       (log.source === 'error' &&
         // These are expected when we visit 404 pages.
-        log.message !==
-          'Failed to load resource: the server responded with a status of 404 (Not Found)')
+        !log.message.startsWith(
+          'Failed to load resource: the server responded with a status of 404'
+        ))
     )
   })
-Original file line number
+Diff line change
@@ Expand Up / @@ -31,6 +31,10 @@ export default function Page() { @@
               To sticky first element
             </Link>
           </div>
+          <div>
+            <Link href="/new-metadata">To new metadata</Link>
+          </div>
         </>
       )
     }