From 4c390161e4486c7a00177ceb627d42e58c5ab190 Mon Sep 17 00:00:00 2001 From: Brian Ford Date: Wed, 30 Oct 2013 14:51:02 -0700 Subject: [PATCH 1/3] fix(ngIf): ngIf removes elements dynamically added to it When using ngIf with ngInclude on the same element, ngIf previously did not remove elements added by ngInclude. Similarly, when using ngIfStart/End, ngIf will miss elements added between the start/end markers added after ngIf is linked. This commit changes the behavior of ngIf to add a comment node at the end of its elements such that elements between the starting comment and this ending comment are removed when ngIf's predicate does not hold. --- src/ng/compile.js | 2 +- src/ng/directive/ngIf.js | 35 +++++++++++++++++++++++++-------- test/ng/directive/ngIfSpec.js | 37 +++++++++++++++++++++++++++++++++++ 3 files changed, 65 insertions(+), 9 deletions(-) diff --git a/src/ng/compile.js b/src/ng/compile.js index 7754a8e6d69f..af7d5e6b319d 100644 --- a/src/ng/compile.js +++ b/src/ng/compile.js @@ -1175,7 +1175,7 @@ function $CompileProvider($provide) { if (directiveValue = directive.transclude) { // Special case ngRepeat so that we don't complain about duplicate transclusion, ngRepeat // knows how to handle this on its own. - if (directiveName !== 'ngRepeat') { + if (directiveName !== 'ngRepeat' && directiveName !== 'ngIf') { assertNoDuplicate('transclusion', transcludeDirective, directive, $compileNode); transcludeDirective = directive; } diff --git a/src/ng/directive/ngIf.js b/src/ng/directive/ngIf.js index b2811ab423f1..662f3f320f72 100644 --- a/src/ng/directive/ngIf.js +++ b/src/ng/directive/ngIf.js @@ -86,20 +86,21 @@ var ngIfDirective = ['$animate', function($animate) { restrict: 'A', compile: function (element, attr, transclude) { return function ($scope, $element, $attr) { - var childElement, childScope; + var block = {}, childScope; $scope.$watch($attr.ngIf, function ngIfWatchAction(value) { - if (childElement) { - $animate.leave(childElement); - childElement = undefined; + if (block.startNode) { + $animate.leave(getBlockElements(block)); + block = {}; } - if (childScope) { - childScope.$destroy(); - childScope = undefined; + if (block.startNode) { + getBlockElements(block).$destroy(); + block = {}; } if (toBoolean(value)) { childScope = $scope.$new(); transclude(childScope, function (clone) { - childElement = clone; + block.startNode = clone[0]; + block.endNode = clone[clone.length++] = document.createComment(' end ngIf: ' + $attr.ngIf + ' '); $animate.enter(clone, $element.parent(), $element); }); } @@ -107,4 +108,22 @@ var ngIfDirective = ['$animate', function($animate) { }; } }; + + // TODO(bford): this helper was copypasta'd from ngRepeat + 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); + } }]; diff --git a/test/ng/directive/ngIfSpec.js b/test/ng/directive/ngIfSpec.js index 79eab6bb2a11..509cf26db371 100755 --- a/test/ng/directive/ngIfSpec.js +++ b/test/ng/directive/ngIfSpec.js @@ -60,6 +60,43 @@ describe('ngIf', function () { expect(element.children().length).toBe(9); }); + it('should play nice with ngInclude on the same element', inject(function($templateCache) { + $templateCache.put('test.html', [200, '{{value}}', {}]); + + $scope.value = 'first'; + element.append($compile( + '
' + )($scope)); + $scope.$apply(); + expect(element.text()).toBe('first'); + + $scope.value = 'later'; + $scope.$apply(); + expect(element.text()).toBe(''); + })); + + it('should work with multiple elements', function() { + $scope.show = true; + $scope.things = [1, 2, 3]; + element.append($compile( + '
before;
' + + '
start;
' + + '
{{thing}};
' + + '
end;
' + + '
after;
' + )($scope)); + $scope.$apply(); + expect(element.text()).toBe('before;start;1;2;3;end;after;'); + + $scope.things.push(4); + $scope.$apply(); + expect(element.text()).toBe('before;start;1;2;3;4;end;after;'); + + $scope.show = false; + $scope.$apply(); + expect(element.text()).toBe('before;after;'); + }); + it('should restore the element to its compiled state', function() { $scope.value = true; makeIf('value'); From 4b4749cd97f389c599383c4e051ef660418559aa Mon Sep 17 00:00:00 2001 From: Brian Ford Date: Wed, 30 Oct 2013 14:51:52 -0700 Subject: [PATCH 2/3] chore: move getBlockElements to Angular.js --- src/.jshintrc | 1 + src/Angular.js | 25 ++++++++++++++++++++++++- src/ng/directive/ngIf.js | 18 ------------------ src/ng/directive/ngRepeat.js | 17 ----------------- 4 files changed, 25 insertions(+), 36 deletions(-) diff --git a/src/.jshintrc b/src/.jshintrc index 754c27dc36ca..e29b09f3a848 100644 --- a/src/.jshintrc +++ b/src/.jshintrc @@ -99,6 +99,7 @@ "assertArgFn": false, "assertNotHasOwnProperty": false, "getter": false, + "getBlockElements": false, /* AngularPublic.js */ "version": false, diff --git a/src/Angular.js b/src/Angular.js index 0cdeab3fecaa..df236e056ee5 100644 --- a/src/Angular.js +++ b/src/Angular.js @@ -79,7 +79,8 @@ -assertArg, -assertArgFn, -assertNotHasOwnProperty, - -getter + -getter, + -getBlockElements */ @@ -1318,3 +1319,25 @@ function getter(obj, path, bindFnToScope) { } return obj; } + +/** + * Return the siblings between `startNode` and `endNode`, inclusive + * @param {Object} object with `startNode` and `endNode` properties + * @returns jQlite object containing the elements + */ +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); +} diff --git a/src/ng/directive/ngIf.js b/src/ng/directive/ngIf.js index 662f3f320f72..3f56449f0571 100644 --- a/src/ng/directive/ngIf.js +++ b/src/ng/directive/ngIf.js @@ -108,22 +108,4 @@ var ngIfDirective = ['$animate', function($animate) { }; } }; - - // TODO(bford): this helper was copypasta'd from ngRepeat - 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); - } }]; diff --git a/src/ng/directive/ngRepeat.js b/src/ng/directive/ngRepeat.js index e3ff1b8e2825..ee0faf23452e 100644 --- a/src/ng/directive/ngRepeat.js +++ b/src/ng/directive/ngRepeat.js @@ -393,22 +393,5 @@ 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); - } }]; From becd7f9caea94597f8e09da037eaeed799c718d2 Mon Sep 17 00:00:00 2001 From: Brian Ford Date: Wed, 30 Oct 2013 15:02:25 -0700 Subject: [PATCH 3/3] chore($compile): remove special case for ngIf and ngRepeat --- src/ng/compile.js | 7 ++++--- src/ng/directive/ngIf.js | 1 + src/ng/directive/ngRepeat.js | 1 + 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/src/ng/compile.js b/src/ng/compile.js index af7d5e6b319d..d3ade6b82360 100644 --- a/src/ng/compile.js +++ b/src/ng/compile.js @@ -1173,9 +1173,10 @@ function $CompileProvider($provide) { } if (directiveValue = directive.transclude) { - // Special case ngRepeat so that we don't complain about duplicate transclusion, ngRepeat - // knows how to handle this on its own. - if (directiveName !== 'ngRepeat' && directiveName !== 'ngIf') { + // Special case ngIf and ngRepeat so that we don't complain about duplicate transclusion. + // This option should only be used by directives that know how to how to safely handle element transclusion, + // where the transcluded nodes are added or replaced after linking. + if (!directive.$$tlb) { assertNoDuplicate('transclusion', transcludeDirective, directive, $compileNode); transcludeDirective = directive; } diff --git a/src/ng/directive/ngIf.js b/src/ng/directive/ngIf.js index 3f56449f0571..35c122ad12d9 100644 --- a/src/ng/directive/ngIf.js +++ b/src/ng/directive/ngIf.js @@ -84,6 +84,7 @@ var ngIfDirective = ['$animate', function($animate) { priority: 600, terminal: true, restrict: 'A', + $$tlb: true, compile: function (element, attr, transclude) { return function ($scope, $element, $attr) { var block = {}, childScope; diff --git a/src/ng/directive/ngRepeat.js b/src/ng/directive/ngRepeat.js index ee0faf23452e..9e5ad2f5a6ad 100644 --- a/src/ng/directive/ngRepeat.js +++ b/src/ng/directive/ngRepeat.js @@ -214,6 +214,7 @@ var ngRepeatDirective = ['$parse', '$animate', function($parse, $animate) { transclude: 'element', priority: 1000, terminal: true, + $$tlb: true, compile: function(element, attr, linker) { return function($scope, $element, $attr){ var expression = $attr.ngRepeat;