Bug 42484 – Support localized numbers in <input type=number>

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Kent Tamura]

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.

[Patryk Zawadzki]

It would also be nice to display numbers in their localized form. Same probably applies to date/time fields.

[Kent Tamura]

Created attachment 80305 [details]
Patch

[Kent Tamura]

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

[Kent Tamura]

Created attachment 80307 [details]
Patch 2

[Alexey Proskuryakov]

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

[Ian 'Hixie' Hickson]

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.

[Kent Tamura]

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

[Alexey Proskuryakov]

> This change is only for UI.

Will is affect VoiceOver?

[Kent Tamura]

(In reply to comment #8)
> Will is affect VoiceOver?

I don't know.  How VoiceOver obtain text form an application?

[Darin Adler]

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

[Kent Tamura]

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

[Kent Tamura]

Created attachment 80733 [details]
Patch 3

Add LocalizedNumberMac.mm and a test

[Kent Tamura]

(In reply to comment #11)
> I think VoiceOver should get localized text.

I confirmed that VoiceOver got a localized one with the patch.

[Kent Tamura]

Created attachment 80755 [details]
Patch 4

Fix parsing error handling in LocalizedNumberMac.mm

[Kent Tamura]

Created attachment 80884 [details]
Patch 5

[Kent Tamura]

Created attachment 84179 [details]
Patch 6

Rebased

[Dimitri Glazkov (Google)]

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?

[Alexey Proskuryakov]

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

[Kent Tamura]

Dimitri,  Alexey, thank you for reviewing.
I'll update the code and commit it.

[Kent Tamura]

> +    NSNumber* num = [formatter numberFromString:numberString];
> +    if (num == nil)

Should we use RetainPtr<> here too?

[Kent Tamura]

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

[Kent Tamura]

Landed: http://trac.webkit.org/changeset/80096

[Alexey Proskuryakov]

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.

[Kent Tamura]

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