From 9355e310ebc1bd2c57c41ab5d211d5ce65dfc03c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Go=C5=82e=CC=A8biowski?= Date: Tue, 24 Jun 2014 00:07:30 +0200 Subject: [PATCH] fix(core): drop the toBoolean function So far Angular have used the toBoolean function to decide if the parsed value is truthy. The function made more values falsy than regular JavaScript would, e.g. strings 'f' and 'no' were both treated as falsy. This creates suble bugs when backend sends a non-empty string with one of these values and something suddenly hides in the application Thanks to lgalfaso for test ideas. BREAKING CHANGE: values 'f', '0', 'false', 'no', 'n', '[]' are no longer treated as falsy. Only JavaScript falsy values are now treated as falsy by the expression parser; there are six of them: false, null, undefined, NaN, 0 and "". Fixes #3969 Fixes #4277 --- src/.jshintrc | 1 - src/Angular.js | 13 ---- src/ng/directive/input.js | 2 +- src/ng/directive/ngIf.js | 2 +- src/ng/directive/ngShowHide.js | 22 ++---- src/ng/filter/orderBy.js | 2 +- test/.jshintrc | 1 - test/BinderSpec.js | 20 ++++++ test/ng/directive/ngIfSpec.js | 20 ++++-- test/ng/directive/ngShowHideSpec.js | 100 +++++++++++++++++++++------- 10 files changed, 121 insertions(+), 62 deletions(-) diff --git a/src/.jshintrc b/src/.jshintrc index 589805d63ede..f1b1c8cc8549 100644 --- a/src/.jshintrc +++ b/src/.jshintrc @@ -85,7 +85,6 @@ "toJsonReplacer": false, "toJson": false, "fromJson": false, - "toBoolean": false, "startingTag": false, "tryDecodeURIComponent": false, "parseKeyValue": false, diff --git a/src/Angular.js b/src/Angular.js index 5aa4da1949e0..8daf1e417140 100644 --- a/src/Angular.js +++ b/src/Angular.js @@ -67,7 +67,6 @@ -toJsonReplacer, -toJson, -fromJson, - -toBoolean, -startingTag, -tryDecodeURIComponent, -parseKeyValue, @@ -1033,18 +1032,6 @@ function fromJson(json) { } -function toBoolean(value) { - if (typeof value === 'function') { - value = true; - } else if (value && value.length !== 0) { - var v = lowercase("" + value); - value = !(v == 'f' || v == '0' || v == 'false' || v == 'no' || v == 'n' || v == '[]'); - } else { - value = false; - } - return value; -} - /** * @returns {string} Returns the string representation of the element. */ diff --git a/src/ng/directive/input.js b/src/ng/directive/input.js index 177919f116e3..df038ef2c2df 100644 --- a/src/ng/directive/input.js +++ b/src/ng/directive/input.js @@ -932,7 +932,7 @@ function textInputType(scope, element, attr, ctrl, $sniffer, $browser) { // By default we will trim the value // If the attribute ng-trim exists we will avoid trimming // e.g. - if (toBoolean(attr.ngTrim || 'T')) { + if (!attr.ngTrim || attr.ngTrim !== 'false') { value = trim(value); } diff --git a/src/ng/directive/ngIf.js b/src/ng/directive/ngIf.js index f75674039a1d..4c37bde87bfb 100644 --- a/src/ng/directive/ngIf.js +++ b/src/ng/directive/ngIf.js @@ -87,7 +87,7 @@ var ngIfDirective = ['$animate', function($animate) { var block, childScope, previousElements; $scope.$watch($attr.ngIf, function ngIfWatchAction(value) { - if (toBoolean(value)) { + if (value) { if (!childScope) { $transclude(function (clone, newScope) { childScope = newScope; diff --git a/src/ng/directive/ngShowHide.js b/src/ng/directive/ngShowHide.js index a04357eca07d..f3dfe10d66ac 100644 --- a/src/ng/directive/ngShowHide.js +++ b/src/ng/directive/ngShowHide.js @@ -19,15 +19,10 @@ *
* ``` * - * When the ngShow expression evaluates to false then the ng-hide CSS class is added to the class attribute - * on the element causing it to become hidden. When true, the ng-hide CSS class is removed + * When the ngShow expression evaluates to a falsy value then the ng-hide CSS class is added to the class + * attribute on the element causing it to become hidden. When truthy, the ng-hide CSS class is removed * from the element causing the element not to appear hidden. * - *
- * **Note:** Here is a list of values that ngShow will consider as a falsy value (case insensitive):
- * "f" / "0" / "false" / "no" / "n" / "[]" - *
- * * ## Why is !important used? * * You may be wondering why !important is used for the .ng-hide CSS class. This is because the `.ng-hide` selector @@ -163,7 +158,7 @@ var ngShowDirective = ['$animate', function($animate) { return function(scope, element, attr) { scope.$watch(attr.ngShow, function ngShowWatchAction(value){ - $animate[toBoolean(value) ? 'removeClass' : 'addClass'](element, 'ng-hide'); + $animate[value ? 'removeClass' : 'addClass'](element, 'ng-hide'); }); }; }]; @@ -188,15 +183,10 @@ var ngShowDirective = ['$animate', function($animate) { *
* ``` * - * When the ngHide expression evaluates to true then the .ng-hide CSS class is added to the class attribute - * on the element causing it to become hidden. When false, the ng-hide CSS class is removed + * When the ngHide expression evaluates to a truthy value then the .ng-hide CSS class is added to the class + * attribute on the element causing it to become hidden. When falsy, the ng-hide CSS class is removed * from the element causing the element not to appear hidden. * - *
- * **Note:** Here is a list of values that ngHide will consider as a falsy value (case insensitive):
- * "f" / "0" / "false" / "no" / "n" / "[]" - *
- * * ## Why is !important used? * * You may be wondering why !important is used for the .ng-hide CSS class. This is because the `.ng-hide` selector @@ -319,7 +309,7 @@ var ngShowDirective = ['$animate', function($animate) { var ngHideDirective = ['$animate', function($animate) { return function(scope, element, attr) { scope.$watch(attr.ngHide, function ngHideWatchAction(value){ - $animate[toBoolean(value) ? 'addClass' : 'removeClass'](element, 'ng-hide'); + $animate[value ? 'addClass' : 'removeClass'](element, 'ng-hide'); }); }; }]; diff --git a/src/ng/filter/orderBy.js b/src/ng/filter/orderBy.js index c20add53dc5d..4efd9e19d9a6 100644 --- a/src/ng/filter/orderBy.js +++ b/src/ng/filter/orderBy.js @@ -144,7 +144,7 @@ function orderByFilter($parse){ return 0; } function reverseComparator(comp, descending) { - return toBoolean(descending) + return descending ? function(a,b){return comp(b,a);} : comp; } diff --git a/test/.jshintrc b/test/.jshintrc index 8c9f8748995d..0127b87819e7 100644 --- a/test/.jshintrc +++ b/test/.jshintrc @@ -85,7 +85,6 @@ "toJsonReplacer": false, "toJson": false, "fromJson": false, - "toBoolean": false, "startingTag": false, "tryDecodeURIComponent": false, "parseKeyValue": false, diff --git a/test/BinderSpec.js b/test/BinderSpec.js index 57f529b0a520..23519fe8fc17 100644 --- a/test/BinderSpec.js +++ b/test/BinderSpec.js @@ -248,6 +248,16 @@ describe('Binder', function() { $rootScope.hidden = 'false'; $rootScope.$apply(); + assertHidden(element); + + $rootScope.hidden = 0; + $rootScope.$apply(); + + assertVisible(element); + + $rootScope.hidden = false; + $rootScope.$apply(); + assertVisible(element); $rootScope.hidden = ''; @@ -267,6 +277,16 @@ describe('Binder', function() { $rootScope.show = 'false'; $rootScope.$apply(); + assertVisible(element); + + $rootScope.show = false; + $rootScope.$apply(); + + assertHidden(element); + + $rootScope.show = false; + $rootScope.$apply(); + assertHidden(element); $rootScope.show = ''; diff --git a/test/ng/directive/ngIfSpec.js b/test/ng/directive/ngIfSpec.js index b18a4301570d..f952500ec90d 100755 --- a/test/ng/directive/ngIfSpec.js +++ b/test/ng/directive/ngIfSpec.js @@ -16,13 +16,15 @@ describe('ngIf', function () { dealoc(element); }); - function makeIf(expr) { - element.append($compile('
Hi
')($scope)); + function makeIf() { + forEach(arguments, function (expr) { + element.append($compile('
Hi
')($scope)); + }); $scope.$apply(); } - it('should immediately remove element if condition is false', function () { - makeIf('false'); + it('should immediately remove the element if condition is falsy', function () { + makeIf('false', 'undefined', 'null', 'NaN', '\'\'', '0'); expect(element.children().length).toBe(0); }); @@ -31,6 +33,16 @@ describe('ngIf', function () { expect(element.children().length).toBe(1); }); + it('should leave the element if the condition is a non-empty string', function () { + makeIf('\'f\'', '\'0\'', '\'false\'', '\'no\'', '\'n\'', '\'[]\''); + expect(element.children().length).toBe(6); + }); + + it('should leave the element if the condition is an object', function () { + makeIf('[]', '{}'); + expect(element.children().length).toBe(2); + }); + it('should not add the element twice if the condition goes from true to true', function () { $scope.hello = 'true1'; makeIf('hello'); diff --git a/test/ng/directive/ngShowHideSpec.js b/test/ng/directive/ngShowHideSpec.js index f1a1986af5f5..7c73c0aa4195 100644 --- a/test/ng/directive/ngShowHideSpec.js +++ b/test/ng/directive/ngShowHideSpec.js @@ -1,54 +1,106 @@ 'use strict'; describe('ngShow / ngHide', function() { - var element; + var $scope, $compile, element; + function expectVisibility(exprs, ngShowOrNgHide, shownOrHidden) { + element = $compile('
')($scope); + forEach(exprs, function (expr) { + var childElem = $compile('
')($scope); + element.append(childElem); + $scope.$digest(); + expect(childElem)[shownOrHidden === 'shown' ? 'toBeShown' : 'toBeHidden'](); + }); + } + + beforeEach(inject(function ($rootScope, _$compile_) { + $scope = $rootScope.$new(); + $compile = _$compile_; + })); afterEach(function() { dealoc(element); }); describe('ngShow', function() { - it('should show and hide an element', inject(function($rootScope, $compile) { + function expectShown() { + expectVisibility(arguments, 'ng-show', 'shown'); + } + + function expectHidden() { + expectVisibility(arguments, 'ng-show', 'hidden'); + } + + it('should show and hide an element', function() { element = jqLite('
'); - element = $compile(element)($rootScope); - $rootScope.$digest(); + element = $compile(element)($scope); + $scope.$digest(); expect(element).toBeHidden(); - $rootScope.exp = true; - $rootScope.$digest(); + $scope.exp = true; + $scope.$digest(); expect(element).toBeShown(); - })); - + }); // https://github.com/angular/angular.js/issues/5414 - it('should show if the expression is a function with a no arguments', inject(function($rootScope, $compile) { + it('should show if the expression is a function with a no arguments', function() { element = jqLite('
'); - element = $compile(element)($rootScope); - $rootScope.exp = function(){}; - $rootScope.$digest(); + element = $compile(element)($scope); + $scope.exp = function(){}; + $scope.$digest(); expect(element).toBeShown(); - })); - + }); - it('should make hidden element visible', inject(function($rootScope, $compile) { + it('should make hidden element visible', function() { element = jqLite('
'); - element = $compile(element)($rootScope); + element = $compile(element)($scope); expect(element).toBeHidden(); - $rootScope.exp = true; - $rootScope.$digest(); + $scope.exp = true; + $scope.$digest(); expect(element).toBeShown(); - })); + }); + + it('should hide the element if condition is falsy', function() { + expectHidden('false', 'undefined', 'null', 'NaN', '\'\'', '0'); + }); + + it('should show the element if condition is a non-empty string', function() { + expectShown('\'f\'', '\'0\'', '\'false\'', '\'no\'', '\'n\'', '\'[]\''); + }); + + it('should show the element if condition is an object', function() { + expectShown('[]', '{}'); + }); }); describe('ngHide', function() { - it('should hide an element', inject(function($rootScope, $compile) { + function expectShown() { + expectVisibility(arguments, 'ng-hide', 'shown'); + } + + function expectHidden() { + expectVisibility(arguments, 'ng-hide', 'hidden'); + } + + it('should hide an element', function() { element = jqLite('
'); - element = $compile(element)($rootScope); + element = $compile(element)($scope); expect(element).toBeShown(); - $rootScope.exp = true; - $rootScope.$digest(); + $scope.exp = true; + $scope.$digest(); expect(element).toBeHidden(); - })); + }); + + it('should show the element if condition is falsy', function() { + expectShown('false', 'undefined', 'null', 'NaN', '\'\'', '0'); + }); + + it('should hide the element if condition is a non-empty string', function() { + expectHidden('\'f\'', '\'0\'', '\'false\'', '\'no\'', '\'n\'', '\'[]\''); + }); + + it('should hide the element if condition is an object', function() { + expectHidden('[]', '{}'); + }); }); });