Skip to content
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

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

Merged
merged 2 commits into from
Jan 10, 2025
+100 −45
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
137ae4e
Failing test
eps1lon Jan 10, 2025
3fee0b4
Ensure metadata doesn't break scroll-to-top on navigation
eps1lon Jan 10, 2025
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
8 changes: 8 additions & 0 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 @@ -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
Expand Down
15 changes: 10 additions & 5 deletions packages/next/src/server/app-render/create-component-tree.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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}
Comment on lines +673 to +678
Copy link
Member Author

@eps1lon eps1lon Jan 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With Fragment Refs, we wouldn't have this problem. We'd just iterate over the children and skip over metadata.

Or rather skip over any hoistable

{layerAssets}
<OutletBoundary>
<MetadataOutlet ready={getViewportReady} />
Expand Down Expand Up @@ -805,12 +810,12 @@ async function createComponentTreeInternal({
notFound={
NotFound ? (
<>
{metadata}
{layerAssets}
<SegmentComponent params={params}>
{notFoundStyles}
<NotFound />
</SegmentComponent>
{metadata}
</>
) : undefined
}
Expand Down
55 changes: 30 additions & 25 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 @@ -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(
Expand Down Expand Up @@ -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>
Expand Down Expand Up @@ -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)
})
Comment on lines -570 to +573
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already did that in some other instances where we assert on multiple errors.


const description = await session.getRedboxDescription()
expect(description).toContain(
Expand Down Expand Up @@ -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`
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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(`
Expand All @@ -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"
`)
}
})
Expand Down
17 changes: 17 additions & 0 deletions test/e2e/app-dir/router-autoscroll/app/new-metadata/page.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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>
))
}
</>
)
}
4 changes: 4 additions & 0 deletions test/e2e/app-dir/router-autoscroll/app/page.tsx
View file
Open in desktop
Original file line number Diff 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>
</>
)
}
41 changes: 28 additions & 13 deletions test/e2e/app-dir/router-autoscroll/router-autoscroll.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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,
Expand Down Expand Up @@ -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', () => {
Expand Down Expand Up @@ -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
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This wasn't a comment and just triggered a build error polluting test assertions i.e. fixes

Error:   × Expected unicode escape
    ╭─[/Users/sebbie/repos/next.js/test/e2e/app-dir/router-autoscroll/app/[layoutPaddingWidth]/[layoutPaddingHeight]/[pageWidth]/[pageHeight]/[param]/page.tsx:14:1]
 11 │   )
 12 │ }
 13 │ 
 14 │ \\\\ Add this meaningless comment to force refresh
    · ▲
    ╰────

`
)
})
Expand Down
5 changes: 3 additions & 2 deletions test/lib/next-test-utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'
))
)
})

Expand Down
Loading