Bug 97633

Summary: [Forms] Adding localization texts for multiple fields date/time input UI
Product: WebKit Reporter: yosin
Component: FormsAssignee: yosin
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dglazkov, fishd, jamesr, tkent, tkent+wkapi, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 97640    
Bug Blocks: 97299    
Attachments:
Description Flags
Patch 1
none
Patch 2
none
Patch 3
none
Patch 4
none
Patch 5
none
Patch 6 none

Description yosin 2012-09-25 22:57:01 PDT
To implement multiple fields date/time input UI, we would like to have:
  1. month format in LDML, e.g. "MM/yyyy"
  2. week format in LDML, e.g. "W/yyyy"
  3. date format indicates letters, e.g. "ymd" to display "dd-mm-yyyy"
Comment 1 yosin 2012-09-25 23:59:41 PDT
Created attachment 165739 [details]
Patch 1
Comment 2 yosin 2012-09-26 00:00:25 PDT
Comment on attachment 165739 [details]
Patch 1

Could you review this patch?
Thanks in advance.
Comment 3 WebKit Review Bot 2012-09-26 00:03:00 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 4 Kent Tamura 2012-09-26 00:15:56 PDT
Comment on attachment 165739 [details]
Patch 1

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

> Source/WebCore/ChangeLog:16
> +        Note: We'll rename ENABLE_INPUT_TYPE_MULTIPLE_FIELDS to generic name
> +        to support "date", "datetime", "month" and "week" multiple fields input
> +        UI.

Why don't you rename it before this patch?

> Source/WebCore/ChangeLog:22
> +        (WebCore): Added declarations of dateFormatHelpLetters(), dateFormatHelpText(), monthFormatInLDML() and
> +        weekFormatInLDML().

Because we have no call sites of these functions yet, none except you can understand what the functions should return.
Please explain what they are and why they are needed concretely.
Comment 5 yosin 2012-09-26 02:16:46 PDT
Created attachment 165755 [details]
Patch 2
Comment 6 yosin 2012-09-26 02:22:41 PDT
Created attachment 165757 [details]
Patch 3
Comment 7 yosin 2012-09-26 02:24:46 PDT
Comment on attachment 165757 [details]
Patch 3

Could you review this patch?
Thanks in advance.

= Changes since the last review =
* Update ChangeLog to refer ENABLE_INPUT_MULTIPLE_FIELDS_UI
* Add more explanations for new functions.
* Fix localized string identifiers in functions.
Comment 8 Kent Tamura 2012-09-26 02:30:56 PDT
Comment on attachment 165757 [details]
Patch 3

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

> Source/WebKit/chromium/ChangeLog:15
> +        (WebCore::dateFormatHelpLetters): Added for DateInputType, DateTimeInputType, and DateTimeLocalInputType to provide threeletters, e.g. "ymd" for English, for indicating order of day, month and year fields to display them intead of "-" when field has empty value.

threeletters -> three letters?
Is "年" one letter? Should we count a character of which code point is greater than U+FFFF as one letter? How about Unicode composition?

> Source/WebKit/chromium/ChangeLog:16
> +        (WebCore::dateFormatHelpText): Added for DateInputType, DateTimeInputType, and DateTimeLocalInputType to provide help text of order of day, month and year fields.

still unclear.  In what situation a user will see this text? What text is expected?
Comment 9 yosin 2012-09-26 02:56:38 PDT
Created attachment 165762 [details]
Patch 4
Comment 10 yosin 2012-09-26 02:58:00 PDT
Comment on attachment 165762 [details]
Patch 4

Could you review this patch?
Thanks in advance.

= Changes since the last review =
* Add more explanations to new functions in ChangeLog.
Comment 11 Kent Tamura 2012-09-26 04:09:37 PDT
Comment on attachment 165762 [details]
Patch 4

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

> Source/WebKit/chromium/ChangeLog:15
> +        (WebCore::dateFormatHelpLetters): Added for DateInputType, DateTimeInputType, and DateTimeLocalInputType to provide three letters, e.g. "ymd" for English, for indicating order of day, month and year fields to display them intead of "-" when field has empty value. Each letter should have same width to "-" in monospace font-family. This function may return empty string if there are no such letters in locale or order of day, month and year is well-known in locale.

This doesn't resolve my previous question.  is a sequence of 'A' and U+0300 (COMBINING GRAVE ACCENT) one letter?  Why don't you support at most two letters for one field though a day/month/year field has at least two digits?

BTW, these explanation should be in LocalizedStrings.h and WebCore/ChangeLog, not WebKit/chromium/ChangeLog.

> Source/WebKit/chromium/ChangeLog:16
> +        (WebCore::dateFormatHelpText): Added for DateInputType, DateTimeInputType, and DateTimeLocalInputType to provide help text of order of day, month and year fields, e.g. "Day/Month/Year". This string will be displayed bubble when the input element is focused and dateFormatHelpLetters() returns empty string. This function may return empty string if order of day, month, and year is well-known in locale.

We already have WebCore::localizedDateFormatText().  Also, a date format depends on a local machine setting, and we need some code to build this text.  Such text is not suitable for LocalizedStrings.
Comment 12 yosin 2012-09-26 22:01:53 PDT
Created attachment 165926 [details]
Patch 5
Comment 13 yosin 2012-09-26 22:03:45 PDT
Comment on attachment 165926 [details]
Patch 5

Could you review this patch?
Thanks in advance.

= Changes since the last review =
* Use placeholderFor{DayOfMonth,Month,Year}() instead of dateFormatHelp{Letters,Text}() for simplicity of implementation and ease of localization.
Comment 14 Kent Tamura 2012-09-26 22:18:14 PDT
Did you read my comment #11?

> BTW, these explanation should be in LocalizedStrings.h and WebCore/ChangeLog, not WebKit/chromium/ChangeLog.
Comment 15 yosin 2012-09-26 22:39:27 PDT
Created attachment 165930 [details]
Patch 6
Comment 16 yosin 2012-09-26 22:40:52 PDT
Comment on attachment 165930 [details]
Patch 6

Could you review this patch?
Thanks in advance.

= Changes since the last review =
* Move explanation of new functions into WebCore/platform/LocalizedString.h and WebCore/ChangeLog from WebKit/chromium/ChangeLog
Comment 17 Kent Tamura 2012-09-26 22:43:44 PDT
Comment on attachment 165930 [details]
Patch 6

ok
Comment 18 yosin 2012-09-26 22:46:40 PDT
Comment on attachment 165930 [details]
Patch 6

Clearing flags on attachment: 165930

Committed r129728: <http://trac.webkit.org/changeset/129728>
Comment 19 yosin 2012-09-26 22:46:45 PDT
All reviewed patches have been landed.  Closing bug.
Comment 20 yosin 2012-09-28 03:20:49 PDT
See http://crbug.com/152632 for Chromium side changes.