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

fix(input): improve html5 validation support #7937

Closed
wants to merge 1 commit into from
+82 −22
Closed

fix(input): improve html5 validation support #7937

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 1 addition & 0 deletions src/.jshintrc
View file
Original file line number Diff line number Diff line change
@@ -101,6 +101,7 @@
"assertNotHasOwnProperty": false,
"getter": false,
"getBlockElements": false,
"VALIDITY_STATE_PROPERTY": false,

/* filters.js */
"getFirstThursdayOfYear": false,
5 changes: 5 additions & 0 deletions src/Angular.js
View file
Original file line number Diff line number Diff line change
@@ -14,6 +14,7 @@
-nodeName_,
-uid,
-REGEX_STRING_REGEXP,
-VALIDITY_STATE_PROPERTY,
-lowercase,
-uppercase,
@@ -105,6 +106,10 @@

var REGEX_STRING_REGEXP = /^\/(.+)\/([a-z]*)$/;

// The name of a form control's ValidityState property.
// This is used so that it's possible for internal tests to create mock ValidityStates.
var VALIDITY_STATE_PROPERTY = 'validity';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added this so that the input source doesn't have to say element.prop('ngMockValidity') || element.prop('validity')

It can probably be done different ways too


/**
* @ngdoc function
* @name angular.lowercase
57 changes: 37 additions & 20 deletions src/ng/directive/input.js
View file
Original file line number Diff line number Diff line change
@@ -862,15 +862,29 @@ function validate(ctrl, validatorName, validity, value){
return validity ? value : undefined;
}

function testFlags(validity, flags) {
var i, flag;
if (flags) {
for (i=0; i<flags.length; ++i) {
flag = flags[i];
if (validity[flag]) {
return true;
}
}
}
return false;
}

function addNativeHtml5Validators(ctrl, validatorName, element) {
var validity = element.prop('validity');
// 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] && (validity.badInput || validity.customError ||
validity.typeMismatch) && !validity.valueMissing) {
if (!ctrl.$error[validatorName] &&
!testFlags(validity, ignoreFlags) &&
testFlags(validity, badFlags)) {
ctrl.$setValidity(validatorName, false);
return;
}
@@ -881,8 +895,9 @@ function addNativeHtml5Validators(ctrl, validatorName, element) {
}

function textInputType(scope, element, attr, ctrl, $sniffer, $browser) {
var validity = element.prop('validity');
var validity = element.prop(VALIDITY_STATE_PROPERTY);
var placeholder = element[0].placeholder, noevent = {};
ctrl.$$validityState = validity;

// In composition mode, users are still inputing intermediate text buffer,
// hold the listener until composition is done.
@@ -921,16 +936,16 @@ function textInputType(scope, element, attr, ctrl, $sniffer, $browser) {
value = trim(value);
}

if (ctrl.$viewValue !== value ||
// If the value is still empty/falsy, and there is no `required` error, run validators
// again. This enables HTML5 constraint validation errors to affect Angular validation
// even when the first character entered causes an error.
(validity && value === '' && !validity.valueMissing)) {
// If a control is suffering from bad input, browsers discard its value, so it may be
// necessary to revalidate even if the control's value is the same empty value twice in
// a row.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added some comments

var revalidate = validity && ctrl.$$hasNativeValidators;
if (ctrl.$viewValue !== value || (value === '' && revalidate)) {
if (scope.$$phase) {
ctrl.$setViewValue(value, event);
ctrl.$setViewValue(value, event, revalidate);
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems a shame that we must touch so many function on NgModelController with this revalidate parameter. Could we not set it as a property on the controller, say needsValidation or something, which is then checked in $commitViewValue?

} else {
scope.$apply(function() {
ctrl.$setViewValue(value, event);
ctrl.$setViewValue(value, event, revalidate);
});
}
}
@@ -1079,6 +1094,8 @@ function createDateInputType(type, regexp, parseDate, format) {
};
}

var numberBadFlags = ['badInput'];

function numberInputType(scope, element, attr, ctrl, $sniffer, $browser) {
textInputType(scope, element, attr, ctrl, $sniffer, $browser);

@@ -1093,7 +1110,7 @@ function numberInputType(scope, element, attr, ctrl, $sniffer, $browser) {
}
});

addNativeHtml5Validators(ctrl, 'number', element);
addNativeHtml5Validators(ctrl, 'number', numberBadFlags, null, ctrl.$$validityState);
Copy link
Contributor

Choose a reason for hiding this comment

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

The ignoreFlags parameter doesn't seem to be used at all. If we set it to [] here rather than null then we could remove the if(flags) line in the testFlags() function above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's needed for other types, eventually we will want to do this for required validation too (at the bare minimum), and we don't want to invalidate required if we have badInput

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(This is because of the really, really stupid constraint validation API, it makes zero sense, but it's what we've got =()

Copy link
Contributor

Choose a reason for hiding this comment

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

But if we insist that these parameters are always arrays then we don't need the check right? Is there anywhere that a null is going to be passed where we can't insist that it is an empty array instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The question is whether you prefer testing if flags is null, or if you prefer creating an extra global object --- my preference is on the former here


ctrl.$formatters.push(function(value) {
return ctrl.$isEmpty(value) ? '' : '' + value;
@@ -1777,11 +1794,11 @@ var NgModelController = ['$scope', '$exceptionHandler', '$attrs', '$element', '$
* event defined in `ng-model-options`. this method is rarely needed as `NgModelController`
* usually handles calling this in response to input events.
*/
this.$commitViewValue = function() {
this.$commitViewValue = function(revalidate) {
var viewValue = ctrl.$viewValue;

$timeout.cancel(pendingDebounce);
if (ctrl.$$lastCommittedViewValue === viewValue) {
if (!revalidate && ctrl.$$lastCommittedViewValue === viewValue) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

CC @petebacondarwin / @shahata --- It's possible for the old value and new value to be the same here, because HTML5 will report controlls suffering from bad input's value as the empty string --- but we should revalidate anyways just so the native HTML validators do their business.

However, it may not be desirable to update the "last committed value" in that case, what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

What does it mean for the native validators to do their business?
Is the question, should we set the last committed value to the empty string if the HTML5 validation fails?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We occasionally need to revalidate, even if the actual value hasn't changed. But I guess by that point, the last committed value is probably the empty string anyways. So maybe it's not worth worrying about

Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably have some unit tests that demonstrate the various scenarios - at least for posterity.

Copy link
Contributor

Choose a reason for hiding this comment

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

  1. If I understand correctly, this is important only in case the html5 validators report a different validity error the second time this is invoked and we want to update ctrl.$error with the latest validation problem. If this is the case, maybe we should consider removing the revalidate parameter and compare the validity state here with the previous committed one. I don't like it very much, but I think it is a bit better then passing revalidate all this way. Another option is to run the validators no matter what in case the string is empty.
  2. Actually, passing the revalidate is a problem any way, since it will not work in case ng-model-options="{updateOn: 'blur'}". In this case, this function will be invoked by the blur trigger handler, which will not pass the revalidate argument...

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not every time, only when prevValue==newValue && prevValue=='' && (prevValidity.badInput || newValidity.badInput) or something like this. This made me realize that there is also the case where a value was invalid and now it is just empty - this case should also be revalidated and currently it is not covered imo. Anyway, even if you don't diff ValidityState, what do you think about the second point I mentioned? The revalidate thing will not work correctly in case of updateOn: 'blur'...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, the ngModelOptions stuff added a ton of complexity to this machinery that didn't exist previously, and there were no tests against it breaking the html5 validation stuff because it's not really possible to test without without the hacks here (those hacks probably should have existed initially, but hey).

So what we need to do is figure out a way for them to work well together in tandum.

Computing a diff between ValidityState objects is problematic, because A) ValidityState keys are not always enumerable (Firefox) which makes a diffing algorithm difficult and makes us have to keep track of possible ValidityState flags which are not necessarily never changing, maintenance hell, and B) it's just a stupid amount of work that isn't really needed anywhere.

You're right that this is broken in the case of invalid -> empty, so that's fixable, but the gunk with ngModelOptions is a lot harder because that piece of machinery is overly complex and not built with constraint validation in mind, which is why you guys are CC'd here

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree diffing the ValidityState is not pretty/cheap and I really don't like it too, I'm just trying to think of a way to do this that will work in all cases. What about the option to run all validators anyway in case the string is empty?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm fine with that, at least in the case where we're using html5 validators

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool!

return;
}
ctrl.$$lastCommittedViewValue = viewValue;
@@ -1846,14 +1863,14 @@ var NgModelController = ['$scope', '$exceptionHandler', '$attrs', '$element', '$
* @param {string} value Value from the view.
* @param {string} trigger Event that triggered the update.
*/
this.$setViewValue = function(value, trigger) {
this.$setViewValue = function(value, trigger, revalidate) {
ctrl.$viewValue = value;
if (!ctrl.$options || ctrl.$options.updateOnDefault) {
ctrl.$$debounceViewValueCommit(trigger);
ctrl.$$debounceViewValueCommit(trigger, revalidate);
}
};

this.$$debounceViewValueCommit = function(trigger) {
this.$$debounceViewValueCommit = function(trigger, revalidate) {
var debounceDelay = 0,
options = ctrl.$options,
debounce;
@@ -1872,10 +1889,10 @@ var NgModelController = ['$scope', '$exceptionHandler', '$attrs', '$element', '$
$timeout.cancel(pendingDebounce);
if (debounceDelay) {
pendingDebounce = $timeout(function() {
ctrl.$commitViewValue();
ctrl.$commitViewValue(revalidate);
}, debounceDelay);
} else {
ctrl.$commitViewValue();
ctrl.$commitViewValue(revalidate);
}
};

1 change: 1 addition & 0 deletions test/.jshintrc
View file
Original file line number Diff line number Diff line change
@@ -101,6 +101,7 @@
"assertNotHasOwnProperty": false,
"getter": false,
"getBlockElements": false,
"VALIDITY_STATE_PROPERTY": true,

/* AngularPublic.js */
"version": false,
40 changes: 38 additions & 2 deletions test/ng/directive/inputSpec.js
View file
Original file line number Diff line number Diff line change
@@ -607,17 +607,26 @@ describe('ngModel', function() {


describe('input', function() {
var formElm, inputElm, scope, $compile, $sniffer, $browser, changeInputValueTo;
var formElm, inputElm, scope, $compile, $sniffer, $browser, changeInputValueTo, currentSpec;

function compileInput(inputHtml) {
function compileInput(inputHtml, mockValidity) {
inputElm = jqLite(inputHtml);
if (isObject(mockValidity)) {
VALIDITY_STATE_PROPERTY = 'ngMockValidity';
inputElm.prop(VALIDITY_STATE_PROPERTY, mockValidity);
currentSpec.after(function() {
VALIDITY_STATE_PROPERTY = 'validity';
});
}
formElm = jqLite('<form name="form"></form>');
formElm.append(inputElm);
$compile(formElm)(scope);
scope.$digest();
}

var attrs;
beforeEach(function() { currentSpec = this; });
afterEach(function() { currentSpec = null; });
beforeEach(module(function($compileProvider) {
$compileProvider.directive('attrCapture', function() {
return function(scope, element, $attrs) {
@@ -2237,6 +2246,33 @@ describe('input', function() {
});


it('should invalidate number if suffering from bad input', function() {
compileInput('<input type="number" ng-model="age" />', {
valid: false,
badInput: true
});

changeInputValueTo('10a');
expect(scope.age).toBeUndefined();
expect(inputElm).toBeInvalid();
});


it('should validate number if transition from bad input to empty string', function() {
var validity = {
valid: false,
badInput: true
};
compileInput('<input type="number" ng-model="age" />', validity);
changeInputValueTo('10a');
validity.badInput = false;
validity.valid = true;
changeInputValueTo('');
expect(scope.age).toBeNull();
expect(inputElm).toBeValid();
});


describe('min', function() {

it('should validate', function() {