From 69f1ebd12430561c1682ced8f2d729d28a73f2e4 Mon Sep 17 00:00:00 2001 From: Caitlin Potter Date: Tue, 7 Jul 2015 08:27:42 -0400 Subject: [PATCH 1/2] revert "revert: "fix($compile): do not write @-bound properties if attribute is not present"" This reverts commit d193c3a25caa0d2c6dd149941c23163dbd062e4d. --- src/ng/compile.js | 27 ++++----- test/ng/compileSpec.js | 124 +++++++++++++++++++++++++++++++++++++---- 2 files changed, 124 insertions(+), 27 deletions(-) diff --git a/src/ng/compile.js b/src/ng/compile.js index 7a926f309fdc..a844f23e10bc 100644 --- a/src/ng/compile.js +++ b/src/ng/compile.js @@ -2567,24 +2567,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); @@ -2592,11 +2587,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 { @@ -2635,7 +2631,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 0239b8cd2cac..c6e49603b4f4 100755 --- a/test/ng/compileSpec.js +++ b/test/ng/compileSpec.js @@ -2543,10 +2543,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); }) ); @@ -2561,10 +2568,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); }) ); @@ -2575,10 +2585,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); }) ); @@ -3576,6 +3593,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) { @@ -4437,6 +4479,64 @@ describe('$compile', function() { childScope.theCtrl.test(); }); }); + + describe('should not overwrite @-bound property each digest when not present', function() { + it('when creating new scope', function() { + module(function($compileProvider) { + $compileProvider.directive('testDir', valueFn({ + scope: true, + 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.scope(); + expect(scope.ctrl.getProp()).toBe('default'); + + $rootScope.$digest(); + expect(scope.ctrl.getProp()).toBe('default'); + }); + }); + + it('when creating isolate scope', 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'); + }); + }); + }); }); From df73233d573ab8f70da9c1e8fd9067dc5b9b48a4 Mon Sep 17 00:00:00 2001 From: Caitlin Potter Date: Tue, 7 Jul 2015 14:07:44 -0400 Subject: [PATCH 2/2] fix($compile): ignore optional =-bound properties with empty value Previously, optional empty '='-bound expressions were ignored --- erroneously they stopped being ignored, and no tests were caused to fail for this reason. This change restores the original ignoring behaviour while still preventing other errors fixed by 8a1eb16 Closes #12144 Closes #12259 --- src/ng/compile.js | 1 + test/ng/compileSpec.js | 27 +++++++++++++++++++++++++++ 2 files changed, 28 insertions(+) diff --git a/src/ng/compile.js b/src/ng/compile.js index a844f23e10bc..b0b2d63940c8 100644 --- a/src/ng/compile.js +++ b/src/ng/compile.js @@ -2591,6 +2591,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { if (optional) break; attrs[attrName] = void 0; } + if (optional && !attrs[attrName]) break; parentGet = $parse(attrs[attrName]); if (parentGet.literal) { diff --git a/test/ng/compileSpec.js b/test/ng/compileSpec.js index c6e49603b4f4..02ea29cc9545 100755 --- a/test/ng/compileSpec.js +++ b/test/ng/compileSpec.js @@ -3618,6 +3618,33 @@ describe('$compile', function() { }); + it('should ignore optional "="-bound property if value is the emptry string', 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'); + scope.prop = 'foop'; + $rootScope.$digest(); + expect(scope.ctrl.getProp()).toBe('foop'); + }); + }); + + describe('bind-once', function() { function countWatches(scope) {