Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

<Link> not scrolling top sticky header #64441

Open
LouisGS44 opened this issue Apr 13, 2024 · 9 comments
Open

<Link> not scrolling top sticky header #64441

LouisGS44 opened this issue Apr 13, 2024 · 9 comments
Labels
bug Issue was opened via the bug report template. linear: next Confirmed issue that is tracked by the Next.js team.

Comments

@LouisGS44
Copy link

LouisGS44 commented Apr 13, 2024

Link to the code that reproduces this issue

https://codesandbox.io/p/devbox/header-no-scroll-nextjs-x84kd3?embed=1&file=%2Fapp%2Flayout.tsx

To Reproduce

  1. Start the application in development (next dev)

Current vs. Expected behavior

A sticky header is present at the top of the layout (it has an opacity of 0.8 for viewing the page below).
If the scroll height is lower than the header height and I click on the link to another page, the scroll doesn't return to the top of the page.

On the other hand, if the scroll height is greater than the header height, then the scroll does return to the top of the page as expected.

Enregistrement.de.l.ecran.2024-04-12.a.23.31.18.mov

Provide environment information

Operating System:
  Platform: darwin
  Arch: x64
  Version: Darwin Kernel Version 23.1.0: Mon Oct  9 21:27:27 PDT 2023; root:xnu-10002.41.9~6/RELEASE_X86_64
  Available memory (MB): 16384
  Available CPU cores: 12
Binaries:
  Node: 20.9.0
  npm: 10.2.5
  Yarn: N/A
  pnpm: N/A
Relevant Packages:
  next: 14.2.0 // There is a newer version (14.2.1) available, upgrade recommended! 
  eslint-config-next: N/A
  react: 18.2.0
  react-dom: 18.2.0
  typescript: 5.4.5
Next.js Config:
  output: N/A

Which area(s) are affected? (Select all that apply)

App Router

Which stage(s) are affected? (Select all that apply)

next dev (local), next build (local), next start (local)

Additional context

No response

NEXT-3099

@LouisGS44 LouisGS44 added the bug Issue was opened via the bug report template. label Apr 13, 2024
@Javad9s
Copy link

Javad9s commented Apr 13, 2024

Some scuffed workaround for me is to use {position: "fixed", width: "100%"} in layout.tsx instead of {position: "sticky"} and manually add a paddingTop to each individual page component. In your case:

export default function Home() {
  return (
    <div style={{ paddingTop: 150 }}>
      <div style={{ height: 1200, backgroundColor: "brown" }}>Page 1</div>
      <div style={{ height: 700, backgroundColor: "purple" }}></div>
    </div>
  );
}

This way the top of each {children} is actually aligned with the top of DOM and router will scroll to top on navigation.

@LouisGS44
Copy link
Author

LouisGS44 commented Apr 13, 2024

Yes thanks I'm using this solution in my production application but I have nested layouts that I can't use.
Some of my pages have the same heading so I have to create a component and import it at the top of every pages.
It works but it doesn't use the power of nested layouts

@AzizTareq295

This comment was marked as off-topic.

@timneutkens timneutkens added kind: bug linear: next Confirmed issue that is tracked by the Next.js team. labels Apr 13, 2024
@timneutkens
Copy link
Member

The element check for if it should scroll (specifically topOfElementInViewport

function topOfElementInViewport(element: HTMLElement, viewportHeight: number) {
) returns true even though it's not fully in the viewport because of the position: sticky element. We'll have to figure out some special handling for that case 🤔

@philwolstenholme
Copy link
Contributor

philwolstenholme commented Apr 20, 2024

I've noticed some funniness with this sort of thing too, it's good to see an issue for it.

@timneutkens if you or your team look into this further then perhaps one option could be to take into account the value of the CSS property scroll-padding-top on the scroll parent (e.g. html in many cases) when working out if an element is visible or not in topOfElementInViewport.

It's a modern CSS way of preventing a sticky header from overlapping content that you scroll to via anchor links, scrollIntoView() etc, so quite a few users with a sticky header will have it set to the height of their sticky header.

Although, some people also use scroll-margin-top on ::target so it wouldn't be a universal fix.

@samcx samcx removed the kind: bug label Apr 30, 2024
@haraldev01
Copy link

What an annoying issue. I read in the documentation that it's supposed to scroll to the top of the page (i.e. barely scroll to cover the top of whatever the page.tsx outputs), but this is clearly not what people always want. I want to put a simple padding in my layout so that the page fits neatly in the site, but when I do that it no longer scrolls to the top. I have to wrap the padding or page in a custom component that I have to import into every page, which defeats the purpose of having the layout in the first place. I also tried using a template, but since it goes off of pages, this didn't work either.

I should be able to override the default behavior and have more control over where the scroll boundaries are. I am unfortunately not experienced enough to contribute myself, but I would look into that.

@aiqbalsyah
Copy link

aiqbalsyah commented Jul 2, 2024

hello everyone may this help you guys
const pathname = usePathname(); useEffect(() => { window.scrollTo(0, 0); }, [pathname]);

@artola
Copy link

artola commented Jul 9, 2024

The element check for if it should scroll (specifically topOfElementInViewport

function topOfElementInViewport(element: HTMLElement, viewportHeight: number) {

) returns true even though it's not fully in the viewport because of the position: sticky element. We'll have to figure out some special handling for that case 🤔

It happens even on: Next.js Documentation. It is related to the common pattern of having a sticky navigation and the check done in topOfElementInViewport because it does not match the UX expectation to scroll to the top.

With such sticky navigation, it is usual to set scroll-padding-top: var(--header-height); and could be a sign that it is intended to be used in the calculation.

const scrollPaddingTop = Number(window.getComputedStyle(document.documentElement).scrollPaddingTop) || 0;

@philwolstenholme
Copy link
Contributor

Some sites use scroll-margin-top on ::target, so checking for scroll-padding-top on html or body wouldn't be a universal fix.

Maybe Next could check for the value of a specific CSS custom property and it could be up to the individual Next.js site to keep that custom property updated and correct?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue was opened via the bug report template. linear: next Confirmed issue that is tracked by the Next.js team.
Projects
None yet
Development

No branches or pull requests

9 participants