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

Nested transclusion issues #7565

Closed
wants to merge 10 commits into from
Closed

Nested transclusion issues #7565

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
a5ca9f7
style($compileSpec): fix typos
petebacondarwin May 23, 2014
d48d129
fix($compile): pass transcludeFn down to nested transclude directives
petebacondarwin May 23, 2014
bcab5ac
refactor($compile): change parameter name
petebacondarwin May 23, 2014
83194e6
fix($compile): fix nested isolated transclude directives
petebacondarwin May 23, 2014
120c592
refactor($compile): no need to use bind
petebacondarwin May 23, 2014
79674f9
fix($compile): don't pass transclude to template of non-transclude di…
petebacondarwin May 23, 2014
dfc3e81
fix(ngIf): ensure that the correct (transcluded) scope is used
petebacondarwin May 23, 2014
cd11938
fix(ngRepeat): ensure that the correct (transcluded) scope is used
petebacondarwin May 23, 2014
cd98b7a
style(ngRepeat): jshint was complaining about var names
petebacondarwin May 23, 2014
de29c24
fix($compile): don't pass transcludes to non-transclude templateUrl d…
petebacondarwin May 26, 2014
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
58 changes: 42 additions & 16 deletions src/ng/compile.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -924,7 +924,9 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
!childNodes.length)
? null
: compileNodes(childNodes,
nodeLinkFn ? nodeLinkFn.transclude : transcludeFn);
nodeLinkFn ? (
(nodeLinkFn.transcludeOnThisElement || !nodeLinkFn.templateOnThisElement)
&& nodeLinkFn.transclude) : transcludeFn);

linkFns.push(nodeLinkFn, childLinkFn);
linkFnFound = linkFnFound || nodeLinkFn || childLinkFn;
Expand All @@ -935,8 +937,8 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
// return a linking function if we have found anything, null otherwise
return linkFnFound ? compositeLinkFn : null;

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

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

if ( nodeLinkFn.transcludeOnThisElement ) {
childBoundTranscludeFn = createBoundTranscludeFn(scope, nodeLinkFn.transclude, parentBoundTranscludeFn);

} else if (!nodeLinkFn.templateOnThisElement && parentBoundTranscludeFn) {
childBoundTranscludeFn = parentBoundTranscludeFn;

} else if (!parentBoundTranscludeFn && transcludeFn) {
childBoundTranscludeFn = createBoundTranscludeFn(scope, transcludeFn);

} else {
nodeLinkFn(childLinkFn, childScope, node, $rootElement, boundTranscludeFn);
childBoundTranscludeFn = null;
}

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

} else if (childLinkFn) {
childLinkFn(scope, node.childNodes, undefined, boundTranscludeFn);
childLinkFn(scope, node.childNodes, undefined, parentBoundTranscludeFn);
}
}
}
}

function createBoundTranscludeFn(scope, transcludeFn) {
return function boundTranscludeFn(transcludedScope, cloneFn, controllers) {
function createBoundTranscludeFn(scope, transcludeFn, previousBoundTranscludeFn) {

// If there is a previous boundTransclude function and it has a transclusionScope then
// use this instead of the current scope
scope = previousBoundTranscludeFn && previousBoundTranscludeFn.transclusionScope || scope;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm afraid this fix is wrong. See this failing test: https://github.com/vojtajina/angular.js/tree/pr-7565-incorrect-transclusion-scope

I need to investigate this more to figure out some better way how to fix this.

Copy link
Contributor

Choose a reason for hiding this comment

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

I pushed a fix into the https://github.com/vojtajina/angular.js/tree/pr-7565-incorrect-transclusion-scope branch.


var boundTranscludeFn = function(transcludedScope, cloneFn, controllers) {
var scopeCreated = false;

if (!transcludedScope) {
Expand All @@ -985,10 +1000,15 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {

var clone = transcludeFn(transcludedScope, cloneFn, controllers);
if (scopeCreated) {
clone.on('$destroy', bind(transcludedScope, transcludedScope.$destroy));
clone.on('$destroy', function() { transcludedScope.$destroy(); });
}
return clone;
};

// Store the transclusionScope for nested transclusions
boundTranscludeFn.transclusionScope = scope;

return boundTranscludeFn;
}

/**
Expand Down Expand Up @@ -1166,6 +1186,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
templateDirective = previousCompileContext.templateDirective,
nonTlbTranscludeDirective = previousCompileContext.nonTlbTranscludeDirective,
hasTranscludeDirective = false,
hasTemplate = false,
hasElementTranscludeDirective = previousCompileContext.hasElementTranscludeDirective,
$compileNode = templateAttrs.$$element = jqLite(compileNode),
directive,
Expand Down Expand Up @@ -1256,6 +1277,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
}

if (directive.template) {
hasTemplate = true;
assertNoDuplicate('template', templateDirective, directive, $compileNode);
templateDirective = directive;

Expand Down Expand Up @@ -1305,6 +1327,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
}

if (directive.templateUrl) {
hasTemplate = true;
assertNoDuplicate('template', templateDirective, directive, $compileNode);
templateDirective = directive;

Expand All @@ -1313,7 +1336,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
}

nodeLinkFn = compileTemplateUrl(directives.splice(i, directives.length - i), $compileNode,
templateAttrs, jqCollection, childTranscludeFn, preLinkFns, postLinkFns, {
templateAttrs, jqCollection, hasTranscludeDirective && childTranscludeFn, preLinkFns, postLinkFns, {
controllerDirectives: controllerDirectives,
newIsolateScopeDirective: newIsolateScopeDirective,
templateDirective: templateDirective,
Expand Down Expand Up @@ -1341,7 +1364,10 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
}

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

previousCompileContext.hasElementTranscludeDirective = hasElementTranscludeDirective;

// might be normal or delayed nodeLinkFn depending on if templateUrl is present
Expand Down Expand Up @@ -1760,7 +1786,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
safeAddClass(jqLite(linkNode), oldClasses);
}
if (afterTemplateNodeLinkFn.transclude) {
childBoundTranscludeFn = createBoundTranscludeFn(scope, afterTemplateNodeLinkFn.transclude);
childBoundTranscludeFn = createBoundTranscludeFn(scope, afterTemplateNodeLinkFn.transclude, boundTranscludeFn);
} else {
childBoundTranscludeFn = boundTranscludeFn;
}
Expand Down
4 changes: 2 additions & 2 deletions src/ng/directive/ngIf.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,8 +89,8 @@ var ngIfDirective = ['$animate', function($animate) {

if (toBoolean(value)) {
if (!childScope) {
childScope = $scope.$new();
$transclude(childScope, function (clone) {
$transclude(function (clone, newScope) {
childScope = newScope;
clone[clone.length++] = document.createComment(' end ngIf: ' + $attr.ngIf + ' ');
// Note: We only need the first/last node of the cloned nodes.
// However, we need to keep the reference to the jqlite wrapper as it might be changed later
Expand Down
45 changes: 20 additions & 25 deletions src/ng/directive/ngRepeat.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,7 +264,6 @@ var ngRepeatDirective = ['$parse', '$animate', function($parse, $animate) {
// lastBlockMap on the next iteration.
nextBlockMap = {},
arrayLength,
childScope,
key, value, // key/value of iteration
trackById,
trackByIdFn,
Expand All @@ -273,6 +272,17 @@ var ngRepeatDirective = ['$parse', '$animate', function($parse, $animate) {
nextBlockOrder = [],
elementsToRemove;

var updateScope = function(scope, index) {
scope[valueIdentifier] = value;
if (keyIdentifier) scope[keyIdentifier] = key;
scope.$index = index;
scope.$first = (index === 0);
scope.$last = (index === (arrayLength - 1));
scope.$middle = !(scope.$first || scope.$last);
// jshint bitwise: false
scope.$odd = !(scope.$even = (index&1) === 0);
// jshint bitwise: 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.

@vojtajina - This function was originally just a block of code. It could still be as it is only called once. Should we revert that, since I guess that creating a function in here is a performance cost?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, inlining might be better.

Copy link
Contributor

Choose a reason for hiding this comment

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

There is more important issue however - this is the reason why the google internal tests are failing (it's a regression in Angular).

The ng-repeat state is set AFTER the transclusion is linked. That is wrong. It's pretty scary that we don't have any test for this (all the existing tests assert through binding which eventually contains the value even if the state is set later). This branch shows the problem (failing test) including possible fix (the test is passing): https://github.com/vojtajina/angular.js/tree/pr-7565-ng-repeat-state-set-after-transclusion-issue


if (isArrayLike(collection)) {
collectionKeys = collection;
Expand All @@ -281,9 +291,9 @@ var ngRepeatDirective = ['$parse', '$animate', function($parse, $animate) {
trackByIdFn = trackByIdExpFn || trackByIdObjFn;
// if object, extract keys, sort them and use to determine order of iteration over obj props
collectionKeys = [];
for (key in collection) {
if (collection.hasOwnProperty(key) && key.charAt(0) != '$') {
collectionKeys.push(key);
for (var itemKey in collection) {
if (collection.hasOwnProperty(itemKey) && itemKey.charAt(0) != '$') {
collectionKeys.push(itemKey);
}
}
collectionKeys.sort();
Expand Down Expand Up @@ -319,10 +329,10 @@ var ngRepeatDirective = ['$parse', '$animate', function($parse, $animate) {
}

// remove existing items
for (key in lastBlockMap) {
for (var blockKey in lastBlockMap) {
// lastBlockMap is our own object so we don't need to use special hasOwnPropertyFn
if (lastBlockMap.hasOwnProperty(key)) {
block = lastBlockMap[key];
if (lastBlockMap.hasOwnProperty(blockKey)) {
block = lastBlockMap[blockKey];
elementsToRemove = getBlockElements(block.clone);
$animate.leave(elementsToRemove);
forEach(elementsToRemove, function(element) { element[NG_REMOVED] = true; });
Expand All @@ -340,8 +350,6 @@ var ngRepeatDirective = ['$parse', '$animate', function($parse, $animate) {
if (block.scope) {
// if we have already seen this object, then we need to reuse the
// associated scope/element
childScope = block.scope;

nextNode = previousNode;
do {
nextNode = nextNode.nextSibling;
Expand All @@ -354,32 +362,19 @@ var ngRepeatDirective = ['$parse', '$animate', function($parse, $animate) {
previousNode = getBlockEnd(block);
} else {
// new item which we don't know about
childScope = $scope.$new();
}

childScope[valueIdentifier] = value;
if (keyIdentifier) childScope[keyIdentifier] = key;
childScope.$index = index;
childScope.$first = (index === 0);
childScope.$last = (index === (arrayLength - 1));
childScope.$middle = !(childScope.$first || childScope.$last);
// jshint bitwise: false
childScope.$odd = !(childScope.$even = (index&1) === 0);
// jshint bitwise: true

if (!block.scope) {
$transclude(childScope, function(clone) {
$transclude(function(clone, scope) {
block.scope = scope;
clone[clone.length++] = document.createComment(' end ngRepeat: ' + expression + ' ');
$animate.enter(clone, null, jqLite(previousNode));
previousNode = clone;
block.scope = childScope;
// Note: We only need the first/last node of the cloned nodes.
// However, we need to keep the reference to the jqlite wrapper as it might be changed later
// by a directive with templateUrl when it's template arrives.
block.clone = clone;
nextBlockMap[block.id] = block;
});
}
updateScope(block.scope, index);
}
lastBlockMap = nextBlockMap;
});
Expand Down
Loading