Bug 29069 – [HTML5][Forms] min/max attributes support for type=number

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Kent Tamura]

Implement min/max attributes and ValidityState.rangeUnderflow and .rangeOverflow for <input type=number>.

[Kent Tamura]

I'm working on this.

[Kent Tamura]

Created attachment 39319 [details]
Proposed patch

[Kent Tamura]

Created attachment 39858 [details]
Proposed patch (rev.2)

resources/ -> script-tests/

[Eric Seidel (no email)]

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.

[Kent Tamura]

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.

[Kent Tamura]

Created attachment 40601 [details]
Proposed patch (rev.4)

Resolved conflicts with today's WebKit.

[Darin Adler]

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

[Kent Tamura]

Created attachment 40602 [details]
Proposed patch (rev.5)

Renamed minRange() to rangeMinimum(), maxRange() to rangeMaximum().

[Darin Adler]

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.

[Kent Tamura]

Created attachment 40608 [details]
Proposed patch (rev.6)

Oops, updated a comment and ChangeLog for rangeMinimum()/rangeMaximum().

[WebKit Commit Bot]

Comment on attachment 40608 [details]
Proposed patch (rev.6)

Clearing flags on attachment: 40608

Committed r49104: <http://trac.webkit.org/changeset/49104>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Seidel (no email)]

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.

[Darin Adler]

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.

[Eric Seidel (no email)]

Brian got past that error by initializing doubleValue to 0.0, but then hit 17 more and gave up.  Hence the rollout.

[Eric Seidel (no email)]

Rolled out in http://trac.webkit.org/changeset/49107.

[Kent Tamura]

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.

[Kent Tamura]

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.

[WebKit Commit Bot]

Comment on attachment 40679 [details]
Proposed patch (rev.7)

Clearing flags on attachment: 40679

Committed r49199: <http://trac.webkit.org/changeset/49199>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.