diff --git a/src/ng/directive/ngRepeat.js b/src/ng/directive/ngRepeat.js index c83a6223c65f..221be223a2d9 100644 --- a/src/ng/directive/ngRepeat.js +++ b/src/ng/directive/ngRepeat.js @@ -338,7 +338,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) { @@ -353,15 +352,10 @@ 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]; }; - return { restrict: 'A', multiElement: true, @@ -438,126 +432,124 @@ 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} + collectionIsLikeArray, + collectionKeys = [], + elementsToRemove, + index, key, value, + 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; } - if (isArrayLike(collection)) { - collectionKeys = collection; + // get collectionKeys + collectionIsLikeArray = isArrayLike(collection); + if (collectionIsLikeArray) { 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 (key in collection) { + if (hasOwnProperty.call(collection, key) && key.charAt(0) !== '$') { + collectionKeys.push(key); } } } + nextLength = collectionIsLikeArray ? collection.length : collectionKeys.length; - collectionLength = collectionKeys.length; - nextBlockOrder = new Array(collectionLength); - - // locate existing items - for (index = 0; index < collectionLength; index++) { - key = (collection === collectionKeys) ? index : collectionKeys[index]; + // setup nextBlockMap + for (index = 0; index < nextLength; index++) { + key = collectionIsLikeArray ? 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}; + 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]; + // Set up lastBlockOrder. Used to determine if a block moved. + for (key in lastBlockMap) { + lastBlockOrder[lastBlockMap[key].index] = key; + } - 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++) { + key = collectionIsLikeArray ? index : collectionKeys[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, collection[key], keyIdentifier, 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, collection[key], keyIdentifier, key, nextLength); }); } } + + // leave + // This must go after enter and move because leave prevents getting element's parent. + for (key in lastBlockMap) { + if (nextBlockMap[key]) { + continue; + } + + block = lastBlockMap[key]; + 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 c26d1a923fb1..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,31 +1507,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 = ['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', inject(function($compile, $rootScope, $animate, $document, $sniffer, $timeout) { if (!$sniffer.transitions) return; - var item; var ss = createMockStyleSheet($document); try { @@ -1531,8 +1558,6 @@ describe('ngRepeat animations', function() { it('should fire off the move animation', inject(function($compile, $rootScope, $animate) { - var item; - element = $compile(html( '
' + '
' + @@ -1544,28 +1569,99 @@ 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'); + 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(); + + 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) { + + element = $compile(html( + '
' + + '
' + + '{{ item }}' + + '
' + + '
' + ))($rootScope); - item = $animate.queue.shift(); - expect(item.event).toBe('enter'); - expect(item.element.text()).toBe('2'); + $rootScope.items = ['1','2','3']; + $rootScope.search = ''; + $rootScope.$digest(); - 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.search = '3'; $rootScope.$digest(); - item = $animate.queue.shift(); - expect(item.event).toBe('move'); - 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 = []; + }) + ); + + 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 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'); - item = $animate.queue.shift(); - expect(item.event).toBe('move'); - expect(item.element.text()).toBe('3'); + $rootScope.items = [items[0], items[1], items[3]]; + $rootScope.$digest(); + + // 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 + expect(element.text()).toBe('ABD'); + + } finally { + ss.destroy(); + } }) ); });