From db044c408a7f8082758b96ab739348810c36e15a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matias=20Niemel=C3=A4?= Date: Tue, 19 Aug 2014 00:04:59 -0400 Subject: [PATCH] fix(ngModel): treat undefined parse responses as parse errors With this commit, ngModel will now handle parsing first and then validation afterwards once the parsing is successful. If any parser along the way returns `undefined` then ngModel will break the chain of parsing and register a a parser error represented by the type of input that is being collected (e.g. number, date, datetime, url, etc...). If a parser fails for a standard text input field then an error of `parse` will be placed on `model.$error`. BREAKING CHANGE Any parser code from before that returned an `undefined` value (or nothing at all) will now cause a parser failure. When this occurs none of the validators present in `$validators` will run until the parser error is gone. --- src/ng/directive/form.js | 11 +- src/ng/directive/input.js | 166 +++++++++++++--------------- test/ng/directive/inputSpec.js | 191 +++++++++++++++++++++++++++++++-- 3 files changed, 264 insertions(+), 104 deletions(-) diff --git a/src/ng/directive/form.js b/src/ng/directive/form.js index 43041c47e708..a499e1fc8119 100644 --- a/src/ng/directive/form.js +++ b/src/ng/directive/form.js @@ -7,7 +7,8 @@ var nullFormCtrl = { $setValidity: noop, $setDirty: noop, $setPristine: noop, - $setSubmitted: noop + $setSubmitted: noop, + $$clearControlValidity: noop }, SUBMITTED_CLASS = 'ng-submitted'; @@ -144,11 +145,15 @@ function FormController(element, attrs, $scope, $animate) { if (control.$name && form[control.$name] === control) { delete form[control.$name]; } + + form.$$clearControlValidity(control); + arrayRemove(controls, control); + }; + + form.$$clearControlValidity = function(control) { forEach(errors, function(queue, validationToken) { form.$setValidity(validationToken, true, control); }); - - arrayRemove(controls, control); }; /** diff --git a/src/ng/directive/input.js b/src/ng/directive/input.js index 630f4c15fc7c..8a8d4b870ceb 100644 --- a/src/ng/directive/input.js +++ b/src/ng/directive/input.js @@ -18,6 +18,7 @@ var MONTH_REGEXP = /^(\d{4})-(\d\d)$/; var TIME_REGEXP = /^(\d\d):(\d\d)(?::(\d\d))?$/; var DEFAULT_REGEXP = /(\s+|^)default(\s+|$)/; +var $ngModelMinErr = new minErr('ngModel'); var inputType = { /** @@ -885,13 +886,6 @@ var inputType = { 'file': noop }; -// A helper function to call $setValidity and return the value / undefined, -// a pattern that is repeated a lot in the input validation logic. -function validate(ctrl, validatorName, validity, value){ - ctrl.$setValidity(validatorName, validity); - return validity ? value : undefined; -} - function testFlags(validity, flags) { var i, flag; if (flags) { @@ -905,25 +899,6 @@ function testFlags(validity, flags) { return false; } -// Pass validity so that behaviour can be mocked easier. -function addNativeHtml5Validators(ctrl, validatorName, badFlags, ignoreFlags, validity) { - if (isObject(validity)) { - ctrl.$$hasNativeValidators = true; - var validator = function(value) { - // Don't overwrite previous validation, don't consider valueMissing to apply (ng-required can - // perform the required validation) - if (!ctrl.$error[validatorName] && - !testFlags(validity, ignoreFlags) && - testFlags(validity, badFlags)) { - ctrl.$setValidity(validatorName, false); - return; - } - return value; - }; - ctrl.$parsers.push(validator); - } -} - function textInputType(scope, element, attr, ctrl, $sniffer, $browser) { var validity = element.prop(VALIDITY_STATE_PROPERTY); var placeholder = element[0].placeholder, noevent = {}; @@ -1074,25 +1049,20 @@ function createDateParser(regexp, mapping) { function createDateInputType(type, regexp, parseDate, format) { return function dynamicDateInputType(scope, element, attr, ctrl, $sniffer, $browser, $filter) { + badInputChecker(scope, element, attr, ctrl); textInputType(scope, element, attr, ctrl, $sniffer, $browser); var timezone = ctrl && ctrl.$options && ctrl.$options.timezone; + ctrl.$$parserName = type; ctrl.$parsers.push(function(value) { - if(ctrl.$isEmpty(value)) { - ctrl.$setValidity(type, true); - return null; - } - - if(regexp.test(value)) { - ctrl.$setValidity(type, true); + if (ctrl.$isEmpty(value)) return null; + if (regexp.test(value)) { var parsedDate = parseDate(value); if (timezone === 'UTC') { parsedDate.setMinutes(parsedDate.getMinutes() - parsedDate.getTimezoneOffset()); } return parsedDate; } - - ctrl.$setValidity(type, false); return undefined; }); @@ -1104,81 +1074,69 @@ function createDateInputType(type, regexp, parseDate, format) { }); if(attr.min) { - var minValidator = function(value) { - var valid = ctrl.$isEmpty(value) || - (parseDate(value) >= parseDate(attr.min)); - ctrl.$setValidity('min', valid); - return valid ? value : undefined; - }; - - ctrl.$parsers.push(minValidator); - ctrl.$formatters.push(minValidator); + ctrl.$validators.min = function(value) { + return ctrl.$isEmpty(value) || isUndefined(attr.min) || parseDate(value) >= parseDate(attr.min); + }; } if(attr.max) { - var maxValidator = function(value) { - var valid = ctrl.$isEmpty(value) || - (parseDate(value) <= parseDate(attr.max)); - ctrl.$setValidity('max', valid); - return valid ? value : undefined; - }; - - ctrl.$parsers.push(maxValidator); - ctrl.$formatters.push(maxValidator); + ctrl.$validators.max = function(value) { + return ctrl.$isEmpty(value) || isUndefined(attr.max) || parseDate(value) <= parseDate(attr.max); + }; } }; } -var numberBadFlags = ['badInput']; +function badInputChecker(scope, element, attr, ctrl) { + var node = element[0]; + var nativeValidation = ctrl.$$hasNativeValidators = isObject(node.validity); + if (nativeValidation) { + ctrl.$parsers.push(function(value) { + var validity = element.prop(VALIDITY_STATE_PROPERTY) || {}; + return validity.badInput || validity.typeMismatch ? undefined : value; + }); + } +} function numberInputType(scope, element, attr, ctrl, $sniffer, $browser) { + badInputChecker(scope, element, attr, ctrl); textInputType(scope, element, attr, ctrl, $sniffer, $browser); + ctrl.$$parserName = 'number'; ctrl.$parsers.push(function(value) { - var empty = ctrl.$isEmpty(value); - if (empty || NUMBER_REGEXP.test(value)) { - ctrl.$setValidity('number', true); - return value === '' ? null : (empty ? value : parseFloat(value)); - } else { - ctrl.$setValidity('number', false); - return undefined; - } + if(ctrl.$isEmpty(value)) return null; + if(NUMBER_REGEXP.test(value)) return parseFloat(value); + return undefined; }); - addNativeHtml5Validators(ctrl, 'number', numberBadFlags, null, ctrl.$$validityState); - ctrl.$formatters.push(function(value) { - return ctrl.$isEmpty(value) ? '' : '' + value; + if (!ctrl.$isEmpty(value)) { + if (!isNumber(value)) { + throw $ngModelMinErr('numfmt', 'Expected `{0}` to be a number', value); + } + value = value.toString(); + } + return value; }); if (attr.min) { - var minValidator = function(value) { - var min = parseFloat(attr.min); - return validate(ctrl, 'min', ctrl.$isEmpty(value) || value >= min, value); + ctrl.$validators.min = function(value) { + return ctrl.$isEmpty(value) || isUndefined(attr.min) || value >= parseFloat(attr.min); }; - - ctrl.$parsers.push(minValidator); - ctrl.$formatters.push(minValidator); } if (attr.max) { - var maxValidator = function(value) { - var max = parseFloat(attr.max); - return validate(ctrl, 'max', ctrl.$isEmpty(value) || value <= max, value); + ctrl.$validators.max = function(value) { + return ctrl.$isEmpty(value) || isUndefined(attr.max) || value <= parseFloat(attr.max); }; - - ctrl.$parsers.push(maxValidator); - ctrl.$formatters.push(maxValidator); } - - ctrl.$formatters.push(function(value) { - return validate(ctrl, 'number', ctrl.$isEmpty(value) || isNumber(value), value); - }); } function urlInputType(scope, element, attr, ctrl, $sniffer, $browser) { + badInputChecker(scope, element, attr, ctrl); textInputType(scope, element, attr, ctrl, $sniffer, $browser); + ctrl.$$parserName = 'url'; ctrl.$validators.url = function(modelValue, viewValue) { var value = modelValue || viewValue; return ctrl.$isEmpty(value) || URL_REGEXP.test(value); @@ -1186,8 +1144,10 @@ function urlInputType(scope, element, attr, ctrl, $sniffer, $browser) { } function emailInputType(scope, element, attr, ctrl, $sniffer, $browser) { + badInputChecker(scope, element, attr, ctrl); textInputType(scope, element, attr, ctrl, $sniffer, $browser); + ctrl.$$parserName = 'email'; ctrl.$validators.email = function(modelValue, viewValue) { var value = modelValue || viewValue; return ctrl.$isEmpty(value) || EMAIL_REGEXP.test(value); @@ -1223,7 +1183,7 @@ function parseConstantExpr($parse, context, name, expression, fallback) { if (isDefined(expression)) { parseFn = $parse(expression); if (!parseFn.constant) { - throw new minErr('ngModel')('constexpr', 'Expected constant expression for `{0}`, but saw ' + + throw minErr('ngModel')('constexpr', 'Expected constant expression for `{0}`, but saw ' + '`{1}`.', name, expression); } return parseFn(context); @@ -1598,7 +1558,7 @@ var NgModelController = ['$scope', '$exceptionHandler', '$attrs', '$element', '$ ctrl = this; if (!ngModelSet) { - throw minErr('ngModel')('nonassign', "Expression '{0}' is non-assignable. Element: {1}", + throw $ngModelMinErr('nonassign', "Expression '{0}' is non-assignable. Element: {1}", $attr.ngModel, startingTag($element)); } @@ -1663,6 +1623,19 @@ var NgModelController = ['$scope', '$exceptionHandler', '$attrs', '$element', '$ $animate.addClass($element, (isValid ? VALID_CLASS : INVALID_CLASS) + validationErrorKey); } + this.$$clearValidity = function() { + forEach(ctrl.$error, function(val, key) { + var validationKey = snake_case(key, '-'); + $animate.removeClass($element, VALID_CLASS + validationKey); + $animate.removeClass($element, INVALID_CLASS + validationKey); + }); + + invalidCount = 0; + $error = ctrl.$error = {}; + + parentForm.$$clearControlValidity(ctrl); + }; + /** * @ngdoc method * @name ngModel.NgModelController#$setValidity @@ -1694,7 +1667,7 @@ var NgModelController = ['$scope', '$exceptionHandler', '$attrs', '$element', '$ ctrl.$valid = true; ctrl.$invalid = false; } - } else { + } else if(!$error[validationErrorKey]) { toggleValidCss(false); ctrl.$invalid = true; ctrl.$valid = false; @@ -1883,16 +1856,27 @@ var NgModelController = ['$scope', '$exceptionHandler', '$attrs', '$element', '$ parentForm.$setDirty(); } - var modelValue = viewValue; - forEach(ctrl.$parsers, function(fn) { - modelValue = fn(modelValue); - }); + var hasBadInput, modelValue = viewValue; + for(var i = 0; i < ctrl.$parsers.length; i++) { + modelValue = ctrl.$parsers[i](modelValue); + if(isUndefined(modelValue)) { + hasBadInput = true; + break; + } + } - if (ctrl.$modelValue !== modelValue && - (isUndefined(ctrl.$$invalidModelValue) || ctrl.$$invalidModelValue != modelValue)) { + var parserName = ctrl.$$parserName || 'parse'; + if (hasBadInput) { + ctrl.$$invalidModelValue = ctrl.$modelValue = undefined; + ctrl.$$clearValidity(); + ctrl.$setValidity(parserName, false); + } else if (ctrl.$modelValue !== modelValue && + (isUndefined(ctrl.$$invalidModelValue) || ctrl.$$invalidModelValue != modelValue)) { + ctrl.$setValidity(parserName, true); ctrl.$$runValidators(modelValue, viewValue); - ctrl.$$writeModelToScope(); } + + ctrl.$$writeModelToScope(); }; this.$$writeModelToScope = function() { diff --git a/test/ng/directive/inputSpec.js b/test/ng/directive/inputSpec.js index abd8dc11e4c2..b574bd0e1871 100644 --- a/test/ng/directive/inputSpec.js +++ b/test/ng/directive/inputSpec.js @@ -9,7 +9,8 @@ describe('NgModelController', function() { parentFormCtrl = { $setValidity: jasmine.createSpy('$setValidity'), - $setDirty: jasmine.createSpy('$setDirty') + $setDirty: jasmine.createSpy('$setDirty'), + $$clearControlValidity: noop }; element = jqLite('
'); @@ -223,6 +224,106 @@ describe('NgModelController', function() { expect(ctrl.$dirty).toBe(true); expect(parentFormCtrl.$setDirty).not.toHaveBeenCalled(); }); + + it('should remove all other errors when any parser returns undefined', function() { + var a, b, val = function(val, x) { + return x ? val : x; + }; + + ctrl.$parsers.push(function(v) { return val(v, a); }); + ctrl.$parsers.push(function(v) { return val(v, b); }); + + ctrl.$validators.high = function(value) { + return !isDefined(value) || value > 5; + }; + + ctrl.$validators.even = function(value) { + return !isDefined(value) || value % 2 === 0; + }; + + a = b = true; + + ctrl.$setViewValue('3'); + expect(ctrl.$error).toEqual({ parse: false, high : true, even : true }); + + ctrl.$setViewValue('10'); + expect(ctrl.$error).toEqual({ parse: false, high : false, even : false }); + + a = undefined; + + ctrl.$setViewValue('12'); + expect(ctrl.$error).toEqual({ parse: true }); + + a = true; + b = undefined; + + ctrl.$setViewValue('14'); + expect(ctrl.$error).toEqual({ parse: true }); + + a = undefined; + b = undefined; + + ctrl.$setViewValue('16'); + expect(ctrl.$error).toEqual({ parse: true }); + + a = b = false; //not undefined + + ctrl.$setViewValue('2'); + expect(ctrl.$error).toEqual({ parse: false, high : true, even : false }); + }); + + it('should remove all non-parse-related CSS classes from the form when a parser fails', + inject(function($compile, $rootScope) { + + var element = $compile('
' + + '' + + '
')($rootScope); + var inputElm = element.find('input'); + var ctrl = $rootScope.myForm.myControl; + + var parserIsFailing = false; + ctrl.$parsers.push(function(value) { + return parserIsFailing ? undefined : value; + }); + + ctrl.$validators.alwaysFail = function() { + return false; + }; + + ctrl.$setViewValue('123'); + scope.$digest(); + + expect(element).not.toHaveClass('ng-valid-parse'); + expect(element).toHaveClass('ng-invalid-always-fail'); + + parserIsFailing = true; + ctrl.$setViewValue('12345'); + scope.$digest(); + + expect(element).toHaveClass('ng-invalid-parse'); + expect(element).not.toHaveClass('ng-invalid-always-fail'); + + dealoc(element); + })); + + it('should set the ng-invalid-parse and ng-valid-parse CSS class when parsers fail and pass', function() { + var pass = true; + ctrl.$parsers.push(function(v) { + return pass ? v : undefined; + }); + + var input = element.find('input'); + + ctrl.$setViewValue('1'); + expect(input).toHaveClass('ng-valid-parse'); + expect(input).not.toHaveClass('ng-invalid-parse'); + + pass = undefined; + + ctrl.$setViewValue('2'); + expect(input).not.toHaveClass('ng-valid-parse'); + expect(input).toHaveClass('ng-invalid-parse'); + }); }); @@ -1639,6 +1740,16 @@ describe('input', function() { expect(inputElm.val()).toBe('2014-07'); }); + it('should label parse errors as `month`', function() { + compileInput('', { + valid: false, + badInput: true + }); + + changeInputValueTo('xxx'); + expect(inputElm).toBeInvalid(); + expect(scope.form.alias.$error.month).toBeTruthy(); + }); describe('min', function (){ beforeEach(function (){ @@ -1770,6 +1881,17 @@ describe('input', function() { expect(inputElm.val()).toBe('2014-W03'); }); + it('should label parse errors as `week`', function() { + compileInput('', { + valid: false, + badInput: true + }); + + changeInputValueTo('yyy'); + expect(inputElm).toBeInvalid(); + expect(scope.form.alias.$error.week).toBeTruthy(); + }); + describe('min', function (){ beforeEach(function (){ compileInput(''); @@ -1918,6 +2040,17 @@ describe('input', function() { expect(+scope.value).toBe(+new Date(2000, 0, 1, 1, 2, 0)); }); + it('should label parse errors as `datetimelocal`', function() { + compileInput('', { + valid: false, + badInput: true + }); + + changeInputValueTo('zzz'); + expect(inputElm).toBeInvalid(); + expect(scope.form.alias.$error.datetimelocal).toBeTruthy(); + }); + describe('min', function (){ beforeEach(function (){ compileInput(''); @@ -2094,6 +2227,17 @@ describe('input', function() { expect(+scope.value).toBe(+new Date(1970, 0, 1, 1, 2, 0)); }); + it('should label parse errors as `time`', function() { + compileInput('', { + valid: false, + badInput: true + }); + + changeInputValueTo('mmm'); + expect(inputElm).toBeInvalid(); + expect(scope.form.alias.$error.time).toBeTruthy(); + }); + describe('min', function (){ beforeEach(function (){ compileInput(''); @@ -2251,6 +2395,17 @@ describe('input', function() { expect(inputElm.val()).toBe('2001-01-01'); }); + it('should label parse errors as `date`', function() { + compileInput('', { + valid: false, + badInput: true + }); + + changeInputValueTo('nnn'); + expect(inputElm).toBeInvalid(); + expect(scope.form.alias.$error.date).toBeTruthy(); + }); + describe('min', function (){ beforeEach(function (){ compileInput(''); @@ -2374,13 +2529,17 @@ describe('input', function() { }); - it('should invalidate number if suffering from bad input', function() { + it('should only invalidate the model if suffering from bad input when the data is parsed', function() { compileInput('', { valid: false, badInput: true }); - changeInputValueTo('10a'); + expect(scope.age).toBeUndefined(); + expect(inputElm).toBeValid(); + + changeInputValueTo('this-will-fail-because-of-the-badInput-flag'); + expect(scope.age).toBeUndefined(); expect(inputElm).toBeInvalid(); }); @@ -2400,6 +2559,13 @@ describe('input', function() { expect(inputElm).toBeValid(); }); + it('should throw if the model value is not a number', function() { + expect(function() { + scope.value = 'one'; + compileInput(''); + }).toThrowMinErr('ngModel', 'numfmt', "Expected `one` to be a number"); + }); + describe('min', function() { @@ -2440,7 +2606,7 @@ describe('input', function() { changeInputValueTo('20'); expect(inputElm).toBeInvalid(); - expect(scope.value).toBeFalsy(); + expect(scope.value).toBeUndefined(); expect(scope.form.alias.$error.max).toBeTruthy(); changeInputValueTo('0'); @@ -2914,13 +3080,18 @@ describe('input', function() { }); - it('should set $valid even if model fails other validators', function() { - compileInput(''); - changeInputValueTo('bademail'); + it('should consider bad input as an error before any other errors are considered', function() { + compileInput('', { badInput : true }); + var ctrl = inputElm.controller('ngModel'); + ctrl.$parsers.push(function() { + return undefined; + }); + + changeInputValueTo('abc123'); - expect(inputElm).toHaveClass('ng-valid-required'); - expect(inputElm.controller('ngModel').$error.required).toBe(false); - expect(inputElm).toBeInvalid(); // invalid because of the email validator + expect(ctrl.$error.parse).toBe(true); + expect(inputElm).toHaveClass('ng-invalid-parse'); + expect(inputElm).toBeInvalid(); // invalid because of the number validator });