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.

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

fix(router): consistent scroll behavior for Link/Router#push #20606

Merged
merged 3 commits into from
Dec 30, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 6 additions & 0 deletions packages/next/next-server/lib/router/router.ts
Original file line number Diff line number Diff line change
Expand Up @@ -649,6 +649,12 @@ export default class Router implements BaseRouter {
window.location.href = url
return false
}

// Default to scroll reset behavior unless explicitly specified to be
// `false`! This makes the behavior between using `Router#push` and a
// `<Link />` consistent.
options.scroll = !!(options.scroll ?? true)

let localeChange = options.locale !== this.locale

if (process.env.__NEXT_I18N_SUPPORT) {
Expand Down
2 changes: 1 addition & 1 deletion test/integration/build-output/test/index.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ describe('Build Output', () => {
expect(parseFloat(err404Size) - 3.7).toBeLessThanOrEqual(0)
expect(err404Size.endsWith('kB')).toBe(true)

expect(parseFloat(err404FirstLoad) - 65.2).toBeLessThanOrEqual(0)
expect(parseFloat(err404FirstLoad)).toBeCloseTo(65.3, 1)
expect(err404FirstLoad.endsWith('kB')).toBe(true)

expect(parseFloat(sharedByAll) - 61.8).toBeLessThanOrEqual(0)
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
import Link from 'next/link'
import { useRouter } from 'next/router'
import React from 'react'

const LongPageToSnapScroll = () => {
const router = useRouter()
return (
<div id="long-page-to-snap-scroll">
<Link href="#item-400">
Expand All @@ -17,8 +19,28 @@ const LongPageToSnapScroll = () => {
})}

<Link href="/snap-scroll-position">
<a id="goto-snap-scroll-position">Go to snap scroll</a>
<a id="goto-snap-scroll-position">Go to snap scroll declarative</a>
</Link>
<div
id="goto-snap-scroll-position-imperative"
onClick={(e) => {
e.preventDefault()
router.push('/snap-scroll-position')
}}
>
Go to snap scroll imperative
</div>
<div
id="goto-snap-scroll-position-imperative-noscroll"
onClick={(e) => {
e.preventDefault()
router.push('/snap-scroll-position', '/snap-scroll-position', {
scroll: false,
})
}}
>
Go to snap scroll imperative (no scroll)
</div>
</div>
)
}
Expand Down
71 changes: 70 additions & 1 deletion test/integration/client-navigation/test/index.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -445,7 +445,7 @@ describe('Client Navigation', () => {
})

describe('resets scroll at the correct time', () => {
it('should reset scroll before the new page runs its lifecycles', async () => {
it('should reset scroll before the new page runs its lifecycles (<Link />)', async () => {
let browser
try {
browser = await webdriver(
Expand Down Expand Up @@ -478,6 +478,75 @@ describe('Client Navigation', () => {
}
}
})

it('should reset scroll before the new page runs its lifecycles (Router#push)', async () => {
let browser
try {
browser = await webdriver(
context.appPort,
'/nav/long-page-to-snap-scroll'
)

// Scrolls to item 400 on the page
await browser
.waitForElementByCss('#long-page-to-snap-scroll')
.elementByCss('#scroll-to-item-400')
.click()

const scrollPosition = await browser.eval('window.pageYOffset')
expect(scrollPosition).toBe(7208)

// Go to snap scroll page
await browser
.elementByCss('#goto-snap-scroll-position-imperative')
.click()
.waitForElementByCss('#scroll-pos-y')

const snappedScrollPosition = await browser.eval(
'document.getElementById("scroll-pos-y").innerText'
)
expect(snappedScrollPosition).toBe('0')
} finally {
if (browser) {
await browser.close()
}
}
})

it('should intentionally not reset scroll before the new page runs its lifecycles (Router#push)', async () => {
let browser
try {
browser = await webdriver(
context.appPort,
'/nav/long-page-to-snap-scroll'
)

// Scrolls to item 400 on the page
await browser
.waitForElementByCss('#long-page-to-snap-scroll')
.elementByCss('#scroll-to-item-400')
.click()

const scrollPosition = await browser.eval('window.pageYOffset')
expect(scrollPosition).toBe(7208)

// Go to snap scroll page
await browser
.elementByCss('#goto-snap-scroll-position-imperative-noscroll')
.click()
.waitForElementByCss('#scroll-pos-y')

const snappedScrollPosition = await browser.eval(
'document.getElementById("scroll-pos-y").innerText'
)
expect(snappedScrollPosition).not.toBe('0')
expect(Number(snappedScrollPosition)).toBeGreaterThanOrEqual(7208)
} finally {
if (browser) {
await browser.close()
}
}
})
})

describe('with hash changes', () => {
Expand Down