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

Commit 434d7a0

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 #6976 Fixes #9235 Closes #9470
1 parent a6e6438 commit 434d7a0

File tree

2 files changed

+178
-31
lines changed

2 files changed

+178
-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
@@ -168,7 +168,9 @@ function Browser(window, document, $log, $sniffer) {
168168
baseElement.attr('href', baseElement.attr('href'));
169169
}
170170
} else {
171-
newLocation = url;
171+
if (!sameBase) {
172+
reloadLocation = url;
173+
}
172174
if (replace) {
173175
location.replace(url);
174176
} else {
@@ -178,22 +180,17 @@ function Browser(window, document, $log, $sniffer) {
178180
return self;
179181
// getter
180182
} else {
181-
// - newLocation is a workaround for an IE7-9 issue with location.replace and location.href
182-
// methods not updating location.href synchronously.
183+
// - reloadLocation is needed as browsers don't allow to read out
184+
// the new location.href if a reload happened.
183185
// - the replacement is a workaround for https://bugzilla.mozilla.org/show_bug.cgi?id=407172
184-
return newLocation || location.href.replace(/%27/g,"'");
186+
return reloadLocation || location.href.replace(/%27/g,"'");
185187
}
186188
};
187189

188190
var urlChangeListeners = [],
189191
urlChangeInit = false;
190192

191193
function fireUrlChange() {
192-
newLocation = null;
193-
checkUrlChange();
194-
}
195-
196-
function checkUrlChange() {
197194
if (lastBrowserUrl == self.url()) return;
198195

199196
lastBrowserUrl = self.url();
@@ -249,7 +246,7 @@ function Browser(window, document, $log, $sniffer) {
249246
* Needs to be exported to be able to check for changes that have been done in sync,
250247
* as hashchange/popstate events fire in async.
251248
*/
252-
self.$$checkUrlChange = checkUrlChange;
249+
self.$$checkUrlChange = fireUrlChange;
253250

254251
//////////////////////////////////////////////////////////////
255252
// Misc API

‎test/ng/browserSpecs.js

+170-20
Original file line numberDiff line numberDiff line change
@@ -490,6 +490,55 @@ describe('browser', function() {
490490
browser.url(current);
491491
expect(fakeWindow.location.href).toBe('dontchange');
492492
});
493+
494+
it('should not read out location.href if a reload was triggered but still allow to change the url', function() {
495+
sniffer.history = false;
496+
browser.url('http://server/someOtherUrlThatCausesReload');
497+
expect(fakeWindow.location.href).toBe('http://server/someOtherUrlThatCausesReload');
498+
499+
fakeWindow.location.href = 'http://someNewUrl';
500+
expect(browser.url()).toBe('http://server/someOtherUrlThatCausesReload');
501+
502+
browser.url('http://server/someOtherUrl');
503+
expect(browser.url()).toBe('http://server/someOtherUrl');
504+
expect(fakeWindow.location.href).toBe('http://server/someOtherUrl');
505+
});
506+
507+
it('assumes that changes to location.hash occur in sync', function() {
508+
// This is an asynchronous integration test that changes the
509+
// hash in all possible ways and checks
510+
// - whether the change to the hash can be read out in sync
511+
// - whether the change to the hash can be read out in the hashchange event
512+
var realWin = window,
513+
$realWin = jqLite(realWin),
514+
hashInHashChangeEvent = [];
515+
516+
runs(function() {
517+
$realWin.on('hashchange', hashListener);
518+
519+
realWin.location.hash = '1';
520+
realWin.location.href += '2';
521+
realWin.location.replace(realWin.location.href + '3');
522+
realWin.location.assign(realWin.location.href + '4');
523+
524+
expect(realWin.location.hash).toBe('#1234');
525+
});
526+
waitsFor(function() {
527+
return hashInHashChangeEvent.length > 3;
528+
});
529+
runs(function() {
530+
$realWin.off('hashchange', hashListener);
531+
532+
forEach(hashInHashChangeEvent, function(hash) {
533+
expect(hash).toBe('#1234');
534+
});
535+
});
536+
537+
function hashListener() {
538+
hashInHashChangeEvent.push(realWin.location.hash);
539+
}
540+
});
541+
493542
});
494543

495544
describe('urlChange', function() {
@@ -568,15 +617,15 @@ describe('browser', function() {
568617
beforeEach(function() {
569618
sniffer.history = false;
570619
sniffer.hashchange = false;
571-
browser.url("http://server.current");
620+
browser.url("http://server/#current");
572621
});
573622

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

577-
fakeWindow.location.href = 'http://server.new';
626+
fakeWindow.location.href = 'http://server/#new';
578627
fakeWindow.setTimeout.flush();
579-
expect(callback).toHaveBeenCalledWith('http://server.new');
628+
expect(callback).toHaveBeenCalledWith('http://server/#new');
580629

581630
fakeWindow.fire('popstate');
582631
fakeWindow.fire('hashchange');
@@ -640,33 +689,134 @@ describe('browser', function() {
640689

641690
describe('integration tests with $location', function() {
642691

643-
beforeEach(module(function($provide, $locationProvider) {
644-
spyOn(fakeWindow.history, 'pushState').andCallFake(function(stateObj, title, newUrl) {
645-
fakeWindow.location.href = newUrl;
692+
function setup(options) {
693+
module(function($provide, $locationProvider) {
694+
spyOn(fakeWindow.history, 'pushState').andCallFake(function(stateObj, title, newUrl) {
695+
fakeWindow.location.href = newUrl;
696+
});
697+
spyOn(fakeWindow.location, 'replace').andCallFake(function(newUrl) {
698+
fakeWindow.location.href = newUrl;
699+
});
700+
$provide.value('$browser', browser);
701+
browser.pollFns = [];
702+
703+
sniffer.history = options.history;
704+
$provide.value('$sniffer', sniffer);
705+
706+
$locationProvider.html5Mode(options.html5Mode);
646707
});
647-
$provide.value('$browser', browser);
648-
browser.pollFns = [];
708+
}
649709

650-
sniffer.history = true;
651-
$provide.value('$sniffer', sniffer);
710+
describe('update $location when it was changed outside of Angular in sync '+
711+
'before $digest was called', function() {
652712

653-
$locationProvider.html5Mode(true);
654-
}));
713+
it('should work with no history support, no html5Mode', function() {
714+
setup({
715+
history: false,
716+
html5Mode: false
717+
});
718+
inject(function($rootScope, $location) {
719+
$rootScope.$apply(function() {
720+
$location.path('/initialPath');
721+
});
722+
expect(fakeWindow.location.href).toBe('http://server/#/initialPath');
655723

656-
it('should update $location when it was changed outside of Angular in sync '+
657-
'before $digest was called', function() {
658-
inject(function($rootScope, $location) {
659-
fakeWindow.history.pushState(null, '', 'http://server/someTestHash');
724+
fakeWindow.location.href = 'http://server/#/someTestHash';
660725

661-
// Verify that infinite digest reported in #6976 no longer occurs
662-
expect(function() {
663726
$rootScope.$digest();
664-
}).not.toThrow();
665727

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

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

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

0 commit comments

Comments
 (0)