vercel / next.js

The React Framework
https://nextjs.org
MIT License
126.86k stars 26.97k forks source link

[NEXT-1130] app router: Scroll position not reset when dynamic segment changes #49427

Open sophiebits opened 1 year ago

sophiebits commented 1 year ago

Verify canary release

Provide environment information

codesandbox on next@13.4.1-canary.2

Which area(s) of Next.js are affected? (leave empty if unsure)

Routing (next/router, next/navigation, next/link)

Link to the code that reproduces this issue

https://codesandbox.io/p/sandbox/hopeful-taussig-uvq5g1

To Reproduce

Click around to different pages, which are all handled by the same /app/[path]/page.tsx route.

Describe the Bug

Note that scroll position is preserved when navigating.

Expected Behavior

You wouldn't expect it to be.

NEXT-1130

timneutkens commented 1 year ago

Hey! I had a look at this, the reason is that the fixed element is selected by findDOMNode and that one stays in view. Here's the Replay with logs where I checked it: https://app.replay.io/recording/untitled--53500c98-e7b4-4ed8-9532-3ad118c409cd.

One way to solve it is with div before the fixed element, I've applied it on a forked sandbox: https://codesandbox.io/p/sandbox/festive-bhabha-5e8lsn?file=%2Fapp%2F%5Bpath%5D%2Fpage.tsx%3A7%2C7

I'm not sure what to do with this particular case because in theory it's doing the right thing based on the available element.

sophiebits commented 1 year ago

Oh interesting, thanks for taking a look. I just checked my real app and although I didn't do this on purpose, the cause is the same (position: sticky nav bar).

I know you're already skipping over hidden elements with the elementCanScroll() check. I wonder if absolute/fixed elements should also be skipped, so that you scroll to the first static or relative one. For sticky it seems like maybe you would want to scroll to where the element would be if it weren't floating because if you scroll the next sibling then it would be obscured by the sticky header. (I also notice we're not including scrollMarginTop in the topOfElementInViewport calculation but maybe we should be?)

sophiebits commented 1 year ago

For sticky elements, the CSS spec says:

For example, if a user clicks a link targeting a sticky-positioned element, if the element’s nearest scrollport is currently scrolled such that the sticky positioned element is offset from its initial position, the scroll container will be scrolled back only the minimum necessary to bring it into its desired position in the scrollport (rather than scrolling all the way back to target its original, non-offsetted position). https://www.w3.org/TR/css-position-3/#example-d3878f82

which would seem to argue against what I proposed, though conceptually if we think of emulating what would happen if we did have a <div> wrapper then I think we do want the original un-sticky position. Maybe if it's sticky we need to temporarily set it to relative to measure it? đŸ¥´

liho00 commented 1 year ago

For sticky elements, the CSS spec says:

For example, if a user clicks a link targeting a sticky-positioned element, if the element’s nearest scrollport is currently scrolled such that the sticky positioned element is offset from its initial position, the scroll container will be scrolled back only the minimum necessary to bring it into its desired position in the scrollport (rather than scrolling all the way back to target its original, non-offsetted position). https://www.w3.org/TR/css-position-3/#example-d3878f82

which would seem to argue against what I proposed, though conceptually if we think of emulating what would happen if we did have a <div> wrapper then I think we do want the original un-sticky position. Maybe if it's sticky we need to temporarily set it to relative to measure it? đŸ¥´

weird issue tho, once i remove sticky css, the scroll position is working normally to top, if i put back sticky on my header nav, when navigate to other link the scroll position is not reset to top.

donnersvensson commented 1 year ago

Hey! I had a look at this, the reason is that the fixed element is selected by findDOMNode and that one stays in view. Here's the Replay with logs where I checked it: https://app.replay.io/recording/untitled--53500c98-e7b4-4ed8-9532-3ad118c409cd.

One way to solve it is with div before the fixed element, I've applied it on a forked sandbox: https://codesandbox.io/p/sandbox/festive-bhabha-5e8lsn?file=%2Fapp%2F%5Bpath%5D%2Fpage.tsx%3A7%2C7

I'm not sure what to do with this particular case because in theory it's doing the right thing based on the available element.

For anybody who might come across this issue in the future. Even though the sandbox example does not seem to work anymore: I had the same issue with a "hero opener" component, that used position: sticky (i suppose the same happens with "position: fixed"). it prevented the default scroll: {true} behavior of the "Link" component. Simply putting a <div> before the components sticky div, solved it.

Before (scroll did not work):

Opener.js (Project is using TailwindCSS)

export default function Opener(){
<div className="sticky">
{children}
</div>
}

After (scroll works):

Opener.js (Project is using TailwindCSS)

export default function Opener(){
<>
<div></div>
<div className="sticky">
{children}
</div>
</>
}
highree commented 1 year ago

works great.

ryami333 commented 8 months ago

For the record - an empty div above our fixed/sticky Navbar does not work. In fact, even a non-empty div doesn't fix it.

guylepage3 commented 7 months ago

Huge issue with PWA's at the moment. Scroll position / page position is critical for a decent UX let alone if you're looking to create a good UX.

prof-awais commented 6 months ago

Found a better solution and exact problem too: https://ziszini.tistory.com/142

For the programmatic route use router.push or replace("path", {scroll: false})

tarushchandra commented 5 months ago

Found a better solution and exact problem too: https://ziszini.tistory.com/142

For the programmatic route use router.push or replace("path", {scroll: false})

Please clarify on this solution, since nextJs also mentions in their docs, that scrolling to top of the page is the default behavior (using scroll={true}) and scroll={false} ensures that page will not scroll to the top of the page. Check this - https://nextjs.org/docs/pages/api-reference/components/link

For me, empty div solution works, and the page is not automatically scrolled and changed to a new route. for the reference - I've have a sticky header which always keeps on scrolling when I change my route.

If there's a better solution than having a empty div, please suggest me, for now I'm going with the empty div approach

joseph-bayer commented 3 months ago

This was resolved for me after updating to next version 14.2.5. I didn't see it mentioned in the update notes, so I'm not sure why that fixed it đŸ˜…

svarup commented 3 months ago

This issue still exists on v14.2.5 when we have a "sticky" position header on top of the page and the below code works for me to temporary fix.

const pathname = usePathname();
useEffect(() => {
    window.scroll(0, 0);
}, [pathname]);