Bug 29069

Summary: [HTML5][Forms] min/max attributes support for type=number
Product: WebKit Reporter: Kent Tamura <tkent>
Component: FormsAssignee: Kent Tamura <tkent>
Status: RESOLVED FIXED    
Severity: Normal CC: adele, commit-queue, darin, eric, hyatt, michelangelo
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
URL: http://www.whatwg.org/specs/web-apps/current-work/multipage/forms.html#number-state
Bug Depends on:    
Bug Blocks: 27450, 27451, 27968, 29071    
Attachments:
Description Flags
Proposed patch
none
Proposed patch (rev.2)
eric: review-
Proposed patch (rev.3)
none
Proposed patch (rev.4)
darin: review+
Proposed patch (rev.5)
darin: review+
Proposed patch (rev.6)
none
Proposed patch (rev.7) none

Description Kent Tamura 2009-09-08 23:04:24 PDT
Implement min/max attributes and ValidityState.rangeUnderflow and .rangeOverflow for <input type=number>.
Comment 1 Kent Tamura 2009-09-08 23:06:06 PDT
I'm working on this.
Comment 2 Kent Tamura 2009-09-09 20:01:04 PDT
Created attachment 39319 [details]
Proposed patch
Comment 3 Kent Tamura 2009-09-21 10:03:19 PDT
Created attachment 39858 [details]
Proposed patch (rev.2)

resources/ -> script-tests/
Comment 4 Eric Seidel (no email) 2009-09-23 10:54:04 PDT
Comment on attachment 39858 [details]
Proposed patch (rev.2)

Reversed, no?
 53     double maximum;  // maximum must be <= minimum.

Tab, will cause commit to fail:
 13 	testPassed(resultText);

I would have probably written a little static inline function to help grab the numbers, but I think my "make a function" threshold is lower than most others:
         double min;
 202         double doubleValue;
 203         if (hasAttribute(minAttr) && formStringToDouble(getAttribute(minAttr).string(), &min) && formStringToDouble(value(), &doubleValue))
 204             return doubleValue < min;

This comment mostly confuses me:
 249                 max = min;  // The spec allows min == max.
I'm not surprised that min is allowed to equal max.

Why not write that whole if as:

max = max(min, max(100, max))
obviously you'd have to use std:: or change the name of max.

r- for the nits.  Really only the backwards comment, the rest doesn't need to be changed.  If you were a committer, I would r+ this and you could commit it with mods yourself.
Comment 5 Kent Tamura 2009-09-23 17:38:38 PDT
Created attachment 40033 [details]
Proposed patch (rev.3)

> Reversed, no?
>  53     double maximum;  // maximum must be <= minimum.

Right!  Fixed.

> Tab, will cause commit to fail:
>  13     testPassed(resultText);

Fixed 8 instances in 4 files.

> I would have probably written a little static inline function to help grab the
> numbers, but I think my "make a function" threshold is lower than most others:
>          double min;
>  202         double doubleValue;
>  203         if (hasAttribute(minAttr) && formStringToDouble(getAttribute(minAttr).string(), &min) && formStringToDouble(value(), &doubleValue))
>  204             return doubleValue < min;

I didn't make a such function, and simplified this code by removing hasAttribute() and .string() instead.  formStringToDouble() returns false if the input string is null.

> This comment mostly confuses me:
>  249                 max = min;  // The spec allows min == max.
> I'm not surprised that min is allowed to equal max.

I improved the code and comments here.
Comment 6 Kent Tamura 2009-10-04 19:08:12 PDT
Created attachment 40601 [details]
Proposed patch (rev.4)

Resolved conflicts with today's WebKit.
Comment 7 Darin Adler 2009-10-04 19:27:14 PDT
Comment on attachment 40601 [details]
Proposed patch (rev.4)

> +    // Returns the minimum value for type=range.  Don't call this for other types.
> +    double minRange() const;

I think this should be named rangeMinimum(), not minRange(), which sounds like "minimum range" to me.

> +    // Returns the maximum value for type=range.  Don't call this for other types.
> +    // This always returns a value which is <= minRange().
> +    double maxRange() const;

And rangeMaximum() for this.

Otherwise seems fine to me, r=me
Comment 8 Kent Tamura 2009-10-04 19:36:35 PDT
Created attachment 40602 [details]
Proposed patch (rev.5)

Renamed minRange() to rangeMinimum(), maxRange() to rangeMaximum().
Comment 9 Darin Adler 2009-10-04 20:18:50 PDT
Comment on attachment 40602 [details]
Proposed patch (rev.5)

> +        (WebCore::HTMLInputElement::minRange):
> +        (WebCore::HTMLInputElement::maxRange):

Needs new name here.

> +    // This always returns a value which is <= minRange().

And here.

Still fine as-is, but better to get it right.
Comment 10 Kent Tamura 2009-10-04 21:24:41 PDT
Created attachment 40608 [details]
Proposed patch (rev.6)

Oops, updated a comment and ChangeLog for rangeMinimum()/rangeMaximum().
Comment 11 WebKit Commit Bot 2009-10-05 11:14:57 PDT
Comment on attachment 40608 [details]
Proposed patch (rev.6)

Clearing flags on attachment: 40608

Committed r49104: <http://trac.webkit.org/changeset/49104>
Comment 12 WebKit Commit Bot 2009-10-05 11:15:01 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Eric Seidel (no email) 2009-10-05 12:00:34 PDT
This broke the windows bots:
http://build.webkit.org/builders/Windows%20Debug%20%28Build%29/builds/5913
so I've asked the commit-bot to roll out this patch.  Should be rolled out in 10 minutes or so.
Comment 14 Darin Adler 2009-10-05 12:02:07 PDT
Looks like the build failure was due to MSVC not being smart enough about uninitialized variables and code flow. Should be easy to work around.
Comment 15 Eric Seidel (no email) 2009-10-05 12:05:26 PDT
Brian got past that error by initializing doubleValue to 0.0, but then hit 17 more and gave up.  Hence the rollout.
Comment 16 Eric Seidel (no email) 2009-10-05 12:11:43 PDT
Rolled out in http://trac.webkit.org/changeset/49107.
Comment 17 Kent Tamura 2009-10-05 19:43:17 PDT
Created attachment 40679 [details]
Proposed patch (rev.7)

Fixed the build problems on Windows.
 - Initialize local variables with 0.0
 - Not define max/min for COM to avoid conflict in WebKit.h
   I think to add "#define NOMINMAX" to WebKit.h is the right fix.  But I couldn't find who generates WebKit.h.
Comment 18 Kent Tamura 2009-10-05 22:40:53 PDT
I found WebKit.h was generated by MIDL.  It's hard to insert "#define NO_MINMAX" to it.  I think WebKit.h is a public header, and we should not add "#define NO_MINMAX" to it and should not request "define NO_MINMAX before including WebKit.h" because it would break source-code compatibility.

So, I think the patch (rev.7) is the best approach.
Comment 19 WebKit Commit Bot 2009-10-06 11:47:29 PDT
Comment on attachment 40679 [details]
Proposed patch (rev.7)

Clearing flags on attachment: 40679

Committed r49199: <http://trac.webkit.org/changeset/49199>
Comment 20 WebKit Commit Bot 2009-10-06 11:47:34 PDT
All reviewed patches have been landed.  Closing bug.