fix($location): correctly handle external URL change during `$digest` #15561

gkalpak · 2016-12-30T15:29:51Z

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Bug fix(es).

What is the current behavior? (You can also link to an open issue here)

$browser.$$checkUrlChange() (which was run before each $digest) will only detect an external change (i.e. not via $location) to the browser URL. External changes to history.state will not be detected and propagated to $location.

This would not be a problem if changes were followed by a popstate or hashchange event (which would call cacheStateAndFireUrlChange()). But since history.pushState()/replaceState() do not fire any events, calling these methods manually will result in $location getting out-of-sync with the actual
history state.

This is not detected in tests, because the mocked window.history will incorrectly trigger popstate when calling pushState()/replaceState(), which "covers" the bug.
When the URL is changed directly (e.g. via location.href) during a $digest (e.g. via scope.$evalAsync() or promise.then()) the change is not handled correctly, unless a popstate or hashchange event is fired synchronously.

This is an issue when calling history.pushState()/replaceState() in all browsers, since these methods do not emit any event. This is also an issue when setting location.href in IE11, where (unlike other browsers) no popstate event is fired at all for hash-only changes (known bug) and the hashchange event is fired asynchronously (which is too late).

What is the new behavior (if this is a feature change)?

The first bug is fixed by ensuring cacheState() is always called, before looking for and propagating a URL/state change.
The second bug(s) is fixed by:
1. Keeping track of $location setter methods being called and only processing a URL change if it originated from such a call. If there is a URL difference but no setter method has been called, this means that the browser URL/history has been updated directly and the change hasn't yet been propagated to $location (e.g. due to no event being fired synchronously or at all).
2. Checking for URL/state changes at the end of the $digest, in order to detect changes via history methods (that took place during the $digest).

Does this PR introduce a breaking change?
_{Who k}NO_ws?
(This PR fixes some uncommon usecases, but I am not 100% sure it was not possible to somehow "exploit" the bugs to support a usecase that is now broken.)

Please check if the PR fulfills these requirements

The commit message follows our guidelines: https://github.com/angular/angular.js/blob/master/CONTRIBUTING.md#commit-message-format
Tests for the changes have been added (for bug fixes / features)
~~Docs have been added / updated (for bug fixes / features)~~

Other information:
Fixes #11075, #12571 and #15556.

Previously, `$browser.$$checkUrlChange()` (which was run before each `$digest`) would only detect an external change (i.e. not via `$location`) to the browser URL. External changes to `history.state` would not be detected and propagated to `$location`. This would not be a problem if changes were followed by a `popstate` or `hashchange` event (which would call `cacheStateAndFireUrlChange()`). But since `history.pushState()/replaceState()` do not fire any events, calling these methods manually would result in `$location` getting out-of-sync with the actual history state. This was not detected in tests, because the mocked `window.history` would incorrectly trigger `popstate` when calling `pushState()/replaceState()`, which "covered" the bug. This commit fixes it by always calling `cacheState()`, before looking for and propagating a URL/state change.

Previously, when the URL was changed directly (e.g. via `location.href`) during a `$digest` (e.g. via `scope.$evalAsync()` or `promise.then()`) the change was not handled correctly, unless a `popstate` or `hashchange` event was fired synchronously. This was an issue when calling `history.pushState()/replaceState()` in all browsers, since these methods do not emit any event. This was also an issue when setting `location.href` in IE11, where (unlike other browsers) no `popstate` event is fired at all for hash-only changes ([known bug][1]) and the `hashchange` event is fired asynchronously (which is too late). This commit fixes both usecases by: 1. Keeping track of `$location` setter methods being called and only processing a URL change if it originated from such a call. If there is a URL difference but no setter method has been called, this means that the browser URL/history has been updated directly and the change hasn't yet been propagated to `$location` (e.g. due to no event being fired synchronously or at all). 2. Checking for URL/state changes at the end of the `$digest`, in order to detect changes via `history` methods (that took place during the `$digest`). [1]: https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/3740423/ Fixes angular#11075 Fixes angular#12571 Fixes angular#15556

petebacondarwin

This sounds like a reasonable change.
The fact that the tests were artificially hiding the real behaviour was worrying (probably my fault).
Are we concerned that we now need a digest for the location to be updated - or was it the case that this was just broken completely previously, so waiting for a digest is a step forward?

gkalpak · 2017-01-04T16:30:48Z

Wow, that was quick 😃

Are we concerned that we now need a digest for the location to be updated - or was it the case that this was just broken completely previously, so waiting for a digest is a step forward?

The latter. $location will still be updated at the same point in time for cases that worked previously.
Previously, there was a "family" of external changes that would either go unnoticed (e.g. changes to history.state only) or cause an infinite digest error (e.g. changes to location.href in IE11 or history.pushState(...) in all browsers). Now, such changes will not cause infinite digest errors and will get noticed (but it is possible that they will immediately trigger another $digest).

I am a (tiny) little concerned that people might somehow rely on history.state changes being unnoticed and using that to support their obscure usecases 😃

petebacondarwin · 2017-01-04T17:46:19Z

I don't that is a breaking change. It was not documented to do that and it is very reasonable that the previous functionality is wrong.

Previously, when the URL was changed directly (e.g. via `location.href`) during a `$digest` (e.g. via `scope.$evalAsync()` or `promise.then()`) the change was not handled correctly, unless a `popstate` or `hashchange` event was fired synchronously. This was an issue when calling `history.pushState()/replaceState()` in all browsers, since these methods do not emit any event. This was also an issue when setting `location.href` in IE11, where (unlike other browsers) no `popstate` event is fired at all for hash-only changes ([known bug][1]) and the `hashchange` event is fired asynchronously (which is too late). This commit fixes both usecases by: 1. Keeping track of `$location` setter methods being called and only processing a URL change if it originated from such a call. If there is a URL difference but no setter method has been called, this means that the browser URL/history has been updated directly and the change hasn't yet been propagated to `$location` (e.g. due to no event being fired synchronously or at all). 2. Checking for URL/state changes at the end of the `$digest`, in order to detect changes via `history` methods (that took place during the `$digest`). [1]: https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/3740423/ Fixes #11075 Fixes #12571 Fixes #15556 Closes #15561

gkalpak · 2017-01-09T22:10:59Z

I've merged this into master and v1.6.x. I thought it might be too much for v1.5.x (considering it is a non-trivial change/fix), but could be convinced otherwise 😃

Previously, when the URL was changed directly (e.g. via `location.href`) during a `$digest` (e.g. via `scope.$evalAsync()` or `promise.then()`) the change was not handled correctly, unless a `popstate` or `hashchange` event was fired synchronously. This was an issue when calling `history.pushState()/replaceState()` in all browsers, since these methods do not emit any event. This was also an issue when setting `location.href` in IE11, where (unlike other browsers) no `popstate` event is fired at all for hash-only changes ([known bug][1]) and the `hashchange` event is fired asynchronously (which is too late). This commit fixes both usecases by: 1. Keeping track of `$location` setter methods being called and only processing a URL change if it originated from such a call. If there is a URL difference but no setter method has been called, this means that the browser URL/history has been updated directly and the change hasn't yet been propagated to `$location` (e.g. due to no event being fired synchronously or at all). 2. Checking for URL/state changes at the end of the `$digest`, in order to detect changes via `history` methods (that took place during the `$digest`). [1]: https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/3740423/ Fixes angular#11075 Fixes angular#12571 Fixes angular#15556 Closes angular#15561

dlongley · 2017-05-12T01:13:50Z

@gkalpak,

I am a (tiny) little concerned that people might somehow rely on history.state changes being unnoticed and using that to support their obscure usecases...

You were right! :)

I was relying on this behavior to ensure that I could hit the back button after opening modals that should not trigger route changes. I was previously adding a new history state and checking for its presence to determine whether or not the back button should close a modal or actually go back in history. It worked great in Angular 1.5.

With this patch now (in Angular 1.6) route changes are triggered after you push the custom history state -- and if you try and cancel them via preventDefault, the $browser code mangles the history. I'm hoping to discover a workaround to restore the functionality.

petebacondarwin · 2017-05-12T08:45:38Z

@dlongley - sorry about this. If you don't find a workaround please post again and we can see what we can do to fix it.

gkalpak added browser: IE11 component: $location frequency: low needs: review severity: broken expected use type: bug labels Dec 30, 2016

gkalpak added this to the Backlog milestone Dec 30, 2016

googlebot added the cla: yes label Dec 30, 2016

This was referenced Dec 30, 2016

Internet Explorer infinite digest with location change #15556

Closed

Bug: Changing the location outside of $location causes infdig and/or resets the original url #11075

Closed

gkalpak force-pushed the fix-location-when-no-popstate branch from 2e7a128 to 2bda69c Compare January 4, 2017 15:14

gkalpak removed the browser: IE11 label Jan 4, 2017

gkalpak force-pushed the fix-location-when-no-popstate branch from 2bda69c to c0a874e Compare January 4, 2017 15:45

gkalpak requested a review from petebacondarwin January 4, 2017 15:46

petebacondarwin approved these changes Jan 4, 2017

View reviewed changes

gkalpak closed this in 752f411 Jan 9, 2017

gkalpak deleted the fix-location-when-no-popstate branch January 11, 2017 21:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix($location): correctly handle external URL change during `$digest` #15561

fix($location): correctly handle external URL change during `$digest` #15561

gkalpak commented Dec 30, 2016 •

edited

Loading

petebacondarwin left a comment

gkalpak commented Jan 4, 2017

petebacondarwin commented Jan 4, 2017

gkalpak commented Jan 9, 2017

dlongley commented May 12, 2017

petebacondarwin commented May 12, 2017

fix($location): correctly handle external URL change during $digest #15561

fix($location): correctly handle external URL change during $digest #15561

Conversation

gkalpak commented Dec 30, 2016 • edited Loading

petebacondarwin left a comment

Choose a reason for hiding this comment

gkalpak commented Jan 4, 2017

petebacondarwin commented Jan 4, 2017

gkalpak commented Jan 9, 2017

dlongley commented May 12, 2017

petebacondarwin commented May 12, 2017

fix($location): correctly handle external URL change during `$digest` #15561

fix($location): correctly handle external URL change during `$digest` #15561

gkalpak commented Dec 30, 2016 •

edited

Loading