1099103 - Prevent numbers input using a grouping separator from being mis-processed as if the separator was a decimal separator (e.g. "12.34" with lang=de)

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[reno.tmpi]

Attachment: bug_input_number3decimals.zip

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:33.0) Gecko/20100101 Firefox/33.0
Build ID: 20141030112145

Steps to reproduce:

Firefox 33.1 Mac OS X (FRENCH).

In an input field of type "number" and with step="0.001":
- enter a decimal number with 3 decimals after separator or more.
- use dot (.) as separator and not comma (,)
- get the value with a simple document.getElementById('fieldId').value



Actual results:

The field is correctly displayed in the field.
However internally when you get the value, Firefox removes the separator and concatenates the value (example: 12.345 becomes 12345 which is really not the same !).


Expected results:

Example : 12.345 should result in 12.345.

Strangely, when theres only 2 decimals it works fine:
12.34 becomes 12.34 (OK)
12,34 becomes 12.34 (OK)

With 3 decimals,
12,345 becomes 12.345 (OK)
12.345 becomes 12345 (NOT OK)

I've created a really basic html test file in pastebin here that clearly show the bug: http://pastebin.com/JRaEn7D6

The html file is attached in this report too.

Note that it really may be possible that the bug is present in the french version only.

Comment 1

[Matthias Versen [:Matti]]

Attachment: html testcase from the reporter

Comment 2

[Loic]

Attachment: bug_input_number3decimals.html

Comment 3

[Matthias Versen [:Matti]]

I can not reproduce the issue on OSX10.9 with german settings and Firefox33/36.

What kind of settings do you have in OSX set in Language&Region-> advanced under Number separators ?
I have grouping="." and decimal="," in my german version

Comment 4

[reno.tmpi]

Hello,

In french, grouping is empty and decimal is set to "," (for "number separator" and "currency" too).

It's not a bug related to my machine because it's a client of us who spotted it first (he was using "." instead of ",", which is not correct but there's no rule forbidding it either).

FYI I made a typo in the html example : I wrote input type="input" instead of input type="number", but the bug is still present even if it is replaced.

Comment 5

[Matthias Versen [:Matti]]

There is a discussion in bug 344616 about the separators

Comment 6

[Jonathan Watt [:jwatt]]

We look for locale information in ICUUtils::LanguageTagIterForContent::GetNext(). Currently we:

  1. look for 'lang'/'xml:lang' attribute on elements along the input's
     parent chain

  2. look for document local info such as Content-Language HTTP header

  3. look at the user-agent's locale

We do not currently look at system settings.

Comment 7

[Jonathan Watt [:jwatt]]

The relevant code here is under the HTMLInputElement::PreHandleEvent call for the number input, when it spots an NS_EDITOR_INPUT event on its way to its text input child. That method first calls nsNumberControlFrame::GetValueOfAnonTextControl to get the number typed in by the user as a de-localized string in canonical form ("." as the decimal separator, no grouping separators, etc.). It then calls HTMLInputElement::SetValueInternal to set the resulting string on itself, and HTMLInputElement::SetValueInternal calls HTMLInputElement::SanitizeValue on the string before actually setting it.

In the case of the user inputting "12.345", when GetValueOfAnonTextControl calls ICUUtils::ParseNumber, that accepts the "." as a grouping separator for some reason and returns "12345". When HTMLInputElement::SanitizeValue is later passed "12345" it accepted and sets that value.

In the case of the user inputting "12.34", ICUUtils::ParseNumber reject the input because in order to be a grouping separator the "." would need to be followed by three digits. When that happens, nsNumberControlFrame::GetValueOfAnonTextControl returns the "12.34" unchanged (see the comment at the end of that method). Of course "12.34" is valid as far as HTMLInputElement::SanitizeValue is concerned, so that string ends up being set as the value on the input.

Comment 8

[Jonathan Watt [:jwatt]]

(In reply to Matthias Versen [:Matti] from comment #3)
> I can not reproduce the issue on OSX10.9 with german settings and
> Firefox33/36.

That's because the testcases are broken (use type=input instead of type=number).

Comment 9

[Jonathan Watt [:jwatt]]

In summary there are arguably two bugs:

 1) "12.345" parses as 12345 when it should parse as NaN since "." is
    not a grouping separator for lang=fr

 2) "12.34" parsing as 12.34 when it should parse as NaN since, even if
    "." is a grouping separator, it is followed by an insufficient number
    of digits to make a group.

I don't know enough about the French locale to know if #1 is actually a bug, and if ICU accepts it I assume "." is acceptable as a grouping separator sometimes in French. (I believe the use of "." as a grouping separator in roman numerals was the reason "," was used as the decimal separator in France.) If anyone disagrees, please open a separate bug.

Therefore I'll focus on #2 in this bug.

Comment 10

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 11

[Daniel Holbert [:dholbert]]

Comment on attachment 8660512 [details] [diff] [review]
patch

Review of attachment 8660512 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f7cbf3aa76fc

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out for bustage: https://treeherder.mozilla.org/logviewer.html#?job_id=14459258&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/7d073d55f7c7

Comment 16

[Jonathan Watt [:jwatt]]

The patch here does change the behavior as intended for localized builds, but the tests fail on inbound because we use English localized builds there and using English localized builds with lang=de isn't the same. I have been considering different ways of fixing this, including whether we should maybe not fall back to the UA's locale, but I've decided to punt on that to bug 1216831. Since this patch does make the desired changes for localized builds I'll land it with the tests disabled for now, and I can work on enabling them in that bug.

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4f554ce35fb8

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4f554ce35fb8