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> components don't work well with browser history navigation #882

Closed
victorlin opened this issue May 29, 2024 · 5 comments · Fixed by #887
Closed

<Link> components don't work well with browser history navigation #882

victorlin opened this issue May 29, 2024 · 5 comments · Fixed by #887
Assignees
Labels
bug Something isn't working

Comments

@victorlin
Copy link
Member

victorlin commented May 29, 2024

Description

Clicking on a<Link> then using the browser to navigate to the previous page updates the URL but page contents remain unchanged.

How to reproduce

Steps to reproduce the current behavior:

  1. Go to https://next.nextstrain.org
  2. Click on "Groups" or "Please see the team page" in the footer
  3. Attempt to navigate back in browser history

Possible solutions

  1. Workaround: Use server-side rendering on internal links #886
  2. Remove scroll handler #887

References

Noted on Slack

@victorlin victorlin added the bug Something isn't working label May 29, 2024
@victorlin victorlin self-assigned this May 29, 2024
@victorlin
Copy link
Member Author

This was noted by @jameshadfield a while back on Slack.

I did some searching and it seems like this is a common problem with client-side navigation, but no clear solution.

I'm thinking to apply the workaround of replacing <Link> with <a> across static-site/* as a quick fix. The server-side navigation with <a> is a bit slower, but we don't really need the client-side navigation anywhere in particular.

@ivan-aksamentov
Copy link
Member

ivan-aksamentov commented May 30, 2024

@victorlin Here is a little experiment:

  • open dev console
  • open new tab, paste to URL bar https://next.nextstrain.org/ and press Enter. Do not scroll!
  • paste window.history.state, observe it's an object
  • scroll the page
  • paste window.history.state again, observe it's an empty string

I suspect it's this magic:

export const removeHash = () => {
history.replaceState(
"",
document.title,
window.location.pathname + window.location.search
)
}

and also maybe this:

window.history.replaceState({}, "", this.props.intendedUri);

When using a client-side router, any fiddling with history API will very likely break the router. I'd zap this magical solution and search for a package on NPM (if the usual anchor id="foo" and <a href="#foo" /> is not enough).

(It might make sense to also review all other overly smart solutions, e.g. involving window., global., manual DOM manipulation, complex on-mount logic etc. Chances are most of them exist as well-known NPM packages or built-in into Next.js)

@victorlin victorlin mentioned this issue May 31, 2024
3 tasks
@victorlin
Copy link
Member Author

Thanks for the pointer @ivan-aksamentov, I confirmed the first snippet is the culprit. Removing it in #887

While researching this issue, my understanding is that <Link> should just work and handle smooth scrolling to an anchor on the same page without the need for third-party react-scrollable-anchor.

  1. Docs:

    If you'd like to scroll to a specific id on navigation, you can append your URL with a # hash link or just pass a hash link to the href prop.

  2. vercel/next.js@8664ffe:

    This updates both pages & app router to restore smooth scroll functionality if the only the route hash changes.

I tried replacing react-scrollable-anchor with <Link> - it updates the URL but no scrolling (direct or smooth) happens. I'll have to test using another Next.js app to see if this is a bug in Next.js or some weird interaction with other parts of our codebase.

@ivan-aksamentov
Copy link
Member

ivan-aksamentov commented May 31, 2024

@victorlin
I think in order for things to work properly, the entire static-site/vendored/react-scrollable-anchor and all of its usages need to be removed. In your PR #887, you removed some of the event handlers and some of the window.history manipulation, but this class still registers "on hash change" event and its handler modifies window.history:

window.addEventListener('hashchange', this.handleHashChange)

The Next.js Link component is just a declarative convenience wrapper around router's imperative methods. I bet hash navigation will start working after this handler is no longer in business.

If you find that the package cannot be removed, then you could try to replace direct History API manipulation in it with equivalent Next.js router imperative methods - this will ensure that the router is also notified about the changes.


For a proper smooth scrolling in modern browsers, I believe that <div id="#foo" /> + <Link href="#foo"/> (or <a href="#foo"/>) + some global CSS:

html {
  scroll-behavior: smooth;
}

(moz, caniuse)

should do without any packages. Never heard of built-in smooth scrolling in Next.js (but there might be). I think vercel/next.js@8664ffe you quoted above is a fix for a bug in their router which prevented scroll-behavior: smooth from working properly when set. But you still need to set it.


If a more complex imperative smooth scroll is needed, then there are many hook libraries having this functionality (example). Underneath they are typically using something like this, if need to scroll to element's ref:

ref.current.scrollIntoView({ behavior: 'smooth' })

or, if need to scroll to coordinates:

window.scrollTo({
  top: 100,
  left: 100,
  behavior: "smooth",
});

(Can you implement your own smooth scroll based on these simple functions? Sure. Will it work? 80% of time, yes. For the remaining 20% you'd spent the next 5 years finding bugs, fixing them, then chasing random breakages in Safari, Edge and mobile browsers. If there's at least 1000 programmers actively using your package and reporting issues back, then you could be done in just 2 years :))


P.S. You likely know that very well already, but I'll mention it just in case. Note that there are differences between the older "pages" router and the newer "app" router. For example, above you linked docs from app router, but my understanding that current setup is using /pages directory and has _app.tsx, meaning that pages router is in works. So the appropriate link will be this and it does not seem to mention navigation to hash. I think it still works though.

The differences between "pages" and "app" setups, other than the obvious replacement of pages/ with app/: different implementations for some of the components, notably router and Link, and the imports are different sometimes.

@victorlin
Copy link
Member Author

victorlin commented May 31, 2024

@ivan-aksamentov

I think in order for things to work properly, the entire static-site/vendored/react-scrollable-anchor and all of its usages need to be removed.

You're probably right. I'll write up another issue to consider this: #888

Note that there are differences between the older "pages" router and the newer "app" router. For example, above you linked docs from app router, but my understanding that current setup is using /pages directory and has _app.tsx, meaning that pages router is in works.

@tsibley noted the difference in #824 (comment) but I forgot and didn't realize I was looking at the wrong docs. Thanks for noting!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants