Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

fix($compile): fix scoping for transclusion #12975

Closed
wants to merge 2 commits into from
Closed

fix($compile): fix scoping for transclusion #12975

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
908fdae
fix($compile): fix scoping for transclusion
mzdunek93 Sep 28, 2015
2182c87
Transclusion fix update
mzdunek93 Oct 6, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 27 additions & 7 deletions src/ng/compile.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1270,7 +1270,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {

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

function compile($compileNodes, transcludeFn, maxPriority, ignoreDirective,
function compile($compileNodes, transcludeFn, maxPriority, changeOuterScope, ignoreDirective,
previousCompileContext) {
if (!($compileNodes instanceof jqLite)) {
// jquery always rewraps, whereas we need to preserve the original selector so that we can
Expand All @@ -1292,6 +1292,10 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
return function publicLinkFn(scope, cloneConnectFn, options) {
assertArg(scope, 'scope');

if (changeOuterScope) {
scope = scope.$parent.$new(false);
}

options = options || {};
var parentBoundTranscludeFn = options.parentBoundTranscludeFn,
transcludeControllers = options.transcludeControllers,
Expand Down Expand Up @@ -1657,16 +1661,16 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
* @param previousCompileContext
* @returns {Function}
*/
function compilationGenerator(eager, $compileNodes, transcludeFn, maxPriority, ignoreDirective, previousCompileContext) {
function compilationGenerator(eager, $compileNodes, transcludeFn, changeOuterScope, maxPriority, ignoreDirective, previousCompileContext) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it looks like the order of maxPriority and changeOuterScope is wrong

if (eager) {
return compile($compileNodes, transcludeFn, maxPriority, ignoreDirective, previousCompileContext);
return compile($compileNodes, transcludeFn, maxPriority, changeOuterScope, ignoreDirective, previousCompileContext);
}

var compiled;

return function() {
if (!compiled) {
compiled = compile($compileNodes, transcludeFn, maxPriority, ignoreDirective, previousCompileContext);
compiled = compile($compileNodes, transcludeFn, maxPriority, changeOuterScope, ignoreDirective, previousCompileContext);

// Null out all of these references in order to make them eligible for garbage collection
// since this is a potentially long lived closure
Expand Down Expand Up @@ -1706,6 +1710,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
previousCompileContext = previousCompileContext || {};

var terminalPriority = -Number.MAX_VALUE,
changeOuterScope,
newScopeDirective = previousCompileContext.newScopeDirective,
controllerDirectives = previousCompileContext.controllerDirectives,
newIsolateScopeDirective = previousCompileContext.newIsolateScopeDirective,
Expand Down Expand Up @@ -1796,6 +1801,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {

if (directiveValue = directive.transclude) {
hasTranscludeDirective = true;
changeOuterScope = directive.$$replacing;

// 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 safely handle element transclusion,
Expand All @@ -1815,8 +1821,8 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
compileNode = $compileNode[0];
replaceWith(jqCollection, sliceArgs($template), compileNode);

childTranscludeFn = compilationGenerator(mightHaveMultipleTransclusionError, $template, transcludeFn, terminalPriority,
replaceDirective && replaceDirective.name, {
childTranscludeFn = compilationGenerator(mightHaveMultipleTransclusionError, $template, transcludeFn, changeOuterScope,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the order of changeOuterScope and terminalPriority is wrong

[I do see that the two order errors cancel each other, but it is better if the order is respected]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did it so that I didn't have to explicitly pass undefined for maxPriority to compilationGenerator instead of just omitting the argument, but I can fix that.

terminalPriority, replaceDirective && replaceDirective.name, {
// Don't pass in:
// - controllerDirectives - otherwise we'll create duplicates controllers
// - newIsolateScopeDirective or templateDirective - combining templates with
Expand All @@ -1829,7 +1835,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
} else {
$template = jqLite(jqLiteClone(compileNode)).contents();
$compileNode.empty(); // clear contents
childTranscludeFn = compilationGenerator(mightHaveMultipleTransclusionError, $template, transcludeFn);
childTranscludeFn = compilationGenerator(mightHaveMultipleTransclusionError, $template, transcludeFn, changeOuterScope);
}
}

Expand Down Expand Up @@ -1874,6 +1880,11 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
if (newIsolateScopeDirective) {
markDirectivesAsIsolate(templateDirectives);
}

if (newScopeDirective || newIsolateScopeDirective) {
markDirectivesAsReplacing(templateDirectives);
}

directives = directives.concat(templateDirectives).concat(unprocessedDirectives);
mergeTemplateAttributes(templateAttrs, newTemplateAttrs);

Expand Down Expand Up @@ -2162,6 +2173,15 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
}
}

function markDirectivesAsReplacing(directives) {
// mark transcluding directives as replacing, so that their outer scope is changed to the directive's scope
for (var j = 0, jj = directives.length; j < jj; j++) {
if (directives[j].transclude) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be if (directive[j].transclude === 'element') { ?

Mmm... thinking about this again, it looks like the transclude === 'element' option is not handled by this PR. This is still ok, as it looks like it handle the case of transclude === true properly

directives[j] = inherit(directives[j], {$$replacing: true});
}
}
}

/**
* looks up the directive and decorates it with exception handling and proper parameters. We
* call this the boundDirective.
Expand Down
28 changes: 28 additions & 0 deletions test/ng/compileSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5629,6 +5629,34 @@ describe('$compile', function() {
});


//see issue https://github.com/angular/angular.js/issues/12936
it('should use the proper scope when being the root element of a replaced directive', function() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for completeness, would it be possible to have the same test with templateUrl ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's necessary, this bug didn't appear for templateUrl, since it loaded and compiled the template separately.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree we probably don't need a test for templateUrl but what we do need a test for is child scope (i.e. not isolated scope), since this is a common failing case (e.g. ngIf or ngRepeat at the root of the template).

module(function() {
directive('parent', valueFn({
scope: {},
replace: true,
template: '<div trans>{{x}}</div>',
link: function(scope, element, attr, ctrl) {
scope.x = 'iso';
}
}));
directive('trans', valueFn({
transclude: 'content',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

transclude: 'content' is not a supported value (even when it fallbacks to true

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I copied it from the other tests.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lgalfaso : Originally the possible values for transclude were indeed "content" and "element" but it got shorthanded to true and "element". So this is a valid value.

link: function(scope, element, attr, ctrl, $transclude) {
$transclude(function(clone) {
element.append(clone);
});
}
}));
});
inject(function($rootScope, $compile) {
element = $compile('<parent></parent>')($rootScope);
$rootScope.x = 'root';
$rootScope.$apply();
expect(element.text()).toEqual('iso');
});
});


it('should not leak if two "element" transclusions are on the same element (with debug info)', function() {
if (jQuery) {
Expand Down