Skip to content

Commit

Permalink
fix(input): false is no longer an empty value by default
Browse files Browse the repository at this point in the history 
`checkboxInputType` and `ngList` directives need to have special logic for whether
they are empty or not.  Previously this had been hard coded into their
own directives or the `ngRequired` directive.  This made it difficult to handle
these special cases.

This change factors out the question of whether an input is empty into a method
`$isEmpty` on the `ngModelController`.  The `ngRequired` directive now uses this
method when testing for validity and directives, such as `checkbox` or `ngList`
can override it to apply logic specific to their needs.

Closes angular#3490, angular#3658, angular#2594
  • Loading branch information
@petebacondarwin
petebacondarwin committed Sep 25, 2013
1 parent f8f8f75 commit 5aa7600
Show file tree
Hide file tree
Showing 3 changed files with 102 additions and 16 deletions.
60 changes: 46 additions & 14 deletions src/ng/directive/input.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -445,7 +445,7 @@ function textInputType(scope, element, attr, ctrl, $sniffer, $browser) {


ctrl.$render = function() {
element.val(isEmpty(ctrl.$viewValue) ? '' : ctrl.$viewValue);
element.val(ctrl.$isEmpty(ctrl.$viewValue) ? '' : ctrl.$viewValue);
};

// pattern validator
Expand All @@ -454,7 +454,7 @@ function textInputType(scope, element, attr, ctrl, $sniffer, $browser) {
match;

var validate = function(regexp, value) {
if (isEmpty(value) || regexp.test(value)) {
if (ctrl.$isEmpty(value) || regexp.test(value)) {
ctrl.$setValidity('pattern', true);
return value;
} else {
Expand All @@ -468,7 +468,7 @@ function textInputType(scope, element, attr, ctrl, $sniffer, $browser) {
if (match) {
pattern = new RegExp(match[1], match[2]);
patternValidator = function(value) {
return validate(pattern, value)
return validate(pattern, value);
};
} else {
patternValidator = function(value) {
Expand All @@ -491,7 +491,7 @@ function textInputType(scope, element, attr, ctrl, $sniffer, $browser) {
if (attr.ngMinlength) {
var minlength = int(attr.ngMinlength);
var minLengthValidator = function(value) {
if (!isEmpty(value) && value.length < minlength) {
if (!ctrl.$isEmpty(value) && value.length < minlength) {
ctrl.$setValidity('minlength', false);
return undefined;
} else {
Expand All @@ -508,7 +508,7 @@ function textInputType(scope, element, attr, ctrl, $sniffer, $browser) {
if (attr.ngMaxlength) {
var maxlength = int(attr.ngMaxlength);
var maxLengthValidator = function(value) {
if (!isEmpty(value) && value.length > maxlength) {
if (!ctrl.$isEmpty(value) && value.length > maxlength) {
ctrl.$setValidity('maxlength', false);
return undefined;
} else {
Expand All @@ -526,7 +526,7 @@ function numberInputType(scope, element, attr, ctrl, $sniffer, $browser) {
textInputType(scope, element, attr, ctrl, $sniffer, $browser);

ctrl.$parsers.push(function(value) {
var empty = isEmpty(value);
var empty = ctrl.$isEmpty(value);
if (empty || NUMBER_REGEXP.test(value)) {
ctrl.$setValidity('number', true);
return value === '' ? null : (empty ? value : parseFloat(value));
Expand All @@ -537,13 +537,13 @@ function numberInputType(scope, element, attr, ctrl, $sniffer, $browser) {
});

ctrl.$formatters.push(function(value) {
return isEmpty(value) ? '' : '' + value;
return ctrl.$isEmpty(value) ? '' : '' + value;
});

if (attr.min) {
var min = parseFloat(attr.min);
var minValidator = function(value) {
if (!isEmpty(value) && value < min) {
if (!ctrl.$isEmpty(value) && value < min) {
ctrl.$setValidity('min', false);
return undefined;
} else {
Expand All @@ -559,7 +559,7 @@ function numberInputType(scope, element, attr, ctrl, $sniffer, $browser) {
if (attr.max) {
var max = parseFloat(attr.max);
var maxValidator = function(value) {
if (!isEmpty(value) && value > max) {
if (!ctrl.$isEmpty(value) && value > max) {
ctrl.$setValidity('max', false);
return undefined;
} else {
Expand All @@ -574,7 +574,7 @@ function numberInputType(scope, element, attr, ctrl, $sniffer, $browser) {

ctrl.$formatters.push(function(value) {

if (isEmpty(value) || isNumber(value)) {
if (ctrl.$isEmpty(value) || isNumber(value)) {
ctrl.$setValidity('number', true);
return value;
} else {
Expand All @@ -588,7 +588,7 @@ function urlInputType(scope, element, attr, ctrl, $sniffer, $browser) {
textInputType(scope, element, attr, ctrl, $sniffer, $browser);

var urlValidator = function(value) {
if (isEmpty(value) || URL_REGEXP.test(value)) {
if (ctrl.$isEmpty(value) || URL_REGEXP.test(value)) {
ctrl.$setValidity('url', true);
return value;
} else {
Expand All @@ -605,7 +605,7 @@ function emailInputType(scope, element, attr, ctrl, $sniffer, $browser) {
textInputType(scope, element, attr, ctrl, $sniffer, $browser);

var emailValidator = function(value) {
if (isEmpty(value) || EMAIL_REGEXP.test(value)) {
if (ctrl.$isEmpty(value) || EMAIL_REGEXP.test(value)) {
ctrl.$setValidity('email', true);
return value;
} else {
Expand Down Expand Up @@ -657,6 +657,12 @@ function checkboxInputType(scope, element, attr, ctrl) {
element[0].checked = ctrl.$viewValue;
};

// Override the standard `$isEmpty` because a value of `false` means empty in a checkbox.
var _$isEmpty = ctrl.$isEmpty;
ctrl.$isEmpty = function(value) {
return _$isEmpty(value) || value === false;
};

ctrl.$formatters.push(function(value) {
return value === trueValue;
});
Expand Down Expand Up @@ -992,6 +998,23 @@ var NgModelController = ['$scope', '$exceptionHandler', '$attrs', '$element', '$
*/
this.$render = noop;

/**
* @ngdoc function
* @name { ng.directive:ngModel.NgModelController#$isEmpty
* @methodOf ng.directive:ngModel.NgModelController
*
* @description
* This is called when we need to determine if the value of the input is empty.
*
* For instance, the required directive does this to work out if the input has data or not.
* The default `$isEmpty` function checks whether the value is `undefined`, `''`, `null` or `NaN`.
*
* You can override this for input directives whose concept of being empty is different to the
* default. The `checkboxInputType` directive does this because in its case a value of `false`
* implies empty.
*/
this.$isEmpty = isEmpty;

var parentForm = $element.inheritedData('$formController') || nullFormCtrl,
invalidCount = 0, // used to easily determine if we are valid
$error = this.$error = {}; // keep invalid keys here
Expand Down Expand Up @@ -1266,7 +1289,7 @@ var requiredDirective = function() {
attr.required = true; // force truthy in case we are on non input element

var validator = function(value) {
if (attr.required && (isEmpty(value) || value === false)) {
if (attr.required && ctrl.$isEmpty(value)) {
ctrl.$setValidity('required', false);
return;
} else {
Expand Down Expand Up @@ -1326,7 +1349,7 @@ var requiredDirective = function() {
it('should be invalid if empty', function() {
input('names').enter('');
expect(binding('names')).toEqual('[]');
expect(binding('names')).toEqual('');
expect(binding('myForm.namesInput.$valid')).toEqual('false');
expect(element('span.error').css('display')).not().toBe('none');
});
Expand All @@ -1341,6 +1364,9 @@ var ngListDirective = function() {
separator = match && new RegExp(match[1]) || attr.ngList || ',';

var parse = function(viewValue) {
// If the viewValue is invalid (say required but empty) it will be `undefined`
if ( isUndefined(viewValue) ) return;

var list = [];

if (viewValue) {
Expand All @@ -1360,6 +1386,12 @@ var ngListDirective = function() {

return undefined;
});

// Override the standard $isEmpty because an empty array means the input is empty
var _$isEmpty = ctrl.$isEmpty;
ctrl.$isEmpty = function(value) {
return _$isEmpty(value) || value.length === 0;
};
}
};
};
Expand Down
33 changes: 31 additions & 2 deletions test/ng/directive/inputSpec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -998,6 +998,16 @@ describe('input', function() {
expect(scope.list).toEqual([]);
});

it('should be invalid if required and empty', function() {
compileInput('<input type="text" ng-list ng-model="list" required>');
changeInputValueTo('');
expect(scope.list).toBeUndefined();
expect(inputElm).toBeInvalid();
changeInputValueTo('a,b');
expect(scope.list).toEqual(['a','b']);
expect(inputElm).toBeValid();
});


it('should allow custom separator', function() {
compileInput('<input type="text" ng-model="list" ng-list=":" />');
Expand Down Expand Up @@ -1090,10 +1100,29 @@ describe('input', function() {


it('should set $invalid when model undefined', function() {
compileInput('<input type="text" ng-model="notDefiend" required />');
compileInput('<input type="text" ng-model="notDefined" required />');
scope.$digest();
expect(inputElm).toBeInvalid();
})
});


it('should allow `false` as a valid value when the input type is not "checkbox"', function() {
compileInput('<input type="radio" ng-value="true" ng-model="answer" required />' +
'<input type="radio" ng-value="false" ng-model="answer" required />');

scope.$apply();
expect(inputElm).toBeInvalid();

scope.$apply(function() {
scope.answer = true;
});
expect(inputElm).toBeValid();

scope.$apply(function() {
scope.answer = false;
});
expect(inputElm).toBeValid();
});
});


Expand Down
25 changes: 25 additions & 0 deletions test/ng/directive/selectSpec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1213,6 +1213,31 @@ describe('select', function() {
});
expect(element).toBeValid();
});


it('should allow falsy values as values', function() {
createSelect({
'ng-model': 'value',
'ng-options': 'item.value as item.name for item in values',
'ng-required': 'required'
}, true);

scope.$apply(function() {
scope.values = [{name: 'True', value: true}, {name: 'False', value: false}];
scope.required = false;
});

element.val('1');
browserTrigger(element, 'change');
expect(element).toBeValid();
expect(scope.value).toBe(false);

scope.$apply(function() {
scope.required = true;
});
expect(element).toBeValid();
expect(scope.value).toBe(false);
});
});
});

Expand Down

0 comments on commit 5aa7600

Please sign in to comment.