Cancel navigateBack from canDeactivate

[TonyLugg]

I'm submitting a bug report

• Library Version: aurelia-router: 1.3.0

• Operating System: Windows 10

• Node Version: 6.11.3

• NPM Version: 5.3.0

• JSPM OR Webpack AND Version Webpack 3.5.5 (using CLI)

• Browser: all

• Language: TypeScript 2.5.2

Current behavior: I load the app, which loads "home" route/view. Press a button and use router.navigate("second") to go to second page. Enter some data and press back button (on page) which executes router.navigateBack(). Because data has changed, display a message box (Aurelia dialog) from canDeactivate() to ask for user confirmation (same behavior with window.confirm()), and return false from canDeactivate(). Navigation stops. Repeat from pressing back button (on page), backward navigation continues and navigates right out of the app as if it has navigated back twice.

https://gist.github.com/TonyLugg/f4a7fa39c9d82f5de0af3b0bc8e01850

Expected/desired behavior: Each time false is returned from canDeactivate, navigateBack() should be stopped.

If I press a different button and perform navigate("someOtherRoute") and return false from canDeactivate it works correctly with multiple navigation attempts.

[TonyLugg]

Just wondering if this has any attention. It is causing me a bit of grief.

[davismj]

@TonyLugg Thanks for your detailed report.

Running gist here: https://gist.run/?id=f4a7fa39c9d82f5de0af3b0bc8e01850

[gama410]

Hello!

Any update on this bug?

[davismj]

I'm going to want to start looking at this for 1.6.0, potentially a March release if all goes well.

[davismj]

The core issue is that router.navigateBack() calls history.back(), which is a purposefully opaque API. When the cancel is processed, it is not forwarded to the history and so the history believes that the navigation back was successful.

[gama410]

I'm not familiar with the implementation details but wouldn't it be logical to call history.back only after candeactivate returns true? It seems weird to me that we navigate back before checking if we are allowed to.

[davismj]

It's not a straightforward switch. history.back() is used to change the url, which kicks off the navigation pipeline including canDeactivate. This strategy makes sense as well because if we just read the previous state and navigated to it, we'd be adding a new entry into the history, not navigating backwards in it.

I've done some experiments calling history.forward when a navigateBack() fails, but it seems that calling the two in rapid succession causes them to fail to function properly. See this gist: https://gist.run/?id=6e306fa6d5c9675e8ea55fe5f495be2f

[davismj]

I've found a number of related bugs. Specifically, if you start at #/first and navigate to #/second via <a href="#/second">, and if first returns false from canDeactivate(), a new history item would be appended for the current route.

Some early tests show this solving this issue and most of the related issues I've uncovered:

  function restorePreviousLocation(router) {
    var previousLocation = router.history.previousLocation;
    if (previousLocation) {
      Promise.resolve().then(() => history.go(router.isNavigatingBack ? 1 : -1));
    } else if (router.fallbackRoute) {
      router.navigate(router.fallbackRoute, { trigger: true, replace: true });
    } else {
      logger.error('Router navigation failed, and no previous location or fallbackRoute could be restored.');
    }
  }

I need to do additional testing to with pushState and with child routers.

[davismj]

This strategy is not a good one because it enters an infinite loop when the user navigates back (or forwards) more than one entry.

I think the only and right way to handle this is using sessionStorage. I'm going to give that a try.

[TonyLugg]

It has been a year since I reported this issue. Any update on expected fix date?

[davismj]

I did spend a fair amount of time trying to fix it, but unfortunately due to limitations on the browser detailed above I was not able to make sufficient progress. I'd be happy to accept a PR from a community member who thinks they may have a solution.

[TonyLugg]

Thanks for the update. We will avoid using CanDeactivate.

[jwx]

@TonyLugg @davismj I think I could put together a PR if it's of interest.

[TonyLugg]

@jwx @davismj It is of great interest. Thank you.

[davismj]

I'd really like to see it too. I'd be happy to work with you on it. It's tougher than it seems. If you open up a preliminary PR we can collaborate on it as well.

[jwx]

I'll take a look later this weekend. Just to be clear (since I haven't really used the "exit" cycle steps): canDeactivate only triggers when you're navigating (either through link or history) to a different page within the same Aurelia application, right? Since navigation is done via browser location, if I go from my-aurelia-app.com/#/some-page to not-an-aurelia-app.com/elsewhere the exit cycle steps can't run due to the nature of the browser switching to the new site immediately. Right?

[jwx]

Note that this doesn't in any way prevent a user from leaving the Aurelia application; it only deals with navigation within the application.

[jwx]

Also, I'd really appreciate some help in testing this with pushstate, other browsers than Chrome, some more use cases etc. @davismj @TonyLugg

[TonyLugg]

I'm happy to help with testing. However, I am a .NET/WPF with TFS guy turned Aurelia, so the whole git workflow is a bit foreign to me. If you can help me setup the test environment I will make the effort.

[jwx]

Thanks @TonyLugg! I've made two extra branches to simplify testing so just installing

npm install --save git://github.com/jwx/history-browser.git#history-index-test
npm install --save git://github.com/jwx/router.git#candeactivate-history-bugfix-test

in a project should do.

[TonyLugg]

OK, I will give this a try. Are they ready now?

[jwx]

Yeah, should be.

[TonyLugg]

@jwx I have tested your fix, both backward and forward, with cancellation and without, with user confirmation and without, including multiple history items deep. It works perfectly! Great work!

@davismj When can we get it released in a router update?

[gama410]

Hello @davismj ! I'm checking my opened issues at work and noticed I still have this issue opened. I'm surprised the fix from @jwx has not been merged, did you encounter some problems with it?

[jwx]

@gama410 This is on me. I was asked to create some thorough tests for this and have so far been coming up short. I hope to have it fixed before the end of the week.

[gama410]

Ok great! Thanks for your work!

[timmyktom]

Hi @davismj @jwx - Any update on this ?

[tomasbillborn]

Would also love to see this implemented, any update on when this could be merged? @davismj @jwx

[davismj]

It works in the basic case but edge cases are not handled, e.g. when a route is not found. See here for the status of the PR: https://github.com/aurelia/router/pull/621

[sousekd]

Is there any viable workaround for this issue? I need to ask user about leaving the large form with unsaved data.