fix(input): create max and/or min validation whatever the initial value is

[tobyee]

• fix issue max validator of input element do not work as expected on some cases #10307
• change tests to corresponding changes
• also change tests for ngmax and ngmin (though they have no some issue)

Closes #10307

[googlebot]

Thanks for your pull request.

It looks like this may be your first contribution to a Google open source project, in which case you'll need to sign a Contributor License Agreement (CLA) at https://cla.developers.google.com/.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check the information on your CLA or see this help article on setting the email on your git commits.

Once you've done that, please reply here to let us know. If you signed the CLA as a corporation, please let us know the company's name.

[tobyee]

I've signed a CLA now.

[googlebot]

CLAs look good, thanks!

[Narretz]

Thanks! Can you add the following: an except after you change the input the first time, and change the tests for ngmin and ngmax accordingly. Even though they are not affected, it's good to have these tests.

[tobyee]

I've updated the PR .

[Narretz]

can you add an expect here (to all the changed tests)? We should make sure that the validator does the right thing when the min/max value is undefined and the input value is changed for the first time

[gkalpak]

Since we are fixing this, lets do it right:

ngMin/ngMax "suffer" from the same issue. They should be fixed too.
And other input types have similar problems (e.g. date).
We should fix all cases (and add tests for all attributes (min/max/ngMin/ngMax)).

[Narretz]

pretty weird, why do the ngMin test use min="min" instead of min="{{min}}"? With the first form, all tests pass, even if min is initially undefined.

[gkalpak]

@Narretz: Not weird, but unexpected 😄

It is because ALIASED_ATTRs (such as ngMin) use $watch instead of $observe (L380):

forEach(ALIASED_ATTR, function(htmlAttr, ngAttr) {// htmlAttr -> 'min' | ngAttr -> 'ngMin'
  ngAttributeAliasDirectives[ngAttr] = function() {
    return {
      priority: 100,
      link: function(scope, element, attr) {
        ...
        scope.$watch(attr[ngAttr], ...);
      }
      ...

[tobyee]

I think the reason why ngMin test use min="min" instead of min="{{min}}" is to reduce the number of test case. Other wise you need to 2x2 test case for min.

ALIASED_ATTR define at jqLite.js#L525

var ALIASED_ATTR = {
  'ngMinlength': 'minlength',
  'ngMaxlength': 'maxlength',
  'ngMin': 'min',
  'ngMax': 'max',
  'ngPattern': 'pattern'
};

Now we get the list:

input[number]: min/max/ngMin/ngMax
input[datetime/week/xxxx]: min/max/ngMin/ngMax
form: min="min" and min="{{min}}"

Any missed?

[gkalpak]

Weird fact 1: I can't find any documentation on ngMin/ngMax.

Weird fact 2: Looking at the code, ngMin/ngMax seem to accept Angular expressions (no interpolated strings; i.e. no {{...}}).

So, theoretically, we don't need the isDefined check for ngMin/ngMax.
Theoretically again, the tests should be {{xyz}} for min/max and xyz on ngMin/ngMax.

@Narretz: WDYT ?

[tobyee]

Be aware that max/min is an attribute of DOM, different from which has ng prefix.

[gkalpak]

@tobyee: Hm...I 'm pretty aware of that :)
What is your point ? Do you think this changes how things should be handled (with regard to my comments above) ? Did I miss something ?

[tobyee]

@gkalpak
Actually, I was thinking why use ngMax/ngMin (ngPattern/ngMinlength/ngMaxlength) for alias, because I can't find any documentation about them. Aware of that they do not exists in v1.2.x. It seem that the purpose is to distinguish DOM's attribute from angular's directive to avoid W3C validation errors, such as required and ng-required. Then we can use the ng directive instead of DOM's attribute when setting a interpolated value.

Given that, I think we should tests both.

[gkalpak]

@tobyee: I don't know why they aren't documented, but there is an important difference between them:
Using min/max (which are HTML attributes) will affect the behaviour of the browser (e.g. clicking on the up-arrow button on the number input will have no effect when you reach the max value; likewise for the min). Using ngMin/ngMax will let Angular do the validation, but the browser won't know anything about it.

In any case, I think we should have tests for both, but checking isDefined on ngMin/ngMax is not necessary, because they should contain expressions (not interpolated strings); so attr.ngMin/Max will always be truthy if ngMin/Max are present.

[tobyee]

@gkalpak
I aware that behaviour too (Though I expect having same behaviour), and I agree with you.
Imaging that when you creating a directive with template

var app = angular.module("foo",[]);
app.directive("myTest", function(){
  return {
    template: '<div><input type="number" max="{{maxval}}"></div>',
    scope: {},
    link: function(scope, elm, attr){
      //xxx
    }
  }
});

It's reasonable to do that, because the max attribute is a part of html. If use ng-max="{{maxval}}" insteaded, it's weird. This form is unexpected, it works because the directive convert string to number (L1257).

attr.$observe('max', function(val) {
      if (isDefined(val) && !isNumber(val)) {
        val = parseFloat(val, 10);
      }
      maxVal = isNumber(val) && !isNaN(val) ? val : undefined;
      // TODO(matsko): implement validateLater to reduce number of validations
      ctrl.$validate();
    });

And more, if we treat ng-max="{{maxval}}" as html, It should means that the expression is a constant, if the initial value is undefined then is undefined ever, change maxval should not make sense and even should prevent this form.

[tobyee]

Should I send a PR to fix this issue in 1.2.x ?

[Narretz]

Hi @tobyee, sorry for the late response. 1.2.x only receives security fixes, so we won't backport it.

In the commit message it says:

also change tests for ngmax and ngmin (though they have no some issue)
is this statement still relevant? If so, to what does "they have no some issue" mean? You mean that they are correctly created even when the value is undefined ?

[tobyee]

Yes, I

mean that they are correctly created even when the value is undefined