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 angular#3969 Fixes angular#4277
mgol · Jun 24, 2014 · 9061552 · 9061552
1 parent 1f6a5a1
commit 9061552
Show file tree

Hide file tree

Showing 10 changed files with 124 additions and 65 deletions.
diff --git 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
@@ -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
@@ -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. <input ng-model="foo" ng-trim="false">
-    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
@@ -85,9 +85,9 @@ var ngIfDirective = ['$animate', function($animate) {
     $$tlb: true,
     link: function ($scope, $element, $attr, ctrl, $transclude) {
         var block, childScope, previousElements;
-        $scope.$watch($attr.ngIf, function ngIfWatchAction(value) {
+        $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
@@ -19,15 +19,10 @@
  * <div ng-show="myValue" class="ng-hide"></div>
  * ```
  *
- * 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.
  *
- * <div class="alert alert-warning">
- * **Note:** Here is a list of values that ngShow will consider as a falsy value (case insensitive):<br />
- * "f" / "0" / "false" / "no" / "n" / "[]"
- * </div>
- *
  * ## 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
@@ -162,8 +157,8 @@
  */
 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');
+    scope.$watch('!!(' + attr.ngShow + ')', function ngShowWatchAction(value){
+      $animate[value ? 'removeClass' : 'addClass'](element, 'ng-hide');
     });
   };
 }];
@@ -188,15 +183,10 @@ var ngShowDirective = ['$animate', function($animate) {
  * <div ng-hide="myValue"></div>
  * ```
  *
- * 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.
  *
- * <div class="alert alert-warning">
- * **Note:** Here is a list of values that ngHide will consider as a falsy value (case insensitive):<br />
- * "f" / "0" / "false" / "no" / "n" / "[]"
- * </div>
- *
  * ## 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
@@ -318,8 +308,8 @@ 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');
+    scope.$watch('!!(' + attr.ngHide + ')', function ngHideWatchAction(value){
+      $animate[value ? 'addClass' : 'removeClass'](element, 'ng-hide');
     });
   };
 }];
diff --git 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
@@ -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
@@ -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
@@ -16,13 +16,15 @@ describe('ngIf', function () {
     dealoc(element);
   });
 
-  function makeIf(expr) {
-    element.append($compile('<div class="my-class" ng-if="' + expr + '"><div>Hi</div></div>')($scope));
+  function makeIf() {
+    forEach(arguments, function (expr) {
+      element.append($compile('<div class="my-class" ng-if="' + expr + '"><div>Hi</div></div>')($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
@@ -1,54 +1,106 @@
 'use strict';
 
 describe('ngShow / ngHide', function() {
-  var element;
+  var $scope, $compile, element;
 
+  function expectVisibility(exprs, ngShowOrNgHide, shownOrHidden) {
+    element = $compile('<div></div>')($scope);
+    forEach(exprs, function (expr) {
+      var childElem = $compile('<div ' + ngShowOrNgHide + '="' + expr + '"></div>')($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('<div ng-show="exp"></div>');
-      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('<div ng-show="exp"></div>');
-      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('<div class="ng-hide" ng-show="exp"></div>');
-      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('<div ng-hide="exp"></div>');
-      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('[]', '{}');
+    });
   });
 });