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

fix($compile): do not rebind parent bound transclude functions #9605

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
54 changes: 49 additions & 5 deletions src/ng/compile.js
Original file line number Diff line number Diff line change
Expand Up @@ -622,10 +622,17 @@
*
*
* @param {string|DOMElement} element Element or HTML string to compile into a template function.
* @param {function(angular.Scope, cloneAttachFn=)} transclude function available to directives.
* @param {function(angular.Scope, cloneAttachFn=)} transclude function available to directives - DEPRECATED.
*
* <div class="alert alert-error">
* **Note:** Passing a `transclude` function to the $compile function is deprecated, as it
* e.g. will not use the right outer scope. Please pass the transclude function as a
* `parentBoundTranscludeFn` to the link function instead.
* </div>
*
* @param {number} maxPriority only apply directives lower than given priority (Only effects the
* root element(s), not their children)
* @returns {function(scope, cloneAttachFn=)} a link function which is used to bind template
* @returns {function(scope, cloneAttachFn=, options=)} a link function which is used to bind template
* (a DOM element/tree) to a scope. Where:
*
* * `scope` - A {@link ng.$rootScope.Scope Scope} to bind to.
Expand All @@ -637,6 +644,19 @@
* * `clonedElement` - is a clone of the original `element` passed into the compiler.
* * `scope` - is the current scope with which the linking function is working with.
*
* * `options` - An optional object hash with linking options. If `options` is provided, then the following
* keys may be used to control linking behavior:
*
* * `parentBoundTranscludeFn` - the transclude function made available to
* directives; if given, it will be passed through to the link functions of
* directives found in `element` during compilation.
* * `transcludeControllers` - an object hash with keys that map controller names
* to controller instances; if given, it will make the controllers
* available to directives.
* * `futureParentElement` - defines the parent to which the `cloneAttachFn` will add
* the cloned elements; only needed for transcludes that are allowed to contain non html
* elements (e.g. SVG elements). See also the directive.controller property.
*
* Calling the linking function returns the element of the template. It is either the original
* element passed in, or the clone of the element if the `cloneAttachFn` is provided.
*
Expand Down Expand Up @@ -1155,8 +1175,22 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
maxPriority, ignoreDirective, previousCompileContext);
compile.$$addScopeClass($compileNodes);
var namespace = null;
return function publicLinkFn(scope, cloneConnectFn, transcludeControllers, parentBoundTranscludeFn, futureParentElement) {
return function publicLinkFn(scope, cloneConnectFn, options) {
assertArg(scope, 'scope');

options = options || {};
var parentBoundTranscludeFn = options.parentBoundTranscludeFn,
transcludeControllers = options.transcludeControllers,
futureParentElement = options.futureParentElement;

// When `parentBoundTranscludeFn` is passed, it is a
// `controllersBoundTransclude` function (it was previously passed
// as `transclude` to directive.link) so we must unwrap it to get
// its `boundTranscludeFn`
if (parentBoundTranscludeFn && parentBoundTranscludeFn.$$boundTransclude) {
parentBoundTranscludeFn = parentBoundTranscludeFn.$$boundTransclude;
}

if (!namespace) {
namespace = detectNamespaceForChildElements(futureParentElement);
}
Expand Down Expand Up @@ -1326,7 +1360,11 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
transcludedScope.$$transcluded = true;
}

return transcludeFn(transcludedScope, cloneFn, controllers, previousBoundTranscludeFn, futureParentElement);
return transcludeFn(transcludedScope, cloneFn, {
parentBoundTranscludeFn: previousBoundTranscludeFn,
transcludeControllers: controllers,
futureParentElement: futureParentElement
});
};

return boundTranscludeFn;
Expand Down Expand Up @@ -1794,7 +1832,13 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
isolateScope = scope.$new(true);
}

transcludeFn = boundTranscludeFn && controllersBoundTransclude;
if (boundTranscludeFn) {
// track `boundTranscludeFn` so it can be unwrapped if `transcludeFn`
// is later passed as `parentBoundTranscludeFn` to `publicLinkFn`
transcludeFn = controllersBoundTransclude;
transcludeFn.$$boundTransclude = boundTranscludeFn;
}

if (controllerDirectives) {
// TODO: merge `controllers` and `elementControllers` into single object.
controllers = {};
Expand Down
2 changes: 1 addition & 1 deletion src/ng/directive/ngInclude.js
Original file line number Diff line number Diff line change
Expand Up @@ -284,7 +284,7 @@ var ngIncludeFillContentDirective = ['$compile',
$compile(jqLiteBuildFragment(ctrl.template, document).childNodes)(scope,
function namespaceAdaptedClone(clone) {
$element.append(clone);
}, undefined, undefined, $element);
}, {futureParentElement: $element});
return;
}

Expand Down
94 changes: 94 additions & 0 deletions test/ng/compileSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -5209,6 +5209,100 @@ describe('$compile', function() {
});


// see issue https://github.com/angular/angular.js/issues/9413
describe('passing a parent bound transclude function to the link ' +
Copy link
Contributor

Choose a reason for hiding this comment

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

Please think about how to simplify this test so that it only tests the changed behavior and is easier to understand?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is just a grouping for the two tests nested in it that are both related to passing a parent bound transclude function. Is that not a good summary title for those two tests?

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 was following the example below ("removing a transcluded element") that then has nested tests below it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I did not mean the test suite but the tests themselves.

Copy link
Contributor

Choose a reason for hiding this comment

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

E.g. do we need a directive with terminal and priority and another directive with isolate scope to test this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, terminal, priority, and the isolate scope have been removed. The recursive directive test was added per @lgalfaso's request.

Copy link
Contributor

Choose a reason for hiding this comment

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

And do we really need 2 directives to test this?

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 can reuse the toggle directive (instead of the recursive directive), but that may end up being less clear with double-toggling. What's being tested is recursive transclusion.

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 thought about this for a bit. I think the second directive is necessary to make clear what's being tested. This test is specifically for recursive transclusion, not for directive recursion.

'function returned from `$compile`', function() {

beforeEach(module(function() {
directive('lazyCompile', function($compile) {
return {
compile: function(tElement, tAttrs) {
var content = tElement.contents();
tElement.empty();
return function(scope, element, attrs, ctrls, transcludeFn) {
element.append(content);
$compile(content)(scope, undefined, {
parentBoundTranscludeFn: transcludeFn
});
};
}
};
});
directive('toggle', valueFn({
scope: {t: '=toggle'},
transclude: true,
template: '<div ng-if="t"><lazy-compile><div ng-transclude></div></lazy-compile></div>'
}));
}));

it('should preserve the bound scope', function() {

inject(function($compile, $rootScope) {
element = $compile(
'<div>' +
'<div ng-init="outer=true"></div>' +
'<div toggle="t">' +
'<span ng-if="outer">Success</span><span ng-if="!outer">Error</span>' +
'</div>' +
'</div>')($rootScope);

$rootScope.$apply('t = false');
expect($rootScope.$countChildScopes()).toBe(1);
expect(element.text()).toBe('');

$rootScope.$apply('t = true');
expect($rootScope.$countChildScopes()).toBe(4);
expect(element.text()).toBe('Success');

$rootScope.$apply('t = false');
expect($rootScope.$countChildScopes()).toBe(1);
expect(element.text()).toBe('');

$rootScope.$apply('t = true');
expect($rootScope.$countChildScopes()).toBe(4);
expect(element.text()).toBe('Success');
});
});


it('should preserve the bound scope when using recursive transclusion', function() {

directive('recursiveTransclude', valueFn({
transclude: true,
template: '<div><lazy-compile><div ng-transclude></div></lazy-compile></div>'
}));

inject(function($compile, $rootScope) {
element = $compile(
'<div>' +
'<div ng-init="outer=true"></div>' +
'<div toggle="t">' +
'<div recursive-transclude>' +
'<span ng-if="outer">Success</span><span ng-if="!outer">Error</span>' +
'</div>' +
'</div>' +
'</div>')($rootScope);

$rootScope.$apply('t = false');
expect($rootScope.$countChildScopes()).toBe(1);
expect(element.text()).toBe('');

$rootScope.$apply('t = true');
expect($rootScope.$countChildScopes()).toBe(4);
expect(element.text()).toBe('Success');

$rootScope.$apply('t = false');
expect($rootScope.$countChildScopes()).toBe(1);
expect(element.text()).toBe('');

$rootScope.$apply('t = true');
expect($rootScope.$countChildScopes()).toBe(4);
expect(element.text()).toBe('Success');
});
});
});


// see issue https://github.com/angular/angular.js/issues/9095
describe('removing a transcluded element', function() {

Expand Down