Bug 28934 - Add <input type=number> validation support
Summary: Add <input type=number> validation support
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL: http://www.whatwg.org/specs/web-apps/...
Keywords:
Depends on:
Blocks: HTML5Forms 27450 27451
  Show dependency treegraph
 
Reported: 2009-09-02 22:42 PDT by Kent Tamura
Modified: 2009-09-08 10:23 PDT (History)
7 users (show)

See Also:


Attachments
Proposed patch (9.17 KB, patch)
2009-09-03 00:17 PDT, Kent Tamura
eric: review-
Details | Formatted Diff | Diff
Proposed patch (rev.2) (9.21 KB, patch)
2009-09-03 02:08 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff
Proposed patch (rev.3) (9.20 KB, patch)
2009-09-03 19:06 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff
Proposed patch (rev.4) (8.18 KB, patch)
2009-09-04 02:18 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.