From 2bcfe551905ec120e99d62b0d100827f50c032ac Mon Sep 17 00:00:00 2001 From: Tobias Bosch Date: Mon, 6 Oct 2014 18:04:22 -0700 Subject: [PATCH] fix($browser): Cache `location.href` only during page reload phase Adds caching for url changes while a reload is happening, as browsers do not allow to read out the new location the browser is navigating to. Removes unnecessary caching from $browser, as IE7-IE9 all have the new hash value in `location.href` after changing it. There was a wrong assumption in the previous version of this code introduced by dca23173e25a32cb740245ca7f7b01a84805f43f and d70711481e6311c9cd283d650f07ca0cca72ecc2. Adds more tests for #6976 Fixes #9235 --- src/ng/browser.js | 19 ++-- test/ng/browserSpecs.js | 190 +++++++++++++++++++++++++++++++++++----- 2 files changed, 178 insertions(+), 31 deletions(-) diff --git a/src/ng/browser.js b/src/ng/browser.js index c71d478ced75..e87e5e184e93 100644 --- a/src/ng/browser.js +++ b/src/ng/browser.js @@ -125,7 +125,7 @@ function Browser(window, document, $log, $sniffer) { var lastBrowserUrl = location.href, baseElement = document.find('base'), - newLocation = null; + reloadLocation = null; /** * @name $browser#url @@ -168,7 +168,9 @@ function Browser(window, document, $log, $sniffer) { baseElement.attr('href', baseElement.attr('href')); } } else { - newLocation = url; + if (!sameBase) { + reloadLocation = url; + } if (replace) { location.replace(url); } else { @@ -178,10 +180,10 @@ function Browser(window, document, $log, $sniffer) { return self; // getter } else { - // - newLocation is a workaround for an IE7-9 issue with location.replace and location.href - // methods not updating location.href synchronously. + // - reloadLocation is needed as browsers don't allow to read out + // the new location.href if a reload happened. // - the replacement is a workaround for https://bugzilla.mozilla.org/show_bug.cgi?id=407172 - return newLocation || location.href.replace(/%27/g,"'"); + return reloadLocation || location.href.replace(/%27/g,"'"); } }; @@ -189,11 +191,6 @@ function Browser(window, document, $log, $sniffer) { urlChangeInit = false; function fireUrlChange() { - newLocation = null; - checkUrlChange(); - } - - function checkUrlChange() { if (lastBrowserUrl == self.url()) return; lastBrowserUrl = self.url(); @@ -249,7 +246,7 @@ function Browser(window, document, $log, $sniffer) { * Needs to be exported to be able to check for changes that have been done in sync, * as hashchange/popstate events fire in async. */ - self.$$checkUrlChange = checkUrlChange; + self.$$checkUrlChange = fireUrlChange; ////////////////////////////////////////////////////////////// // Misc API diff --git a/test/ng/browserSpecs.js b/test/ng/browserSpecs.js index c3a3f42bbd4a..62b0a80c5298 100755 --- a/test/ng/browserSpecs.js +++ b/test/ng/browserSpecs.js @@ -490,6 +490,55 @@ describe('browser', function() { browser.url(current); expect(fakeWindow.location.href).toBe('dontchange'); }); + + it('should not read out location.href if a reload was triggered but still allow to change the url', function() { + sniffer.history = false; + browser.url('http://server/someOtherUrlThatCausesReload'); + expect(fakeWindow.location.href).toBe('http://server/someOtherUrlThatCausesReload'); + + fakeWindow.location.href = 'http://someNewUrl'; + expect(browser.url()).toBe('http://server/someOtherUrlThatCausesReload'); + + browser.url('http://server/someOtherUrl'); + expect(browser.url()).toBe('http://server/someOtherUrl'); + expect(fakeWindow.location.href).toBe('http://server/someOtherUrl'); + }); + + it('assumes that changes to location.hash occur in sync', function() { + // This is an asynchronous integration test that changes the + // hash in all possible ways and checks + // - whether the change to the hash can be read out in sync + // - whether the change to the hash can be read out in the hashchange event + var realWin = window, + $realWin = jqLite(realWin), + hashInHashChangeEvent = []; + + runs(function() { + $realWin.on('hashchange', hashListener); + + realWin.location.hash = '1'; + realWin.location.href += '2'; + realWin.location.replace(realWin.location.href + '3'); + realWin.location.assign(realWin.location.href + '4'); + + expect(realWin.location.hash).toBe('#1234'); + }); + waitsFor(function() { + return hashInHashChangeEvent.length > 3; + }); + runs(function() { + $realWin.off('hashchange', hashListener); + + forEach(hashInHashChangeEvent, function(hash) { + expect(hash).toBe('#1234'); + }); + }); + + function hashListener() { + hashInHashChangeEvent.push(realWin.location.hash); + } + }); + }); describe('urlChange', function() { @@ -568,15 +617,15 @@ describe('browser', function() { beforeEach(function() { sniffer.history = false; sniffer.hashchange = false; - browser.url("http://server.current"); + browser.url("http://server/#current"); }); it('should fire callback with the correct URL on location change outside of angular', function() { browser.onUrlChange(callback); - fakeWindow.location.href = 'http://server.new'; + fakeWindow.location.href = 'http://server/#new'; fakeWindow.setTimeout.flush(); - expect(callback).toHaveBeenCalledWith('http://server.new'); + expect(callback).toHaveBeenCalledWith('http://server/#new'); fakeWindow.fire('popstate'); fakeWindow.fire('hashchange'); @@ -640,33 +689,134 @@ describe('browser', function() { describe('integration tests with $location', function() { - beforeEach(module(function($provide, $locationProvider) { - spyOn(fakeWindow.history, 'pushState').andCallFake(function(stateObj, title, newUrl) { - fakeWindow.location.href = newUrl; + function setup(options) { + module(function($provide, $locationProvider) { + spyOn(fakeWindow.history, 'pushState').andCallFake(function(stateObj, title, newUrl) { + fakeWindow.location.href = newUrl; + }); + spyOn(fakeWindow.location, 'replace').andCallFake(function(newUrl) { + fakeWindow.location.href = newUrl; + }); + $provide.value('$browser', browser); + browser.pollFns = []; + + sniffer.history = options.history; + $provide.value('$sniffer', sniffer); + + $locationProvider.html5Mode(options.html5Mode); }); - $provide.value('$browser', browser); - browser.pollFns = []; + } - sniffer.history = true; - $provide.value('$sniffer', sniffer); + describe('update $location when it was changed outside of Angular in sync '+ + 'before $digest was called', function() { - $locationProvider.html5Mode(true); - })); + it('should work with no history support, no html5Mode', function() { + setup({ + history: false, + html5Mode: false + }); + inject(function($rootScope, $location) { + $rootScope.$apply(function() { + $location.path('/initialPath'); + }); + expect(fakeWindow.location.href).toBe('http://server/#/initialPath'); - it('should update $location when it was changed outside of Angular in sync '+ - 'before $digest was called', function() { - inject(function($rootScope, $location) { - fakeWindow.history.pushState(null, '', 'http://server/someTestHash'); + fakeWindow.location.href = 'http://server/#/someTestHash'; - // Verify that infinite digest reported in #6976 no longer occurs - expect(function() { $rootScope.$digest(); - }).not.toThrow(); - expect($location.path()).toBe('/someTestHash'); + expect($location.path()).toBe('/someTestHash'); + }); }); + + it('should work with history support, no html5Mode', function() { + setup({ + history: true, + html5Mode: false + }); + inject(function($rootScope, $location) { + $rootScope.$apply(function() { + $location.path('/initialPath'); + }); + expect(fakeWindow.location.href).toBe('http://server/#/initialPath'); + + fakeWindow.location.href = 'http://server/#/someTestHash'; + + $rootScope.$digest(); + + expect($location.path()).toBe('/someTestHash'); + }); + }); + + it('should work with no history support, with html5Mode', function() { + setup({ + history: false, + html5Mode: true + }); + inject(function($rootScope, $location) { + $rootScope.$apply(function() { + $location.path('/initialPath'); + }); + expect(fakeWindow.location.href).toBe('http://server/#/initialPath'); + + fakeWindow.location.href = 'http://server/#/someTestHash'; + + $rootScope.$digest(); + + expect($location.path()).toBe('/someTestHash'); + }); + }); + + it('should work with history support, with html5Mode', function() { + setup({ + history: true, + html5Mode: true + }); + inject(function($rootScope, $location) { + $rootScope.$apply(function() { + $location.path('/initialPath'); + }); + expect(fakeWindow.location.href).toBe('http://server/initialPath'); + + fakeWindow.location.href = 'http://server/someTestHash'; + + $rootScope.$digest(); + + expect($location.path()).toBe('/someTestHash'); + }); + }); + }); + it('should not reload the page on every $digest when the page will be reloaded due to url rewrite on load', function() { + setup({ + history: false, + html5Mode: true + }); + fakeWindow.location.href = 'http://server/some/deep/path'; + var changeUrlCount = 0; + var _url = browser.url; + browser.url = function(newUrl, replace) { + if (newUrl) { + changeUrlCount++; + } + return _url.call(this, newUrl, replace); + }; + spyOn(browser, 'url').andCallThrough(); + inject(function($rootScope, $location) { + $rootScope.$digest(); + $rootScope.$digest(); + $rootScope.$digest(); + $rootScope.$digest(); + + // from $location for rewriting the initial url into a hash url + expect(browser.url).toHaveBeenCalledWith('http://server/#/some/deep/path', true); + // from the initial call to the watch in $location for watching $location + expect(browser.url).toHaveBeenCalledWith('http://server/#/some/deep/path', false); + expect(changeUrlCount).toBe(2); + }); + + }); }); describe('integration test with $rootScope', function() {