Skip to content

Commit

Permalink
fix($compile): prevent infinite loop w/ replace+transclude directives
Browse files Browse the repository at this point in the history 
Previously if a template contained a directive that had a template
(sync or async) and the directive template was to replace the original
element and the directive template contained another directive on the
root element of this template and this new directive was an element
transclude directive then an infinite recursion would follow because
the compiler kept on re-adding and reapplying the original directive
to the replaced node.

This change fixes that.

Closes angular#2155
  • Loading branch information
@IgorMinar @ctrahey
IgorMinar authored and ctrahey committed Jul 22, 2013
1 parent 9835d72 commit 75186b3
Show file tree
Hide file tree
Showing 2 changed files with 84 additions and 18 deletions.
45 changes: 27 additions & 18 deletions src/ng/compile.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -362,7 +362,7 @@ function $CompileProvider($provide) {

//================================

function compile($compileNodes, transcludeFn, maxPriority) {
function compile($compileNodes, transcludeFn, maxPriority, ignoreDirective) {
if (!($compileNodes instanceof jqLite)) {
// jquery always rewraps, whereas we need to preserve the original selector so that we can modify it.
$compileNodes = jqLite($compileNodes);
Expand All @@ -375,7 +375,7 @@ function $CompileProvider($provide) {
$compileNodes[index] = node = jqLite(node).wrap('<span></span>').parent()[0];
}
});
var compositeLinkFn = compileNodes($compileNodes, transcludeFn, $compileNodes, maxPriority);
var compositeLinkFn = compileNodes($compileNodes, transcludeFn, $compileNodes, maxPriority, ignoreDirective);
return function publicLinkFn(scope, cloneConnectFn){
assertArg(scope, 'scope');
// important!!: we must call our jqLite.clone() since the jQuery one is trying to be smart
Expand Down Expand Up @@ -422,15 +422,15 @@ 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) {
function compileNodes(nodeList, transcludeFn, $rootElement, maxPriority, ignoreDirective) {
var linkFns = [],
nodeLinkFn, childLinkFn, directives, attrs, linkFnFound;

for(var i = 0; i < nodeList.length; i++) {
attrs = new Attributes();

// we must always refer to nodeList[i] since the nodes can be replaced underneath us.
directives = collectDirectives(nodeList[i], [], attrs, i == 0 ? maxPriority : undefined);
directives = collectDirectives(nodeList[i], [], attrs, i == 0 ? maxPriority : undefined, ignoreDirective);

nodeLinkFn = (directives.length)
? applyDirectivesToNode(directives, nodeList[i], attrs, transcludeFn, $rootElement)
Expand Down Expand Up @@ -504,7 +504,7 @@ function $CompileProvider($provide) {
* @param attrs The shared attrs object which is used to populate the normalized attributes.
* @param {number=} maxPriority Max directive priority.
*/
function collectDirectives(node, directives, attrs, maxPriority) {
function collectDirectives(node, directives, attrs, maxPriority, ignoreDirective) {
var nodeType = node.nodeType,
attrsMap = attrs.$attr,
match,
Expand All @@ -514,7 +514,7 @@ function $CompileProvider($provide) {
case 1: /* Element */
// use the node name: <directive>
addDirective(directives,
directiveNormalize(nodeName_(node).toLowerCase()), 'E', maxPriority);
directiveNormalize(nodeName_(node).toLowerCase()), 'E', maxPriority, ignoreDirective);

// iterate over the attributes
for (var attr, name, nName, ngAttrName, value, nAttrs = node.attributes,
Expand Down Expand Up @@ -545,7 +545,7 @@ function $CompileProvider($provide) {
attrs[nName] = true; // presence means true
}
addAttrInterpolateDirective(node, directives, value, nName);
addDirective(directives, nName, 'A', maxPriority, attrStartName, attrEndName);
addDirective(directives, nName, 'A', maxPriority, ignoreDirective, attrStartName, attrEndName);
}
}

Expand All @@ -554,7 +554,7 @@ function $CompileProvider($provide) {
if (isString(className) && className !== '') {
while (match = CLASS_DIRECTIVE_REGEXP.exec(className)) {
nName = directiveNormalize(match[2]);
if (addDirective(directives, nName, 'C', maxPriority)) {
if (addDirective(directives, nName, 'C', maxPriority, ignoreDirective)) {
attrs[nName] = trim(match[3]);
}
className = className.substr(match.index + match[0].length);
Expand All @@ -569,7 +569,7 @@ function $CompileProvider($provide) {
match = COMMENT_DIRECTIVE_REGEXP.exec(node.nodeValue);
if (match) {
nName = directiveNormalize(match[1]);
if (addDirective(directives, nName, 'M', maxPriority)) {
if (addDirective(directives, nName, 'M', maxPriority, ignoreDirective)) {
attrs[nName] = trim(match[2]);
}
}
Expand Down Expand Up @@ -643,7 +643,7 @@ function $CompileProvider($provide) {
* argument has the root jqLite array so that we can replace nodes on it.
* @returns linkFn
*/
function applyDirectivesToNode(directives, compileNode, templateAttrs, transcludeFn, jqCollection) {
function applyDirectivesToNode(directives, compileNode, templateAttrs, transcludeFn, jqCollection, originalReplaceDirective) {
var terminalPriority = -Number.MAX_VALUE,
preLinkFns = [],
postLinkFns = [],
Expand All @@ -655,6 +655,7 @@ function $CompileProvider($provide) {
directiveName,
$template,
transcludeDirective,
replaceDirective = originalReplaceDirective,
childTranscludeFn = transcludeFn,
controllerDirectives,
linkFn,
Expand Down Expand Up @@ -705,7 +706,9 @@ function $CompileProvider($provide) {
jqLite(document.createComment(' ' + directiveName + ': ' + templateAttrs[directiveName] + ' '));
compileNode = $compileNode[0];
replaceWith(jqCollection, jqLite(sliceArgs($template)), compileNode);
childTranscludeFn = compile($template, transcludeFn, terminalPriority);

childTranscludeFn = compile($template, transcludeFn, terminalPriority,
replaceDirective && replaceDirective.name);
} else {
$template = jqLite(JQLiteClone(compileNode)).contents();
$compileNode.html(''); // clear contents
Expand All @@ -724,6 +727,7 @@ function $CompileProvider($provide) {
directiveValue = denormalizeTemplate(directiveValue);

if (directive.replace) {
replaceDirective = directive;
$template = jqLite('<div>' +
trim(directiveValue) +
'</div>').contents();
Expand Down Expand Up @@ -760,9 +764,12 @@ function $CompileProvider($provide) {
if (directive.templateUrl) {
assertNoDuplicate('template', templateDirective, directive, $compileNode);
templateDirective = directive;

if (directive.replace) {
replaceDirective = directive;
}
nodeLinkFn = compileTemplateUrl(directives.splice(i, directives.length - i),
nodeLinkFn, $compileNode, templateAttrs, jqCollection, directive.replace,
childTranscludeFn);
nodeLinkFn, $compileNode, templateAttrs, jqCollection, childTranscludeFn);
ii = directives.length;
} else if (directive.compile) {
try {
Expand Down Expand Up @@ -978,7 +985,8 @@ function $CompileProvider($provide) {
* * `M`: comment
* @returns true if directive was added.
*/
function addDirective(tDirectives, name, location, maxPriority, startAttrName, endAttrName) {
function addDirective(tDirectives, name, location, maxPriority, ignoreDirective, startAttrName, endAttrName) {
if (name === ignoreDirective) return null;
var match = null;
if (hasDirectives.hasOwnProperty(name)) {
for(var directive, directives = $injector.get(name + Suffix),
Expand Down Expand Up @@ -1039,15 +1047,15 @@ function $CompileProvider($provide) {


function compileTemplateUrl(directives, beforeTemplateNodeLinkFn, $compileNode, tAttrs,
$rootElement, replace, childTranscludeFn) {
$rootElement, childTranscludeFn) {
var linkQueue = [],
afterTemplateNodeLinkFn,
afterTemplateChildLinkFn,
beforeTemplateCompileNode = $compileNode[0],
origAsyncDirective = directives.shift(),
// The fact that we have to copy and patch the directive seems wrong!
derivedSyncDirective = extend({}, origAsyncDirective, {
controller: null, templateUrl: null, transclude: null, scope: null
controller: null, templateUrl: null, transclude: null, scope: null, replace: null
}),
templateUrl = (isFunction(origAsyncDirective.templateUrl))
? origAsyncDirective.templateUrl($compileNode, tAttrs)
Expand All @@ -1061,7 +1069,7 @@ function $CompileProvider($provide) {

content = denormalizeTemplate(content);

if (replace) {
if (origAsyncDirective.replace) {
$template = jqLite('<div>' + trim(content) + '</div>').contents();
compileNode = $template[0];

Expand All @@ -1080,7 +1088,8 @@ function $CompileProvider($provide) {
}

directives.unshift(derivedSyncDirective);
afterTemplateNodeLinkFn = applyDirectivesToNode(directives, compileNode, tAttrs, childTranscludeFn, $compileNode);

afterTemplateNodeLinkFn = applyDirectivesToNode(directives, compileNode, tAttrs, childTranscludeFn, $compileNode, origAsyncDirective);
forEach($rootElement, function(node, i) {
if (node == compileNode) {
$rootElement[i] = $compileNode[0];
Expand Down
57 changes: 57 additions & 0 deletions test/ng/directive/ngRepeatSpec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -454,6 +454,63 @@ describe('ngRepeat', function() {

describe('nesting in replaced directive templates', function() {

it('should work when placed on a non-root element of attr directive with SYNC replaced template',
inject(function($templateCache, $compile, $rootScope) {
$compileProvider.directive('rr', function() {
return {
restrict: 'A',
replace: true,
template: '<div ng-repeat="i in items">{{i}}|</div>'
};
});
element = jqLite('<div><span rr>{{i}}|</span></div>');
$compile(element)($rootScope);
$rootScope.$apply();
expect(element.text()).toBe('');

$rootScope.items = [1, 2];
$rootScope.$apply();
expect(element.text()).toBe('1|2|');
expect(sortedHtml(element)).toBe(
'<div>' +
'<!-- ngRepeat: i in items -->' +
'<div ng-repeat="i in items" rr="">1|</div>' +
'<div ng-repeat="i in items" rr="">2|</div>' +
'</div>'
);
}));


it('should work when placed on a non-root element of attr directive with ASYNC replaced template',
inject(function($templateCache, $compile, $rootScope) {
$compileProvider.directive('rr', function() {
return {
restrict: 'A',
replace: true,
templateUrl: 'rr.html'
};
});

$templateCache.put('rr.html', '<div ng-repeat="i in items">{{i}}|</div>');

element = jqLite('<div><span rr>{{i}}|</span></div>');
$compile(element)($rootScope);
$rootScope.$apply();
expect(element.text()).toBe('');

$rootScope.items = [1, 2];
$rootScope.$apply();
expect(element.text()).toBe('1|2|');
expect(sortedHtml(element)).toBe(
'<div>' +
'<!-- ngRepeat: i in items -->' +
'<div ng-repeat="i in items" rr="">1|</div>' +
'<div ng-repeat="i in items" rr="">2|</div>' +
'</div>'
);
}));


it('should work when placed on a root element of attr directive with SYNC replaced template',
inject(function($templateCache, $compile, $rootScope) {
$compileProvider.directive('replaceMeWithRepeater', function() {
Expand Down

0 comments on commit 75186b3

Please sign in to comment.