parsing input type=number should skip spaces

parsing input type=number should skip spaces
skipping spaces in number string parsing without reformatting user input.

BUG=162463
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=171834

[gnana, 2014-04-11 14:30:28]

Please have a look.

[tkent, 2014-04-14 00:47:37]

What we should update is Locale::convertFromLocalizedNumber(). We should not
change behavior of input.value and value HTML attribute parsing.

https://codereview.chromium.org/235273003/diff/1/LayoutTests/fast/forms/numbe...
File LayoutTests/fast/forms/number/number-validity-typemismatch.html (right):

https://codereview.chromium.org/235273003/diff/1/LayoutTests/fast/forms/numbe...
LayoutTests/fast/forms/number/number-validity-typemismatch.html:40: check('
10');
This test should not be updated.  The behavior of input.value should not be
changed.

https://codereview.chromium.org/235273003/diff/1/LayoutTests/fast/forms/numbe...
File LayoutTests/fast/forms/number/number-valueasnumber.html (right):

https://codereview.chromium.org/235273003/diff/1/LayoutTests/fast/forms/numbe...
LayoutTests/fast/forms/number/number-valueasnumber.html:14: input.value =
stringValue;
This test should not be updated.  The behavior of input.value should not be
changed.

https://codereview.chromium.org/235273003/diff/1/Source/core/html/parser/HTML...
File Source/core/html/parser/HTMLParserIdioms.cpp (right):

https://codereview.chromium.org/235273003/diff/1/Source/core/html/parser/HTML...
Source/core/html/parser/HTMLParserIdioms.cpp:101: Decimal
parseToDecimalForNumberType(const String& numberString, const Decimal&
fallbackValue)
Do not modify this function. The behavior of this function is defined by the
HTML standard, and we can't change it.

https://codereview.chromium.org/235273003/diff/1/Source/core/html/parser/HTML...
Source/core/html/parser/HTMLParserIdioms.cpp:123: double
parseToDoubleForNumberType(const String& numberString, double fallbackValue)
Ditto.

https://codereview.chromium.org/235273003/diff/1/Source/platform/text/Platfor...
File Source/platform/text/PlatformLocale.cpp (right):

https://codereview.chromium.org/235273003/diff/1/Source/platform/text/Platfor...
Source/platform/text/PlatformLocale.cpp:287: case ' ':
This change is unnecessary.  The input string must not contain spaces.

[gnana, 2014-04-14 11:27:27]

On 2014/04/14 00:47:37, tkent wrote:
> What we should update is Locale::convertFromLocalizedNumber(). We should not
> change behavior of input.value and value HTML attribute parsing.
> 
>
https://codereview.chromium.org/235273003/diff/1/LayoutTests/fast/forms/numbe...
> File LayoutTests/fast/forms/number/number-validity-typemismatch.html (right):
> 
>
https://codereview.chromium.org/235273003/diff/1/LayoutTests/fast/forms/numbe...
> LayoutTests/fast/forms/number/number-validity-typemismatch.html:40: check('
> 10');
> This test should not be updated.  The behavior of input.value should not be
> changed.
> 
>
https://codereview.chromium.org/235273003/diff/1/LayoutTests/fast/forms/numbe...
> File LayoutTests/fast/forms/number/number-valueasnumber.html (right):
> 
>
https://codereview.chromium.org/235273003/diff/1/LayoutTests/fast/forms/numbe...
> LayoutTests/fast/forms/number/number-valueasnumber.html:14: input.value =
> stringValue;
> This test should not be updated.  The behavior of input.value should not be
> changed.
> 
>
https://codereview.chromium.org/235273003/diff/1/Source/core/html/parser/HTML...
> File Source/core/html/parser/HTMLParserIdioms.cpp (right):
> 
>
https://codereview.chromium.org/235273003/diff/1/Source/core/html/parser/HTML...
> Source/core/html/parser/HTMLParserIdioms.cpp:101: Decimal
> parseToDecimalForNumberType(const String& numberString, const Decimal&
> fallbackValue)
> Do not modify this function. The behavior of this function is defined by the
> HTML standard, and we can't change it.
> 
>
https://codereview.chromium.org/235273003/diff/1/Source/core/html/parser/HTML...
> Source/core/html/parser/HTMLParserIdioms.cpp:123: double
> parseToDoubleForNumberType(const String& numberString, double fallbackValue)
> Ditto.
> 
>
https://codereview.chromium.org/235273003/diff/1/Source/platform/text/Platfor...
> File Source/platform/text/PlatformLocale.cpp (right):
> 
>
https://codereview.chromium.org/235273003/diff/1/Source/platform/text/Platfor...
> Source/platform/text/PlatformLocale.cpp:287: case ' ':
> This change is unnecessary.  The input string must not contain spaces.

Thanks tkent for the review.
Locale::convertFromLocalizedNumber() is used only for
HTMLInputElement::hasBadInput()
other wise i dont see any other functions which uses it. So i feel updating
convertFromLocalizedNumber
will be of no benefit. I would be happy if you can give more pointers

[gnana, 2014-04-14 11:29:41]

Thanks tkent for the review.
Locale::convertFromLocalizedNumber() is used only for
HTMLInputElement::hasBadInput()
other wise i dont see any other functions which uses it. So i feel updating
convertFromLocalizedNumber
will be of no benefit. I would be happy if you can give more pointers

[tkent, 2014-04-14 23:34:22]

On 2014/04/14 11:29:41, gnana wrote:
> Thanks tkent for the review.
> Locale::convertFromLocalizedNumber() is used only for
> HTMLInputElement::hasBadInput()
> other wise i dont see any other functions which uses it. So i feel updating
> convertFromLocalizedNumber
> will be of no benefit. I would be happy if you can give more pointers

Did you try updating Locale::convertFromLocalizedNumber()?
NumberInputType::convertFromVisibleValue uses it.

[gnana, 2014-04-15 13:33:11]

Thanks tkent!
This works. 
I have put the patch again.
Please have a look.

[tkent, 2014-04-16 01:52:51]

https://codereview.chromium.org/235273003/diff/60001/LayoutTests/fast/forms/n...
File LayoutTests/fast/forms/number/number-setvalueforuser-skip-spaces.html
(right):

https://codereview.chromium.org/235273003/diff/60001/LayoutTests/fast/forms/n...
LayoutTests/fast/forms/number/number-setvalueforuser-skip-spaces.html:1: <html>
The test name is confusing.  We have HTMLInputElemenet::setValueForUser, but
this test is not for it.

number-skip-spaces-in-user-input.html?

https://codereview.chromium.org/235273003/diff/60001/LayoutTests/fast/forms/n...
LayoutTests/fast/forms/number/number-setvalueforuser-skip-spaces.html:4: <input
type="number" id="input" style="width:300px; height:50px; padding-left: 10px;
margin-bottom: 10px; border:1px solid green;"></input>
The style attribute looks unnecessary.

https://codereview.chromium.org/235273003/diff/60001/LayoutTests/fast/forms/n...
LayoutTests/fast/forms/number/number-setvalueforuser-skip-spaces.html:12:
document.getElementById('print').innerHTML = inputValue;
Wrong indentation.

https://codereview.chromium.org/235273003/diff/60001/LayoutTests/fast/forms/n...
LayoutTests/fast/forms/number/number-setvalueforuser-skip-spaces.html:13: if
(!isNaN(inputValue))
We should check the specific value.
  shouldBeEqualToString('inputValue', '123456');
and we don't need to insert the value to the 'print' element.

https://codereview.chromium.org/235273003/diff/60001/LayoutTests/fast/forms/n...
LayoutTests/fast/forms/number/number-setvalueforuser-skip-spaces.html:19: var
input = document.getElementById('input');
This variable is redundant.

https://codereview.chromium.org/235273003/diff/60001/LayoutTests/fast/forms/n...
LayoutTests/fast/forms/number/number-setvalueforuser-skip-spaces.html:24:
eventSender.keyDown(" ");
We don't need to use eventSender.  document.execCommand('InsertText', false, '
123 456 ') works and we can remove the manual test instruction.

https://codereview.chromium.org/235273003/diff/60001/Source/platform/text/Pla...
File Source/platform/text/PlatformLocale.cpp (right):

https://codereview.chromium.org/235273003/diff/60001/Source/platform/text/Pla...
Source/platform/text/PlatformLocale.cpp:359: String input =
localized.removeCharacters(isSpaceCharacter);
Let's use isASCIISpace in wtf/ASCIICType.h for simplicity.

[gnana, 2014-04-16 14:13:56]

Please have a look

https://codereview.chromium.org/235273003/diff/60001/LayoutTests/fast/forms/n...
File LayoutTests/fast/forms/number/number-setvalueforuser-skip-spaces.html
(right):

https://codereview.chromium.org/235273003/diff/60001/LayoutTests/fast/forms/n...
LayoutTests/fast/forms/number/number-setvalueforuser-skip-spaces.html:1: <html>
On 2014/04/16 01:52:51, tkent wrote:
> The test name is confusing.  We have HTMLInputElemenet::setValueForUser, but
> this test is not for it.
> 
> number-skip-spaces-in-user-input.html?

Done.

https://codereview.chromium.org/235273003/diff/60001/LayoutTests/fast/forms/n...
LayoutTests/fast/forms/number/number-setvalueforuser-skip-spaces.html:4: <input
type="number" id="input" style="width:300px; height:50px; padding-left: 10px;
margin-bottom: 10px; border:1px solid green;"></input>
On 2014/04/16 01:52:51, tkent wrote:
> The style attribute looks unnecessary.

Done.

https://codereview.chromium.org/235273003/diff/60001/LayoutTests/fast/forms/n...
LayoutTests/fast/forms/number/number-setvalueforuser-skip-spaces.html:12:
document.getElementById('print').innerHTML = inputValue;
On 2014/04/16 01:52:51, tkent wrote:
> Wrong indentation.

Done.

https://codereview.chromium.org/235273003/diff/60001/LayoutTests/fast/forms/n...
LayoutTests/fast/forms/number/number-setvalueforuser-skip-spaces.html:13: if
(!isNaN(inputValue))
On 2014/04/16 01:52:51, tkent wrote:
> We should check the specific value.
>   shouldBeEqualToString('inputValue', '123456');
> and we don't need to insert the value to the 'print' element.
> 

Done.

https://codereview.chromium.org/235273003/diff/60001/LayoutTests/fast/forms/n...
LayoutTests/fast/forms/number/number-setvalueforuser-skip-spaces.html:19: var
input = document.getElementById('input');
On 2014/04/16 01:52:51, tkent wrote:
> This variable is redundant.

Done.

https://codereview.chromium.org/235273003/diff/60001/LayoutTests/fast/forms/n...
LayoutTests/fast/forms/number/number-setvalueforuser-skip-spaces.html:24:
eventSender.keyDown(" ");
On 2014/04/16 01:52:51, tkent wrote:
> We don't need to use eventSender.  document.execCommand('InsertText', false, '
> 123 456 ') works and we can remove the manual test instruction.

Done.

https://codereview.chromium.org/235273003/diff/60001/Source/platform/text/Pla...
File Source/platform/text/PlatformLocale.cpp (right):

https://codereview.chromium.org/235273003/diff/60001/Source/platform/text/Pla...
Source/platform/text/PlatformLocale.cpp:359: String input =
localized.removeCharacters(isSpaceCharacter);
On 2014/04/16 01:52:51, tkent wrote:
> Let's use isASCIISpace in wtf/ASCIICType.h for simplicity.

Done.

[tkent, 2014-04-17 02:17:06]

lgtm

[commit-bot: I haz the power, 2014-04-17 02:17:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gnanasekar.s@samsung.com/235273003/100001

[commit-bot: I haz the power, 2014-04-17 03:22:31]

Change committed as 171834