fix(router): don't break history when CanDeactivate cancel back navigation by DzmitryShylovich · Pull Request #13922 · angular/angular

DzmitryShylovich · 2017-01-13T23:06:50Z

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

mhevery · 2017-02-10T21:47:06Z

@vicb Review when you get a chance.

vicb · 2017-03-29T22:31:51Z

@DzmitryShylovich could toy please rebase the PR ?
Thanks

vicb · 2017-03-30T20:11:42Z

why is this removed ?

I don't think it relates to the issue ?

this check breaks this test.

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

vicb · 2017-03-30T20:12:09Z

What if was the forward button ?

vicb

see inline comments

navigation Closes angular#13586

DzmitryShylovich · 2017-03-30T23:31:32Z

@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:

if back button was canceled we need to push 1 item by calling location.pushState because we're 1 item above
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 · 2017-03-31T12:38:06Z

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

jasonaden · 2017-07-14T20:25:14Z

Fixed moved to #18135

blueberrynotblue · 2017-08-03T06:26:40Z

feel sad for problem remained on 4.3

UlricW · 2018-01-12T14:40:50Z

This issue is it fixed ?
Thanks

zvonimirprofico · 2019-04-22T14:09:27Z

Does not seem like it is #13586 (comment)

angular-automatic-lock-bot · 2019-09-14T23:52:36Z

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.}

googlebot added the cla: yes label Jan 13, 2017

mhevery requested a review from vicb January 18, 2017 23:56

mhevery added the area: router label Jan 18, 2017

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

vicb reviewed Mar 30, 2017

View reviewed changes

Comment thread modules/@angular/router/src/router.ts Outdated

vicb Mar 30, 2017

Copy link
Copy Markdown

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 suggested changes Mar 30, 2017

View reviewed changes

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

fix(router): shouldn't break history when CanDeactivate cancel back

81f02d2

navigation Closes angular#13586

bakhshi approved these changes May 24, 2017

View reviewed changes

jasonaden mentioned this pull request Jul 14, 2017

fix(router): shouldn't break history when CanDeactivate cancel back #18135

Closed

jasonaden closed this Jul 14, 2017

angular-automatic-lock-bot Bot locked and limited conversation to collaborators Sep 14, 2019

Conversation

DzmitryShylovich commented Jan 13, 2017 • edited by jasonaden Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhevery commented Feb 10, 2017

Uh oh!

vicb commented Mar 29, 2017

Uh oh!

vicb Mar 30, 2017

Choose a reason for hiding this comment

Uh oh!

DzmitryShylovich Mar 30, 2017

Choose a reason for hiding this comment

Uh oh!

DzmitryShylovich Mar 30, 2017

Choose a reason for hiding this comment

Uh oh!

vicb Mar 30, 2017

Choose a reason for hiding this comment

Uh oh!

vicb left a comment

Choose a reason for hiding this comment

Uh oh!

DzmitryShylovich commented Mar 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DzmitryShylovich commented Mar 31, 2017

Uh oh!

jasonaden commented Jul 14, 2017

Uh oh!

blueberrynotblue commented Aug 3, 2017

Uh oh!

UlricW commented Jan 12, 2018

Uh oh!

zvonimirprofico commented Apr 22, 2019

Uh oh!

angular-automatic-lock-bot Bot commented Sep 14, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

DzmitryShylovich commented Jan 13, 2017 •

edited by jasonaden

Loading

DzmitryShylovich commented Mar 30, 2017 •

edited

Loading