From 2331c42d15ad294ba839a499d3e2109816b3e6f8 Mon Sep 17 00:00:00 2001 From: pondermatic Date: Tue, 30 Aug 2016 16:41:08 -0500 Subject: [PATCH 1/9] fix(ngRepeat): trigger move animation ngRepeat does not trigger a move animation for items that come after inserted or deleted items. The documentation says that a move animation occurs "when an adjacent item is filtered out causing a reorder or when the item contents are reordered". With this fix, items that are moved can use an animation, such as changing the border or background color, to show a change. This will fix issue #15068. --- src/ng/directive/ngRepeat.js | 154 +++++++++++++++--------------- test/ng/directive/ngRepeatSpec.js | 58 ++++++++++- 2 files changed, 135 insertions(+), 77 deletions(-) diff --git a/src/ng/directive/ngRepeat.js b/src/ng/directive/ngRepeat.js index 0b1240d5c4d4..66df7e1eb64f 100644 --- a/src/ng/directive/ngRepeat.js +++ b/src/ng/directive/ngRepeat.js @@ -325,7 +325,6 @@ */ var ngRepeatDirective = ['$parse', '$animate', '$compile', function($parse, $animate, $compile) { - var NG_REMOVED = '$$NG_REMOVED'; var ngRepeatMinErr = minErr('ngRepeat'); var updateScope = function(scope, index, valueIdentifier, value, keyIdentifier, key, arrayLength) { @@ -425,126 +424,129 @@ var ngRepeatDirective = ['$parse', '$animate', '$compile', function($parse, $ani //watch props $scope.$watchCollection(rhs, function ngRepeatAction(collection) { - var index, length, - previousNode = $element[0], // node that cloned nodes should be inserted after - // initialized to the comment node anchor - nextNode, - // Same as lastBlockMap but it has the current state. It will become the - // lastBlockMap on the next iteration. - nextBlockMap = createMap(), - collectionLength, - key, value, // key/value of iteration - trackById, - trackByIdFn, - collectionKeys, - block, // last object information {scope, element, id} - nextBlockOrder, - elementsToRemove; + var + block, // last object information {scope, element, id} + collectionKey, + collectionKeys = [], + elementsToRemove, + index, key, value, // key/value of iteration + lastBlockOrder = [], + lastKey, + nextBlockMap = createMap(), + nextBlockOrder = [], + nextKey, nextLength, + previousNode = $element[0], // node that cloned nodes should be inserted after + // initialized to the comment node anchor + trackById, + trackByIdFn; if (aliasAs) { $scope[aliasAs] = collection; } + // get collectionKeys if (isArrayLike(collection)) { collectionKeys = collection; trackByIdFn = trackByIdExpFn || trackByIdArrayFn; } else { trackByIdFn = trackByIdExpFn || trackByIdObjFn; // if object, extract keys, in enumeration order, unsorted - collectionKeys = []; - for (var itemKey in collection) { - if (hasOwnProperty.call(collection, itemKey) && itemKey.charAt(0) !== '$') { - collectionKeys.push(itemKey); + for (collectionKey in collection) { + if (hasOwnProperty.call(collection, collectionKey) && collectionKey.charAt(0) !== '$') { + collectionKeys.push(collectionKey); } } } + nextLength = collectionKeys.length; - collectionLength = collectionKeys.length; - nextBlockOrder = new Array(collectionLength); - - // locate existing items - for (index = 0; index < collectionLength; index++) { + // setup nextBlockMap + for (index = 0; index < nextLength; index++) { key = (collection === collectionKeys) ? index : collectionKeys[index]; value = collection[key]; trackById = trackByIdFn(key, value, index); - if (lastBlockMap[trackById]) { - // found previously seen block - block = lastBlockMap[trackById]; - delete lastBlockMap[trackById]; - nextBlockMap[trackById] = block; - nextBlockOrder[index] = block; - } else if (nextBlockMap[trackById]) { - // if collision detected. restore lastBlockMap and throw an error - forEach(nextBlockOrder, function(block) { - if (block && block.scope) lastBlockMap[block.id] = block; - }); + + if (nextBlockMap[trackById]) { + // if collision detected, throw an error throw ngRepeatMinErr('dupes', - 'Duplicates in a repeater are not allowed. Use \'track by\' expression to specify unique keys. Repeater: {0}, Duplicate key: {1}, Duplicate value: {2}', - expression, trackById, value); - } else { - // new never before seen block - nextBlockOrder[index] = {id: trackById, scope: undefined, clone: undefined}; - nextBlockMap[trackById] = true; + 'Duplicates in a repeater are not allowed. Use \'track by\' expression to specify unique keys. Repeater: {0}, Duplicate key: {1}, Duplicate value: {2}', + expression, trackById, value); } - } - // remove leftover items - for (var blockKey in lastBlockMap) { - block = lastBlockMap[blockKey]; - elementsToRemove = getBlockNodes(block.clone); - $animate.leave(elementsToRemove); - if (elementsToRemove[0].parentNode) { - // if the element was not removed yet because of pending animation, mark it as deleted - // so that we can ignore it later - for (index = 0, length = elementsToRemove.length; index < length; index++) { - elementsToRemove[index][NG_REMOVED] = true; - } - } - block.scope.$destroy(); + nextBlockMap[trackById] = {id: trackById, clone: undefined, scope: undefined, index: index, key: key, value: value}; + nextBlockOrder[index] = trackById; } - // we are not using forEach for perf reasons (trying to avoid #call) - for (index = 0; index < collectionLength; index++) { - key = (collection === collectionKeys) ? index : collectionKeys[index]; - value = collection[key]; - block = nextBlockOrder[index]; + // setup lastBlockOrder, used to determine if block moved + for (lastKey in lastBlockMap) { + lastBlockOrder.push(lastKey); + } - if (block.scope) { - // if we have already seen this object, then we need to reuse the - // associated scope/element + for (index = 0; index < nextLength; index++) { + nextKey = nextBlockOrder[index]; - nextNode = previousNode; + if (lastBlockMap[nextKey]) { + // we have already seen this object and need to reuse the associated scope/element + block = lastBlockMap[nextKey]; - // skip nodes that are already pending removal via leave animation - do { - nextNode = nextNode.nextSibling; - } while (nextNode && nextNode[NG_REMOVED]); + // move + if (lastBlockMap[nextKey].index !== nextBlockMap[nextKey].index) { + // If this block has moved because the last previous block was removed, + // then use the last previous block to set previousNode. + lastKey = lastBlockOrder[lastBlockMap[nextKey].index - 1]; + if (lastKey && !nextBlockMap[lastKey]) { + previousNode = getBlockEnd(lastBlockMap[lastKey]); + } - if (getBlockStart(block) !== nextNode) { - // existing item which got moved $animate.move(getBlockNodes(block.clone), null, previousNode); + block.index = nextBlockMap[nextKey].index; } + + updateScope(block.scope, index, + valueIdentifier, nextBlockMap[nextKey].value, + keyIdentifier, nextBlockMap[nextKey].key, nextLength); + + nextBlockMap[nextKey] = block; previousNode = getBlockEnd(block); - updateScope(block.scope, index, valueIdentifier, value, keyIdentifier, key, collectionLength); + } else { + // enter // new item which we don't know about $transclude(function ngRepeatTransclude(clone, scope) { - block.scope = scope; + nextBlockMap[nextKey].scope = scope; // http://jsperf.com/clone-vs-createcomment var endNode = ngRepeatEndComment.cloneNode(false); clone[clone.length++] = endNode; $animate.enter(clone, null, previousNode); previousNode = endNode; + // Note: We only need the first/last node of the cloned nodes. // However, we need to keep the reference to the jqlite wrapper as it might be changed later // by a directive with templateUrl when its template arrives. - block.clone = clone; - nextBlockMap[block.id] = block; - updateScope(block.scope, index, valueIdentifier, value, keyIdentifier, key, collectionLength); + nextBlockMap[nextKey].clone = clone; + updateScope(scope, nextBlockMap[nextKey].index, + valueIdentifier, nextBlockMap[nextKey].value, + keyIdentifier, nextBlockMap[nextKey].key, nextLength); + + delete nextBlockMap[nextKey].key; + delete nextBlockMap[nextKey].value; }); } } + + // leave + // This must go after enter and move because leave prevents getting element's parent. + for (lastKey in lastBlockMap) { + if (nextBlockMap[lastKey]) { + continue; + } + + block = lastBlockMap[lastKey]; + elementsToRemove = getBlockNodes(block.clone); + $animate.leave(elementsToRemove); + block.scope.$destroy(); + } + lastBlockMap = nextBlockMap; }); }; diff --git a/test/ng/directive/ngRepeatSpec.js b/test/ng/directive/ngRepeatSpec.js index 71e5287a5233..8a49ba2e45d1 100644 --- a/test/ng/directive/ngRepeatSpec.js +++ b/test/ng/directive/ngRepeatSpec.js @@ -1487,7 +1487,13 @@ describe('ngRepeat animations', function() { $rootScope.items = ['1','3']; $rootScope.$digest(); - item = $animate.queue.shift(); + while ($animate.queue.length) { + item = $animate.queue.shift(); + if (item.event == 'leave') { + break; + } + } + expect(item.event).toBe('leave'); expect(item.element.text()).toBe('2'); })); @@ -1565,6 +1571,56 @@ describe('ngRepeat animations', function() { item = $animate.queue.shift(); expect(item.event).toBe('move'); expect(item.element.text()).toBe('3'); + + item = $animate.queue.shift(); + expect(item.event).toBe('move'); + expect(item.element.text()).toBe('1'); + }) + ); + + it('should fire off the move animation for filtered items', + inject(function($compile, $rootScope, $animate) { + + var item; + + element = $compile(html( + '
' + + '
' + + '{{ item }}' + + '
' + + '
' + ))($rootScope); + + $rootScope.items = ['1','2','3']; + $rootScope.search = ''; + $rootScope.$digest(); + + item = $animate.queue.shift(); + expect(item.event).toBe('enter'); + expect(item.element.text()).toBe('1'); + + item = $animate.queue.shift(); + expect(item.event).toBe('enter'); + expect(item.element.text()).toBe('2'); + + item = $animate.queue.shift(); + expect(item.event).toBe('enter'); + expect(item.element.text()).toBe('3'); + + $rootScope.search = '3'; + $rootScope.$digest(); + + item = $animate.queue.shift(); + expect(item.event).toBe('move'); + expect(item.element.text()).toBe('3'); + + item = $animate.queue.shift(); + expect(item.event).toBe('leave'); + expect(item.element.text()).toBe('1'); + + item = $animate.queue.shift(); + expect(item.event).toBe('leave'); + expect(item.element.text()).toBe('2'); }) ); }); From 263be6bbdfe67a877c50430fcb4acd9045ff8448 Mon Sep 17 00:00:00 2001 From: pondermatic Date: Tue, 30 Aug 2016 16:55:48 -0500 Subject: [PATCH 2/9] style(ngRepeat) eslint does not like unused functions and requires comparison operators with type. --- src/ng/directive/ngRepeat.js | 6 +++--- test/ng/directive/ngRepeatSpec.js | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/ng/directive/ngRepeat.js b/src/ng/directive/ngRepeat.js index 66df7e1eb64f..6820e2ff2c3c 100644 --- a/src/ng/directive/ngRepeat.js +++ b/src/ng/directive/ngRepeat.js @@ -339,9 +339,9 @@ var ngRepeatDirective = ['$parse', '$animate', '$compile', function($parse, $ani scope.$odd = !(scope.$even = (index & 1) === 0); }; - var getBlockStart = function(block) { - return block.clone[0]; - }; + // var getBlockStart = function(block) { + // return block.clone[0]; + // }; var getBlockEnd = function(block) { return block.clone[block.clone.length - 1]; diff --git a/test/ng/directive/ngRepeatSpec.js b/test/ng/directive/ngRepeatSpec.js index 8a49ba2e45d1..9bc2a065ea84 100644 --- a/test/ng/directive/ngRepeatSpec.js +++ b/test/ng/directive/ngRepeatSpec.js @@ -1489,7 +1489,7 @@ describe('ngRepeat animations', function() { while ($animate.queue.length) { item = $animate.queue.shift(); - if (item.event == 'leave') { + if (item.event === 'leave') { break; } } From f3c6062630a682b63f22d137335893d7ac5f91e4 Mon Sep 17 00:00:00 2001 From: James Richards Date: Wed, 31 Aug 2016 23:04:21 -0500 Subject: [PATCH 3/9] style(ngRepeat): getBlockStart function is no longer needed --- src/ng/directive/ngRepeat.js | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/ng/directive/ngRepeat.js b/src/ng/directive/ngRepeat.js index 6820e2ff2c3c..b4f82de7eaca 100644 --- a/src/ng/directive/ngRepeat.js +++ b/src/ng/directive/ngRepeat.js @@ -339,10 +339,6 @@ var ngRepeatDirective = ['$parse', '$animate', '$compile', function($parse, $ani scope.$odd = !(scope.$even = (index & 1) === 0); }; - // var getBlockStart = function(block) { - // return block.clone[0]; - // }; - var getBlockEnd = function(block) { return block.clone[block.clone.length - 1]; }; From 47bd3a4eff6e1849c9fee8176807b2e5ca185589 Mon Sep 17 00:00:00 2001 From: James Richards Date: Thu, 1 Sep 2016 00:19:10 -0500 Subject: [PATCH 4/9] style(ngRepeat): kick off Travis-CI The previous Travis test failed because of a network issue with saucelabs.com. --- src/ng/directive/ngRepeat.js | 1 - 1 file changed, 1 deletion(-) diff --git a/src/ng/directive/ngRepeat.js b/src/ng/directive/ngRepeat.js index b4f82de7eaca..b44a83153a25 100644 --- a/src/ng/directive/ngRepeat.js +++ b/src/ng/directive/ngRepeat.js @@ -343,7 +343,6 @@ var ngRepeatDirective = ['$parse', '$animate', '$compile', function($parse, $ani return block.clone[block.clone.length - 1]; }; - return { restrict: 'A', multiElement: true, From 7a981722316779a8c14c5a1669d974db54b847ef Mon Sep 17 00:00:00 2001 From: pondermatic Date: Mon, 12 Sep 2016 17:03:12 -0500 Subject: [PATCH 5/9] perf(ngRepeat): use less memory and compare less In the ngRepeatAction function: * The collection keys and values are accessed directly instead of copying them to nextBlockMap temporarily. * Checking the collectionIsLikeArray boolean value is faster than comparing value and type of collectionKeys and collection. --- src/ng/directive/ngRepeat.js | 47 ++++++++++++++++-------------------- 1 file changed, 21 insertions(+), 26 deletions(-) diff --git a/src/ng/directive/ngRepeat.js b/src/ng/directive/ngRepeat.js index 6820e2ff2c3c..b95f990a94eb 100644 --- a/src/ng/directive/ngRepeat.js +++ b/src/ng/directive/ngRepeat.js @@ -425,16 +425,17 @@ var ngRepeatDirective = ['$parse', '$animate', '$compile', function($parse, $ani //watch props $scope.$watchCollection(rhs, function ngRepeatAction(collection) { var - block, // last object information {scope, element, id} - collectionKey, + block, // last object information {scope, element, id} + collectionIsLikeArray, collectionKeys = [], elementsToRemove, - index, key, value, // key/value of iteration + index, key, value, lastBlockOrder = [], lastKey, nextBlockMap = createMap(), nextBlockOrder = [], - nextKey, nextLength, + nextKey, + nextLength, previousNode = $element[0], // node that cloned nodes should be inserted after // initialized to the comment node anchor trackById, @@ -445,23 +446,23 @@ var ngRepeatDirective = ['$parse', '$animate', '$compile', function($parse, $ani } // get collectionKeys - if (isArrayLike(collection)) { - collectionKeys = collection; + collectionIsLikeArray = isArrayLike(collection); + if (collectionIsLikeArray) { trackByIdFn = trackByIdExpFn || trackByIdArrayFn; } else { trackByIdFn = trackByIdExpFn || trackByIdObjFn; // if object, extract keys, in enumeration order, unsorted - for (collectionKey in collection) { - if (hasOwnProperty.call(collection, collectionKey) && collectionKey.charAt(0) !== '$') { - collectionKeys.push(collectionKey); + for (key in collection) { + if (hasOwnProperty.call(collection, key) && key.charAt(0) !== '$') { + collectionKeys.push(key); } } } - nextLength = collectionKeys.length; + nextLength = collectionIsLikeArray ? collection.length : collectionKeys.length; // setup nextBlockMap for (index = 0; index < nextLength; index++) { - key = (collection === collectionKeys) ? index : collectionKeys[index]; + key = collectionIsLikeArray ? index : collectionKeys[index]; value = collection[key]; trackById = trackByIdFn(key, value, index); @@ -472,16 +473,17 @@ var ngRepeatDirective = ['$parse', '$animate', '$compile', function($parse, $ani expression, trackById, value); } - nextBlockMap[trackById] = {id: trackById, clone: undefined, scope: undefined, index: index, key: key, value: value}; + nextBlockMap[trackById] = {id: trackById, clone: undefined, scope: undefined, index: index}; nextBlockOrder[index] = trackById; } // setup lastBlockOrder, used to determine if block moved - for (lastKey in lastBlockMap) { - lastBlockOrder.push(lastKey); + for (key in lastBlockMap) { + lastBlockOrder.push(key); } for (index = 0; index < nextLength; index++) { + key = collectionIsLikeArray ? index : collectionKeys[index]; nextKey = nextBlockOrder[index]; if (lastBlockMap[nextKey]) { @@ -501,9 +503,7 @@ var ngRepeatDirective = ['$parse', '$animate', '$compile', function($parse, $ani block.index = nextBlockMap[nextKey].index; } - updateScope(block.scope, index, - valueIdentifier, nextBlockMap[nextKey].value, - keyIdentifier, nextBlockMap[nextKey].key, nextLength); + updateScope(block.scope, index, valueIdentifier, collection[key], keyIdentifier, key, nextLength); nextBlockMap[nextKey] = block; previousNode = getBlockEnd(block); @@ -524,24 +524,19 @@ var ngRepeatDirective = ['$parse', '$animate', '$compile', function($parse, $ani // However, we need to keep the reference to the jqlite wrapper as it might be changed later // by a directive with templateUrl when its template arrives. nextBlockMap[nextKey].clone = clone; - updateScope(scope, nextBlockMap[nextKey].index, - valueIdentifier, nextBlockMap[nextKey].value, - keyIdentifier, nextBlockMap[nextKey].key, nextLength); - - delete nextBlockMap[nextKey].key; - delete nextBlockMap[nextKey].value; + updateScope(scope, nextBlockMap[nextKey].index, valueIdentifier, collection[key], keyIdentifier, key, nextLength); }); } } // leave // This must go after enter and move because leave prevents getting element's parent. - for (lastKey in lastBlockMap) { - if (nextBlockMap[lastKey]) { + for (key in lastBlockMap) { + if (nextBlockMap[key]) { continue; } - block = lastBlockMap[lastKey]; + block = lastBlockMap[key]; elementsToRemove = getBlockNodes(block.clone); $animate.leave(elementsToRemove); block.scope.$destroy(); From c969ae23c48eab32989435d8ed4f9767e89c5ad4 Mon Sep 17 00:00:00 2001 From: pondermatic Date: Tue, 21 Mar 2017 11:56:10 -0500 Subject: [PATCH 6/9] fix(ngRepeat): correctly re-create last block order when track by is an integer Merge changes from https://github.com/Narretz/angular.js/commit/7dbf428765d3093cd0419c8dd5b4e7bd52cbb290. --- src/ng/directive/ngRepeat.js | 4 +-- test/ng/directive/ngRepeatSpec.js | 51 +++++++++++++++++++++++++++++++ 2 files changed, 53 insertions(+), 2 deletions(-) diff --git a/src/ng/directive/ngRepeat.js b/src/ng/directive/ngRepeat.js index fdd5763958b7..221be223a2d9 100644 --- a/src/ng/directive/ngRepeat.js +++ b/src/ng/directive/ngRepeat.js @@ -485,9 +485,9 @@ var ngRepeatDirective = ['$parse', '$animate', '$compile', function($parse, $ani nextBlockOrder[index] = trackById; } - // setup lastBlockOrder, used to determine if block moved + // Set up lastBlockOrder. Used to determine if a block moved. for (key in lastBlockMap) { - lastBlockOrder.push(key); + lastBlockOrder[lastBlockMap[key].index] = key; } for (index = 0; index < nextLength; index++) { diff --git a/test/ng/directive/ngRepeatSpec.js b/test/ng/directive/ngRepeatSpec.js index 5ac036ed58d5..e12923252cdb 100644 --- a/test/ng/directive/ngRepeatSpec.js +++ b/test/ng/directive/ngRepeatSpec.js @@ -1624,4 +1624,55 @@ describe('ngRepeat animations', function() { expect(item.element.text()).toBe('2'); }) ); + it('should maintain the order when the track by expression evaluates to an integer', + inject(function($compile, $rootScope, $animate, $document, $sniffer, $timeout) { + if (!$sniffer.transitions) return; + + var item; + var ss = createMockStyleSheet($document); + + var items = [ + {id: 1, name: 'A'}, + {id: 2, name: 'B'}, + {id: 4, name: 'C'}, + {id: 3, name: 'D'} + ]; + + try { + + $animate.enabled(true); + + ss.addRule('.animate-me div', + 'transition:1s linear all;'); + + element = $compile(html('
' + + '
{{ item.name }}
' + + '
'))($rootScope); + + $rootScope.items = [items[0], items[1], items[2]]; + $rootScope.$digest(); + expect(element.text()).toBe('ABC'); + + $rootScope.items.push(items[3]); + $rootScope.$digest(); + + expect(element.text()).toBe('ABCD'); // the original order should be preserved + $animate.flush(); + $timeout.flush(1500); // 1s * 1.5 closing buffer + expect(element.text()).toBe('ABCD'); + + $rootScope.items = [items[0], items[1], items[3]]; + $rootScope.$digest(); + + // The leaving item should maintain it's position until it is removed + expect(element.text()).toBe('ABCD'); + $animate.flush(); + $timeout.flush(1500); // 1s * 1.5 closing buffer + expect(element.text()).toBe('ABD'); + + } finally { + ss.destroy(); + } + }) + ); }); From 410312ce76c20ab9b3e4d01efb89711569ed7921 Mon Sep 17 00:00:00 2001 From: pondermatic Date: Fri, 24 Mar 2017 12:41:46 -0500 Subject: [PATCH 7/9] test(ngRepeat): Update based on feedback. In the 'should fire off the leave animation' test, revert change in how the animation queue is shifted to find item '2'. At one time during the refactoring of ngRepeat, the order of events in the queue changed. Improve title of the spec that checks for 'move' animation events when filtering items changes other items positions. --- test/ng/directive/ngRepeatSpec.js | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/test/ng/directive/ngRepeatSpec.js b/test/ng/directive/ngRepeatSpec.js index e12923252cdb..aac5d0e826ef 100644 --- a/test/ng/directive/ngRepeatSpec.js +++ b/test/ng/directive/ngRepeatSpec.js @@ -1488,13 +1488,7 @@ describe('ngRepeat animations', function() { $rootScope.items = ['1','3']; $rootScope.$digest(); - while ($animate.queue.length) { - item = $animate.queue.shift(); - if (item.event === 'leave') { - break; - } - } - + item = $animate.queue.shift(); expect(item.event).toBe('leave'); expect(item.element.text()).toBe('2'); })); @@ -1579,7 +1573,7 @@ describe('ngRepeat animations', function() { }) ); - it('should fire off the move animation for filtered items', + it('should fire off the move animation for items whose position changed due to other items being filtered out', inject(function($compile, $rootScope, $animate) { var item; @@ -1624,6 +1618,7 @@ describe('ngRepeat animations', function() { expect(item.element.text()).toBe('2'); }) ); + it('should maintain the order when the track by expression evaluates to an integer', inject(function($compile, $rootScope, $animate, $document, $sniffer, $timeout) { if (!$sniffer.transitions) return; @@ -1664,7 +1659,7 @@ describe('ngRepeat animations', function() { $rootScope.items = [items[0], items[1], items[3]]; $rootScope.$digest(); - // The leaving item should maintain it's position until it is removed + // The leaving item should maintain its position until it is removed expect(element.text()).toBe('ABCD'); $animate.flush(); $timeout.flush(1500); // 1s * 1.5 closing buffer From 4d6feb99f3aff9d878563ea9a2c5b7a310df8891 Mon Sep 17 00:00:00 2001 From: pondermatic Date: Sat, 25 Mar 2017 08:52:02 -0500 Subject: [PATCH 8/9] test(ngRepeat): Update based on feedback. Improve title of the spec that checks for 'move' animation events when filtering items changes other items positions. Remove unused item variable in two tests. --- test/ng/directive/ngRepeatSpec.js | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/test/ng/directive/ngRepeatSpec.js b/test/ng/directive/ngRepeatSpec.js index aac5d0e826ef..29da9d53b624 100644 --- a/test/ng/directive/ngRepeatSpec.js +++ b/test/ng/directive/ngRepeatSpec.js @@ -1497,7 +1497,6 @@ describe('ngRepeat animations', function() { inject(function($compile, $rootScope, $animate, $document, $sniffer, $timeout) { if (!$sniffer.transitions) return; - var item; var ss = createMockStyleSheet($document); try { @@ -1573,7 +1572,7 @@ describe('ngRepeat animations', function() { }) ); - it('should fire off the move animation for items whose position changed due to other items being filtered out', + it('should fire off the move animation for items that change position when other items are filtered out', inject(function($compile, $rootScope, $animate) { var item; @@ -1623,7 +1622,6 @@ describe('ngRepeat animations', function() { inject(function($compile, $rootScope, $animate, $document, $sniffer, $timeout) { if (!$sniffer.transitions) return; - var item; var ss = createMockStyleSheet($document); var items = [ From 3b17d5d886ce5ed9cd02c69346c560f0e81a237a Mon Sep 17 00:00:00 2001 From: pondermatic Date: Sat, 25 Mar 2017 10:17:33 -0500 Subject: [PATCH 9/9] test(ngRepeat): Add 'toContainQueueItem' Jasmine matcher The 'ngRepeat animations' test group should not assume the order of items in the animation queue. This matcher searches the animation queue for an item with an expected element text and event. --- test/ng/directive/ngRepeatSpec.js | 150 +++++++++++++++--------------- 1 file changed, 73 insertions(+), 77 deletions(-) diff --git a/test/ng/directive/ngRepeatSpec.js b/test/ng/directive/ngRepeatSpec.js index 29da9d53b624..86aa1853abd1 100644 --- a/test/ng/directive/ngRepeatSpec.js +++ b/test/ng/directive/ngRepeatSpec.js @@ -1424,6 +1424,51 @@ describe('ngRepeat animations', function() { }; })); + beforeEach(function() { + jasmine.addMatchers({ + toContainQueueItem: function() { + return { + compare: generateCompare(false), + negativeCompare: generateCompare(true) + }; + function generateCompare(isNot) { + /** + * Matcher that checks that the expected element text is in the actual Array of + * animation queue items and that the event matches. + * @param {array} actual + * @param {string} expectedElementText + * @param {string} expectedEvent optional if isNot = true + * @returns {{pass: boolean, message: message}} + */ + return function(actual, expectedElementText, expectedEvent) { + if (expectedEvent === undefined) { + expectedEvent = ''; + } + var actualLength = actual.length; + var i; + var item; + var message = valueFn( + 'Expected animation queue to ' + (isNot ? 'not ' : '') + 'contain an item ' + + 'where the element\'s text is "' + expectedElementText + + '"' + (isNot ? '' : ' and the event is "' + expectedEvent + '"') + ); + var pass = isNot; + + for (i = 0; i < actualLength; i++) { + item = actual[i]; + if (item.element.text() === expectedElementText) { + pass = item.event === expectedEvent; + break; + } + } + + return {'pass': pass, 'message': message}; + }; + } + } + }); + }); + afterEach(function() { body.empty(); }); @@ -1431,8 +1476,6 @@ describe('ngRepeat animations', function() { it('should fire off the enter animation', inject(function($compile, $rootScope, $animate) { - var item; - element = $compile(html( '
' + @@ -1445,24 +1488,15 @@ describe('ngRepeat animations', function() { $rootScope.items = ['1','2','3']; $rootScope.$digest(); - item = $animate.queue.shift(); - expect(item.event).toBe('enter'); - expect(item.element.text()).toBe('1'); - - item = $animate.queue.shift(); - expect(item.event).toBe('enter'); - expect(item.element.text()).toBe('2'); - - item = $animate.queue.shift(); - expect(item.event).toBe('enter'); - expect(item.element.text()).toBe('3'); + expect($animate.queue).toContainQueueItem('1', 'enter'); + expect($animate.queue).toContainQueueItem('2', 'enter'); + expect($animate.queue).toContainQueueItem('3', 'enter'); + $animate.queue = []; })); it('should fire off the leave animation', inject(function($compile, $rootScope, $animate) { - var item; - element = $compile(html( '
' + @@ -1473,24 +1507,18 @@ describe('ngRepeat animations', function() { $rootScope.items = ['1','2','3']; $rootScope.$digest(); - item = $animate.queue.shift(); - expect(item.event).toBe('enter'); - expect(item.element.text()).toBe('1'); - - item = $animate.queue.shift(); - expect(item.event).toBe('enter'); - expect(item.element.text()).toBe('2'); - - item = $animate.queue.shift(); - expect(item.event).toBe('enter'); - expect(item.element.text()).toBe('3'); + expect($animate.queue).toContainQueueItem('1', 'enter'); + expect($animate.queue).toContainQueueItem('2', 'enter'); + expect($animate.queue).toContainQueueItem('3', 'enter'); + $animate.queue = []; $rootScope.items = ['1','3']; $rootScope.$digest(); - item = $animate.queue.shift(); - expect(item.event).toBe('leave'); - expect(item.element.text()).toBe('2'); + expect($animate.queue).not.toContainQueueItem('1'); + expect($animate.queue).toContainQueueItem('2', 'leave'); + expect($animate.queue).toContainQueueItem('3', 'move'); + $animate.queue = []; })); it('should not change the position of the block that is being animated away via a leave animation', @@ -1530,8 +1558,6 @@ describe('ngRepeat animations', function() { it('should fire off the move animation', inject(function($compile, $rootScope, $animate) { - var item; - element = $compile(html( '
' + '
' + @@ -1543,40 +1569,24 @@ describe('ngRepeat animations', function() { $rootScope.items = ['1','2','3']; $rootScope.$digest(); - item = $animate.queue.shift(); - expect(item.event).toBe('enter'); - expect(item.element.text()).toBe('1'); - - item = $animate.queue.shift(); - expect(item.event).toBe('enter'); - expect(item.element.text()).toBe('2'); - - item = $animate.queue.shift(); - expect(item.event).toBe('enter'); - expect(item.element.text()).toBe('3'); + expect($animate.queue).toContainQueueItem('1', 'enter'); + expect($animate.queue).toContainQueueItem('2', 'enter'); + expect($animate.queue).toContainQueueItem('3', 'enter'); + $animate.queue = []; $rootScope.items = ['2','3','1']; $rootScope.$digest(); - item = $animate.queue.shift(); - expect(item.event).toBe('move'); - expect(item.element.text()).toBe('2'); - - item = $animate.queue.shift(); - expect(item.event).toBe('move'); - expect(item.element.text()).toBe('3'); - - item = $animate.queue.shift(); - expect(item.event).toBe('move'); - expect(item.element.text()).toBe('1'); + expect($animate.queue).toContainQueueItem('1', 'move'); + expect($animate.queue).toContainQueueItem('2', 'move'); + expect($animate.queue).toContainQueueItem('3', 'move'); + $animate.queue = []; }) ); it('should fire off the move animation for items that change position when other items are filtered out', inject(function($compile, $rootScope, $animate) { - var item; - element = $compile(html( '
' + '
' + @@ -1589,32 +1599,18 @@ describe('ngRepeat animations', function() { $rootScope.search = ''; $rootScope.$digest(); - item = $animate.queue.shift(); - expect(item.event).toBe('enter'); - expect(item.element.text()).toBe('1'); - - item = $animate.queue.shift(); - expect(item.event).toBe('enter'); - expect(item.element.text()).toBe('2'); - - item = $animate.queue.shift(); - expect(item.event).toBe('enter'); - expect(item.element.text()).toBe('3'); + expect($animate.queue).toContainQueueItem('1', 'enter'); + expect($animate.queue).toContainQueueItem('2', 'enter'); + expect($animate.queue).toContainQueueItem('3', 'enter'); + $animate.queue = []; $rootScope.search = '3'; $rootScope.$digest(); - item = $animate.queue.shift(); - expect(item.event).toBe('move'); - expect(item.element.text()).toBe('3'); - - item = $animate.queue.shift(); - expect(item.event).toBe('leave'); - expect(item.element.text()).toBe('1'); - - item = $animate.queue.shift(); - expect(item.event).toBe('leave'); - expect(item.element.text()).toBe('2'); + expect($animate.queue).toContainQueueItem('1', 'leave'); + expect($animate.queue).toContainQueueItem('2', 'leave'); + expect($animate.queue).toContainQueueItem('3', 'move'); + $animate.queue = []; }) );