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

Commit

Permalink
fix($browser): handle async updates to location
Browse files Browse the repository at this point in the history
Both browser reloads and iOS 9 bugs cause the window.location to report
a different href that which we have just set. The change does not become
available until the next tick.

This change generalises previous work to deal with reloads to deal with
the iOS 9 bug in the UIWebView component.

Closes #12241
Closes #12819
  • Loading branch information
petebacondarwin committed Sep 14, 2015
1 parent 472d076 commit 8d39bd8
Show file tree
Hide file tree
Showing 2 changed files with 59 additions and 12 deletions.
17 changes: 11 additions & 6 deletions src/ng/browser.js
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ function Browser(window, document, $log, $sniffer) {
var cachedState, lastHistoryState,
lastBrowserUrl = location.href,
baseElement = document.find('base'),
reloadLocation = null;
pendingLocation = null;

cacheState();
lastHistoryState = cachedState;
Expand Down Expand Up @@ -147,8 +147,8 @@ function Browser(window, document, $log, $sniffer) {
// Do the assignment again so that those two variables are referentially identical.
lastHistoryState = cachedState;
} else {
if (!sameBase || reloadLocation) {
reloadLocation = url;
if (!sameBase || pendingLocation) {
pendingLocation = url;
}
if (replace) {
location.replace(url);
Expand All @@ -157,14 +157,18 @@ function Browser(window, document, $log, $sniffer) {
} else {
location.hash = getHash(url);
}
if (location.href !== url) {
pendingLocation = url;
}
}
return self;
// getter
} else {
// - reloadLocation is needed as browsers don't allow to read out
// the new location.href if a reload happened.
// - pendingLocation is needed as browsers don't allow to read out
// the new location.href if a reload happened or if there is a bug like in iOS 9 (see
// https://openradar.appspot.com/22186109).
// - the replacement is a workaround for https://bugzilla.mozilla.org/show_bug.cgi?id=407172
return reloadLocation || location.href.replace(/%27/g,"'");
return pendingLocation || location.href.replace(/%27/g,"'");
}
};

Expand All @@ -186,6 +190,7 @@ function Browser(window, document, $log, $sniffer) {
urlChangeInit = false;

function cacheStateAndFireUrlChange() {
pendingLocation = null;
cacheState();
fireUrlChange();
}
Expand Down
54 changes: 48 additions & 6 deletions test/ng/browserSpecs.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,20 @@ function MockWindow(options) {
var events = {};
var timeouts = this.timeouts = [];
var locationHref = 'http://server/';
var committedHref = 'http://server/';
var mockWindow = this;
var msie = options.msie;
var ieState;

historyEntriesLength = 1;

function replaceHash(href, hash) {
// replace the hash with the new one (stripping off a leading hash if there is one)
// See hash setter spec: https://url.spec.whatwg.org/#urlutils-and-urlutilsreadonly-members
return stripHash(href) + '#' + hash.replace(/^#/,'');
}


this.setTimeout = function(fn) {
return timeouts.push(fn) - 1;
};
Expand Down Expand Up @@ -46,24 +54,28 @@ function MockWindow(options) {

this.location = {
get href() {
return locationHref;
return committedHref;
},
set href(value) {
locationHref = value;
mockWindow.history.state = null;
historyEntriesLength++;
if (!options.updateAsync) this.flushHref();
},
get hash() {
return getHash(locationHref);
return getHash(committedHref);
},
set hash(value) {
// replace the hash with the new one (stripping off a leading hash if there is one)
// See hash setter spec: https://url.spec.whatwg.org/#urlutils-and-urlutilsreadonly-members
locationHref = stripHash(locationHref) + '#' + value.replace(/^#/,'');
locationHref = replaceHash(locationHref, value);
if (!options.updateAsync) this.flushHref();
},
replace: function(url) {
locationHref = url;
mockWindow.history.state = null;
if (!options.updateAsync) this.flushHref();
},
flushHref: function() {
committedHref = locationHref;
}
};

Expand Down Expand Up @@ -132,7 +144,7 @@ describe('browser', function() {

logs = {log:[], warn:[], info:[], error:[]};

var fakeLog = {log: function() { logs.log.push(slice.call(arguments)); },
fakeLog = {log: function() { logs.log.push(slice.call(arguments)); },
warn: function() { logs.warn.push(slice.call(arguments)); },
info: function() { logs.info.push(slice.call(arguments)); },
error: function() { logs.error.push(slice.call(arguments)); }};
Expand Down Expand Up @@ -703,7 +715,11 @@ describe('browser', function() {
describe('integration tests with $location', function() {

function setup(options) {
fakeWindow = new MockWindow(options);
browser = new Browser(fakeWindow, fakeDocument, fakeLog, sniffer);

module(function($provide, $locationProvider) {

spyOn(fakeWindow.history, 'pushState').andCallFake(function(stateObj, title, newUrl) {
fakeWindow.location.href = newUrl;
});
Expand Down Expand Up @@ -827,6 +843,32 @@ describe('browser', function() {
});

});

// issue #12241
it('should not infinite digest if the browser does not synchronously update the location properties', function() {
setup({
history: true,
html5Mode: true,
updateAsync: true // Simulate a browser that doesn't update the href synchronously
});

inject(function($location, $rootScope) {

// Change the hash within Angular and check that we don't infinitely digest
$location.hash('newHash');
expect(function() { $rootScope.$digest(); }).not.toThrow();
expect($location.absUrl()).toEqual('http://server/#newHash');

// Now change the hash from outside Angular and check that $location updates correctly
fakeWindow.location.hash = '#otherHash';

// simulate next tick - since this browser doesn't update synchronously
fakeWindow.location.flushHref();
fakeWindow.fire('hashchange');

expect($location.absUrl()).toEqual('http://server/#otherHash');
});
});
});

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

3 comments on commit 8d39bd8

@mhartington
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@petebacondarwin In testing angular 1.4.6 in ionic, we're experiencing some flickers when changing tabs. I remember this being an issue when developing the iOS9 patch.

The patch from Igor does not have this issue.

@penfold
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm also seeing flickering issues using Angular 1.4.7 with an Ionic app on iOS 9.02.

[Android works fine.]

@petebacondarwin
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mhartington and @penfold - can you create an issue and describe exactly how the application is set up and what actions cause this flickering?

Please sign in to comment.