[NEXT-378] Next 13: Navigation with Link does not scroll up the page

alexvcasillas commented 2 years ago

Verify canary release

[X] I verified that the issue exists in the latest Next.js canary release

Provide environment information

Operating System: Platform: linux Arch: x64 Version: #1 SMP Mon Sep 19 19:14:52 UTC 2022 Binaries: Node: 16.15.0 npm: 8.5.5 Yarn: 3.2.4 pnpm: N/A Relevant packages: next: 13.0.2 eslint-config-next: 13.0.0 react: 18.2.0 react-dom: 18.2.0

What browser are you using? (if relevant)

Chrome 107.0.5304.87

How are you deploying your application? (if relevant)

Vercel

Describe the Bug

When navigating between pages using Link, the page does not scroll up by default wich is supposed to be the default behaviour of next/link

Expected Behavior

The page scrolls up when switching pages unless I manually disable this feature of next/link

Link to reproduction

https://beta.otpfy.com/

To Reproduce

Access https://beta.otpfy.com/
Scroll down until you see the "Get started for free" button
Click the given button
Check that when navigating to the auth page the scroll is preserved and not scrolled to the top

\:

_{From SyncLinear.com | NEXT-378}

semy commented 2 years ago

Same for me... How to fix that?

Fredkiss3 commented 2 years ago

I think it scrolls up to the nearest <div data-nextjs-scroll-focus-boundary />, this div is automatically created when you wrap your page in a layout. It is supposed to assure scroll restoration within layouts.

you can see in the video below :

https://user-images.githubusercontent.com/38298743/202875831-177954a5-cd9c-4afc-b758-684adc544b46.mov

semy commented 2 years ago

@ Fredkiss3 Thanks for your answer :) Yes you are right, but what if I don't want to? Normally I want it to just scroll all the way to the top when clicking on another page. That should be clarified.

Fredkiss3 commented 2 years ago

I don't know, they added this feature because it seemed really nice (that's what I know, but maybe I'm wrong), but it seems like it requires the creation of a new react API to do so. What I would have liked from them is that they don't include the scroll restore functionality before it is implemented by react.

(sorry for my broken english)

rshaul commented 1 year ago

I thought this scrolling behavior was a bug at first and rolled back to the pages directory because of it.

Scrolling to top after clicking a link seems like it should be the default behavior to match "web" expectations and maybe you should have to opt into this scroll to nearest layout container div behavior if that's what you want?

Or maybe rather than scrolling to the top of layouts inner content, it should scroll to the top of the layout itself to give context to the page navigation? So ie if you are just using a root layout it would always just scroll to the top of the entire pag. If your nested layout includes tabs it would scroll to show the tabs also.

rijk commented 1 year ago

For now can be fixed by adding a <ScrollUp /> component to your page:

'use client'

import { useEffect } from 'react'

export default function ScrollUp() {
  useEffect(() => window.document.scrollingElement?.scrollTo(0, 0), [])

  return null
}

trevorpfiz commented 1 year ago

I have some routes using the Tailwind UI Sidebar Layout where html and body need h-full. So this wasn't working. more details

So I had to use this on those routes:

'use client';

import { useEffect } from 'react';

export default function ScrollUpBody() {
  useEffect(() => document.body?.scrollTo(0, 0), []);

  return null;
}

Not fun.

Fredkiss3 commented 1 year ago

Hey, are you still having the same problem after the latest canary release ? You can install it with next@canary.

I've seen that they removed the unnecessary divs appearing inside children of layouts in this PR : #43717

trevorpfiz commented 1 year ago

I have been curious as well. There is a bug on canary that breaks my app, and maybe all Windows users. issue

rosszurowski commented 1 year ago

For others who come across this issue, I ran into the same thing, but was able to fix it by removing an unnecessary layout. My directory structure looked like:

app/layout.tsx
app/page.tsx
app/blog/[...slug]/layout.tsx
app/blog/[...slug]/page.tsx

In my case, navigating to one of the blog/2020/post-name pages had the page start half-way down, without scrolling back up to the top. Looking at the DOM structure, there were two layers of <div data-nextjs-scroll-focus-boundary /> elements.

I found that removing my app/blog/[...slug]/layout.tsx and wrapping my one-off layout into app/blog/[...slug]/page.tsx instead fixed the issue, since now there was only one layer of layouts rather than two. I moved common layout code into a separate component to share between the two pages where it's needed:

app/layout.tsx
app/page.tsx
app/blog/[...slug]/page.tsx
components/layout.tsx

For a content-based website like the one I was working on, it was a good reminder that nested layouts are probably more meant for apps with truly nested frames rather than a single view. I guess hearing the word "layout" made me think it'd replace my Layout components 1:1.

valse commented 1 year ago

Hey, are you still having the same problem after the latest canary release ? You can install it with next@canary.

I've seen that they removed the unnecessary divs appearing inside children of layouts in this PR : #43717

Hi, I'm using the latest 13.0.7-canary.6 version but I have the same issue

trevorpfiz commented 1 year ago

I'm using 13.0.7 and the issue is better, but it still does not scroll all the way up if I want it to. It will scroll right below the header.

davidcm62 commented 1 year ago

Same here. We're trying to add a simple layout with a shared header and the main content. On navigation, the page doesn't scroll to the top and the header is always hidden. We're using 13.0.7

AndrewIngram commented 1 year ago

For now can be fixed by adding a <ScrollUp /> component to your page:

'use client'

import { useEffect } from 'react'

export default function ScrollUp() {
  useEffect(() => window.document.scrollingElement?.scrollTo(0, 0), [])

  return null
}

Won't this also be triggered when using the back button though?

metawise commented 1 year ago

Confirming the scroll restoration not working after upgrade to NextJS 13.1 just today.

mikeyakymenko commented 1 year ago

Still doesn't work, nextJS 13.1.1.

stevenpetryk commented 1 year ago

I'm also on 13.1.1, and it doesn't seem like Next.js is adding any elements with data-nextjs-scroll-focus-boundary.

I actually have a relatively simple website that reproduces this behavior if it helps.

Repo: https://github.com/stevenpetryk/mafs (the /docs folder)
Live site example page

Running document.querySelectorAll('[data-nextjs-scroll-focus-boundary']) on a few pages returns empty NodeLists. I also tried things like removing display: flex from body to no avail.

semy commented 1 year ago

The next problem is when you press the browser's back button: Normally the previous HTML page should be displayed in the same scroll position as before. But the page hangs and blocks for a moment, and then the document jumps back to the top of the page. This is very inconvenient when I'm scrolling through a list of, for example, blog posts and then go to the details of a post and then want to go back to look at the next blog post. I find this behavior very annoying.

semy commented 1 year ago

+1

valse commented 1 year ago

I'm also on 13.1.1, and it doesn't seem like Next.js is adding any elements with data-nextjs-scroll-focus-boundary.

I actually have a relatively simple website that reproduces this behavior if it helps.

Repo: https://github.com/stevenpetryk/mafs (the /docs folder)

Live site example page

Running document.querySelectorAll('[data-nextjs-scroll-focus-boundary']) on a few pages returns empty NodeLists. I also tried things like removing display: flex from body to no avail.

Hi the div data-nextjs-scroll-focus-boundary is not longer available... This issue is very annoying and I can't understand why is not on the top priority list. Maybe it happens only for certain configuration or styles?!

qpavy commented 1 year ago

Still doesn't work on NextJs 13.1.2-canary.0 It seems a little better on NextJs 13.0.5

jessicalc commented 1 year ago

I'm on NextJS 12.2.5 and was also observing this problem on pages with shared layouts, where, when I clicked on a Link on Page A to get to Page B, Page B would load scrolled to the position that Page A was at. (i.e. if I was 50% of the way scrolled down on Page A, Page B would load scrolled 50% of the way down!)

Things I tried that didn't work:

Not using layouts at all (no dice)

Things that I did to make it work:

I removed scroll-behavior: smooth from my html element (and stuck it on the body). h/t https://stackoverflow.com/a/63379899/4172535
~When using the next/link Link, I passed scroll={true}~

~That combination seems to have fixed things for my situation.~ Update: I think scroll={true} doesn't do anything and the real issue is the scroll behavior smooth on the HTML element.

Also wonder to what degree this issue is related: https://github.com/vercel/next.js/issues/40719

qpavy commented 1 year ago

With scroll={true} on next/link Link it seems to work better. But sometimes, scrolling stop under my header I don't know why.

steve-marmalade commented 1 year ago

What's the intuition behind scroll={true} helping, given that the docs indicate that's the default?

qpavy commented 1 year ago

yeah I don't know why it's working better

jessicalc commented 1 year ago

What's the intuition behind scroll={true} helping, given that the docs indicate that's the default?

That's a good question, @steve-marmalade. I'm going to update my answer to strike-through that bit, because it doesn't do anything.

trevorpfiz commented 1 year ago

I just wanted to point out that I do not see a scroll property on the beta docs. It is not in props or legacy props. 🤷

pogran commented 1 year ago

i think problem isn't solved yet. Right?

devRabbani commented 1 year ago

Same problem

devRabbani commented 1 year ago

I found the error....Actually it is happening because I set height:100% and overflow: auto on html element.

reiv commented 1 year ago

For what it's worth, this is reproducible on the app playground, which has neither of those.

saarnav890 commented 1 year ago

I found the error....Actually it is happening because I set height:100% and overflow: auto on html element.

I don't have either in my project, but am still facing the issue :(

nx-dariomunoz commented 1 year ago

Still and issue in NextJS 13.1.2, it seems to not scroll at all after navigating using next/link component. Specifically in Server Side components.

timneutkens commented 1 year ago

why is not on the top priority list.

Scroll behavior and back navigation are on the list of things to investigate further. E.g. there's this PR: #43845. As of right now it's expected that navigation scrolls to the nearest common layout that changed, this ensures that when you're navigating between tabs it keeps the relevant changed content in view and doesn't start from the top of the page. #43845 changes that behavior to scroll to the top of the page if the element stays in the viewport while at the top of the page. This behavior is relevant because in the new router you can soon create e.g. a tab UI similar to what you can see on https://app-dir.vercel.app/ssg where you wouldn't want it to scroll to top on interactions.

Please be patient while we work through the backlog of all app related issues, it's in beta for a reason and to reiterate it's not meant to be used in production yet.

Please don't post comments like "same issue", "It's still an issue in version x.x.x", "is this resolved yet" they're not constructive.

nx-dariomunoz commented 1 year ago

While it's getting under further investigation, for the ones who are still facing this unexpected behaviour (although there're some reasons behind this as Tim stated), this workaround is functional and working:

For now can be fixed by adding a <ScrollUp /> component to your page:

'use client'

import { useEffect } from 'react'

export default function ScrollUp() {
  useEffect(() => window.document.scrollingElement?.scrollTo(0, 0), [])

  return null
}

Just wanted to remark that you can import this "Scrollup" component into your desired "page.jsx" in the route you expect to navigate and be auto-scrolled to the top of the page instead of remaining at the actual scroll spot.

Thanks Tim for the insights about this topic.

AndrewIngram commented 1 year ago

The problem with that workaround is that it'll also be triggered if you use the back button, which is generally not the desired behaviour -- the "native" behaviour is to to restore the previous scroll position.

It's a viable interim solution if you don't mind that though.

sookmax commented 1 year ago

The problem with that workaround is that it'll also be triggered if you use the back button, which is generally not the desired behaviour -- the "native" behaviour is to to restore the previous scroll position.

This is a good point. I also wanted to preserve the default behavior of back/forward navigations from the browser UI (or mouse forward/back, swiping gesture at the screen edge, etc) while bringing the scroll to the top for the navigations triggered by my <Link />.

So I came up with something that works for me, but not really sure if the solution is a right way to approach this or if it's against best practices ¯_(ツ)_/¯.

Anyways, here's the basic idea (in case it's helpful to someone).

First of all, I need to differentiate the page navigation caused by my <Link />s and that caused by all the other methods. I mark this in onClick of my <Link />s.
In some appropriate client component (layout component in my case), subscribe to the change in the current path using usePathname and useEffect.
in useEffect above, check if the path change was triggered by the <Link />.
if it is, do window.scrollTo({ top: 0 }) otherwise do nothing.

Here's some code.

// ./components/MyLayout.tsx
"use client";
import { usePathname } from "next/navigation";
import { useEffect } from "react";

let top0Requested = false;
export function requestTopO() {
  top0Requested = true;
}

export default function MyLayout(props) {
  const currentPath = usePathname();

  useEffect(() => {
    let requestId: number | undefined;
    if (top0Requested) {
      const rAFCallback = () => {
        if (window.scrollY !== 0) {
          window.scrollTo({ top: 0 });
          requestId = requestAnimationFrame(rAFCallback);
        } else {
          top0Requested = false;
        }
      };
      requestId = requestAnimationFrame(rAFCallback);
    }

    return () => {
      requestId && cancelAnimationFrame(requestId);
    };
  }, [currentPath]);

  return <YourUIComponent {...props} />
}

// ./components/MyNav.tsx
import { requestTopO } from "./MyLayout";

export default function MyNav() {
  return (
    <ul>
      <li>
        <Link href="/about" onClick={requestTopO}>
          About
        </Link>
        <Link href="/work" onClick={requestTopO}>
          Work
        </Link>
        <Link href="/contact" onClick={requestTopO}>
          Contact
        </Link>
      </li>
    </ul>
  );
}

lnikell commented 1 year ago

I experience similar behavior, but there is something worth mentioning the behavior is different from a page visit through other pages, vs a direct visit with further navigation within the page. In both scenarios behavior is unexpected.

Visit through the homepage and then navigating between links https://user-images.githubusercontent.com/2697570/213864595-3daf1685-7883-473a-a813-de4661e07f4f.mp4

Page refresh and navigation between links https://user-images.githubusercontent.com/2697570/213864614-70788b56-0bd9-450d-bb96-401be0b78091.mp4

valse commented 1 year ago

it's in beta for a reason and to reiterate it's not meant to be used in production yet

@timneutkens you're right and I'm sorry: I thought only to solve one of my needs and not for the benefit of the product: thanks for your useful explanation and I'm sure we'll find the best solution to cover all the desired use cases 💪🏻

Sincerely, a Next.js ♥️ lover

timneutkens commented 1 year ago

We've landed #43845 which changes the heuristic to scroll to top when the element is in the viewport when scroll position is at the top. If it's not in the viewport when at the top of the page it'll scroll to the changed element.

simonecervini commented 1 year ago

I am trying with version 13.1.6, but personally I am still experiencing problems using a basic layout with fixed navbar to navigate between pages that share the same layout. I don't understand if with https://github.com/vercel/next.js/pull/43845 merged this case should already be covered or it is still a work in progress.

jankaifer commented 1 year ago

This shouldn't happen. I tried to reproduce it, but I wasn't able to. Could you try reproducing the issue here https://github.com/JanKaifer/next-repro-42492 It has a navbar with position: fixed, but it still works as expected.

There could be an issue if the navbar is taller than the viewport, could that be your case? (In that case, it should scroll to the top of the page, just below the navbar).

simonecervini commented 1 year ago

This shouldn't happen. I tried to reproduce it, but I wasn't able to. Could you try reproducing the issue here https://github.com/JanKaifer/next-repro-42492 It has a navbar with position: fixed, but it still works as expected.

There could be an issue if the navbar is taller than the viewport, could that be your case? (In that case, it should scroll to the top of the page, just below the navbar).

Thank you for your reply! After an hour of trying, I think the problem is related to CSS Modules. You can try this diff, the content of styles.module.css is not relevant.

diff --git a/app/[num]/page.tsx b/app/[num]/page.tsx
index 83206f4..a939440 100644
--- a/app/[num]/page.tsx
+++ b/app/[num]/page.tsx
@@ -1,17 +1,13 @@
 import Link from "next/link";
+import styles from './styles.module.css'

 export default function Page({ params: { num } }) {
   return (
     <div>
       {new Array(100).fill(0).map((_, i) => (
         <div
+          className={styles.square}
           key={i}
-          style={{
-            height: 100,
-            width: 100,
-            background: "pink",
-            margin: 10,
-          }}
         >
           <Link href={`/${Number(num) - 1}`}>lower</Link>
           <div>{num}</div>

jankaifer commented 1 year ago

Thank you, I added that to the repro, and I'm able to reproduce this now. Will look into it.

jankaifer commented 1 year ago

I have added a test with a repro here:

[branch was deleted]

It's a bit tricky because the playwright click() won't reproduce this. We need to do document.getElementById("...").click()

simonhrogers commented 1 year ago

+1

This affects the back button with both the pages and app directories for me and breaks a fundamental part of the browsing experience.

valse commented 1 year ago

+1

This affects the back button with both the pages and app directories for me and breaks a fundamental part of the browsing experience.

Hi, using pages you can add:

experimental: {
   scrollRestoration: true,
}

in the experimental section of your next.config.js file to support the back button restoration.

chrber04 commented 1 year ago

For me, the scroll does work but it doesn't scroll all the way up. Seems like it's scrolling to the top, minus the height of the sticky navbar. When I remove the header, it scrolls just as expected. Anyone know a solution to this?

edit: it also works perfectly fine when just reloading the page, with the header/navbar still there.

jankaifer commented 1 year ago

@chrber04 This sounds like a bug. Could you open a new issue for this, please?

gjuoun commented 1 year ago

I have the same issue during the development.

This is clear a bug, please keep it open

vercel / next.js