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

fix(router): fix a problem with router not responding to back button #30160

Closed

Conversation

Projects
None yet
4 participants
@jasonaden
Copy link
Contributor

commented Apr 26, 2019

There was a problem with a combination of the eager URL update, browser back button, and hybrid applications. Details provided in internal ticket http://b/123667227.

This fix handles the problem by setting router.browserUrlTree when all conditions have failed, meaning the browser doesn't do anything with the navigation other than update internal data structures. Without this change, the problem was an old value was stored in router.broserUrlTree causing some new navigations to be compared to an old value and breaking future navigations.

fix(router): fix a problem with router not responding to back button
There was a problem with a combination of the `eager` URL update, browser `back` button, and hybrid applications. Details provided in internal ticket http://b/123667227.

This fix handles the problem by setting `router.browserUrlTree` when all conditions have failed, meaning the browser doesn't do anything with the navigation other than update internal data structures. Without this change, the problem was an old value was stored in `router.broserUrlTree` causing some new navigations to be compared to an old value and breaking future navigations.

@jasonaden jasonaden requested a review from angular/fw-router as a code owner Apr 26, 2019

@googlebot googlebot added the cla: yes label Apr 26, 2019

@jasonaden

This comment has been minimized.

Copy link
Contributor Author

commented Apr 26, 2019

@kara

This comment has been minimized.

Copy link
Contributor

commented May 6, 2019

@jasonaden Linter is failing, so I added "cleanup" to this.

@kara

This comment has been minimized.

Copy link
Contributor

commented May 6, 2019

@jasonaden Still failing :-(

@jasonaden jasonaden force-pushed the jasonaden:FW-1297_fix_state_on_failure branch from 6b2f28d to cc785f4 May 6, 2019

@jasonaden

This comment has been minimized.

Copy link
Contributor Author

commented May 6, 2019

Thanks... passed now.

@kara kara closed this in 3327bd8 May 6, 2019

kara added a commit that referenced this pull request May 6, 2019

fix(router): fix a problem with router not responding to back button (#…
…30160)

There was a problem with a combination of the `eager` URL update, browser `back` button, and hybrid applications. Details provided in internal ticket http://b/123667227.

This fix handles the problem by setting `router.browserUrlTree` when all conditions have failed, meaning the browser doesn't do anything with the navigation other than update internal data structures. Without this change, the problem was an old value was stored in `router.broserUrlTree` causing some new navigations to be compared to an old value and breaking future navigations.

PR Close #30160

jasonaden added a commit to jasonaden/angular that referenced this pull request May 8, 2019

alxhub added a commit that referenced this pull request May 8, 2019

alxhub added a commit that referenced this pull request May 8, 2019

jasonaden added a commit to jasonaden/angular that referenced this pull request May 8, 2019

fix(router): ensure navigations start with the current URL value inca…
…se redirect is skipped

In some cases where multiple navigations happen to the same URL, the router will not process a given URL. In those cases, we fall into logic that resets state for the next navigation. One piece of this resetting is to set the `browserUrlTree` to the most recent `urlAfterRedirects`i.

However, there was bug in this logic because in some cases the `urlAfterRedirects` is a stale value. This happens any time a URL won't be processed, and the previous URL will also not be processed. This creates unpredictable behavior, not the least of which ends up being a broken `back` button.

This PR kicks off new navigations with the current value the router assumes is in the browser. All the logic around how to handle future navigations is based on this value compared to the current transition, so it's important to kick off all new navigations with the current value so in the edge case described above we don't end up with an old value being set into `browserUrlTree`.

Fixes angular#30340
Related to angular#30160

jasonaden added a commit to jasonaden/angular that referenced this pull request May 8, 2019

fix(router): ensure navigations start with the current URL value inca…
…se redirect is skipped

In some cases where multiple navigations happen to the same URL, the router will not process a given URL. In those cases, we fall into logic that resets state for the next navigation. One piece of this resetting is to set the `browserUrlTree` to the most recent `urlAfterRedirects`i.

However, there was bug in this logic because in some cases the `urlAfterRedirects` is a stale value. This happens any time a URL won't be processed, and the previous URL will also not be processed. This creates unpredictable behavior, not the least of which ends up being a broken `back` button.

This PR kicks off new navigations with the current value the router assumes is in the browser. All the logic around how to handle future navigations is based on this value compared to the current transition, so it's important to kick off all new navigations with the current value so in the edge case described above we don't end up with an old value being set into `browserUrlTree`.

Fixes angular#30340
Related to angular#30160

jasonaden added a commit to jasonaden/angular that referenced this pull request May 8, 2019

alxhub added a commit that referenced this pull request May 15, 2019

fix(router): ensure navigations start with the current URL value inca…
…se redirect is skipped (#30344)

In some cases where multiple navigations happen to the same URL, the router will not process a given URL. In those cases, we fall into logic that resets state for the next navigation. One piece of this resetting is to set the `browserUrlTree` to the most recent `urlAfterRedirects`i.

However, there was bug in this logic because in some cases the `urlAfterRedirects` is a stale value. This happens any time a URL won't be processed, and the previous URL will also not be processed. This creates unpredictable behavior, not the least of which ends up being a broken `back` button.

This PR kicks off new navigations with the current value the router assumes is in the browser. All the logic around how to handle future navigations is based on this value compared to the current transition, so it's important to kick off all new navigations with the current value so in the edge case described above we don't end up with an old value being set into `browserUrlTree`.

Fixes #30340
Related to #30160

PR Close #30344

alxhub added a commit that referenced this pull request May 15, 2019

alxhub added a commit that referenced this pull request May 15, 2019

fix(router): ensure navigations start with the current URL value inca…
…se redirect is skipped (#30344)

In some cases where multiple navigations happen to the same URL, the router will not process a given URL. In those cases, we fall into logic that resets state for the next navigation. One piece of this resetting is to set the `browserUrlTree` to the most recent `urlAfterRedirects`i.

However, there was bug in this logic because in some cases the `urlAfterRedirects` is a stale value. This happens any time a URL won't be processed, and the previous URL will also not be processed. This creates unpredictable behavior, not the least of which ends up being a broken `back` button.

This PR kicks off new navigations with the current value the router assumes is in the browser. All the logic around how to handle future navigations is based on this value compared to the current transition, so it's important to kick off all new navigations with the current value so in the edge case described above we don't end up with an old value being set into `browserUrlTree`.

Fixes #30340
Related to #30160

PR Close #30344

alxhub added a commit that referenced this pull request May 15, 2019

BioPhoton added a commit to BioPhoton/angular that referenced this pull request May 21, 2019

fix(router): fix a problem with router not responding to back button (a…
…ngular#30160)

There was a problem with a combination of the `eager` URL update, browser `back` button, and hybrid applications. Details provided in internal ticket http://b/123667227.

This fix handles the problem by setting `router.browserUrlTree` when all conditions have failed, meaning the browser doesn't do anything with the navigation other than update internal data structures. Without this change, the problem was an old value was stored in `router.broserUrlTree` causing some new navigations to be compared to an old value and breaking future navigations.

PR Close angular#30160

BioPhoton added a commit to BioPhoton/angular that referenced this pull request May 21, 2019

BioPhoton added a commit to BioPhoton/angular that referenced this pull request May 21, 2019

fix(router): ensure navigations start with the current URL value inca…
…se redirect is skipped (angular#30344)

In some cases where multiple navigations happen to the same URL, the router will not process a given URL. In those cases, we fall into logic that resets state for the next navigation. One piece of this resetting is to set the `browserUrlTree` to the most recent `urlAfterRedirects`i.

However, there was bug in this logic because in some cases the `urlAfterRedirects` is a stale value. This happens any time a URL won't be processed, and the previous URL will also not be processed. This creates unpredictable behavior, not the least of which ends up being a broken `back` button.

This PR kicks off new navigations with the current value the router assumes is in the browser. All the logic around how to handle future navigations is based on this value compared to the current transition, so it's important to kick off all new navigations with the current value so in the edge case described above we don't end up with an old value being set into `browserUrlTree`.

Fixes angular#30340
Related to angular#30160

PR Close angular#30344

BioPhoton added a commit to BioPhoton/angular that referenced this pull request May 21, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.