Skip to content

Commit

Permalink
fix($compile): link async+replace element transclusion directives wit…
Browse files Browse the repository at this point in the history
…h comment element

Previously, element transclusion directives would not receive a comment node in their
link functions when they were the root of an asynchronous replace template. This would
cause duplicate elements to appear in an ng-if expression within ng-repeat.

Closes angular#6006
  • Loading branch information
caitp committed Feb 3, 2014
1 parent 19ba651 commit 48fa08b
Show file tree
Hide file tree
Showing 2 changed files with 68 additions and 5 deletions.
22 changes: 17 additions & 5 deletions src/ng/compile.js
Original file line number Diff line number Diff line change
Expand Up @@ -1662,7 +1662,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {

$http.get($sce.getTrustedResourceUrl(templateUrl), {cache: $templateCache}).
success(function(content) {
var compileNode, tempTemplateAttrs, $template, childBoundTranscludeFn;
var compileNode, tempTemplateAttrs, $template, childBoundTranscludeFn, replaceTransclude;

content = denormalizeTemplate(content);

Expand All @@ -1685,6 +1685,12 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
}
directives = templateDirectives.concat(directives);
mergeTemplateAttributes(tAttrs, tempTemplateAttrs);
for (var i=0, ii=directives.length; i<ii; ++i) {

This comment has been minimized.

Copy link
@IgorMinar

IgorMinar Feb 4, 2014

this seems to duplicate what previousCompileContext is for. though I think that we'd need to erase hasElementTranscludeDirective from previousCompileContext before using it again when the template returns. or maybe not? not sure right now.

This comment has been minimized.

Copy link
@caitp

caitp Feb 4, 2014

Author Owner

Ah that's true, I've updated to get hasElementTranscludeDirective added to previousCompileContext, it seems to work using that.

if (directives[i].transclude === 'element') {
replaceTransclude = true;
break;
}
}
} else {
compileNode = beforeTemplateCompileNode;
$compileNode.html(content);
Expand Down Expand Up @@ -1712,12 +1718,18 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {

if (beforeTemplateLinkNode !== beforeTemplateCompileNode) {
var oldClasses = beforeTemplateLinkNode.className;
// it was cloned therefore we have to clone as well.
linkNode = jqLiteClone(compileNode);

if (!replaceTransclude) {
// it was cloned therefore we have to clone as well.
linkNode = jqLiteClone(compileNode);
}

replaceWith(linkRootElement, jqLite(beforeTemplateLinkNode), linkNode);

// Copy in CSS classes from original node
safeAddClass(jqLite(linkNode), oldClasses);
if (isDefined(oldClasses)) {

This comment has been minimized.

Copy link
@IgorMinar

IgorMinar Feb 4, 2014

is this related to this bug?

This comment has been minimized.

Copy link
@caitp

caitp Feb 4, 2014

Author Owner

true, removed it

// Copy in CSS classes from original node
safeAddClass(jqLite(linkNode), oldClasses);
}
}
if (afterTemplateNodeLinkFn.transclude) {
childBoundTranscludeFn = createBoundTranscludeFn(scope, afterTemplateNodeLinkFn.transclude);
Expand Down
51 changes: 51 additions & 0 deletions test/ng/compileSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -3972,6 +3972,57 @@ describe('$compile', function() {
});
});

// issue #6006
it('should link directive with $element as a comment node', function() {
module(function($provide) {
directive('innerAgain', function(log) {
return {
transclude: 'element',
link: function(scope, element, attr, controllers, transclude) {
log('innerAgain:'+lowercase(nodeName_(element))+':'+trim(element[0].data));
transclude(scope, function(clone) {
element.parent().append(clone);
});
}
};
});
directive('inner', function(log) {
return {
replace: true,
templateUrl: 'inner.html',
link: function(scope, element) {
log('inner:'+lowercase(nodeName_(element))+':'+trim(element[0].data));
}
};
});
directive('outer', function(log) {
return {
transclude: 'element',
link: function(scope, element, attrs, controllers, transclude) {
log('outer:'+lowercase(nodeName_(element))+':'+trim(element[0].data));
transclude(scope, function(clone) {
element.parent().append(clone);
});
}
};
});
});
inject(function(log, $compile, $rootScope, $templateCache) {
$templateCache.put('inner.html', '<div inner-again><p>Content</p></div>');
element = $compile('<div><div outer><div inner></div></div></div>')($rootScope);
$rootScope.$digest();
var child = element.children();

expect(log.toArray()).toEqual([
"outer:#comment:outer:",
"innerAgain:#comment:innerAgain:",
"inner:#comment:innerAgain:"]);
expect(child.length).toBe(1);
expect(child.contents().length).toBe(2);
expect(lowercase(nodeName_(child.contents().eq(0)))).toBe('#comment');
expect(lowercase(nodeName_(child.contents().eq(1)))).toBe('div');
});
});
});

it('should safely create transclude comment node and not break with "-->"',
Expand Down

0 comments on commit 48fa08b

Please sign in to comment.