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

[not for merge] Flush out faulty scroll behavior #74755

Draft
wants to merge 3 commits into
base: canary
Choose a base branch
from
+100 −46
Draft

[not for merge] Flush out faulty scroll behavior #74755

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 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
f6c8578
[not for merge] Flush out faulty scroll behavior
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
3 changes: 2 additions & 1 deletion packages/next/errors.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -621,5 +621,6 @@
"620": "A required parameter (%s) was not provided as %s received %s in getStaticPaths for %s",
"621": "Required root params (%s) were not provided in generateStaticParams for %s, please provide at least one value for each.",
"622": "A required root parameter (%s) was not provided in generateStaticParams for %s, please provide at least one value.",
"623": "Invalid quality prop (%s) on \\`next/image\\` does not match \\`images.qualities\\` configured in your \\`next.config.js\\`\\nSee more info: https://nextjs.org/docs/messages/next-image-unconfigured-qualities"
"623": "Invalid quality prop (%s) on \\`next/image\\` does not match \\`images.qualities\\` configured in your \\`next.config.js\\`\\nSee more info: https://nextjs.org/docs/messages/next-image-unconfigured-qualities",
"624": "Tried to scroll to a head element. This is a bug in Next.js."
}
6 changes: 6 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,12 @@ 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 (domNode.parentElement?.localName === 'head') {
throw new 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}
{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)
})

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
`
)
})
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