From 905b62e59917109867e4e3d88f51a3eca663ed71 Mon Sep 17 00:00:00 2001 From: Tim Neutkens Date: Sat, 29 Apr 2023 19:43:55 +0200 Subject: [PATCH] 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. --- .../src/client/components/layout-router.tsx | 91 +++++++++++-------- .../app/loading-scroll/loading.tsx | 13 +++ .../app/loading-scroll/page.tsx | 17 ++++ .../app-dir/router-autoscroll/app/page.tsx | 13 ++- .../router-autoscroll.test.ts | 12 +++ 5 files changed, 106 insertions(+), 40 deletions(-) create mode 100644 test/e2e/app-dir/router-autoscroll/app/loading-scroll/loading.tsx create mode 100644 test/e2e/app-dir/router-autoscroll/app/loading-scroll/page.tsx diff --git a/packages/next/src/client/components/layout-router.tsx b/packages/next/src/client/components/layout-router.tsx index 6c2a1acf25c94..5df8f31e704f7 100644 --- 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 { +class InnerScrollAndFocusHandler extends React.Component { 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 } } +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 ( + + {children} + + ) +} + /** * 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({ ) // Ensure root layout is not wrapped in a div as the root layout renders `` - return ( - - {subtree} - - ) + return subtree } /** @@ -551,34 +566,36 @@ export default function OuterLayoutRouter({ - - + + - - - - - - + + + + + + + + } > <> 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 new file mode 100644 index 0000000000000..071ae5928f5b7 --- /dev/null +++ b/test/e2e/app-dir/router-autoscroll/app/loading-scroll/loading.tsx @@ -0,0 +1,13 @@ +export default function Loading() { + return ( + <> +
Loading component
+ { + // Repeat 500 elements + Array.from({ length: 500 }, (_, i) => ( +
Loading {i}...
+ )) + } + + ) +} 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 new file mode 100644 index 0000000000000..e23f68654be67 --- /dev/null +++ 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 ( + <> +
Content that is hidden.
+
Content which is not hidden.
+ { + // Repeat 500 elements + Array.from({ length: 500 }, (_, i) => ( +
{i}
+ )) + } + + ) +} diff --git a/test/e2e/app-dir/router-autoscroll/app/page.tsx b/test/e2e/app-dir/router-autoscroll/app/page.tsx index 3892d8e0f9f2a..b89bd39ca68cf 100644 --- 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() {
{i}
)) } - - To invisible first element - +
+ + To loading scroll + +
+
+ + To invisible first element + +
) } diff --git a/test/e2e/app-dir/router-autoscroll/router-autoscroll.test.ts b/test/e2e/app-dir/router-autoscroll/router-autoscroll.test.ts index eabf8e5b0fd41..b81a5984cb7de 100644 --- 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) + }) }) } )