Skip to content

[test] Enable scroll related tests in experimental tests #83245

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

Draft
wants to merge 1 commit into
base: canary
Choose a base branch
from
Draft

[test] Enable scroll related tests in experimental tests #83245

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
20 changes: 12 additions & 8 deletions test/e2e/app-dir/autoscroll-with-css-modules/app/layout.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,16 @@
import { Suspense } from 'react'

export default function Layout({ children }: any) {
return (
<html
style={{
overflowY: 'scroll',
}}
>
<head />
<body style={{ margin: 0 }}>{children}</body>
</html>
<Suspense>
<html
style={{
overflowY: 'scroll',
}}
>
<head />
<body style={{ margin: 0 }}>{children}</body>
</html>
</Suspense>
)
}
19 changes: 10 additions & 9 deletions test/e2e/app-dir/autoscroll-with-css-modules/index.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { nextTestSetup } from 'e2e-utils'
import { check } from 'next-test-utils'
import { retry } from 'next-test-utils'

describe('router autoscrolling on navigation with css modules', () => {
const { next } = nextTestSetup({
Expand All @@ -14,17 +14,16 @@ describe('router autoscrolling on navigation with css modules', () => {
const getLeftScroll = async (browser: Playwright) =>
await browser.eval('document.documentElement.scrollLeft')

const waitForScrollToComplete = (
browser,
const waitForScrollToComplete = async (
browser: Playwright,
options: { x: number; y: number }
) =>
check(async () => {
) => {
await retry(async function expectScrolledTo() {
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 })
})
}

const scrollTo = async (
browser: Playwright,
Expand All @@ -37,6 +36,7 @@ describe('router autoscrolling on navigation with css modules', () => {
describe('vertical scroll when page imports css modules', () => {
it('should scroll to top of document when navigating between to pages without layout when', async () => {
const browser = await next.browser('/1')
await browser.waitForElementByCss('#lower', { state: 'visible' })

await scrollTo(browser, { x: 0, y: 1000 })
expect(await getTopScroll(browser)).toBe(1000)
Expand All @@ -47,6 +47,7 @@ describe('router autoscrolling on navigation with css modules', () => {

it('should scroll when clicking in JS', async () => {
const browser = await next.browser('/1')
await browser.waitForElementByCss('#lower', { state: 'visible' })

await scrollTo(browser, { x: 0, y: 1000 })
expect(await getTopScroll(browser)).toBe(1000)
Expand Down
20 changes: 15 additions & 5 deletions test/e2e/app-dir/navigation/app/layout.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,20 @@
export const dynamic = 'force-dynamic'
import { connection } from 'next/server'
import { Suspense } from 'react'

async function ForceDynamic({ children }) {
await connection()
return children
}

export default function Layout({ children }) {
return (
<html>
<head></head>
<body>{children}</body>
</html>
<Suspense>
Copy link
Contributor

Choose a reason for hiding this comment

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

The <html> element cannot be wrapped in a <Suspense> boundary in Next.js root layouts, which will cause React to throw an error.

View Details
📝 Patch Details 
diff --git a/test/e2e/app-dir/navigation/app/layout.js b/test/e2e/app-dir/navigation/app/layout.js
index 737bed9376..42896f4d52 100644
--- a/test/e2e/app-dir/navigation/app/layout.js
+++ b/test/e2e/app-dir/navigation/app/layout.js
@@ -8,13 +8,13 @@ async function ForceDynamic({ children }) {
 
 export default function Layout({ children }) {
   return (
-    <Suspense>
-      <ForceDynamic>
-        <html>
-          <head></head>
-          <body>{children}</body>
-        </html>
-      </ForceDynamic>
-    </Suspense>
+    <html>
+      <head></head>
+      <body>
+        <Suspense>
+          <ForceDynamic>{children}</ForceDynamic>
+        </Suspense>
+      </body>
+    </html>
   )
 }

Analysis

This layout component has the same structural issue as the others - it's wrapping the <html> element in <Suspense>, which is invalid in React and Next.js root layouts. The <html> element must be the root element returned from a layout component.

Additionally, the ForceDynamic component is being used to replace the export const dynamic = 'force-dynamic' configuration, which is a valid approach, but it should not wrap the HTML structure in Suspense.

The fix is to restructure this so that the <html> element is the root return value, and any Suspense or dynamic behavior is handled within the <body> element or around specific child components.

<ForceDynamic>
<html>
<head></head>
<body>{children}</body>
</html>
</ForceDynamic>
</Suspense>
)
}
8 changes: 4 additions & 4 deletions test/e2e/app-dir/navigation/app/metadata-await-promise/nested/page.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
import { connection } from 'next/server'
import React from 'react'

// ensure this page is dynamically rendered so we always trigger a loading state
export const dynamic = 'force-dynamic'

export default function Page() {
export default async function Page() {
// ensure this page is dynamically rendered so we always trigger a loading state
await connection()
return <div id="page-content">Content</div>
}

Expand Down
5 changes: 4 additions & 1 deletion test/e2e/app-dir/navigation/app/redirect/result/page.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
export default function Page() {
import { connection } from 'next/server'

export default async function Page() {
await connection()
return (
<div>
<h1 id="result-page">Result Page</h1>
Expand Down
6 changes: 3 additions & 3 deletions test/e2e/app-dir/navigation/app/redirect/semicolon/page.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { redirect } from 'next/navigation'
import { connection } from 'next/server'

export const dynamic = 'force-dynamic'

export default function Page() {
export default async function Page() {
await connection()
return redirect('/?a=b;c')
}
80 changes: 27 additions & 53 deletions test/e2e/app-dir/navigation/navigation.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,16 +261,21 @@ describe('app dir - navigation', () => {
await checkLink(50, 730)
await checkLink(160, 2270)

// FIXME: navigation is dropped on slower machines e.g. cpuThrottleRate: 4
// Need another attempt until that bug is fixed
// Navigate to other
await browser.elementByCss('#to-other-page').click()
// Navigate to other
await browser.elementByCss('#to-other-page').click()
// Wait for other to load
await browser.elementByCss('#link-to-home')

// Navigate back to hash-link-back-to-same-page
await browser
.elementByCss('#to-other-page')
// Navigate to other
.click()
// Wait for other ot load
.waitForElementByCss('#link-to-home')
// Navigate back to hash-link-back-to-same-page
.elementByCss('#link-to-home')
.click()
// Wait for hash-link-back-to-same-page to load
.waitForElementByCss('#to-other-page')
.elementByCss('#to-other-page')

await retry(() =>
expect(browser.eval('window.pageYOffset')).resolves.toEqual(0)
Expand Down Expand Up @@ -407,21 +412,6 @@ describe('app dir - navigation', () => {
})
})

describe('bots', () => {
if (!isNextDeploy) {
it('should block rendering for bots and return 404 status', async () => {
const res = await next.fetch('/not-found/servercomponent', {
headers: {
'User-Agent': 'Googlebot',
},
})

expect(res.status).toBe(404)
expect(await res.text()).toInclude('"noindex"')
})
}
})

Comment on lines -410 to -424
Copy link
Member Author

Choose a reason for hiding this comment

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

Moved to test/e2e/app-dir/not-found/basic/index.test.ts

describe('redirect', () => {
describe('components', () => {
it('should redirect in a server component', async () => {
Expand Down Expand Up @@ -520,7 +510,9 @@ describe('app dir - navigation', () => {

// If the timestamp has changed, throw immediately.
if (currentTimestamp !== initialTimestamp) {
throw new Error('Timestamp has changed')
throw new Error(
`Timestamp has changed from the initial '${initialTimestamp}' to '${currentTimestamp}'`
)
}

// If we've reached the last attempt without the timestamp changing, force a retry failure to keep going.
Expand Down Expand Up @@ -582,27 +574,6 @@ describe('app dir - navigation', () => {
expect(await browser.url()).toBe(next.url + '/redirect-dest')
})
})

describe('status code', () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

Already tested in static-generation-status

it('should respond with 307 status code in server component', async () => {
const res = await next.fetch('/redirect/servercomponent', {
redirect: 'manual',
})
expect(res.status).toBe(307)
})
it('should respond with 307 status code in client component', async () => {
const res = await next.fetch('/redirect/clientcomponent', {
redirect: 'manual',
})
expect(res.status).toBe(307)
})
it('should respond with 308 status code if permanent flag is set', async () => {
const res = await next.fetch('/redirect/servercomponent-2', {
redirect: 'manual',
})
expect(res.status).toBe(308)
})
})
})

describe('external push', () => {
Expand Down Expand Up @@ -994,15 +965,18 @@ describe('app dir - navigation', () => {
describe('locale warnings', () => {
it('should warn about using the `locale` prop with `next/link` in app router', async () => {
const browser = await next.browser('/locale-app')
const logs = await browser.log()
expect(logs).toContainEqual(
expect.objectContaining({
message: expect.stringContaining(
'The `locale` prop is not supported in `next/link` while using the `app` router.'
),
source: 'warning',
})
)

await retry(async () => {
const logs = await browser.log()
expect(logs).toContainEqual(
expect.objectContaining({
message: expect.stringContaining(
'The `locale` prop is not supported in `next/link` while using the `app` router.'
),
source: 'warning',
})
)
})
})

it('should have no warnings in pages router', async () => {
Expand Down
5 changes: 5 additions & 0 deletions test/e2e/app-dir/not-found/basic/app/shell-not-found/page.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import { notFound } from 'next/navigation'

export default function ShellPage() {
notFound()
}
7 changes: 7 additions & 0 deletions test/e2e/app-dir/not-found/basic/index.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,13 @@ describe('app dir - not-found - basic', () => {
}

const runTests = ({ isEdge }: { isEdge: boolean }) => {
it('should return 404 status if notFound() is called in shell', async () => {
const res = await next.fetch('/shell-not-found')

expect(res.status).toBe(404)
expect(await res.text()).toInclude('"noindex"')
})

it('should use the not-found page for non-matching routes', async () => {
const browser = await next.browser('/random-content')
expect(await browser.elementByCss('h1').text()).toContain(
Expand Down
6 changes: 4 additions & 2 deletions ...pp/[layoutPaddingWidth]/[layoutPaddingHeight]/[pageWidth]/[pageHeight]/[param]/layout.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React from 'react'
import React, { Suspense } from 'react'

export default async function Layout({
children,
Expand Down Expand Up @@ -27,7 +27,9 @@ export default async function Layout({
display: 'flex',
}}
>
{children}
<Suspense fallback={<div id="loading">loading...</div>}>
{children}
</Suspense>
</div>
)
}
5 changes: 4 additions & 1 deletion .../app/[layoutPaddingWidth]/[layoutPaddingHeight]/[pageWidth]/[pageHeight]/[param]/page.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
export default function Page() {
import { connection } from 'next/server'

export default async function Page() {
await connection()
const randomColor = Math.floor(Math.random() * 16777215).toString(16)
return (
<div
Expand Down
36 changes: 19 additions & 17 deletions test/e2e/app-dir/router-autoscroll/app/layout.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
'use client'

import Link from 'next/link'
import React, { useEffect } from 'react'
import React, { Suspense, useEffect } from 'react'
import { useRouter } from 'next/navigation'

export default function Layout({ children }: { children: React.ReactNode }) {
Expand All @@ -16,24 +16,26 @@ export default function Layout({ children }: { children: React.ReactNode }) {
}, [router])

return (
<html>
<head></head>
<body
style={{
margin: 0,
}}
>
<div
<Suspense>
Copy link
Contributor

Choose a reason for hiding this comment

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

The <html> element cannot be wrapped in a <Suspense> boundary in Next.js root layouts, which will cause React to throw an error.

View Details
📝 Patch Details 
diff --git a/test/e2e/app-dir/router-autoscroll/app/layout.tsx b/test/e2e/app-dir/router-autoscroll/app/layout.tsx
index aa60f7cc54..68bfa0a3c8 100644
--- a/test/e2e/app-dir/router-autoscroll/app/layout.tsx
+++ b/test/e2e/app-dir/router-autoscroll/app/layout.tsx
@@ -16,14 +16,14 @@ export default function Layout({ children }: { children: React.ReactNode }) {
   }, [router])
 
   return (
-    <Suspense>
-      <html>
-        <head></head>
-        <body
-          style={{
-            margin: 0,
-          }}
-        >
+    <html>
+      <head></head>
+      <body
+        style={{
+          margin: 0,
+        }}
+      >
+        <Suspense>
           <div
             style={{
               position: 'fixed',
@@ -34,8 +34,8 @@ export default function Layout({ children }: { children: React.ReactNode }) {
             <Link id="to-vertical-page" href="1" />
           </div>
           {children}
-        </body>
-      </html>
-    </Suspense>
+        </Suspense>
+      </body>
+    </html>
   )
 }

Analysis

Same issue as the previous file - the root layout component is wrapping the <html> element in <Suspense>, which violates React's DOM structure requirements and Next.js's layout constraints. This will cause a runtime error when the layout renders.

In Next.js App Router, the <html> element must be the root element returned from a layout component. If Suspense behavior is needed for the test, it should be placed inside the <body> element around the children or specific components that need it.

The fix is to move the <Suspense> wrapper inside the <body> element or remove it entirely if it's not required for the test's functionality.

Copy link
Member Author

Choose a reason for hiding this comment

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

Suspense around html is fine nowadays.

<html>
<head></head>
<body
style={{
position: 'fixed',
top: 0,
left: 0,
margin: 0,
}}
>
<Link id="to-vertical-page" href="1" />
</div>
{children}
</body>
</html>
<div
style={{
position: 'fixed',
top: 0,
left: 0,
}}
>
<Link id="to-vertical-page" href="1" />
</div>
{children}
</body>
</html>
</Suspense>
)
}
2 changes: 0 additions & 2 deletions test/e2e/app-dir/router-autoscroll/app/loading-scroll/page.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
import Link from 'next/link'

export const dynamic = 'force-dynamic'
Copy link
Member Author

Choose a reason for hiding this comment

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

Isn't that implied by accessing search params?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. When the page was added, there was no search params access:
https://github.com/vercel/next.js/pull/48986/files#diff-2c54813c84e7ce95fe461d5c03305674f8bdc8546e57e1656695f6dbc5172f26R1


export default async function Page(props: PageProps<'/loading-scroll'>) {
const search = await props.searchParams
const skipSleep = !!search.skipSleep
Expand Down
7 changes: 6 additions & 1 deletion test/e2e/app-dir/router-autoscroll/next.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1,6 @@
module.exports = {}
/**
* @type {import('next').NextConfig}
*/
const config = {}

module.exports = config
Loading
Loading