Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

Update input.js #6000

Closed
wants to merge 1 commit into from
Closed

Update input.js #6000

wants to merge 1 commit into from

Conversation

vitorlopez
Copy link

This commit solves de issue 5936 (#5936), by just retoring the previous condition in the negative form.

This commit solves de issue 5936.
@caitp
Copy link
Contributor

caitp commented Jan 27, 2014

So, under what conditions are you seeing typeof value === 'number' ? I believe input.value is always a string. For input[type=number] the only place you should see a number is the valueAsNumber property, which jqLite will ignore. I don't know if jQuery ignores it, but it certainly should.

@vitorlopez
Copy link
Author

If the value in my model is a Number, then the input value that angular gets is a number too.
In the issue I give 2 examples of this behavior.

But again, i agree with you. When I need a number I should use input[type=number].
But in the previous commits, works fine if I use a number in a input[type=text].

I just want to be sure that this behavior was intentional, or if was a small mistake. This why I send the pull request.

But if I am wrong, I suggest a small change in the docs to be clear that ngMinlength and ngMaxlength only should be use if the model value is a string, because if the model value was a number or anything else this won't work.

@caitp
Copy link
Contributor

caitp commented Jan 27, 2014

I've just tested the examples in your issue and value is never "a number" on Chrome 34, FF 26 or Safari 7 (I don't have an IE on hand, but I expect it to be the same). Validation behaves as expected, so I'm going to need you to provide a clear example of this going wrong.

The ngMinlength and ngMaxlength parameters don't care about changes to the model value (or the model value at all), they only apply to user input text, and the value is not the model value but rather the value property of the input element

We could make this work with explicit model changes too by adding a version of the validator to formatters which appends value to the empty string, but it's not clear that this is necessary

@vitorlopez
Copy link
Author

I will try to explain better... My issue is not browser related. In any browser you see this behavior.

In my 2 examples the variable 'test' equals '2' in the scope. You can see this in the ng-init at line 10. So it's type Number. Am I right?

But in the Angular 1.2.7 the value of the input binds perfectly (First example)

In Angular 1.2.8 the values don't bind. (Second example)

This occurs because of the change that was made by Igor in commit that I showed.
But theoretically he did nothing wrong. Except that previously had an 'else' statement, and is now only returning itself.

And when the value was a number, 'value.length' will return 'undefined'.
Previously the same code snippet returing the value, bacause catch in the 'else' statement.

I ask sorry if I am not clear enough, but I believe if take a deep look in the examples you will see my point.

@caitp
Copy link
Contributor

caitp commented Jan 27, 2014

The behaviour I'm seeing in both of your examples is the expected behaviour, I'm not seeing anything happening in either one that is unexpected. If the minlength is 2, and you type in 1 character, the field is invalid. This is the expected behaviour. minlength="1" doesn't really mean anything, because an empty field satisfies minlength validation (you need ngRequired to make an empty field invalid). This is all as-expected. What are you saying is going "wrong" with this?

And when the value was a number, 'value.length' will return 'undefined'.

The value is never a number, it is always a string. It can't not be if the DOM is behaving as it should. The value seen by the validator is the (possibly trimmed by the textInputType change handler) text value of the input.

From the HTMLInputElement IDL

[TreatNullAs=EmptyString] attribute DOMString value;

Also applies to the HTMLSelectElement

@vitorlopez
Copy link
Author

When Angular parses the value by the 'ctrl.$parsers' the value is exactly what is in the model. In this case will be a number. When the function 'minLengthValidator' runs, the 'value' parameter will be given by Angular and will be the same type as model.
I am not referring to the DOM value property. I referring to the value parameter of the function.

You say that minLength="1" should do nothing, so i updated my 2 examples. Now I am using ngMinlength="2" but the value still not bind...

@caitp
Copy link
Contributor

caitp commented Jan 27, 2014

No, the minLengthValidator is one of the parsers (it is not added to formatters). The value it is given is the result of the parsers which executed before it, however by default, the number parsers will be evaluated after the text parsers (including minLengthValidator), thus minLengthValidator's value should always be a string unless you intervene in some way

I think you misunderstand what I'm saying... The minLengthValidator (and maxLengthValidator) do not care what the model value is, it is not given the model value, it is given the text content of an input element or textarea. It will never receive a number, because the textInputType parsers/formatters are always added before the "special" input types. So it's always a string.

@vitorlopez
Copy link
Author

I updated my second exemple again (http://plnkr.co/edit/rZgEtjyDuErDro9K94Q3). Now i am using a custom version of the angular.js. The only change is that I put a 'console.log' in the file near the ngMinlength validation.

If you take a look at your console while runinng the example, you see that the value that shows the first time is a number, then the next time is a string.

So, not always the value is a string.....

@caitp
Copy link
Contributor

caitp commented Jan 27, 2014

Hmm, that's true, it is added to the formatters as well... And we don't have any tests verifying that it works correctly.

Okay, so I don't think this particular solution makes any sense at all, but FWIW I will write some failing tests and address this today. Unless you want to fix up the minlength/maxlength tests to get them testing the formatters as well

@vitorlopez
Copy link
Author

I agree with you about my solution, but I only write it to explain what i mean in the issue.

Hopefully now I have managed to explain better.

caitp added a commit to caitp/angular.js that referenced this pull request Jan 27, 2014
…ike values

A regression reported in angular#5936 shows that prior to cdc4d48, an
input with an ngMinlength or ngMaxlength validator would not invalidate values where value.length
was undefined.

This patch addresses this by making use of the Infinity global when a value's length property is
undefined.

The behaviour of this validator is still incorrect in the case of objects with a length property
which are not arrays or strings, and will most likely remain that way. This cannot change as it is
possibly desirable to use ngMinlength/ngMaxlength in conjunction with ngList.

Closes angular#5936
Closes angular#6000
caitp added a commit to caitp/angular.js that referenced this pull request Jan 27, 2014
…ike values

A regression reported in angular#5936 shows that prior to cdc4d48, an
input with an ngMinlength or ngMaxlength validator would not invalidate values where value.length
was undefined.

This patch addresses this by making use of the Infinity global when a value's length property is
undefined.

The behaviour of this validator is still incorrect in the case of objects with a length property
which are not arrays or strings, and will most likely remain that way. This cannot change as it is
possibly desirable to use ngMinlength/ngMaxlength in conjunction with ngList.

Closes angular#5936
Closes angular#6000
@vitorlopez
Copy link
Author

thanks for spend time to try understand my issue and for the solution.

caitp added a commit to caitp/angular.js that referenced this pull request Jan 27, 2014
…ike values

A regression reported in angular#5936 shows that prior to cdc4d48, an
input with an ngMinlength or ngMaxlength validator would not invalidate values where value.length
was undefined.

This patch addresses this by making use of the Infinity global when a value's length property is
undefined.

The behaviour of this validator is still incorrect in the case of objects with a length property
which are not arrays or strings, and will most likely remain that way. This cannot change as it is
possibly desirable to use ngMinlength/ngMaxlength in conjunction with ngList.

Closes angular#5936
Closes angular#6000
caitp added a commit to caitp/angular.js that referenced this pull request Mar 19, 2014
…ike values

A regression reported in angular#5936 shows that prior to cdc4d48, an
input with an ngMinlength or ngMaxlength validator would not invalidate values where value.length
was undefined.

The behaviour of this validator is still incorrect in the case of objects with a length property
which are not arrays or strings, and will most likely remain that way. This cannot change as it is
possibly desirable to use ngMinlength/ngMaxlength in conjunction with ngList.

Closes angular#5936
Closes angular#6000
@ashleygwilliams ashleygwilliams added this to the 1.3.0-beta.4 milestone Mar 27, 2014
@ashleygwilliams
Copy link

Adding this to milestone-1.3.0 beta.4 so that it follows @caitp's PRs. Thanks for putting the time and effort into working on this! :)

@tbosch
Copy link
Contributor

tbosch commented Mar 31, 2014

Will be solved with #6928

@tbosch tbosch modified the milestones: Backlog, 1.3.0-beta.5 Mar 31, 2014
@tan9
Copy link

tan9 commented Apr 21, 2014

Will this commit be back ported to 1.2.x, I believe there are many projects still cannot get ride of IE 8...

@caitp
Copy link
Contributor

caitp commented Sep 25, 2014

This is a bit bitrotten now --- however the behaviour should have been fixed by 77ce5b8

@caitp caitp closed this Sep 25, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants