From d58ae61c255d62e48dafb8f3ac1b2ec0ebde8c5e Mon Sep 17 00:00:00 2001 From: Peter Bacon Darwin Date: Thu, 25 Sep 2014 21:06:07 +0100 Subject: [PATCH 1/5] feat(Scope): allow the parent of a new scope to be specified This enables us to place transclude scopes more accurately in the scope hierarchy. --- src/ng/rootScope.js | 22 +++++++++++++++------- test/ng/rootScopeSpec.js | 9 +++++++++ 2 files changed, 24 insertions(+), 7 deletions(-) diff --git a/src/ng/rootScope.js b/src/ng/rootScope.js index 34e69b1bd325..15eabf7a8896 100644 --- a/src/ng/rootScope.js +++ b/src/ng/rootScope.js @@ -184,12 +184,20 @@ function $RootScopeProvider(){ * When creating widgets, it is useful for the widget to not accidentally read parent * state. * + * @param {Scope} [parent=this] The {@link ng.$rootScope.Scope `Scope`} that will be the `$parent` + * of the newly created scope. Defaults to `this` scope if not provided. + * This is used when creating a transclude scope to correctly place it + * in the scope hierarchy while maintaining the correct prototypical + * inheritance. + * * @returns {Object} The newly created child scope. * */ - $new: function(isolate) { + $new: function(isolate, parent) { var child; + parent = parent || this; + if (isolate) { child = new Scope(); child.$root = this.$root; @@ -213,13 +221,13 @@ function $RootScopeProvider(){ child = new this.$$ChildScope(); } child['this'] = child; - child.$parent = this; - child.$$prevSibling = this.$$childTail; - if (this.$$childHead) { - this.$$childTail.$$nextSibling = child; - this.$$childTail = child; + child.$parent = parent; + child.$$prevSibling = parent.$$childTail; + if (parent.$$childHead) { + parent.$$childTail.$$nextSibling = child; + parent.$$childTail = child; } else { - this.$$childHead = this.$$childTail = child; + parent.$$childHead = parent.$$childTail = child; } return child; }, diff --git a/test/ng/rootScopeSpec.js b/test/ng/rootScopeSpec.js index e3168ec10e86..1331a97f7d20 100644 --- a/test/ng/rootScopeSpec.js +++ b/test/ng/rootScopeSpec.js @@ -72,6 +72,15 @@ describe('Scope', function() { expect(child.$new).toBe($rootScope.$new); expect(child.$root).toBe($rootScope); })); + + it("should attach the child scope to a specified parent", inject(function($rootScope) { + var isolated = $rootScope.$new(true); + var trans = $rootScope.$new(false, isolated); + $rootScope.a = 123; + expect(isolated.a).toBeUndefined(); + expect(trans.a).toEqual(123); + expect(trans.$parent).toBe(isolated); + })); }); From 43a3b4e14fec811302ecb6257644ff8474ed2e54 Mon Sep 17 00:00:00 2001 From: Peter Bacon Darwin Date: Thu, 25 Sep 2014 21:21:59 +0100 Subject: [PATCH 2/5] fix($compile): connect transclude scopes to their containing scope to prevent memory leaks Transcluded scopes are now connected to the scope in which they are created via their `$parent` property. This means that they will be automatically destroyed when their "containing" scope is destroyed, without having to resort to listening for a `$destroy` event on various DOM elements or other scopes. Previously, transclude scope not only inherited prototypically from the scope from which they were transcluded but they were also still owned by that "outer" scope. This meant that there were scenarios where the "real" container scope/element was destroyed but the transclude scope was not, leading to memory leaks. The original strategy for dealing with this was to attach a `$destroy` event handler to the DOM elements in the transcluded content, so that if the elements were removed from the DOM then their associated transcluded scope would be destroyed. This didn't work for transclude contents that didn't contain any elements - most importantly in the case of the transclude content containing an element transclude directive at its root, since the compiler swaps out this element for a comment before a destroy handler could be attached. BREAKING CHANGE: `$transclude` functions no longer attach `$destroy` event handlers to the transcluded content, and so the associated transclude scope will not automatically be destroyed if you remove a transcluded element from the DOM using direct DOM manipulation such as the jquery `remove()` method. If you want to explicitly remove DOM elements inside your directive that have been compiled, and so potentially contain child (and transcluded) scopes, then it is your responsibility to get hold of the scope and destroy it at the same time. The suggested approach is to create a new child scope of your own around any DOM elements that you wish to manipulate in this way and destroy those scopes if you remove their contents - any child scopes will then be destroyed and cleaned up automatically. Note that all the built-in directives that manipulate the DOM (ngIf, ngRepeat, ngSwitch, etc) already follow this best practice, so if you only use these for manipulating the DOM then you do not have to worry about this change. Closes #9095 --- src/ng/compile.js | 32 ++++---- test/ng/compileSpec.js | 167 +++++++++++++++++++++++++++++------------ 2 files changed, 138 insertions(+), 61 deletions(-) diff --git a/src/ng/compile.js b/src/ng/compile.js index f75cb7393f11..8efd83077afd 100644 --- a/src/ng/compile.js +++ b/src/ng/compile.js @@ -294,13 +294,20 @@ * compile the content of the element and make it available to the directive. * Typically used with {@link ng.directive:ngTransclude * ngTransclude}. The advantage of transclusion is that the linking function receives a - * transclusion function which is pre-bound to the correct scope. In a typical setup the widget - * creates an `isolate` scope, but the transclusion is not a child, but a sibling of the `isolate` - * scope. This makes it possible for the widget to have private state, and the transclusion to - * be bound to the parent (pre-`isolate`) scope. + * transclusion function which is pre-bound to the scope of the position in the DOM from where + * it was taken. * - * * `true` - transclude the content of the directive. - * * `'element'` - transclude the whole element including any directives defined at lower priority. + * In a typical setup the widget creates an `isolate` scope, but the transcluded + * content has its own **transclusion scope**. While the **transclusion scope** is owned as a child, + * by the **isolate scope**, it prototypically inherits from the original scope from where the + * transcluded content was taken. + * + * This makes it possible for the widget to have private state, and the transclusion to + * be bound to the original (pre-`isolate`) scope. + * + * * `true` - transclude the content (i.e. the child nodes) of the directive's element. + * * `'element'` - transclude the whole of the directive's element including any directives on this + * element that defined at a lower priority than this directive. * *
* **Note:** When testing an element transclude directive you must not place the directive at the root of the @@ -1166,20 +1173,15 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { function createBoundTranscludeFn(scope, transcludeFn, previousBoundTranscludeFn, elementTransclusion) { - var boundTranscludeFn = function(transcludedScope, cloneFn, controllers, futureParentElement) { + var boundTranscludeFn = function(transcludedScope, cloneFn, controllers, futureParentElement, containingScope) { var scopeCreated = false; if (!transcludedScope) { - transcludedScope = scope.$new(); + transcludedScope = scope.$new(false, containingScope); transcludedScope.$$transcluded = true; - scopeCreated = true; } - var clone = transcludeFn(transcludedScope, cloneFn, controllers, previousBoundTranscludeFn, futureParentElement); - if (scopeCreated && !elementTransclusion) { - clone.on('$destroy', function() { transcludedScope.$destroy(); }); - } - return clone; + return transcludeFn(transcludedScope, cloneFn, controllers, previousBoundTranscludeFn, futureParentElement); }; return boundTranscludeFn; @@ -1810,7 +1812,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { if (!futureParentElement) { futureParentElement = hasElementTranscludeDirective ? $element.parent() : $element; } - return boundTranscludeFn(scope, cloneAttachFn, transcludeControllers, futureParentElement); + return boundTranscludeFn(scope, cloneAttachFn, transcludeControllers, futureParentElement, scopeToChild); } } } diff --git a/test/ng/compileSpec.js b/test/ng/compileSpec.js index 2a0d84d04150..111aba50da1d 100755 --- a/test/ng/compileSpec.js +++ b/test/ng/compileSpec.js @@ -4331,17 +4331,21 @@ describe('$compile', function() { return { transclude: 'content', replace: true, - scope: true, - template: '
  • W:{{$parent.$id}}-{{$id}};
' + scope: {}, + link: function(scope) { + scope.x='iso'; + }, + template: '
  • W:{{x}}-{{$parent.$id}}-{{$id}};
' }; }); }); inject(function(log, $rootScope, $compile) { - element = $compile('
T:{{$parent.$id}}-{{$id}};
') + element = $compile('
T:{{x}}-{{$parent.$id}}-{{$id}};
') ($rootScope); + $rootScope.x = 'root'; $rootScope.$apply(); - expect(element.text()).toEqual('W:1-2;T:1-3;'); - expect(jqLite(element.find('span')[0]).text()).toEqual('T:1-3'); + expect(element.text()).toEqual('W:iso-1-2;T:root-2-3;'); + expect(jqLite(element.find('span')[0]).text()).toEqual('T:root-2-3'); expect(jqLite(element.find('span')[1]).text()).toEqual(';'); }); }); @@ -4551,47 +4555,6 @@ describe('$compile', function() { } - it('should remove transclusion scope, when the DOM is destroyed', function() { - module(function() { - directive('box', valueFn({ - transclude: true, - scope: { name: '=', show: '=' }, - template: '

Hello: {{name}}!

', - link: function(scope, element) { - scope.$watch( - 'show', - function(show) { - if (!show) { - element.find('div').find('div').remove(); - } - } - ); - } - })); - }); - inject(function($compile, $rootScope) { - $rootScope.username = 'Misko'; - $rootScope.select = true; - element = $compile( - '
user: {{username}}
') - ($rootScope); - $rootScope.$apply(); - expect(element.text()).toEqual('Hello: Misko!user: Misko'); - - var widgetScope = $rootScope.$$childHead; - var transcludeScope = widgetScope.$$nextSibling; - expect(widgetScope.name).toEqual('Misko'); - expect(widgetScope.$parent).toEqual($rootScope); - expect(transcludeScope.$parent).toEqual($rootScope); - - $rootScope.select = false; - $rootScope.$apply(); - expect(element.text()).toEqual('Hello: Misko!'); - expect(widgetScope.$$nextSibling).toEqual(null); - }); - }); - - it('should add a $$transcluded property onto the transcluded scope', function() { module(function() { directive('trans', function() { @@ -4964,6 +4927,118 @@ describe('$compile', function() { }); + // see issue https://github.com/angular/angular.js/issues/9095 + describe('removing a transcluded element', function() { + + function countScopes($rootScope) { + return [$rootScope].concat( + getChildScopes($rootScope) + ).length; + + function getChildScopes(scope) { + var children = []; + if (!scope.$$childHead) { return children; } + var childScope = scope.$$childHead; + do { + children.push(childScope); + children = children.concat(getChildScopes(childScope)); + } while ((childScope = childScope.$$nextSibling)); + return children; + } + } + + beforeEach(module(function() { + directive('toggle', function() { + return { + transclude: true, + template: '
' + }; + }); + })); + + + it('should not leak the transclude scope when the transcluded content is an element transclusion directive', + inject(function($compile, $rootScope) { + + element = $compile( + '
' + + '
{{ msg }}
' + + '
' + )($rootScope); + + $rootScope.$apply('t = true'); + expect(element.text()).toContain('msg-1'); + expect(countScopes($rootScope)).toEqual(4); + + $rootScope.$apply('t = false'); + expect(element.text()).not.toContain('msg-1'); + expect(countScopes($rootScope)).toEqual(1); + + $rootScope.$apply('t = true'); + expect(element.text()).toContain('msg-1'); + expect(countScopes($rootScope)).toEqual(4); + + $rootScope.$apply('t = false'); + expect(element.text()).not.toContain('msg-1'); + expect(countScopes($rootScope)).toEqual(1); + })); + + + it('should not leak the transclude scope if the transcluded contains only comments', + inject(function($compile, $rootScope) { + + element = $compile( + '
' + + '' + + '
' + )($rootScope); + + $rootScope.$apply('t = true'); + expect(element.html()).toContain('some comment'); + expect(countScopes($rootScope)).toEqual(3); + + $rootScope.$apply('t = false'); + expect(element.html()).not.toContain('some comment'); + expect(countScopes($rootScope)).toEqual(1); + + $rootScope.$apply('t = true'); + expect(element.html()).toContain('some comment'); + expect(countScopes($rootScope)).toEqual(3); + + $rootScope.$apply('t = false'); + expect(element.html()).not.toContain('some comment'); + expect(countScopes($rootScope)).toEqual(1); + })); + + it('should not leak the transclude scope if the transcluded contains only text nodes', + inject(function($compile, $rootScope) { + + element = $compile( + '
' + + 'some text' + + '
' + )($rootScope); + + $rootScope.$apply('t = true'); + expect(element.html()).toContain('some text'); + expect(countScopes($rootScope)).toEqual(3); + + $rootScope.$apply('t = false'); + expect(element.html()).not.toContain('some text'); + expect(countScopes($rootScope)).toEqual(1); + + $rootScope.$apply('t = true'); + expect(element.html()).toContain('some text'); + expect(countScopes($rootScope)).toEqual(3); + + $rootScope.$apply('t = false'); + expect(element.html()).not.toContain('some text'); + expect(countScopes($rootScope)).toEqual(1); + })); + + }); + + describe('nested transcludes', function() { beforeEach(module(function($compileProvider) { From ac39095b1f720814cfa8d1db624726717d7e14d0 Mon Sep 17 00:00:00 2001 From: Peter Bacon Darwin Date: Thu, 25 Sep 2014 22:11:48 +0100 Subject: [PATCH 3/5] test($compile): add comments to clarify what scopes we expect --- test/ng/compileSpec.js | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/test/ng/compileSpec.js b/test/ng/compileSpec.js index 111aba50da1d..69e904821532 100755 --- a/test/ng/compileSpec.js +++ b/test/ng/compileSpec.js @@ -4968,18 +4968,22 @@ describe('$compile', function() { $rootScope.$apply('t = true'); expect(element.text()).toContain('msg-1'); + // Expected scopes: $rootScope, ngIf, transclusion, ngRepeat expect(countScopes($rootScope)).toEqual(4); $rootScope.$apply('t = false'); expect(element.text()).not.toContain('msg-1'); + // Expected scopes: $rootScope expect(countScopes($rootScope)).toEqual(1); $rootScope.$apply('t = true'); expect(element.text()).toContain('msg-1'); + // Expected scopes: $rootScope, ngIf, transclusion, ngRepeat expect(countScopes($rootScope)).toEqual(4); $rootScope.$apply('t = false'); expect(element.text()).not.toContain('msg-1'); + // Expected scopes: $rootScope expect(countScopes($rootScope)).toEqual(1); })); @@ -4995,18 +4999,22 @@ describe('$compile', function() { $rootScope.$apply('t = true'); expect(element.html()).toContain('some comment'); + // Expected scopes: $rootScope, ngIf, transclusion expect(countScopes($rootScope)).toEqual(3); $rootScope.$apply('t = false'); expect(element.html()).not.toContain('some comment'); + // Expected scopes: $rootScope expect(countScopes($rootScope)).toEqual(1); $rootScope.$apply('t = true'); expect(element.html()).toContain('some comment'); + // Expected scopes: $rootScope, ngIf, transclusion expect(countScopes($rootScope)).toEqual(3); $rootScope.$apply('t = false'); expect(element.html()).not.toContain('some comment'); + // Expected scopes: $rootScope expect(countScopes($rootScope)).toEqual(1); })); @@ -5021,18 +5029,22 @@ describe('$compile', function() { $rootScope.$apply('t = true'); expect(element.html()).toContain('some text'); + // Expected scopes: $rootScope, ngIf, transclusion expect(countScopes($rootScope)).toEqual(3); $rootScope.$apply('t = false'); expect(element.html()).not.toContain('some text'); + // Expected scopes: $rootScope expect(countScopes($rootScope)).toEqual(1); $rootScope.$apply('t = true'); expect(element.html()).toContain('some text'); + // Expected scopes: $rootScope, ngIf, transclusion expect(countScopes($rootScope)).toEqual(3); $rootScope.$apply('t = false'); expect(element.html()).not.toContain('some text'); + // Expected scopes: $rootScope expect(countScopes($rootScope)).toEqual(1); })); From 594c00f820e7a660ee280b646642328752e9882f Mon Sep 17 00:00:00 2001 From: Peter Bacon Darwin Date: Thu, 25 Sep 2014 22:27:00 +0100 Subject: [PATCH 4/5] test($compile): add extra multi-element transclude test --- test/ng/compileSpec.js | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/test/ng/compileSpec.js b/test/ng/compileSpec.js index 69e904821532..f52fc1faa6c1 100755 --- a/test/ng/compileSpec.js +++ b/test/ng/compileSpec.js @@ -4988,6 +4988,38 @@ describe('$compile', function() { })); + it('should not leak the transclude scope when the transcluded content is an multi-element transclusion directive', + inject(function($compile, $rootScope) { + + element = $compile( + '
' + + '
{{ msg }}
' + + '
{{ msg }}
' + + '
' + )($rootScope); + + $rootScope.$apply('t = true'); + expect(element.text()).toContain('msg-1msg-1'); + // Expected scopes: $rootScope, ngIf, transclusion, ngRepeat + expect(countScopes($rootScope)).toEqual(4); + + $rootScope.$apply('t = false'); + expect(element.text()).not.toContain('msg-1msg-1'); + // Expected scopes: $rootScope + expect(countScopes($rootScope)).toEqual(1); + + $rootScope.$apply('t = true'); + expect(element.text()).toContain('msg-1msg-1'); + // Expected scopes: $rootScope, ngIf, transclusion, ngRepeat + expect(countScopes($rootScope)).toEqual(4); + + $rootScope.$apply('t = false'); + expect(element.text()).not.toContain('msg-1msg-1'); + // Expected scopes: $rootScope + expect(countScopes($rootScope)).toEqual(1); + })); + + it('should not leak the transclude scope if the transcluded contains only comments', inject(function($compile, $rootScope) { From 53bc4757246dfe9fd2349813fbab6c8cf79aaae3 Mon Sep 17 00:00:00 2001 From: Peter Bacon Darwin Date: Thu, 25 Sep 2014 22:27:16 +0100 Subject: [PATCH 5/5] style($compile): remove unused variable declaration --- src/ng/compile.js | 1 - 1 file changed, 1 deletion(-) diff --git a/src/ng/compile.js b/src/ng/compile.js index 8efd83077afd..a6aeb7420599 100644 --- a/src/ng/compile.js +++ b/src/ng/compile.js @@ -1174,7 +1174,6 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { function createBoundTranscludeFn(scope, transcludeFn, previousBoundTranscludeFn, elementTransclusion) { var boundTranscludeFn = function(transcludedScope, cloneFn, controllers, futureParentElement, containingScope) { - var scopeCreated = false; if (!transcludedScope) { transcludedScope = scope.$new(false, containingScope);