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

fix(input): improve html5 validation for ngModelOptions #7976

Closed
wants to merge 1 commit into from
Closed

fix(input): improve html5 validation for ngModelOptions #7976

Show file tree
Hide file tree
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
38 changes: 19 additions & 19 deletions src/ng/directive/input.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -939,13 +939,12 @@ function textInputType(scope, element, attr, ctrl, $sniffer, $browser) {
// 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.
var revalidate = validity && ctrl.$$hasNativeValidators;
if (ctrl.$viewValue !== value || (value === '' && revalidate)) {
if (ctrl.$viewValue !== value || (value === '' && ctrl.$$hasNativeValidators)) {
if (scope.$$phase) {
ctrl.$setViewValue(value, event, revalidate);
ctrl.$setViewValue(value, event);
} else {
scope.$apply(function() {
ctrl.$setViewValue(value, event, revalidate);
ctrl.$setViewValue(value, event);
});
}
}
Expand Down Expand Up @@ -1812,23 +1811,24 @@ 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(revalidate) {
this.$commitViewValue = function() {
var viewValue = ctrl.$viewValue;

$timeout.cancel(pendingDebounce);
if (!revalidate && ctrl.$$lastCommittedViewValue === viewValue) {
if (ctrl.$$lastCommittedViewValue !== viewValue) {
// change to dirty
if (ctrl.$pristine) {
ctrl.$dirty = true;
ctrl.$pristine = false;
$animate.removeClass($element, PRISTINE_CLASS);
$animate.addClass($element, DIRTY_CLASS);
parentForm.$setDirty();
}
} else if (viewValue !== '' || !ctrl.$$hasNativeValidators) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@shahata thinking about this more, I don't think this is right.

We have a number of different scenarios where we need to revalidate: when an interpolated attribute that we care about changes, when native validation changes, and when the view value changes (OR, whenever ngModelOptions decides to commit a value).

Currently, we really suck at re-validating stuff in the parsers/formatters pipelines, but not all of that stuff can be necessarily ported into the $validators pipeline due to needing to be run in a specific order.

I think we need a better refactoring, because this isn't quite enough. It SHOULD be a very simple thing to do, but we've overengineered the form controls so much that it's become quite a pain to manage.

Copy link
Contributor

Choose a reason for hiding this comment

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

On blink-dev there's been a thread lately about implementing reportValidity() --- distinct from checkValidity().

It's not clear whether there's enough vendor interest to implement that, but from my perspective, it's a model which makes good sense, and drastically simplifies things (if applied to Angular).

If we separate reporting validity (adding classes, rendering widgets, and so forth) and testing for validity, we have fewer knots and things to worry about breaking in the forms component.

Obviously, these aren't the only two things that need to happen, we also need to decide on a view value and model value, so I guess that would be a reportValue stage.

I think it might be worth thinking about how we can organize this better so that there is less difficulty maintaining and improving on this in its current state.

@IgorMinar (don't read this now, but maybe on monday), what would you think about streamlining this a bit? Since there are three operations happening here, I think they should definitely not be mixed up together, they should be distinct and simpler. The way things are right now, we are crossing the streams and it becomes difficult to fix these issues without hacks.

So, my proposal is, separating this all into test validity, report validity, and report value --- and run each operation as necessary (for instance, testing for validity on input and digest, reporting validity and reporting values at the whim of some simpler version of ngModelOptions)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@caitp I couldn't understand from your comments if there is a specific use case that you think is not covered here, or if you only think that an additional refactor is required for the form validation process. An example would probably be very helpful.

Copy link
Contributor

Choose a reason for hiding this comment

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

Part of the problem is when we want to re-validate when an interpolated attribute pertinent to a validator changes. We can't do this currently without jumping through hoops, because basically all we can do is $setViewValue() again, and that will be thrown away due to the value not changing.

The fact that setViewValue is tied so closely to validation and committing the view value is a problem. We have multiple operations happening here, and it's not really possible to run any of them independently without adding serious hacks, and quite likely breaking things. We're "crossing the streams", and badness and hacks ensue, this is not good.

This is, in my view, a poorly designed system which has been hacked together carelessly, and we really need to put some serious thought into how this can work better.

All of these operations need to be able to happen independently, without side-effects, so that we can cover all of these use cases:

  • Native HTML5 validation
  • Validators with parameters which can change at runtime
  • Deferred or instant reporting of validity
  • Deferred or instant updating bound values

There are certainly other cases which are missing here. But the solution to these problems is not to pile on more hacks. It's gotten to the point where it's crazy, and I don't think we should keep it that way.

I'll need to hear what Igor thinks about this, because I'm guessing he would rather keep it as is, but I don't think we're going to stop fixing bugs in 1.x in the near future, and I'm pretty sure that the current forms design is just going to keep creating new issues which are impossible to fix without hacks. Combining the three operations into one is just making a mess of things, and it's a disaster

Copy link
Contributor

Choose a reason for hiding this comment

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

To put this another way, the model we had previously was much simpler, we were still mixing different operations, but not as badly, and it would have been much easier to fix these use cases which are broken in 1.3. But now that we've got this poorly designed schlop which combines too many things, it becomes extremely difficult to fix any problem with one operation without side effects or blockage from another operation, because they're so tied together.

I want to revalidate without updating the value, but the value reporter blocks me on this because validation and value reporting are so tied together

This is not a workable system, and I think we need to fix this, or else just throw away ngModelOptions entirely.

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 want to revalidate without updating the value, but the value reporter blocks me on this because validation and value reporting are so tied together

This has always been the case regardless of ngModelOptions and it has now made possible with the use of ctrl.$validate(), you can do it any time you like without changing the value and we are already doing it, for example, when validator definitions change.

  • Native HTML5 validation
  • Validators with parameters which can change at runtime
  • Deferred or instant reporting of validity
  • Deferred or instant updating bound values

The current implementation supports all of those use cases, and I'm sorry but I really don't think it is poorly designed and certainly not made out of hacks. Refactoring is always a good idea with code which changed so much in so little time, obviously, but I have to disagree about he rest...

Copy link
Contributor

Choose a reason for hiding this comment

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

This is objectively a poor design.

We have multiple operations happening, and it is not currently possible for them to happen independently. For a number of these use cases, we need them to happen independently.

In order for them to happen independently, we have to introduce hacks to get around restrictions introduced by other parts of the system, which in turn break yet other parts of the system.

There is no way for this to work successfully in the long run, we need to do a better job of this.

Whether we actually do fix this or not, that depends on priorities set out by other people. However at the moment, this is complete and utter nonsense, it simply does not work.

It's not that the use-cases of ngModelOptions are bad or invalid, I'm not saying that. However the execution of ngModelOptions and the validators pipeline currently cause a lot of problems. The original system had its share of problems too, but it was more manageable due to not having these extra two hacks piled on.

We can (and arguably should) do this better, but I think we should wait and see what other people think before trying to come up with a better way to do this.

I admittedly didn't offer many thoughts when ngModelOptions and the validator pipelines were being reviewed, and it was less clear what the problems were at the time, but it's become clearer to me now that we are mixing up too much functionality which needs to be performed independently under a number of circumstances, and there's no good way to make this work without adding hacks like the revalidate option. The solution here removes one hack, but it replaces the hack with another restriction which will block similar use cases. This is not workable, this is not good, we need to do better, and we need to actually think about what we're putting together. Angular isn't really a toy project, people use this library, so we should put some actual thought into its development and avoid releasing stuff which is hacked together in this way.

I apologize if I sound too critical right now, I wouldn't have objected during review of ngModelOptions because these problems weren't obvious to me at the time. I just think we should learn from these mistakes and not ship a pile of hacks when 1.3 is released. But whether it's a priority to fix this or not remains to be seen. There are a lot of areas which need to be refactored, and some of them simply won't be due to complexity and time constraints, but maybe the forms component is important enough that it can be corrected.

Copy link
Contributor

Choose a reason for hiding this comment

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

you can do it any time you like without changing the value and we are already doing it, for example, when validator definitions change.

Not really --- If a validation parameter changes, we can A) run $validate() --- which will not run parsers or formatters (and due to its design, not all validators can necessarily be stuck in the $validators pipeline), or we can run $setViewValue(), which will have problems due to being so thoroughly tied to the ngModelOptions system, and introducing undesirable side-effects.

Needless to say, this is not good, either for users or maintainers of the library. We need to do this better. At least, that's what I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When do you have to do validation in parsers/formatters instead of validators?

return;
}
ctrl.$$lastCommittedViewValue = viewValue;

// change to dirty
if (ctrl.$pristine) {
ctrl.$dirty = true;
ctrl.$pristine = false;
$animate.removeClass($element, PRISTINE_CLASS);
$animate.addClass($element, DIRTY_CLASS);
parentForm.$setDirty();
}

var modelValue = viewValue;
forEach(ctrl.$parsers, function(fn) {
Expand Down Expand Up @@ -1881,14 +1881,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, revalidate) {
this.$setViewValue = function(value, trigger) {
ctrl.$viewValue = value;
if (!ctrl.$options || ctrl.$options.updateOnDefault) {
ctrl.$$debounceViewValueCommit(trigger, revalidate);
ctrl.$$debounceViewValueCommit(trigger);
}
};

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

Expand Down
28 changes: 28 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 @@ -2228,6 +2228,18 @@ describe('input', function() {
expect(inputElm).toBeInvalid();
});

it('should invalidate number with ngModelOptions if suffering from bad input', function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

another test case similar to the one from line 2274, but with ngModelOptions, would be useful

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

compileInput('<input type="number" ng-model="age" ' +
'ng-model-options="{updateOn: \'blur\'}"/>', {
valid: false,
badInput: true
});

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

it('should validate number if transition from bad input to empty string', function() {
var validity = {
Expand All @@ -2243,6 +2255,22 @@ describe('input', function() {
expect(inputElm).toBeValid();
});

it('should validate when bad input number with ngModelOptions changes to empty', function() {
var validity = {
valid: false,
badInput: true
};
compileInput('<input type="number" ng-model="age" ' +
'ng-model-options="{updateOn: \'blur\'}"/>', validity);
changeInputValueTo('10a');
browserTrigger(inputElm, 'blur');
validity.badInput = false;
validity.valid = true;
changeInputValueTo('');
browserTrigger(inputElm, 'blur');
expect(scope.age).toBeNull();
expect(inputElm).toBeValid();
});

describe('min', function() {

Expand Down