From b92e6fa069971499603d407ae9e3455c759c3844 Mon Sep 17 00:00:00 2001 From: Tim Neutkens Date: Tue, 4 Jul 2023 10:25:25 +0200 Subject: [PATCH] Support scroll: false for Link component for app router (#51869) ### What Support `scroll={false}` for Link component in app router. This can be used when you don't need to scroll back to top again when route url changes. For instance hash query changes, if you want to keep the scrolling as it is, you can use this option. ### How Handling the `scroll` option in navigation reducer on client side. Fixes #50105 Fixes NEXT-1377 --------- Co-authored-by: Jiachi Liu --- .../next/src/client/components/app-router.tsx | 21 ++- .../router-reducer/handle-mutable.ts | 29 ++- .../reducers/navigate-reducer.test.tsx | 174 ++++++++++++++++++ .../reducers/navigate-reducer.ts | 5 +- .../reducers/server-patch-reducer.test.tsx | 1 + .../router-reducer/router-reducer-types.ts | 5 +- packages/next/src/client/link.tsx | 4 +- .../next/src/shared/lib/app-router-context.ts | 1 + test/e2e/app-dir/import/next.config.js | 4 +- .../navigation/app/hash-changes/page.js | 75 ++++++++ .../e2e/app-dir/navigation/navigation.test.ts | 11 ++ .../app-dir/router-autoscroll/next.config.js | 4 +- 12 files changed, 308 insertions(+), 26 deletions(-) create mode 100644 test/e2e/app-dir/navigation/app/hash-changes/page.js diff --git a/packages/next/src/client/components/app-router.tsx b/packages/next/src/client/components/app-router.tsx index da8e27b8b7e0d..9f6e87fa5943a 100644 --- a/packages/next/src/client/components/app-router.tsx +++ b/packages/next/src/client/components/app-router.tsx @@ -231,11 +231,7 @@ function Router({ ) const navigate: RouterNavigate = useCallback( - ( - href: string, - navigateType: 'push' | 'replace', - forceOptimisticNavigation: boolean - ) => { + (href, navigateType, forceOptimisticNavigation, shouldScroll) => { const url = new URL(addBasePath(href), location.href) return dispatch({ @@ -244,6 +240,7 @@ function Router({ isExternalUrl: isExternalURL(url), locationSearch: location.search, forceOptimisticNavigation, + shouldScroll: shouldScroll ?? true, navigateType, cache: createEmptyCacheNode(), mutable: {}, @@ -296,12 +293,22 @@ function Router({ }, replace: (href, options = {}) => { startTransition(() => { - navigate(href, 'replace', Boolean(options.forceOptimisticNavigation)) + navigate( + href, + 'replace', + Boolean(options.forceOptimisticNavigation), + options.scroll ?? true + ) }) }, push: (href, options = {}) => { startTransition(() => { - navigate(href, 'push', Boolean(options.forceOptimisticNavigation)) + navigate( + href, + 'push', + Boolean(options.forceOptimisticNavigation), + options.scroll ?? true + ) }) }, refresh: () => { diff --git a/packages/next/src/client/components/router-reducer/handle-mutable.ts b/packages/next/src/client/components/router-reducer/handle-mutable.ts index 91b49c72b4e92..a1444072f1661 100644 --- a/packages/next/src/client/components/router-reducer/handle-mutable.ts +++ b/packages/next/src/client/components/router-reducer/handle-mutable.ts @@ -9,6 +9,9 @@ export function handleMutable( state: ReadonlyReducerState, mutable: Mutable ): ReducerState { + // shouldScroll is true by default, can override to false. + const shouldScroll = mutable.shouldScroll ?? true + return { buildId: state.buildId, // Set href. @@ -30,19 +33,25 @@ export function handleMutable( }, // All navigation requires scroll and focus management to trigger. focusAndScrollRef: { - apply: - mutable?.scrollableSegments !== undefined + apply: shouldScroll + ? mutable?.scrollableSegments !== undefined ? true - : state.focusAndScrollRef.apply, - hashFragment: - // Empty hash should trigger default behavior of scrolling layout into view. - // #top is handled in layout-router. - mutable.hashFragment && mutable.hashFragment !== '' + : state.focusAndScrollRef.apply + : // If shouldScroll is false then we should not apply scroll and focus management. + false, + hashFragment: shouldScroll + ? // Empty hash should trigger default behavior of scrolling layout into view. + // #top is handled in layout-router. + mutable.hashFragment && mutable.hashFragment !== '' ? // Remove leading # and decode hash to make non-latin hashes work. decodeURIComponent(mutable.hashFragment.slice(1)) - : state.focusAndScrollRef.hashFragment, - segmentPaths: - mutable?.scrollableSegments ?? state.focusAndScrollRef.segmentPaths, + : state.focusAndScrollRef.hashFragment + : // If shouldScroll is false then we should not apply scroll and focus management. + null, + segmentPaths: shouldScroll + ? mutable?.scrollableSegments ?? state.focusAndScrollRef.segmentPaths + : // If shouldScroll is false then we should not apply scroll and focus management. + [], }, // Apply cache. cache: mutable.cache ? mutable.cache : state.cache, diff --git a/packages/next/src/client/components/router-reducer/reducers/navigate-reducer.test.tsx b/packages/next/src/client/components/router-reducer/reducers/navigate-reducer.test.tsx index 75a4d5abdc96a..a5f9580c327c0 100644 --- a/packages/next/src/client/components/router-reducer/reducers/navigate-reducer.test.tsx +++ b/packages/next/src/client/components/router-reducer/reducers/navigate-reducer.test.tsx @@ -3,6 +3,7 @@ import type { fetchServerResponse as fetchServerResponseType } from '../fetch-se import type { FlightData } from '../../../../server/app-render/types' const buildId = 'development' + const flightData: FlightData = [ [ 'children', @@ -58,6 +59,9 @@ jest.mock('../fetch-server-response', () => { fetchServerResponse: ( url: URL ): ReturnType => { + if (url.pathname === '/linking' && url.hash === '#hash') { + return Promise.resolve(['', undefined]) + } if (url.pathname === '/linking/about') { return Promise.resolve([flightData, undefined]) } @@ -174,6 +178,7 @@ describe('navigateReducer', () => { isExternalUrl: false, locationSearch: '', navigateType: 'push', + shouldScroll: true, forceOptimisticNavigation: false, cache: { status: CacheStates.LAZY_INITIALIZED, @@ -362,6 +367,7 @@ describe('navigateReducer', () => { isExternalUrl: false, locationSearch: '', navigateType: 'push', + shouldScroll: true, forceOptimisticNavigation: false, cache: { status: CacheStates.LAZY_INITIALIZED, @@ -555,6 +561,7 @@ describe('navigateReducer', () => { isExternalUrl, locationSearch: '', navigateType: 'push', + shouldScroll: true, forceOptimisticNavigation: false, cache: { status: CacheStates.LAZY_INITIALIZED, @@ -717,6 +724,7 @@ describe('navigateReducer', () => { isExternalUrl, locationSearch: '', navigateType: 'replace', + shouldScroll: true, forceOptimisticNavigation: false, cache: { status: CacheStates.LAZY_INITIALIZED, @@ -876,6 +884,7 @@ describe('navigateReducer', () => { isExternalUrl: false, locationSearch: '', navigateType: 'push', + shouldScroll: true, forceOptimisticNavigation: true, cache: { status: CacheStates.LAZY_INITIALIZED, @@ -977,6 +986,169 @@ describe('navigateReducer', () => { expect(newState).toMatchObject(expectedState) }) + it('should apply navigation for scroll', async () => { + const initialTree = getInitialRouterStateTree() + const initialCanonicalUrl = '/linking' + const children = ( + + + Root layout + + ) + const initialParallelRoutes: CacheNode['parallelRoutes'] = new Map([ + [ + 'children', + new Map([ + [ + 'linking', + { + status: CacheStates.READY, + parallelRoutes: new Map([ + [ + 'children', + new Map([ + [ + '__PAGE__', + { + status: CacheStates.READY, + data: null, + subTreeData: <>Linking page, + parallelRoutes: new Map(), + }, + ], + ]), + ], + ]), + data: null, + subTreeData: <>Linking layout level, + }, + ], + ]), + ], + ]) + + const state = createInitialRouterState({ + buildId, + initialTree, + initialHead: null, + initialCanonicalUrl, + children, + initialParallelRoutes, + isServer: false, + location: new URL('/linking', 'https://localhost') as any, + }) + + const state2 = createInitialRouterState({ + buildId, + initialTree, + initialHead: null, + initialCanonicalUrl, + children, + initialParallelRoutes, + isServer: false, + location: new URL('/linking#hash', 'https://localhost') as any, + }) + + const action: NavigateAction = { + type: ACTION_NAVIGATE, + url: new URL('/linking#hash', 'https://localhost'), + isExternalUrl: false, + locationSearch: '', + navigateType: 'push', + shouldScroll: false, // should not scroll + forceOptimisticNavigation: false, + cache: { + status: CacheStates.LAZY_INITIALIZED, + data: null, + subTreeData: null, + parallelRoutes: new Map(), + }, + mutable: {}, + } + + await runPromiseThrowChain(() => navigateReducer(state, action)) + + const newState = await runPromiseThrowChain(() => + navigateReducer(state2, action) + ) + + const expectedState: ReturnType = { + buildId, + prefetchCache: new Map(), + pushRef: { + mpaNavigation: true, + pendingPush: true, + }, + focusAndScrollRef: { + apply: false, + hashFragment: null, + segmentPaths: [], + }, + canonicalUrl: '', + nextUrl: '/linking', + cache: { + status: CacheStates.READY, + data: null, + subTreeData: ( + + + Root layout + + ), + parallelRoutes: new Map([ + [ + 'children', + new Map([ + [ + 'linking', + { + status: CacheStates.READY, + parallelRoutes: new Map([ + [ + 'children', + new Map([ + [ + '__PAGE__', + { + status: CacheStates.READY, + // Real promise is not needed here. + data: null, + parallelRoutes: new Map(), + subTreeData: <>Linking page, + }, + ], + ]), + ], + ]), + // Real promise is not needed here. + data: null, + subTreeData: <>Linking layout level, + }, + ], + ]), + ], + ]), + }, + tree: [ + '', + { + children: [ + 'linking', + { + children: ['__PAGE__', {}], + }, + ], + }, + // TODO-APP: optimistic tree is wrong + undefined, + undefined, + true, + ], + } + + expect(newState).toMatchObject(expectedState) + }) + it('should apply navigation with prefetched data', async () => { const initialTree = getInitialRouterStateTree() const initialCanonicalUrl = '/linking' @@ -1060,6 +1232,7 @@ describe('navigateReducer', () => { isExternalUrl: false, navigateType: 'push', locationSearch: '', + shouldScroll: true, forceOptimisticNavigation: false, cache: { status: CacheStates.LAZY_INITIALIZED, @@ -1342,6 +1515,7 @@ describe('navigateReducer', () => { isExternalUrl: false, locationSearch: '', navigateType: 'push', + shouldScroll: true, forceOptimisticNavigation: false, cache: { status: CacheStates.LAZY_INITIALIZED, diff --git a/packages/next/src/client/components/router-reducer/reducers/navigate-reducer.ts b/packages/next/src/client/components/router-reducer/reducers/navigate-reducer.ts index 1ef59e2cb1cf2..f22bfc72401aa 100644 --- a/packages/next/src/client/components/router-reducer/reducers/navigate-reducer.ts +++ b/packages/next/src/client/components/router-reducer/reducers/navigate-reducer.ts @@ -114,6 +114,7 @@ export function navigateReducer( cache, mutable, forceOptimisticNavigation, + shouldScroll, } = action const { pathname, hash } = url const href = createHrefFromUrl(url) @@ -191,6 +192,7 @@ export function navigateReducer( mutable.patchedTree = optimisticTree mutable.pendingPush = pendingPush mutable.hashFragment = hash + mutable.shouldScroll = shouldScroll mutable.scrollableSegments = [] mutable.cache = temporaryCacheNode mutable.canonicalUrl = href @@ -355,12 +357,13 @@ export function navigateReducer( mutable.previousTree = state.tree mutable.patchedTree = currentTree - mutable.scrollableSegments = scrollableSegments mutable.canonicalUrl = canonicalUrlOverride ? createHrefFromUrl(canonicalUrlOverride) : href mutable.pendingPush = pendingPush + mutable.scrollableSegments = scrollableSegments mutable.hashFragment = hash + mutable.shouldScroll = shouldScroll return handleMutable(state, mutable) } diff --git a/packages/next/src/client/components/router-reducer/reducers/server-patch-reducer.test.tsx b/packages/next/src/client/components/router-reducer/reducers/server-patch-reducer.test.tsx index 6ffcd0548496c..913b1dd8afcc7 100644 --- a/packages/next/src/client/components/router-reducer/reducers/server-patch-reducer.test.tsx +++ b/packages/next/src/client/components/router-reducer/reducers/server-patch-reducer.test.tsx @@ -502,6 +502,7 @@ describe('serverPatchReducer', () => { isExternalUrl: false, locationSearch: '', navigateType: 'push', + shouldScroll: true, forceOptimisticNavigation: false, cache: { status: CacheStates.LAZY_INITIALIZED, diff --git a/packages/next/src/client/components/router-reducer/router-reducer-types.ts b/packages/next/src/client/components/router-reducer/router-reducer-types.ts index 9a066acb573e6..c7dfe69472ee7 100644 --- a/packages/next/src/client/components/router-reducer/router-reducer-types.ts +++ b/packages/next/src/client/components/router-reducer/router-reducer-types.ts @@ -23,7 +23,8 @@ export type RouterChangeByServerResponse = ( export type RouterNavigate = ( href: string, navigateType: 'push' | 'replace', - forceOptimisticNavigation: boolean + forceOptimisticNavigation: boolean, + shouldScroll: boolean ) => void export interface Mutable { @@ -36,6 +37,7 @@ export interface Mutable { cache?: CacheNode prefetchCache?: AppRouterState['prefetchCache'] hashFragment?: string + shouldScroll?: boolean } export interface ServerActionMutable { @@ -125,6 +127,7 @@ export interface NavigateAction { locationSearch: Location['search'] navigateType: 'push' | 'replace' forceOptimisticNavigation: boolean + shouldScroll: boolean cache: CacheNode mutable: Mutable } diff --git a/packages/next/src/client/link.tsx b/packages/next/src/client/link.tsx index d17341b43edd2..39fd9ecbd0719 100644 --- a/packages/next/src/client/link.tsx +++ b/packages/next/src/client/link.tsx @@ -217,15 +217,17 @@ function linkClicked( const navigate = () => { // If the router is an NextRouter instance it will have `beforePopState` + const routerScroll = scroll ?? true if ('beforePopState' in router) { router[replace ? 'replace' : 'push'](href, as, { shallow, locale, - scroll, + scroll: routerScroll, }) } else { router[replace ? 'replace' : 'push'](as || href, { forceOptimisticNavigation: !prefetchEnabled, + scroll: routerScroll, }) } } diff --git a/packages/next/src/shared/lib/app-router-context.ts b/packages/next/src/shared/lib/app-router-context.ts index b0ca3c444afb6..6d47491eb8b9f 100644 --- a/packages/next/src/shared/lib/app-router-context.ts +++ b/packages/next/src/shared/lib/app-router-context.ts @@ -70,6 +70,7 @@ export type CacheNode = export interface NavigateOptions { /** @internal */ forceOptimisticNavigation?: boolean + scroll?: boolean } export interface PrefetchOptions { diff --git a/test/e2e/app-dir/import/next.config.js b/test/e2e/app-dir/import/next.config.js index 53c8bdda514e2..4ba52ba2c8df6 100644 --- a/test/e2e/app-dir/import/next.config.js +++ b/test/e2e/app-dir/import/next.config.js @@ -1,3 +1 @@ -module.exports = { - strictMode: true, -} +module.exports = {} diff --git a/test/e2e/app-dir/navigation/app/hash-changes/page.js b/test/e2e/app-dir/navigation/app/hash-changes/page.js new file mode 100644 index 0000000000000..39ec0bde5cd5f --- /dev/null +++ b/test/e2e/app-dir/navigation/app/hash-changes/page.js @@ -0,0 +1,75 @@ +'use client' + +import React from 'react' +import Link from 'next/link' +import { useRouter } from 'next/navigation' + +const HashChanges = () => { + const router = useRouter() + + return ( +
+
+ + Via Link + +
+ +
+ + Via Empty Hash + +
+ +
+ + Go to item 400 + +
+ +
+ + Go to name item 400 + +
+ +
+ + Go to name item 400 (no scroll) + +
+ +
+ + Go to CJK anchor + +
+ +
+ + Via Top Hash + +
+
ASPATH: {router.asPath}
+
PATHNAME: {router.pathname}
+
CJK anchor
+ {Array.from({ length: 500 }, (x, i) => i + 1).map((i) => { + return ( +
+ {i} +
+ ) + })} +
+ ) +} + +export default HashChanges diff --git a/test/e2e/app-dir/navigation/navigation.test.ts b/test/e2e/app-dir/navigation/navigation.test.ts index 8aaf93dd0bf30..a6e6fab2bcac7 100644 --- a/test/e2e/app-dir/navigation/navigation.test.ts +++ b/test/e2e/app-dir/navigation/navigation.test.ts @@ -85,6 +85,17 @@ createNextDescribe( await checkLink('top', 0) await checkLink('non-existent', 0) }) + + it('should not scroll to hash when scroll={false} is set', async () => { + const browser = await next.browser('/hash-changes') + const curScroll = await browser.eval( + 'document.documentElement.scrollTop' + ) + await browser.elementByCss('#scroll-to-name-item-400-no-scroll').click() + expect(curScroll).toBe( + await browser.eval('document.documentElement.scrollTop') + ) + }) }) describe('hash-with-scroll-offset', () => { diff --git a/test/e2e/app-dir/router-autoscroll/next.config.js b/test/e2e/app-dir/router-autoscroll/next.config.js index 53c8bdda514e2..4ba52ba2c8df6 100644 --- a/test/e2e/app-dir/router-autoscroll/next.config.js +++ b/test/e2e/app-dir/router-autoscroll/next.config.js @@ -1,3 +1 @@ -module.exports = { - strictMode: true, -} +module.exports = {}