fix(input): improve html5 validation for ngModelOptions

[shahata]

This is a minor adjustment over the recent html5 validation fix in order to make it work correctly with ngModelOptions. The gist is that $commitViewValue now runs the validation pipeline even if the view value remains empty in order to refresh the validity state.

[mary-poppins]

Thanks for the PR! Please check the items below to help us merge this faster. See the contributing docs for more information.

• Uses the issue template (#7976)

If you need to make changes to your pull request, you can update the commit with git commit --amend.
Then, update the pull request with git push -f.

Thanks again for your help!

[caitp]

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

[shahata]

done

[shahata]

ping @caitp

[caitp]

pong, it looks good to me (but maybe we should re-dirty the input if the value goes from, for instance, badInput + empty to valid empty --- or maybe that doesn't matter?) Anyways, @matsko, wdyt?

[caitp]

@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.

[caitp]

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)

[shahata]

@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.

[caitp]

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

[caitp]

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.

[shahata]

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...

[caitp]

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.

[caitp]

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.

[shahata]

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

[petebacondarwin]

@caitp - clearly you feel very strongly about this. While it is useful to have these discussions and we should not be bound unnecessarily to the current design, be careful to stay focussed on specific issues as I am worried this this sort of discussion could become unconstructive.

I think we would be helped if we had concrete examples of how the current ngModel architecture was not able to cope well. For example, it would be useful to list the situations where validators cannot live outside of parsers/formatters as requested by @shahata.

Also I am not clear what one means by reporting over testing. Perhaps you could provide some pseudo code for this?

[caitp]

I've already provided specific examples @petebacondarwin. I don't think I can boil this down any simpler than this: http://youtu.be/jyaLZHiJJnE

Too many unrelated operations are happening in $setViewValue, and not enough is happening in $validate --- one has side effects and is prevented from behaving as an extended $validate() because of those side effects, and requires hacks to be used. The other is inadequate because we aren't able to call any of the validators which live in $parsers/$formatters --- rightly so because this is tied too closely with modifying the view value. But the problem is, a number of validators live in $parsers/$formatters by necessity, due to being required to work in a specific order (we still get an array instead of a map with an implementation-defined order of operations).

This design is not good --- we have too many cases where the operation we want to perform has undesirable side-effects, or requires stupid hacks to circumvent problems. We need to pick these operations apart, because when they are so tied together like this, it's really impossible to work with.

That said, it's not clear we actually will fix this, so maybe it will be a mess like this forever. But I think we should, because this should not ship. We are not supporting all use cases, and we are making maintenance impossible.

[shahata]

But the problem is, a number of validators live in $parsers/$formatters by necessity, due to being required to work in a specific order.

@caitp can you please give an example where this is important? What do you mean by "specific order"? Why can't $validators run in specific order just as well? The way I see it the real problem here is that this kind of validators have to exist in $parsers/$formatters and I really think we should just abolish them... (or at least agree that they are far fetched and shouldn't effect our design)

[caitp]

@shahata I've done that earlier in this thread. This is especially important for HTML5 validators, because when certain validators are run, others need to be aborted, the order of operations is important here. This is also important for other validators, because their running depends on what's happen in the preceding validators.

We need this to be ordered, and a map just does not do that for us. Yes, we should not be changing the value during validation, but we should be running validators in a specific order.

And please, stop trying to make up excuses about this design somehow "working", this is unsuitable for a number of the common use-cases. The only use-case that this design actually "works for" is when a validation needs to happen because a user changed a value (because all of the side-effects are desirable in that case) --- and even then it doesn't really work and requires us to add hacks in certain cases (aka the reason for this patch in the first place --- which doesn't actually get rid of the hack, it just moves it).

We CAN NOT ship code like this. Maybe we will, but I don't think we should. This is bad.

[shahata]

Sorry, I probably missed it or didn't understand. Well, I see the need (though I'm not sure I understand the exact use case - we can sort that out later) to abort validations before other $validators run, but it seems to me that we just need to re-think how $validators are executed, which is really not a big deal imo. Once html5 validators can be added to $validators and still run in the correct order and abort validations, this whole thing can look much better... Do you disagree?

[caitp]

The primary need is to avoid erroneously reporting errors and causing naive applications to report errors incorrectly. That's why the ordering is important in native HTML5 validation, and it applies here as well.

Ordering is also very important when validation is tied to parsing / formatting the value, which we obviously shouldn't be doing

---

Once html5 validators can be added to $validators and still run in the correct order and abort validations, this whole thing can look much better... Do you disagree?

No, I'm still not sold on the way $setViewValue is doing far more than it should be doing. I think that whole area needs to be re-designed. If we prevent undesirable side-effects from being unavoidable, then we make this code far nicer for both users AND maintainers.

[shahata]

Yes, I guessed so. So why not just support controlling the order & allow aborting in $validators so we can move html5 validators there? it seems wrong to me that to leave those validators in $parsers and indeed it causes a lot of problem when we need to re-validate.

[caitp]

I agree that we don't want to be doing validation during parsing or formatting.

But moving those doesn't solve this entire design problem really. The validators pipeline needs work, yes, but so does the view value pipeline. I just don't see this as adequate, I am fairly confident this will pose more problems in the future, and those problems --- if they're fixed at all --- will probably be fixed with hacks that make things even worse. I do not feel good about shipping this.

[shahata]

What design problems do you see in the view value pipeline? Regarding the validators pipeline, I would be happy to work on this with you if indeed this is going to happen.

[caitp]

It does too much, and it aborts the operation based on some criteria which we don't always want --- breaking this apart into separate operations would solve this.

To restate my original idea, testing validity, formatting/parsing values, and committing values, are all really distinct operations --- currently, the three of these are all happening in $setViewValue, and I don't think we want this. Sometimes we only want to execute 2/3 of these, and sometimes only 1/3, so having them so tightly coiled together is a problem (and also leads us to add hacks like this / the previous CL)

[petebacondarwin]

Can we create a pseudo code set of use cases that demonstrate how it ought to work?

For instance, one could be:

• some validator criterion changes
• run $validate()
  • update the $modelValue / $invalidModelValue
  • update $valid/$invalid, etc

Another could be:

• input value changes (no debounce)
• $setViewValue()
  • $viewValue updated
  • $commitViewValue()
  • $validate()
    • update the $modelValue/$invalidModelValue`
    • update $valid/$invalid, etc
  • run $parsers
    • update $model / $invalidModelValue

And so on. I am sure that we have many of these covered (and probably in unit tests already) but in particular, for those cases that are problematic, I find that statements like:

It does too much, and it aborts the operation based on some criteria which we don't always want

too vague.

[petebacondarwin]

I need to spend some time looking at the issues around interoperating with HTML5 validation so I am not talking from any stand of authority here, but my understanding is this:

Really, we would like to only validate the $modelValue. This would mean that the parsers and formatters are purely transformers that convert the $viewValue to the $modelValue and back again. The validations should run when the $modelValue changes or when some other validation dependency changes causing the validation to need to be run again (by calling $validate(). This also means that we are free to debounce the $viewValue changes without worrying about validations, since they are only concerned with the $modelValue.

The problem is that HTML5 validations are run by the browser, and that those validations occur on the value of the input element, before the transformations have had an opportunity to run. So we end up with this nasty situation where the viewValue can be modified by the browser because it thinks that the value is invalid which causes our $parsers to run with a new $viewValue.

The question is how do we marry these two scenarios, right?

[caitp]

This case is pretty unrelated to HTML5 validation --- however it is one of the cases where we abort validation when we don't want to, and where setting tis as one of the $validators rather than parsers or formatters is problematic because we really need to either A) abort validation before this one runs, or B) abort validation after this one runs --- so that we don't over-report error conditions, but we can't actually do that at the appropriate time in this case.

However, this case isn't really the only case, it is a good example of the problems with shoving too much into $setViewValue().

We've discussed how we can fix this up and balance this a bit better, but even if we fix the $validators pipeline, we're still going to be doing too much in $setViewValue(), and it's still going to be covering work which isn't related to its actual job.

[Narretz]

Interesting discussion here. Here are my 2 cents:

Another use case to be thought of: Setting invalid values on the model. This is a legit thing for keeping pre-filled invalid form data in the model.

I am also for a systematic approach for the discussion. Once we have compiled all the requirements we need, everybody can have a shot at coding something.
Would it make sense to provide unit tests for cases that should work (but currently don't)? Specifically I would like to see a case where HTML5 validators cause overreporting as @caitp described.

@petebacondarwin

Sometimes it is necessary to validate the $viewValue (but it is possible to do this in $validators, after $parsers ran). Two examples:
a) maxLength currently checks the modelValue, but for some values this doesn't make sense, e.g. dates parsed to objects. In that case, it is more consistent to always parse the $viewValue
b) date again; currently a date like 1999-13-32 is allowed, because javascript converts these to real dates. That is undesired and it should be possible to write a validator for that, which is only possible with the $viewValue

[caitp]

Specifically I would like to see a case where HTML5 validators cause overreporting as @caitp described.

We have piles of these --- In the current implementation we over-report required validation in the case of numbers suffering from bad input in modern browsers, and there are only going to be more of these as time goes by.

The constraint validation API was really put together without too much thought, and using it in angular has similarly been hacked together without too much thought. The shortcomings of the spec require us to be clever in how we deal with that, otherwise we overreport errors when we ought not to.

This imposes strict requirements about the order different validators are run in, and this is something that iterating over keys in a map simply doesn't give us.

[caitp]

All I can say is, this current implementation is extremely frustrating to do anything with, because it is so tightly built around a specific use case, and does not leave any room whatsoever for handling alternate cases where we only need a subset of the functionality.

This is the primary reason why we're implementing these hacks (as seen in this PR and the one which preceded it) --- to hack around just plain shoddy design decisions. I would argue that this is not acceptable and should not ship this way, and I've said this quite clearly in this thread a few times.

The intention isn't to upset anyone or be overly critical, but what we have here is just plain bad, it's not good enough, it does not suit a number of use cases adequately, and we need to do this over again.

I will not be happy fixing bugs in this module in 6 months if it is still built the way it is today, because it's a Rube Goldberg machine, every single change has unexpected side-effects, and this is just plain wrong. Seriously, it's amazing that this wasn't so obvious while this was being reviewed.

[shahata]

I'll try to submit a pull request tomorrow night which tries to address the issues raised here. In general I believe this is mostly related to embedding html5 validators into the $validators pipeline correctly, but I'm pretty sure after that's done the view value pipeline can be refactored nicely.

[caitp]

You're wrong about that! Again!

Listen --- It's not that simple, it's not just "move things into $validators and it's all honky dory!". We are mixing up too much functionality in $setViewValue --- we should not be doing that. We have no good reason to do that. Yes, parsers and formatters should not be performing validation, but we should also not be parsing or formatting in this pipeline --- we should not be dirtying or undirtying inputs in this pipeline. Literally the only thing $setViewValue should do, is what it purports to do --- update the value in the view. Everything else is unrelated to this operation, and should be handled seperately, so that we aren't forcing it to happen in cases where we don't want it to. And there are cases where we don't want to, but this design is built so heavily around the use cases where we do want to that it makes fixing other peoples problems impossible -__-

[shahata]

@caitp - this is the current implementation of $setViewValue:

  this.$setViewValue = function(value, trigger) {
    ctrl.$viewValue = value;
    if (!ctrl.$options || ctrl.$options.updateOnDefault) {
      ctrl.$$debounceViewValueCommit(trigger);
    }
  };

Are you saying that it shouldn't even invoke $$debounceViewValueCommit and that this should be done by the input directive?

[lrlopez]

@shahata @caitp Let's try to be constructive. I don't think is a bad idea to go back to the drawing board now that we have a $validator pipeline that needs to work with parsers, formatters and debouncing. All the new changes are still in beta, so they're not yet written into stone.

Surely I'm not the right person to opinate on this, but I suggest creating a new branch and a discussion thread/PR where we could state the use cases that must be covered so we are able to distribute responsabilities to each pipeline or directive (input, ngModel, $formatters, $parsers and $validators, etc). On the worst case, If the final solution is not better that the current one, we could discard the branch or just cherrypick some enhancements. In the best case, we could get a revamped, bug-free and future-proof code.

[IgorMinar]

@caitp can you please control your temper? the sentiment in your comments is inappropriate and on the border of violating our code of conduct. As a committer you are expected to show exemplary behavior in our community.

It's certainly expected from you to be critical of things you perceive as incorrect or flawed but you can do so in a constructive way without being accusatory or rude.

Everyone who has contributed to ngModel and ngModelOptions changes had their best intention on mind and was trying to address valid use-cases. When doing so, it's possible that some higher level design decision was overlooked.

It's apparent that you feel that we could redesign the current system to make it better and I therefore challenge you to create a high-level design doc on the changes you are proposing, the use-cases they cover and flaws of the current system that it solves.

[shahata]

@caitp maybe this should be merged in the meantime? I think we want this in anyway since this test should pass even if we decide to refactor the whole thing...

[caitp]

Yes, you're right. It's still okay to land this, from my perspective.

[matsko]

Hopefully ES6 will give us better language tools to let us develop a more evolved API for ngModel.

[matsko]

Closing this for now since db044c4 fixed the native HTML5-related bugs. The plan is to use as little as possible of that API.

[caitp]

@matsko did you get rid of the revalidate parameter? because we want to drop that. (I don't see it in the diff you linked)

[matsko]

Sorry. Looks like I missed that one. I need to take a further glance and compare the revalidate portion to see if it fits in with what's on master.

[IgorMinar]

Please get rid of it
On Aug 27, 2014 7:11 AM, "Matias Niemelä" [email protected] wrote:

Sorry. Looks like I missed that one. I need to take a further glance and
compare the revalidate portion to see if it fits in with what's on master.

—
Reply to this email directly or view it on GitHub
#7976 (comment).