Closed Bug 1099103 Opened 7 years ago Closed 6 years ago

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)

Categories

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

33 Branch
x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: reno.tmpi, Assigned: jwatt)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

Attached file bug_input_number3decimals.zip (obsolete) —
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.
Attached file bug_input_number3decimals.html (obsolete) —
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
Flags: needinfo?(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.
Flags: needinfo?(reno.tmpi)
There is a discussion in bug 344616 about the separators
Component: Untriaged → DOM: Core & HTML
Product: Firefox → Core
Flags: needinfo?(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.
Flags: needinfo?(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.
Summary: Input number bug with 3 decimals or more → 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)
(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).
Attachment #8522966 - Attachment is obsolete: true
Attachment #8522984 - Attachment is obsolete: true
Attachment #8522986 - Attachment is obsolete: true
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.
Attached patch patchSplinter Review
Assignee: nobody → jwatt
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #8660512 - Flags: review?(dholbert)
Comment on attachment 8660512 [details] [diff] [review]
patch

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

Looks good.
Attachment #8660512 - Flags: review?(dholbert) → review+
Duplicate of this bug: 1198760
Duplicate of this bug: 1208923
Blocks: 1216831
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.
Flags: needinfo?(jwatt)
https://hg.mozilla.org/mozilla-central/rev/4f554ce35fb8
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.