From 5f760668d8e5632d1fdac696e296fdf777c19408 Mon Sep 17 00:00:00 2001 From: Vojta Jina Date: Tue, 5 Nov 2013 12:31:20 -0800 Subject: [PATCH] fix($compile): make isolate scope truly isolate Fixes issue with isolate scope leaking all over the place into other directives on the same element. Isolate scope is now available only to the isolate directive that requested it and its template. A non-isolate directive should not get the isolate scope of an isolate directive on the same element, instead they will receive the original scope (which is the parent scope of the newly created isolate scope). Paired with Tobias. BREAKING CHANGE: Directives without isolate scope do not get the isolate scope from an isolate directive on the same element. If your code depends on this behavior (non-isolate directive needs to access state from within the isolate scope), change the isolate directive to use scope locals to pass these explicitly. // before .directive('ngIsolate', function() { return { scope: {}, template: '{{value}}' }; }); // after .directive('ngIsolate', function() { return { scope: {value: '=ngModel'}, template: '{{value}} }; }); Closes #1924 Closes #2500 --- src/ng/compile.js | 56 +++++++++++++++------------ test/ng/compileSpec.js | 87 ++++++++++++++++++++++++++++++++++++++++-- xx | 0 3 files changed, 114 insertions(+), 29 deletions(-) create mode 100644 xx diff --git a/src/ng/compile.js b/src/ng/compile.js index d3ade6b82360..adcbb6afda1e 100644 --- a/src/ng/compile.js +++ b/src/ng/compile.js @@ -901,7 +901,7 @@ function $CompileProvider($provide) { return linkFnFound ? compositeLinkFn : null; function compositeLinkFn(scope, nodeList, $rootElement, boundTranscludeFn) { - var nodeLinkFn, childLinkFn, node, childScope, childTranscludeFn, i, ii, n; + var nodeLinkFn, childLinkFn, node, $node, childScope, childTranscludeFn, i, ii, n; // copy nodeList so that linking doesn't break due to live list updates. var stableNodeList = []; @@ -913,11 +913,13 @@ function $CompileProvider($provide) { node = stableNodeList[n]; nodeLinkFn = linkFns[i++]; childLinkFn = linkFns[i++]; + $node = jqLite(node); if (nodeLinkFn) { if (nodeLinkFn.scope) { - childScope = scope.$new(isObject(nodeLinkFn.scope)); - jqLite(node).data('$scope', childScope); + childScope = scope.$new(); + $node.data('$scope', childScope); + safeAddClass($node, 'ng-scope'); } else { childScope = scope; } @@ -1155,10 +1157,8 @@ function $CompileProvider($provide) { assertNoDuplicate('new/isolated scope', newIsolateScopeDirective, directive, $compileNode); if (isObject(directiveValue)) { - safeAddClass($compileNode, 'ng-isolate-scope'); newIsolateScopeDirective = directive; } - safeAddClass($compileNode, 'ng-scope'); } } @@ -1291,7 +1291,7 @@ function $CompileProvider($provide) { } - nodeLinkFn.scope = newScopeDirective && newScopeDirective.scope; + nodeLinkFn.scope = newScopeDirective && newScopeDirective.scope === true; nodeLinkFn.transclude = transcludeDirective && childTranscludeFn; // might be normal or delayed nodeLinkFn depending on if templateUrl is present @@ -1303,11 +1303,13 @@ function $CompileProvider($provide) { if (pre) { if (attrStart) pre = groupElementsLinkFnWrapper(pre, attrStart, attrEnd); pre.require = directive.require; + if (newIsolateScopeDirective === directive) pre.isolateScope = true; preLinkFns.push(pre); } if (post) { if (attrStart) post = groupElementsLinkFnWrapper(post, attrStart, attrEnd); post.require = directive.require; + if (newIsolateScopeDirective === directive) post.isolateScope = true; postLinkFns.push(post); } } @@ -1348,7 +1350,7 @@ function $CompileProvider($provide) { function nodeLinkFn(childLinkFn, scope, linkNode, $rootElement, boundTranscludeFn) { - var attrs, $element, i, ii, linkFn, controller; + var attrs, $element, i, ii, linkFn, controller, isolateScope; if (compileNode === linkNode) { attrs = templateAttrs; @@ -1359,8 +1361,11 @@ function $CompileProvider($provide) { if (newIsolateScopeDirective) { var LOCAL_REGEXP = /^\s*([@=&])(\??)\s*(\w*)\s*$/; + var $linkNode = jqLite(linkNode); - var parentScope = scope.$parent || scope; + isolateScope = scope.$new(true); + $linkNode.data('$isolateScope', isolateScope); + safeAddClass($linkNode, 'ng-isolate-scope'); forEach(newIsolateScopeDirective.scope, function(definition, scopeName) { var match = definition.match(LOCAL_REGEXP) || [], @@ -1370,19 +1375,19 @@ function $CompileProvider($provide) { lastValue, parentGet, parentSet; - scope.$$isolateBindings[scopeName] = mode + attrName; + isolateScope.$$isolateBindings[scopeName] = mode + attrName; switch (mode) { case '@': attrs.$observe(attrName, function(value) { - scope[scopeName] = value; + isolateScope[scopeName] = value; }); - attrs.$$observers[attrName].$$scope = parentScope; + attrs.$$observers[attrName].$$scope = scope; if( attrs[attrName] ) { // If the attribute has been provided then we trigger an interpolation to ensure // the value is there for use in the link fn - scope[scopeName] = $interpolate(attrs[attrName])(parentScope); + isolateScope[scopeName] = $interpolate(attrs[attrName])(scope); } break; @@ -1393,23 +1398,23 @@ function $CompileProvider($provide) { parentGet = $parse(attrs[attrName]); parentSet = parentGet.assign || function() { // reset the change, or we will throw this exception on every $digest - lastValue = scope[scopeName] = parentGet(parentScope); + lastValue = isolateScope[scopeName] = parentGet(scope); throw $compileMinErr('nonassign', "Expression '{0}' used with directive '{1}' is non-assignable!", attrs[attrName], newIsolateScopeDirective.name); }; - lastValue = scope[scopeName] = parentGet(parentScope); - scope.$watch(function parentValueWatch() { - var parentValue = parentGet(parentScope); + lastValue = isolateScope[scopeName] = parentGet(scope); + isolateScope.$watch(function parentValueWatch() { + var parentValue = parentGet(scope); - if (parentValue !== scope[scopeName]) { + if (parentValue !== isolateScope[scopeName]) { // we are out of sync and need to copy if (parentValue !== lastValue) { // parent changed and it has precedence - lastValue = scope[scopeName] = parentValue; + lastValue = isolateScope[scopeName] = parentValue; } else { // if the parent can be assigned then do so - parentSet(parentScope, parentValue = lastValue = scope[scopeName]); + parentSet(scope, parentValue = lastValue = isolateScope[scopeName]); } } return parentValue; @@ -1418,8 +1423,8 @@ function $CompileProvider($provide) { case '&': parentGet = $parse(attrs[attrName]); - scope[scopeName] = function(locals) { - return parentGet(parentScope, locals); + isolateScope[scopeName] = function(locals) { + return parentGet(scope, locals); }; break; @@ -1435,7 +1440,7 @@ function $CompileProvider($provide) { if (controllerDirectives) { forEach(controllerDirectives, function(directive) { var locals = { - $scope: scope, + $scope: directive === newIsolateScopeDirective ? isolateScope : scope, $element: $element, $attrs: attrs, $transclude: boundTranscludeFn @@ -1467,7 +1472,7 @@ function $CompileProvider($provide) { for(i = 0, ii = preLinkFns.length; i < ii; i++) { try { linkFn = preLinkFns[i]; - linkFn(scope, $element, attrs, + linkFn(linkFn.isolateScope ? isolateScope : scope, $element, attrs, linkFn.require && getControllers(linkFn.require, $element)); } catch (e) { $exceptionHandler(e, startingTag($element)); @@ -1475,13 +1480,14 @@ function $CompileProvider($provide) { } // RECURSION - childLinkFn && childLinkFn(scope, linkNode.childNodes, undefined, boundTranscludeFn); + // TODO(vojta): only pass isolate if the isolate directive has template + childLinkFn && childLinkFn(isolateScope || scope, linkNode.childNodes, undefined, boundTranscludeFn); // POSTLINKING for(i = postLinkFns.length - 1; i >= 0; i--) { try { linkFn = postLinkFns[i]; - linkFn(scope, $element, attrs, + linkFn(linkFn.isolateScope ? isolateScope : scope, $element, attrs, linkFn.require && getControllers(linkFn.require, $element)); } catch (e) { $exceptionHandler(e, startingTag($element)); diff --git a/test/ng/compileSpec.js b/test/ng/compileSpec.js index c6cba3a42fb1..0904888533d4 100755 --- a/test/ng/compileSpec.js +++ b/test/ng/compileSpec.js @@ -1375,7 +1375,7 @@ describe('$compile', function() { return function (scope, element) { iscope = scope; log(scope.$id); - expect(element.data('$scope')).toBe(scope); + expect(element.data('$isolateScope')).toBe(scope); }; } }; @@ -1416,7 +1416,7 @@ describe('$compile', function() { return function (scope, element) { iscope = scope; log(scope.$id); - expect(element.data('$scope')).toBe(scope); + expect(element.data('$isolateScope')).toBe(scope); }; } }; @@ -1535,7 +1535,7 @@ describe('$compile', function() { expect(function(){ $compile('
'); }).toThrowMinErr('$compile', 'multidir', 'Multiple directives [iscopeA, scopeB] asking for new/isolated scope on: ' + - '
'); + '
'); }) ); @@ -2085,7 +2085,7 @@ describe('$compile', function() { describe('isolated locals', function() { - var componentScope; + var componentScope, regularScope; beforeEach(module(function() { directive('myComponent', function() { @@ -2112,6 +2112,23 @@ describe('$compile', function() { scope: { attr: 'xxx' } }; }); + directive('storeScope', function() { + return { + link: function(scope) { + regularScope = scope; + } + } + }); + })); + + it('should give other directives the parent scope', inject(function($rootScope) { + compile('
'); + $rootScope.$apply(function() { + $rootScope.value = 'from-parent'; + }); + expect(element.find('input').val()).toBe('from-parent'); + expect(componentScope).not.toBe(regularScope); + expect(componentScope.$parent).toBe(regularScope) })); describe('attribute', function() { @@ -2376,6 +2393,68 @@ describe('$compile', function() { }); + it('should require controller of an isolate directive from a non-isolate directive on the ' + + 'same element', function() { + var IsolateController = function() {}; + var isolateDirControllerInNonIsolateDirective; + + module(function() { + directive('isolate', function() { + return { + scope: {}, + controller: IsolateController + }; + }); + directive('nonIsolate', function() { + return { + require: 'isolate', + link: function(_, __, ___, isolateDirController) { + isolateDirControllerInNonIsolateDirective = isolateDirController; + } + }; + }); + }); + + inject(function($compile, $rootScope) { + element = $compile('
')($rootScope); + + expect(isolateDirControllerInNonIsolateDirective).toBeDefined(); + expect(isolateDirControllerInNonIsolateDirective instanceof IsolateController).toBe(true); + }); + }); + + + it('should require controller of a non-isolate directive from an isolate directive on the ' + + 'same element', function() { + var NonIsolateController = function() {}; + var nonIsolateDirControllerInIsolateDirective; + + module(function() { + directive('isolate', function() { + return { + scope: {}, + require: 'nonIsolate', + link: function(_, __, ___, nonIsolateDirController) { + nonIsolateDirControllerInIsolateDirective = nonIsolateDirController; + } + }; + }); + directive('nonIsolate', function() { + return { + controller: NonIsolateController + }; + }); + }); + + inject(function($compile, $rootScope) { + element = $compile('
')($rootScope); + + expect(nonIsolateDirControllerInIsolateDirective).toBeDefined(); + expect(nonIsolateDirControllerInIsolateDirective instanceof NonIsolateController).toBe(true); + }); + }); + + it('should support controllerAs', function() { module(function() { directive('main', function() { diff --git a/xx b/xx new file mode 100644 index 000000000000..e69de29bb2d1