Skip to content

Commit 2bda69c

Browse files
gkalpakgkalpak
committed
fix($location): correctly handle external URL change during $digest
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 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
1 parent fef4ad2 commit 2bda69c

File tree

3 files changed

+102
-29
lines changed

3 files changed

+102
-29
lines changed

‎src/ng/location.js

+40-29
Original file line numberDiff line numberDiff line change
@@ -137,6 +137,8 @@ function LocationHtml5Url(appBase, appBaseNoFile, basePrefix) {
137137

138138
this.$$url = encodePath(this.$$path) + (search ? '?' + search : '') + hash;
139139
this.$$absUrl = appBaseNoFile + this.$$url.substr(1); // first char is always '/'
140+
141+
this.$$urlUpdatedByLocation = true;
140142
};
141143

142144
this.$$parseLinkUrl = function(url, relHref) {
@@ -270,6 +272,8 @@ function LocationHashbangUrl(appBase, appBaseNoFile, hashPrefix) {
270272

271273
this.$$url = encodePath(this.$$path) + (search ? '?' + search : '') + hash;
272274
this.$$absUrl = appBase + (this.$$url ? hashPrefix + this.$$url : '');
275+
276+
this.$$urlUpdatedByLocation = true;
273277
};
274278

275279
this.$$parseLinkUrl = function(url, relHref) {
@@ -327,6 +331,8 @@ function LocationHashbangInHtml5Url(appBase, appBaseNoFile, hashPrefix) {
327331
this.$$url = encodePath(this.$$path) + (search ? '?' + search : '') + hash;
328332
// include hashPrefix in $$absUrl when $$url is empty so IE9 does not reload page because of removal of '#'
329333
this.$$absUrl = appBase + hashPrefix + this.$$url;
334+
335+
this.$$urlUpdatedByLocation = true;
330336
};
331337

332338
}
@@ -656,6 +662,7 @@ forEach([LocationHashbangInHtml5Url, LocationHashbangUrl, LocationHtml5Url], fun
656662
// but we're changing the $$state reference to $browser.state() during the $digest
657663
// so the modification window is narrow.
658664
this.$$state = isUndefined(state) ? null : state;
665+
this.$$urlUpdatedByLocation = true;
659666

660667
return this;
661668
};
@@ -968,36 +975,40 @@ function $LocationProvider() {
968975

969976
// update browser
970977
$rootScope.$watch(function $locationWatch() {
971-
var oldUrl = trimEmptyHash($browser.url());
972-
var newUrl = trimEmptyHash($location.absUrl());
973-
var oldState = $browser.state();
974-
var currentReplace = $location.$$replace;
975-
var urlOrStateChanged = oldUrl !== newUrl ||
976-
($location.$$html5 && $sniffer.history && oldState !== $location.$$state);
977-
978-
if (initializing || urlOrStateChanged) {
979-
initializing = false;
980-
981-
$rootScope.$evalAsync(function() {
982-
var newUrl = $location.absUrl();
983-
var defaultPrevented = $rootScope.$broadcast('$locationChangeStart', newUrl, oldUrl,
984-
$location.$$state, oldState).defaultPrevented;
985-
986-
// if the location was changed by a `$locationChangeStart` handler then stop
987-
// processing this location change
988-
if ($location.absUrl() !== newUrl) return;
989-
990-
if (defaultPrevented) {
991-
$location.$$parse(oldUrl);
992-
$location.$$state = oldState;
993-
} else {
994-
if (urlOrStateChanged) {
995-
setBrowserUrlWithFallback(newUrl, currentReplace,
996-
oldState === $location.$$state ? null : $location.$$state);
978+
if (initializing || $location.$$urlUpdatedByLocation) {
979+
$location.$$urlUpdatedByLocation = false;
980+
981+
var oldUrl = trimEmptyHash($browser.url());
982+
var newUrl = trimEmptyHash($location.absUrl());
983+
var oldState = $browser.state();
984+
var currentReplace = $location.$$replace;
985+
var urlOrStateChanged = oldUrl !== newUrl ||
986+
($location.$$html5 && $sniffer.history && oldState !== $location.$$state);
987+
988+
if (initializing || urlOrStateChanged) {
989+
initializing = false;
990+
991+
$rootScope.$evalAsync(function() {
992+
var newUrl = $location.absUrl();
993+
var defaultPrevented = $rootScope.$broadcast('$locationChangeStart', newUrl, oldUrl,
994+
$location.$$state, oldState).defaultPrevented;
995+
996+
// if the location was changed by a `$locationChangeStart` handler then stop
997+
// processing this location change
998+
if ($location.absUrl() !== newUrl) return;
999+
1000+
if (defaultPrevented) {
1001+
$location.$$parse(oldUrl);
1002+
$location.$$state = oldState;
1003+
} else {
1004+
if (urlOrStateChanged) {
1005+
setBrowserUrlWithFallback(newUrl, currentReplace,
1006+
oldState === $location.$$state ? null : $location.$$state);
1007+
}
1008+
afterLocationChange(oldUrl, oldState);
9971009
}
998-
afterLocationChange(oldUrl, oldState);
999-
}
1000-
});
1010+
});
1011+
}
10011012
}
10021013

10031014
$location.$$replace = false;

‎src/ng/rootScope.js

+4
Original file line numberDiff line numberDiff line change
@@ -876,6 +876,10 @@ function $RootScopeProvider() {
876876
}
877877
}
878878
postDigestQueue.length = postDigestQueuePosition = 0;
879+
880+
// Check for changes to browser url that happened during the $digest
881+
// (for which no event is fired; e.g. via `history.pushState()`)
882+
$browser.$$checkUrlChange();
879883
},
880884

881885

‎test/ng/locationSpec.js

+58
Original file line numberDiff line numberDiff line change
@@ -710,6 +710,7 @@ describe('$location', function() {
710710
);
711711
});
712712

713+
713714
it('should not infinitely digest when using a semicolon in initial path', function() {
714715
initService({html5Mode:true,supportHistory:true});
715716
mockUpBrowser({initialUrl:'http://localhost:9876/;jsessionid=foo', baseHref:'/'});
@@ -721,6 +722,63 @@ describe('$location', function() {
721722
});
722723

723724

725+
describe('when changing the browser URL/history directly during a `$digest`', function() {
726+
727+
beforeEach(function() {
728+
initService({supportHistory: true});
729+
mockUpBrowser({initialUrl: 'http://foo.bar/', baseHref: '/'});
730+
});
731+
732+
733+
it('should correctly update `$location` from history and not digest infinitely', inject(
734+
function($browser, $location, $rootScope, $window) {
735+
$location.url('baz');
736+
$rootScope.$digest();
737+
738+
var originalUrl = $window.location.href;
739+
740+
$rootScope.$apply(function() {
741+
$rootScope.$evalAsync(function() {
742+
$window.history.pushState({}, null, originalUrl + '/qux');
743+
});
744+
});
745+
746+
expect($browser.url()).toBe('http://foo.bar/#!/baz/qux');
747+
expect($location.absUrl()).toBe('http://foo.bar/#!/baz/qux');
748+
749+
$rootScope.$apply(function() {
750+
$rootScope.$evalAsync(function() {
751+
$window.history.replaceState({}, null, originalUrl + '/quux');
752+
});
753+
});
754+
755+
expect($browser.url()).toBe('http://foo.bar/#!/baz/quux');
756+
expect($location.absUrl()).toBe('http://foo.bar/#!/baz/quux');
757+
})
758+
);
759+
760+
761+
it('should correctly update `$location` from URL and not digest infinitely', inject(
762+
function($browser, $location, $rootScope, $window) {
763+
$location.url('baz');
764+
$rootScope.$digest();
765+
766+
$rootScope.$apply(function() {
767+
$rootScope.$evalAsync(function() {
768+
$window.location.href += '/qux';
769+
});
770+
});
771+
772+
jqLite($window).triggerHandler('hashchange');
773+
774+
expect($browser.url()).toBe('http://foo.bar/#!/baz/qux');
775+
expect($location.absUrl()).toBe('http://foo.bar/#!/baz/qux');
776+
})
777+
);
778+
779+
});
780+
781+
724782
function updatePathOnLocationChangeSuccessTo(newPath) {
725783
inject(function($rootScope, $location) {
726784
$rootScope.$on('$locationChangeSuccess', function(event, newUrl, oldUrl) {

0 commit comments

Comments
 (0)