From 2ed414a4e79e224b2ec5823dbf5552f9d2f2cd54 Mon Sep 17 00:00:00 2001 From: Dave Longley Date: Mon, 13 Oct 2014 23:30:39 -0400 Subject: [PATCH 1/2] fix($compile): do not rebind parent bound transclude functions Do not rebind parent bound transclude functions passed to the link function returned from `$compile`. Change parameter order of ```publicLinkFn``` to allow parent bound transclude functions to be passed (do not make public yet). The current `$compile` public API documentation indicates that a transclude function may be passed as a second parameter, but it is not clear that this is not the same function that is given to directive link functions as the `transcludeFn` parameter. This parameter must be passed in order to implement, for example, lazy compilation. In order to pass the `transcludeFn` parameter given to a directive's link function, it must be passed to the result of a call to `$compile` (i.e. `publicLinkFn`). Doing so, however, would cause two bugs. First, the passed transclude function would get rebound, improperly changing its scope from its original binding. Second, the `containingScope` would not be updated and the wrong `$parent` would be assigned to the `transcludedScope` resulting in a memory leak. This patch fixes both of these issues. This patch does not expose a new public API for `publicLinkFn`, rather, it leaves this to a later change that better considers how that public API should look. Thanks to @lgalfaso, @tbosch, and @petebacondarwin. Related to #9413 --- src/ng/compile.js | 19 ++++++++- test/ng/compileSpec.js | 92 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 109 insertions(+), 2 deletions(-) diff --git a/src/ng/compile.js b/src/ng/compile.js index 00f174d10954..5fdf04059e0b 100644 --- a/src/ng/compile.js +++ b/src/ng/compile.js @@ -1157,6 +1157,15 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { var namespace = null; return function publicLinkFn(scope, cloneConnectFn, transcludeControllers, parentBoundTranscludeFn, futureParentElement) { assertArg(scope, 'scope'); + + // 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); } @@ -1326,7 +1335,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { transcludedScope.$$transcluded = true; } - return transcludeFn(transcludedScope, cloneFn, controllers, previousBoundTranscludeFn, futureParentElement); + return transcludeFn(transcludedScope, cloneFn, previousBoundTranscludeFn, controllers, futureParentElement); }; return boundTranscludeFn; @@ -1794,7 +1803,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 = {}; diff --git a/test/ng/compileSpec.js b/test/ng/compileSpec.js index 294140bdd8e1..e08e928716ea 100755 --- a/test/ng/compileSpec.js +++ b/test/ng/compileSpec.js @@ -5209,6 +5209,98 @@ describe('$compile', function() { }); + // see issue https://github.com/angular/angular.js/issues/9413 + describe('passing a parent bound transclude function to the link ' + + '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, transcludeFn); + }; + } + }; + }); + directive('toggle', valueFn({ + scope: {t: '=toggle'}, + transclude: true, + template: '
' + })); + })); + + it('should preserve the bound scope', function() { + + inject(function($compile, $rootScope) { + element = $compile( + '
' + + '
' + + '
' + + 'SuccessError' + + '
' + + '
')($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: '
' + })); + + inject(function($compile, $rootScope) { + element = $compile( + '
' + + '
' + + '
' + + '
' + + 'SuccessError' + + '
' + + '
' + + '
')($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() { From ff06bbd60df2651f5f68038c2c18a2517564f2a0 Mon Sep 17 00:00:00 2001 From: Dave Longley Date: Tue, 14 Oct 2014 12:52:04 -0400 Subject: [PATCH 2/2] feat($compile): expose publicLinkFn options Use `options` object hash style parameter for publicLinkFn. Expose `options` parameter as part of the public API. Closes #9413 --- src/ng/compile.js | 37 +++++++++++++++++++++++++++++++---- src/ng/directive/ngInclude.js | 2 +- test/ng/compileSpec.js | 4 +++- 3 files changed, 37 insertions(+), 6 deletions(-) diff --git a/src/ng/compile.js b/src/ng/compile.js index 5fdf04059e0b..e4b42e75a9c7 100644 --- a/src/ng/compile.js +++ b/src/ng/compile.js @@ -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. + * + *
+ * **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. + *
+ * * @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. @@ -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. * @@ -1155,9 +1175,14 @@ 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 @@ -1335,7 +1360,11 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { transcludedScope.$$transcluded = true; } - return transcludeFn(transcludedScope, cloneFn, previousBoundTranscludeFn, controllers, futureParentElement); + return transcludeFn(transcludedScope, cloneFn, { + parentBoundTranscludeFn: previousBoundTranscludeFn, + transcludeControllers: controllers, + futureParentElement: futureParentElement + }); }; return boundTranscludeFn; diff --git a/src/ng/directive/ngInclude.js b/src/ng/directive/ngInclude.js index ceb0ee3368a9..e8634993c440 100644 --- a/src/ng/directive/ngInclude.js +++ b/src/ng/directive/ngInclude.js @@ -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; } diff --git a/test/ng/compileSpec.js b/test/ng/compileSpec.js index e08e928716ea..39e0a8c1e109 100755 --- a/test/ng/compileSpec.js +++ b/test/ng/compileSpec.js @@ -5221,7 +5221,9 @@ describe('$compile', function() { tElement.empty(); return function(scope, element, attrs, ctrls, transcludeFn) { element.append(content); - $compile(content)(scope, undefined, transcludeFn); + $compile(content)(scope, undefined, { + parentBoundTranscludeFn: transcludeFn + }); }; } };