From 67cc94ae3ff9c0051327498aa1fea36a8f945706 Mon Sep 17 00:00:00 2001 From: Igor Minar Date: Thu, 10 Oct 2013 21:35:54 -0700 Subject: [PATCH] fix($compile): abort compilation when duplicate element transclusion Issue an error and abort compilation when two directives that ask for transclusion are found on a single element. This configuration is not supported and we previously failed to issue the error because in the case of element transclusion the compilation is re-started and this caused the compilation context to be lost. The ngRepeat directive has been special-cased to bypass this warning because it knows how to handle this scenario internally. This is not an ideal solution to the problem of multiple transclusions per element, we are hoping to have this configuration supported by the compiler in the future. See #4357. Closes #3893 Closes #4217 Closes #3307 --- src/ng/compile.js | 50 +++++++++++------- test/ng/compileSpec.js | 84 ++++++++++++++++++++++++++----- test/ng/directive/ngRepeatSpec.js | 37 ++++++++++++++ 3 files changed, 142 insertions(+), 29 deletions(-) diff --git a/src/ng/compile.js b/src/ng/compile.js index a13be3b8fffd..c44985fdee26 100644 --- a/src/ng/compile.js +++ b/src/ng/compile.js @@ -469,7 +469,7 @@ function $CompileProvider($provide) { //================================ - function compile($compileNodes, transcludeFn, maxPriority, ignoreDirective) { + function compile($compileNodes, transcludeFn, maxPriority, ignoreDirective, previousCompileContext) { if (!($compileNodes instanceof jqLite)) { // jquery always rewraps, whereas we need to preserve the original selector so that we can modify it. $compileNodes = jqLite($compileNodes); @@ -481,7 +481,7 @@ function $CompileProvider($provide) { $compileNodes[index] = node = jqLite(node).wrap('').parent()[0]; } }); - var compositeLinkFn = compileNodes($compileNodes, transcludeFn, $compileNodes, maxPriority, ignoreDirective); + var compositeLinkFn = compileNodes($compileNodes, transcludeFn, $compileNodes, maxPriority, ignoreDirective, previousCompileContext); return function publicLinkFn(scope, cloneConnectFn){ assertArg(scope, 'scope'); // important!!: we must call our jqLite.clone() since the jQuery one is trying to be smart @@ -528,7 +528,7 @@ function $CompileProvider($provide) { * @param {number=} max directive priority * @returns {?function} A composite linking function of all of the matched directives or null. */ - function compileNodes(nodeList, transcludeFn, $rootElement, maxPriority, ignoreDirective) { + function compileNodes(nodeList, transcludeFn, $rootElement, maxPriority, ignoreDirective, previousCompileContext) { var linkFns = [], nodeLinkFn, childLinkFn, directives, attrs, linkFnFound; @@ -539,7 +539,7 @@ function $CompileProvider($provide) { directives = collectDirectives(nodeList[i], [], attrs, i == 0 ? maxPriority : undefined, ignoreDirective); nodeLinkFn = (directives.length) - ? applyDirectivesToNode(directives, nodeList[i], attrs, transcludeFn, $rootElement, null, [], []) + ? applyDirectivesToNode(directives, nodeList[i], attrs, transcludeFn, $rootElement, null, [], [], previousCompileContext) : null; childLinkFn = (nodeLinkFn && nodeLinkFn.terminal || !nodeList[i].childNodes || !nodeList[i].childNodes.length) @@ -550,6 +550,7 @@ function $CompileProvider($provide) { linkFns.push(nodeLinkFn); linkFns.push(childLinkFn); linkFnFound = (linkFnFound || nodeLinkFn || childLinkFn); + previousCompileContext = null; //use the previous context only for the first element in the virtual group } // return a linking function if we have found anything, null otherwise @@ -750,23 +751,26 @@ function $CompileProvider($provide) { * scope argument is auto-generated to the new child of the transcluded parent scope. * @param {JQLite} jqCollection If we are working on the root of the compile tree then this * argument has the root jqLite array so that we can replace nodes on it. - * @param {Object=} ignoreDirective An optional directive that will be ignored when compiling + * @param {Object=} originalReplaceDirective An optional directive that will be ignored when compiling * the transclusion. * @param {Array.} preLinkFns * @param {Array.} postLinkFns + * @param {Object} previousCompileContext Context used for previous compilation of the current node * @returns linkFn */ function applyDirectivesToNode(directives, compileNode, templateAttrs, transcludeFn, jqCollection, - originalReplaceDirective, preLinkFns, postLinkFns) { + originalReplaceDirective, preLinkFns, postLinkFns, previousCompileContext) { + previousCompileContext = previousCompileContext || {}; + var terminalPriority = -Number.MAX_VALUE, - newScopeDirective = null, - newIsolateScopeDirective = null, - templateDirective = null, + newScopeDirective, + newIsolateScopeDirective = previousCompileContext.newIsolateScopeDirective, + templateDirective = previousCompileContext.templateDirective, $compileNode = templateAttrs.$$element = jqLite(compileNode), directive, directiveName, $template, - transcludeDirective, + transcludeDirective = previousCompileContext.transcludeDirective, replaceDirective = originalReplaceDirective, childTranscludeFn = transcludeFn, controllerDirectives, @@ -815,19 +819,27 @@ function $CompileProvider($provide) { } if (directiveValue = directive.transclude) { - assertNoDuplicate('transclusion', transcludeDirective, directive, $compileNode); - transcludeDirective = directive; + // Special case ngRepeat so that we don't complain about duplicate transclusion, ngRepeat knows how to handle + // this on its own. + if (directiveName !== 'ngRepeat') { + assertNoDuplicate('transclusion', transcludeDirective, directive, $compileNode); + transcludeDirective = directive; + } if (directiveValue == 'element') { terminalPriority = directive.priority; - $template = groupScan(compileNode, attrStart, attrEnd) + $template = groupScan(compileNode, attrStart, attrEnd); $compileNode = templateAttrs.$$element = jqLite(document.createComment(' ' + directiveName + ': ' + templateAttrs[directiveName] + ' ')); compileNode = $compileNode[0]; replaceWith(jqCollection, jqLite(sliceArgs($template)), compileNode); childTranscludeFn = compile($template, transcludeFn, terminalPriority, - replaceDirective && replaceDirective.name); + replaceDirective && replaceDirective.name, { + newIsolateScopeDirective: newIsolateScopeDirective, + transcludeDirective: transcludeDirective, + templateDirective: templateDirective + }); } else { $template = jqLite(JQLiteClone(compileNode)).contents(); $compileNode.html(''); // clear contents @@ -889,7 +901,11 @@ function $CompileProvider($provide) { } nodeLinkFn = compileTemplateUrl(directives.splice(i, directives.length - i), $compileNode, - templateAttrs, jqCollection, childTranscludeFn, preLinkFns, postLinkFns); + templateAttrs, jqCollection, childTranscludeFn, preLinkFns, postLinkFns, { + newIsolateScopeDirective: newIsolateScopeDirective, + transcludeDirective: transcludeDirective, + templateDirective: templateDirective + }); ii = directives.length; } else if (directive.compile) { try { @@ -1188,7 +1204,7 @@ function $CompileProvider($provide) { function compileTemplateUrl(directives, $compileNode, tAttrs, - $rootElement, childTranscludeFn, preLinkFns, postLinkFns) { + $rootElement, childTranscludeFn, preLinkFns, postLinkFns, previousCompileContext) { var linkQueue = [], afterTemplateNodeLinkFn, afterTemplateChildLinkFn, @@ -1231,7 +1247,7 @@ function $CompileProvider($provide) { directives.unshift(derivedSyncDirective); afterTemplateNodeLinkFn = applyDirectivesToNode(directives, compileNode, tAttrs, - childTranscludeFn, $compileNode, origAsyncDirective, preLinkFns, postLinkFns); + childTranscludeFn, $compileNode, origAsyncDirective, preLinkFns, postLinkFns, previousCompileContext); forEach($rootElement, function(node, i) { if (node == compileNode) { $rootElement[i] = $compileNode[0]; diff --git a/test/ng/compileSpec.js b/test/ng/compileSpec.js index 1e6f6e26b592..9a69824aaca3 100755 --- a/test/ng/compileSpec.js +++ b/test/ng/compileSpec.js @@ -1042,10 +1042,12 @@ describe('$compile', function() { templateUrl: 'template.html' })); }); - inject(function($compile){ + inject(function($compile, $httpBackend){ + $httpBackend.whenGET('template.html').respond('

template.html

'); expect(function() { $compile('
'); - }).toThrowMinErr('$compile', 'multidir', 'Multiple directives [sync, async] asking for template on: '+ + $httpBackend.flush(); + }).toThrowMinErr('$compile', 'multidir', 'Multiple directives [async, sync] asking for template on: '+ '
'); }); }); @@ -1205,7 +1207,7 @@ describe('$compile', function() { )); - it('should work when directive is a repeater', inject( + it('should work when directive is in a repeater', inject( function($compile, $httpBackend, $rootScope) { $httpBackend.expect('GET', 'hello.html'). respond('i=;'); @@ -1317,7 +1319,7 @@ describe('$compile', function() { }); - describe('template as function', function() { + describe('templateUrl as function', function() { beforeEach(module(function() { directive('myDirective', valueFn({ @@ -2745,23 +2747,81 @@ describe('$compile', function() { }); - it('should only allow one transclude per element', function() { + it('should only allow one content transclusion per element', function() { module(function() { directive('first', valueFn({ - scope: {}, - restrict: 'CA', - transclude: 'content' + transclude: true })); directive('second', valueFn({ - restrict: 'CA', - transclude: 'content' + transclude: true })); }); inject(function($compile) { expect(function() { - $compile('
'); + $compile('
'); + }).toThrowMinErr('$compile', 'multidir', /Multiple directives \[first, second\] asking for transclusion on:
'); }).toThrowMinErr('$compile', 'multidir', 'Multiple directives [first, second] asking for transclusion on: ' + - '
'); + ''); + }); + }); + + + it('should only allow one element transclusion per element when directives have different priorities', function() { + // we restart compilation in this case and we need to remember the duplicates during the second compile + // regression #3893 + module(function() { + directive('first', valueFn({ + transclude: 'element', + priority: 100 + })); + directive('second', valueFn({ + transclude: 'element' + })); + }); + inject(function($compile) { + expect(function() { + $compile('
'); + }).toThrowMinErr('$compile', 'multidir', /Multiple directives \[first, second\] asking for transclusion on:
template.html

'); + $compile('
'); + expect(function() { + $httpBackend.flush(); + }).toThrowMinErr('$compile', 'multidir', /Multiple directives \[first, second\] asking for transclusion on:

{{i}}
')($rootScope); + $rootScope.$digest(); + expect(element.text()).toBe('[[1]][[2]]') + }); + }); + + + it('should allow mixing ngRepeat with ngInclude', inject(function($compile, $rootScope, $httpBackend) { + $httpBackend.whenGET('someTemplate.html').respond('

some template;

'); + element = $compile('
')($rootScope); + $rootScope.$digest(); + $httpBackend.flush(); + expect(element.text()).toBe('some template; some template; '); + })); + + + it('should allow mixing ngRepeat with ngIf', inject(function($compile, $rootScope) { + element = $compile('
{{i}};
')($rootScope); + $rootScope.$digest(); + expect(element.text()).toBe('2;4;'); + })); + }); + + describe('ngRepeatStart', function () { it('should grow multi-node repeater', inject(function($compile, $rootScope) { $rootScope.show = false;