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

Navigating between explore and report page with back button does not work #2403

Open
acmiyaguchi opened this issue Oct 30, 2019 · 10 comments · Fixed by #2529
Open

Navigating between explore and report page with back button does not work #2403

acmiyaguchi opened this issue Oct 30, 2019 · 10 comments · Fixed by #2529
Assignees

Comments

@acmiyaguchi
Copy link

A common thing I do is to write some code in the "explore" mode, and check on the "report" mode occasionally. My first reaction to go from the report mode back into explore is to hit the back button, which navigates back to the previous page (not the explore view).

What I Did

  1. Start at https://alpha.iodide.io/ (the homepage)
  2. Click on "A Brief Tour through Iodide" (or any other notebook)
  3. Click on "REPORT" in the top right
  4. Click browser back button

What I Expected

I expect the page to navigate back to "EXPLORE", which is the last immediate view. An alternative is to provide the same dialog when "backspace" is pressed.

What Happened

I navigate back to the homepage without warning dialog, losing any computed state. This is particularly felt when running python-heavy notebooks.

@acmiyaguchi
Copy link
Author

This is somewhat related to #1725, but for the browser back button.

@wlach
Copy link
Contributor

wlach commented Nov 1, 2019

This should not be very hard to fix. We need to push the state (instead of replacing it) when switching modes:

window.history.replaceState(

And also watch document history for this mode to change, probably somewhere around here:

https://github.com/iodide-project/iodide/blob/master/src/editor/index.jsx

@bcolloran
Copy link
Contributor

hmm, in addition to pushing onto the history stack in the case of report/explore transitions, I wonder if we should also just always alert when navigating away from an NB that has had code evaluations to keep people from losing computed state? or we could use a more sophisticated heuristic:

  • if you arrive at the page in Report view and never enter explore view, don't alert on nav away (since no novel state can be lost)
  • if you make any edits and then eval and then attempt to navigate away, alert since novel state may be lost.

@Algentile
Copy link
Contributor

Hey @wlach, @bcolloran,

I can look into this issue, as @acmiyaguchi mentioned we handle this case somewhat, but by intercepting the backspace keybind. I think we can make it a bit smarter using @wlach suggestion of identifying the change in the document history state, and if any changes have been recorded from the original we can introduce warning dialog for back button.

@wlach
Copy link
Contributor

wlach commented Nov 25, 2019

Hey @wlach, @bcolloran,

I can look into this issue, as @acmiyaguchi mentioned we handle this case somewhat, but by intercepting the backspace keybind. I think we can make it a bit smarter using @wlach suggestion of identifying the change in the document history state, and if any changes have been recorded from the original we can introduce warning dialog for back button.

That would be amazing!

bcolloran pushed a commit that referenced this issue Dec 10, 2019
* submitting intercept changes for behavior testing

* made changes to test beforeunload

* remnant for testing lint comment
@wlach
Copy link
Contributor

wlach commented Dec 23, 2019

Hmm, sorry for the late reply on this, but I think this issue is not actually fixed. The behavior we want here is:

The PR that was landed improves things somewhat in that it makes it harder to lose your work, but it does not fix the issue as stated. My suggestions here would still apply:

#2403 (comment)

@wlach wlach reopened this Dec 23, 2019
@bcolloran
Copy link
Contributor

@wlach the issue as-stated is not fixed (tho anthony does say "An alternative is to provide the same dialog when "backspace" is pressed."...), but the big problem here is users losing work, not back button behavior. as long as we warn users about navigating away, the major issue is solved. we can keep this open, but i think implementing this back button behavior is quite low priority

@Algentile
Copy link
Contributor

Hey @bcolloran,

I believe @wlach reopened the issue because there are a few edge cases where intercepting the back button does prevent the loss of work, but in certain edge cases the event is captured even when back navigation occurs on notebooks that the have never been edited by the user, which is not a great user experience.

I have been slow on the implementation on this due to the fact that the state change does not seem to be made on replace state, but also appears to be manipulated via the setViewMode action for explore and report view.

My suggestion is if we do keep this issue open we might want to close #2537 as a duplicate.

@bcolloran
Copy link
Contributor

hmm, i think will reopened this b/c of the part about pushing history and back button behavior rather than the bad experience bits you mention, which he brings up in #2537

but in any case, that you for the reminder @Algentile -- perhaps this issue is now a subset of #2537 , which also talks about pushing history onto the stack and using the back button to move through that stack
#2537 (comment)
i think we should keep #2537 open, and if anything close this as a subset of that

@wlach
Copy link
Contributor

wlach commented Jan 6, 2020

I think these are really seperate issues, and should be kept as such. Really this bug has nothing to do with losing state per se-- it's just about making navigation events happen as the user interacts with iodide. If you have any more questions, please feel free to ping me either on gitter or in the iodide meeting and I'll be glad to explain more.

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

Successfully merging a pull request may close this issue.

4 participants