Bug 42484 - Support localized numbers in <input type=number>
Summary: Support localized numbers in <input type=number>
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 43973 45730
Blocks: 27968
  Show dependency treegraph
 
Reported: 2010-07-16 15:21 PDT by Kent Tamura
Modified: 2011-03-02 15:41 PST (History)
5 users (show)

See Also:


Attachments
Patch (18.24 KB, patch)
2011-01-27 01:23 PST, Kent Tamura
no flags Details | Formatted Diff | Diff
Patch 2 (18.29 KB, patch)
2011-01-27 01:31 PST, Kent Tamura
no flags Details | Formatted Diff | Diff
Patch 3 (32.40 KB, patch)
2011-02-01 02:33 PST, Kent Tamura
no flags Details | Formatted Diff | Diff
Patch 4 (32.63 KB, patch)
2011-02-01 06:30 PST, Kent Tamura
no flags Details | Formatted Diff | Diff
Patch 5 (30.03 KB, patch)
2011-02-01 20:55 PST, Kent Tamura
no flags Details | Formatted Diff | Diff
Patch 6 (30.10 KB, patch)
2011-02-28 19:51 PST, Kent Tamura
dglazkov: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kent Tamura 2010-07-16 15:21:02 PDT
In the current implementation, the value of <input type=number> must be an HTML5 number (ASCII digits) and is sent to a server as is.
We should accept localized number representations and convert them to an HTML5 number for form submission.
Comment 1 Patryk Zawadzki 2010-09-30 15:09:29 PDT
It would also be nice to display numbers in their localized form. Same probably applies to date/time fields.
Comment 2 Kent Tamura 2011-01-27 01:23:24 PST
Created attachment 80305 [details]
Patch
Comment 3 Kent Tamura 2011-01-27 01:26:18 PST
I'm not sure how to use new ICU headers in Mac.  Why we have Source/WebCore/icu/?
Comment 4 Kent Tamura 2011-01-27 01:31:26 PST
Created attachment 80307 [details]
Patch 2
Comment 5 Alexey Proskuryakov 2011-01-27 08:56:01 PST
> We should accept localized number representations and convert them to an HTML5 number for form submission.

Is that mandated by HTML5? The bug needs to explain where this "should" comes from. <http://www.whatwg.org/specs/web-apps/current-work/multipage/common-microsyntaxes.html#rules-for-parsing-floating-point-number-values> in particular is not localization aware.

> I'm not sure how to use new ICU headers in Mac.  Why we have Source/WebCore/icu/?

ICU is not an officially supported API on Mac OS X, so ICU headers aren't shipped with it. When really needed, we copy an appropriate version of a particular ICU header to our repository. I'm not sure if this is one of those cases, or we should just use CFNumberFormatter API.
Comment 6 Ian 'Hixie' Hickson 2011-01-27 15:54:47 PST
This is a UI issue and is out of scope of the spec. You "should" accept localised forms if that's what's best for users.
Comment 7 Kent Tamura 2011-01-27 16:07:30 PST
(In reply to comment #5)
> > We should accept localized number representations and convert them to an HTML5 number for form submission.
> 
> Is that mandated by HTML5? The bug needs to explain where this "should" comes from. <http://www.whatwg.org/specs/web-apps/current-work/multipage/common-microsyntaxes.html#rules-for-parsing-floating-point-number-values> in particular is not localization aware.

It's not mandatory.

I should have added more words to ChangeLog.  This change is only for UI.  HTML parsing and DOM property access are not affected.

I saw several requests for localized numbers for <input type=number>, I haven't seen any objections, and I believe we should support them.

> > I'm not sure how to use new ICU headers in Mac.  Why we have Source/WebCore/icu/?
> 
> ICU is not an officially supported API on Mac OS X, so ICU headers aren't shipped with it. When really needed, we copy an appropriate version of a particular ICU header to our repository. I'm not sure if this is one of those cases, or we should just use CFNumberFormatter API.

CFNumberFormatter looks enough.  Thank you.
Comment 8 Alexey Proskuryakov 2011-01-27 16:27:45 PST
> This change is only for UI.

Will is affect VoiceOver?
Comment 9 Kent Tamura 2011-01-27 17:26:44 PST
(In reply to comment #8)
> Will is affect VoiceOver?

I don't know.  How VoiceOver obtain text form an application?
Comment 10 Darin Adler 2011-01-27 17:41:11 PST
(In reply to comment #9)
> (In reply to comment #8)
> > Will it affect VoiceOver?
> 
> I don't know. How does VoiceOver obtain text form an application?

It calls the functions defined in the WebCore/accessibility directory.
Comment 11 Kent Tamura 2011-01-31 21:53:40 PST
(In reply to comment #10)
> > > Will it affect VoiceOver?
> > 
> > I don't know. How does VoiceOver obtain text form an application?
> 
> It calls the functions defined in the WebCore/accessibility directory.

Thanks.  I took a look at WebCore/accessibility/, and I couldn't get what would happen in this case.
ok, I'll implement LocalizedNumberCF and try VoiceOver.
I think VoiceOver should get localized text.
Comment 12 Kent Tamura 2011-02-01 02:33:42 PST
Created attachment 80733 [details]
Patch 3

Add LocalizedNumberMac.mm and a test
Comment 13 Kent Tamura 2011-02-01 02:34:35 PST
(In reply to comment #11)
> I think VoiceOver should get localized text.

I confirmed that VoiceOver got a localized one with the patch.
Comment 14 Kent Tamura 2011-02-01 06:30:57 PST
Created attachment 80755 [details]
Patch 4

Fix parsing error handling in LocalizedNumberMac.mm
Comment 15 Kent Tamura 2011-02-01 20:55:30 PST
Created attachment 80884 [details]
Patch 5
Comment 16 Kent Tamura 2011-02-28 19:51:45 PST
Created attachment 84179 [details]
Patch 6

Rebased
Comment 17 Dimitri Glazkov (Google) 2011-03-01 15:28:58 PST
Comment on attachment 84179 [details]
Patch 6

View in context: https://bugs.webkit.org/attachment.cgi?id=84179&action=review

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:5665
> +		F5CC42DC12F801CA00D5F7E3 /* LocalizedNumberMac.mm in Sources */ = {isa = PBXBuildFile; fileRef = F5CC42DB12F801CA00D5F7E3 /* LocalizedNumberMac.mm */; };

Did you run sort-Xcode-project-file after your changes? This file looks out of order.

> Source/WebCore/platform/text/mac/LocalizedNumberMac.mm:58
> +    NSNumberFormatter* formatter = [[[NSNumberFormatter alloc] init] autorelease];

I don't know how this stuff works. Can you check with ObJC-heads before landing?
Comment 18 Alexey Proskuryakov 2011-03-01 16:08:42 PST
> Source/WebCore/platform/text/mac/LocalizedNumberMac.mm:58
> +    NSNumberFormatter* formatter = [[[NSNumberFormatter alloc] init] autorelease];

Although autorelease works, we prefer explicit refcounting in WebCore. So, this should be:

RetainPtr<NSNumberFormatter> formatter(AdoptNS, [[NSNumberFormatter alloc] init]);

+    NSNumber* num = [formatter numberFromString:numberString];

Please don't abbreviate.

+    if (num == nil)

WebKit style is to not compare to zero, so this should just be "if (!num)" (with a better name for num).

+    NSNumber* num = [NSNumber numberWithDouble:number];

WebKit style is to put stars on the other side for Objective-C types. But it would be better to use alloc/init and RetainPtr here, too. The reasons I know of are:
1) It's faster to destroy temporaries right away, while they are in memory cache.
2) The autorelease pool won't be drained during micro-benchmarks that spin without returning to message loop, causing high memory usage.
Comment 19 Kent Tamura 2011-03-01 17:24:03 PST
Dimitri,  Alexey, thank you for reviewing.
I'll update the code and commit it.
Comment 20 Kent Tamura 2011-03-01 22:21:45 PST
> +    NSNumber* num = [formatter numberFromString:numberString];
> +    if (num == nil)

Should we use RetainPtr<> here too?
Comment 21 Kent Tamura 2011-03-01 22:47:06 PST
(In reply to comment #20)
> > +    NSNumber* num = [formatter numberFromString:numberString];
> > +    if (num == nil)
> 
> Should we use RetainPtr<> here too?

Probably, no.  This object is already registered to an autorelease pool by NSNumberFormatter.
Comment 22 Kent Tamura 2011-03-01 22:55:56 PST
Landed: http://trac.webkit.org/changeset/80096
Comment 23 Alexey Proskuryakov 2011-03-01 23:04:44 PST
I think that it's more of a coding style issue in this case. If we were optimizing this function, we'd likely have to cache NSNumberFormatter instance at least.
Comment 24 Kent Tamura 2011-03-02 15:41:52 PST
(In reply to comment #23)
> I think that it's more of a coding style issue in this case. If we were optimizing this function, we'd likely have to cache NSNumberFormatter instance at least.

Indeed.  These functions should be called only the main thread.  It's easy to cache the number formatter.