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

Nested transclude issue #7387

Closed
wants to merge 1 commit into from
Closed

Nested transclude issue #7387

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
23 changes: 14 additions & 9 deletions src/ng/compile.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -941,7 +941,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
return linkFnFound ? compositeLinkFn : null;

function compositeLinkFn(scope, nodeList, $rootElement, boundTranscludeFn) {
var nodeLinkFn, childLinkFn, node, $node, childScope, childTranscludeFn, i, ii, n;
var nodeLinkFn, childLinkFn, node, $node, childScope, i, ii, n;

// copy nodeList so that linking doesn't break due to live list updates.
var nodeListLength = nodeList.length,
Expand All @@ -963,14 +963,17 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
} else {
childScope = scope;
}
childTranscludeFn = nodeLinkFn.transclude;
if (childTranscludeFn || (!boundTranscludeFn && transcludeFn)) {
nodeLinkFn(childLinkFn, childScope, node, $rootElement,
createBoundTranscludeFn(scope, childTranscludeFn || transcludeFn)
);
} else {
nodeLinkFn(childLinkFn, childScope, node, $rootElement, boundTranscludeFn);

// We need to create a new boundTranscludeFn if
// - a directive on this element wants to transclude
// or
// - there is no boundTranscludeFn already and a transcludeFn was passed in
if ( nodeLinkFn.transcludeOnThisElement || (!boundTranscludeFn && transcludeFn) ) {
boundTranscludeFn = createBoundTranscludeFn(scope, nodeLinkFn.transclude || transcludeFn);
}

nodeLinkFn(childLinkFn, childScope, node, $rootElement, boundTranscludeFn);

} else if (childLinkFn) {
childLinkFn(scope, node.childNodes, undefined, boundTranscludeFn);
}
Expand Down Expand Up @@ -1346,7 +1349,9 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
}
Copy link
Contributor

Choose a reason for hiding this comment

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

@petebacondarwin shouldn't this depend on whether there is a template directive or not?
If there is a template directive, then we don't pass the transclusion, right?

At least that was what we came up with, I think...
vojtajina@e10bd88#diff-a732922b631efed1b9f33a24082ae0dbR1335

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 moved this check further up the diff

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vojtajina - sorry about the short answer I was on my phone.

I looked through the logic and I am pretty sure that the logic here works out the same as our original diff.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

      if (hasTranscludeDirective) {
        nodeLinkFn.transcludeOnThisElement = true;
      } else if (!templateDirective) {
        nodeLinkFn.transcludeOnThisElement = false;
      }

is equivalent to

nodeLinkFn.transcludeOnThisElement = hasTranscludeElement

since the default of this property is undefined (i.e. falsy)

Copy link
Contributor

Choose a reason for hiding this comment

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

The original code does not set nodeLinkFn.transclude if !hasTransclude && templateDirective.

Copy link
Contributor

Choose a reason for hiding this comment

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

@petebacondarwin Oh, thanks. Now I understand that the code does the same thing. You are right.

I'm still however not sure if that's correct, as template SHOULD be deciding factor, no?

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean, I understand that the code change you did, does not change any behavior, but I'm still questioning whether it is the correct behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I am willing to agree

Copy link
Contributor

Choose a reason for hiding this comment

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

This is what I was talking about. I think this failing test shows the problem:
vojtajina@4d152c8

Copy link
Contributor

Choose a reason for hiding this comment

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

We can fix that in a separate PR though.


nodeLinkFn.scope = newScopeDirective && newScopeDirective.scope === true;
nodeLinkFn.transclude = hasTranscludeDirective && childTranscludeFn;
nodeLinkFn.transcludeOnThisElement = hasTranscludeDirective;
nodeLinkFn.transclude = childTranscludeFn;

previousCompileContext.hasElementTranscludeDirective = hasElementTranscludeDirective;

// might be normal or delayed nodeLinkFn depending on if templateUrl is present
Expand Down
72 changes: 72 additions & 0 deletions test/ng/compileSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1497,6 +1497,78 @@ describe('$compile', function() {
));


describe('nested transcludes', function() {

beforeEach(module(function($compileProvider) {

$compileProvider.directive('noop', valueFn({}));

$compileProvider.directive('sync', valueFn({
template: '<div ng-transclude></div>',
transclude: true
}));

$compileProvider.directive('async', valueFn({
templateUrl: 'async',
transclude: true
}));

$compileProvider.directive('syncSync', valueFn({
template: '<div noop><div sync><div ng-transclude></div></div></div>',
transclude: true
}));

$compileProvider.directive('syncAsync', valueFn({
template: '<div noop><div async><div ng-transclude></div></div></div>',
transclude: true
}));

$compileProvider.directive('asyncSync', valueFn({
templateUrl: 'asyncSync',
transclude: true
}));

$compileProvider.directive('asyncAsync', valueFn({
templateUrl: 'asyncAsync',
transclude: true
}));

}));

beforeEach(inject(function($templateCache) {
$templateCache.put('async', '<div ng-transclude></div>');
$templateCache.put('asyncSync', '<div noop><div sync><div ng-transclude></div></div></div>');
$templateCache.put('asyncAsync', '<div noop><div async><div ng-transclude></div></div></div>');
}));


it('should allow nested transclude directives with sync template containing sync template', inject(function($compile, $rootScope) {
element = $compile('<div sync-sync>transcluded content</div>')($rootScope);
$rootScope.$digest();
expect(element.text().trim()).toEqual('transcluded content');
}));

it('should allow nested transclude directives with sync template containing async template', inject(function($compile, $rootScope) {
element = $compile('<div sync-async>transcluded content</div>')($rootScope);
$rootScope.$digest();
expect(element.text().trim()).toEqual('transcluded content');
}));

it('should allow nested transclude directives with async template containing sync template', inject(function($compile, $rootScope) {
element = $compile('<div async-sync>transcluded content</div>')($rootScope);
$rootScope.$digest();
expect(element.text().trim()).toEqual('transcluded content');
}));

it('should allow nested transclude directives with async template containing asynch template', inject(function($compile, $rootScope) {
element = $compile('<div async-async>transcluded content</div>')($rootScope);
$rootScope.$digest();
expect(element.text().trim()).toEqual('transcluded content');
}));
});



it("should fail if replacing and template doesn't have a single root element", function() {
module(function($exceptionHandlerProvider) {
$exceptionHandlerProvider.mode('log');
Expand Down