beforePopState returning false doesn't preserve URL

[solita-thaan]

Verify canary release

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

Provide environment information

Operating System:
  Platform: darwin
  Arch: arm64
  Version: Darwin Kernel Version 21.6.0: Mon Aug 22 20:19:52 PDT 2022; root:xnu-8020.140.49~2/RELEASE_ARM64_T6000
Binaries:
  Node: 16.17.0
  npm: 8.15.0
  Yarn: N/A
  pnpm: N/A
Relevant packages:
  next: 12.3.0
  eslint-config-next: N/A
  react: 17.0.2
  react-dom: 17.0.2

What browser are you using? (if relevant)

Firefox 105.0.3, Chrome 106.0.5249.119

How are you deploying your application? (if relevant)

next start, next dev

Describe the Bug

When router.beforePopState callback returns false, the navigation to the previous page is blocked as expected, but the URL in the browser address bar changes. Originally reported in https://github.com/vercel/next.js/issues/6784 but closed without action. The repro on that ticket seems to reproduce the issue as well.

Expected Behavior

The URL should not update if the PopState is prevented by the beforePopState callback.

Link to reproduction

https://github.com/solita-thaan/nextjs-bug-repro

To Reproduce

1. npm run build; npm run start;
2. Navigate to localhost:3000
3. Click the link "the bug" on the main page
4. Click the browser back button

The view remains on the "the bug" page as expected, but the page address now refers to the previous page.

[solita-thaan]

I've looked into this a bit more. Use case is rather straightforward: display a warning dialog when user is about to navigate away from an unsubmitted form using the browser back button and discard their changes. It seems there are hacks and workarounds for this problem (e.g. https://github.com/vercel/next.js/discussions/32231, throwing a mock error in the routeChangeStart event seems to be a common approach), but I can't find a proper way to implement this without hacks, and it seems common enough functionality that it should be supported. It seems to me that next router should support catching this event and aborting it if necessary, either through beforePopState or with router.events, but it seems at least the URL mutation always takes place?

[ChrisHamilton91]

Just want to express my interest in fixing this. This is a basic function of almost every site in existence, yet implementing it in Next.js is extremely complicated. Since we don't know whether the user is going backwards or forwards, and we don't know how many steps they moved, this would require us to keep track of our own history stack.

Not warning users that they're discarding changes looks really bad to anyone doing QA, and not having this built in makes me want to drop Next as a framework even though I like a lot of other aspects.

[ChrisHamilton91]

I've looked into this a bit more. Use case is rather straightforward: display a warning dialog when user is about to navigate away from an unsubmitted form using the browser back button and discard their changes. It seems there are hacks and workarounds for this problem (e.g. #32231, throwing a mock error in the routeChangeStart event seems to be a common approach), but I can't find a proper way to implement this without hacks, and it seems common enough functionality that it should be supported. It seems to me that next router should support catching this event and aborting it if necessary, either through beforePopState or with router.events, but it seems at least the URL mutation always takes place?

I don't believe that any of the hacks actually preserve the history stack properly, the component won't reload, but the browser will still move your history index.

[ChrisHamilton91]

Here's a simple reproduction:

https://next-41422-bug-repro.vercel.app

source: https://github.com/ChrisHamilton91/next-41422-bug-repro

pages/index.tsx

import Router from "next/router";
import { useEffect } from "react";

export default function Home() {
  useEffect(() => {
    Router.beforePopState(() => false);
  }, []);
  useEffect(() => {
    Router.push("one");
  }, []);
  return null;
}

pages/one.tsx

export default function OnePage() {
  return (
    <>
      <h1>Component One</h1>
      <p>Go forward a few times, then back</p>
      <p>Notice how the url changes when the component does not</p>
      <button onClick={() => history.back()}>BACK</button>
      <button onClick={() => Router.push("/two")}>FORWARD</button>
    </>
  );
}

pages/two.tsx

export default function TwoPage() {
  return (
    <>
      <h1>Component Two</h1>
      <p>Go forward a few times, then back</p>
      <p>Notice how the url changes when the component does not</p>
      <button onClick={() => history.back()}>BACK</button>
      <button onClick={() => Router.push("/one")}>FORWARD</button>
    </>
  );
}

Destroying the history stack defeats the purpose of returning false to beforePopState. Note that simply calling history.go(1) is not good enough, since the user can go back OR forward, and go an arbitrary number of steps at one time (via history menu or long press back / forward buttons).

[ChrisHamilton91]

Looking into it further, seems like it's impossible to stop the browser from moving the index of history along, best you can do is push the old state in on top of the new one.

const [currentState, setCurrentState] = useState({ url: "", as: "" });

  useEffect(() => {
    setCurrentState(history.state);
  }, []);

  useEffect(() => {
    Router.beforePopState((newState) => {
      history.pushState(newState, "", newState.url);
      Router.push(currentState.url, currentState.as);
      return false;
    });
  }, [currentState]);

Unfortunately it has the side effect of discarding any forward history, but I think it's the best that can be done.

I use that conditionally to show a prompt, it lets the user choose whether to save changes, discard and navigate away, or cancel navigation. I think it's a good trade off for messing with their history.