Bug 48308 – Too precise serialization from floating point number to string for "number" input elements

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Dai Mikurube]

Steps to reproduce:
1. use following input <input type="number" step="0.005" min="4" value="5.005" />
2. stepUp(2) (by clicking the upper button or in another way)
3. check the value

Expected:
5.015

Actually happens:
5.015000000000001

[Simon Fraser (smfr)]

Testcase?

[Alexey Proskuryakov]

Added the same test case to bug URL.

[Dai Mikurube]

Created attachment 72189 [details]
Patch

[Dai Mikurube]

Thanks, Alwxey.

The attached Patch includes the test case.

[Simon Fraser (smfr)]

Comment on attachment 72189 [details]
Patch

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

> WebCore/html/parser/HTMLParserIdioms.cpp:70
>      NumberToStringBuffer buffer;
> -    unsigned length = numberToString(number, buffer);
> +    // Round the result with 9 significant digits.
> +    // 9 because of the significant digits of IEEE 754 single-precision numbers.
> +    // They have 23 fraction bits, then 2^24 = 16,677,216.
> +    // +1 to be safe in case of re-parsing required.
> +    // Another reason for 9 is to follow the example in HTML5 4.10.7.2.10.
> +    // <input name=opacity type=range min=0 max=1 step=0.00392156863>
> +    unsigned length = numberToString(number, buffer, 9);

Isn't there a potential buffer overflow here (in the old code, as well as yours)? See
http://trac.webkit.org/changeset/70198

[Simon Fraser (smfr)]

(In reply to comment #5)
> (From update of attachment 72189 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=72189&action=review
> 
> > WebCore/html/parser/HTMLParserIdioms.cpp:70
> >      NumberToStringBuffer buffer;
> > -    unsigned length = numberToString(number, buffer);
> > +    // Round the result with 9 significant digits.
> > +    // 9 because of the significant digits of IEEE 754 single-precision numbers.
> > +    // They have 23 fraction bits, then 2^24 = 16,677,216.
> > +    // +1 to be safe in case of re-parsing required.
> > +    // Another reason for 9 is to follow the example in HTML5 4.10.7.2.10.
> > +    // <input name=opacity type=range min=0 max=1 step=0.00392156863>
> > +    unsigned length = numberToString(number, buffer, 9);
> 
> Isn't there a potential buffer overflow here (in the old code, as well as yours)? See
> http://trac.webkit.org/changeset/70198

Never mind; numberToString() is OK.

[Kent Tamura]

Comment on attachment 72189 [details]
Patch

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

> JavaScriptCore/ChangeLog:5
> +        Too precise serialization from floating point number to string

Please change the summary as Simon did for the bug entry.
We should mention that this affects only to input element.

> JavaScriptCore/JavaScriptCore.exp:392
> +__ZN3WTF14numberToStringEdPtj

We need similar change to JavaScriptCore/JavaScriptCore.vcproj/JavaScriptCore/JavaScriptCore.def. The mangled symbol name in VC++ is different from gcc's.

> LayoutTests/fast/forms/script-tests/input-valueasnumber-number.js:24
> +shouldBe('valueAsNumberFor("123456789012345678901234567890123456789")', '1.23456789012345678E+38');

Why the result is changed? This test doesn't use serializeForNumberType().

> WebCore/html/HTMLInputElement.cpp:302
> +    double acceptableError = step / pow(2.0, FLT_MANT_DIG);

This is specific to type=number.  Other types such as type=date may have different acceptableError value.
Please introduce InputType::acceptableError(step) and override it in NumberInputType.  You can merge the same expression in NumberImputType::stepMismatch().

> WebCore/html/HTMLInputElement.cpp:303
> +    if (newValue - m_inputType->minimum() < -acceptableError) {

Is this change needed to pass existing tests?  If not, we need new test cases.

> WebCore/html/HTMLInputElement.cpp:309
> +    if (newValue - m_inputType->maximum() > acceptableError) {

ditto.

[Gavin Barraclough]

Comment on attachment 72189 [details]
Patch

Since you seem to print precision within the single precision range, in order to achieve the same effect could you not just round the value to float before passing it to numberToString()? (or maybe better still, cast to float prior to passing the value to serializeForNumberType – that way serializeForNumberType can continue to provide JavaScript ToString conversion, as defined by the spec).  This would seem a much simpler solution and smaller change, if it works.

If you do want to change serializeForNumberType I think you need to change the comments on the function in the .cpp and the .h to make it clear this method no longer provides conversion compliant with HTML5 (as per section 2.5.4.3) – and please don't change numberToString, this is a reasonably hot function, so if you do require a different behavior please just use DecimalNumber directly from HTMLParserIdioms.

thanks,
G.

[Dai Mikurube]

(In reply to comment #8)
Thanks for your comments, Gavin.

Unfortunately, casting to float and printing with numberToString() don't work. The original numberToString() try printing numbers with 16-17 digits. Then, for example,
> setValueAsNumberAndGetValue(-1.2)
is printed as
> -1.2000000476837158
with casting to float.

To solve the first problem (like 5.015000000000001), we have to
    1. limit the printed digits in the HTML context, or
    2. make stepUp() and stepDown() more and more precise

But [1.] is not compliant with HTML5 2.5.4.3 :
> The best representation of the number n as a floating point number is the string obtained from applying the JavaScript operator ToString to n.
Yes, it is a difficult problem, I think. The difficulty comes from
    A. single-precision in HTML5, but double-precision in JavaScript, and
    B. numeric errors from computing stepUp and stepDown with floating point numbers.
A and B looks like incompatible with HTML5 2.5.4.3. [2.] is, of course, naturally challenging.

What do you think about adding another numberToString with a different signature without rewriting the original numberToString? It's to avoid rewriting the original numberToString, but also to make it easy to keep following with future changes on the original numberToString. My first idea here was to print with limited digits by the same logic as JavaScript's ToString.

[Kent Tamura]

(In reply to comment #9)
> Unfortunately, casting to float and printing with numberToString() don't work. The original numberToString() try printing numbers with 16-17 digits. Then, for example,
> > setValueAsNumberAndGetValue(-1.2)
> is printed as
> > -1.2000000476837158
> with casting to float.

ok.

> What do you think about adding another numberToString with a different signature without rewriting the original numberToString? It's to avoid rewriting the original numberToString, but also to make it easy to keep following with future changes on the original numberToString. My first idea here was to print with limited digits by the same logic as JavaScript's ToString.

I think using DecimalNumber in HTMLParserIdioms as Gavin wrote is better. because HTML floating pint number doesn't need NaN/Infinity handling, so it is so different from the existing numberToString().

[Dai Mikurube]

Created attachment 72456 [details]
Patch

[Dai Mikurube]

Comment on attachment 72189 [details]
Patch

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

>> JavaScriptCore/ChangeLog:5
>> +        Too precise serialization from floating point number to string
> 
> Please change the summary as Simon did for the bug entry.
> We should mention that this affects only to input element.

Modified.

>> JavaScriptCore/JavaScriptCore.exp:392
>> +__ZN3WTF14numberToStringEdPtj
> 
> We need similar change to JavaScriptCore/JavaScriptCore.vcproj/JavaScriptCore/JavaScriptCore.def. The mangled symbol name in VC++ is different from gcc's.

Finally, removed it and added a new function toStringExponential to JavaScriptCore.exp and .def.

>> LayoutTests/fast/forms/script-tests/input-valueasnumber-number.js:24
>> +shouldBe('valueAsNumberFor("123456789012345678901234567890123456789")', '1.23456789012345678E+38');
> 
> Why the result is changed? This test doesn't use serializeForNumberType().

It was a mistake. I brought it back. (Actually, both results before and after the change PASSed.)

>> WebCore/html/HTMLInputElement.cpp:302
>> +    double acceptableError = step / pow(2.0, FLT_MANT_DIG);
> 
> This is specific to type=number.  Other types such as type=date may have different acceptableError value.
> Please introduce InputType::acceptableError(step) and override it in NumberInputType.  You can merge the same expression in NumberImputType::stepMismatch().

Done.

>> WebCore/html/HTMLInputElement.cpp:303
>> +    if (newValue - m_inputType->minimum() < -acceptableError) {
> 
> Is this change needed to pass existing tests?  If not, we need new test cases.

Yes, it is required to pass /platform/mac/fast/forms/input-number-click.html .

[Kent Tamura]

Comment on attachment 72456 [details]
Patch

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

> WebCore/html/NumberInputType.cpp:132
>      // Accepts erros in lower fractional part which IEEE 754 single-precision

The comment should be moved to NumberInputType::acceptableError().

> WebCore/html/parser/HTMLParserIdioms.cpp:71
> +    static const unsigned NumberToStringBufferLength = 96;

Why don't you use NumberToStringBuffer and NumberToStringBufferLength in dtoa.h?

[Dai Mikurube]

Comment on attachment 72456 [details]
Patch

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

>> WebCore/html/NumberInputType.cpp:132
>>      // Accepts erros in lower fractional part which IEEE 754 single-precision
> 
> The comment should be moved to NumberInputType::acceptableError().

Moved. Thanks.

>> WebCore/html/parser/HTMLParserIdioms.cpp:71
>> +    static const unsigned NumberToStringBufferLength = 96;
> 
> Why don't you use NumberToStringBuffer and NumberToStringBufferLength in dtoa.h?

It's because NumberToStringBuffer and -Length is just for numberToString(). Currently serializeForNumberType() doesn't use numberToString(), so I stopped to use NumberToStringBuffer here.
(I removed #include <dtoa.h>. I forgot to remove it.)

[Dai Mikurube]

Created attachment 72484 [details]
Patch

[Kent Tamura]

Comment on attachment 72484 [details]
Patch

ok

[Gavin Barraclough]

Sorry, just in the middle of commenting, explanation for r- is incoming!

[Gavin Barraclough]

I think this is the wrong fix for this problem – I think there is actually a bug here that we should be fixing, and adopting non-spec compliant behavior as a bug work-around wouldn't be the right solution.

All the code in HTMLInputElement is already written using double precision, which should be sufficient to obtain the desired results.  Looking through the code, it looks like the accuracy error is introduced by this line in HTMLInputElement::applyStep:

>    newValue = base + round((newValue - base) / step) * step;

I believe this ensures that after stepping the value will snap to the nearest number that fits the formula (base + step * I, for some integer I).  But looking through the defined algorithm for stepUp() in section 4.10.7.3 of the HTML5 spec I can't find any reference to the behavior.  Unless I'm missing something, this is also a bug – this should not be happening.  Assuming I'm reading the spec correctly, I think the real fix for this bug is probably just to remove this line of code.

I've not yet tested this, it is possible there is more than one place that errors are being introduced, but this is the only one I can spot.  Fundamentally 5.005 + (0.005 * 2) (which I believe is the calculation the spec requires us to perform) evaluated at double precision (which is what we're doing) produces a result that prints as desired using numberToString.  Any inaccuracies being introduced by errors in our code are bugs that should be fixed.  Once we are producing the correct value, per the spec, no changes to to-string conversion should be necessary.

cheers,
G.

[Kent Tamura]

(In reply to comment #18)
> >    newValue = base + round((newValue - base) / step) * step;

Unfortunately, removing the adjustment doesn't solve this issue.

In JavaScript console, 5.005 + 0.005 * 2 prints 5.015, but 5.005 + 0.005 * 4 prints 5.0249999999999995.

[Dai Mikurube]

(In reply to comment #18)
Thanks for your comments, Gavin.

I agree with your comments that reducing numeric erros is the first priority. It's very important. But I believe adjustment is always required. Numeric errors occur and accumulate if using double.

I tried just removing the line (from the version without my patch). Then, numeric errors still occur. And the error piles up by and by clicking. Like :
> 5.0249999999999995
> ...
> 5.064999999999999
> ...
> 5.109999999999998
> ...
> 5.149999999999997
> ...

I guess this snapping to integral multiples comes from 4.10.7.2.10 `The step attribute' (not 4.10.7.3) below. (Actually I have no idea on the true reason...it's not by me.) It mentions about stepMismatch(), not stepUp(). But the result of stepUp() should be false for stepMismatch().
> Constraint validation: When the element ... (snip) ... number, and that number subtracted from the step base is not an integral multiple of the allowed value step, the element is suffering from a step mismatch.

Currently, a similar adjustment is done at NumberInputType::stepMismatch() to fix the bug 48220. This change replaced using fmod() with adjusting to integral multiples.
> https://bugs.webkit.org/show_bug.cgi?id=48220

[Kent Tamura]

I think HTML5 specification doesn't assume internal representation of numbers is IEEE 754.  So the specification don't mention such rounding errors.

The ideal solution might be BCD.

[Kent Tamura]

(In reply to comment #21)
> I think HTML5 specification doesn't assume internal representation of numbers is IEEE 754.  So the specification don't mention such rounding errors.
> 
> The ideal solution might be BCD.

Ah, no.  `2.5.4.3 Real numbers' has a rounding step.  So we should not use BCD.
The specification should take care of rounding erros...

[Gavin Barraclough]

Hi Dai,

Okay, so assuming that we're going to continue to store the value as double-precision, and that as such errors may accumulate, I buy that we don't want to present these errors to the user, so I agree that we need to round the values converted to strings.

Couple more thoughts though:

(1) I think these comments in the patch are slightly misleading:

 63     // It's because printing
 64     // as double-precision floating point numbers (by JavaScript's ToString())
 65     // cannot work due to numeric errors. (JavaScript uses double-precision,
 66     // though HTML5 uses single-precision.)

 72     // Round the result with 9 significant digits.
 73     // 9 because of the significant digits of IEEE 754 single-precision numbers.

I don't think this is completely accurate.  I don't believe HTML5 can be said to use single-precision (it does spec single-precision range for values parsed, but the requirement to use JS ToString would seem to implicitly favor double-precision for any operations, to avoid a precision mismatch).  Looking at the code, I'm only seeing WebCore using double precision values here – so I'm not sure this issue can really be said to be related to single-precision.  Given this, I think 9 is really just an arbitrary limit, which is not ideal.

(2) There seems to be a second problem, in that the internal value of the input element won't accurately match its string representation.  I can imagine this exposing itself in two ways.  Firstly, since rounding errors are retained in the internal value, it seems they could accumulate – if a script triggered stepUp/stepDown enough, it would seem possible that the rounding errors in value could accumulate to the point of overflowing into the 9 s.f. range.  Secondly, if there is any method allowing direct access to the internal value (or if any such interface is introduced), then the value accessed as a string would not match the value accessed as a number and then converted to string (e.g. in script code).  The problem still seems to lie in the fact that the applyStep() method is generating a value containing rounding errors (and as such is possibly failing to generate a new value that satisfies its constraints).  As such, I think the right fix is still to change applyStep(), such that this produces a suitable result.  Rounding at this stage will both prevent errors from propagating, and ensure that if the value is read directly as a double through any other means its string representation will still match the string value of the input element.

Let me suggest a possible alternate solution.  Presumably the number of decimal places we want the value rounded to is equal to the max of the number of decimal places in the base and step values.  For example, with a base of 0.1 and a step of 0.01, we never want more than 2 DP of precision to be retained.  We can easily enough calculate the number of DP of precision required to represent the base/step values based on their string length, and index('.').  You could then truncate then newValue in applyStep() by multiplying by 10^DP, rounding, and then dividing by 10^DP.

cheers,
G.

[Dai Mikurube]

(In reply to comment #23)
Hi Gavin,

Ok, I agree with your final suggestion. Rounding in applyStep() is ideally the best and your rounding method looks great.

But I think it requires deep modifications in the class WTFString and other fundamental classes. (E.g. adding another overloaded WTFString::toDouble(bool *, int decimalPlaces), the same for some StringImpls, parseToDoubleForNumberType, InputType::parseToDouble, *InputType::parseToDouble, and so on...)

Is it acceptable? I'd like to hear some opinions from others. It was the reason that I was hesitating acculate rounding in applyStep(). It essentially requires information of the given string.


Memo:

I thought maximum/minimum limitations for confirmation, but no problem.
For single-precision numbers, DP is at most 38. The maximum number in single precision is 10^38. 10^(38*2) is ultimately required temporally. Double-precision can contain at most 10^308. (* But, of course, rounding errors are large near the boundaries.)

But we have to limit DP in parsing unreasonably long numbers given as a string. (E.g. <input step="0.0000...(snip)...01">)

[Dai Mikurube]

(In reply to comment #24)
Ah, I found it can be done without deep modifications. I'll do it.

[Gavin Barraclough]

One more quick thought, the method for rounding I described might cause some issues for very large/small values.

- The decimal-places check I described would not work for values with an exponential string representation – I'd suggest checking base/step for index('e'), and if it is found in either then don't perform the additional truncation.

- Also, for large values of newValue, the truncation may result in the value overflowing to infinity, so I'd suggest only rounding where (newValue < 10^21).  (This limit is the matches the cut-off where ToString conversion switches to exponential, and since values above this limit never have any decimal places in their representation we have nothing to truncate here anyway).

cheers,
G.

[Dai Mikurube]

(In reply to comment #26)
Hi,

For the first one, I think we have no special reason to give up for small values with exponential expression like "1.02E3" or "3.3013E-5". I tried handling values with exponential expressions in the Patch, but I will discard it if it results worse than nothing.

For the second one, I agree with your suggestion. Another reason is that digits around zero are not significant when newValue > 10^21. Because 2^53 < 10^21. (Actually 2^53 < 10^17 < 10^23.)

Then, may I ask your opinion on the upper limit of DP? My thought :

* Naturally, it must be smaller than 21 because of the same ("another") reason above. Multiplying by 10^21 occurs unnecessary rounding error.
* Actually, I guess at most 16 is better for the same reason. 2^53 < 10^17. (I used 16 in the Patch. Multiplying 10^16 for numbers with DP > 16.)
* But it makes "step=0.000000000000000001" incorrect though it is a valid number > FLT_MIN.
* Doing no truncation for any number with DP > 16 (like 0.123456789012345678) is another option.

[Dai Mikurube]

Created attachment 72653 [details]
Patch

[Gavin Barraclough]

Comment on attachment 72653 [details]
Patch

Hi Dai,

Sorry for taking the day to get back to you, but took while to get my head around the fp math (particularly convince myself that allowing rounding all the way up to the 16 DP limit was okay & wouldn't be introducing significant inaccuracies at that point).  But all looks like it should work fine to me.

I think this patch looks awesome – I think this will be a really great resolution to the bug.
Couple of really trivial issues with the code.

> WebCore/html/HTMLInputElement.cpp:320
> +    if (newValue - m_inputType->minimum() < -acceptableError) {

You've changed the logic here to allow a margin of error on this check.  I see why that makes sense, but it seems this could lead to the internal value being lower than the permitted minimum (albeit by a very small amount).  We could maybe catch this with a second check? - leave the error check as is with the tolerance, but below it follow up with a quick:...

+  if (newValue < m_inputType->minimum())
+      newValue = m_inputType->minimum();

... to make sure the internal value is never below the minimum, by snapping back up to the minimum if it was close but below.
What do you think, worth doing anything to fix this case up?
Also, I guess the same is true for the maximum check, that could go slightly over, may need the value nudging down to ensure it's in range.

> WebCore/html/HTMLInputElement.cpp:329
> +    baseDecimalPlaces = min(baseDecimalPlaces, (unsigned) 16);
> +    if (newValue < pow((double) 10, (double) 21)) {
> +        double scale = pow((double) 10, (double) max(stepDecimalPlaces, baseDecimalPlaces));

WebKit coding guidelines require c++ casts – personally I prefer C style casts for numeric code, but this should be sticking to the rules.
Also, in this case, we could probably ditch most of these casts anyway – we normally write (double)10 as 10.0 etc, and (unsigned)16 can just be 16u.
These casts should really be fixed, so r- I'm afraid.


One last thought looking at this code, just wondering if this is another bug in applyStep (this would be a bug in the existing code too, not an issue specific to your patch).

We're doing the minimum/maximum bounds checks before we adjust to ensure we're at an integer multiple of steps.  Could this be an error?
I'm thinking, could we end up in a situation something like:
  base = 0;
  value = 8.7;
  step = 1;
  maximum = 9.9
(I assume this is already in an invalid state, since value is not equal to base + step * INTEGER, but I'm also assuming it is possible to get into such states, since otherwise I don't think we'd need the code to snap back to a multiple of steps).

If we called stepUp from this state we'd increment value, giving a new value of 9.7.  This would pass the maximum bounds check, so no error would be generated.  We'd then adjust to snap to an integer multiple of steps, and round off new value to 10, above the maximum, and not catch this.

Think there's any danger this could be a real error? – I'm thinking maybe it would be safer to sink the min/max checks to below the code to snap to multiples on step, to prevent any risk of this.
What do you think?

cheers,
G.

[Kent Tamura]

Comment on attachment 72653 [details]
Patch

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

> WebCore/html/InputType.cpp:240
> +double InputType::stepBaseWithDecimalPlaces(unsigned* decimalPlaces) const

If you assume decimalPlaces must be non-0,
 - add ASSERT(decimalPlaces) and document it in the header, or
 - use unsigned&.

> WebCore/html/InputType.cpp:243
> +    stepBase();

"retrurn stepBase()" ?

> WebCore/html/InputType.cpp:284
> +    *decimalPlaces = 0;

ASSERT or unsigned&.

> WebCore/html/parser/HTMLParserIdioms.cpp:101
> +    *decimalPlaces = 0;

ditto.

[Dai Mikurube]

Comment on attachment 72653 [details]
Patch

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

Hi Gavin and Kent-san,

Thank you for the feedback. No problem, yesterday was a national holiday in Japan.

>> WebCore/html/HTMLInputElement.cpp:320
>> +    if (newValue - m_inputType->minimum() < -acceptableError) {
> 
> You've changed the logic here to allow a margin of error on this check.  I see why that makes sense, but it seems this could lead to the internal value being lower than the permitted minimum (albeit by a very small amount).  We could maybe catch this with a second check? - leave the error check as is with the tolerance, but below it follow up with a quick:...
> 
> +  if (newValue < m_inputType->minimum())
> +      newValue = m_inputType->minimum();
> 
> ... to make sure the internal value is never below the minimum, by snapping back up to the minimum if it was close but below.
> What do you think, worth doing anything to fix this case up?
> Also, I guess the same is true for the maximum check, that could go slightly over, may need the value nudging down to ensure it's in range.

These second checks are nice! I've added them. I was out of touch with the case.

>> WebCore/html/HTMLInputElement.cpp:329
>> +        newValue = round((base + round((newValue - base) / step) * step) * scale) / scale;
> 
> WebKit coding guidelines require c++ casts – personally I prefer C style casts for numeric code, but this should be sticking to the rules.
> Also, in this case, we could probably ditch most of these casts anyway – we normally write (double)10 as 10.0 etc, and (unsigned)16 can just be 16u.
> These casts should really be fixed, so r- I'm afraid.
> 
> 
> One last thought looking at this code, just wondering if this is another bug in applyStep (this would be a bug in the existing code too, not an issue specific to your patch).
> 
> We're doing the minimum/maximum bounds checks before we adjust to ensure we're at an integer multiple of steps.  Could this be an error?
> I'm thinking, could we end up in a situation something like:
>   base = 0;
>   value = 8.7;
>   step = 1;
>   maximum = 9.9
> (I assume this is already in an invalid state, since value is not equal to base + step * INTEGER, but I'm also assuming it is possible to get into such states, since otherwise I don't think we'd need the code to snap back to a multiple of steps).
> 
> If we called stepUp from this state we'd increment value, giving a new value of 9.7.  This would pass the maximum bounds check, so no error would be generated.  We'd then adjust to snap to an integer multiple of steps, and round off new value to 10, above the maximum, and not catch this.
> 
> Think there's any danger this could be a real error? – I'm thinking maybe it would be safer to sink the min/max checks to below the code to snap to multiples on step, to prevent any risk of this.
> What do you think?
> 
> cheers,
> G.

I see. It's an issue to be considered, I think. I've created another bug since it's a different issue from the bug 48308.
https://bugs.webkit.org/show_bug.cgi?id=48976

For short, I guess it's not an issue of min/max checks, but an issue on general steps from values in step-mismatching state.
As written in the bug 48976, for the case <input type="number" step="1" min="0" max="10.9" value="8.7" /> then stepUp(), I think the result should be 9 rather than 10. May I ask your opinion?

>> WebCore/html/InputType.cpp:240
>> +double InputType::stepBaseWithDecimalPlaces(unsigned* decimalPlaces) const
> 
> If you assume decimalPlaces must be non-0,
>  - add ASSERT(decimalPlaces) and document it in the header, or
>  - use unsigned&.

I've added a 0-check, and do nothing for 0. Because:
- Other functions for decimal places (by me) accept 0 and do nothing.
- Other existing functions use pointers for outputs by arguments.

>> WebCore/html/InputType.cpp:243
>> +    stepBase();
> 
> "retrurn stepBase()" ?

Exactly. Thank you.

[Dai Mikurube]

Created attachment 72906 [details]
Patch

[Kent Tamura]

Comment on attachment 72906 [details]
Patch

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

> WebCore/html/InputType.cpp:244
> +// decimalPlaces must be non-0..
> +double InputType::stepBaseWithDecimalPlaces(unsigned* decimalPlaces) const
> +{
> +    if (decimalPlaces)
> +        *decimalPlaces = 0;

The comment is confusing.  The code accepts decimalPlace==0.

[Dai Mikurube]

Created attachment 72908 [details]
Patch

[Dai Mikurube]

Comment on attachment 72906 [details]
Patch

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

>> WebCore/html/InputType.cpp:244
>> +        *decimalPlaces = 0;
> 
> The comment is confusing.  The code accepts decimalPlace==0.

Thanks. I failed in removing.

[Kent Tamura]

Comment on attachment 72908 [details]
Patch

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

> WebCore/ChangeLog:10
> +        * html/HTMLInputElement.cpp:

Please add what you changed for each of files/functions as possible.

> WebCore/html/parser/HTMLParserIdioms.cpp:130
> +        switch (digit = string[cursor]) {

Can't we use String::toInt()?  If not, please add the reason as a code comment or ChangeLog.

[Dai Mikurube]

Created attachment 73333 [details]
Patch

[Kent Tamura]

(In reply to comment #37)
> Created an attachment (id=73333) [details]
> Patch

Looks ok.  I'll set r+ tomorrow if Gavin has no comments.

[Gavin Barraclough]

Comment on attachment 73333 [details]
Patch

The patch looks great to me, setting r+ based on Kent's last comment.  

> I see. It's an issue to be considered, I think. I've created another bug since it's a different issue from the bug 48308.
> https://bugs.webkit.org/show_bug.cgi?id=48976
> 
> For short, I guess it's not an issue of min/max checks, but an issue on general steps from values in step-mismatching state.
> As written in the bug 48976, for the case <input type="number" step="1" min="0" max="10.9" value="8.7" /> then stepUp(), I think the result should be 9 rather than 10. May I ask your opinion?

Dealing with the step mismatch as a separate bug seems sensible, re the change in behavior, I'm not sure I have much of an opinion on this – it's quite a way from my area of expertise – but your proposed change certainly sounds right to me, seems intuitively like better behavior for stepUp to always take you to the next step, never the next-but-one.

cheers,
G.

[WebKit Commit Bot]

Comment on attachment 73333 [details]
Patch

Rejecting patch 73333 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=abarth-cq-sl', 'apply-attachment', '--force-clean', '--non-interactive', 73333]" exit_code: 1
Traceback (most recent call last):
  File "./WebKitTools/Scripts/webkit-patch", line 70, in <module>
    main()
  File "./WebKitTools/Scripts/webkit-patch", line 63, in main
    from webkitpy.tool.main import WebKitPatch
  File "/Users/abarth/git/webkit-queue/WebKitTools/Scripts/webkitpy/tool/main.py", line 42, in <module>
    from webkitpy.common.net.rietveld import Rietveld
ImportError: No module named rietveld

Full output: http://queues.webkit.org/results/5473057

[Kent Tamura]

I tried the 7th patch locally on Snow Leopard, and got a failure:

--- /tmp/layout-test-results/fast/forms/input-stepup-stepdown-expected.txt	2010-11-09 15:18:37.000000000 +0900
+++ /tmp/layout-test-results/fast/forms/input-stepup-stepdown-actual.txt	2010-11-09 15:18:37.000000000 +0900
@@ -167,7 +167,7 @@
 PASS for (var i = 0; i < 255; i++) { input.stepDown(); }; input.value is "0"
 Rounding
 PASS input.min="4"; stepUp("5.005", "0.005", "", 2) is "5.015"
-PASS input.min="4"; stepUp("5.005", "0.005", "", 11) is "5.06"
+FAIL input.min="4"; stepUp("5.005", "0.005", "", 11) should be 5.06. Was 5.0600000000000005.
 PASS input.min="4"; stepUp("5.005", "0.005", "", 12) is "5.065"
 
 Range type

Mikurube-san, would you check it please?

[Dai Mikurube]

(In reply to comment #41)
Hmm, it passed the test in my environment on Snow Leopard as follows:
Rounding
PASS input.min="4"; stepUp("5.005", "0.005", "", 2) is "5.015"
PASS input.min="4"; stepUp("5.005", "0.005", "", 11) is "5.06"
PASS input.min="4"; stepUp("5.005", "0.005", "", 12) is "5.065"


And #41 looks like a merge failure. I'll submit another patch.

[Dai Mikurube]

(In reply to comment #42)
>+FAIL input.min="4"; stepUp("5.005", "0.005", "", 11) should be 5.06. Was 5.0600000000000005.
It occurred in the rebased version. I'll check it.

[Dai Mikurube]

Created attachment 73347 [details]
Patch

[Dai Mikurube]

(In reply to comment #43)
It was an error in case of min=null. Fixed and added tests.

[Kent Tamura]

Comment on attachment 73347 [details]
Patch

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

> WebCore/html/HTMLInputElement.cpp:328
> +    fprintf(stderr, "b = %u\n", baseDecimalPlaces);

please remove this.

[Dai Mikurube]

Created attachment 73348 [details]
Patch

[Dai Mikurube]

(In reply to comment #46)
Thank you. Sorry for my careless.

[WebKit Commit Bot]

Comment on attachment 73348 [details]
Patch

Clearing flags on attachment: 73348

Committed r71622: <http://trac.webkit.org/changeset/71622>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.