From 2182f8e4b2e29785670e469e6a32e0ff03caa4af Mon Sep 17 00:00:00 2001 From: Martin Staffa Date: Wed, 12 Nov 2014 21:08:39 +0100 Subject: [PATCH] fix(ngModel): don't run parsers when executing $validate Previously, $validate would execute the parsers to obtain a modelValue for validation. This was necessary, because a validator that is called outside of model / view update (e.g. from an observer) otherwise might only an undefined modelValue, because a previous view update has found a validation $error and set the model to undefined (as is tradition in angular) This is problematic as validators that are run immediately after the ngModelController initializes would parse the modelValue and replace the model, even though there had been no user input. The solution is to go back to an older design: the ngModelController will now internally record the $$rawModelValue. This means a model or view update will store the set / parsed modelValue regardless of validity, that is, it will never set it to undefined because of validation errors. When $validate is called, the $$rawModelValue will passed to the validators. If the validity has changed, the usual behavior is kept: if it became invalid, set the model to undefined, if valid, restore the last available modelValue - the $$rawModelValue. Additionally, $validate will only update the model when the validity changed. This is to prevent setting initially invalid models other than undefined to undefined (see #9063) Fixes: #9063 Fixes: #9959 Fixes: #9996 Fixes: #10025 Closes: #9890 Closes: #9913 Closes: #9997 --- src/ng/directive/input.js | 32 ++++- test/ng/directive/inputSpec.js | 229 +++++++++++++++++++++++++++++++++ 2 files changed, 259 insertions(+), 2 deletions(-) diff --git a/src/ng/directive/input.js b/src/ng/directive/input.js index f7e270b0f502..64d33a89ec96 100644 --- a/src/ng/directive/input.js +++ b/src/ng/directive/input.js @@ -1956,13 +1956,40 @@ var NgModelController = ['$scope', '$exceptionHandler', '$attrs', '$element', '$ * * @description * Runs each of the registered validators (first synchronous validators and then asynchronous validators). + * If the validity changes to invalid, the model will be set to undefined, unless ngModelOptions.allowInvalid + * is `true`. If the validity changes to valid, it will set the model to the last available valid + * modelValue. */ this.$validate = function() { // ignore $validate before model is initialized if (isNumber(ctrl.$modelValue) && isNaN(ctrl.$modelValue)) { return; } - this.$$parseAndValidate(); + + var viewValue = ctrl.$$lastCommittedViewValue; + var modelValue = ctrl.$$rawModelValue; + + var prevValid = ctrl.$valid; + var prevModelValue = ctrl.$modelValue; + + var allowInvalid = ctrl.$options && ctrl.$options.allowInvalid; + + ctrl.$$runValidators(undefined, modelValue, viewValue, function(allValid) { + // If there was no change in validity, don't update the model + // This prevents changing an invalid modelValue to undefined + if (!allowInvalid && prevValid !== allValid) { + // Note: Don't check ctrl.$valid here, as we could have + // external validators (e.g. calculated on the server), + // that just call $setValidity and need the model value + // to calculate their validity. + ctrl.$modelValue = allValid ? modelValue : undefined; + + if (ctrl.$modelValue !== prevModelValue) { + ctrl.$$writeModelToScope(); + } + } + }); + }; this.$$runValidators = function(parseValid, modelValue, viewValue, doneCallback) { @@ -2110,6 +2137,7 @@ var NgModelController = ['$scope', '$exceptionHandler', '$attrs', '$element', '$ } var prevModelValue = ctrl.$modelValue; var allowInvalid = ctrl.$options && ctrl.$options.allowInvalid; + ctrl.$$rawModelValue = modelValue; if (allowInvalid) { ctrl.$modelValue = modelValue; writeToModelIfNeeded(); @@ -2234,7 +2262,7 @@ var NgModelController = ['$scope', '$exceptionHandler', '$attrs', '$element', '$ // if scope model value and ngModel value are out of sync // TODO(perf): why not move this to the action fn? if (modelValue !== ctrl.$modelValue) { - ctrl.$modelValue = modelValue; + ctrl.$modelValue = ctrl.$$rawModelValue = modelValue; var formatters = ctrl.$formatters, idx = formatters.length; diff --git a/test/ng/directive/inputSpec.js b/test/ng/directive/inputSpec.js index e7064a0259f2..e73a8a1e7207 100644 --- a/test/ng/directive/inputSpec.js +++ b/test/ng/directive/inputSpec.js @@ -934,6 +934,209 @@ describe('NgModelController', function() { }); }); +describe('initialization', function() { + var formElm, inputElm, ctrl, scope, $compile, $sniffer, $compileProvider, changeInputValueTo; + + function compileInput(inputHtml) { + inputElm = jqLite(inputHtml); + formElm = jqLite('
'); + formElm.append(inputElm); + $compile(formElm)(scope); + ctrl = inputElm.controller('ngModel'); + scope.$digest(); + } + + function addValidator(validity, shouldObserve) { + if (!isDefined(shouldObserve)) { + shouldObserve = true; + } + + $compileProvider.directive('obs', function() { + return { + require: 'ngModel', + link: function(scope, element, attrs, ngModelCtrl) { + + ngModelCtrl.$validators.obs = isFunction(validity) ? validity : function(value) { + return validity; + }; + + if (shouldObserve) { + attrs.$observe('obs', function() { + ngModelCtrl.$validate(); + }); + } + } + }; + + }); + } + + function addFormatter(formatFunction) { + $compileProvider.directive('format', function() { + return { + require: 'ngModel', + link: function(scope, element, attrs, ctrl) { + + ctrl.$formatters.push(formatFunction); + } + }; + + }); + } + + function addParser(parseFunction) { + $compileProvider.directive('parse', function() { + return { + require: 'ngModel', + link: function(scope, element, attrs, ctrl) { + + ctrl.$parsers.push(parseFunction); + } + }; + + }); + } + + beforeEach(module(function(_$compileProvider_) { + $compileProvider = _$compileProvider_; + })); + + beforeEach(inject(function(_$compile_, _$rootScope_, _$sniffer_) { + $compile = _$compile_; + $sniffer = _$sniffer_; + scope = _$rootScope_; + + changeInputValueTo = function(value) { + inputElm.val(value); + browserTrigger(inputElm, $sniffer.hasEvent('input') ? 'input' : 'change'); + }; + })); + + afterEach(function() { + dealoc(formElm); + }); + + // https://github.com/angular/angular.js/issues/9959 + it('should not change model of type number to string with validator using observer', function() { + addValidator(true); + scope.value = 12345; + scope.attr = 'mock'; + scope.ngChangeSpy = jasmine.createSpy(); + + compileInput(''); + + expect(scope.value).toBe(12345); + expect(scope.ngChangeSpy).not.toHaveBeenCalled(); + }); + + //https://github.com/angular/angular.js/issues/9063 + it('should not set a null model that is invalid to undefined', function() { + addValidator(false); + scope.value = null; + scope.required = true; + compileInput(''); + + expect(inputElm).toBeInvalid(); + expect(scope.value).toBe(null); + expect(scope.form.textInput.$error.obs).toBeTruthy(); + }); + + //https://github.com/angular/angular.js/issues/9996 + it('should not change an undefined model that uses ng-required and formatters and parsers', function() { + addParser(function(viewValue) { + return null; + }); + addFormatter(function(modelValue) { + return ''; + }); + + scope.ngChangeSpy = jasmine.createSpy(); + compileInput(''); + + expect(inputElm).toBeValid(); + expect(scope.value).toBeUndefined(); + expect(scope.ngChangeSpy).not.toHaveBeenCalled(); + }); + + // https://github.com/angular/angular.js/issues/10025 + it('should not change a model that uses custom $formatters and $parsers', function() { + addValidator(true); + addFormatter(function(modelValue) { + return 'abc'; + }); + addParser(function(viewValue) { + return 'xyz'; + }); + scope.value = 'abc'; + scope.attr = 'mock'; + compileInput(''); + + expect(inputElm).toBeValid(); + expect(scope.value).toBe('abc'); + }); + + describe('$validate', function() { + + // Sanity test: since a parse error sets the modelValue to undefined, the + // $$rawModelValue will always be undefined, hence $validate does not have + // a 'good' value to update + it('should not update a model that has a parse error', function() { + scope.value = 'abc'; + addParser(function() { + return undefined; + }); + + addValidator(true, false); + + compileInput(''); + expect(inputElm).toBeValid(); + expect(scope.value).toBe('abc'); + + changeInputValueTo('xyz'); + expect(inputElm).toBeInvalid(); + expect(scope.value).toBeUndefined(); + expect(ctrl.$error.parse).toBe(true); + + ctrl.$validate(); + expect(inputElm).toBeInvalid(); + expect(scope.value).toBeUndefined(); + }); + + it('should restore the last valid modelValue when a validator becomes valid', function() { + scope.value = 'abc'; + scope.count = 0; + + addValidator(function() { + scope.count++; + dump('count', scope.count); + return scope.count === 1 ? true : scope.count === 2 ? false : true; + }); + + compileInput(''); + expect(inputElm).toBeValid(); + expect(scope.value).toBe('abc'); + expect(ctrl.$viewValue).toBe('abc'); + + ctrl.$validate(); + scope.$digest(); + expect(inputElm).toBeInvalid(); + expect(scope.value).toBeUndefined(); + expect(ctrl.$viewValue).toBe('abc'); + + ctrl.$validate(); + scope.$digest(); + expect(inputElm).toBeValid(); + expect(scope.value).toBe('abc'); + }); + + + }); +}); + describe('ngModel', function() { var EMAIL_REGEXP = /^[a-z0-9!#$%&'*+\/=?^_`{|}~.-]+@[a-z0-9]([a-z0-9-]*[a-z0-9])?(\.[a-z0-9]([a-z0-9-]*[a-z0-9])?)*$/i; @@ -1348,6 +1551,16 @@ describe('input', function() { expect(scope.form.$$renameControl).not.toHaveBeenCalled(); }); + it('should not invoke viewChangeListeners before input is touched', function() { + scope.value = 1; + var change = scope.change = jasmine.createSpy('change'); + var element = $compile('
' + + '' + + '
')(scope); + scope.$digest(); + expect(change).not.toHaveBeenCalled(); + dealoc(element); + }); describe('compositionevents', function() { it('should not update the model between "compositionstart" and "compositionend" on non android', inject(function($sniffer) { @@ -2264,6 +2477,14 @@ describe('input', function() { expect(inputElm).toBeValid(); expect(scope.form.input.$error.minlength).not.toBe(true); }); + + it('should validate when the model is initalized as a number', function() { + scope.value = 12345; + compileInput(''); + expect(scope.value).toBe(12345); + expect(scope.form.input.$error.minlength).toBeUndefined(); + }); + }); @@ -2362,6 +2583,14 @@ describe('input', function() { expect(scope.value).toBe('12345'); }); + // This works both for string formatter and toString() in validator + it('should validate when the model is initalized as a number', function() { + scope.value = 12345; + compileInput(''); + expect(scope.value).toBe(12345); + expect(scope.form.input.$error.maxlength).toBeUndefined(); + }); + });