From f8afd611df64abc1a1119224dda3bc0b358fcdeb Mon Sep 17 00:00:00 2001 From: "Sebastian \"Sebbie\" Silbermann" Date: Fri, 10 Jan 2025 19:03:59 +0100 Subject: [PATCH] Ensure metadata doesn't break scroll-to-top on navigation (#74748) --- .../src/client/components/layout-router.tsx | 8 +++ .../app-render/create-component-tree.tsx | 15 +++-- .../acceptance-app/hydration-error.test.ts | 55 ++++++++++--------- .../app/new-metadata/page.tsx | 17 ++++++ .../app-dir/router-autoscroll/app/page.tsx | 4 ++ .../router-autoscroll.test.ts | 41 +++++++++----- test/lib/next-test-utils.ts | 5 +- 7 files changed, 100 insertions(+), 45 deletions(-) create mode 100644 test/e2e/app-dir/router-autoscroll/app/new-metadata/page.tsx diff --git a/packages/next/src/client/components/layout-router.tsx b/packages/next/src/client/components/layout-router.tsx index 07a63326cce97..a8ab66802e785 100644 --- 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 + {metadata} {notFoundStyles} - ) : undefined const forbiddenElement = Forbidden ? ( <> + {metadata} {forbiddenStyles} - ) : undefined const unauthorizedElement = Unauthorized ? ( <> + {metadata} {unauthorizedStyles} - ) : undefined @@ -669,8 +669,13 @@ async function createComponentTreeInternal({ return [ actualSegment, - {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 which breaks scroll handling. + */} + {metadata} {layerAssets} @@ -805,12 +810,12 @@ async function createComponentTreeInternal({ notFound={ NotFound ? ( <> - {metadata} {layerAssets} {notFoundStyles} + {metadata} ) : undefined } diff --git a/test/development/acceptance-app/hydration-error.test.ts b/test/development/acceptance-app/hydration-error.test.ts index b81186149d41b..108c2e8fe5364 100644 --- 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', () => { - <__next_metadata_boundary__> > @@ -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', () => { - <__next_metadata_boundary__> - - + + +
-
- -

- - ... - + client - - server" + +

+ + ... + + client + - server" `) } else { expect(fullPseudoHtml).toMatchInlineSnapshot(` @@ -916,19 +922,18 @@ describe('Error overlay for hydration errors in App router', () => { - <__next_metadata_boundary__> - - + + +

-
- -

- - ... - + client - - server" + +

+ + ... + + 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 new file mode 100644 index 0000000000000..fb32ac1ac9550 --- /dev/null +++ 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) => ( +

{i}
+ )) + } + + ) +} diff --git a/test/e2e/app-dir/router-autoscroll/app/page.tsx b/test/e2e/app-dir/router-autoscroll/app/page.tsx index b0f000388bb17..a58ac993518ab 100644 --- 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
+ +
+ To new metadata +
) } diff --git a/test/e2e/app-dir/router-autoscroll/router-autoscroll.test.ts b/test/e2e/app-dir/router-autoscroll/router-autoscroll.test.ts index d89480bfa7550..05e67136ce8e4 100644 --- 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> - 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 index d4993ae128c66..57e99192267d4 100644 --- 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' + )) ) })