Skip to content

Commit

Permalink
Fix scrolling on navigation in app (#48863)
Browse files Browse the repository at this point in the history
## What?

When investigating #48852 I found that when there's a `segmentPath` set
in the `focusAndScrollRef` it would compare that entire path with the
`segmentPath` of the layout-router. This logic works in cases where the
entire page is rendered but doesn't take into account cases where for
example a suspense boundary is triggered below the common layout.

Because of the order `useEffect` is run in we can already assume that if
a more specific match existed it would have scrolled there. As that is
not the case in e.g. #48852 it ended up opting out of scrolling
entirely. In order to fix that the logic needs to be different, it needs
to check if the layout-router's `segmentPath` matches, if it matches
every level then we can apply scrolling in that layout-router.

## How?

Implemented the opposite of the current logic, running `.every` on the
layout-router segmentPath instead of on the router provided segmentPath.

Fixes #48852
Fixes NEXT-1053

Related PR #48862

<!-- Thanks for opening a PR! Your contribution is much appreciated.
To make sure your PR is handled as smoothly as possible we request that
you follow the checklist sections below.
Choose the right checklist for the change(s) that you're making:

## For Contributors

### Improving Documentation or adding/fixing Examples

- The "examples guidelines" are followed from our contributing doc
https://github.com/vercel/next.js/blob/canary/contributing/examples/adding-examples.md
- Make sure the linting passes by running `pnpm build && pnpm lint`. See
https://github.com/vercel/next.js/blob/canary/contributing/repository/linting.md

### Fixing a bug

- Related issues linked using `fixes #number`
- Tests added. See:
https://github.com/vercel/next.js/blob/canary/contributing/core/testing.md#writing-tests-for-nextjs
- Errors have a helpful link attached, see
https://github.com/vercel/next.js/blob/canary/contributing.md

### Adding a feature

- Implements an existing feature request or RFC. Make sure the feature
request has been accepted for implementation before opening a PR. (A
discussion must be opened, see
https://github.com/vercel/next.js/discussions/new?category=ideas)
- Related issues/discussions are linked using `fixes #number`
- e2e tests added
(https://github.com/vercel/next.js/blob/canary/contributing/core/testing.md#writing-tests-for-nextjs
- Documentation added
- Telemetry added. In case of a feature if it's used or not.
- Errors have a helpful link attached, see
https://github.com/vercel/next.js/blob/canary/contributing.md



## For Maintainers

- Minimal description (aim for explaining to someone not on the team to
understand the PR)
- When linking to a Slack thread, you might want to share details of the
conclusion
- Link both the Linear (Fixes NEXT-xxx) and the GitHub issues
- Add review comments if necessary to explain to the reviewer the logic
behind a change

### What?

### Why?

### How?

Closes NEXT-
Fixes #

-->
  • Loading branch information
timneutkens committed Apr 26, 2023
1 parent 6902e5b commit da2804f
Show file tree
Hide file tree
Showing 2 changed files with 13 additions and 12 deletions.
12 changes: 5 additions & 7 deletions packages/next/src/client/components/layout-router.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -140,20 +140,18 @@ interface ScrollAndFocusHandlerProps {
class ScrollAndFocusHandler extends React.Component<ScrollAndFocusHandlerProps> {
handlePotentialScroll = () => {
// Handle scroll and focus, it's only applied once in the first useEffect that triggers that changed.
const { focusAndScrollRef } = this.props
const { focusAndScrollRef, segmentPath } = this.props

if (focusAndScrollRef.apply) {
// segmentPaths is an array of segment paths that should be scrolled to
// if the current segment path is not in the array, the scroll is not applied
// unless the array is empty, in which case the scroll is always applied
if (
focusAndScrollRef.segmentPaths.length !== 0 &&
!focusAndScrollRef.segmentPaths.some(
(segmentPath) =>
segmentPath.length === this.props.segmentPath.length &&
segmentPath.every((segment, index) =>
matchSegment(segment, this.props.segmentPath[index])
)
!focusAndScrollRef.segmentPaths.some((scrollRefSegmentPath) =>
segmentPath.every((segment, index) =>
matchSegment(segment, scrollRefSegmentPath[index])
)
)
) {
return
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -318,11 +318,14 @@ export function navigateReducer(
currentTree = newTree

for (const subSegment of generateSegmentsFromPatch(treePatch)) {
scrollableSegments.push(
[...flightSegmentPath, ...subSegment].filter(
(segment) => segment !== '__PAGE__'
)
)
const scrollableSegmentPath = [...flightSegmentPath, ...subSegment]
// Filter out the __DEFAULT__ paths as they shouldn't be scrolled to in this case.
if (
scrollableSegmentPath[scrollableSegmentPath.length - 1] !==
'__DEFAULT__'
) {
scrollableSegments.push(scrollableSegmentPath)
}
}
}
}
Expand Down

0 comments on commit da2804f

Please sign in to comment.