From 9845cee63eda3ad5024622c792c5e745b59ec5cb Mon Sep 17 00:00:00 2001 From: Peter Bacon Darwin Date: Tue, 11 Nov 2014 17:10:29 +0000 Subject: [PATCH 1/2] fix($browser): prevent infinite digests when clearing the hash of a url By using `location.hash` to update the current browser location when only the hash has changed, we prevent the browser from attempting to reload. Closes #9629 Closes #9635 Closes #10228 Closes #10308 --- src/ng/browser.js | 9 +++++++- test/ng/browserSpecs.js | 46 +++++++++++++++++++++++++++++++++++++---- 2 files changed, 50 insertions(+), 5 deletions(-) diff --git a/src/ng/browser.js b/src/ng/browser.js index e87e5e184e93..b149ee66f4f3 100644 --- a/src/ng/browser.js +++ b/src/ng/browser.js @@ -62,6 +62,11 @@ function Browser(window, document, $log, $sniffer) { } } + function getHash(url) { + var index = url.indexOf('#'); + return index === -1 ? '' : url.substr(index + 1); + } + /** * @private * Note: this method is used only by scenario runner @@ -173,8 +178,10 @@ function Browser(window, document, $log, $sniffer) { } if (replace) { location.replace(url); - } else { + } else if (!sameBase) { location.href = url; + } else { + location.hash = getHash(url); } } return self; diff --git a/test/ng/browserSpecs.js b/test/ng/browserSpecs.js index 62b0a80c5298..c506028638b5 100755 --- a/test/ng/browserSpecs.js +++ b/test/ng/browserSpecs.js @@ -1,8 +1,18 @@ 'use strict'; -function MockWindow() { +/* global getHash:true, stripHash:true */ + +var historyEntriesLength; +var sniffer = {}; + +function MockWindow(options) { + if (typeof options !== 'object') { + options = {}; + } var events = {}; var timeouts = this.timeouts = []; + var locationHref = 'http://server/'; + var mockWindow = this; this.setTimeout = function(fn) { return timeouts.push(fn) - 1; @@ -36,8 +46,24 @@ function MockWindow() { }; this.location = { - href: 'http://server/', - replace: noop + get href() { + return locationHref; + }, + set href(value) { + locationHref = value; + mockWindow.history.state = null; + historyEntriesLength++; + }, + get hash() { + return getHash(locationHref); + }, + set hash(value) { + locationHref = stripHash(locationHref) + '#' + value; + }, + replace: function(url) { + locationHref = url; + mockWindow.history.state = null; + } }; this.history = { @@ -451,6 +477,17 @@ describe('browser', function() { expect(locationReplace).not.toHaveBeenCalled(); }); + it("should retain the # character when the only change is clearing the hash fragment, to prevent page reload", function() { + sniffer.history = true; + + browser.url('http://server/#123'); + expect(fakeWindow.location.href).toEqual('http://server/#123'); + + browser.url('http://server/'); + expect(fakeWindow.location.href).toEqual('http://server/#'); + + }); + it('should use location.replace when history.replaceState not available', function() { sniffer.history = false; browser.url('http://new.org', true); @@ -462,6 +499,7 @@ describe('browser', function() { expect(fakeWindow.location.href).toEqual('http://server/'); }); + it('should use location.replace and not use replaceState when the url only changed in the hash fragment to please IE10/11', function() { sniffer.history = true; browser.url('http://server/#123', true); @@ -473,6 +511,7 @@ describe('browser', function() { expect(fakeWindow.location.href).toEqual('http://server/'); }); + it('should return $browser to allow chaining', function() { expect(browser.url('http://any.com')).toBe(browser); }); @@ -835,7 +874,6 @@ describe('browser', function() { expect(browser.url()).toBe(newUrl); $rootScope.$digest(); expect(browser.url()).toBe(newUrl); - expect(fakeWindow.location.href).toBe(current); }); }); From 97a91199adddfdcb86233b0eea7c7cfb66b4de48 Mon Sep 17 00:00:00 2001 From: Jeff Cross Date: Tue, 16 Dec 2014 23:05:12 -0800 Subject: [PATCH 2/2] test(browser): change mock location definition to use defineProperty The original fix for which this mock location logic was written fixes a bug in master which also exists in 1.2.x. Cherry-picking the fix to the 1.2.x branch was difficult because the mock location object used ES5 get/set syntax, which is not supported in IE8. This fix changes the implementation to work with IE8 and modern browsers. IE8's defineProperty only works on certain types of objects, such as DOM elements. So the mock location is a div element in this implementation. --- test/ng/browserSpecs.js | 38 ++++++++++++++++++++++++-------------- 1 file changed, 24 insertions(+), 14 deletions(-) diff --git a/test/ng/browserSpecs.js b/test/ng/browserSpecs.js index c506028638b5..b6200cb2c296 100755 --- a/test/ng/browserSpecs.js +++ b/test/ng/browserSpecs.js @@ -45,26 +45,37 @@ function MockWindow(options) { }); }; - this.location = { - get href() { - return locationHref; - }, - set href(value) { + //IE8 hack. defineProperty doesn't work with POJS, just with certain DOM elements + this.location = document.createElement('div'); + this.location.href = {}; + this.location.hash = {}; + this.location.replace = function(url) { + locationHref = url; + mockWindow.history.state = null; + }; + Object.defineProperty(this.location, 'href', { + enumerable: false, + configurable: true, + set: function(value) { locationHref = value; mockWindow.history.state = null; historyEntriesLength++; }, - get hash() { - return getHash(locationHref); - }, - set hash(value) { + get: function() { + return locationHref; + } + }); + + Object.defineProperty(this.location, 'hash', { + enumerable: false, + configurable: true, + set: function(value) { locationHref = stripHash(locationHref) + '#' + value; }, - replace: function(url) { - locationHref = url; - mockWindow.history.state = null; + get: function() { + return getHash(locationHref); } - }; + }); this.history = { replaceState: noop, @@ -115,7 +126,6 @@ describe('browser', function() { warn: function() { logs.warn.push(slice.call(arguments)); }, info: function() { logs.info.push(slice.call(arguments)); }, error: function() { logs.error.push(slice.call(arguments)); }}; - browser = new Browser(fakeWindow, fakeDocument, fakeLog, sniffer); });