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

Commit

Permalink
fix(*): detect external changes in history.state
Browse files Browse the repository at this point in the history
Previously, `$browser.$$checkUrlChange()` (which was run before each `$digest`)
would only detect an external change (i.e. not via `$location`) to the browser
URL. External changes to `history.state` would not be detected and propagated to
`$location`.

This would not be a problem if changes were followed by a `popstate` or
`hashchange` event (which would call `cacheStateAndFireUrlChange()`). But since
`history.pushState()/replaceState()` do not fire any events, calling these
methods manually would result in `$location` getting out-of-sync with the actual
history state.

This was not detected in tests, because the mocked `window.history` would
incorrectly trigger `popstate` when calling `pushState()/replaceState()`, which
"covered" the bug.

This commit fixes it by always calling `cacheState()`, before looking for and
propagating a URL/state change.
  • Loading branch information
gkalpak committed Jan 9, 2017
1 parent c00903b commit 2b360bf
Show file tree
Hide file tree
Showing 2 changed files with 22 additions and 13 deletions.
17 changes: 9 additions & 8 deletions src/ng/browser.js
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,6 @@ function Browser(window, document, $log, $sniffer) {
};

cacheState();
lastHistoryState = cachedState;

/**
* @name $browser#url
Expand Down Expand Up @@ -150,8 +149,6 @@ function Browser(window, document, $log, $sniffer) {
if ($sniffer.history && (!sameBase || !sameState)) {
history[replace ? 'replaceState' : 'pushState'](state, '', url);
cacheState();
// Do the assignment again so that those two variables are referentially identical.
lastHistoryState = cachedState;
} else {
if (!sameBase) {
pendingLocation = url;
Expand Down Expand Up @@ -200,8 +197,7 @@ function Browser(window, document, $log, $sniffer) {

function cacheStateAndFireUrlChange() {
pendingLocation = null;
cacheState();
fireUrlChange();
fireStateOrUrlChange();
}

// This variable should be used *only* inside the cacheState function.
Expand All @@ -215,11 +211,16 @@ function Browser(window, document, $log, $sniffer) {
if (equals(cachedState, lastCachedState)) {
cachedState = lastCachedState;
}

lastCachedState = cachedState;
lastHistoryState = cachedState;
}

function fireUrlChange() {
if (lastBrowserUrl === self.url() && lastHistoryState === cachedState) {
function fireStateOrUrlChange() {
var prevLastHistoryState = lastHistoryState;
cacheState();

if (lastBrowserUrl === self.url() && prevLastHistoryState === cachedState) {
return;
}

Expand Down Expand Up @@ -285,7 +286,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 = fireUrlChange;
self.$$checkUrlChange = fireStateOrUrlChange;

//////////////////////////////////////////////////////////////
// Misc API
Expand Down
18 changes: 13 additions & 5 deletions test/ng/locationSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -1039,11 +1039,21 @@ describe('$location', function() {
});

it('should update $location when browser state changes', function() {
initService({html5Mode:true, supportHistory: true});
mockUpBrowser({initialUrl:'http://new.com/a/b/', baseHref:'/a/b/'});
inject(function($location, $window) {
initService({html5Mode: true, supportHistory: true});
mockUpBrowser({initialUrl: 'http://new.com/a/b/', baseHref: '/a/b/'});
inject(function($location, $rootScope, $window) {
$window.history.pushState({b: 3});
$rootScope.$digest();

expect($location.state()).toEqual({b: 3});

$window.history.pushState({b: 4}, null, $window.location.href + 'c?d=e#f');
$rootScope.$digest();

expect($location.path()).toBe('/c');
expect($location.search()).toEqual({d: 'e'});
expect($location.hash()).toBe('f');
expect($location.state()).toEqual({b: 4});
});
});

Expand Down Expand Up @@ -2666,12 +2676,10 @@ describe('$location', function() {
replaceState: function(state, title, url) {
win.history.state = copy(state);
if (url) win.location.href = url;
jqLite(win).triggerHandler('popstate');
},
pushState: function(state, title, url) {
win.history.state = copy(state);
if (url) win.location.href = url;
jqLite(win).triggerHandler('popstate');
}
};
win.addEventListener = angular.noop;
Expand Down

0 comments on commit 2b360bf

Please sign in to comment.