Bug 48221

Created attachment 71718 [details] Patch

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.

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

Created attachment 71838 [details] Patch

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

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

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

Created attachment 71851 [details] Patch

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

Created attachment 71854 [details] Patch

Comment on attachment 71854 [details] Patch ok. Thank you for working on this!

Comment on attachment 71854 [details] Patch Clearing flags on attachment: 71854 Committed r70549: <http://trac.webkit.org/changeset/70549>

All reviewed patches have been landed. Closing bug.

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

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