Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

$isEmpty fixes #10164

Merged
merged 4 commits into from
Nov 23, 2014
Merged

$isEmpty fixes #10164

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4644c58
revert: fix(input): always pass in the model value to `ctrl.$isEmpty`
petebacondarwin Nov 21, 2014
40406e2
fix(input[date]): do not use `$isEmpty` to check the model validity
petebacondarwin Nov 21, 2014
8692f87
fix(input): set ngTrueValue on required checkbox
Narretz Nov 21, 2014
42d09f1
docs(NgModelController): clarify the value parameter for $isEmpty
petebacondarwin Nov 23, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 27 additions & 22 deletions src/ng/directive/input.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1039,7 +1039,7 @@ function baseInputType(scope, element, attr, ctrl, $sniffer, $browser) {
element.on('change', listener);

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

Expand Down Expand Up @@ -1149,10 +1149,10 @@ function createDateInputType(type, regexp, parseDate, format) {
});

ctrl.$formatters.push(function(value) {
if (!ctrl.$isEmpty(value)) {
if (!isDate(value)) {
throw $ngModelMinErr('datefmt', 'Expected `{0}` to be a date', value);
}
if (value && !isDate(value)) {
throw $ngModelMinErr('datefmt', 'Expected `{0}` to be a date', value);
}
if (isValidDate(value)) {
previousDate = value;
if (previousDate && timezone === 'UTC') {
var timezoneOffset = 60000 * previousDate.getTimezoneOffset();
Expand All @@ -1161,14 +1161,14 @@ function createDateInputType(type, regexp, parseDate, format) {
return $filter('date')(value, format, timezone);
} else {
previousDate = null;
return '';
}
return '';
});

if (isDefined(attr.min) || attr.ngMin) {
var minVal;
ctrl.$validators.min = function(value) {
return ctrl.$isEmpty(value) || isUndefined(minVal) || parseDate(value) >= minVal;
return !isValidDate(value) || isUndefined(minVal) || parseDate(value) >= minVal;
};
attr.$observe('min', function(val) {
minVal = parseObservedDateValue(val);
Expand All @@ -1179,18 +1179,18 @@ function createDateInputType(type, regexp, parseDate, format) {
if (isDefined(attr.max) || attr.ngMax) {
var maxVal;
ctrl.$validators.max = function(value) {
return ctrl.$isEmpty(value) || isUndefined(maxVal) || parseDate(value) <= maxVal;
return !isValidDate(value) || isUndefined(maxVal) || parseDate(value) <= maxVal;
};
attr.$observe('max', function(val) {
maxVal = parseObservedDateValue(val);
ctrl.$validate();
});
}
// Override the standard $isEmpty to detect invalid dates as well
ctrl.$isEmpty = function(value) {

function isValidDate(value) {
// Invalid Date: getTime() returns NaN
return !value || (value.getTime && value.getTime() !== value.getTime());
};
return value && !(value.getTime && value.getTime() !== value.getTime());
}

function parseObservedDateValue(val) {
return isDefined(val) ? (isDate(val) ? val : parseDate(val)) : undefined;
Expand Down Expand Up @@ -1274,7 +1274,8 @@ function urlInputType(scope, element, attr, ctrl, $sniffer, $browser) {
stringBasedInputType(ctrl);

ctrl.$$parserName = 'url';
ctrl.$validators.url = function(value) {
ctrl.$validators.url = function(modelValue, viewValue) {
var value = modelValue || viewValue;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really understand why there is this modelValue || viewValue condition. If you are saying, hey take the modelValue and if that isn't there, take the viewValue why not always take the modelValue? Text-based input types by default do not change their type when parsed, so we should be good checking the modeValue. Unless someone messes with parsers / formatters, I don't think there's a case where we have a viewValue, but not a modelValue in the validator.
Conundrum is obviously that we say isEmpty should only be for the viewValue, so passing modelValue to it is fishy.

So what about always passing the viewValue to $isEmpty, regardless of modelValue?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again this is part of the revert of Tobias's commit.
It doesn't actually have anything to do with $isEmpty.

I am not sure of the reasoning behind checking both values. It would seem to me that we should be checking the $viewValue only as you say, since this is what the input[url] directive has control over. You can't assume anything about the model, since users can add in their own crazy parsers via their own directives if they want.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I didn't see your reverted the commit.

return ctrl.$isEmpty(value) || URL_REGEXP.test(value);
};
}
Expand All @@ -1286,7 +1287,8 @@ function emailInputType(scope, element, attr, ctrl, $sniffer, $browser) {
stringBasedInputType(ctrl);

ctrl.$$parserName = 'email';
ctrl.$validators.email = function(value) {
ctrl.$validators.email = function(modelValue, viewValue) {
var value = modelValue || viewValue;
return ctrl.$isEmpty(value) || EMAIL_REGEXP.test(value);
};
}
Expand Down Expand Up @@ -1340,9 +1342,11 @@ function checkboxInputType(scope, element, attr, ctrl, $sniffer, $browser, $filt
element[0].checked = ctrl.$viewValue;
};

// Override the standard `$isEmpty` because an empty checkbox is never equal to the trueValue
// Override the standard `$isEmpty` because the $viewValue of an empty checkbox is always set to `false`
// This is because of the parser below, which compares the `$modelValue` with `trueValue` to convert
// it to a boolean.
ctrl.$isEmpty = function(value) {
return value !== trueValue;
return value === false;
};

ctrl.$formatters.push(function(value) {
Expand Down Expand Up @@ -1810,17 +1814,18 @@ var NgModelController = ['$scope', '$exceptionHandler', '$attrs', '$element', '$
* @name ngModel.NgModelController#$isEmpty
*
* @description
* This is called when we need to determine if the value of the input is empty.
* This is called when we need to determine if the value of an 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.
*
* @param {*} value Model value to check.
* @returns {boolean} True if `value` is empty.
* @param {*} value The value of the input to check for emptiness.
* @returns {boolean} True if `value` is "empty".
*/
this.$isEmpty = function(value) {
return isUndefined(value) || value === '' || value === null || value !== value;
Expand Down Expand Up @@ -2647,8 +2652,8 @@ var requiredDirective = function() {
if (!ctrl) return;
attr.required = true; // force truthy in case we are on non input element

ctrl.$validators.required = function(value) {
return !attr.required || !ctrl.$isEmpty(value);
ctrl.$validators.required = function(modelValue, viewValue) {
return !attr.required || !ctrl.$isEmpty(viewValue);
};

attr.$observe('required', function() {
Expand Down Expand Up @@ -2723,7 +2728,7 @@ var minlengthDirective = function() {
ctrl.$validate();
});
ctrl.$validators.minlength = function(modelValue, viewValue) {
return ctrl.$isEmpty(modelValue) || viewValue.length >= minlength;
return ctrl.$isEmpty(viewValue) || viewValue.length >= minlength;
};
}
};
Expand Down
16 changes: 16 additions & 0 deletions test/ng/directive/inputSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2290,6 +2290,22 @@ describe('input', function() {
});


it('should render the $viewValue when $modelValue is empty', function() {
compileInput('<input type="text" ng-model="value" />');

var ctrl = inputElm.controller('ngModel');

ctrl.$modelValue = null;

expect(ctrl.$isEmpty(ctrl.$modelValue)).toBe(true);

ctrl.$viewValue = 'abc';
ctrl.$render();

expect(inputElm.val()).toBe('abc');
});


describe('pattern', function() {

it('should validate in-lined pattern', function() {
Expand Down