Fix scroll when loading.js/ts is used (#48986)

### What? Whenever you navigated and a page suspended through `loading` or an error happened caught by `error` in the first level of segments (e.g. `/dashboard` but not `/dashboard/settings`) scroll would not be applied. This happened because the focus and scroll handling component is rendered as part of `InnerLayoutRouter` and the Suspense / Error boundary was rendered **around** `InnerLayoutRouter`. This behavior is incorrect as we still want to immediately scroll to the place where the loading is rendered. This PR fixes the behavior by allowing the scroll to apply to loading / error too. ### How? Moved the scrolling component around the loading/error/innerlayout boundary and added tests.
vercel · Apr 29, 2023 · 905b62e · 905b62e
1 parent 986039d
commit 905b62e
Show file tree

Hide file tree

Showing 5 changed files with 106 additions and 40 deletions.
diff --git a/packages/next/src/client/components/layout-router.tsx b/packages/next/src/client/components/layout-router.tsx
@@ -157,7 +157,7 @@ interface ScrollAndFocusHandlerProps {
   children: React.ReactNode
   segmentPath: FlightSegmentPath
 }
-class ScrollAndFocusHandler extends React.Component<ScrollAndFocusHandlerProps> {
+class InnerScrollAndFocusHandler extends React.Component<ScrollAndFocusHandlerProps> {
   handlePotentialScroll = () => {
     // Handle scroll and focus, it's only applied once in the first useEffect that triggers that changed.
     const { focusAndScrollRef, segmentPath } = this.props
@@ -268,6 +268,28 @@ class ScrollAndFocusHandler extends React.Component<ScrollAndFocusHandlerProps>
   }
 }
 
+function ScrollAndFocusHandler({
+  segmentPath,
+  children,
+}: {
+  segmentPath: FlightSegmentPath
+  children: React.ReactNode
+}) {
+  const context = useContext(GlobalLayoutRouterContext)
+  if (!context) {
+    throw new Error('invariant global layout router not mounted')
+  }
+
+  return (
+    <InnerScrollAndFocusHandler
+      segmentPath={segmentPath}
+      focusAndScrollRef={context.focusAndScrollRef}
+    >
+      {children}
+    </InnerScrollAndFocusHandler>
+  )
+}
+
 /**
  * InnerLayoutRouter handles rendering the provided segment based on the cache.
  */
@@ -296,7 +318,7 @@ function InnerLayoutRouter({
     throw new Error('invariant global layout router not mounted')
   }
 
-  const { changeByServerResponse, tree: fullTree, focusAndScrollRef } = context
+  const { changeByServerResponse, tree: fullTree } = context
 
   // Read segment path from the parallel router cache node.
   let childNode = childNodes.get(cacheKey)
@@ -418,14 +440,7 @@ function InnerLayoutRouter({
     </LayoutRouterContext.Provider>
   )
   // Ensure root layout is not wrapped in a div as the root layout renders `<html>`
-  return (
-    <ScrollAndFocusHandler
-      focusAndScrollRef={focusAndScrollRef}
-      segmentPath={segmentPath}
-    >
-      {subtree}
-    </ScrollAndFocusHandler>
-  )
+  return subtree
 }
 
 /**
@@ -551,34 +566,36 @@ export default function OuterLayoutRouter({
           <TemplateContext.Provider
             key={cacheKey}
             value={
-              <ErrorBoundary errorComponent={error} errorStyles={errorStyles}>
-                <LoadingBoundary
-                  hasLoading={hasLoading}
-                  loading={loading}
-                  loadingStyles={loadingStyles}
-                >
-                  <NotFoundBoundary
-                    notFound={notFound}
-                    notFoundStyles={notFoundStyles}
-                    asNotFound={asNotFound}
+              <ScrollAndFocusHandler segmentPath={segmentPath}>
+                <ErrorBoundary errorComponent={error} errorStyles={errorStyles}>
+                  <LoadingBoundary
+                    hasLoading={hasLoading}
+                    loading={loading}
+                    loadingStyles={loadingStyles}
                   >
-                    <RedirectBoundary>
-                      <InnerLayoutRouter
-                        parallelRouterKey={parallelRouterKey}
-                        url={url}
-                        tree={tree}
-                        childNodes={childNodesForParallelRouter!}
-                        childProp={isChildPropSegment ? childProp : null}
-                        segmentPath={segmentPath}
-                        cacheKey={cacheKey}
-                        isActive={
-                          currentChildSegmentValue === preservedSegmentValue
-                        }
-                      />
-                    </RedirectBoundary>
-                  </NotFoundBoundary>
-                </LoadingBoundary>
-              </ErrorBoundary>
+                    <NotFoundBoundary
+                      notFound={notFound}
+                      notFoundStyles={notFoundStyles}
+                      asNotFound={asNotFound}
+                    >
+                      <RedirectBoundary>
+                        <InnerLayoutRouter
+                          parallelRouterKey={parallelRouterKey}
+                          url={url}
+                          tree={tree}
+                          childNodes={childNodesForParallelRouter!}
+                          childProp={isChildPropSegment ? childProp : null}
+                          segmentPath={segmentPath}
+                          cacheKey={cacheKey}
+                          isActive={
+                            currentChildSegmentValue === preservedSegmentValue
+                          }
+                        />
+                      </RedirectBoundary>
+                    </NotFoundBoundary>
+                  </LoadingBoundary>
+                </ErrorBoundary>
+              </ScrollAndFocusHandler>
             }
           >
             <>

diff --git a/test/e2e/app-dir/router-autoscroll/app/loading-scroll/loading.tsx b/test/e2e/app-dir/router-autoscroll/app/loading-scroll/loading.tsx
@@ -0,0 +1,13 @@
+export default function Loading() {
+  return (
+    <>
+      <div id="loading-component">Loading component</div>
+      {
+        // Repeat 500 elements
+        Array.from({ length: 500 }, (_, i) => (
+          <div key={i}>Loading {i}...</div>
+        ))
+      }
+    </>
+  )
+}
diff --git a/test/e2e/app-dir/router-autoscroll/app/loading-scroll/page.tsx b/test/e2e/app-dir/router-autoscroll/app/loading-scroll/page.tsx
@@ -0,0 +1,17 @@
+export const dynamic = 'force-dynamic'
+
+export default async function Page() {
+  await new Promise((resolve) => setTimeout(resolve, 5000))
+  return (
+    <>
+      <div style={{ display: 'none' }}>Content that is hidden.</div>
+      <div id="content-that-is-visible">Content which is not hidden.</div>
+      {
+        // Repeat 500 elements
+        Array.from({ length: 500 }, (_, i) => (
+          <div key={i}>{i}</div>
+        ))
+      }
+    </>
+  )
+}
diff --git a/test/e2e/app-dir/router-autoscroll/app/page.tsx b/test/e2e/app-dir/router-autoscroll/app/page.tsx
@@ -9,9 +9,16 @@ export default function Page() {
           <div key={i}>{i}</div>
         ))
       }
-      <Link href="/invisible-first-element" id="to-invisible-first-element">
-        To invisible first element
-      </Link>
+      <div>
+        <Link href="/loading-scroll" id="to-loading-scroll">
+          To loading scroll
+        </Link>
+      </div>
+      <div>
+        <Link href="/invisible-first-element" id="to-invisible-first-element">
+          To invisible first element
+        </Link>
+      </div>
     </>
   )
 }
diff --git a/test/e2e/app-dir/router-autoscroll/router-autoscroll.test.ts b/test/e2e/app-dir/router-autoscroll/router-autoscroll.test.ts
@@ -170,6 +170,18 @@ createNextDescribe(
           .waitForElementByCss('#content-that-is-visible')
         await check(() => browser.eval('window.scrollY'), 0)
       })
+
+      it('Should apply scroll when loading.js is used', async () => {
+        const browser = await webdriver(next.url, '/')
+        await browser.eval('window.scrollTo(0, 500)')
+        await browser
+          .elementByCss('#to-loading-scroll')
+          .click()
+          .waitForElementByCss('#loading-component')
+        await check(() => browser.eval('window.scrollY'), 0)
+        await browser.waitForElementByCss('#content-that-is-visible')
+        await check(() => browser.eval('window.scrollY'), 0)
+      })
     })
   }
 )