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

Bug in Resetting the Form DOM on Browser Back/Forth #6128

Open
fabb opened this issue Jan 25, 2019 · 9 comments
Open

Bug in Resetting the Form DOM on Browser Back/Forth #6128

fabb opened this issue Jan 25, 2019 · 9 comments
Labels
good first issue Easy to fix issues, good for newcomers

Comments

@fabb
Copy link
Contributor

fabb commented Jan 25, 2019

Bug report

Describe the bug

In Chrome, when page navigating back/forth to a next.js page that contains a form with previously selected radio and select values, these selected values are still shown, but do not match the react component state. This leads to business logic errors, since the fields are expected to be empty or set to some other initial values.

In a create-react-app this works fine, so I guess it has something to do with how next.js handles the popstate event.

To Reproduce

I have created a full example repo here: https://github.com/fabb/select-bug

  1. Create a new next app using npx create-next-app select-next-test
  2. Replace the index page with the following code:
class App extends React.Component {
  constructor(props) {
    super(props);
    this.state = {
      myradio: undefined,
      myselect: "one"
    };
  }

  onChangeRadio = event => {
    this.setState({ myradio: event.target.value });
  };

  onChangeSelect = event => {
    this.setState({ myselect: event.target.value });
  };

  render() {
    return (
      <div>
        <input
          type="radio"
          value="one"
          name="myradio"
          required
          onChange={this.onChangeRadio}
          checked={this.state.myradio === "one"}
        />
        <input
          type="radio"
          value="two"
          name="myradio"
          required
          checked={this.state.myradio === "two"}
          onChange={this.onChangeRadio}
        />
        <select
          name="myselect"
          required
          value={this.state.myselect}
          onChange={this.onChangeSelect}
        >
          <option value="">Please choose...</option>
          <option value="one">One</option>
          <option value="two">Two</option>
        </select>
      </div>
    );
  }
}
  1. npm run dev
  2. In the browser, select the second radio button, and in the select set "Two"
  3. In the same browser tab, go to another url, e.g. https://www.npmjs.com/package/next
  4. Press the browser back button

Actual Buggy Behavior

  • The second radio button is still selected, and in the select "Two" is still selected BUT in the react component's state the state does not match that
  • Now when the first radio button is clicked, the component rerenders, and also the select suddenly jumps to "One" (which is the initial state set in the constructor)

Expected behavior

  • The radio buttons should both be deselected, and for the select "One" should be selected
  • Therefore, the component state should match what is displayed

Caveat

  • Ignore the browser's DOM inspector, it does not correctly update checked and value attributes, even though they are correctly set when doing console.log.

Screenshots

next_bug

System information

  • OS: macOS Mojave
  • Browser: Chrome 71
  • Version of Next.js: 7.0.2 and 8.0.0-canary-13
@timneutkens timneutkens added help wanted good first issue Easy to fix issues, good for newcomers labels Jan 25, 2019
@doniyor2109
Copy link

It is interesting that when using pure SSR without Next.js, it gives same result. Here is demo: https://github.com/doniyor2109/select-bug

@fabb
Copy link
Contributor Author

fabb commented Jan 28, 2019

It‘s probably related to the Back-Forward/Page-Cache in Chrome. The problem in a react app is that the internal state does not match what is shown on screen. I guess next.js would need to rerender the current state after the page gets navigated to, maybe using the pageshow event.

@doniyor2109
Copy link

@fabb Yes. It seems Chrome implements page caching. However it works on Safari as expected.

@giuseppeg
Copy link
Contributor

pasting a private conversation I had with Tim:

[me] not sure how Next handles popstate events but do you replace the last entry in the state with null before leaving the site? This way you can ignore the popstate event when the user navigates back to the app.

[me] yeah that must be the cause of the bug. Chrome/WebKit fire a popstate event for the initial navigation. In fact the test case works on Firefox

[Tim] Giuseppe yeah we put in a replaceState on initial page load: https://github.com/zeit/next.js/blob/f4f3649de33830d6758172a8d9df8b9a3e7142e9/packages/next-server/lib/router/router.js#L39

[me] I think that you should defer that to the first time users navigate (via pushState). Then register an beforeunload event handler to do a replaceState to null so that when they come back and the state is null and you can noop

@marbiano
Copy link
Contributor

marbiano commented Jun 2, 2020

I took this issue thinking that it could be a simple one, oh boy I was wrong.

After a bazillion hours testing stuff on different setups, I came to the conclusion that browsers need to grow up and do similar stuff when it comes to bfcache. Right now I'm getting like at least 10 different inconsistencies between what Chrome, Edge, Safari and Firefox do. Sometimes, they are not even respectful of their own documentation. Here's some of the highlights of my research:

  • popstate on Chrome doesn't get called on page load since v34. Same with Edge.
  • there's no way to "clear page's cache" before you leave. Docs around insist on using unload or beforeunload events, but that literally doesn't work on any of the browsers. I spent a lot of time in here, since it seemed to be the logic solution. There's an alternative using headers sent from the server, but I didn't explore it because it seemed a bit too convoluted for such an edge case.
  • the recommended way to take care of this is to use the pageshow event, and look for the persisted event attribute. It sounds lovely, but Chrome and Edge always set it to false, no matter what you do. Safari and Firefox manage it correctly.
  • there's an escape hatch, which is to use window.performance.navigation.type in order to see if the last navigation action was a back/forward one. So that's how you do it with the old Chromium engine.
  • once I figured how to clean up, I thought the same as @fabb, which is just to add a reset action every time the current page is "dirty". But then I realized that in the context of a Next.js site, this could mean a lot of different things. You could have a page with a contact form only where it makes a ton of sense, but you could also have a server-side generated page with no forms at all, where triggering a page refresh would be the wrong thing to do.

So I came up with the PR that you can see above, haven't added tests nor docs yet but I hope it's a step in the right direction!

I fully recommend folks jumping into random issues, I ended up learning a ton of stuff related to how the History API works!

@karlhorky
Copy link
Contributor

I created a reproduction of the bug in Next.js and added it to the Chrome bug that @stefanpenner opened (mentioned in the pull request):

https://bugs.chromium.org/p/chromium/issues/detail?id=1143298#c7

@jakeorr
Copy link

jakeorr commented Oct 15, 2021

I ran into this today. The workaround of adding a key to the component seemed to fix the issue for me: <Component key={router.asPath}>.

#9992

@Enchiridion
Copy link

I don't use Next.js, just React and had the same issue. After trying many solutions I just set the field name to a random string. On each page load, even back/forward the name is different, so the browser doesn't try to restore it. This is one infuriating bug. I never noticed it before as I use Firefox, had a coworker point it out.

@camunoz2
Copy link

I run into this as well using just react + vite with this functional approach instead:

function App() {
  const [myradio, setMyRadio] = useState(undefined);
  const [myselect, setMySelect] = useState("one");

  const onChangeRadio = event => {
    setMyRadio(event.target.value);
  };

  const onChangeSelect = event => {
    setMySelect(event.target.value);
  };

  return (
    <div>
      <input
        type="radio"
        value="one"
        name="myradio"
        required
        onChange={onChangeRadio}
        checked={myradio === "one"}
      />
      <input
        type="radio"
        value="two"
        name="myradio"
        required
        checked={myradio === "two"}
        onChange={onChangeRadio}
      />
      <select
        name="myselect"
        required
        value={myselect}
        onChange={onChangeSelect}
      >
        <option value="">Please choose...</option>
        <option value="one">One</option>
        <option value="two">Two</option>
      </select>
    </div>
  );
}

But, i dont think this is a bug, because it can be solved with

  useEffect(() => {
    // Store the current state in the browser history when the component unmounts
    const state = { myradio, myselect };
    window.history.pushState(state, null);
    return () => {
      // Remove the state from the browser history when the component mounts again
      window.history.replaceState({}, null);
    };
  }, [myradio, myselect]);

  useEffect(() => {
    // Restore the state from the browser history when the component mounts
    const state = window.history.state || {};
    setMyRadio(state.myradio || undefined);
    setMySelect(state.myselect || "one");
  }, []);

But i dont know if is the developer responsability to manage state persistance using back and forward buttons, or of this should be react defaults behaviour, but anyways i think this issue should be closed because is not nextjs specific.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Easy to fix issues, good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.