From 1a98c0ee346b718b9462da1abf4352a4605cbc7f Mon Sep 17 00:00:00 2001 From: mzdunek93 Date: Tue, 3 Nov 2015 22:44:53 +0000 Subject: [PATCH 1/4] fix($compile): fix scoping of transclusion directives inside replace directive Closes #12975 Closes #12936 Closes #13244 --- src/ng/compile.js | 33 +++++++++++++++++++------ test/ng/compileSpec.js | 56 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 82 insertions(+), 7 deletions(-) diff --git a/src/ng/compile.js b/src/ng/compile.js index afafb081cf72..270d563b727e 100644 --- a/src/ng/compile.js +++ b/src/ng/compile.js @@ -1293,6 +1293,14 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { return function publicLinkFn(scope, cloneConnectFn, options) { assertArg(scope, 'scope'); + if (previousCompileContext && previousCompileContext.needsNewScope) { + // A parent directive did a replace and a directive on this element asked + // for transclusion, which caused us to lose a layer of element on which + // we could hold the new transclusion scope, so we will create it manually + // here. + scope = scope.$parent.$new(); + } + options = options || {}; var parentBoundTranscludeFn = options.parentBoundTranscludeFn, transcludeControllers = options.transcludeControllers, @@ -1874,7 +1882,8 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { } $compileNode.empty(); // clear contents - childTranscludeFn = compilationGenerator(mightHaveMultipleTransclusionError, $template, transcludeFn); + childTranscludeFn = compilationGenerator(mightHaveMultipleTransclusionError, $template, transcludeFn, undefined, + undefined, { needsNewScope: directive.$$isolateScope || directive.$$newScope}); childTranscludeFn.$$slots = slots; } } @@ -1917,8 +1926,11 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { var templateDirectives = collectDirectives(compileNode, [], newTemplateAttrs); var unprocessedDirectives = directives.splice(i + 1, directives.length - (i + 1)); - if (newIsolateScopeDirective) { - markDirectivesAsIsolate(templateDirectives); + if (newIsolateScopeDirective || newScopeDirective) { + // The original directive caused the current element to be replaced but this element + // also needs to have a new scope, so we need to tell the template directives + // that they would need to get their scope from further up, if they require transclusion + markDirectiveScope(templateDirectives, newIsolateScopeDirective, newScopeDirective); } directives = directives.concat(templateDirectives).concat(unprocessedDirectives); mergeTemplateAttributes(templateAttrs, newTemplateAttrs); @@ -2213,10 +2225,15 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { } } - function markDirectivesAsIsolate(directives) { - // mark all directives as needing isolate scope. + // Depending upon the context in which a directive finds itself it might need to have a new isolated + // or child scope created. For instance: + // * if the directive has been pulled into a template because another directive with a higher priority + // asked for element transclusion + // * if the directive itself asks for transclusion but it is at the root of a template and the original + // element was replaced. See https://github.com/angular/angular.js/issues/12936 + function markDirectiveScope(directives, isolateScope, newScope) { for (var j = 0, jj = directives.length; j < jj; j++) { - directives[j] = inherit(directives[j], {$$isolateScope: true}); + directives[j] = inherit(directives[j], {$$isolateScope: isolateScope, $$newScope: newScope}); } } @@ -2363,7 +2380,9 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { var templateDirectives = collectDirectives(compileNode, [], tempTemplateAttrs); if (isObject(origAsyncDirective.scope)) { - markDirectivesAsIsolate(templateDirectives); + // the original directive that caused the template to be loaded async required + // an isolate scope + markDirectiveScope(templateDirectives, true); } directives = templateDirectives.concat(directives); mergeTemplateAttributes(tAttrs, tempTemplateAttrs); diff --git a/test/ng/compileSpec.js b/test/ng/compileSpec.js index da016f74c5ac..c265ffab952e 100755 --- a/test/ng/compileSpec.js +++ b/test/ng/compileSpec.js @@ -5636,6 +5636,62 @@ describe('$compile', function() { }); }); + //see issue https://github.com/angular/angular.js/issues/12936 + it('should use the proper scope when it is on the root element of a replaced directive template', function() { + module(function() { + directive('isolate', valueFn({ + scope: {}, + replace: true, + template: '
{{x}}
', + link: function(scope, element, attr, ctrl) { + scope.x = 'iso'; + } + })); + directive('trans', valueFn({ + transclude: 'content', + link: function(scope, element, attr, ctrl, $transclude) { + $transclude(function(clone) { + element.append(clone); + }); + } + })); + }); + inject(function($rootScope, $compile) { + element = $compile('')($rootScope); + $rootScope.x = 'root'; + $rootScope.$apply(); + expect(element.text()).toEqual('iso'); + }); + }); + + + //see issue https://github.com/angular/angular.js/issues/12936 + it('should use the proper scope when it is on the root element of a replaced directive template with child scope', function() { + module(function() { + directive('child', valueFn({ + scope: true, + replace: true, + template: '
{{x}}
', + link: function(scope, element, attr, ctrl) { + scope.x = 'child'; + } + })); + directive('trans', valueFn({ + transclude: 'content', + link: function(scope, element, attr, ctrl, $transclude) { + $transclude(function(clone) { + element.append(clone); + }); + } + })); + }); + inject(function($rootScope, $compile) { + element = $compile('')($rootScope); + $rootScope.x = 'root'; + $rootScope.$apply(); + expect(element.text()).toEqual('child'); + }); + }); it('should not leak if two "element" transclusions are on the same element (with debug info)', function() { From 1c13a4f45ddc86805a96576b75c969ad577b6274 Mon Sep 17 00:00:00 2001 From: Jakub Torbicki Date: Wed, 18 Mar 2015 10:16:14 +0100 Subject: [PATCH 2/4] fix($compile): bind all directive controllers correctly when using `bindToController` Previously only the first directive's controller would be bound correctly. Closes #11343 Closes #11345 --- src/ng/compile.js | 53 +++++------- test/ng/compileSpec.js | 177 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 198 insertions(+), 32 deletions(-) diff --git a/src/ng/compile.js b/src/ng/compile.js index 270d563b727e..2376257b5331 100644 --- a/src/ng/compile.js +++ b/src/ng/compile.js @@ -2084,7 +2084,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { } function nodeLinkFn(childLinkFn, scope, linkNode, $rootElement, boundTranscludeFn) { - var i, ii, linkFn, controller, isolateScope, elementControllers, transcludeFn, $element, + var linkFn, isolateScope, elementControllers, transcludeFn, $element, attrs, removeScopeBindingWatches, removeControllerBindingWatches; if (compileNode === linkNode) { @@ -2124,38 +2124,27 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { isolateScope.$on('$destroy', removeScopeBindingWatches); } } - if (elementControllers) { - // Initialize bindToController bindings for new/isolate scopes - var scopeDirective = newIsolateScopeDirective || newScopeDirective; - var bindings; - var controllerForBindings; - if (scopeDirective && elementControllers[scopeDirective.name]) { - bindings = scopeDirective.$$bindings.bindToController; - controller = elementControllers[scopeDirective.name]; - - if (controller && controller.identifier && bindings) { - controllerForBindings = controller; - removeControllerBindingWatches = - initializeDirectiveBindings(scope, attrs, controller.instance, - bindings, scopeDirective); - } + + // Initialize bindToController bindings + for (var name in elementControllers) { + var controllerDirective = controllerDirectives[name]; + var controller = elementControllers[name]; + var bindings = controllerDirective.$$bindings.bindToController; + + if (controller.identifier && bindings) { + removeControllerBindingWatches = + initializeDirectiveBindings(scope, attrs, controller.instance, bindings, controllerDirective); } - for (i in elementControllers) { - controller = elementControllers[i]; - var controllerResult = controller(); - - if (controllerResult !== controller.instance) { - // If the controller constructor has a return value, overwrite the instance - // from setupControllers and update the element data - controller.instance = controllerResult; - $element.data('$' + i + 'Controller', controllerResult); - if (controller === controllerForBindings) { - // Remove and re-install bindToController bindings - removeControllerBindingWatches && removeControllerBindingWatches(); - removeControllerBindingWatches = - initializeDirectiveBindings(scope, attrs, controllerResult, bindings, scopeDirective); - } - } + + var controllerResult = controller(); + if (controllerResult !== controller.instance) { + // If the controller constructor has a return value, overwrite the instance + // from setupControllers + controller.instance = controllerResult; + $element.data('$' + controllerDirective.name + 'Controller', controllerResult); + removeControllerBindingWatches && removeControllerBindingWatches(); + removeControllerBindingWatches = + initializeDirectiveBindings(scope, attrs, controller.instance, bindings, controllerDirective); } } diff --git a/test/ng/compileSpec.js b/test/ng/compileSpec.js index c265ffab952e..136bec90a25b 100755 --- a/test/ng/compileSpec.js +++ b/test/ng/compileSpec.js @@ -4414,6 +4414,183 @@ describe('$compile', function() { }); + it('should bind to multiple directives controllers via object notation (no scope)', function() { + var controller1Called = false; + var controller2Called = false; + module(function($compileProvider, $controllerProvider) { + $compileProvider.directive('foo', valueFn({ + bindToController: { + 'data': '=fooData', + 'str': '@fooStr', + 'fn': '&fooFn' + }, + controllerAs: 'fooCtrl', + controller: function() { + expect(this.data).toEqualData({'foo': 'bar', 'baz': 'biz'}); + expect(this.str).toBe('Hello, world!'); + expect(this.fn()).toBe('called!'); + controller1Called = true; + } + })); + $compileProvider.directive('bar', valueFn({ + bindToController: { + 'data': '=barData', + 'str': '@barStr', + 'fn': '&barFn' + }, + controllerAs: 'barCtrl', + controller: function() { + expect(this.data).toEqualData({'foo2': 'bar2', 'baz2': 'biz2'}); + expect(this.str).toBe('Hello, second world!'); + expect(this.fn()).toBe('second called!'); + controller2Called = true; + } + })); + }); + inject(function($compile, $rootScope) { + $rootScope.fn = valueFn('called!'); + $rootScope.string = 'world'; + $rootScope.data = {'foo': 'bar','baz': 'biz'}; + $rootScope.fn2 = valueFn('second called!'); + $rootScope.string2 = 'second world'; + $rootScope.data2 = {'foo2': 'bar2', 'baz2': 'biz2'}; + element = $compile( + '
' + + '
')($rootScope); + $rootScope.$digest(); + expect(controller1Called).toBe(true); + expect(controller2Called).toBe(true); + }); + }); + + + it('should bind to multiple directives controllers via object notation (new iso scope)', function() { + var controller1Called = false; + var controller2Called = false; + module(function($compileProvider, $controllerProvider) { + $compileProvider.directive('foo', valueFn({ + bindToController: { + 'data': '=fooData', + 'str': '@fooStr', + 'fn': '&fooFn' + }, + scope: true, + controllerAs: 'fooCtrl', + controller: function() { + expect(this.data).toEqualData({'foo': 'bar', 'baz': 'biz'}); + expect(this.str).toBe('Hello, world!'); + expect(this.fn()).toBe('called!'); + controller1Called = true; + } + })); + $compileProvider.directive('bar', valueFn({ + bindToController: { + 'data': '=barData', + 'str': '@barStr', + 'fn': '&barFn' + }, + controllerAs: 'barCtrl', + controller: function() { + expect(this.data).toEqualData({'foo2': 'bar2', 'baz2': 'biz2'}); + expect(this.str).toBe('Hello, second world!'); + expect(this.fn()).toBe('second called!'); + controller2Called = true; + } + })); + }); + inject(function($compile, $rootScope) { + $rootScope.fn = valueFn('called!'); + $rootScope.string = 'world'; + $rootScope.data = {'foo': 'bar','baz': 'biz'}; + $rootScope.fn2 = valueFn('second called!'); + $rootScope.string2 = 'second world'; + $rootScope.data2 = {'foo2': 'bar2', 'baz2': 'biz2'}; + element = $compile( + '
' + + '
')($rootScope); + $rootScope.$digest(); + expect(controller1Called).toBe(true); + expect(controller2Called).toBe(true); + }); + }); + + + it('should bind to multiple directives controllers via object notation (new scope)', function() { + var controller1Called = false; + var controller2Called = false; + module(function($compileProvider, $controllerProvider) { + $compileProvider.directive('foo', valueFn({ + bindToController: { + 'data': '=fooData', + 'str': '@fooStr', + 'fn': '&fooFn' + }, + scope: true, + controllerAs: 'fooCtrl', + controller: function() { + expect(this.data).toEqualData({'foo': 'bar', 'baz': 'biz'}); + expect(this.str).toBe('Hello, world!'); + expect(this.fn()).toBe('called!'); + controller1Called = true; + } + })); + $compileProvider.directive('bar', valueFn({ + bindToController: { + 'data': '=barData', + 'str': '@barStr', + 'fn': '&barFn' + }, + scope: true, + controllerAs: 'barCtrl', + controller: function() { + expect(this.data).toEqualData({'foo2': 'bar2', 'baz2': 'biz2'}); + expect(this.str).toBe('Hello, second world!'); + expect(this.fn()).toBe('second called!'); + controller2Called = true; + } + })); + }); + inject(function($compile, $rootScope) { + $rootScope.fn = valueFn('called!'); + $rootScope.string = 'world'; + $rootScope.data = {'foo': 'bar','baz': 'biz'}; + $rootScope.fn2 = valueFn('second called!'); + $rootScope.string2 = 'second world'; + $rootScope.data2 = {'foo2': 'bar2', 'baz2': 'biz2'}; + element = $compile( + '
' + + '
')($rootScope); + $rootScope.$digest(); + expect(controller1Called).toBe(true); + expect(controller2Called).toBe(true); + }); + }); + + it('should put controller in scope when controller identifier present but not using controllerAs', function() { var controllerCalled = false; var myCtrl; From 50557a6cd329e8438fb5694d11e8a7d018142afe Mon Sep 17 00:00:00 2001 From: Georgios Kalpakas Date: Tue, 6 Oct 2015 22:43:14 +0300 Subject: [PATCH 3/4] fix($compile): evaluate against the correct scope with bindToController on new scope Previously, the directive bindings were evaluated against the directive's new (non-isolate) scope, instead of the correct (parent) scope. This went unnoticed most of the time, since a property would be eventually looked up in the parent scope due to prototypal inheritance. The incorrect behaviour was exhibited when a property on the child scope was shadowing that on the parent scope. This commit fixes it. Fixes #13021 Closes #13025 --- src/ng/compile.js | 9 ++-- test/ng/compileSpec.js | 118 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 124 insertions(+), 3 deletions(-) diff --git a/src/ng/compile.js b/src/ng/compile.js index 2376257b5331..b333b079aa82 100644 --- a/src/ng/compile.js +++ b/src/ng/compile.js @@ -2084,7 +2084,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { } function nodeLinkFn(childLinkFn, scope, linkNode, $rootElement, boundTranscludeFn) { - var linkFn, isolateScope, elementControllers, transcludeFn, $element, + var linkFn, isolateScope, controllerScope, elementControllers, transcludeFn, $element, attrs, removeScopeBindingWatches, removeControllerBindingWatches; if (compileNode === linkNode) { @@ -2095,8 +2095,11 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { attrs = new Attributes($element, templateAttrs); } + controllerScope = scope; if (newIsolateScopeDirective) { isolateScope = scope.$new(true); + } else if (newScopeDirective) { + controllerScope = scope.$parent; } if (boundTranscludeFn) { @@ -2133,7 +2136,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { if (controller.identifier && bindings) { removeControllerBindingWatches = - initializeDirectiveBindings(scope, attrs, controller.instance, bindings, controllerDirective); + initializeDirectiveBindings(controllerScope, attrs, controller.instance, bindings, controllerDirective); } var controllerResult = controller(); @@ -2144,7 +2147,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { $element.data('$' + controllerDirective.name + 'Controller', controllerResult); removeControllerBindingWatches && removeControllerBindingWatches(); removeControllerBindingWatches = - initializeDirectiveBindings(scope, attrs, controller.instance, bindings, controllerDirective); + initializeDirectiveBindings(controllerScope, attrs, controller.instance, bindings, controllerDirective); } } diff --git a/test/ng/compileSpec.js b/test/ng/compileSpec.js index 136bec90a25b..db48aa665770 100755 --- a/test/ng/compileSpec.js +++ b/test/ng/compileSpec.js @@ -4591,6 +4591,124 @@ describe('$compile', function() { }); + it('should evaluate against the correct scope, when using `bindToController` (new scope)', + function() { + module(function($compileProvider, $controllerProvider) { + $controllerProvider.register({ + 'ParentCtrl': function() { + this.value1 = 'parent1'; + this.value2 = 'parent2'; + this.value3 = function() { return 'parent3'; }; + }, + 'ChildCtrl': function() { + this.value1 = 'child1'; + this.value2 = 'child2'; + this.value3 = function() { return 'child3'; }; + } + }); + + $compileProvider.directive('child', valueFn({ + scope: true, + controller: 'ChildCtrl as ctrl', + bindToController: { + fromParent1: '@', + fromParent2: '=', + fromParent3: '&' + }, + template: '' + })); + }); + + inject(function($compile, $rootScope) { + element = $compile( + '
' + + '' + + '' + + '
')($rootScope); + $rootScope.$digest(); + + var parentCtrl = element.controller('ngController'); + var childCtrl = element.find('child').controller('child'); + + expect(childCtrl.fromParent1).toBe(parentCtrl.value1); + expect(childCtrl.fromParent1).not.toBe(childCtrl.value1); + expect(childCtrl.fromParent2).toBe(parentCtrl.value2); + expect(childCtrl.fromParent2).not.toBe(childCtrl.value2); + expect(childCtrl.fromParent3()()).toBe(parentCtrl.value3()); + expect(childCtrl.fromParent3()()).not.toBe(childCtrl.value3()); + + childCtrl.fromParent2 = 'modified'; + $rootScope.$digest(); + + expect(parentCtrl.value2).toBe('modified'); + expect(childCtrl.value2).toBe('child2'); + }); + } + ); + + + it('should evaluate against the correct scope, when using `bindToController` (new iso scope)', + function() { + module(function($compileProvider, $controllerProvider) { + $controllerProvider.register({ + 'ParentCtrl': function() { + this.value1 = 'parent1'; + this.value2 = 'parent2'; + this.value3 = function() { return 'parent3'; }; + }, + 'ChildCtrl': function() { + this.value1 = 'child1'; + this.value2 = 'child2'; + this.value3 = function() { return 'child3'; }; + } + }); + + $compileProvider.directive('child', valueFn({ + scope: {}, + controller: 'ChildCtrl as ctrl', + bindToController: { + fromParent1: '@', + fromParent2: '=', + fromParent3: '&' + }, + template: '' + })); + }); + + inject(function($compile, $rootScope) { + element = $compile( + '
' + + '' + + '' + + '
')($rootScope); + $rootScope.$digest(); + + var parentCtrl = element.controller('ngController'); + var childCtrl = element.find('child').controller('child'); + + expect(childCtrl.fromParent1).toBe(parentCtrl.value1); + expect(childCtrl.fromParent1).not.toBe(childCtrl.value1); + expect(childCtrl.fromParent2).toBe(parentCtrl.value2); + expect(childCtrl.fromParent2).not.toBe(childCtrl.value2); + expect(childCtrl.fromParent3()()).toBe(parentCtrl.value3()); + expect(childCtrl.fromParent3()()).not.toBe(childCtrl.value3()); + + childCtrl.fromParent2 = 'modified'; + $rootScope.$digest(); + + expect(parentCtrl.value2).toBe('modified'); + expect(childCtrl.value2).toBe('child2'); + }); + } + ); + + it('should put controller in scope when controller identifier present but not using controllerAs', function() { var controllerCalled = false; var myCtrl; From bd7b2177291697a665e4068501b3704200972467 Mon Sep 17 00:00:00 2001 From: Peter Bacon Darwin Date: Tue, 10 Nov 2015 10:52:18 +0000 Subject: [PATCH 4/4] fix($compile): bind all directive controllers correctly when using `bindToController` Previously only the first directive's controller would be bound correctly. Closes #11343 Closes #11345 --- test/ng/compileSpec.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/ng/compileSpec.js b/test/ng/compileSpec.js index db48aa665770..ee09676bc4dd 100755 --- a/test/ng/compileSpec.js +++ b/test/ng/compileSpec.js @@ -4482,7 +4482,7 @@ describe('$compile', function() { 'str': '@fooStr', 'fn': '&fooFn' }, - scope: true, + scope: {}, controllerAs: 'fooCtrl', controller: function() { expect(this.data).toEqualData({'foo': 'bar', 'baz': 'biz'});