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

replaceState on scroll after visiting external site #53347

Closed
1 task done
altano opened this issue Jul 30, 2023 · 9 comments · Fixed by #54081
Closed
1 task done

replaceState on scroll after visiting external site #53347

altano opened this issue Jul 30, 2023 · 9 comments · Fixed by #54081
Labels
bug Issue was opened via the bug report template. linear: next Confirmed issue that is tracked by the Next.js team. locked Navigation Related to Next.js linking (e.g., <Link>) and navigation.

Comments

@altano
Copy link

altano commented Jul 30, 2023

Verify canary release

  • I verified that the issue exists in the latest Next.js canary release

Provide environment information

npx --no-install next info

    Operating System:
      Platform: darwin
      Arch: arm64
      Version: Darwin Kernel Version 22.5.0: Thu Jun  8 22:22:23 PDT 2023; root:xnu-8796.121.3~7/RELEASE_ARM64_T6020
    Binaries:
      Node: 20.3.1
      npm: 9.6.7
      Yarn: N/A
      pnpm: 8.6.5
    Relevant Packages:
      next: 13.4.13-canary.6
      eslint-config-next: 13.4.6
      react: 18.2.0
      react-dom: 18.2.0
      typescript: 5.1.3
    Next.js Config:
      output: N/A

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

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

Link to the code that reproduces this issue or a replay of the bug

https://bright.codehike.org

To Reproduce

I haven't been able to create a minimal reproduction on Stackblitz yet, but I'm observing this issue on multiple public Next.js sites so I thought I'd report it anyway.

Example 1 – bright

  1. Go to https://bright.codehike.org
  2. Click the "the Next.js docs" link
  3. Click Back button
  4. Scroll around on the page. If nothing happens, mouse over the "recipes" link.
  5. Either the scrolling around OR mousing over the recipes link will trigger a navigation back to the "the Next.js docs"
  6. Clicking "Back" will NOT take you back to bright.codehike.org, as it appears the navigation was a replaceState and your history is now busted.

Video Demo:

bright.-.replacestate.mp4

Example 2 – my personal site

  1. Go to https://alan.norbauer.com/articles/bookcision
  2. Click the "View this article's source code" link which links to github
  3. Click Back button
  4. Scroll around on the page.
  5. Scrolling around will trigger a navigation back to github
  6. Clicking "Back" will NOT take you back to my site, as it appears the navigation was a replaceState and your history is now busted.

Video Demo:

alan.-.replacestate.mp4

Describe the Bug

Here's what I can gather:

  1. Navigating to an external site is key to triggering the issue.
  2. I can reproduce the issue on desktop Chrome and mobile Safari
  3. These are pretty basic Next.js sites. Bright's source is here and my site's source is here.
  4. I CANNOT reproduce this with the devtools open, making debugging extremely difficult.
  5. This reproduces in both dev mode and production deploys.
  6. This is my unsuccessful attempt to reproduce in stackblitz
  7. In mobile Safari I'm more likely to get stuck in a loop where scrolling isn't even required to trigger the erroneous navigation. When I hit back it immediately shoves me forward. I can keep hitting back and my personal site will flash before I'm navigated back to github, stuck there:
    https://github.com/vercel/next.js/assets/1009/7f7227eb-ea92-4c84-ae4a-74422a599a6f

Expected Behavior

I would not expect scrolling to trigger a history replaceState or any navigation at all.

Which browser are you using? (if relevant)

Chrome 115.0.5790.114 (arm64)

How are you deploying your application? (if relevant)

No response

NEXT-1511

@altano altano added the bug Issue was opened via the bug report template. label Jul 30, 2023
@github-actions github-actions bot added the Navigation Related to Next.js linking (e.g., <Link>) and navigation. label Jul 30, 2023
@steveluscher
Copy link
Contributor

I was able to get a stack trace by:

  1. Loading the site on my phone
  2. Performing the repro up until step 3
  3. Connecting a debugger to mobile Chrome using chrome://inspect from the desktop
  4. window.addEventListener('beforeunload', e => { debugger; })
  5. Resuming the repro steps from step 4

I'll get you an unminified stack trace in a bit.

@steveluscher
Copy link
Contributor

It's location.assign(canonicalUrl) that causes the errant navigation, here:

// When mpaNavigation flag is set do a hard navigation to the new url.
// Infinitely suspend because we don't actually want to rerender any child
// components with the new URL and any entangled state updates shouldn't
// commit either (eg: useTransition isPending should stay true until the page
// unloads).
//
// This is a side effect in render. Don't try this at home, kids. It's
// probably safe because we know this is a singleton component and it's never
// in <Offscreen>. At least I hope so. (It will run twice in dev strict mode,
// but that's... fine?)
if (pushRef.mpaNavigation) {
const location = window.location
if (pushRef.pendingPush) {
location.assign(canonicalUrl)
} else {
location.replace(canonicalUrl)
}
// TODO-APP: Should we listen to navigateerror here to catch failed
// navigations somehow? And should we call window.stop() if a SPA navigation
// should interrupt an MPA one?
use(createInfinitePromise())
}

👋🏻 @sophiebits. RIP #49058.

@steveluscher
Copy link
Contributor

Question is, who else from the old crew can we drag into this PR? 🤣

@ReeceHumphreys
Copy link

Experiencing the same issue. Tried replicating it with the next.js starter repo but was unable to.

@michel-kraemer
Copy link
Contributor

I was able to reproduce it as follows:

  1. Run npx create-next-app@latest to create a new project.
  2. Use the default values for everything:
✔ What is your project named? … my-app
✔ Would you like to use TypeScript? … Yes
✔ Would you like to use ESLint? … Yes
✔ Would you like to use Tailwind CSS? … Yes
✔ Would you like to use `src/` directory? … No
✔ Would you like to use App Router? (recommended) … Yes
✔ Would you like to customize the default import alias? … No
  1. Open next.config.js and add output: "export":
/** @type {import('next').NextConfig} */
const nextConfig = {
  output: "export",
}

module.exports = nextConfig
  1. Replace the contents of app/page.tsx with the following code:
import Link from 'next/link'

export default function Home() {
  return (
    <main className="flex min-h-screen flex-col items-center justify-between p-24 m-96">
      <Link href="/">self</Link><br />
      <Link href="https://github.com">GitHub</Link>
    </main>
  )
}
  1. Run npm run build to create a static build
  2. Run a web server (e.g. npx serve -l 3000 out)
  3. Open http://localhost:3000 in your web browser
  4. Scroll down and click on the "GitHub" link, then go back, scroll up and hover your mouse cursor over the "self" link (without clicking it!). The page should change to "GitHub" again and the back button will not work correctly any longer as described by @altano.

It seems the "self" link is the culprit. I haven't tested if it also happens if you link to another internal page, but as soon as I remove the self link, everything works OK.

I hope this helps!

michel-kraemer added a commit to steep-wms/steep-wms.github.io that referenced this issue Aug 13, 2023
@michel-kraemer
Copy link
Contributor

I'm surprised not more people are affected by this issue. All I had to do to reproduce it was to create a new project, enable static export, and add an external link. This should basically affect all apps using the app router and external links.

Nevertheless, I created a workaround for the website I'm currently working on. I didn't want to find all external links in my app and replace them with a normal <a> tag, so I created a small wrapper around "next/link" instead. It just renders <a> for external links and <Link> for internal ones.

Click on the following (internal) link if you're interested (pun intended 😉):
https://github.com/steep-wms/steep-wms.github.io/blob/ce98ee8c0bba282ac79d26d7e78c9343cc7ca9b2/src/components/LinkFix.tsx

@honzasladek
Copy link

Just found out we're affected as well. Exactly as @michel-kraemer says above - I think almost everyone should be affected if they're using <Link> for external links.

@timneutkens timneutkens added the linear: next Confirmed issue that is tracked by the Next.js team. label Aug 14, 2023
@steveluscher
Copy link
Contributor

Just to put a fine point on it, this has nothing to do with Link and everything to do with the AppRouter PR I linked above. It needs to either be reverted or fixed-forward.

@kodiakhq kodiakhq bot closed this as completed in #54081 Aug 16, 2023
kodiakhq bot pushed a commit that referenced this issue Aug 16, 2023
When an mpa navigation takes place, we currently push the user to the new route and suspend the page indefinitely (x-ref: #49058). When navigating back, if the browser opts into using the [bfcache](https://web.dev/bfcache/), it will remain suspended and `pushRef.mpaNavigation` will be true. This means that anything that would cause the component to re-render will trigger the mpa navigation again (such as hovering over another `Link`, as reported in #53347)

This PR checks to see if bfcache is being used by observing `PageTransitionEvent.persisted` and if so, resets the router state to clear out `pushRef`. 

Closes NEXT-1511
Fixes #53347
altano added a commit to altano/alan.norbauer.com that referenced this issue Aug 16, 2023
Fixes navigation bug with external sites (vercel/next.js#53347)
altano added a commit to altano/alan.norbauer.com that referenced this issue Aug 16, 2023
Fixes navigation bug with external sites (vercel/next.js#53347)
altano added a commit to altano/alan.norbauer.com that referenced this issue Aug 20, 2023
Fixes navigation bug with external sites (vercel/next.js#53347)
@github-actions
Copy link
Contributor

This closed issue has been automatically locked because it had no new activity for 2 weeks. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 31, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Issue was opened via the bug report template. linear: next Confirmed issue that is tracked by the Next.js team. locked Navigation Related to Next.js linking (e.g., <Link>) and navigation.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants