fix(input): validate minlength/maxlength for non-string values - #7968

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 (#7968)

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, seems that #7856 and #7858 should be closed, because they are duplicates

I'm going to leave them open for now because of the different approach taken (view value vs stringified model value). I'm undecided on which is the better approach

@caitp The order of processing is $viewValue -> $parsers -> $validators, so validation will possibly be wrong when comparing with viewValue, no?

Define "wrong"? If your view value is like $500.00, are you expecting to test the max length of $500.00 or 500.00?

I feel like you'd generally expect the typed value to have to fit into the maxlength, because people aren't thinking about what the data looks like under the hood, so I think comparing the $viewValue for non-string models should be fine

Maybe we should always be testing the $viewValue rather than model value, in that case

Hmm, with the $validators, the role of $parsers is kind of undefined, but I tihnk @matsko had his reasons to use the parsed model value for validation.

With something like ui-mask, http://angular-ui.github.io/ui-utils/#/mask, it's imperative that the value that is validated is parsed, because the viewValue contains characters that do not end up in the model and will distort the validation result.

From a developer's perspective it also doesn't make much sense to validate the model, then parse and commit it, possibly making it in invalid again in the process.

The whole thing is veeery complex. I always have the feeling I'm only looking at half the cases that need to be covered.

maybe people shouldn't use maxlength with ui-utils --- maxlength validation should validate what the user enters, not some transformed value that might look totally different, heh

That's a good point because the mask will do it's own validation anyway ... it also wouldn't work with objects (say type="date"). Maybe it's okay.
Makes me think there's a need for viewValue and modelValue validators, e.g. for disallowing dates like 99-99-9999

I think we should merge this (maybe after validation settles), but convert the viewValue to string beforehand. If the model is a Number we have no length attribute.

There is a question about applying this directives. According to MDN documentation:

If the value of the type attribute is text, email, search, password, tel, or url, this attribute specifies the maximum number of characters (in Unicode code points) that the user can enter; for other control types, it is ignored.

I think we should apply same rule here.

There is not much sense in validating length for a number input, can use the max and min directives for that problem. Same thing about input[date].
ng-minlength and ng-maxlength are not mentioned in the documentation for input[date] anyway :)

@airato Thanks for the input again. While ng-maxlength is not documented for date etc., it's now a standalone directive and can be applied to every input.

We should just document that this directive will always validate the input value that's visible to the user ($viewValue), and internally convert the $viewValue to a string before validating it.

@matsko / @shahata could you review this? thanks

Since this isn't entirely clear from the initial description:
This fixes not only cases where input=number, but also where input=text and ng-model is a number. So it's also a fix for #8811

It's not just for type=number, because the fix is to look at the viewValue rather than modelValue. yes.

Just change the variable names. Otherwise LGTM.

Okay, just updated the variable names. I think this closes #7967, #8811, #7856, #7848, #7858, and maybe others. @Narretz am I missing any? are any of those incorrect?

I think you got them all @caitp

However, I am not entirely sure this is completely correct. Why aren't we simply always checking the $viewValue? If someone parses $viewValue into a different length $modelValue string (for whatever reasons), the result may be unexpected to the user (or developer).
Also, are we sure that the viewValue will always be a string? I remember in one of those isses we talked about how one browser actually represents something as number where it should be string. I'll see if I can find it. If this is indeed an issure, we should convert to string for good measure.

@Narretz your points are valid but I think that the natural use of the minlength attribute is for comparing user entered data on input elements. I think we should really question in what situations minlength or maxlength doesn't work (say date elements and so on).

Thinking about this some more, we should always compare the viewValue property and never the modelValue. This is because the viewValue is always what the user enters and if the parser changes the input data then it may be confusing to figure out if their input is wrong.

There are at least two cases where the $viewValue in type=text can be a number. First, when the value comes from the scope, and second when it's set programmatically with $setViewValue().

http://plnkr.co/edit/euEvTIbIxr1piZ093SnC?p=preview

Maybe $setViewValue should convert everything to string? But that probably wouldn't catch all cases. Or we could simply always do the conversion in the min/maxlength validator.

@Narretz input[type=text] should format the model value to a string and parse the view value to string as well before setting the model.

I think that it's safe to say that input[type=text] will always return a string based value when truthy (via element.value).

But for when the model is assigned we should be doing this:

function textInputType(scope, element, attr, ctrl, $sniffer, $browser) {
  ctrl.$formatters.push(function(value) {
    return ctrl.$isEmpty(value) ? value : value.toString();
  });
  baseInputElement(scope, element, attr, ctrl, $sniffer, $browser);
}

I just created this fiddle to file this exact issue, guess I should've searched here before because you guys are one step ahead 😄

Hoping to see this merged. Thanks for the good work!

@matsko Will this formatter be applied to all input types that are text-like, i.e. also url, email etc.? They all have the same requirement I think.

@Narretz yes. It applies to: blank (when no type is set), text, email and url.

matsko@fd817ec

This commit is now merged
77ce5b8

And the view value is always formatted as a string:
1eda183