Closed
Bug 969928
Opened 7 years ago
Closed 7 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)
Core
DOM: Core & HTML
Tracking
()
VERIFIED
FIXED
mozilla30
Tracking | Status | |
---|---|---|
firefox27 | --- | unaffected |
firefox28 | --- | unaffected |
firefox29 | + | verified |
firefox30 | --- | verified |
People
(Reporter: aryx, Assigned: jwatt)
References
Details
Attachments
(2 files)
258 bytes,
text/html
|
Details | |
3.78 KB,
patch
|
dholbert
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•7 years ago
|
||
(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
![]() |
Reporter | |
Comment 2•7 years ago
|
||
(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).
Assignee | ||
Comment 3•7 years ago
|
||
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).
Assignee | ||
Comment 4•7 years ago
|
||
(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?
Assignee | ||
Comment 5•7 years ago
|
||
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
Assignee | ||
Comment 6•7 years ago
|
||
In about:config what is the value of the pref general.useragent.locale for you?
Assignee | ||
Comment 7•7 years ago
|
||
Wait, I do see the problem.
Comment 8•7 years ago
|
||
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.
Assignee | ||
Comment 9•7 years ago
|
||
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?
Assignee | ||
Updated•7 years ago
|
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
Assignee | ||
Comment 10•7 years ago
|
||
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•7 years ago
|
||
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+
Assignee | ||
Comment 12•7 years ago
|
||
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.
Assignee | ||
Updated•7 years ago
|
tracking-firefox29:
--- → ?
OS: Windows 8.1 → All
Hardware: x86_64 → All
Target Milestone: --- → mozilla30
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•7 years ago
|
||
(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?)
Assignee | ||
Comment 15•7 years ago
|
||
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
Assignee | ||
Comment 16•7 years ago
|
||
Relanded with *test* fix...
Updated•7 years ago
|
Assignee | ||
Comment 17•7 years ago
|
||
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?
Comment 18•7 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/183cb1d53c35
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Updated•7 years ago
|
Attachment #8377651 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 19•7 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/90263d117191
Comment 20•7 years ago
|
||
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.
Description
•