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.

Sign up for GitHub

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

Jump to bottom

fix(router): don't break history when CanDeactivate cancel back navigation #13922

Closed
wants to merge 1 commit into from
Closed

fix(router): don't break history when CanDeactivate cancel back navigation #13922

wants to merge 1 commit into from

Conversation

DzmitryShylovich
Copy link
Contributor

@DzmitryShylovich DzmitryShylovich commented Jan 13, 2017 •
edited by jasonaden

Closes ##13586

Description:
location.back() is called before router so there's no way to prevent if when CanDeactivate return false.
To maintain proper history we need to push (location.pushState) new entry into the history instead of replacing (location.replaceState) the current entry.

History before the fix

Action Length Current index Url
Init navigation 1 1 /
Navigate /route1 2 2 /route1
Navigate /route2 3 3 /route2
Navigate /route3 4 4 /route3
Navigate back (CanDeactivate false) 4 3 (here we are replacing current state instead of pushing new entry, this is why index is 3 instead of 4) /route3
Navigate back (CanDeactivate true) 4 2 /route1

After the fix

Action Length Current index Url
Init navigation 1 1 /
Navigate /route1 2 2 /route1
Navigate /route2 3 3 /route2
Navigate /route3 4 4 /route3
Navigate back (CanDeactivate false) 4 4 /route3
Navigate back (CanDeactivate true) 4 3 /route2

TLTR our goal: we need to call location.pushState instead of location.replaceState when CanDeactivate cancels back navigation

http://plnkr.co/edit/GMkEl74JkQiRPMv6Km5Z?p=preview

@DzmitryShylovich DzmitryShylovich changed the title fix(router): wip fix(router): don't break history when CanDeactivate cancel back navigation Jan 14, 2017
@mhevery mhevery requested a review from vicb January 18, 2017 23:56
@mhevery
Copy link
Contributor

mhevery commented Feb 10, 2017

@vicb Review when you get a chance.

@vicb vicb added the action: review The PR is still awaiting reviews from at least one requested reviewer label Mar 29, 2017
@vicb
Copy link
Contributor

vicb commented Mar 29, 2017

@DzmitryShylovich could toy please rebase the PR ?
Thanks

vicb
vicb reviewed Mar 30, 2017
View reviewed changes
modules/@angular/router/src/router.ts Outdated
@@ -613,14 +613,6 @@ export class Router {
Promise<boolean> {
const lastNavigation = this.navigations.value;

// If the user triggers a navigation imperatively (e.g., by using navigateByUrl),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this removed ?

I don't think it relates to the issue ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this check breaks this test.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there're no unit tests for this so I'm not sure what uses cases it covers

vicb
vicb reviewed Mar 30, 2017
View reviewed changes
modules/@angular/router/src/router.ts Outdated
this.resetUrlToCurrentUrlTree();
this.routerEvents.next(new NavigationCancel(id, this.serializeUrl(url), ''));
const navigatedUrl: string = this.serializeUrl(url);
// When navigation is canceled and it was initiated by the click on Back button,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if was the forward button ?

vicb
vicb suggested changes Mar 30, 2017
View reviewed changes
Copy link
Contributor

@vicb vicb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see inline comments

@vicb vicb added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Mar 30, 2017
@DzmitryShylovich
Copy link
Contributor Author

DzmitryShylovich commented Mar 30, 2017 •
edited

@vicb rebased

What if was the forward button ?

if forward button was called but navigation was canceled by a guard browser history would be 1 item ahead. afaik hostory api doesn't allow to delete entries so to maintain a proper history we will need to call location.back().
Basically it's:

  1. if back button was canceled we need to push 1 item by calling location.pushState because we're 1 item above
  2. if forward button was canceled we need to remove 1 item by calling location.back because we're 1 item ahead (but location.back will trigger new navigation...)

does it make sense?

the biggest problem is to figure out when back/forward button is called

@DzmitryShylovich
Copy link
Contributor Author

@vicb I've created a new issue for Location.forward bug #15664

@jasonaden jasonaden mentioned this pull request Jul 14, 2017
Closed
@jasonaden
Copy link
Contributor

Fixed moved to #18135

@jasonaden jasonaden closed this Jul 14, 2017
@blueberrynotblue
Copy link

feel sad for problem remained on 4.3

@UlricW
Copy link

UlricW commented Jan 12, 2018

This issue is it fixed ?
Thanks

@zvonimirprofico
Copy link

Does not seem like it is #13586 (comment)

@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@vicb vicb vicb requested changes

@bakhshi bakhshi bakhshi approved these changes

Assignees
No one assigned
Labels
action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews area: router cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@DzmitryShylovich @mhevery @vicb @jasonaden @blueberrynotblue @UlricW @zvonimirprofico @bakhshi @googlebot