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

victorlin commented 1 month ago

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:

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

Possible solutions

Workaround: #886
887

References

Noted on Slack

victorlin commented 4 weeks ago

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 commented 4 weeks ago

@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:

https://github.com/nextstrain/nextstrain.org/blob/64470d388c33a04ae8b83942bfaf828e4edfe885/static-site/vendored/react-scrollable-anchor/utils/hash.js#L14-L20

and also maybe this:

https://github.com/nextstrain/nextstrain.org/blob/64470d388c33a04ae8b83942bfaf828e4edfe885/static-site/src/components/Datasets/dataset-select.jsx#L52

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 commented 4 weeks ago

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.

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.
https://github.com/vercel/next.js/commit/8664ffe26dafd7529d5fc674294c0f53709d51c3:

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 commented 4 weeks ago

@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 https://github.com/nextstrain/nextstrain.org/pull/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:

https://github.com/nextstrain/nextstrain.org/blob/f5fce6950895ef33054fef99a6a8262b40e006a6/static-site/vendored/react-scrollable-anchor/Manager.js#L25

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 https://github.com/vercel/next.js/commit/8664ffe26dafd7529d5fc674294c0f53709d51c3 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 commented 4 weeks ago

@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 https://github.com/nextstrain/nextstrain.org/pull/824#issuecomment-2105384576 but I forgot and didn't realize I was looking at the wrong docs. Thanks for noting!

nextstrain / nextstrain.org