Skip to content

Commit

Permalink
feat(ngRepeat): use block separator comments
Browse files Browse the repository at this point in the history 
Issue: multi-elements ng-repeat (ng-repeat-start, ng-repeat-end) can contain elements with a trancluding directive. This directive changes content of the row (template) and ng-repeat does not work correctly (when removing/moving rows), because ng-repeat works with the original template (elements).

This changes ng-repeat behavior to traverse the DOM to find current elements everytime we are moving/removing rows (if the template has multiple elements).

Closes angular#3104
  • Loading branch information
@jankuca @jamesdaily
jankuca authored and jamesdaily committed Jan 27, 2014
1 parent f1d5ae4 commit d9dc937
Show file tree
Hide file tree
Showing 5 changed files with 212 additions and 31 deletions.
32 changes: 26 additions & 6 deletions src/ng/directive/ngRepeat.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -279,7 +279,8 @@ var ngRepeatDirective = ['$parse', '$animate', function($parse, $animate) {
trackByIdFn,
collectionKeys,
block, // last object information {scope, element, id}
nextBlockOrder = [];
nextBlockOrder = [],
elementsToRemove;


if (isArrayLike(collection)) {
Expand Down Expand Up @@ -331,8 +332,9 @@ var ngRepeatDirective = ['$parse', '$animate', function($parse, $animate) {
// lastBlockMap is our own object so we don't need to use special hasOwnPropertyFn
if (lastBlockMap.hasOwnProperty(key)) {
block = lastBlockMap[key];
$animate.leave(block.elements);
forEach(block.elements, function(element) { element[NG_REMOVED] = true});
elementsToRemove = getBlockElements(block);
$animate.leave(elementsToRemove);
forEach(elementsToRemove, function(element) { element[NG_REMOVED] = true; });
block.scope.$destroy();
}
}
Expand All @@ -342,6 +344,7 @@ var ngRepeatDirective = ['$parse', '$animate', function($parse, $animate) {
key = (collection === collectionKeys) ? index : collectionKeys[index];
value = collection[key];
block = nextBlockOrder[index];
if (nextBlockOrder[index - 1]) previousNode = nextBlockOrder[index - 1].endNode;

if (block.startNode) {
// if we have already seen this object, then we need to reuse the
Expand All @@ -357,7 +360,7 @@ var ngRepeatDirective = ['$parse', '$animate', function($parse, $animate) {
// do nothing
} else {
// existing item which got moved
$animate.move(block.elements, null, jqLite(previousNode));
$animate.move(getBlockElements(block), null, jqLite(previousNode));
}
previousNode = block.endNode;
} else {
Expand All @@ -375,11 +378,11 @@ var ngRepeatDirective = ['$parse', '$animate', function($parse, $animate) {

if (!block.startNode) {
linker(childScope, function(clone) {
clone[clone.length++] = document.createComment(' end ngRepeat: ' + expression + ' ');
$animate.enter(clone, null, jqLite(previousNode));
previousNode = clone;
block.scope = childScope;
block.startNode = clone[0];
block.elements = clone;
block.startNode = previousNode && previousNode.endNode ? previousNode.endNode : clone[0];
block.endNode = clone[clone.length - 1];
nextBlockMap[block.id] = block;
});
Expand All @@ -390,5 +393,22 @@ var ngRepeatDirective = ['$parse', '$animate', function($parse, $animate) {
};
}
};

function getBlockElements(block) {
if (block.startNode === block.endNode) {
return jqLite(block.startNode);
}

var element = block.startNode;
var elements = [element];

do {
element = element.nextSibling;
if (!element) break;
elements.push(element);
} while (element !== block.endNode);

return jqLite(elements);
}
}];

27 changes: 23 additions & 4 deletions test/BinderSpec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,9 @@ describe('Binder', function() {
'<ul>' +
'<!-- ngRepeat: item in model.items -->' +
'<li ng-bind="item.a" ng-repeat="item in model.items">A</li>' +
'<!-- end ngRepeat: item in model.items -->' +
'<li ng-bind="item.a" ng-repeat="item in model.items">B</li>' +
'<!-- end ngRepeat: item in model.items -->' +
'</ul>');

items.unshift({a: 'C'});
Expand All @@ -105,8 +107,11 @@ describe('Binder', function() {
'<ul>' +
'<!-- ngRepeat: item in model.items -->' +
'<li ng-bind="item.a" ng-repeat="item in model.items">C</li>' +
'<!-- end ngRepeat: item in model.items -->' +
'<li ng-bind="item.a" ng-repeat="item in model.items">A</li>' +
'<!-- end ngRepeat: item in model.items -->' +
'<li ng-bind="item.a" ng-repeat="item in model.items">B</li>' +
'<!-- end ngRepeat: item in model.items -->' +
'</ul>');

items.shift();
Expand All @@ -115,7 +120,9 @@ describe('Binder', function() {
'<ul>' +
'<!-- ngRepeat: item in model.items -->' +
'<li ng-bind="item.a" ng-repeat="item in model.items">A</li>' +
'<!-- end ngRepeat: item in model.items -->' +
'<li ng-bind="item.a" ng-repeat="item in model.items">B</li>' +
'<!-- end ngRepeat: item in model.items -->' +
'</ul>');

items.shift();
Expand All @@ -134,6 +141,7 @@ describe('Binder', function() {
'<ul>' +
'<!-- ngRepeat: item in model.items -->' +
'<li ng-repeat="item in model.items"><span ng-bind="item.a">A</span></li>' +
'<!-- end ngRepeat: item in model.items -->' +
'</ul>');
}));

Expand All @@ -148,15 +156,15 @@ describe('Binder', function() {
$rootScope.items = items;

$rootScope.$apply();
expect(element[0].childNodes.length - 1).toEqual(0);
expect(element[0].childNodes.length).toEqual(1);

items.name = 'misko';
$rootScope.$apply();
expect(element[0].childNodes.length - 1).toEqual(1);
expect(element[0].childNodes.length).toEqual(3);

delete items.name;
$rootScope.$apply();
expect(element[0].childNodes.length - 1).toEqual(0);
expect(element[0].childNodes.length).toEqual(1);
}));

it('IfTextBindingThrowsErrorDecorateTheSpan', function() {
Expand Down Expand Up @@ -223,13 +231,19 @@ describe('Binder', function() {
'<div name="a" ng-repeat="m in model">'+
'<!-- ngRepeat: i in m.item -->' +
'<ul name="a1" ng-repeat="i in m.item"></ul>'+
'<!-- end ngRepeat: i in m.item -->' +
'<ul name="a2" ng-repeat="i in m.item"></ul>'+
'<!-- end ngRepeat: i in m.item -->' +
'</div>'+
'<!-- end ngRepeat: m in model -->' +
'<div name="b" ng-repeat="m in model">'+
'<!-- ngRepeat: i in m.item -->' +
'<ul name="b1" ng-repeat="i in m.item"></ul>'+
'<!-- end ngRepeat: i in m.item -->' +
'<ul name="b2" ng-repeat="i in m.item"></ul>'+
'<!-- end ngRepeat: i in m.item -->' +
'</div>' +
'<!-- end ngRepeat: m in model -->' +
'</div>');
}));

Expand Down Expand Up @@ -306,15 +320,18 @@ describe('Binder', function() {
'<div ng-repeat="i in [0,1]" ng-class-even="\'e\'" ng-class-odd="\'o\'"></div>' +
'</div>')($rootScope);
$rootScope.$apply();

var d1 = jqLite(element[0].childNodes[1]);
var d2 = jqLite(element[0].childNodes[2]);
var d2 = jqLite(element[0].childNodes[3]);
expect(d1.hasClass('o')).toBeTruthy();
expect(d2.hasClass('e')).toBeTruthy();
expect(sortedHtml(element)).toBe(
'<div>' +
'<!-- ngRepeat: i in [0,1] -->' +
'<div class="o" ng-class-even="\'e\'" ng-class-odd="\'o\'" ng-repeat="i in [0,1]"></div>' +
'<!-- end ngRepeat: i in [0,1] -->' +
'<div class="e" ng-class-even="\'e\'" ng-class-odd="\'o\'" ng-repeat="i in [0,1]"></div>' +
'<!-- end ngRepeat: i in [0,1] -->' +
'</div>');
}));

Expand Down Expand Up @@ -420,7 +437,9 @@ describe('Binder', function() {
'<ul>' +
'<!-- ngRepeat: (k,v) in {a:0,b:1} -->' +
'<li ng-bind=\"k + v\" ng-repeat="(k,v) in {a:0,b:1}">a0</li>' +
'<!-- end ngRepeat: (k,v) in {a:0,b:1} -->' +
'<li ng-bind=\"k + v\" ng-repeat="(k,v) in {a:0,b:1}">b1</li>' +
'<!-- end ngRepeat: (k,v) in {a:0,b:1} -->' +
'</ul>');
}));

Expand Down
11 changes: 11 additions & 0 deletions test/helpers/testabilityPatch.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -226,6 +226,17 @@ function sortedHtml(element, showNgClass) {
}


function childrenTagsOf(element) {
var tags = [];

forEach(jqLite(element).children(), function(child) {
tags.push(child.nodeName.toLowerCase());
});

return tags;
}


// TODO(vojta): migrate these helpers into jasmine matchers
/**a
* This method is a cheap way of testing if css for a given node is not set to 'none'. It doesn't
Expand Down
10 changes: 5 additions & 5 deletions test/ng/directive/ngClassSpec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@ describe('ngClass', function() {
element = $compile('<ul><li ng-repeat="i in [0,1]" class="existing" ng-class-odd="\'odd\'" ng-class-even="\'even\'"></li><ul>')($rootScope);
$rootScope.$digest();
var e1 = jqLite(element[0].childNodes[1]);
var e2 = jqLite(element[0].childNodes[2]);
var e2 = jqLite(element[0].childNodes[3]);
expect(e1.hasClass('existing')).toBeTruthy();
expect(e1.hasClass('odd')).toBeTruthy();
expect(e2.hasClass('existing')).toBeTruthy();
Expand All @@ -181,7 +181,7 @@ describe('ngClass', function() {
'<ul>')($rootScope);
$rootScope.$apply();
var e1 = jqLite(element[0].childNodes[1]);
var e2 = jqLite(element[0].childNodes[2]);
var e2 = jqLite(element[0].childNodes[3]);

expect(e1.hasClass('plainClass')).toBeTruthy();
expect(e1.hasClass('odd')).toBeTruthy();
Expand All @@ -199,7 +199,7 @@ describe('ngClass', function() {
'<ul>')($rootScope);
$rootScope.$apply();
var e1 = jqLite(element[0].childNodes[1]);
var e2 = jqLite(element[0].childNodes[2]);
var e2 = jqLite(element[0].childNodes[3]);

expect(e1.hasClass('A')).toBeTruthy();
expect(e1.hasClass('B')).toBeTruthy();
Expand Down Expand Up @@ -273,7 +273,7 @@ describe('ngClass', function() {
$rootScope.$digest();

var e1 = jqLite(element[0].childNodes[1]);
var e2 = jqLite(element[0].childNodes[2]);
var e2 = jqLite(element[0].childNodes[3]);

expect(e1.hasClass('odd')).toBeTruthy();
expect(e1.hasClass('even')).toBeFalsy();
Expand All @@ -295,7 +295,7 @@ describe('ngClass', function() {
$rootScope.$digest();

var e1 = jqLite(element[0].childNodes[1]);
var e2 = jqLite(element[0].childNodes[2]);
var e2 = jqLite(element[0].childNodes[3]);

expect(e1.hasClass('odd')).toBeTruthy();
expect(e1.hasClass('even')).toBeFalsy();
Expand Down
Loading

0 comments on commit d9dc937

Please sign in to comment.