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

acmiyaguchi commented 5 years ago

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

Start at https://alpha.iodide.io/ (the homepage)
Click on "A Brief Tour through Iodide" (or any other notebook)
Click on "REPORT" in the top right
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 commented 5 years ago

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

wlach commented 5 years ago

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

https://github.com/iodide-project/iodide/blob/8a176a44858018d0e8f4230154235778da2c5d2f/src/editor/actions/notebook-actions.js#L41

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 commented 5 years ago

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 commented 4 years ago

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 commented 4 years ago

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!

wlach commented 4 years ago

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

User presses back button in report view (e.g. https://definitely-staging.iodide.io/notebooks/232/?viewMode=report)
Iodide returns to "explore" view (including updating the URL e.g. https://definitely-staging.iodide.io/notebooks/232/) while preserving any state changes

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:

https://github.com/iodide-project/iodide/issues/2403#issuecomment-548816541

bcolloran commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

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 https://github.com/iodide-project/iodide/issues/2537#issuecomment-564293106 i think we should keep #2537 open, and if anything close this as a subset of that

wlach commented 4 years ago

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.

iodide-project / iodide