Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

rebased Fix #639/#648 min/max validation #666

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
8 participants
Contributor

ekonijn commented Feb 28, 2013

As promissed, here's the min/max commit, rebased to upstream head,
with the two commits squashed into one.

In 1.10.0, min/max validation was supported for input type="text",
where min/max were interpreted as numbers. This means min/max
for date would not work: min="2012-02-13" was interpreted as min="Not a Number".

In 1.11.0, min/max were no longer converted to numbers. This means
min/max for dates worked, but min/max for numbers failed:
"50" < "150" < "1000" does not hold.

For an example, see http://jsbin.com/awokex/3

This commit makes the behaviour of min/max dependent on input type:

  • input type=text has numeric min/max, as in 1.10.0
  • input type=date has working min/max for type date;
    on mobile browsers you also get a date picker,
    plus the browser may reject invalid dates before
    javascript gets a chance to complain.
  • input type=number or range get numeric min/max,
    plus numeric keypad or slider on mobile browsers,
    plus browser may reject invalid input before javascript
    gets a chance to complain

Allowing use of min/max with type=number/range/date is important
for mobile browsers, where the numeric keypad or date picker
make the input much easier to use than a generic text input field.
In this situation jquery-validate remains necessary to support
older browsers that do not do input validation based on type
and min/max.

For situations where numeric input should be validated by jquery
without giving the browser a chance to validate the input format,
input type=text in combination with min/max can be used, as in 1.10.0.

Note that some older browsers do not correctly support Date conversion,
which can cause the validation suite to fail with message "Please enter a valid date."

In particular, this bug hit phantomjs 1.7.0 as used in validating 1.11.0;
the issue is solved in phantomjs 1.8.0 used now.

http://code.google.com/p/phantomjs/issues/detail?id=187#c6
ariya/phantomjs@35c1971

Tested with chromium, firefox, grunt.

ekonijn added some commits Feb 23, 2013

Fix #639/#648 min/max validation
In 1.10.0, min/max validation was supported for input type="text",
where min/max were interpreted as numbers.  This means min/max
for date would not work: min="2012-02-13" was interpreted as min="Not a Number".

In 1.11.0, min/max were no longer converted to numbers.  This means
min/max for dates worked, but min/max for numbers failed:
"50" < "150" < "1000" does not hold.

For an example, see http://jsbin.com/awokex/3

This commit makes the behaviour of min/max dependent on input type:

 * input type=text has numeric min/max, as in 1.10.0
 * input type=date has working min/max for type date;
   on mobile browsers you also get a date picker,
   plus the browser may reject invalid dates before
   javascript gets a chance to complain.
 * input type=number or range get numeric min/max,
   plus numeric keypad or slider on mobile browsers,
   plus browser may reject invalid input before javascript
   gets a chance to complain

Allowing use of min/max with type=number/range/date is important
for mobile browsers, where the numeric keypad or date picker
make the input much easier to use than a generic text input field.
In this situation jquery-validate remains necessary to support
older browsers that do not do input validation based on type
and min/max.

For situations where numeric input should be validated by jquery
without giving the browser a chance to validate the input format,
input type=text in combination with min/max can be used, as in 1.10.0.

Note that some older browsers do not correctly support Date conversion,
which can cause the validation suite to fail with message "Please enter a valid date."

In particular, this bug hit phantomjs 1.7.0 as used in validating 1.11.0;
the issue is solved in phantomjs 1.8.0 used now.

http://code.google.com/p/phantomjs/issues/detail?id=187#c6
ariya/phantomjs@35c1971

Tested with chromium, firefox, grunt.
Fix min/max for input without type attribute
As noted in #660 (comment),
input fields without type did not treat min/max numerically.
This happens in jqueryui forms.

For a demonstration see http://jsbin.com/uxigir/1
(As an aside, that demo also shows that I don't know how to make
error messages show up in the right place, but that's unrelated
to this issue)

This commit treats input fields without type like input fields
with type=text.

Adapted test cases accordingly; tested in chromium.

There is still the same problem with min/max validation for data-val-range-max and data-val-range-min attribute.

Contributor

ekonijn commented Mar 12, 2013

Had a look: if I understand #626 correctly, data-val-range-min/max are part of a Microsoft provided jquery.validate.unobtrusive.js, with the attributes generated by ASP.NET. I don't have access to that, so can't work on this issue.

Please fix this.

Thanks for the hard work in solving this comprehensively!

Collaborator

jzaefferer commented Mar 20, 2013

Thanks so much, especially all the additional tests! I just landed this, though with a few changes. I considerably rewrote the actual patch to be a lot smaller, while adding two line comments. I also squashed the two commits into one, removing some of the notes about compability with phantomjs. Its working fine with the current grunt qunit plugin.

martonl commented Mar 20, 2013

Thanks, I will give it a try.

Contributor

jcbowman commented Mar 21, 2013

This issue still remains in ASP .NET MVC.

I pulled down the version you released yesterday, and it is still doing a string compare on the range field.

Here is an example of my element:
Capture

Collaborator

jzaefferer commented Mar 21, 2013

What do you mean, "released"? Did you get the code from GitHub? I didn't yet release anything.

Contributor

jcbowman commented Mar 21, 2013

Yes I got the update from yesterday on GitHub.

Collaborator

jzaefferer commented Mar 21, 2013

Can you tell me what your asp.net wrapper actually outputs? The markup you have isn't so very useful, only the result that the plugin gets to see.

Contributor

jcbowman commented Mar 21, 2013

Sorry for the confusion, but I believe the markup that I provided is the output of the asp.net wrapper. What is there is exactly what the plugin will get to see.

Collaborator

jzaefferer commented Mar 21, 2013

Those data-val- attributes have to meaning for the plugin. It looks for data-rule- and data-msg-. Can you please file a separate issue with more details to reproduce?

Contributor

jcbowman commented Mar 21, 2013

I absolutely can, but I think there is a similar issue discussed in Issue #626. I was posting on this page because you mentioned yesterday that you thought this fix would cover the issue. Let me know if you still need me to post more information.

I assume I have to wait for these changes to be committed first before I can get at them and test. Cant work out which branch I would need to use.... as I tested this the other day with the latest... dev builds and this was not working. So I want to ensure that I am testing with the correct build. Could you point me to the build which will have these changes which are meant to fix the above, and I will test it for ya ;-) .

I just tried that version now and it does not fix it.

here is the mark up.

Select value 4 or anything above value 2

<select 
data-val="true" data-val-number="The field Activity must be a number." 
data-val-range="You must select an option." 
data-val-range-max="2147483647" 
data-val-range-min="1" 
data-val-required="The Activity field is required." 
id="TimeKeepingActivityID" 
name="TimeKeepingActivityID" 
style="width:212px">

<option selected="selected" value="0">Please Select</option>
<option value="1">Development</option>
<option value="2">Testing</option>
<option value="3">Training</option>
<option value="4">Admin</option>
<option value="5">Spare Capacity</option>
<option value="6">Compliance Development</option>
</select>

here is the MVC information

[Required]
[Range(1, 120, ErrorMessage = "You must select an option.")]
[Display(Name = "Client")]
public int TimeKeepingClientID { get; set; }

Hope this helps...

Collaborator

nschonni commented Mar 21, 2013

Fixed markup above, see Fenced code blocks

thanks still took me a while to find.. updated.

Contributor

jcbowman commented Mar 22, 2013

I apologize I believe I misspoke earlier today. It looks like the actual rule being added for the ASP .NET MVC [Range()] attribute is just 'range'. I think it is a fairly small fix. I will issue a pull request.

Capture

ruado1987 added a commit to ruado1987/jquery-validation that referenced this pull request Mar 22, 2013

Fix min/max validation. Closes gh-666. Fixes #648
In 1.10.0, min/max validation was supported for input type="text",
where min/max were interpreted as numbers.  This means min/max
for date would not work: min="2012-02-13" was interpreted as min="Not a Number".

In 1.11.0, min/max were no longer converted to numbers.  This means
min/max for dates worked, but min/max for numbers failed:
"50" < "150" < "1000" does not hold.

For an example, see http://jsbin.com/awokex/3

This commit makes the behaviour of min/max dependent on input type:

 * input type=text (or not type attribute) has numeric min/max, as in 1.10.0
 * input type=date has working min/max for type date;
   on mobile browsers you also get a date picker,
   plus the browser may reject invalid dates before
   javascript gets a chance to complain.
 * input type=number or range get numeric min/max,
   plus numeric keypad or slider on mobile browsers,
   plus browser may reject invalid input before javascript
   gets a chance to complain

Allowing use of min/max with type=number/range/date is important
for mobile browsers, where the numeric keypad or date picker
make the input much easier to use than a generic text input field.
In this situation jquery-validate remains necessary to support
older browsers that do not do input validation based on type
and min/max.

For situations where numeric input should be validated by jquery
without giving the browser a chance to validate the input format,
input type=text in combination with min/max can be used, as in 1.10.0.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment