Bug 28934

Summary: Add <input type=number> validation support
Product: WebKit Reporter: Kent Tamura <tkent>
Component: FormsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: adele, darin, eric, hyatt, ian, michelangelo, pkasting
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: 19264, 27450, 27451    
Attachments:
Description Flags
Proposed patch
eric: review-
Proposed patch (rev.2)
none
Proposed patch (rev.3)
none
Proposed patch (rev.4) none

Comment 1 Kent Tamura 2009-09-03 00:17:08 PDT
Created attachment 38969 [details]
Proposed patch

stringToFloat() is in HTMLInputElement.cpp because it will be used to parse min= max= step= attributes.
Comment 2 Eric Seidel (no email) 2009-09-03 01:21:51 PDT
Comment on attachment 38969 [details]
Proposed patch

It seems stringToFloat should have HTML5 in teh name.  It's not a generic function, but rather a specific html5-compatible implementation.

html5CompatStringToFloat
html5SpecStingToFloat
?

This would be better as a single if unless there is a grand plan to add a bunch more types...
+    switch (input->inputType()) {
+    case HTMLInputElement::NUMBER:
+        return !HTMLInputElement::stringToFloat(value, 0);
+    default:
+        return false;
+    }

if (input->type() != HTMLInputElement::NUMBER)
    return false;
// Check to make sure the number is valid, ignore the parsed number.
return !HTMLInputElement::stringToFloat(value, 0);

Otherwise looks fine.
Comment 3 Kent Tamura 2009-09-03 02:08:26 PDT
Created attachment 38976 [details]
Proposed patch (rev.2)

- Followed Eric's comments
Comment 4 Peter Kasting 2009-09-03 11:19:09 PDT
I disagree with Eric.  Putting "HTML5" in function names is completely inappropriate given that this functionality will also be required for HTML 6, 7, ... and in fact HTML is switching to be unversioned (see e.g. "DOCTYPE HTML" and some of Hixie's comments).  These functions should be called things like "userInputToDouble" or "inputStringToDouble" or something to indicate that the difference between them and a generic conversion to double is the fact that the origin is some kind of user input which needs additional validation or conversion.

Also, nitpick: "numeber" -> "number" in at least one place.
Comment 5 Darin Adler 2009-09-03 12:59:36 PDT
(In reply to comment #4)
> These functions should be called things like
> "userInputToDouble" or "inputStringToDouble" or something to indicate that the
> difference between them and a generic conversion to double is the fact that the
> origin is some kind of user input which needs additional validation or
> conversion.

That sounds right to me too.
Comment 6 Ian 'Hixie' Hickson 2009-09-03 15:50:18 PDT
Putting "html5" in method or file names will just mean that a few years from now someone will have to go and rename the methods or files, just like we did recently with html4.css.
Comment 7 Kent Tamura 2009-09-03 19:06:56 PDT
Created attachment 39031 [details]
Proposed patch (rev.3)

Thank you for the comments.

- toDoubleForHtml5() -> formStringToDouble()
 I didn't use "input" "user" because this method will be used for min= max= step= attributes.

- Fix a typo
Comment 8 Kent Tamura 2009-09-04 02:18:51 PDT
Created attachment 39046 [details]
Proposed patch (rev.4)

- Resolve conflict with type=color.
Comment 9 Eric Seidel (no email) 2009-09-08 09:49:53 PDT
Comment on attachment 39046 [details]
Proposed patch (rev.4)

LGTM.
Comment 10 Eric Seidel (no email) 2009-09-08 10:00:55 PDT
Comment on attachment 39046 [details]
Proposed patch (rev.4)

Rejecting patch 39046 from commit-queue.  This patch will require manual commit.

['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1'] failed with exit code 1
Comment 11 Eric Seidel (no email) 2009-09-08 10:02:48 PDT
Comment on attachment 39046 [details]
Proposed patch (rev.4)

Bitten by bug 29037.
Comment 12 Eric Seidel (no email) 2009-09-08 10:23:15 PDT
Comment on attachment 39046 [details]
Proposed patch (rev.4)

Clearing flags on attachment: 39046

Committed r48163: <http://trac.webkit.org/changeset/48163>
Comment 13 Eric Seidel (no email) 2009-09-08 10:23:20 PDT
All reviewed patches have been landed.  Closing bug.