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
Prev Previous commit
Transclusion fix update
  • Loading branch information
@mzdunek93
mzdunek93 committed Oct 6, 2015
commit 2182c879b6c41ac44014ebfb0ee2bb9065a870df
9 changes: 5 additions & 4 deletions src/ng/compile.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1661,7 +1661,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
* @param previousCompileContext
* @returns {Function}
*/
function compilationGenerator(eager, $compileNodes, transcludeFn, changeOuterScope, maxPriority, ignoreDirective, previousCompileContext) {
function compilationGenerator(eager, $compileNodes, transcludeFn, maxPriority, changeOuterScope, ignoreDirective, previousCompileContext) {
if (eager) {
return compile($compileNodes, transcludeFn, maxPriority, changeOuterScope, ignoreDirective, previousCompileContext);
}
Expand Down Expand Up @@ -1821,8 +1821,8 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
compileNode = $compileNode[0];
replaceWith(jqCollection, sliceArgs($template), compileNode);

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

Expand Down
33 changes: 31 additions & 2 deletions test/ng/compileSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5632,7 +5632,7 @@ 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({
directive('isolate', valueFn({
scope: {},
replace: true,
template: '<div trans>{{x}}</div>',
Expand All @@ -5650,14 +5650,43 @@ describe('$compile', function() {
}));
});
inject(function($rootScope, $compile) {
element = $compile('<parent></parent>')($rootScope);
element = $compile('<isolate></isolate>')($rootScope);
$rootScope.x = 'root';
$rootScope.$apply();
expect(element.text()).toEqual('iso');
});
});


//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 with child scope', function() {
module(function() {
directive('child', valueFn({
scope: true,
replace: true,
template: '<div trans>{{x}}</div>',
link: function(scope, element, attr, ctrl) {
scope.x = 'child';
}
}));
directive('trans', valueFn({
transclude: 'content',
link: function(scope, element, attr, ctrl, $transclude) {
$transclude(function(clone) {
element.append(clone);
});
}
}));
});
inject(function($rootScope, $compile) {
element = $compile('<child></child>')($rootScope);
$rootScope.x = 'root';
$rootScope.$apply();
expect(element.text()).toEqual('child');
});
});


it('should not leak if two "element" transclusions are on the same element (with debug info)', function() {
if (jQuery) {
// jQuery 2.x doesn't expose the cache storage.
Expand Down