From 5e5c724590623cbecdd03396f77705ccf3700b57 Mon Sep 17 00:00:00 2001 From: Igor Minar Date: Thu, 22 Nov 2012 23:18:31 +0100 Subject: [PATCH 1/6] refactor(ngRepeat): add tests and simplify code This commits adds test for #933 and simplifies the implementation of the fix Closes #933 --- src/apis.js | 4 +- src/ng/directive/ngRepeat.js | 34 +++++----------- test/ng/directive/ngRepeatSpec.js | 64 ++++++++++++++++++------------- 3 files changed, 49 insertions(+), 53 deletions(-) diff --git a/src/apis.js b/src/apis.js index 098d9cdd6be8..0e94e2a55ce2 100644 --- a/src/apis.js +++ b/src/apis.js @@ -98,12 +98,12 @@ HashQueueMap.prototype = { } } }, - + /** * return the first item without deleting it */ peek: function(key) { - var array = this[key = hashKey(key)]; + var array = this[hashKey(key)]; if (array) { return array[0]; } diff --git a/src/ng/directive/ngRepeat.js b/src/ng/directive/ngRepeat.js index 021845fae0ad..8c934b767210 100644 --- a/src/ng/directive/ngRepeat.js +++ b/src/ng/directive/ngRepeat.js @@ -88,7 +88,7 @@ var ngRepeatDirective = ngDirective({ // We need an array of these objects since the same object can be returned from the iterator. // We expect this to be a rare case. var lastOrder = new HashQueueMap(); - var indexValues = []; + scope.$watch(function ngRepeatWatch(scope){ var index, length, collection = scope.$eval(rhs), @@ -119,18 +119,14 @@ var ngRepeatDirective = ngDirective({ key = (collection === array) ? index : array[index]; value = collection[key]; - // if collection is array and value is object, it can be shifted to allow for position change - // if collection is array and value is not object, need to first check whether index is same to - // avoid shifting wrong value - // if collection is not array, need to always check index to avoid shifting wrong value - if (lastOrder.peek(value)) { - last = collection === array ? - ((isObject(value)) ? lastOrder.shift(value) : - (index === lastOrder.peek(value).index ? lastOrder.shift(value) : undefined)) : - (index === lastOrder.peek(value).index ? lastOrder.shift(value) : undefined); - } else { - last = undefined; - } + // if value is object, it can be shifted to allow for position change + // if is not object, need to first check whether index is same to avoid shifting wrong val + last = isObject(value) + ? lastOrder.shift(value) + : (last = lastOrder.peek(value)) && (index === last.index) + ? lastOrder.shift(value) + : undefined; + if (last) { // if we have already seen this object, then we need to reuse the @@ -151,12 +147,6 @@ var ngRepeatDirective = ngDirective({ cursor = last.element; } } else { - if (indexValues.hasOwnProperty(index) && collection !== array) { - var preValue = indexValues[index]; - var v = lastOrder.shift(preValue); - v.element.remove(); - v.scope.$destroy(); - } // new item which we don't know about childScope = scope.$new(); } @@ -178,16 +168,10 @@ var ngRepeatDirective = ngDirective({ index: index }; nextOrder.push(value, last); - indexValues[index] = value; }); } } - var i, l; - for (i = 0, l = indexValues.length - length; i < l; i++) { - indexValues.pop(); - } - //shrink children for (key in lastOrder) { if (lastOrder.hasOwnProperty(key)) { diff --git a/test/ng/directive/ngRepeatSpec.js b/test/ng/directive/ngRepeatSpec.js index 83f23cec370d..3a2e2bb35969 100644 --- a/test/ng/directive/ngRepeatSpec.js +++ b/test/ng/directive/ngRepeatSpec.js @@ -37,10 +37,10 @@ describe('ngRepeat', function() { })); - it('should ngRepeat over array of primitive correctly', inject(function($rootScope, $compile) { + it('should ngRepeat over array of primitives', inject(function($rootScope, $compile) { element = $compile( '')($rootScope); Array.prototype.extraProperty = "should be ignored"; @@ -91,13 +91,13 @@ describe('ngRepeat', function() { $rootScope.$digest(); expect(element.find('li').length).toEqual(1); expect(element.text()).toEqual('test;'); - + $rootScope.items = ['same', 'value']; $rootScope.$digest(); expect(element.find('li').length).toEqual(2); expect(element.text()).toEqual('same;value;'); - - // number + + // number $rootScope.items = [12, 12, 12]; $rootScope.$digest(); expect(element.find('li').length).toEqual(3); @@ -130,36 +130,48 @@ describe('ngRepeat', function() { expect(element.text()).toEqual('misko:swe;shyam:set;'); })); - - it('should ngRepeat over object with primitive value correctly', inject(function($rootScope, $compile) { + + it('should ngRepeat over object with changing primitive value', + inject(function($rootScope, $compile) { + element = $compile( '')($rootScope); - $rootScope.items = {misko:'true', shyam:'true', zhenbo: 'true'}; + //document.body.appendChild(element[0]); + $rootScope.items = {misko: true, shyam: true, zhenbo:true}; $rootScope.$digest(); expect(element.find('li').length).toEqual(3); expect(element.text()).toEqual('misko:true;shyam:true;zhenbo:true;'); - - $rootScope.items = {misko:'false', shyam:'true', zhenbo: 'true'}; - $rootScope.$digest(); - expect(element.find('li').length).toEqual(3); + + browserTrigger(element.find('input').eq(0), 'click'); + expect(element.text()).toEqual('misko:false;shyam:true;zhenbo:true;'); - - $rootScope.items = {misko:'false', shyam:'false', zhenbo: 'false'}; - $rootScope.$digest(); - expect(element.find('li').length).toEqual(3); - expect(element.text()).toEqual('misko:false;shyam:false;zhenbo:false;'); - - $rootScope.items = {misko:'true'}; - $rootScope.$digest(); - expect(element.find('li').length).toEqual(1); - expect(element.text()).toEqual('misko:true;'); + expect(element.find('input')[0].checked).toBe(false); + expect(element.find('input')[1].checked).toBe(true); + expect(element.find('input')[2].checked).toBe(true); - $rootScope.items = {shyam:'true', zhenbo: 'false'}; + browserTrigger(element.find('input').eq(0), 'click'); + expect(element.text()).toEqual('misko:true;shyam:true;zhenbo:true;'); + expect(element.find('input')[0].checked).toBe(true); + expect(element.find('input')[1].checked).toBe(true); + expect(element.find('input')[2].checked).toBe(true); + + browserTrigger(element.find('input').eq(1), 'click'); + expect(element.text()).toEqual('misko:true;shyam:false;zhenbo:true;'); + expect(element.find('input')[0].checked).toBe(true); + expect(element.find('input')[1].checked).toBe(false); + expect(element.find('input')[2].checked).toBe(true); + + $rootScope.items = {misko: false, shyam: true, zhenbo: true}; $rootScope.$digest(); - expect(element.find('li').length).toEqual(2); - expect(element.text()).toEqual('shyam:true;zhenbo:false;'); + expect(element.text()).toEqual('misko:false;shyam:true;zhenbo:true;'); + expect(element.find('input')[0].checked).toBe(false); + expect(element.find('input')[1].checked).toBe(true); + expect(element.find('input')[2].checked).toBe(true); })); From f24a50adc7d1ad243432a9364c4a47af98feac43 Mon Sep 17 00:00:00 2001 From: Igor Minar Date: Fri, 23 Nov 2012 16:08:47 +0100 Subject: [PATCH 2/6] test(ngRepeat): clean up and improve tests --- test/ng/directive/ngRepeatSpec.js | 347 +++++++++++++++--------------- 1 file changed, 175 insertions(+), 172 deletions(-) diff --git a/test/ng/directive/ngRepeatSpec.js b/test/ng/directive/ngRepeatSpec.js index 3a2e2bb35969..0db701af791f 100644 --- a/test/ng/directive/ngRepeatSpec.js +++ b/test/ng/directive/ngRepeatSpec.js @@ -1,7 +1,13 @@ 'use strict'; describe('ngRepeat', function() { - var element; + var element, $compile, scope; + + + beforeEach(inject(function(_$compile_, $rootScope) { + $compile = _$compile_; + scope = $rootScope.$new(); + })); afterEach(function(){ @@ -9,130 +15,131 @@ describe('ngRepeat', function() { }); - it('should ngRepeat over array', inject(function($rootScope, $compile) { + it('should iterate over an array of objects', function() { element = $compile( '')($rootScope); + '
  • {{item.name}};
    • ' + + '')(scope); Array.prototype.extraProperty = "should be ignored"; // INIT - $rootScope.items = ['misko', 'shyam']; - $rootScope.$digest(); + scope.items = [{name: 'misko'}, {name:'shyam'}]; + scope.$digest(); expect(element.find('li').length).toEqual(2); expect(element.text()).toEqual('misko;shyam;'); delete Array.prototype.extraProperty; // GROW - $rootScope.items = ['adam', 'kai', 'brad']; - $rootScope.$digest(); + scope.items.push({name: 'adam'}); + scope.$digest(); expect(element.find('li').length).toEqual(3); - expect(element.text()).toEqual('adam;kai;brad;'); + expect(element.text()).toEqual('misko;shyam;adam;'); // SHRINK - $rootScope.items = ['brad']; - $rootScope.$digest(); + scope.items.pop(); + scope.items.shift(); + scope.$digest(); expect(element.find('li').length).toEqual(1); - expect(element.text()).toEqual('brad;'); - })); + expect(element.text()).toEqual('shyam;'); + }); - it('should ngRepeat over array of primitives', inject(function($rootScope, $compile) { + it('should iterate over an array of primitives', function() { element = $compile( '')($rootScope); + '')(scope); Array.prototype.extraProperty = "should be ignored"; // INIT - $rootScope.items = [true, true, true]; - $rootScope.$digest(); + scope.items = [true, true, true]; + scope.$digest(); expect(element.find('li').length).toEqual(3); expect(element.text()).toEqual('true;true;true;'); delete Array.prototype.extraProperty; - $rootScope.items = [false, true, true]; - $rootScope.$digest(); + scope.items = [false, true, true]; + scope.$digest(); expect(element.find('li').length).toEqual(3); expect(element.text()).toEqual('false;true;true;'); - $rootScope.items = [false, true, false]; - $rootScope.$digest(); + scope.items = [false, true, false]; + scope.$digest(); expect(element.find('li').length).toEqual(3); expect(element.text()).toEqual('false;true;false;'); - $rootScope.items = [true]; - $rootScope.$digest(); + scope.items = [true]; + scope.$digest(); expect(element.find('li').length).toEqual(1); expect(element.text()).toEqual('true;'); - $rootScope.items = [true, true, false]; - $rootScope.$digest(); + scope.items = [true, true, false]; + scope.$digest(); expect(element.find('li').length).toEqual(3); expect(element.text()).toEqual('true;true;false;'); - $rootScope.items = [true, false, false]; - $rootScope.$digest(); + scope.items = [true, false, false]; + scope.$digest(); expect(element.find('li').length).toEqual(3); expect(element.text()).toEqual('true;false;false;'); // string - $rootScope.items = ['a', 'a', 'a']; - $rootScope.$digest(); + scope.items = ['a', 'a', 'a']; + scope.$digest(); expect(element.find('li').length).toEqual(3); expect(element.text()).toEqual('a;a;a;'); - $rootScope.items = ['ab', 'a', 'a']; - $rootScope.$digest(); + scope.items = ['ab', 'a', 'a']; + scope.$digest(); expect(element.find('li').length).toEqual(3); expect(element.text()).toEqual('ab;a;a;'); - $rootScope.items = ['test']; - $rootScope.$digest(); + scope.items = ['test']; + scope.$digest(); expect(element.find('li').length).toEqual(1); expect(element.text()).toEqual('test;'); - $rootScope.items = ['same', 'value']; - $rootScope.$digest(); + scope.items = ['same', 'value']; + scope.$digest(); expect(element.find('li').length).toEqual(2); expect(element.text()).toEqual('same;value;'); // number - $rootScope.items = [12, 12, 12]; - $rootScope.$digest(); + scope.items = [12, 12, 12]; + scope.$digest(); expect(element.find('li').length).toEqual(3); expect(element.text()).toEqual('12;12;12;'); - $rootScope.items = [53, 12, 27]; - $rootScope.$digest(); + scope.items = [53, 12, 27]; + scope.$digest(); expect(element.find('li').length).toEqual(3); expect(element.text()).toEqual('53;12;27;'); - $rootScope.items = [89]; - $rootScope.$digest(); + scope.items = [89]; + scope.$digest(); expect(element.find('li').length).toEqual(1); expect(element.text()).toEqual('89;'); - $rootScope.items = [89, 23]; - $rootScope.$digest(); + scope.items = [89, 23]; + scope.$digest(); expect(element.find('li').length).toEqual(2); expect(element.text()).toEqual('89;23;'); - })); + }); - it('should ngRepeat over object', inject(function($rootScope, $compile) { + it('should iterate over on object/map', function() { element = $compile( '')($rootScope); - $rootScope.items = {misko:'swe', shyam:'set'}; - $rootScope.$digest(); - expect(element.text()).toEqual('misko:swe;shyam:set;'); - })); + '
  • {{key}}:{{value}}|
    • ' + + '')(scope); + scope.items = {misko:'swe', shyam:'set'}; + scope.$digest(); + expect(element.text()).toEqual('misko:swe|shyam:set|'); + }); - it('should ngRepeat over object with changing primitive value', - inject(function($rootScope, $compile) { + it('should iterate over object with changing primitive property values', function() { + // test for issue #933 element = $compile( '')($rootScope); + '')(scope); //document.body.appendChild(element[0]); - $rootScope.items = {misko: true, shyam: true, zhenbo:true}; - $rootScope.$digest(); + scope.items = {misko: true, shyam: true, zhenbo:true}; + scope.$digest(); expect(element.find('li').length).toEqual(3); expect(element.text()).toEqual('misko:true;shyam:true;zhenbo:true;'); @@ -166,216 +173,214 @@ describe('ngRepeat', function() { expect(element.find('input')[1].checked).toBe(false); expect(element.find('input')[2].checked).toBe(true); - $rootScope.items = {misko: false, shyam: true, zhenbo: true}; - $rootScope.$digest(); + scope.items = {misko: false, shyam: true, zhenbo: true}; + scope.$digest(); expect(element.text()).toEqual('misko:false;shyam:true;zhenbo:true;'); expect(element.find('input')[0].checked).toBe(false); expect(element.find('input')[1].checked).toBe(true); expect(element.find('input')[2].checked).toBe(true); - })); + }); - it('should not ngRepeat over parent properties', inject(function($rootScope, $compile) { + it('should not ngRepeat over parent properties', function() { var Class = function() {}; Class.prototype.abc = function() {}; Class.prototype.value = 'abc'; element = $compile( '')($rootScope); - $rootScope.items = new Class(); - $rootScope.items.name = 'value'; - $rootScope.$digest(); + '
  • {{key}}:{{value}};
    • ' + + '')(scope); + scope.items = new Class(); + scope.items.name = 'value'; + scope.$digest(); expect(element.text()).toEqual('name:value;'); - })); + }); - it('should error on wrong parsing of ngRepeat', inject(function($rootScope, $compile) { + it('should error on wrong parsing of ngRepeat', function() { expect(function() { - element = $compile('')($rootScope); + element = jqLite(''); + $compile(element)(scope); }).toThrow("Expected ngRepeat in form of '_item_ in _collection_' but got 'i dont parse'."); - })); + }); - it("should throw error when left-hand-side of ngRepeat can't be parsed", inject( - function($rootScope, $compile) { + it("should throw error when left-hand-side of ngRepeat can't be parsed", function() { expect(function() { - element = $compile('')($rootScope); + element = jqLite(''); + $compile(element)(scope); }).toThrow("'item' in 'item in collection' should be identifier or (key, value) but got " + "'i dont parse'."); - })); + }); it('should expose iterator offset as $index when iterating over arrays', - inject(function($rootScope, $compile) { + function() { element = $compile( '')($rootScope); - $rootScope.items = ['misko', 'shyam', 'frodo']; - $rootScope.$digest(); - expect(element.text()).toEqual('misko0|shyam1|frodo2|'); - })); + '
  • {{item}}:{{$index}}|
    • ' + + '')(scope); + scope.items = ['misko', 'shyam', 'frodo']; + scope.$digest(); + expect(element.text()).toEqual('misko:0|shyam:1|frodo:2|'); + }); - it('should expose iterator offset as $index when iterating over objects', - inject(function($rootScope, $compile) { + it('should expose iterator offset as $index when iterating over objects', function() { element = $compile( '')($rootScope); - $rootScope.items = {'misko':'m', 'shyam':'s', 'frodo':'f'}; - $rootScope.$digest(); - expect(element.text()).toEqual('frodo:f0|misko:m1|shyam:s2|'); - })); + '
  • {{key}}:{{val}}:{{$index}}|
    • ' + + '')(scope); + scope.items = {'misko':'m', 'shyam':'s', 'frodo':'f'}; + scope.$digest(); + expect(element.text()).toEqual('frodo:f:0|misko:m:1|shyam:s:2|'); + }); it('should expose iterator position as $first, $middle and $last when iterating over arrays', - inject(function($rootScope, $compile) { + function() { element = $compile( '')($rootScope); - $rootScope.items = ['misko', 'shyam', 'doug']; - $rootScope.$digest(); + '')(scope); + scope.items = ['misko', 'shyam', 'doug']; + scope.$digest(); expect(element.text()). toEqual('misko:true-false-false|shyam:false-true-false|doug:false-false-true|'); - $rootScope.items.push('frodo'); - $rootScope.$digest(); + scope.items.push('frodo'); + scope.$digest(); expect(element.text()). toEqual('misko:true-false-false|' + 'shyam:false-true-false|' + 'doug:false-true-false|' + 'frodo:false-false-true|'); - $rootScope.items.pop(); - $rootScope.items.pop(); - $rootScope.$digest(); + scope.items.pop(); + scope.items.pop(); + scope.$digest(); expect(element.text()).toEqual('misko:true-false-false|shyam:false-false-true|'); - $rootScope.items.pop(); - $rootScope.$digest(); + scope.items.pop(); + scope.$digest(); expect(element.text()).toEqual('misko:true-false-true|'); - })); + }); it('should expose iterator position as $first, $middle and $last when iterating over objects', - inject(function($rootScope, $compile) { + function() { element = $compile( '')($rootScope); - $rootScope.items = {'misko':'m', 'shyam':'s', 'doug':'d', 'frodo':'f'}; - $rootScope.$digest(); + '')(scope); + scope.items = {'misko':'m', 'shyam':'s', 'doug':'d', 'frodo':'f'}; + scope.$digest(); expect(element.text()). toEqual('doug:d:true-false-false|' + 'frodo:f:false-true-false|' + 'misko:m:false-true-false|' + 'shyam:s:false-false-true|'); - delete $rootScope.items.doug; - delete $rootScope.items.frodo; - $rootScope.$digest(); + delete scope.items.doug; + delete scope.items.frodo; + scope.$digest(); expect(element.text()).toEqual('misko:m:true-false-false|shyam:s:false-false-true|'); - delete $rootScope.items.shyam; - $rootScope.$digest(); + delete scope.items.shyam; + scope.$digest(); expect(element.text()).toEqual('misko:m:true-false-true|'); - })); + }); - it('should ignore $ and $$ properties', inject(function($rootScope, $compile) { - element = $compile('')($rootScope); - $rootScope.items = ['a', 'b', 'c']; - $rootScope.items.$$hashkey = 'xxx'; - $rootScope.items.$root = 'yyy'; - $rootScope.$digest(); + it('should ignore $ and $$ properties', function() { + element = $compile('')(scope); + scope.items = ['a', 'b', 'c']; + scope.items.$$hashkey = 'xxx'; + scope.items.$root = 'yyy'; + scope.$digest(); expect(element.text()).toEqual('a|b|c|'); - })); + }); - it('should repeat over nested arrays', inject(function($rootScope, $compile) { + it('should repeat over nested arrays', function() { element = $compile( '')($rootScope); - $rootScope.groups = [['a', 'b'], ['c','d']]; - $rootScope.$digest(); + '')(scope); + scope.groups = [['a', 'b'], ['c','d']]; + scope.$digest(); expect(element.text()).toEqual('a|b|Xc|d|X'); - })); + }); - it('should ignore non-array element properties when iterating over an array', - inject(function($rootScope, $compile) { - element = $compile('')($rootScope); - $rootScope.array = ['a', 'b', 'c']; - $rootScope.array.foo = '23'; - $rootScope.array.bar = function() {}; - $rootScope.$digest(); + it('should ignore non-array element properties when iterating over an array', function() { + element = $compile('')(scope); + scope.array = ['a', 'b', 'c']; + scope.array.foo = '23'; + scope.array.bar = function() {}; + scope.$digest(); expect(element.text()).toBe('a|b|c|'); - })); + }); - it('should iterate over non-existent elements of a sparse array', - inject(function($rootScope, $compile) { - element = $compile('')($rootScope); - $rootScope.array = ['a', 'b']; - $rootScope.array[4] = 'c'; - $rootScope.array[6] = 'd'; - $rootScope.$digest(); + it('should iterate over non-existent elements of a sparse array', function() { + element = $compile('')(scope); + scope.array = ['a', 'b']; + scope.array[4] = 'c'; + scope.array[6] = 'd'; + scope.$digest(); expect(element.text()).toBe('a|b|||c||d|'); - })); + }); - it('should iterate over all kinds of types', inject(function($rootScope, $compile) { - element = $compile('')($rootScope); - $rootScope.array = ['a', 1, null, undefined, {}]; - $rootScope.$digest(); + it('should iterate over all kinds of types', function() { + element = $compile('')(scope); + scope.array = ['a', 1, null, undefined, {}]; + scope.$digest(); expect(element.text()).toMatch(/a\|1\|\|\|\{\s*\}\|/); - })); + }); describe('stability', function() { var a, b, c, d, lis; - beforeEach(inject(function($rootScope, $compile) { + beforeEach(function() { element = $compile( '')($rootScope); + '')(scope); a = {}; b = {}; c = {}; d = {}; - $rootScope.items = [a, b, c]; - $rootScope.$digest(); + scope.items = [a, b, c]; + scope.$digest(); lis = element.find('li'); - })); + }); - it('should preserve the order of elements', inject(function($rootScope) { - $rootScope.items = [a, c, d]; - $rootScope.$digest(); + it('should preserve the order of elements', function() { + scope.items = [a, c, d]; + scope.$digest(); var newElements = element.find('li'); expect(newElements[0]).toEqual(lis[0]); expect(newElements[1]).toEqual(lis[2]); expect(newElements[2]).not.toEqual(lis[1]); - })); + }); - it('should support duplicates', inject(function($rootScope) { - $rootScope.items = [a, a, b, c]; - $rootScope.$digest(); + it('should support duplicates', function() { + scope.items = [a, a, b, c]; + scope.$digest(); var newElements = element.find('li'); expect(newElements[0]).toEqual(lis[0]); expect(newElements[1]).not.toEqual(lis[0]); @@ -383,50 +388,48 @@ describe('ngRepeat', function() { expect(newElements[3]).toEqual(lis[2]); lis = newElements; - $rootScope.$digest(); + scope.$digest(); newElements = element.find('li'); expect(newElements[0]).toEqual(lis[0]); expect(newElements[1]).toEqual(lis[1]); expect(newElements[2]).toEqual(lis[2]); expect(newElements[3]).toEqual(lis[3]); - $rootScope.$digest(); + scope.$digest(); newElements = element.find('li'); expect(newElements[0]).toEqual(lis[0]); expect(newElements[1]).toEqual(lis[1]); expect(newElements[2]).toEqual(lis[2]); expect(newElements[3]).toEqual(lis[3]); - })); + }); - it('should remove last item when one duplicate instance is removed', - inject(function($rootScope) { - $rootScope.items = [a, a, a]; - $rootScope.$digest(); + it('should remove last item when one duplicate instance is removed', function() { + scope.items = [a, a, a]; + scope.$digest(); lis = element.find('li'); - $rootScope.items = [a, a]; - $rootScope.$digest(); + scope.items = [a, a]; + scope.$digest(); var newElements = element.find('li'); expect(newElements.length).toEqual(2); expect(newElements[0]).toEqual(lis[0]); expect(newElements[1]).toEqual(lis[1]); - })); + }); - it('should reverse items when the collection is reversed', - inject(function($rootScope) { - $rootScope.items = [a, b, c]; - $rootScope.$digest(); + it('should reverse items when the collection is reversed', function() { + scope.items = [a, b, c]; + scope.$digest(); lis = element.find('li'); - $rootScope.items = [c, b, a]; - $rootScope.$digest(); + scope.items = [c, b, a]; + scope.$digest(); var newElements = element.find('li'); expect(newElements.length).toEqual(3); expect(newElements[0]).toEqual(lis[2]); expect(newElements[1]).toEqual(lis[1]); expect(newElements[2]).toEqual(lis[0]); - })); + }); }); }); From ab1f1c85f8519fd96bbb308d0d72c405c60ed8e6 Mon Sep 17 00:00:00 2001 From: Igor Minar Date: Fri, 23 Nov 2012 22:40:01 +0100 Subject: [PATCH 3/6] fix(ngModel): sync ngModel state with scope state In cases when we reuse elements in a repeater but associate them with a new scope (see #933 - repeating over array of primitives) it's possible for the internal ngModel state and the scope state to get out of sync. This change ensure that the two are always sync-ed up even in cases where we reassociate an element with a different (but similar) scope. In the case of repeating over array of primitives it's still possible to run into issue if we iterate over primitives and use form controls or similar widgets without ngModel - oh well, we'd likely need a special repeater for primitives to deal with this properly, even then there might be cornercases. Closes #933 --- src/ng/directive/input.js | 27 +++++++++++++++------------ 1 file changed, 15 insertions(+), 12 deletions(-) diff --git a/src/ng/directive/input.js b/src/ng/directive/input.js index 8e66d6a2450a..ce7300654054 100644 --- a/src/ng/directive/input.js +++ b/src/ng/directive/input.js @@ -1010,22 +1010,25 @@ var NgModelController = ['$scope', '$exceptionHandler', '$attrs', '$element', '$ // model -> value var ctrl = this; - $scope.$watch(ngModelGet, function ngModelWatchAction(value) { - // ignore change from view - if (ctrl.$modelValue === value) return; + $scope.$watch(function ngModelWatch() { + var value = ngModelGet($scope); - var formatters = ctrl.$formatters, - idx = formatters.length; + // if scope model value and ngModel value are out of sync + if (ctrl.$modelValue !== value) { - ctrl.$modelValue = value; - while(idx--) { - value = formatters[idx](value); - } + var formatters = ctrl.$formatters, + idx = formatters.length; - if (ctrl.$viewValue !== value) { - ctrl.$viewValue = value; - ctrl.$render(); + ctrl.$modelValue = value; + while(idx--) { + value = formatters[idx](value); + } + + if (ctrl.$viewValue !== value) { + ctrl.$viewValue = value; + ctrl.$render(); + } } }); }]; From 2f34fe89c79306a5c5fa09356ccd34823f3ff997 Mon Sep 17 00:00:00 2001 From: Igor Minar Date: Fri, 23 Nov 2012 22:43:30 +0100 Subject: [PATCH 4/6] fix(ngRepeat): support mostly-stable repeating for primitives I'm reverting changes that were originally done to ngRepeat to fix #933, because these are now not necessary after the previous changes to keep ngModel always synced with the DOM. --- src/ng/directive/ngRepeat.js | 9 +-------- test/ng/directive/ngRepeatSpec.js | 18 ++++++++++++++++++ 2 files changed, 19 insertions(+), 8 deletions(-) diff --git a/src/ng/directive/ngRepeat.js b/src/ng/directive/ngRepeat.js index 8c934b767210..893ad442e1b4 100644 --- a/src/ng/directive/ngRepeat.js +++ b/src/ng/directive/ngRepeat.js @@ -119,14 +119,7 @@ var ngRepeatDirective = ngDirective({ key = (collection === array) ? index : array[index]; value = collection[key]; - // if value is object, it can be shifted to allow for position change - // if is not object, need to first check whether index is same to avoid shifting wrong val - last = isObject(value) - ? lastOrder.shift(value) - : (last = lastOrder.peek(value)) && (index === last.index) - ? lastOrder.shift(value) - : undefined; - + last = lastOrder.shift(value); if (last) { // if we have already seen this object, then we need to reuse the diff --git a/test/ng/directive/ngRepeatSpec.js b/test/ng/directive/ngRepeatSpec.js index 0db701af791f..6485387427d0 100644 --- a/test/ng/directive/ngRepeatSpec.js +++ b/test/ng/directive/ngRepeatSpec.js @@ -431,5 +431,23 @@ describe('ngRepeat', function() { expect(newElements[1]).toEqual(lis[1]); expect(newElements[2]).toEqual(lis[0]); }); + + + it('should reuse elements even when model is composed of primitives', function() { + // rebuilding repeater from scratch can be expensive, we should try to avoid it even for + // model that is composed of primitives. + + scope.items = ['hello', 'cau', 'ahoj']; + scope.$digest(); + lis = element.find('li'); + + scope.items = ['ahoj', 'hello', 'cau']; + scope.$digest(); + var newLis = element.find('li'); + expect(newLis.length).toEqual(3); + expect(newLis[0]).toEqual(lis[2]); + expect(newLis[1]).toEqual(lis[0]); + expect(newLis[2]).toEqual(lis[1]); + }); }); }); From 3f25e7d4fc85510b71cba65733a7ddf21303e44b Mon Sep 17 00:00:00 2001 From: Igor Minar Date: Fri, 23 Nov 2012 22:45:46 +0100 Subject: [PATCH 5/6] style(jqLite): better variable names selector => cssClasses --- src/jqLite.js | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/jqLite.js b/src/jqLite.js index 1ba270b671d3..864e842d69f4 100644 --- a/src/jqLite.js +++ b/src/jqLite.js @@ -266,9 +266,9 @@ function JQLiteHasClass(element, selector) { indexOf( " " + selector + " " ) > -1); } -function JQLiteRemoveClass(element, selector) { - if (selector) { - forEach(selector.split(' '), function(cssClass) { +function JQLiteRemoveClass(element, cssClasses) { + if (cssClasses) { + forEach(cssClasses.split(' '), function(cssClass) { element.className = trim( (" " + element.className + " ") .replace(/[\n\t]/g, " ") @@ -278,9 +278,9 @@ function JQLiteRemoveClass(element, selector) { } } -function JQLiteAddClass(element, selector) { - if (selector) { - forEach(selector.split(' '), function(cssClass) { +function JQLiteAddClass(element, cssClasses) { + if (cssClasses) { + forEach(cssClasses.split(' '), function(cssClass) { if (!JQLiteHasClass(element, cssClass)) { element.className = trim(element.className + ' ' + trim(cssClass)); } From a58210d8d2344a7ef06edef1fa9d60fc0552facd Mon Sep 17 00:00:00 2001 From: Igor Minar Date: Fri, 23 Nov 2012 22:46:58 +0100 Subject: [PATCH 6/6] fix(ngClassOdd/ngClassEven): support shrinking/reordering in repeaters We need to watch $index in addition to cssClasses because only then we can correctly respond to shrinking or reordered repeaters. Closes #1076 --- src/ng/directive/ngClass.js | 56 ++++++++++++++++++++++++++++--------- 1 file changed, 43 insertions(+), 13 deletions(-) diff --git a/src/ng/directive/ngClass.js b/src/ng/directive/ngClass.js index 13b0378a9f13..76b0ed570dda 100644 --- a/src/ng/directive/ngClass.js +++ b/src/ng/directive/ngClass.js @@ -3,25 +3,55 @@ function classDirective(name, selector) { name = 'ngClass' + name; return ngDirective(function(scope, element, attr) { - // Reusable function for re-applying the ngClass - function ngClassWatchAction(newVal, oldVal) { - if (selector === true || scope.$index % 2 === selector) { - if (oldVal && (newVal !== oldVal)) { - if (isObject(oldVal) && !isArray(oldVal)) - oldVal = map(oldVal, function(v, k) { if (v) return k }); - element.removeClass(isArray(oldVal) ? oldVal.join(' ') : oldVal); - } - if (isObject(newVal) && !isArray(newVal)) - newVal = map(newVal, function(v, k) { if (v) return k }); - if (newVal) element.addClass(isArray(newVal) ? newVal.join(' ') : newVal); - } - }; + scope.$watch(attr[name], ngClassWatchAction, true); attr.$observe('class', function(value) { var ngClass = scope.$eval(attr[name]); ngClassWatchAction(ngClass, ngClass); }); + + + if (name !== 'ngClass') { + scope.$watch('$index', function($index, old$index) { + var mod = $index % 2; + if (mod !== old$index % 2) { + if (mod == selector) { + addClass(scope.$eval(attr[name])); + } else { + removeClass(scope.$eval(attr[name])); + } + } + }); + } + + + function ngClassWatchAction(newVal, oldVal) { + if (selector === true || scope.$index % 2 === selector) { + if (oldVal && (newVal !== oldVal)) { + removeClass(oldVal); + } + addClass(newVal); + } + } + + + function removeClass(classVal) { + if (isObject(classVal) && !isArray(classVal)) { + classVal = map(classVal, function(v, k) { if (v) return k }); + } + element.removeClass(isArray(classVal) ? classVal.join(' ') : classVal); + } + + + function addClass(classVal) { + if (isObject(classVal) && !isArray(classVal)) { + classVal = map(classVal, function(v, k) { if (v) return k }); + } + if (classVal) { + element.addClass(isArray(classVal) ? classVal.join(' ') : classVal); + } + } }); }