Skip to content

Commit

Permalink
Fix issue where layout-router wouldn't auto-scroll if we imported sty…
Browse files Browse the repository at this point in the history
…les or fonts (vercel#45487)

fixes vercel#42492
  • Loading branch information
jankaifer committed Feb 14, 2023
1 parent 74f8de0 commit dbfd39e
Show file tree
Hide file tree
Showing 9 changed files with 358 additions and 251 deletions.
3 changes: 3 additions & 0 deletions packages/next/src/client/components/layout-router.tsx
Expand Up @@ -118,6 +118,9 @@ class ScrollAndFocusHandler extends React.Component<{
componentDidMount() {
// Handle scroll and focus, it's only applied once in the first useEffect that triggers that changed.
const { focusAndScrollRef } = this.props

// `findDOMNode` is tricky because it returns just the first child if the component is a fragment.
// This already caused a bug where the first child was a <link/> in head.
const domNode = findDOMNode(this)

if (focusAndScrollRef.apply && domNode instanceof HTMLElement) {
Expand Down
6 changes: 4 additions & 2 deletions packages/next/src/server/app-render.tsx
Expand Up @@ -840,7 +840,8 @@ function getPreloadedFontFilesInlineLinkTags(
return null
}

return [...fontFiles]
// Sorting to make order deterministic
return [...fontFiles].sort()
}

function getScriptNonceFromHeader(cspHeaderValue: string): string | undefined {
Expand Down Expand Up @@ -1527,6 +1528,8 @@ export async function renderToHTMLOrFlight(
Component: () => {
return (
<>
{/* <Component /> needs to be the first element because we use `findDOMONode` in layout router to locate it. */}
<Component {...props} />
{preloadedFontFiles?.length === 0 ? (
<link
data-next-font={
Expand Down Expand Up @@ -1574,7 +1577,6 @@ export async function renderToHTMLOrFlight(
/>
))
: null}
<Component {...props} />
</>
)
},
Expand Down
29 changes: 29 additions & 0 deletions test/e2e/app-dir/autoscroll-with-css-modules/app/[num]/page.tsx
@@ -0,0 +1,29 @@
import Link from 'next/link'
import styles from './styles.module.css'

export default function Page({ params: { num } }) {
return (
<div>
{new Array(100).fill(0).map((_, i) => (
<div
key={i}
style={{
height: 100,
width: 100,
background: 'pink',
margin: 10,
}}
>
<Link id="lower" href={`/${Number(num) - 1}`}>
lower
</Link>
<div>{num}</div>
<Link id="higher" href={`/${Number(num) + 1}`}>
higher
</Link>
</div>
))}
<div className={styles.square} />
</div>
)
}
@@ -0,0 +1,6 @@
.square {
/* height: 100px;
width: 100px;
background: pink;
margin: 10px;*/
}
12 changes: 12 additions & 0 deletions test/e2e/app-dir/autoscroll-with-css-modules/app/layout.tsx
@@ -0,0 +1,12 @@
export default function Layout({ children }: any) {
return (
<html
style={{
overflowY: 'scroll',
}}
>
<head />
<body style={{ margin: 0 }}>{children}</body>
</html>
)
}
60 changes: 60 additions & 0 deletions test/e2e/app-dir/autoscroll-with-css-modules/index.test.ts
@@ -0,0 +1,60 @@
import { createNextDescribe } from 'e2e-utils'
import { check } from 'next-test-utils'

createNextDescribe(
'router autoscrolling on navigation',
{
files: __dirname,
},
({ next }) => {
type BrowserInterface = Awaited<ReturnType<typeof next['browser']>>

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,
options: { x: number; y: number }
) =>
check(async () => {
const top = await getTopScroll(browser)
const left = await getLeftScroll(browser)
return top === options.y && left === options.x
? 'success'
: JSON.stringify({ top, left })
}, 'success')

const scrollTo = async (
browser: BrowserInterface,
options: { x: number; y: number }
) => {
await browser.eval(`window.scrollTo(${options.x}, ${options.y})`)
await waitForScrollToComplete(browser, options)
}

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: BrowserInterface = await next.browser('/1')

await scrollTo(browser, { x: 0, y: 1000 })
expect(await getTopScroll(browser)).toBe(1000)

await browser.elementById('lower').click()
await waitForScrollToComplete(browser, { x: 0, y: 0 })
})

it('should scroll when clicking in JS', async () => {
const browser: BrowserInterface = await next.browser('/1')

await scrollTo(browser, { x: 0, y: 1000 })
expect(await getTopScroll(browser)).toBe(1000)

await browser.eval(() => document.getElementById('lower').click())
await waitForScrollToComplete(browser, { x: 0, y: 0 })
})
})
}
)
5 changes: 5 additions & 0 deletions test/e2e/app-dir/autoscroll-with-css-modules/next.config.js
@@ -0,0 +1,5 @@
/** @type {import("next").NextConfig} */
module.exports = {
reactStrictMode: true,
experimental: { appDir: true },
}
185 changes: 97 additions & 88 deletions test/e2e/app-dir/next-font/next-font.test.ts
@@ -1,6 +1,13 @@
import { createNextDescribe } from 'e2e-utils'
import { getRedboxSource, hasRedbox } from 'next-test-utils'

const getAttrs = (elems: Cheerio) =>
Array.from(elems)
.map((elem) => elem.attribs)
// There is something weord that causes different machines to have different order of things
// My machine behaves differently to CI
.sort((a, b) => (a.href < b.href ? -1 : 1))

createNextDescribe(
'app dir next-font',
{
Expand Down Expand Up @@ -219,31 +226,33 @@ createNextDescribe(
// Preconnect
expect($('link[rel="preconnect"]').length).toBe(0)

expect($('link[as="font"]').length).toBe(3)
expect($('link[as="font"]').get(0).attribs).toEqual({
as: 'font',
crossorigin: '',
href: '/_next/static/media/e9b9dc0d8ba35f48-s.p.woff2',
rel: 'preload',
type: 'font/woff2',
'data-next-font': 'size-adjust',
})
expect($('link[as="font"]').get(1).attribs).toEqual({
as: 'font',
crossorigin: '',
href: '/_next/static/media/b61859a50be14c53-s.p.woff2',
rel: 'preload',
type: 'font/woff2',
'data-next-font': 'size-adjust',
})
expect($('link[as="font"]').get(2).attribs).toEqual({
as: 'font',
crossorigin: '',
href: '/_next/static/media/b2104791981359ae-s.p.woff2',
rel: 'preload',
type: 'font/woff2',
'data-next-font': 'size-adjust',
})
// From root layout
expect(getAttrs($('link[as="font"]'))).toEqual([
{
as: 'font',
crossorigin: '',
href: '/_next/static/media/b2104791981359ae-s.p.woff2',
rel: 'preload',
type: 'font/woff2',
'data-next-font': 'size-adjust',
},
{
as: 'font',
crossorigin: '',
href: '/_next/static/media/b61859a50be14c53-s.p.woff2',
rel: 'preload',
type: 'font/woff2',
'data-next-font': 'size-adjust',
},
{
as: 'font',
crossorigin: '',
href: '/_next/static/media/e9b9dc0d8ba35f48-s.p.woff2',
rel: 'preload',
type: 'font/woff2',
'data-next-font': 'size-adjust',
},
])
})

it('should preload correctly with client components', async () => {
Expand All @@ -252,33 +261,33 @@ createNextDescribe(
// Preconnect
expect($('link[rel="preconnect"]').length).toBe(0)

expect($('link[as="font"]').length).toBe(3)
// From root layout
expect($('link[as="font"]').get(0).attribs).toEqual({
as: 'font',
crossorigin: '',
href: '/_next/static/media/e9b9dc0d8ba35f48-s.p.woff2',
rel: 'preload',
type: 'font/woff2',
'data-next-font': 'size-adjust',
})

expect($('link[as="font"]').get(1).attribs).toEqual({
as: 'font',
crossorigin: '',
href: '/_next/static/media/e1053f04babc7571-s.p.woff2',
rel: 'preload',
type: 'font/woff2',
'data-next-font': 'size-adjust',
})
expect($('link[as="font"]').get(2).attribs).toEqual({
as: 'font',
crossorigin: '',
href: '/_next/static/media/feab2c68f2a8e9a4-s.p.woff2',
rel: 'preload',
type: 'font/woff2',
'data-next-font': 'size-adjust',
})
expect(getAttrs($('link[as="font"]'))).toEqual([
{
as: 'font',
crossorigin: '',
href: '/_next/static/media/e1053f04babc7571-s.p.woff2',
rel: 'preload',
type: 'font/woff2',
'data-next-font': 'size-adjust',
},
{
as: 'font',
crossorigin: '',
href: '/_next/static/media/e9b9dc0d8ba35f48-s.p.woff2',
rel: 'preload',
type: 'font/woff2',
'data-next-font': 'size-adjust',
},
{
as: 'font',
crossorigin: '',
href: '/_next/static/media/feab2c68f2a8e9a4-s.p.woff2',
rel: 'preload',
type: 'font/woff2',
'data-next-font': 'size-adjust',
},
])
})

it('should preload correctly with layout using fonts', async () => {
Expand All @@ -287,25 +296,25 @@ createNextDescribe(
// Preconnect
expect($('link[rel="preconnect"]').length).toBe(0)

expect($('link[as="font"]').length).toBe(2)
// From root layout
expect($('link[as="font"]').get(0).attribs).toEqual({
as: 'font',
crossorigin: '',
href: '/_next/static/media/e9b9dc0d8ba35f48-s.p.woff2',
rel: 'preload',
type: 'font/woff2',
'data-next-font': 'size-adjust',
})

expect($('link[as="font"]').get(1).attribs).toEqual({
as: 'font',
crossorigin: '',
href: '/_next/static/media/75c5faeeb9c86969-s.p.woff2',
rel: 'preload',
type: 'font/woff2',
'data-next-font': 'size-adjust',
})
expect(getAttrs($('link[as="font"]'))).toEqual([
{
as: 'font',
crossorigin: '',
href: '/_next/static/media/75c5faeeb9c86969-s.p.woff2',
rel: 'preload',
type: 'font/woff2',
'data-next-font': 'size-adjust',
},
{
as: 'font',
crossorigin: '',
href: '/_next/static/media/e9b9dc0d8ba35f48-s.p.woff2',
rel: 'preload',
type: 'font/woff2',
'data-next-font': 'size-adjust',
},
])
})

it('should preload correctly with page using fonts', async () => {
Expand All @@ -314,25 +323,25 @@ createNextDescribe(
// Preconnect
expect($('link[rel="preconnect"]').length).toBe(0)

expect($('link[as="font"]').length).toBe(2)
// From root layout
expect($('link[as="font"]').get(0).attribs).toEqual({
as: 'font',
crossorigin: '',
href: '/_next/static/media/e9b9dc0d8ba35f48-s.p.woff2',
rel: 'preload',
type: 'font/woff2',
'data-next-font': 'size-adjust',
})

expect($('link[as="font"]').get(1).attribs).toEqual({
as: 'font',
crossorigin: '',
href: '/_next/static/media/568e4c6d8123c4d6-s.p.woff2',
rel: 'preload',
type: 'font/woff2',
'data-next-font': 'size-adjust',
})
expect(getAttrs($('link[as="font"]'))).toEqual([
{
as: 'font',
crossorigin: '',
href: '/_next/static/media/568e4c6d8123c4d6-s.p.woff2',
rel: 'preload',
type: 'font/woff2',
'data-next-font': 'size-adjust',
},
{
as: 'font',
crossorigin: '',
href: '/_next/static/media/e9b9dc0d8ba35f48-s.p.woff2',
rel: 'preload',
type: 'font/woff2',
'data-next-font': 'size-adjust',
},
])
})
})

Expand Down Expand Up @@ -361,7 +370,7 @@ createNextDescribe(
// Preconnect
expect($('link[rel="preconnect"]').length).toBe(0)
// Preload
expect($('link[as="font"]').length).toBe(0)
expect(getAttrs($('link[as="font"]'))).toEqual([])
})
})
}
Expand Down

0 comments on commit dbfd39e

Please sign in to comment.