Bug 48221 – Number values for form controls should be in the range of IEEE 754 single-precision floating point number

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Dai Mikurube]

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.

[Dai Mikurube]

Created attachment 71718 [details]
Patch

[Kent Tamura]

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.

[Kent Tamura]

(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.

[Dai Mikurube]

Created attachment 71838 [details]
Patch

[Kent Tamura]

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".

[Dai Mikurube]

(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.

[Kent Tamura]

(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.

[Dai Mikurube]

Created attachment 71851 [details]
Patch

[Dai Mikurube]

(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

[Kent Tamura]

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.

[Dai Mikurube]

Created attachment 71854 [details]
Patch

[Kent Tamura]

Comment on attachment 71854 [details]
Patch

ok.  Thank you for working on this!

[WebKit Commit Bot]

Comment on attachment 71854 [details]
Patch

Clearing flags on attachment: 71854

Committed r70549: <http://trac.webkit.org/changeset/70549>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Fraser (smfr)]

Does this just apply to form controls? If so, neither the changelog nor the commit message say so.

[Kent Tamura]

(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.