From b607618342d6c4fab364966fe05f152be6bd4d5f Mon Sep 17 00:00:00 2001 From: Georgios Kalpakas Date: Fri, 30 Dec 2016 15:49:45 +0200 Subject: [PATCH] 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 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 --- src/ng/location.js | 69 ++++++++++++++++++++++++----------------- src/ng/rootScope.js | 4 +++ test/ng/locationSpec.js | 58 ++++++++++++++++++++++++++++++++++ 3 files changed, 102 insertions(+), 29 deletions(-) diff --git a/src/ng/location.js b/src/ng/location.js index 905407e4e191..b92ff8ca38c4 100644 --- a/src/ng/location.js +++ b/src/ng/location.js @@ -137,6 +137,8 @@ function LocationHtml5Url(appBase, appBaseNoFile, basePrefix) { this.$$url = encodePath(this.$$path) + (search ? '?' + search : '') + hash; this.$$absUrl = appBaseNoFile + this.$$url.substr(1); // first char is always '/' + + this.$$urlUpdatedByLocation = true; }; this.$$parseLinkUrl = function(url, relHref) { @@ -270,6 +272,8 @@ function LocationHashbangUrl(appBase, appBaseNoFile, hashPrefix) { this.$$url = encodePath(this.$$path) + (search ? '?' + search : '') + hash; this.$$absUrl = appBase + (this.$$url ? hashPrefix + this.$$url : ''); + + this.$$urlUpdatedByLocation = true; }; this.$$parseLinkUrl = function(url, relHref) { @@ -327,6 +331,8 @@ function LocationHashbangInHtml5Url(appBase, appBaseNoFile, hashPrefix) { this.$$url = encodePath(this.$$path) + (search ? '?' + search : '') + hash; // include hashPrefix in $$absUrl when $$url is empty so IE9 does not reload page because of removal of '#' this.$$absUrl = appBase + hashPrefix + this.$$url; + + this.$$urlUpdatedByLocation = true; }; } @@ -656,6 +662,7 @@ forEach([LocationHashbangInHtml5Url, LocationHashbangUrl, LocationHtml5Url], fun // but we're changing the $$state reference to $browser.state() during the $digest // so the modification window is narrow. this.$$state = isUndefined(state) ? null : state; + this.$$urlUpdatedByLocation = true; return this; }; @@ -968,36 +975,40 @@ function $LocationProvider() { // update browser $rootScope.$watch(function $locationWatch() { - var oldUrl = trimEmptyHash($browser.url()); - var newUrl = trimEmptyHash($location.absUrl()); - var oldState = $browser.state(); - var currentReplace = $location.$$replace; - var urlOrStateChanged = oldUrl !== newUrl || - ($location.$$html5 && $sniffer.history && oldState !== $location.$$state); - - if (initializing || urlOrStateChanged) { - initializing = false; - - $rootScope.$evalAsync(function() { - var newUrl = $location.absUrl(); - var defaultPrevented = $rootScope.$broadcast('$locationChangeStart', newUrl, oldUrl, - $location.$$state, oldState).defaultPrevented; - - // if the location was changed by a `$locationChangeStart` handler then stop - // processing this location change - if ($location.absUrl() !== newUrl) return; - - if (defaultPrevented) { - $location.$$parse(oldUrl); - $location.$$state = oldState; - } else { - if (urlOrStateChanged) { - setBrowserUrlWithFallback(newUrl, currentReplace, - oldState === $location.$$state ? null : $location.$$state); + if (initializing || $location.$$urlUpdatedByLocation) { + $location.$$urlUpdatedByLocation = false; + + var oldUrl = trimEmptyHash($browser.url()); + var newUrl = trimEmptyHash($location.absUrl()); + var oldState = $browser.state(); + var currentReplace = $location.$$replace; + var urlOrStateChanged = oldUrl !== newUrl || + ($location.$$html5 && $sniffer.history && oldState !== $location.$$state); + + if (initializing || urlOrStateChanged) { + initializing = false; + + $rootScope.$evalAsync(function() { + var newUrl = $location.absUrl(); + var defaultPrevented = $rootScope.$broadcast('$locationChangeStart', newUrl, oldUrl, + $location.$$state, oldState).defaultPrevented; + + // if the location was changed by a `$locationChangeStart` handler then stop + // processing this location change + if ($location.absUrl() !== newUrl) return; + + if (defaultPrevented) { + $location.$$parse(oldUrl); + $location.$$state = oldState; + } else { + if (urlOrStateChanged) { + setBrowserUrlWithFallback(newUrl, currentReplace, + oldState === $location.$$state ? null : $location.$$state); + } + afterLocationChange(oldUrl, oldState); } - afterLocationChange(oldUrl, oldState); - } - }); + }); + } } $location.$$replace = false; diff --git a/src/ng/rootScope.js b/src/ng/rootScope.js index 89279608464c..690d42d430c8 100644 --- a/src/ng/rootScope.js +++ b/src/ng/rootScope.js @@ -876,6 +876,10 @@ function $RootScopeProvider() { } } postDigestQueue.length = postDigestQueuePosition = 0; + + // Check for changes to browser url that happened during the $digest + // (for which no event is fired; e.g. via `history.pushState()`) + $browser.$$checkUrlChange(); }, diff --git a/test/ng/locationSpec.js b/test/ng/locationSpec.js index 63cbbad5046a..3ab431234dff 100644 --- a/test/ng/locationSpec.js +++ b/test/ng/locationSpec.js @@ -710,6 +710,7 @@ describe('$location', function() { ); }); + it('should not infinitely digest when using a semicolon in initial path', function() { initService({html5Mode:true,supportHistory:true}); mockUpBrowser({initialUrl:'http://localhost:9876/;jsessionid=foo', baseHref:'/'}); @@ -721,6 +722,63 @@ describe('$location', function() { }); + describe('when changing the browser URL/history directly during a `$digest`', function() { + + beforeEach(function() { + initService({supportHistory: true}); + mockUpBrowser({initialUrl: 'http://foo.bar/', baseHref: '/'}); + }); + + + it('should correctly update `$location` from history and not digest infinitely', inject( + function($browser, $location, $rootScope, $window) { + $location.url('baz'); + $rootScope.$digest(); + + var originalUrl = $window.location.href; + + $rootScope.$apply(function() { + $rootScope.$evalAsync(function() { + $window.history.pushState({}, null, originalUrl + '/qux'); + }); + }); + + expect($browser.url()).toBe('http://foo.bar/#!/baz/qux'); + expect($location.absUrl()).toBe('http://foo.bar/#!/baz/qux'); + + $rootScope.$apply(function() { + $rootScope.$evalAsync(function() { + $window.history.replaceState({}, null, originalUrl + '/quux'); + }); + }); + + expect($browser.url()).toBe('http://foo.bar/#!/baz/quux'); + expect($location.absUrl()).toBe('http://foo.bar/#!/baz/quux'); + }) + ); + + + it('should correctly update `$location` from URL and not digest infinitely', inject( + function($browser, $location, $rootScope, $window) { + $location.url('baz'); + $rootScope.$digest(); + + $rootScope.$apply(function() { + $rootScope.$evalAsync(function() { + $window.location.href += '/qux'; + }); + }); + + jqLite($window).triggerHandler('hashchange'); + + expect($browser.url()).toBe('http://foo.bar/#!/baz/qux'); + expect($location.absUrl()).toBe('http://foo.bar/#!/baz/qux'); + }) + ); + + }); + + function updatePathOnLocationChangeSuccessTo(newPath) { inject(function($rootScope, $location) { $rootScope.$on('$locationChangeSuccess', function(event, newUrl, oldUrl) {