Closed Bug 969928 Opened 6 years ago Closed 6 years ago

Fix a bug in the de-localization code that can cause validation issues with locales that use '.' as their grouping separator

Categories

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

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla30
Tracking Status
firefox27 --- unaffected
firefox28 --- unaffected
firefox29 + verified
firefox30 --- verified

People

(Reporter: aryx, Assigned: jwatt)

References

Details

Attachments

(2 files)

Attached file test case
Windows 8.1 Pro 64 bit in German, latest Nightly and Aurora in German

A localized number format is used for filling an <input type="number"> with a default value, but using |pattern="\d+"| as validation pattern shows that the English format is used for validation.

Steps to reproduce:
1. Load the test case in a localized Nightly or Aurora build where "," is the decimal separator and "." the separator for thousands.
2. Remove the last number from the input value and add it again, "6.005" > "6.00" > "6.005".
3. Press the tab key.

Actual result:
Input validation doesn't match pattern (if you do this on the remote debugging screen, it will tell you that the next allowed values are 6 and 7).

Expected result:
Input validates.

Further interesting note:
Validation only fails after user changed value.
(In reply to Archaeopteryx [:aryx] from comment #0)
> Actual result:
> Input validation doesn't match pattern (if you do this on the remote
> debugging screen, it will tell you that the next allowed values are 6 and 7).

The default "step" is 1. So what you're experiencing here is a step mismatch, which is why it's saying you need to choose either 6 or 7.

Try adding a step attribute to test the invalidation:

data:text/html,<input type=number lang=de step=0.001>

> Further interesting note:
> Validation only fails after user changed value.

That's intended so that intermediate invalid states are not shown.

So as far as I can tell this is working as expected.
Component: JavaScript Engine → DOM: Core & HTML
(In reply to Jonathan Watt [:jwatt] from comment #1)
> The default "step" is 1. So what you're experiencing here is a step
> mismatch, which is why it's saying you need to choose either 6 or 7.
> 
> Try adding a step attribute to test the invalidation:
> 
> data:text/html,<input type=number lang=de step=0.001>
But this is not about 0.001 as step. As mentioned above, this is about a German nightly where the "." is a thousand separator and Nightly initially also uses the dot as thousand separator (showing 6005 as "6.005"=, but later interprets it as decimal operator (validating the input value as not matching \d+ and being between 6 and 7 instead).
Oh, sorry, I'm forgetting "." is the thousand separator for German. So yes, this is a bug.

Note that "pattern" doesn't apply to <input type=number> (it's ignored).
(In reply to Archaeopteryx [:aryx] from comment #0)
> Windows 8.1 Pro 64 bit in German, latest Nightly and Aurora in German

I've just created a German localized Nightly and it works as expected. Where are you getting your localized Nightly from?
In about:config what is the value of the pref general.useragent.locale for you?
Wait, I do see the problem.
This might be related, or maybe should be its own bug, but the formatting causes another problem.  Say you have a value of "10,000" in the input and you want to change it to "100,000".  Without the formatting I would naturally just add a "0" to the end, but with the formatting it ends up being "10,0000" and this fails the validation.  Seems to me that it should automatically adjust the position of the thousands separators or just ignore them in the validation.
Matt, that's a separate issue, and one that I don't think we can get right in all cases. However, I think it is worth discussing, so could you file a bug for it nonetheless?
Summary: localized number format used for filling <input type="number"> with default value, but English format used for validation → Fix a bug in the de-localization code that can cause validation issues with locales that use '.' as their grouping separator
Attached patch patchSplinter Review
This is an issue for any locale that uses '.' as its grouping separator when the number given doesn't have a fractional part. The bug is that nsNumberControlFrame::GetValueOfAnonTextControl checks if the number "is an English locale", in which case we need to avoid sanitizing, by checking if HTMLInputElement::StringToDecimal(aValue).toDouble() is finite. That turns out to fail in German, for example, where "1.234" is the value 1234, but for which HTMLInputElement::StringToDecimal will return the finite value 1.234.

A much better check is to compare the result of localizing the value to the result of calling HTMLInputElement::StringToDecimal, and avoiding sanitization if the values are the same.
Assignee: nobody → jwatt
Status: NEW → ASSIGNED
Attachment #8377651 - Flags: review?(dholbert)
Depends on: 844744
Comment on attachment 8377651 [details] [diff] [review]
patch

>+  // Extra german test to check that a locale that uses '.' as its grouping
>+  // separator doesn't result in it being invalid (due to step mismatch) due
>+  // to the de-localization code mishandling numbers that look like numbers
>+  // formatted for English speakers

maybe s/look like numbers/look like *other* numbers/

(The "other" is the key part. :) )

>diff --git a/layout/forms/nsNumberControlFrame.cpp b/layout/forms/nsNumberControlFrame.cpp
>+  // Here we need to de-localize any value typed in by the user.

Could you clarify what "de-localize" means, in a parenthetical or something? That would make the rest of this documentation much clearer. It dances around the idea, but never directly explains it.

Maybe something like...
  // (i.e. we convert the number into an equivalent representation that
  // is parsable without without needing locale information.)
...if that's what you mean?

>+  // need to be careful to avoid normalizing numbers that are valid
>+  // floating-point number according to the HTML 5 spec:

grammar nit: "s/number/numbers/"

>+  // if those numbers do not need to be localized.

I don't understand what "do not need to be localized" means. (firstly, this function doesn't localize, it (sometimes) *de-*localizes; secondly, it's unclear what numbers *need* to be [de-]localized.)

Maybe clarify/reword? Perhaps something like:
  "if those numbers can already be parsed correctly without locale information."
?

(The "correctly" is important modifier, as shown by this bug. :))

>+ [...] This is because content (and
>+  // tests) expect us to avoid "normalizing" the number that the user types in
>+  // if it's not necessary.

Similar to above, it's unclear what "not necessary" means (i.e. which conversions are "necessary"?). This would be clearer with the clarifications I've suggested above, though, so probably no change needed here, if you clarify earlier bits.

>+  // Note that there are hazzards here though. For example, just because

(s/hazzards/hazards/)

>+  // "1.005" _look_ like a valid floating-point number according to the spec

s/look/looks/

>+  // does not mean that it does not need to be de-localized.

This sentence is hard to read (my mind trips over "does not mean that it does not" a bit). Maybe phrase as "Even though 1.005 looks like ..., it might still need to be de-localized"? [or normalized]

>+  [...] Hence we check whether the
>+  // value returned by ICUUtils::ParseNumber is different to the value
>+  // returned by HTMLInputElement::StringToDecimal to determine whether we
>+  // should avoid sanitizing.

s/should avoid sanitizing/should normalize/

(for consistency -- this is the first mention of "sanitize" -- and "sanitize" also has implications of security-checks, which aren't relevant here)

(and IMHO dropping "avoid" makes the sentence easier to read, by removing an unnecessary negation)

>   ICUUtils::LanguageTagIterForContent langTagIter(mContent);
>   double value = ICUUtils::ParseNumber(aValue, langTagIter);
>   if (NS_finite(value) &&
>-      !HTMLInputElement::StringToDecimal(aValue).isFinite()) {
>+      value != HTMLInputElement::StringToDecimal(aValue).toDouble()) {
>     aValue.Truncate();
>     aValue.AppendFloat(value);
>   }
> #endif
> }

Makes sense.

One last side note, though -- it looks like we do aValue.Truncate() in the early-return, as well as in the AppendFloat() case, but we don't touch the outparam at all in the implicit-final-return code-path here (after the #endif). That seems like a bug, or at least an inconsistency -- shouldn't we at least be truncating there?

Maybe we should just do a single "aValue.Truncate()" at the beginning of this function? Technically a separate bug, but small enough that it's probably fine to just fix as part of this patch, or as a followup.

r=me with the above
Attachment #8377651 - Flags: review?(dholbert) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/6fc2f69df1ee

The big comment was worked over quite considerably over IRC.

(In reply to Daniel Holbert [:dholbert] from comment #11)
> One last side note, though -- it looks like we do aValue.Truncate() in the
> early-return, as well as in the AppendFloat() case, but we don't touch the
> outparam at all in the implicit-final-return code-path here (after the
> #endif).

That's not the case, since the line |HTMLInputElement::FromContent(mTextField)->GetValue(aValue);| touches it. As agreed on IRC I added an 'else' comment at the end of the function.
OS: Windows 8.1 → All
Hardware: x86_64 → All
Target Milestone: --- → mozilla30
(Nit: I was imagining the "else" comment being *after* the #endif, since it's also the behavior we'll have if that whole #ifdef section gets skipped.  Doesn't matter too much, but maybe adjust that when re-landing if you agree?)
Relanded with text fix to avoid rounding issues, and with the comment move as requested in comment 14:

https://hg.mozilla.org/integration/mozilla-inbound/rev/183cb1d53c35
Relanded with *test* fix...
Comment on attachment 8377651 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug in new feature (bug 344616)
User impact if declined: number controls can be broken for users of certain locales
Testing completed (on m-c, etc.): landed on m-i yesterday, will merge to m-c on next merge
Risk to taking this patch (and alternatives if risky): low risk, early in aurora
String or IDL/UUID changes made by this patch: none
Attachment #8377651 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/183cb1d53c35
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Attachment #8377651 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Keywords: verifyme
Verified on Windows 7 x64 and Ubuntu 13.10 x86 using:
- Firefox 29.0b1
Build ID: 20140318013849
- Firefox 30.0a2
Build ID: 20140323004001
- Firefox 31.0a1
Build ID: 20140322030204

The issue can no longer be reproduced, since now we strip away "." or "," when manually entered as thousands separators (for DE-DE or EN-US respectively), upon step up/down (e.g. "1.234" becomes "1235" on step up).
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.