From b0972a2e75909e41dbac6e4413ada7df2d51df3a Mon Sep 17 00:00:00 2001 From: Tobias Bosch Date: Tue, 26 Nov 2013 19:55:02 -0800 Subject: fix($compile): update cloned elements if the template arrives after the cloning If an element has a directive whose content is loaded using `templateUrl`, and the element is cloned using a linking function before the template arrives, the clone needs to be updated as well. This also updates `ngIf` and `ngRepeat` to keep the connection to the clone of a tranclude function, so that they know about the changes a directive with `templateUrl` does to the element in the future. Fixes to #4930. --- src/Angular.js | 16 ++++++------ src/ng/compile.js | 2 +- src/ng/directive/ngIf.js | 9 ++++--- src/ng/directive/ngRepeat.js | 30 ++++++++++++++-------- test/ng/compileSpec.js | 53 +++++++++++++++++++++++++++++++++++++++ test/ng/directive/ngIfSpec.js | 27 +++++++++++++++++++- test/ng/directive/ngRepeatSpec.js | 24 +++++++++++++++++- 7 files changed, 138 insertions(+), 23 deletions(-) diff --git a/src/Angular.js b/src/Angular.js index 478ef2a2..b09d3a7f 100644 --- a/src/Angular.js +++ b/src/Angular.js @@ -1330,23 +1330,25 @@ function getter(obj, path, bindFnToScope) { } /** - * Return the siblings between `startNode` and `endNode`, inclusive - * @param {Object} object with `startNode` and `endNode` properties + * Return the DOM siblings between the first and last node in the given array. + * @param {Array} array like object * @returns jQlite object containing the elements */ -function getBlockElements(block) { - if (block.startNode === block.endNode) { - return jqLite(block.startNode); +function getBlockElements(nodes) { + var startNode = nodes[0], + endNode = nodes[nodes.length - 1]; + if (startNode === endNode) { + return jqLite(startNode); } - var element = block.startNode; + var element = startNode; var elements = [element]; do { element = element.nextSibling; if (!element) break; elements.push(element); - } while (element !== block.endNode); + } while (element !== endNode); return jqLite(elements); } diff --git a/src/ng/compile.js b/src/ng/compile.js index fd8a8729..7d0bb008 100644 --- a/src/ng/compile.js +++ b/src/ng/compile.js @@ -931,7 +931,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { createBoundTranscludeFn(scope, childTranscludeFn || transcludeFn) ); } else { - nodeLinkFn(childLinkFn, childScope, node, undefined, boundTranscludeFn); + nodeLinkFn(childLinkFn, childScope, node, $rootElement, boundTranscludeFn); } } else if (childLinkFn) { childLinkFn(scope, node.childNodes, undefined, boundTranscludeFn); diff --git a/src/ng/directive/ngIf.js b/src/ng/directive/ngIf.js index dcb3825d..a05d30d0 100644 --- a/src/ng/directive/ngIf.js +++ b/src/ng/directive/ngIf.js @@ -94,9 +94,12 @@ var ngIfDirective = ['$animate', function($animate) { if (!childScope) { childScope = $scope.$new(); $transclude(childScope, function (clone) { + clone[clone.length++] = document.createComment(' end ngIf: ' + $attr.ngIf + ' '); + // 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 it's template arrives. block = { - startNode: clone[0], - endNode: clone[clone.length++] = document.createComment(' end ngIf: ' + $attr.ngIf + ' ') + clone: clone }; $animate.enter(clone, $element.parent(), $element); }); @@ -109,7 +112,7 @@ var ngIfDirective = ['$animate', function($animate) { } if (block) { - $animate.leave(getBlockElements(block)); + $animate.leave(getBlockElements(block.clone)); block = null; } } diff --git a/src/ng/directive/ngRepeat.js b/src/ng/directive/ngRepeat.js index cd605bf3..86874a41 100644 --- a/src/ng/directive/ngRepeat.js +++ b/src/ng/directive/ngRepeat.js @@ -301,7 +301,7 @@ var ngRepeatDirective = ['$parse', '$animate', function($parse, $animate) { } else if (nextBlockMap.hasOwnProperty(trackById)) { // restore lastBlockMap forEach(nextBlockOrder, function(block) { - if (block && block.startNode) lastBlockMap[block.id] = block; + if (block && block.scope) lastBlockMap[block.id] = block; }); // This is a duplicate and we need to 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}", @@ -318,7 +318,7 @@ 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]; - elementsToRemove = getBlockElements(block); + elementsToRemove = getBlockElements(block.clone); $animate.leave(elementsToRemove); forEach(elementsToRemove, function(element) { element[NG_REMOVED] = true; }); block.scope.$destroy(); @@ -330,9 +330,9 @@ 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 (nextBlockOrder[index - 1]) previousNode = getBlockEnd(nextBlockOrder[index - 1]); - if (block.startNode) { + if (block.scope) { // if we have already seen this object, then we need to reuse the // associated scope/element childScope = block.scope; @@ -342,11 +342,11 @@ var ngRepeatDirective = ['$parse', '$animate', function($parse, $animate) { nextNode = nextNode.nextSibling; } while(nextNode && nextNode[NG_REMOVED]); - if (block.startNode != nextNode) { + if (getBlockStart(block) != nextNode) { // existing item which got moved - $animate.move(getBlockElements(block), null, jqLite(previousNode)); + $animate.move(getBlockElements(block.clone), null, jqLite(previousNode)); } - previousNode = block.endNode; + previousNode = getBlockEnd(block); } else { // new item which we don't know about childScope = $scope.$new(); @@ -362,14 +362,16 @@ var ngRepeatDirective = ['$parse', '$animate', function($parse, $animate) { childScope.$odd = !(childScope.$even = (index&1) === 0); // jshint bitwise: true - if (!block.startNode) { + if (!block.scope) { $transclude(childScope, function(clone) { clone[clone.length++] = document.createComment(' end ngRepeat: ' + expression + ' '); $animate.enter(clone, null, jqLite(previousNode)); previousNode = clone; block.scope = childScope; - block.startNode = previousNode && previousNode.endNode ? previousNode.endNode : clone[0]; - block.endNode = clone[clone.length - 1]; + // 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 it's template arrives. + block.clone = clone; nextBlockMap[block.id] = block; }); } @@ -378,5 +380,13 @@ var ngRepeatDirective = ['$parse', '$animate', function($parse, $animate) { }); } }; + + function getBlockStart(block) { + return block.clone[0]; + } + + function getBlockEnd(block) { + return block.clone[block.clone.length - 1]; + } }]; diff --git a/test/ng/compileSpec.js b/test/ng/compileSpec.js index c017bfa6..f2fa4ef6 100755 --- a/test/ng/compileSpec.js +++ b/test/ng/compileSpec.js @@ -969,6 +969,32 @@ describe('$compile', function() { }); }); + it('should resolve widgets after cloning in append mode without $templateCache', function() { + module(function($exceptionHandlerProvider) { + $exceptionHandlerProvider.mode('log'); + }); + inject(function($compile, $templateCache, $rootScope, $httpBackend, $browser, + $exceptionHandler) { + $httpBackend.expect('GET', 'cau.html').respond('{{name}}'); + $rootScope.name = 'Elvis'; + var template = $compile('
'); + var e1; + var e2; + + e1 = template($rootScope.$new(), noop); // clone + expect(e1.text()).toEqual(''); + + $httpBackend.flush(); + + e2 = template($rootScope.$new(), noop); // clone + $rootScope.$digest(); + expect(e1.text()).toEqual('Elvis'); + expect(e2.text()).toEqual('Elvis'); + + dealoc(e1); + dealoc(e2); + }); + }); it('should resolve widgets after cloning in inline mode', function() { module(function($exceptionHandlerProvider) { @@ -1010,6 +1036,33 @@ describe('$compile', function() { }); }); + it('should resolve widgets after cloning in inline mode without $templateCache', function() { + module(function($exceptionHandlerProvider) { + $exceptionHandlerProvider.mode('log'); + }); + inject(function($compile, $templateCache, $rootScope, $httpBackend, $browser, + $exceptionHandler) { + $httpBackend.expect('GET', 'cau.html').respond('{{name}}'); + $rootScope.name = 'Elvis'; + var template = $compile('
'); + var e1; + var e2; + + e1 = template($rootScope.$new(), noop); // clone + expect(e1.text()).toEqual(''); + + $httpBackend.flush(); + + e2 = template($rootScope.$new(), noop); // clone + $rootScope.$digest(); + expect(e1.text()).toEqual('Elvis'); + expect(e2.text()).toEqual('Elvis'); + + dealoc(e1); + dealoc(e2); + }); + }); + it('should be implicitly terminal and not compile placeholder content in append', inject( function($compile, $templateCache, $rootScope, log) { diff --git a/test/ng/directive/ngIfSpec.js b/test/ng/directive/ngIfSpec.js index 427bfd59..db923150 100755 --- a/test/ng/directive/ngIfSpec.js +++ b/test/ng/directive/ngIfSpec.js @@ -1,8 +1,11 @@ 'use strict'; describe('ngIf', function () { - var $scope, $compile, element; + var $scope, $compile, element, $compileProvider; + beforeEach(module(function(_$compileProvider_) { + $compileProvider = _$compileProvider_; + })); beforeEach(inject(function ($rootScope, _$compile_) { $scope = $rootScope.$new(); $compile = _$compile_; @@ -146,6 +149,28 @@ describe('ngIf', function () { expect(element.children()[0].className).toContain('my-class'); }); + it('should work when combined with an ASYNC template that loads after the first digest', inject(function($httpBackend, $compile, $rootScope) { + $compileProvider.directive('test', function() { + return { + templateUrl: 'test.html' + }; + }); + $httpBackend.whenGET('test.html').respond('hello'); + element.append('
'); + $compile(element)($rootScope); + $rootScope.show = true; + $rootScope.$apply(); + expect(element.text()).toBe(''); + + $httpBackend.flush(); + expect(element.text()).toBe('hello'); + + $rootScope.show = false; + $rootScope.$apply(); + // Note: there are still comments in element! + expect(element.children().length).toBe(0); + expect(element.text()).toBe(''); + })); }); describe('ngIf and transcludes', function() { diff --git a/test/ng/directive/ngRepeatSpec.js b/test/ng/directive/ngRepeatSpec.js index 6584f31a..bdc0b8f5 100644 --- a/test/ng/directive/ngRepeatSpec.js +++ b/test/ng/directive/ngRepeatSpec.js @@ -749,8 +749,30 @@ describe('ngRepeat', function() { expect(element.text()).toBe('123'); })); } - }); + it('should work when combined with an ASYNC template that loads after the first digest', inject(function($httpBackend, $compile, $rootScope) { + $compileProvider.directive('test', function() { + return { + templateUrl: 'test.html' + }; + }); + $httpBackend.whenGET('test.html').respond('hello'); + element = jqLite('
'); + $compile(element)($rootScope); + $rootScope.items = [1]; + $rootScope.$apply(); + expect(element.text()).toBe(''); + + $httpBackend.flush(); + expect(element.text()).toBe('hello'); + + $rootScope.items = []; + $rootScope.$apply(); + // Note: there are still comments in element! + expect(element.children().length).toBe(0); + expect(element.text()).toBe(''); + })); + }); it('should add separator comments after each item', inject(function ($compile, $rootScope) { var check = function () { -- cgit v1.2.3