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

…20606) This pull request makes `Router#push` and `Router#replace` function identically to `<Link />`, i.e. reset scroll when the new render is complete. Users can opt out of this new behavior via: ```tsx const path = '/my-page' router.push(path, path, { scroll: false }) ``` --- Fixes vercel#3249
freekrai · Dec 30, 2020 · 7646921 · 7646921
1 parent 3246274
commit 7646921
Show file tree

Hide file tree

Showing 4 changed files with 100 additions and 3 deletions.
diff --git a/packages/next/next-server/lib/router/router.ts b/packages/next/next-server/lib/router/router.ts
@@ -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) {

diff --git a/test/integration/build-output/test/index.test.js b/test/integration/build-output/test/index.test.js
@@ -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)

diff --git a/test/integration/client-navigation/pages/nav/long-page-to-snap-scroll.js b/test/integration/client-navigation/pages/nav/long-page-to-snap-scroll.js
@@ -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">
@@ -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>
   )
 }

diff --git a/test/integration/client-navigation/test/index.test.js b/test/integration/client-navigation/test/index.test.js
@@ -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(
@@ -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', () => {