Bug 48221 - Number values for form controls should be in the range of IEEE 754 single-precision floating point number
Summary: Number values for form controls should be in the range of IEEE 754 single-pre...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 48220
  Show dependency treegraph
 
Reported: 2010-10-25 00:07 PDT by Dai Mikurube
Modified: 2010-10-26 17:20 PDT (History)
3 users (show)

See Also:


Attachments
Patch (30.25 KB, patch)
2010-10-25 00:33 PDT, Dai Mikurube
no flags Details | Formatted Diff | Diff
Patch (32.33 KB, patch)
2010-10-25 20:02 PDT, Dai Mikurube
no flags Details | Formatted Diff | Diff
Patch (31.88 KB, patch)
2010-10-26 02:07 PDT, Dai Mikurube
no flags Details | Formatted Diff | Diff
Patch (32.62 KB, patch)
2010-10-26 02:40 PDT, Dai Mikurube
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dai Mikurube 2010-10-25 00:07:00 PDT
Number values in HTML should be in the range of IEEE 754 single-precision floating point number.
-// http://www.whatwg.org/specs/web-apps/current-work/multipage/common-microsyntaxes.html#real-numbers

Actually:
WebKit accepts numbers in the range of IEEE 754 double-precision floating point numbers.
LayoutTests also include tests accepting huge numbers out of the range of IEEE 754 single-precision numbers.
Comment 1 Dai Mikurube 2010-10-25 00:33:56 PDT
Created attachment 71718 [details]
Patch
Comment 2 Kent Tamura 2010-10-25 01:48:25 PDT
Comment on attachment 71718 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=71718&action=review

> LayoutTests/ChangeLog:9
> +

Our standard ChangeLog entry format is:

Reviewed By ...

<Single-line change description, typically it's a title of the corresponding bug>
<bug URL>

<More description>

* <file>: <comments for the file>

We use identical single-line description in LayoutTests/ChangeLog and WebCore/ChangeLog.
The short description in your ChangeLog should be moved after the bug URL.

> WebCore/html/parser/HTMLParserIdioms.cpp:86
> +    // Numbers are considered finite IEEE 754 single-precision floating point values.

I think we need to update serializeForNumberType() in HTMLParserIdioms.cpp too so that it doesn't show digits out of the single-precision.
Comment 3 Kent Tamura 2010-10-25 02:35:21 PDT
(In reply to comment #2)

NumberInputType::setValueAsNumber() needs range check.
For example,
 input.type = 'number';
 input.valueAsNumber = 1.7976931348623156e+308;
should throw an exception, probably INVALID_STATE_ERR, though HTML5 doesn't specify such case.
Comment 4 Dai Mikurube 2010-10-25 20:02:34 PDT
Created attachment 71838 [details]
Patch
Comment 5 Kent Tamura 2010-10-25 20:24:34 PDT
Comment on attachment 71838 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=71838&action=review

> WebCore/html/parser/HTMLParserIdioms.cpp:66
> +        number = -HUGE_VAL;

I don't think printing +-HUGE_VAL makes much sense, and huge values which are out of the float range should not be passed to this function if other code correctly handle minimum/maximum limits.

My concern is precision, not value range. Suppose that we have
  <input type=number step=0.005 min=4 value=5.005 >
and we call stepUp(2).  The value will be "5.015000000000001" because serializeForNumberType() handles lower bits of the double value.  The value should be "5.015".
Comment 6 Dai Mikurube 2010-10-25 22:58:27 PDT
(In reply to comment #5)
I'm investigating the serialization. Fixing it seems to be complicated and may have large effects. (Because of (*1))

My idea here is to file another bug for it. Such serializations (like 5.015000000000001) is not caused by this patch. The current version without this patch shows the same serialization, too. What do you think on it?



(*1)
I found few ways to limit the precision in converting double to string.
- No extra parameters for the precision in numberToString (JavaScriptCore/wtf/dtoa.cpp).
- numberToString uses DecimalNumber (.../wtf/DecimalNumber.h) which doesn't accept a parameter for precisions.
- DecimalNumber calls dtoa (.../wtf/dtoa.cpp) which decides the precision only with the double value.

My idea of changing is :
- to add another overloaded numberToString with specifying the precision, and
- to add another constructor of DecimalNumber with specifying the precision
But it should be a kind of large patch and may have large effects to the JavaScript core. Another nice idea is, of course, welcome. But I think it should be done in another bug.



In addition, another difficulty from the specification.

- Double values should be serialized with the same way as JavaScript ToString. ("The best representation of the number n as a floating point number is the string obtained from applying the JavaScript operator ToString to n." [http://www.whatwg.org/specs/web-apps/current-work/multipage/common-microsyntaxes.html#real-numbers]

- The floating point number in JavaScript is in IEEE 754 double-precision.
Comment 7 Kent Tamura 2010-10-25 23:06:29 PDT
(In reply to comment #6)
> (In reply to comment #5)
> I'm investigating the serialization. Fixing it seems to be complicated and may have large effects. (Because of (*1))
> 
> My idea here is to file another bug for it. Such serializations (like 5.015000000000001) is not caused by this patch. The current version without this patch shows the same serialization, too. What do you think on it?

Ok, it sounds reasonable.
So please remove the change to searizlieForNumberType() from the last patch and upload it.
Comment 8 Dai Mikurube 2010-10-26 02:07:02 PDT
Created attachment 71851 [details]
Patch
Comment 9 Dai Mikurube 2010-10-26 02:08:59 PDT
(In reply to comment #7)

Ok, I've uploaded a new patch. And filed a new bug :
https://bugs.webkit.org/show_bug.cgi?id=48308
Comment 10 Kent Tamura 2010-10-26 02:16:13 PDT
Comment on attachment 71851 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=71851&action=review

> WebCore/html/NumberInputType.cpp:69
> +    if (newValue < numberDefaultMinimum) {

You need to add test cases for this change into LayoutTests/fast/forms/script-tests/input-valueasnumber-number.js.

> WebCore/html/NumberInputType.cpp:73
> +    if (newValue > numberDefaultMaximum) {

ditto.
Comment 11 Dai Mikurube 2010-10-26 02:40:07 PDT
Created attachment 71854 [details]
Patch
Comment 12 Kent Tamura 2010-10-26 02:42:37 PDT
Comment on attachment 71854 [details]
Patch

ok.  Thank you for working on this!
Comment 13 WebKit Commit Bot 2010-10-26 11:48:01 PDT
Comment on attachment 71854 [details]
Patch

Clearing flags on attachment: 71854

Committed r70549: <http://trac.webkit.org/changeset/70549>
Comment 14 WebKit Commit Bot 2010-10-26 11:48:07 PDT
All reviewed patches have been landed.  Closing bug.
Comment 15 Simon Fraser (smfr) 2010-10-26 11:53:48 PDT
Does this just apply to form controls? If so, neither the changelog nor the commit message say so.
Comment 16 Kent Tamura 2010-10-26 17:20:44 PDT
(In reply to comment #15)
> Does this just apply to form controls? If so, neither the changelog nor the commit message say so.

Right.  I'll update the ChangeLog entry.