Skip to content

Commit 8ee1ba4

Browse files
tboschtbosch
committed
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 dca2317 and d707114. Adds more tests for angular#6976 Fixes angular#9235 Closes angular#9455
1 parent ab24019 commit 8ee1ba4

2 files changed

Lines changed: 178 additions & 31 deletions

File tree

‎src/ng/browser.js‎

Lines changed: 8 additions & 11 deletions
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‎

Lines changed: 170 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -495,6 +495,55 @@ 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+
});
511+
512+
it('assumes that changes to location.hash occur in sync', function() {
513+
// This is an asynchronous integration test that changes the
514+
// hash in all possible ways and checks
515+
// - whether the change to the hash can be read out in sync
516+
// - whether the change to the hash can be read out in the hashchange event
517+
var realWin = window,
518+
$realWin = jqLite(realWin),
519+
hashInHashChangeEvent = [];
520+
521+
runs(function() {
522+
$realWin.on('hashchange', hashListener);
523+
524+
realWin.location.hash = '1';
525+
realWin.location.href += '2';
526+
realWin.location.replace(realWin.location.href + '3');
527+
realWin.location.assign(realWin.location.href + '4');
528+
529+
expect(realWin.location.hash).toBe('#1234');
530+
});
531+
waitsFor(function() {
532+
return hashInHashChangeEvent.length > 3;
533+
});
534+
runs(function() {
535+
$realWin.off('hashchange', hashListener);
536+
537+
forEach(hashInHashChangeEvent, function(hash) {
538+
expect(hash).toBe('#1234');
539+
});
540+
});
541+
542+
function hashListener() {
543+
hashInHashChangeEvent.push(realWin.location.hash);
544+
}
545+
});
546+
498547
});
499548

500549
describe('urlChange', function() {
@@ -573,15 +622,15 @@ describe('browser', function() {
573622
beforeEach(function() {
574623
sniffer.history = false;
575624
sniffer.hashchange = false;
576-
browser.url("http://server.current");
625+
browser.url("http://server/#current");
577626
});
578627

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

582-
fakeWindow.location.href = 'http://server.new';
631+
fakeWindow.location.href = 'http://server/#new';
583632
fakeWindow.setTimeout.flush();
584-
expect(callback).toHaveBeenCalledWith('http://server.new');
633+
expect(callback).toHaveBeenCalledWith('http://server/#new');
585634

586635
fakeWindow.fire('popstate');
587636
fakeWindow.fire('hashchange');
@@ -645,33 +694,134 @@ describe('browser', function() {
645694

646695
describe('integration tests with $location', function() {
647696

648-
beforeEach(module(function($provide, $locationProvider) {
649-
spyOn(fakeWindow.history, 'pushState').andCallFake(function(stateObj, title, newUrl) {
650-
fakeWindow.location.href = newUrl;
697+
function setup(options) {
698+
module(function($provide, $locationProvider) {
699+
spyOn(fakeWindow.history, 'pushState').andCallFake(function(stateObj, title, newUrl) {
700+
fakeWindow.location.href = newUrl;
701+
});
702+
spyOn(fakeWindow.location, 'replace').andCallFake(function(newUrl) {
703+
fakeWindow.location.href = newUrl;
704+
});
705+
$provide.value('$browser', browser);
706+
browser.pollFns = [];
707+
708+
sniffer.history = options.history;
709+
$provide.value('$sniffer', sniffer);
710+
711+
$locationProvider.html5Mode(options.html5Mode);
651712
});
652-
$provide.value('$browser', browser);
653-
browser.pollFns = [];
713+
}
654714

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

658-
$locationProvider.html5Mode(true);
659-
}));
718+
it('should work with no history support, no html5Mode', function() {
719+
setup({
720+
history: false,
721+
html5Mode: false
722+
});
723+
inject(function($rootScope, $location) {
724+
$rootScope.$apply(function() {
725+
$location.path('/initialPath');
726+
});
727+
expect(fakeWindow.location.href).toBe('http://server/#/initialPath');
660728

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');
729+
fakeWindow.location.href = 'http://server/#/someTestHash';
665730

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

671-
expect($location.path()).toBe('/someTestHash');
733+
expect($location.path()).toBe('/someTestHash');
734+
});
672735
});
736+
737+
it('should work with history support, no html5Mode', function() {
738+
setup({
739+
history: true,
740+
html5Mode: false
741+
});
742+
inject(function($rootScope, $location) {
743+
$rootScope.$apply(function() {
744+
$location.path('/initialPath');
745+
});
746+
expect(fakeWindow.location.href).toBe('http://server/#/initialPath');
747+
748+
fakeWindow.location.href = 'http://server/#/someTestHash';
749+
750+
$rootScope.$digest();
751+
752+
expect($location.path()).toBe('/someTestHash');
753+
});
754+
});
755+
756+
it('should work with no history support, with html5Mode', function() {
757+
setup({
758+
history: false,
759+
html5Mode: true
760+
});
761+
inject(function($rootScope, $location) {
762+
$rootScope.$apply(function() {
763+
$location.path('/initialPath');
764+
});
765+
expect(fakeWindow.location.href).toBe('http://server/#/initialPath');
766+
767+
fakeWindow.location.href = 'http://server/#/someTestHash';
768+
769+
$rootScope.$digest();
770+
771+
expect($location.path()).toBe('/someTestHash');
772+
});
773+
});
774+
775+
it('should work with history support, with html5Mode', function() {
776+
setup({
777+
history: true,
778+
html5Mode: true
779+
});
780+
inject(function($rootScope, $location) {
781+
$rootScope.$apply(function() {
782+
$location.path('/initialPath');
783+
});
784+
expect(fakeWindow.location.href).toBe('http://server/initialPath');
785+
786+
fakeWindow.location.href = 'http://server/someTestHash';
787+
788+
$rootScope.$digest();
789+
790+
expect($location.path()).toBe('/someTestHash');
791+
});
792+
});
793+
673794
});
674795

796+
it('should not reload the page on every $digest when the page will be reloaded due to url rewrite on load', function() {
797+
setup({
798+
history: false,
799+
html5Mode: true
800+
});
801+
fakeWindow.location.href = 'http://server/some/deep/path';
802+
var changeUrlCount = 0;
803+
var _url = browser.url;
804+
browser.url = function(newUrl, replace) {
805+
if (newUrl) {
806+
changeUrlCount++;
807+
}
808+
return _url.call(this, newUrl, replace);
809+
};
810+
spyOn(browser, 'url').andCallThrough();
811+
inject(function($rootScope, $location) {
812+
$rootScope.$digest();
813+
$rootScope.$digest();
814+
$rootScope.$digest();
815+
$rootScope.$digest();
816+
817+
// from $location for rewriting the initial url into a hash url
818+
expect(browser.url).toHaveBeenCalledWith('http://server/#/some/deep/path', true);
819+
// from the initial call to the watch in $location for watching $location
820+
expect(browser.url).toHaveBeenCalledWith('http://server/#/some/deep/path', false);
821+
expect(changeUrlCount).toBe(2);
822+
});
823+
824+
});
675825
});
676826

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

0 commit comments

Comments
 (0)