Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

feat(ngRepeat): ngRepeat is now immune to removing or moving its elements without its corresponding comment node #6016

Closed
wants to merge 5 commits into from
Closed

feat(ngRepeat): ngRepeat is now immune to removing or moving its elements without its corresponding comment node #6016

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ee668a7
feat(ngRepeat): ngRepeat is now immune to removing or moving its elem…
kamilkp Jan 26, 2014
4777caa
fix(ngRepeat): fixes issue with non-existing parentElement property i…
kamilkp Jan 28, 2014
a7f9bec
jQuery methods patches now support all original methods signatures.
kamilkp Feb 24, 2014
4e5aa2c
removed trailing whitespace
kamilkp Feb 24, 2014
2a1e0f9
fix(ngRepeat): fixed a memory leak
kamilkp Mar 18, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/.jshintrc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,7 @@
"jqNextId": false,
"camelCase": false,
"jqLitePatchJQueryRemove": false,
"jqLitePatchJQueryInsert": false,
"JQLite": false,
"jqLiteClone": false,
"jqLiteDealoc": false,
Expand Down
10 changes: 10 additions & 0 deletions src/Angular.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1270,6 +1270,16 @@ function bindJQuery() {
jqLitePatchJQueryRemove('remove', true, true, false);
jqLitePatchJQueryRemove('empty', false, false, false);
jqLitePatchJQueryRemove('html', false, false, true);

// Method signature:
// jqLitePatchJQueryInsert(name)
// all jQuery methods that can insert an element into DOM
jqLitePatchJQueryInsert('append');
jqLitePatchJQueryInsert('prepend');
jqLitePatchJQueryInsert('before');
jqLitePatchJQueryInsert('after');
jqLitePatchJQueryInsert('replaceWith');
jqLitePatchJQueryInsert('wrapAll');
} else {
jqLite = JQLite;
}
Expand Down
94 changes: 87 additions & 7 deletions src/jqLite.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,10 +166,46 @@ function jqLitePatchJQueryRemove(name, dispatchThis, filterElems, getterIfNoArgu
}
}
}
if(dispatchThis){
var elementsDetached = param ? jQuery(this).filter(param) : jQuery(this);
if(elementsDetached.length){
elementsDetached.triggerHandler('$detach', elementsDetached);
}
}
return originalJqFn.apply(this, arguments);
}
}

function jqLitePatchJQueryInsert(name){
var originalJqFn = jQuery.fn[name];
originalJqFn = originalJqFn.$original || originalJqFn;
insertPatch.$original = originalJqFn;
jQuery.fn[name] = insertPatch;

function insertPatch() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about this signature of append?

Tests for that should be added.

// jshint -W040
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What error does this silence?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

http://www.jshint.com/docs/options/#validthis

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then it's better to just do:

/* jshint validthis: true */

at the beginning of the function. jsHint comment directives are scoped, no need to rely on warning numbers that are cryptic & subject to change.

var elementsAttached,
arg = arguments[0];
if(typeof arg === "function"){
elementsAttached = jQuery();
var res,
self = this;
this.each(function(index){
res = arg.call(this, index, self.html());
elementsAttached = elementsAttached.add(res);
});
}
else{
elementsAttached = jQuery(arg);
}
var originalResult = originalJqFn.apply(this, arguments);
if(elementsAttached){
elementsAttached.triggerHandler('$attach', elementsAttached);
}
return originalResult;
}
}

/////////////////////////////////////////////
function JQLite(element) {
if (element instanceof JQLite) {
Expand Down Expand Up @@ -759,16 +795,21 @@ forEach({
},

replaceWith: function(element, replaceNode) {
var index, parent = element.parentNode;
var nodeElem = new JQLite(replaceNode),
args = [],
index, parent = element.parentNode;
jqLiteDealoc(element);
forEach(new JQLite(replaceNode), function(node){
forEach(nodeElem, function(node){
if (index) {
parent.insertBefore(node, index.nextSibling);
args = args.concat(node);
} else {
parent.replaceChild(node, element);
args = args.concat(node);
}
index = node;
});
nodeElem.triggerHandler('$attach', args);
},

children: function(element) {
Expand All @@ -785,43 +826,59 @@ forEach({
},

append: function(element, node) {
forEach(new JQLite(node), function(child){
var nodeElem = new JQLite(node),
args = [];
forEach(nodeElem, function(child){
if (element.nodeType === 1 || element.nodeType === 11) {
element.appendChild(child);
args = args.concat(child);
}
});
nodeElem.triggerHandler('$attach', args);
},

prepend: function(element, node) {
var nodeElem = new JQLite(node),
args = [];
if (element.nodeType === 1) {
var index = element.firstChild;
forEach(new JQLite(node), function(child){
forEach(nodeElem, function(child){
element.insertBefore(child, index);
args = args.concat(child);
});
}
nodeElem.triggerHandler('$attach', args);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does jQuery emit these events? I can't find any evidence of it in the 2.x.x tree, so this feels wrong to me. I think I need to investigate the issue a bit to do a proper review.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After this patch:

// in Angular.js
// Method signature:
//     jqLitePatchJQueryInsert(name)
// all jQuery methods that can insert an element into DOM
jqLitePatchJQueryInsert('append');
jqLitePatchJQueryInsert('prepend');
jqLitePatchJQueryInsert('before');
jqLitePatchJQueryInsert('after');
jqLitePatchJQueryInsert('replaceWith');
jqLitePatchJQueryInsert('wrapAll');

// in jqLite.js
function jqLitePatchJQueryInsert(name){
    var originalJqFn = jQuery.fn[name];
    originalJqFn = originalJqFn.$original || originalJqFn;
    insertPatch.$original = originalJqFn;
    jQuery.fn[name] = insertPatch;

    function insertPatch() {
      // jshint -W040
      var elementsAttached = jQuery(arguments[0]),
        originalResult = originalJqFn.apply(this, arguments);
      if(elementsAttached){
        elementsAttached.triggerHandler('$attach', elementsAttached);
      }
      return originalResult;
    }
}

It does emit these events. It is the exact same kind of jQuery function patch that alredy exists in angular.js for jQuery remove, empty and html functions. It's called jqLitePatchJQueryRemove

},

wrap: function(element, wrapNode) {
wrapNode = jqLite(wrapNode)[0];
var wrapElem = jqLite(wrapNode);
wrapNode = wrapElem[0];
var parent = element.parentNode;
if (parent) {
parent.replaceChild(wrapNode, element);
}
wrapNode.appendChild(element);
wrapElem.triggerHandler('$attach', wrapElem);
},

remove: function(element) {
var jelement = new JQLite(element);
jelement.triggerHandler('$detach', jelement);
jqLiteDealoc(element);
var parent = element.parentNode;
if (parent) parent.removeChild(element);
},

after: function(element, newElement) {
var index = element, parent = element.parentNode;
forEach(new JQLite(newElement), function(node){
var nodeElem = new JQLite(newElement),
args = [],
index = element, parent = element.parentNode;
forEach(nodeElem, function(node){
parent.insertBefore(node, index.nextSibling);
index = node;
args = args.concat(node);
});
nodeElem.triggerHandler('$attach', args);
},

addClass: jqLiteAddClass,
Expand Down Expand Up @@ -875,6 +932,29 @@ forEach({
forEach(eventFns, function(fn) {
fn.apply(element, event.concat(eventData));
});
},

detach: function(element) {
var jelement = new JQLite(element);
jelement.triggerHandler('$detach', jelement);

function triggerDestroy(el){
if(el.nodeType === 1){
new JQLite(el).triggerHandler('$destroy');
for ( var i = 0, children = el.childNodes || []; i < children.length; i++) {
triggerDestroy(children[i]);
}
}
}

// jQuery 'detach' method calls jQuery 'remvove' method with additional parameter
// this casues that the jqLitePatchJQueryRemove that patches jQuery remove triggers $destroy events as well
// the following function mimics this behaviour
triggerDestroy(element);

var parent = element.parentNode;
if (parent) parent.removeChild(element);
return jelement;
}
}, function(fn, name){
/**
Expand Down
41 changes: 40 additions & 1 deletion src/ng/directive/ngRepeat.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -366,7 +366,46 @@ var ngRepeatDirective = ['$parse', '$animate', function($parse, $animate) {

if (!block.scope) {
$transclude(childScope, function(clone) {
clone[clone.length++] = document.createComment(' end ngRepeat: ' + expression + ' ');
var correspondingCommentElement = document.createComment(' end ngRepeat: ' + expression + ' ');

if(clone.length === 1){
clone.on('$attach', function(event, origElem, origComment){
// should append the original corresponding comment element
if(origComment !== correspondingCommentElement){
var parent = origElem.parentNode;
if(parent){
if(origElem.nextSibling)
parent.insertBefore(correspondingCommentElement, origElem.nextSibling);
else
parent.appendChild(correspondingCommentElement);

var nextSibling = correspondingCommentElement.nextSibling;
// get first sibling comment node, or if it's a regular element don't bother
while(nextSibling && nextSibling.nodeType !== 8 && nextSibling.nodeType !== 1)
nextSibling = nextSibling.nextSibling;
// inserted in the middle of another ngRepeat pair
if(nextSibling && nextSibling.nodeType === 8 && /ngRepeat/i.test(nextSibling.nodeValue)){
parent.removeChild(nextSibling);
parent.insertBefore(nextSibling, origElem);
}
}
}
});
clone.on('$detach', function(event, origElem, origComment){
// should remove the original corresponding comment element
if(origComment !== correspondingCommentElement){
var parent = correspondingCommentElement.parentNode;
if(parent)
parent.removeChild(correspondingCommentElement);
}
});
childScope.$on('$destroy', function(){
clone.off('$attach');
clone.off('$detach');
});
}

clone[clone.length++] = correspondingCommentElement;
$animate.enter(clone, null, jqLite(previousNode));
previousNode = clone;
block.scope = childScope;
Expand Down
100 changes: 98 additions & 2 deletions test/ng/directive/ngRepeatSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1027,8 +1027,8 @@ describe('ngRepeat', function() {
expect(newLis.length).toBe(2);
expect(newLis[0]).toBe(lis[1]);
});
});

});

describe('compatibility', function() {

Expand All @@ -1037,7 +1037,7 @@ describe('ngRepeat', function() {
transclude: 'element',
controller: function($transclude, $scope, $element) {
$transclude(function(transcludedNodes) {
$element.after(']]').after(transcludedNodes).after('[[');
$element.after('<span>]]</span>').after(transcludedNodes).after('<span>[[</span>');
});
}
}));
Expand Down Expand Up @@ -1110,6 +1110,102 @@ describe('ngRepeat', function() {
expect(element.text()).not.toContain('if:1;');
}));
});

describe('adding/removing the element itself', function() {
var detached;
beforeEach(function(){
scope.array = [1, 2, 3];
element = $compile(
'<ul>' +
'<li ng-repeat="item in array">{{item}};</li>' +
'</ul>'
)(scope);
scope.$digest();
detached = null;
});
afterEach(function(){
if(detached)
dealoc(detached);
});
it('should remove the corresponding comment node if an element is removed', function() {
element.find('li').eq(0).remove();
expect(sortedHtml(element)).toBe(
'<ul>' +
'<!-- ngRepeat: item in array -->' +
'<li ng-repeat="item in array">2;</li>' +
'<!-- end ngRepeat: item in array -->' +
'<li ng-repeat="item in array">3;</li>' +
'<!-- end ngRepeat: item in array -->' +
'</ul>'
);

element.find('li').eq(1).remove();
expect(sortedHtml(element)).toBe(
'<ul>' +
'<!-- ngRepeat: item in array -->' +
'<li ng-repeat="item in array">2;</li>' +
'<!-- end ngRepeat: item in array -->' +
'</ul>'
);
});
it('should remove the correspoding comment node if an element is detached', function(){
detached = element.find('li').eq(1).detach();
expect(sortedHtml(element)).toBe(
'<ul>' +
'<!-- ngRepeat: item in array -->' +
'<li ng-repeat="item in array">1;</li>' +
'<!-- end ngRepeat: item in array -->' +
'<li ng-repeat="item in array">3;</li>' +
'<!-- end ngRepeat: item in array -->' +
'</ul>'
);
});
it('should insert the correspoding comment node if an element is reattached', function(){
detached = element.find('li').eq(1).detach();
element.append(detached);
expect(sortedHtml(element)).toBe(
'<ul>' +
'<!-- ngRepeat: item in array -->' +
'<li ng-repeat="item in array">1;</li>' +
'<!-- end ngRepeat: item in array -->' +
'<li ng-repeat="item in array">3;</li>' +
'<!-- end ngRepeat: item in array -->' +
'<li ng-repeat="item in array">2;</li>' +
'<!-- end ngRepeat: item in array -->' +
'</ul>'
);
});
it('should reorder nodes if an element is inserted in the middle of another element-comment node pair', function(){
detached = element.find('li').eq(0).detach();
element.find('li').eq(1).after(detached);
expect(sortedHtml(element)).toBe(
'<ul>' +
'<!-- ngRepeat: item in array -->' +
'<li ng-repeat="item in array">2;</li>' +
'<!-- end ngRepeat: item in array -->' +
'<li ng-repeat="item in array">3;</li>' +
'<!-- end ngRepeat: item in array -->' +
'<li ng-repeat="item in array">1;</li>' +
'<!-- end ngRepeat: item in array -->' +
'</ul>'
);
});
it('should reorder nodes if an element is inserted before the ngRepeat opening comment node', function(){
detached = element.find('li').eq(2).detach();
element.prepend(detached);
expect(sortedHtml(element)).toBe(
'<ul>' +
'<!-- ngRepeat: item in array -->' +
'<li ng-repeat="item in array">3;</li>' +
'<!-- end ngRepeat: item in array -->' +
'<li ng-repeat="item in array">1;</li>' +
'<!-- end ngRepeat: item in array -->' +
'<li ng-repeat="item in array">2;</li>' +
'<!-- end ngRepeat: item in array -->' +
'</ul>'
);
});
});
});

describe('ngRepeat and transcludes', function() {
Expand Down