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

ngMinlength and ngMaxlength attribute not bind value, when model value is type Number #5936

Closed
vitorlopez opened this issue Jan 22, 2014 · 7 comments

Comments

@vitorlopez
Copy link

cdc4d48#diff-c244afd8def7f268b16ee91a0341c4b2R514

I was reading the commit above, and i have a question...

In the previous code, if the 'value' was type Number, the '.length' property return 'undefined'. So instead of go in the 'if' condition, go in the 'else'.

But in this new condition 'ctrl.$isEmpty(value) || value.length', if the 'value' is a Number, then the condition will return false. Then the validation wont pass. But in the previous condition the same case pass correctly.

My question is: This behavior was intentional? Because i understand if i want a field with a number value i can use input type="number" and then use the 'min' and 'max' attributes. But the layout that i get for this type of input is different in each browser, and some times doesnt fit in the design of the app.

If i am right, i can submit a pull request do help with this, but first i have to be sure that this behavior wasnt intentional.

This example uses Angular 1.2.7 and will success the validation:
http://plnkr.co/edit/YRpqT6Dwc6xbe2LYHEwq

This example uses Angular 1.2.8 and will fail the validation: http://plnkr.co/edit/rZgEtjyDuErDro9K94Q3

The same will apply for 'maxlength' attribute.

@caitp
Copy link
Contributor

caitp commented Jan 23, 2014

This is a dupe of a number of issues/pull requests like #1588... I'm going to mark this for 1.3 but I can't guarantee that this will be implemented

@vitorlopez
Copy link
Author

I don't believe that's the same case. In my exemple the the new expression return a different value than the previous condition.

I think this is a honest mistake, because if you look this condition (that is the current):

ctrl.$isEmpty(value) || value.length <= maxlength

is the exactly opposite of this (that is the legacy)

!ctrl.$isEmpty(value) && value.length < minlength

But before it was in a if else statement. Now is simply returning itself.
This causes the bug that i mentioned.

I think that's not what the other issue you mentioned means.

I don't want the parameter takes expressions. I want the input value can be type Number and still bind correctly.
If you look at the 2 examples that i put at Plunker, you see this behavior.
My first try would be change the current expression to:

var oldExp = !ctrl.$isEmpty(value) && value.length < minlength;
return validate(ctrl, 'minlength', !oldExp, value);

This works, but is ugly. But explains my point.

@caitp
Copy link
Contributor

caitp commented Jan 23, 2014

Ok, thanks for the clarification

caitp added a commit to caitp/angular.js that referenced this issue 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 issue 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 issue 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 issue 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
@caitp
Copy link
Contributor

caitp commented Sep 25, 2014

This has been fixed in the master branch, but it looks like stable branch still has issues. I'll see if this can be fixed in stable

@caitp
Copy link
Contributor

caitp commented Sep 25, 2014

So this does work on stable (I'm not sure which commit fixed it), but there aren't really any test cases or anything.

What doesn't work is the attribute is not observed the way it is in the master branch, so the validation only happens during parsing/formatting, not just when the attribute value changes.

I think I'm not going to push extra tests because I don't want stable to diverge unnecessarily from master, but yeah this works

@caitp
Copy link
Contributor

caitp commented Sep 25, 2014

hmm... but the plunker doesn't do what you expect in v1.2.25... sigh :( looking again

@caitp caitp reopened this Sep 25, 2014
caitp added a commit to caitp/angular.js that referenced this issue Sep 25, 2014
…tters

Previously, minlength/maxlength would not work correctly when dealing with non-string model values.

A better fix for this has already been implemented in 1.3, but unfortunately implementing that fix
in 1.2 is a bit harder as it requires a number of commits to be backported in order to solve the
problem correctly.

Closes angular#5936
caitp added a commit to caitp/angular.js that referenced this issue Sep 25, 2014
…rols

Backported from 1eda183

NgModel will format all scope-based values to string when setting the viewValue for
the associated input element. The formatting, however, only applies to input elements
that contain a text, email, url or blank input type. In the event of a null or undefined
scope or model value, the viewValue will be set to null or undefined instead of being
converted to an empty string.

Closes angular#5936
caitp added a commit that referenced this issue Sep 25, 2014
…rols

Backported from 1eda183

NgModel will format all scope-based values to string when setting the viewValue for
the associated input element. The formatting, however, only applies to input elements
that contain a text, email, url or blank input type. In the event of a null or undefined
scope or model value, the viewValue will be set to null or undefined instead of being
converted to an empty string.

Closes #5936
Closes #9277
@caitp
Copy link
Contributor

caitp commented Sep 25, 2014

Closed by 1b8b41a

@caitp caitp closed this as completed 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 a pull request may close this issue.

2 participants