Update handling of autoscrolling on navigation in app (#43845)

- Add support for scrolling to the sides and down when navigating
- Add tests for vertical scrolling
- Add tests for horizontal scrolling
- Add tests for `router.refresh()`
	- should not scroll by itself
	- should not block router.push from scrolling
	- should not scroll when page refreshed using fash refresh
- Scroll to the top of document if that gets page into viewport
	- I didn't want to implement some heuristics on if we can scroll to the top of the page so I just scroll there and check.
	- This implementation may not play well with some nested scrollable containers (but that never worked, just FYI)
	
- Improved typings on `BrowserInterface` a little - backward compatible change.

Co-authored-by: Shu Ding <3676859+shuding@users.noreply.github.com>

packages/next/src/client/components/layout-router.tsx

@@ -24,6 +24,7 @@ import { createInfinitePromise } from './infinite-promise'
 import { ErrorBoundary } from './error-boundary'
 import { matchSegment } from './match-segments'
 import { useRouter } from './navigation'
+import { handleSmoothScroll } from '../../shared/lib/router/router'
 
 /**
  * Add refetch marker to router state at the point of the current layout segment.
@@ -103,11 +104,11 @@ function findDOMNode(
 }
 
 /**
- * Check if the top of the HTMLElement is in the viewport.
+ * Check if the top corner of the HTMLElement is in the viewport.
  */
-function topOfElementInViewport(element: HTMLElement) {
+function topOfElementInViewport(element: HTMLElement, viewportHeight: number) {
   const rect = element.getBoundingClientRect()
-  return rect.top >= 0
+  return rect.top >= 0 && rect.top <= viewportHeight
 }
 
 class ScrollAndFocusHandler extends React.Component<{
@@ -122,20 +123,39 @@ class ScrollAndFocusHandler extends React.Component<{
     if (focusAndScrollRef.apply && domNode instanceof HTMLElement) {
       // State is mutated to ensure that the focus and scroll is applied only once.
       focusAndScrollRef.apply = false
+
+      handleSmoothScroll(
+        () => {
+          // Store the current viewport height because reading `clientHeight` causes a reflow,
+          // and it won't change during this function.
+          const htmlElement = document.documentElement
+          const viewportHeight = htmlElement.clientHeight
+
+          // If the element's top edge is already in the viewport, exit early.
+          if (topOfElementInViewport(domNode, viewportHeight)) {
+            return
+          }
+
+          // Otherwise, try scrolling go the top of the document to be backward compatible with pages
+          // scrollIntoView() called on `<html/>` element scrolls horizontally on chrome and firefox (that shouldn't happen)
+          // We could use it to scroll horizontally following RTL but that also seems to be broken - it will always scroll left
+          // scrollLeft = 0 also seems to ignore RTL and manually checking for RTL is too much hassle so we will scroll just vertically
+          htmlElement.scrollTop = 0
+
+          // Scroll to domNode if domNode is not in viewport when scrolled to top of document
+          if (!topOfElementInViewport(domNode, viewportHeight)) {
+            // Scroll into view doesn't scroll horizontally by default when not needed
+            domNode.scrollIntoView()
+          }
+        },
+        {
+          // We will force layout by querying domNode position
+          dontForceLayout: true,
+        }
+      )
+
       // Set focus on the element
       domNode.focus()
-      // Only scroll into viewport when the layout is not visible currently.
-      if (!topOfElementInViewport(domNode)) {
-        const htmlElement = document.documentElement
-        const existing = htmlElement.style.scrollBehavior
-        htmlElement.style.scrollBehavior = 'auto'
-        // In Chrome-based browsers we need to force reflow before calling `scrollTo`.
-        // Otherwise it will not pickup the change in scrollBehavior
-        // More info here: https://github.com/vercel/next.js/issues/40719#issuecomment-1336248042
-        htmlElement.getClientRects()
-        domNode.scrollIntoView()
-        htmlElement.style.scrollBehavior = existing
-      }
     }
   }
 

packages/next/src/client/index.tsx

@@ -10,6 +10,7 @@ import { RouterContext } from '../shared/lib/router-context'
 import {
   AppComponent,
   AppProps,
+  handleSmoothScroll,
   PrivateRouteInfo,
 } from '../shared/lib/router/router'
 import { isDynamicRoute } from '../shared/lib/router/utils/is-dynamic'
@@ -691,15 +692,10 @@ function doRender(input: RenderRouteInfo): Promise<any> {
     }
 
     if (input.scroll) {
-      const htmlElement = document.documentElement
-      const existing = htmlElement.style.scrollBehavior
-      htmlElement.style.scrollBehavior = 'auto'
-      // In Chrome-based browsers we need to force reflow before calling `scrollTo`.
-      // Otherwise it will not pickup the change in scrollBehavior
-      // More info here: https://github.com/vercel/next.js/issues/40719#issuecomment-1336248042
-      htmlElement.getClientRects()
-      window.scrollTo(input.scroll.x, input.scroll.y)
-      htmlElement.style.scrollBehavior = existing
+      const { x, y } = input.scroll
+      handleSmoothScroll(() => {
+        window.scrollTo(x, y)
+      })
     }
   }
 

packages/next/src/shared/lib/router/router.ts

@@ -672,14 +672,23 @@ interface FetchNextDataParams {
   unstable_skipClientCache?: boolean
 }
 
-function handleSmoothScroll(fn: () => void) {
+/**
+ * Run function with `scroll-behavior: auto` applied to `<html/>`.
+ * This css change will be reverted after the function finishes.
+ */
+export function handleSmoothScroll(
+  fn: () => void,
+  options: { dontForceLayout?: boolean } = {}
+) {
   const htmlElement = document.documentElement
   const existing = htmlElement.style.scrollBehavior
   htmlElement.style.scrollBehavior = 'auto'
-  // In Chrome-based browsers we need to force reflow before calling `scrollTo`.
-  // Otherwise it will not pickup the change in scrollBehavior
-  // More info here: https://github.com/vercel/next.js/issues/40719#issuecomment-1336248042
-  htmlElement.getClientRects()
+  if (!options.dontForceLayout) {
+    // In Chrome-based browsers we need to force reflow before calling `scrollTo`.
+    // Otherwise it will not pickup the change in scrollBehavior
+    // More info here: https://github.com/vercel/next.js/issues/40719#issuecomment-1336248042
+    htmlElement.getClientRects()
+  }
   fn()
   htmlElement.style.scrollBehavior = existing
 }

test/e2e/app-dir/router-autoscroll.test.ts

@@ -0,0 +1,216 @@
+import path from 'path'
+import fs from 'fs-extra'
+import webdriver from 'next-webdriver'
+import { createNext, FileRef } from 'e2e-utils'
+import { NextInstance } from 'test/lib/next-modes/base'
+import { waitFor } from 'next-test-utils'
+
+describe('router autoscrolling on navigation', () => {
+  let next: NextInstance
+
+  const filesPath = path.join(__dirname, './router-autoscroll')
+  beforeAll(async () => {
+    next = await createNext({
+      files: new FileRef(filesPath),
+      dependencies: {
+        react: 'latest',
+        'react-dom': 'latest',
+        typescript: 'latest',
+        '@types/react': 'latest',
+        '@types/node': 'latest',
+      },
+    })
+  })
+  afterAll(() => next.destroy())
+
+  const isReact17 = process.env.NEXT_TEST_REACT_VERSION === '^17'
+  if (isReact17) {
+    it('should skip tests for react 17', () => {})
+    return
+  }
+
+  /** These is no clear API so we just wait a really long time to avoid flakiness */
+  const waitForScrollToComplete = () => waitFor(1000)
+
+  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 scrollTo = async (
+    browser: BrowserInterface,
+    options: { x: number; y: number }
+  ) => {
+    await browser.eval(`window.scrollTo(${options.x}, ${options.y})`)
+    await waitForScrollToComplete()
+  }
+
+  describe('vertical scroll', () => {
+    it('should scroll to top of document when navigating between to pages without layout', async () => {
+      const browser = await webdriver(next.url, '/0/0/100/10000/page1')
+
+      await scrollTo(browser, { x: 0, y: 1000 })
+      expect(await getTopScroll(browser)).toBe(1000)
+
+      await browser.eval(`window.router.push("/0/0/100/10000/page2")`)
+      await waitForScrollToComplete()
+
+      expect(await getTopScroll(browser)).toBe(0)
+
+      browser.quit()
+    })
+
+    it("should scroll to top of page when scrolling to phe top of the document wouldn't have the page in the viewport", async () => {
+      const browser = await webdriver(next.url, '/0/1000/100/1000/page1')
+
+      await scrollTo(browser, { x: 0, y: 1500 })
+      expect(await getTopScroll(browser)).toBe(1500)
+
+      await browser.eval(`window.router.push("/0/1000/100/1000/page2")`)
+      await waitForScrollToComplete()
+
+      expect(await getTopScroll(browser)).toBe(1000)
+
+      browser.quit()
+    })
+
+    it("should scroll down to the navigated page when it's below viewort", async () => {
+      const browser = await webdriver(next.url, '/0/1000/100/1000/page1')
+      expect(await getTopScroll(browser)).toBe(0)
+
+      await browser.eval(`window.router.push("/0/1000/100/1000/page2")`)
+      await waitForScrollToComplete()
+
+      expect(await getTopScroll(browser)).toBe(1000)
+
+      browser.quit()
+    })
+
+    it('should not scroll when the top of the page is in the viewport', async () => {
+      const browser = await webdriver(next.url, '/10/1000/100/1000/page1')
+
+      await scrollTo(browser, { x: 0, y: 800 })
+      expect(await getTopScroll(browser)).toBe(800)
+
+      await browser.eval(`window.router.push("/10/1000/100/1000/page2")`)
+      await waitForScrollToComplete()
+
+      expect(await getTopScroll(browser)).toBe(800)
+
+      browser.quit()
+    })
+
+    it('should not scroll to top of document if page in viewport', async () => {
+      const browser = await webdriver(next.url, '/10/100/100/1000/page1')
+
+      await scrollTo(browser, { x: 0, y: 50 })
+      expect(await getTopScroll(browser)).toBe(50)
+
+      await browser.eval(`window.router.push("/10/100/100/1000/page2")`)
+      await waitForScrollToComplete()
+
+      expect(await getTopScroll(browser)).toBe(50)
+
+      browser.quit()
+    })
+
+    it('should scroll to top of document if possible while giving focus to page', async () => {
+      const browser = await webdriver(next.url, '/10/100/100/1000/page1')
+
+      await scrollTo(browser, { x: 0, y: 200 })
+      expect(await getTopScroll(browser)).toBe(200)
+
+      await browser.eval(`window.router.push("/10/100/100/1000/page2")`)
+      await waitForScrollToComplete()
+
+      expect(await getTopScroll(browser)).toBe(0)
+
+      browser.quit()
+    })
+  })
+
+  describe('horizontal scroll', () => {
+    it("should't scroll horizontally", async () => {
+      const browser = await webdriver(next.url, '/0/0/10000/10000/page1')
+
+      await scrollTo(browser, { x: 1000, y: 1000 })
+      expect(await getLeftScroll(browser)).toBe(1000)
+      expect(await getTopScroll(browser)).toBe(1000)
+
+      await browser.eval(`window.router.push("/0/0/10000/10000/page2")`)
+      await waitForScrollToComplete()
+
+      expect(await getLeftScroll(browser)).toBe(1000)
+      expect(await getTopScroll(browser)).toBe(0)
+
+      browser.quit()
+    })
+  })
+
+  describe('router.refresh()', () => {
+    it('should not scroll when called alone', async () => {
+      const browser = await webdriver(next.url, '/10/10000/100/1000/page1')
+
+      await scrollTo(browser, { x: 0, y: 12000 })
+      expect(await getTopScroll(browser)).toBe(12000)
+
+      await browser.eval(`window.router.refresh()`)
+      await waitForScrollToComplete()
+
+      expect(await getTopScroll(browser)).toBe(12000)
+
+      browser.quit()
+    })
+
+    // TODO fix next js to pass this
+    it.skip('should not stop router.push() from scrolling', async () => {
+      const browser = await webdriver(next.url, '/10/10000/100/1000/page1')
+
+      await scrollTo(browser, { x: 0, y: 12000 })
+      expect(await getTopScroll(browser)).toBe(12000)
+
+      await browser.eval(`
+        window.React.startTransition(() => {
+          window.router.push('/10/10000/100/1000/page2')
+          window.router.refresh()
+        })
+      `)
+      await waitForScrollToComplete()
+
+      expect(await getTopScroll(browser)).toBe(10000)
+
+      browser.quit()
+    })
+
+    // Test hot reloading only in development
+    ;((global as any).isDev ? it : it.skip)(
+      'should not scroll the page when we hot reload',
+      async () => {
+        const browser = await webdriver(next.url, '/10/10000/100/1000/page1')
+
+        await scrollTo(browser, { x: 0, y: 12000 })
+        expect(await getTopScroll(browser)).toBe(12000)
+
+        const pagePath =
+          'app/[layoutPaddingWidth]/[layoutPaddingHeight]/[pageWidth]/[pageHeight]/[param]/page.tsx'
+
+        await browser.eval(`window.router.refresh()`)
+        await next.patchFile(
+          pagePath,
+          fs.readFileSync(path.join(filesPath, pagePath)).toString() +
+            `
+        \\\\ Add this meaningless comment to force refresh
+        `
+        )
+        await waitFor(1000)
+
+        expect(await getTopScroll(browser)).toBe(12000)
+
+        browser.quit()
+      }
+    )
+  })
+})

test/e2e/app-dir/router-autoscroll/app/[layoutPaddingWidth]/[layoutPaddingHeight]/[pageWidth]/[pageHeight]/[param]/layout.tsx

@@ -0,0 +1,31 @@
+import React from 'react'
+
+export default function Layout({
+  children,
+  params: { layoutPaddingHeight, layoutPaddingWidth, pageWidth, pageHeight },
+}: {
+  children: React.ReactNode
+  params: {
+    layoutPaddingWidth: string
+    layoutPaddingHeight: string
+    pageWidth: string
+    pageHeight: string
+  }
+}) {
+  return (
+    <div
+      style={{
+        height: Number(pageHeight),
+        width: Number(pageWidth),
+        paddingTop: Number(layoutPaddingHeight),
+        paddingBottom: Number(layoutPaddingHeight),
+        paddingLeft: Number(layoutPaddingWidth),
+        paddingRight: Number(layoutPaddingWidth),
+        background: 'pink',
+        display: 'flex',
+      }}
+    >
+      {children}
+    </div>
+  )
+}

test/e2e/app-dir/router-autoscroll/app/[layoutPaddingWidth]/[layoutPaddingHeight]/[pageWidth]/[pageHeight]/[param]/page.tsx

@@ -0,0 +1,12 @@
+export default function Page() {
+  const randomColor = Math.floor(Math.random() * 16777215).toString(16)
+  return (
+    <div
+      id="page"
+      style={{
+        background: `#${randomColor}`,
+        flexGrow: 1,
+      }}
+    />
+  )
+}

test/e2e/app-dir/router-autoscroll/app/layout.tsx

@@ -0,0 +1,39 @@
+'use client'
+
+import Link from 'next/link'
+import React, { useEffect } from 'react'
+import { useRouter } from 'next/navigation'
+
+export default function Layout({ children }: { children: React.ReactNode }) {
+  const router = useRouter()
+
+  // We export these so that we can access them from tests
+  useEffect(() => {
+    // @ts-ignore
+    window.router = router
+    // @ts-ignore
+    window.React = React
+  }, [router])
+
+  return (
+    <html>
+      <head></head>
+      <body
+        style={{
+          margin: 0,
+        }}
+      >
+        <div
+          style={{
+            position: 'fixed',
+            top: 0,
+            left: 0,
+          }}
+        >
+          <Link id="to-vertical-page" href="1" />
+        </div>
+        {children}
+      </body>
+    </html>
+  )
+}

test/e2e/app-dir/router-autoscroll/next.config.js

@@ -0,0 +1,6 @@
+module.exports = {
+  strictMode: true,
+  experimental: {
+    appDir: true,
+  },
+}

test/lib/browsers/base.ts

@@ -116,10 +116,18 @@ export class BrowserInterface implements PromiseLike<any> {
   ): Promise<void> {}
   async get(url: string): Promise<void> {}
 
-  async getValue(): Promise<any> {}
-  async getAttribute(name: string): Promise<any> {}
-  async eval(snippet: string | Function): Promise<any> {}
-  async evalAsync(snippet: string | Function): Promise<any> {}
+  async getValue<T = any>(): Promise<T> {
+    return
+  }
+  async getAttribute<T = any>(name: string): Promise<T> {
+    return
+  }
+  async eval<T = any>(snippet: string | Function): Promise<T> {
+    return
+  }
+  async evalAsync<T = any>(snippet: string | Function): Promise<T> {
+    return
+  }
   async text(): Promise<string> {
     return ''
   }

test/lib/browsers/playwright.ts

@@ -277,8 +277,8 @@ export class Playwright extends BrowserInterface {
     }) as any
   }
 
-  async getAttribute(attr) {
-    return this.chain((el) => el.getAttribute(attr))
+  async getAttribute<T = any>(attr) {
+    return this.chain((el) => el.getAttribute(attr)) as T
   }
 
   hasElementByCssSelector(selector: string) {
@@ -359,7 +359,7 @@ export class Playwright extends BrowserInterface {
     )
   }
 
-  async evalAsync(snippet) {
+  async evalAsync<T = any>(snippet) {
     if (typeof snippet === 'function') {
       snippet = snippet.toString()
     }
@@ -377,7 +377,7 @@ export class Playwright extends BrowserInterface {
       })()`
     }
 
-    return page.evaluate(snippet).catch(() => null)
+    return page.evaluate<T>(snippet).catch(() => null)
   }
 
   async log() {

test/lib/browsers/selenium.ts

@@ -300,8 +300,8 @@ export class Selenium extends BrowserInterface {
     }) as any
   }
 
-  async getAttribute(attr) {
-    return this.chain((el) => el.getAttribute(attr))
+  async getAttribute<T = any>(attr) {
+    return this.chain((el) => el.getAttribute(attr)) as T
   }
 
   async hasElementByCssSelector(selector: string) {
@@ -334,18 +334,18 @@ export class Selenium extends BrowserInterface {
     )
   }
 
-  async eval(snippet) {
+  async eval<T = any>(snippet) {
     if (typeof snippet === 'string' && !snippet.startsWith('return')) {
       snippet = `return ${snippet}`
     }
-    return browser.executeScript(snippet)
+    return browser.executeScript<T>(snippet)
   }
 
-  async evalAsync(snippet) {
+  async evalAsync<T = any>(snippet) {
     if (typeof snippet === 'string' && !snippet.startsWith('return')) {
       snippet = `return ${snippet}`
     }
-    return browser.executeAsyncScript(snippet)
+    return browser.executeAsyncScript<T>(snippet)
   }
 
   async log() {