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

The URL does not reflect the current page after refreshing on a form and then cancelling the navigation warning. #37

Open
ghost opened this issue Sep 19, 2018 · 8 comments

Comments

@ghost
Copy link

ghost commented Sep 19, 2018

Do you want to request a feature or report a bug?

Bug

What is the current behavior?

The URL does not reflect the current page after refreshing on a form and then cancelling the navigation warning.

If the current behavior is a bug, please provide the steps to reproduce.

  1. Navigate to a form with the "NavigationPrompt" component
  2. Press the browser refresh button.
  3. Type something into the form to meet the "when" condition on unmount.
  4. Press the browser back button, and press "Cancel" on the prompt. Note how the URL has changed.
  5. After doing any other browser navigation or refreshing at this point, it will load the wrong page. This means that the URL is not reflecting the current page correctly because the URL is now out of sync.

Tested on 3 browsers (Firefox/IE11/Chrome).

What is the expected behavior?

After refreshing, the URL should not change when being prompted before navigating away. Instead, it should only change the URL in the address bar when the onConfirm is called and the page needs to redirect.

Please post the output of npm list react react-router react-router-dom react-router-navigation-prompt (with redactions if needed).

├── react-router@4.3.1
├─┬ react-router-dom@4.3.1
│ └── react-router@4.3.1 deduped
└── react-router-navigation-prompt@1.6.5

@ZacharyRSmith
Copy link
Owner

ZacharyRSmith commented Sep 20, 2018

thanks for the thoroughly written issue, @shepswitcher . unfortunately i am busy and cannot look into it right now. maybe this weekend?

@ghost
Copy link
Author

ghost commented Sep 20, 2018

No worries! It's my first issue filed on GitHub actually

That's not a problem. This weekend will be good thank you

@ZacharyRSmith
Copy link
Owner

ZacharyRSmith commented Sep 24, 2018

@shepswitcher as you may have noticed, i did not get around to it last weekend, and i doubt i will this week

@ghost
Copy link
Author

ghost commented Sep 24, 2018

Yep that is totally fine mate! No rush here, whenever you're free is cool

@ZacharyRSmith
Copy link
Owner

ZacharyRSmith commented Sep 29, 2018

react-router's history.block() is not being invoked, so fixing this would require a solution similar to what NavigationPrompt is doing with window.addEventListener('beforeunload', this.onBeforeUnload);

I looked into using window.addEventListener('popstate'), but the event was never triggered (Chrome v69). Any ideas on how to get a handle on the event?

@ghost
Copy link
Author

ghost commented Sep 30, 2018

Thanks for looking into this!
Alright so at which point is history.block() not being invoked? Any time after you refresh?
Will popstate work because you would need one or more entries in the history stack when you click the browser back button and maybe refreshing the page confuses it

@ZacharyRSmith
Copy link
Owner

My bad, I had a filter that caused me to not see the logs history.block() was making.

This is quite an edge case! It seems that react-router is ignoring the request to block navigation because we are navigating to a context not handled by this "session" of react-router (I haven't looked at their code, but this seems to be the case). Ie, it seems that react-router is treating this identically to navigating to a separate site. Meanwhile, the browser sees this as navigating to the same site, so 'beforeunload' is not getting triggered.

There might be a solution with window.addEventListener('popstate'). I don't think I'll get around to looking into it for at least a week. If you can help, I'd appreciate it!

@ghost
Copy link
Author

ghost commented Oct 1, 2018

All good. Makes sense now!
Yep I'll see if I can look into it myself at some stage

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant