fix(input): fix false initial 'number' validation error in input[number]...

[gkalpak]

... with observed attributes

When no value has been set for an input[number] it's viewValue is undefined. This lead to false
parse validation errors, when ctrl.$validate() was fired before interacting with the element. A
typical situation of this happening is when other directives on the same element $observe()
attribute values. (Such directives are ngRequired, min, max etc.)
More specifically, the parser for native HTML5 validation returns undefined when the input is
invalid and returns the value unchanged when the input is valid. Thus, when the value itself is
undefined, the NgModelController is tricked into believing there is a native validation error.

Closes #9106

[gkalpak]

Hm...originally I thought it was related to ngRequired specifically, but it turnd out it was a more general issue. Nonetheless, I have also added some tests for ngRequired on input[number](there was none previously), which aren't all relevant to the issue.
Do you think I should remove them and put them into a separate commit ?

(I also removed some blank lines in the name of uniformity; I am not sure if it's OK though :D)

[gkalpak]

For the fun of it, until this issue gets solved, here is a little addition that solves the problem.
(It's a small directive for input[number] elements that swaps the first two parsers.)

[Narretz]

Hi, thanks for the PR. However, I'd like to check if this isn't a more general issue that should be solved differently, since (if I follow your explanation in #9106 (comment) correctly) the parser pipeline goes haywire if validate is called with "empty" modelValues.

[gkalpak]

@Narretz:
It is the parser added by badInputChecker() that introduces this problem (i.e. confuses the NgModelController if the value is valid but undefined).
The only two input-types that use the badInputChecker() are number and the various date types.

My PR solves the number-type problem.
With date-types there is no similar problem, because the date-type formatter sets the viewValue to an empty string (not undefined):

ctrl.$formatters.push(function(value) {
    if (isDate(value)) { return ... }
    return '';   // <-- If the modelValue was undefined, the viewValue will be an empty string
});

This comes in contrast with the number-type formatter, which allows an originaly undefined viewValue to remain undefined:

ctrl.$formatters.push(function(value) {
    if (!ctrl.$isEmpty(value)) { ... }
    return value;   // <-- If the modelValue was undefined, the viewValue will be undefined as well
});

[43081j]

Just FYI this is likely related to #9044 too, so it also affects selects.

Maybe some general solution could be achieved? As it isn't exactly specific to one input type. I'm not sure why we even started treating undefined as invalid, i've never seen a case where it is until these changes were implemented and forced people to treat it as such.

[gkalpak]

The problem with ngList is indeed similar (although not related to badInputChecker() nor native validation.

But it is again the combination:
a. The formatter returns undefined if the modelValue is not an Array (e.g. uninitialized).
b. The parser returns undefined if the viewValue is undefined (which tricks NgModelController into believing there is a parse error).

So, it is a more generic problem.
The truth is that there is great inconsistency on how each input type handles their empty/uninitialized values. E.g. some format undefined modelValues to empty strings as viewValue, some leave it undefined, some return it as is.
That, in conjunction with the fact that NgModelController recognises an undefined modelValue as a parse error leads to issues.

I am not that confident with all the complexities of the NgModelStuff and all input directives, but to me it doesn't sound horrible to initialize a modelValue/viewValue to a default empty value per input type (if no value is specified).
I mean, if I bind my model to an empty text-field, I shouldn't be bother that it is an emty string and not undefined when the field is empty (even without user interaction). If I bind my model to an unchecked checkbox, I shouldn't be bothered if it is false and not undefined. If I bind my model to an ngList on an empty field, I shouldn't be bothered if it is an empty list and not undefined.

I feel that binding the model to an input is "programatic change" enough to justify setting it to the value that the input's current state/value call for.

(I am afraid @caitp will not agree though :))

[Narretz]

Yeah, this is an interesting question But the deeper issue was that angular should not modify the modelValue when the input / parsed viewValue is empty, that is it should somehow short-circuit after parsing if no modelValue is created. I thought about aborting parsing altogether when the viewValue is empty, but it's possible that a parser transforms an empty value into a value, so that's not gonna work. Invalid parsers returning undefined is obviously the elephant in the room for this approach.

[gkalpak]

The main issue is that parsing and validating should be independent, but aren't right now.

That said, I am not convinced that "Angular should not modify the modelValue when the input / parsed viewValue is empty". Why shouln't it ? As soon as I bound my model to specific type of input, I make a commitment and expect that my model will have a value reasonable for that type of input. This includes the empty value.

I don't agree that a model bound to an empty number input should have the same value as a model bound to an empty data input or to an empty ngList input. I think it is only natural that an empty list will be [], an empty text will be '', an unselected checkbox will be false etc.
(I don't say it is ideal, but I find it acceptable and it would certainly bother me less than the current situation.)

[caitp]

See #9044 (comment) --- the fix for avoiding the parse error when validate() is called with undefined view values has already landed

[caitp]

I think we might want these tests though anyways, I will refactor the commit a bit and get the tests in

[caitp]

then again, the tests are sort of variations on existing tests in the tree.. hmm --- I will add one of them anyways

[gkalpak]

@caitp: Yes, I've seen you landed a fix for that. I was thinking if it would make sense to merge the tests though, because actually there is only one test for ngRequired.
There are several tests for required, but it is not the same attribute and we might make sure ngRequired works as expected (in addition to required).

Tell me if you think this is a good idea and if you need me to make a separate commit with just the tests. (Note that since your commit landed, 3 of my tests need a slight modification (.toBeNull() --> .toBeUndefined()) in order to pass.

[caitp]

There are ngRequired tests for the more general case, not specific to number inputs --- but it's basically verifiably working as expected when we don't treat an unspecified view value as a parse error

[caitp]

However if you think the entire suite of ngRequired tests are needed for number inputs, we could certainly add another commit with those tests

[gkalpak]

@caitp: I added the tests in #9316.
They are bound to number inputs, but any other input would do I guess.

I can see there are tests for required, but very few for ngRequired (only 2 in the core right now) and not covering the full functinality spectrum of ngRequired.
In any case, a couple more quick tests never hurt anyone :)