From 914fa11cdf7175f7bb4e6d9ca38a9360b553bb3a Mon Sep 17 00:00:00 2001 From: Caitlin Potter Date: Wed, 17 Jun 2015 11:21:58 -0400 Subject: [PATCH] fix($compile): be more careful about shadowing $attrs values Shadows only when attributes are non-optional and not own properties, only stores the observed '@' values when they are indeed strings. Partial revert of 6339d30d1379689da5eec9647a953f64821f8b0 --- src/ng/compile.js | 27 ++++++------ test/ng/compileSpec.js | 94 ++++++++++++++++++++++++++++++++++++------ 2 files changed, 94 insertions(+), 27 deletions(-) diff --git a/src/ng/compile.js b/src/ng/compile.js index a22c90b4c8e7..85728ea01315 100644 --- a/src/ng/compile.js +++ b/src/ng/compile.js @@ -2566,24 +2566,19 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { lastValue, parentGet, parentSet, compare; - if (!hasOwnProperty.call(attrs, attrName)) { - // In the case of user defined a binding with the same name as a method in Object.prototype but didn't set - // the corresponding attribute. We need to make sure subsequent code won't access to the prototype function - attrs[attrName] = undefined; - } - switch (mode) { case '@': - if (!attrs[attrName] && !optional) { - destination[scopeName] = undefined; + if (!optional && !hasOwnProperty.call(attrs, attrName)) { + destination[scopeName] = attrs[attrName] = void 0; } - attrs.$observe(attrName, function(value) { - destination[scopeName] = value; + if (isString(value)) { + destination[scopeName] = value; + } }); attrs.$$observers[attrName].$$scope = scope; - if (attrs[attrName]) { + if (isString(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 destination[scopeName] = $interpolate(attrs[attrName])(scope); @@ -2591,11 +2586,12 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { break; case '=': - if (optional && !attrs[attrName]) { - return; + if (!hasOwnProperty.call(attrs, attrName)) { + if (optional) break; + attrs[attrName] = void 0; } - parentGet = $parse(attrs[attrName]); + parentGet = $parse(attrs[attrName]); if (parentGet.literal) { compare = equals; } else { @@ -2634,7 +2630,8 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { break; case '&': - parentGet = $parse(attrs[attrName]); + // Don't assign Object.prototype method to scope + parentGet = attrs.hasOwnProperty(attrName) ? $parse(attrs[attrName]) : noop; // Don't assign noop to destination if expression is not valid if (parentGet === noop && optional) break; diff --git a/test/ng/compileSpec.js b/test/ng/compileSpec.js index c90b51c5de79..253ae6e50028 100755 --- a/test/ng/compileSpec.js +++ b/test/ng/compileSpec.js @@ -2521,10 +2521,17 @@ describe('$compile', function() { }; expect(func).not.toThrow(); - expect(element.find('span').scope()).toBe(element.isolateScope()); - expect(element.isolateScope()).not.toBe($rootScope); - expect(element.isolateScope()['constructor']).toBe($rootScope.constructor); - expect(element.isolateScope()['valueOf']).toBeUndefined(); + var scope = element.isolateScope(); + expect(element.find('span').scope()).toBe(scope); + expect(scope).not.toBe($rootScope); + + // Not shadowed because optional + expect(scope.constructor).toBe($rootScope.constructor); + expect(scope.hasOwnProperty('constructor')).toBe(false); + + // Shadowed with undefined because not optional + expect(scope.valueOf).toBeUndefined(); + expect(scope.hasOwnProperty('valueOf')).toBe(true); }) ); @@ -2539,10 +2546,13 @@ describe('$compile', function() { }; expect(func).not.toThrow(); - expect(element.find('span').scope()).toBe(element.isolateScope()); - expect(element.isolateScope()).not.toBe($rootScope); - expect(element.isolateScope()['constructor']).toBe('constructor'); - expect(element.isolateScope()['valueOf']).toBe('valueOf'); + var scope = element.isolateScope(); + expect(element.find('span').scope()).toBe(scope); + expect(scope).not.toBe($rootScope); + expect(scope.constructor).toBe('constructor'); + expect(scope.hasOwnProperty('constructor')).toBe(true); + expect(scope.valueOf).toBe('valueOf'); + expect(scope.hasOwnProperty('valueOf')).toBe(true); }) ); @@ -2553,10 +2563,17 @@ describe('$compile', function() { }; expect(func).not.toThrow(); - expect(element.find('span').scope()).toBe(element.isolateScope()); - expect(element.isolateScope()).not.toBe($rootScope); - expect(element.isolateScope()['constructor']).toBe($rootScope.constructor); - expect(element.isolateScope()['valueOf']).toBeUndefined(); + var scope = element.isolateScope(); + expect(element.find('span').scope()).toBe(scope); + expect(scope).not.toBe($rootScope); + + // Does not shadow value because optional + expect(scope.constructor).toBe($rootScope.constructor); + expect(scope.hasOwnProperty('constructor')).toBe(false); + + // Shadows value because not optional + expect(scope.valueOf).toBeUndefined(); + expect(scope.hasOwnProperty('valueOf')).toBe(true); }) ); @@ -3554,6 +3571,31 @@ describe('$compile', function() { })); + it('should not overwrite @-bound property each digest when not present', function() { + module(function($compileProvider) { + $compileProvider.directive('testDir', valueFn({ + scope: {prop: '@'}, + controller: function($scope) { + $scope.prop = $scope.prop || 'default'; + this.getProp = function() { + return $scope.prop; + }; + }, + controllerAs: 'ctrl', + template: '

' + })); + }); + inject(function($compile, $rootScope) { + element = $compile('
')($rootScope); + var scope = element.isolateScope(); + expect(scope.ctrl.getProp()).toBe('default'); + + $rootScope.$digest(); + expect(scope.ctrl.getProp()).toBe('default'); + }); + }); + + describe('bind-once', function() { function countWatches(scope) { @@ -4415,6 +4457,34 @@ describe('$compile', function() { childScope.theCtrl.test(); }); }); + + it('should not overwrite @-bound property each digest when not present', function() { + module(function($compileProvider) { + $compileProvider.directive('testDir', valueFn({ + scope: {}, + bindToController: { + prop: '@' + }, + controller: function() { + var self = this; + this.prop = this.prop || 'default'; + this.getProp = function() { + return self.prop; + }; + }, + controllerAs: 'ctrl', + template: '

' + })); + }); + inject(function($compile, $rootScope) { + element = $compile('
')($rootScope); + var scope = element.isolateScope(); + expect(scope.ctrl.getProp()).toBe('default'); + + $rootScope.$digest(); + expect(scope.ctrl.getProp()).toBe('default'); + }); + }); });