feat(ngMaxlength): disable maxlength when not set to a non-negative integer

[gkalpak]

1. Previously, setting the maxlength to a negative number, would make all input values invalid (since their length should be less than maxlength, which is impossible).
2. Furthermore, supplying a non-numeric value would set maxlength to 0 (effectively only allowing empty values - although this is not very likely the intended behaviour).

The 1st commit fixes (1): Adds support for disabling the max length limit by setting maxlength to a negative number.

The 2nd commit "fixes" (2): When specifying a non-numeric value for maxlength (e.g. "", null, "abc"), the max length limit is disabled (i.e. the validator will return true for any input).

Note that the behaviour implemented in these 2 commits is more inline with how the HTML5 maxlength attribute works (both in browsers and according to the spec).

Closes #9874
Closes #9995

[gkalpak]

I splitted this in 2 separate commits, as the second one is possibly a "breaking change".

(Not sure about the definition of "breaking change", but...) if someone sets maxlength to a non-numeric value expecting the input to be invalid unless empty, then this change will break the "expected behaviour".
But defaulting to 0 for non-numeric values was never documented and I find it highly unlikely to be the case for any app (and if it is, simply setting maxlength to 0 will have the same effect).

[pkozlowski-opensource]

LGTM after first, quick look. While this change might break some crazy corner scenarios I think this is a right thing to do.

I would say - let's have another pair of eyes glancing over this one and let's get it in.

[Narretz]

Does new Array(1001).join('a') serve any deeper purpose? If not, I'd make it a simple string.

[gkalpak]

Just avoiding hard-coding a 1000 character long string.
But if you want me to hard-code that, let me know (in which case we probably don't need such a long string).

In fact, if we wanted to push this to the limits we might want to use new Array(Number.MAX_SAFE_INTEGER) or something.

[pkozlowski-opensource]

Well, we can't use Number.MAX_SAFE_INTEGER - afaik it is only part of ECMAScript 6 proposal, no?

[Narretz]

Unless I am missing something, this test does not actually test if I can enter a very long value, it just tests wheter a value of any length is valid. So the last change/expect is redundant.

[pkozlowski-opensource]

Agree with @Narretz

[gkalpak]

@pkozlowski-opensource: You are right about MAX_SAFE_INTEGER. I meant Number.MAX_VALUE or something, but...see below.

@Narretz, @pkozlowski-opensource:
Basically, it is difficult (if at all possible) to test that values of any length are allowed.
So, I guess just testing a non-empty value will be "good enough" :(
I'll remove the last test.

[gkalpak]

@Narretz, @pkozlowski-opensource: Do you think I should get rid of new Array(...).join(...) here and just use a string (say 10 chars long) ?

[Narretz]

Yep, sounds good to me

----- Ursprüngliche Nachricht -----
Von: "Georgios Kalpakas" notifications@github.com
Gesendet: ‎11.‎11.‎2014 18:34
An: "angular/angular.js" angular.js@noreply.github.com
Cc: "Narretz" mjstaffa@googlemail.com
Betreff: Re: [angular.js] feat(ngMaxlength): disable maxlength when not set toa non-negative integer (#9998)

In test/ng/directive/inputSpec.js:

•  expect(inputElm).toBeInvalid();
  

• });

• it('should accept values of any length when maxlength is negative', function() {

•  compileInput('<input type="text" ng-model="value" ng-maxlength="-1" />');
  

•  changeInputValueTo('');
  

•  expect(inputElm).toBeValid();
  

•  changeInputValueTo('aaaaaaaaaa');
  

•  expect(inputElm).toBeValid();
  

• });

• it('should accept values of any length when maxlength is non-numeric', function() {

•  compileInput('<input type="text" ng-model="value" ng-maxlength="{{maxlength}}" />');
  

•  changeInputValueTo(new Array(1001).join('a'));
  

  @Narretz, @pkozlowski-opensource: Do you think I should get rid of new Array(...).join(...) here and just use a string (say 10 chars long) ?
  —
  Reply to this email directly or view it on GitHub.=

[gkalpak]

Removed new Array(...) and rebased.
/ping @Narretz, @pkozlowski-opensource

[pkozlowski-opensource]

This LGTM, putting it for 1.3.4. Anyone wants to have another look before it gets merged?

[shahata]

I think this behavior should be documented...

[gkalpak]

Added a mention in the docs (thx @shahata).
Not particularly fond of my wording. (Open to better suggestions 😃)

[Narretz]

How about "Setting the attribute to a negative or non-numeric value allows view values of any length" Oh, and doesn't that happen for 0 too?

[gkalpak]

I like this better; will update the docs.

The spec requires a non-negative integer, so 0 is valid (and will basically only accept an empty value).

[gkalpak]

Updated the wording based on @Narretz's suggestion.

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