From ee668a716dc8ecb5d56e741ad27b3bf6833d8744 Mon Sep 17 00:00:00 2001 From: Kamil Pekala Date: Sun, 26 Jan 2014 20:20:13 +0100 Subject: [PATCH 1/5] feat(ngRepeat): ngRepeat is now immune to removing or moving its elements without its corresponding comment node . --- src/.jshintrc | 1 + src/Angular.js | 10 +++ src/jqLite.js | 81 +++++++++++++++++++++--- src/ng/directive/ngRepeat.js | 41 +++++++++++- test/ng/directive/ngRepeatSpec.js | 100 +++++++++++++++++++++++++++++- 5 files changed, 223 insertions(+), 10 deletions(-) diff --git a/src/.jshintrc b/src/.jshintrc index f32caa451ed6..e499a9c4dc67 100644 --- a/src/.jshintrc +++ b/src/.jshintrc @@ -116,6 +116,7 @@ "jqNextId": false, "camelCase": false, "jqLitePatchJQueryRemove": false, + "jqLitePatchJQueryInsert": false, "JQLite": false, "jqLiteClone": false, "jqLiteDealoc": false, diff --git a/src/Angular.js b/src/Angular.js index 21ce1ff7282d..19f576423fd5 100644 --- a/src/Angular.js +++ b/src/Angular.js @@ -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; } diff --git a/src/jqLite.js b/src/jqLite.js index e59805147886..878d0f6f6558 100644 --- a/src/jqLite.js +++ b/src/jqLite.js @@ -166,10 +166,33 @@ function jqLitePatchJQueryRemove(name, dispatchThis, filterElems, getterIfNoArgu } } } + if(name === 'remove'){ + var elementsDetached = jQuery(this); + if(elementsDetached){ + 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() { + // jshint -W040 + var elementsAttached = jQuery(arguments[0]), + originalResult = originalJqFn.apply(this, arguments); + if(elementsAttached){ + elementsAttached.triggerHandler('$attach', elementsAttached); + } + return originalResult; + } +} + ///////////////////////////////////////////// function JQLite(element) { if (element instanceof JQLite) { @@ -759,16 +782,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) { @@ -785,43 +813,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); }, 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, @@ -875,6 +919,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){ /** diff --git a/src/ng/directive/ngRepeat.js b/src/ng/directive/ngRepeat.js index 43db58ba5ab5..6a0ef4d3a925 100644 --- a/src/ng/directive/ngRepeat.js +++ b/src/ng/directive/ngRepeat.js @@ -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.parentElement; + 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.parentElement; + if(parent) + parent.removeChild(correspondingCommentElement); + } + }); + $scope.$on('$destroy', function(){ + clone.off('$attach'); + clone.off('$detach'); + }); + } + + clone[clone.length++] = correspondingCommentElement; $animate.enter(clone, null, jqLite(previousNode)); previousNode = clone; block.scope = childScope; diff --git a/test/ng/directive/ngRepeatSpec.js b/test/ng/directive/ngRepeatSpec.js index 638f082c1474..acf4695b71d1 100644 --- a/test/ng/directive/ngRepeatSpec.js +++ b/test/ng/directive/ngRepeatSpec.js @@ -1027,8 +1027,8 @@ describe('ngRepeat', function() { expect(newLis.length).toBe(2); expect(newLis[0]).toBe(lis[1]); }); - }); + }); describe('compatibility', function() { @@ -1037,7 +1037,7 @@ describe('ngRepeat', function() { transclude: 'element', controller: function($transclude, $scope, $element) { $transclude(function(transcludedNodes) { - $element.after(']]').after(transcludedNodes).after('[['); + $element.after(']]').after(transcludedNodes).after('[['); }); } })); @@ -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( + '' + )(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( + '' + ); + + element.find('li').eq(1).remove(); + expect(sortedHtml(element)).toBe( + '' + ); + }); + it('should remove the correspoding comment node if an element is detached', function(){ + detached = element.find('li').eq(1).detach(); + expect(sortedHtml(element)).toBe( + '' + ); + }); + 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( + '' + ); + }); + 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( + '' + ); + }); + 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( + '' + ); + }); + }); }); describe('ngRepeat and transcludes', function() { From 4777caae8714e55f015d0bc318fc6ed0a5fb7d7b Mon Sep 17 00:00:00 2001 From: Kamil Pekala Date: Tue, 28 Jan 2014 11:36:40 +0100 Subject: [PATCH 2/5] fix(ngRepeat): fixes issue with non-existing parentElement property in IE on comment nodes --- src/ng/directive/ngRepeat.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/ng/directive/ngRepeat.js b/src/ng/directive/ngRepeat.js index 6a0ef4d3a925..3e3133b5c952 100644 --- a/src/ng/directive/ngRepeat.js +++ b/src/ng/directive/ngRepeat.js @@ -372,7 +372,7 @@ var ngRepeatDirective = ['$parse', '$animate', function($parse, $animate) { clone.on('$attach', function(event, origElem, origComment){ // should append the original corresponding comment element if(origComment !== correspondingCommentElement){ - var parent = origElem.parentElement; + var parent = origElem.parentNode; if(parent){ if(origElem.nextSibling) parent.insertBefore(correspondingCommentElement, origElem.nextSibling); @@ -394,7 +394,7 @@ var ngRepeatDirective = ['$parse', '$animate', function($parse, $animate) { clone.on('$detach', function(event, origElem, origComment){ // should remove the original corresponding comment element if(origComment !== correspondingCommentElement){ - var parent = correspondingCommentElement.parentElement; + var parent = correspondingCommentElement.parentNode; if(parent) parent.removeChild(correspondingCommentElement); } From a7f9bec8eeed7a273feb08644cda68fb7e269b21 Mon Sep 17 00:00:00 2001 From: Kamil Pekala Date: Mon, 24 Feb 2014 08:37:28 +0100 Subject: [PATCH 3/5] jQuery methods patches now support all original methods signatures. --- src/jqLite.js | 23 ++++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-) diff --git a/src/jqLite.js b/src/jqLite.js index 878d0f6f6558..b04794202465 100644 --- a/src/jqLite.js +++ b/src/jqLite.js @@ -166,9 +166,9 @@ function jqLitePatchJQueryRemove(name, dispatchThis, filterElems, getterIfNoArgu } } } - if(name === 'remove'){ - var elementsDetached = jQuery(this); - if(elementsDetached){ + if(dispatchThis){ + var elementsDetached = param ? jQuery(this).filter(param) : jQuery(this); + if(elementsDetached.length){ elementsDetached.triggerHandler('$detach', elementsDetached); } } @@ -184,8 +184,21 @@ function jqLitePatchJQueryInsert(name){ function insertPatch() { // jshint -W040 - var elementsAttached = jQuery(arguments[0]), - originalResult = originalJqFn.apply(this, arguments); + 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); } From 4e5aa2c8760e77a2470f328899f2aada34b1cee8 Mon Sep 17 00:00:00 2001 From: Kamil Pekala Date: Mon, 24 Feb 2014 09:06:55 +0100 Subject: [PATCH 4/5] removed trailing whitespace --- src/jqLite.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/jqLite.js b/src/jqLite.js index b04794202465..709ce528dfcf 100644 --- a/src/jqLite.js +++ b/src/jqLite.js @@ -946,7 +946,7 @@ forEach({ } } } - + // 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 From 2a1e0f9cca586402d91a113a809dbdc590fa91c0 Mon Sep 17 00:00:00 2001 From: Kamil Pekala Date: Tue, 18 Mar 2014 14:22:49 +0100 Subject: [PATCH 5/5] fix(ngRepeat): fixed a memory leak --- src/ng/directive/ngRepeat.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ng/directive/ngRepeat.js b/src/ng/directive/ngRepeat.js index 3e3133b5c952..671fde2d26be 100644 --- a/src/ng/directive/ngRepeat.js +++ b/src/ng/directive/ngRepeat.js @@ -399,7 +399,7 @@ var ngRepeatDirective = ['$parse', '$animate', function($parse, $animate) { parent.removeChild(correspondingCommentElement); } }); - $scope.$on('$destroy', function(){ + childScope.$on('$destroy', function(){ clone.off('$attach'); clone.off('$detach'); });