Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

Commit e4e3abb

Browse files
tboschtbosch
committed
fix($browser): Cache location.href only during page reload phase
Adds caching for url changes after a reload happens Removes unnecessary caching from $browser, as IE-IE9 all allow to change `location.href` synchronously, i.e. after changing it (via property write or `location.replace) it can be read out immediately with the up to date value. There was a wrong assumption in the previous version of this code introduced by dca2317 and d707114. Adds more tests for #6976 Fixes #9235
1 parent 409ad62 commit e4e3abb

File tree

2 files changed

+142
-31
lines changed

2 files changed

+142
-31
lines changed

‎src/ng/browser.js

+8-11
Original file line numberDiff line numberDiff line change
@@ -125,7 +125,7 @@ function Browser(window, document, $log, $sniffer) {
125125

126126
var lastBrowserUrl = location.href,
127127
baseElement = document.find('base'),
128-
newLocation = null;
128+
reloadLocation = null;
129129

130130
/**
131131
* @name $browser#url
@@ -166,7 +166,9 @@ function Browser(window, document, $log, $sniffer) {
166166
history.pushState(null, '', url);
167167
}
168168
} else {
169-
newLocation = url;
169+
if (!sameBase) {
170+
reloadLocation = url;
171+
}
170172
if (replace) {
171173
location.replace(url);
172174
} else {
@@ -176,22 +178,17 @@ function Browser(window, document, $log, $sniffer) {
176178
return self;
177179
// getter
178180
} else {
179-
// - newLocation is a workaround for an IE7-9 issue with location.replace and location.href
180-
// methods not updating location.href synchronously.
181+
// - reloadLocation is needed as browsers don't allow to read out
182+
// the new location.href if a reload happened.
181183
// - the replacement is a workaround for https://bugzilla.mozilla.org/show_bug.cgi?id=407172
182-
return newLocation || location.href.replace(/%27/g,"'");
184+
return reloadLocation || location.href.replace(/%27/g,"'");
183185
}
184186
};
185187

186188
var urlChangeListeners = [],
187189
urlChangeInit = false;
188190

189191
function fireUrlChange() {
190-
newLocation = null;
191-
checkUrlChange();
192-
}
193-
194-
function checkUrlChange() {
195192
if (lastBrowserUrl == self.url()) return;
196193

197194
lastBrowserUrl = self.url();
@@ -247,7 +244,7 @@ function Browser(window, document, $log, $sniffer) {
247244
* Needs to be exported to be able to check for changes that have been done in sync,
248245
* as hashchange/popstate events fire in async.
249246
*/
250-
self.$$checkUrlChange = checkUrlChange;
247+
self.$$checkUrlChange = fireUrlChange;
251248

252249
//////////////////////////////////////////////////////////////
253250
// Misc API

‎test/ng/browserSpecs.js

+134-20
Original file line numberDiff line numberDiff line change
@@ -495,6 +495,19 @@ describe('browser', function() {
495495
browser.url(current);
496496
expect(fakeWindow.location.href).toBe('dontchange');
497497
});
498+
499+
it('should not read out location.href if a reload was triggered but still allow to change the url', function() {
500+
sniffer.history = false;
501+
browser.url('http://server/someOtherUrlThatCausesReload');
502+
expect(fakeWindow.location.href).toBe('http://server/someOtherUrlThatCausesReload');
503+
504+
fakeWindow.location.href = 'http://someNewUrl';
505+
expect(browser.url()).toBe('http://server/someOtherUrlThatCausesReload');
506+
507+
browser.url('http://server/someOtherUrl');
508+
expect(browser.url()).toBe('http://server/someOtherUrl');
509+
expect(fakeWindow.location.href).toBe('http://server/someOtherUrl');
510+
});
498511
});
499512

500513
describe('urlChange', function() {
@@ -573,15 +586,15 @@ describe('browser', function() {
573586
beforeEach(function() {
574587
sniffer.history = false;
575588
sniffer.hashchange = false;
576-
browser.url("http://server.current");
589+
browser.url("http://server/#current");
577590
});
578591

579592
it('should fire callback with the correct URL on location change outside of angular', function() {
580593
browser.onUrlChange(callback);
581594

582-
fakeWindow.location.href = 'http://server.new';
595+
fakeWindow.location.href = 'http://server/#new';
583596
fakeWindow.setTimeout.flush();
584-
expect(callback).toHaveBeenCalledWith('http://server.new');
597+
expect(callback).toHaveBeenCalledWith('http://server/#new');
585598

586599
fakeWindow.fire('popstate');
587600
fakeWindow.fire('hashchange');
@@ -645,33 +658,134 @@ describe('browser', function() {
645658

646659
describe('integration tests with $location', function() {
647660

648-
beforeEach(module(function($provide, $locationProvider) {
649-
spyOn(fakeWindow.history, 'pushState').andCallFake(function(stateObj, title, newUrl) {
650-
fakeWindow.location.href = newUrl;
661+
function setup(options) {
662+
module(function($provide, $locationProvider) {
663+
spyOn(fakeWindow.history, 'pushState').andCallFake(function(stateObj, title, newUrl) {
664+
fakeWindow.location.href = newUrl;
665+
});
666+
spyOn(fakeWindow.location, 'replace').andCallFake(function(newUrl) {
667+
fakeWindow.location.href = newUrl;
668+
});
669+
$provide.value('$browser', browser);
670+
browser.pollFns = [];
671+
672+
sniffer.history = options.history;
673+
$provide.value('$sniffer', sniffer);
674+
675+
$locationProvider.html5Mode(options.html5Mode);
651676
});
652-
$provide.value('$browser', browser);
653-
browser.pollFns = [];
677+
}
654678

655-
sniffer.history = true;
656-
$provide.value('$sniffer', sniffer);
679+
describe('update $location when it was changed outside of Angular in sync '+
680+
'before $digest was called', function() {
657681

658-
$locationProvider.html5Mode(true);
659-
}));
682+
it('should work with no history support, no html5Mode', function() {
683+
setup({
684+
history: false,
685+
html5Mode: false
686+
});
687+
inject(function($rootScope, $location) {
688+
$rootScope.$apply(function() {
689+
$location.path('/initialPath');
690+
});
691+
expect(fakeWindow.location.href).toBe('http://server/#/initialPath');
660692

661-
it('should update $location when it was changed outside of Angular in sync '+
662-
'before $digest was called', function() {
663-
inject(function($rootScope, $location) {
664-
fakeWindow.history.pushState(null, '', 'http://server/someTestHash');
693+
fakeWindow.location.href = 'http://server/#/someTestHash';
665694

666-
// Verify that infinite digest reported in #6976 no longer occurs
667-
expect(function() {
668695
$rootScope.$digest();
669-
}).not.toThrow();
670696

671-
expect($location.path()).toBe('/someTestHash');
697+
expect($location.path()).toBe('/someTestHash');
698+
});
672699
});
700+
701+
it('should work with history support, no html5Mode', function() {
702+
setup({
703+
history: true,
704+
html5Mode: false
705+
});
706+
inject(function($rootScope, $location) {
707+
$rootScope.$apply(function() {
708+
$location.path('/initialPath');
709+
});
710+
expect(fakeWindow.location.href).toBe('http://server/#/initialPath');
711+
712+
fakeWindow.location.href = 'http://server/#/someTestHash';
713+
714+
$rootScope.$digest();
715+
716+
expect($location.path()).toBe('/someTestHash');
717+
});
718+
});
719+
720+
it('should work with no history support, with html5Mode', function() {
721+
setup({
722+
history: false,
723+
html5Mode: true
724+
});
725+
inject(function($rootScope, $location) {
726+
$rootScope.$apply(function() {
727+
$location.path('/initialPath');
728+
});
729+
expect(fakeWindow.location.href).toBe('http://server/#/initialPath');
730+
731+
fakeWindow.location.href = 'http://server/#/someTestHash';
732+
733+
$rootScope.$digest();
734+
735+
expect($location.path()).toBe('/someTestHash');
736+
});
737+
});
738+
739+
it('should work with history support, with html5Mode', function() {
740+
setup({
741+
history: true,
742+
html5Mode: true
743+
});
744+
inject(function($rootScope, $location) {
745+
$rootScope.$apply(function() {
746+
$location.path('/initialPath');
747+
});
748+
expect(fakeWindow.location.href).toBe('http://server/initialPath');
749+
750+
fakeWindow.location.href = 'http://server/someTestHash';
751+
752+
$rootScope.$digest();
753+
754+
expect($location.path()).toBe('/someTestHash');
755+
});
756+
});
757+
673758
});
674759

760+
it('should not reload the page multiple times when the page will be reloaded due to url rewrite on load', function() {
761+
setup({
762+
history: false,
763+
html5Mode: true
764+
});
765+
fakeWindow.location.href = 'http://server/some/deep/path';
766+
var changeUrlCount = 0;
767+
var _url = browser.url;
768+
browser.url = function(newUrl, replace) {
769+
if (newUrl) {
770+
changeUrlCount++;
771+
}
772+
return _url.call(this, newUrl, replace);
773+
};
774+
spyOn(browser, 'url').andCallThrough();
775+
inject(function($rootScope, $location) {
776+
$rootScope.$digest();
777+
$rootScope.$digest();
778+
$rootScope.$digest();
779+
$rootScope.$digest();
780+
781+
// from $location for rewriting the initial url into a hash url
782+
expect(browser.url).toHaveBeenCalledWith('http://server/#/some/deep/path', true);
783+
// from the initial call to the watch in $location for watching $location
784+
expect(browser.url).toHaveBeenCalledWith('http://server/#/some/deep/path', false);
785+
expect(changeUrlCount).toBe(2);
786+
});
787+
788+
});
675789
});
676790

677791
describe('integration test with $rootScope', function() {

0 commit comments

Comments
 (0)