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

Status: VERIFIED FIXED

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

Description

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: 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.

Comment 1

[Jonathan Watt [:jwatt]]

(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.

Comment 2

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

(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).

Comment 3

[Jonathan Watt [:jwatt]]

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).

Comment 4

[Jonathan Watt [:jwatt]]

(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?

Comment 5

[Jonathan Watt [:jwatt]]

This build also works fine for me:

http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla-central-l10n/firefox-30.0a1.de.mac.dmg

Comment 6

[Jonathan Watt [:jwatt]]

In about:config what is the value of the pref general.useragent.locale for you?

Comment 7

[Jonathan Watt [:jwatt]]

Wait, I do see the problem.

Comment 8

[Matt Cosentino]

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.

Comment 9

[Jonathan Watt [:jwatt]]

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?

Comment 10

[Jonathan Watt [:jwatt]]

Attachment: patch

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.

Comment 11

[Daniel Holbert [:dholbert]]

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

Comment 12

[Jonathan Watt [:jwatt]]

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.

Comment 13

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

Backed out in http://hg.mozilla.org/integration/mozilla-inbound/rev/56fa355b196d

For mochitest-1 bustage: https://tbpl.mozilla.org/php/getParsedLog.php?id=34866693&tree=Mozilla-Inbound

Comment 14

[Daniel Holbert [:dholbert]]

(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?)

Comment 15

[Jonathan Watt [:jwatt]]

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

Comment 16

[Jonathan Watt [:jwatt]]

Relanded with *test* fix...

Comment 17

[Jonathan Watt [:jwatt]]

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

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/183cb1d53c35

Comment 19

[Jonathan Watt [:jwatt]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/90263d117191

Comment 20

[Florin Mezei, QA (:FlorinMezei)]

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).