Bug 97997

Summary: [Forms] Multiple fields datetime/datetime-local input UI
Product: WebKit Reporter: yosin
Component: FormsAssignee: yosin
Status: RESOLVED FIXED    
Severity: Normal CC: cmarcelo, macpherson, menard, mifenton, tkent, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 97998, 98109, 98116, 98117, 98118, 98227    
Bug Blocks: 29359    
Attachments:
Description Flags
Patch 1
none
Patch 2 none

Description yosin 2012-10-01 00:30:39 PDT
Chromium port wants to have multiple fields week input UI like multiple fields time input UI.
Comment 1 yosin 2012-10-02 01:53:19 PDT
Created attachment 166643 [details]
Patch 1
Comment 2 yosin 2012-10-02 01:55:26 PDT
Comment on attachment 166643 [details]
Patch 1

Could you review this patch?
Thanks in advance.

P.S.
Bug 97299 has similar patch for input type "week".
Comment 3 Kent Tamura 2012-10-02 02:12:32 PDT
Comment on attachment 166643 [details]
Patch 1

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

> Source/WebCore/ChangeLog:12
> +        for multiple fields week input UI.

week -> datetime/datetime-local

> Source/WebCore/ChangeLog:18
> +        Note: Actual outputs of four tests
> +        - fast/forms/datetime/datetime-input-visible-string.html
> +        - fast/forms/datetime/datetime-stepup-stepdown-from-renderer.html
> +        - fast/forms/datetimelocal/datetimelocal-input-visible-string.html
> +        - fast/forms/datetimelocal/datetimelocal-stepup-stepdown-from-renderer.html

What do you mean by this paragraph?

> Source/WebCore/ChangeLog:33
> +        (WebCore::DateTimeLocalInputType::formatDateTimeFieldsState): Added to format numeric value to string value as specified in HTML5 specification.
> +        (WebCore::DateTimeLocalInputType::setupLayoutParameters):  Added to set layout of multiple fields.

These functions are very similar to DateTimeInputType functions.  Can you share them?

> Source/WebCore/html/DateTimeInputType.cpp:148
> +    builder.append(layoutParameters.localizer.dateFormat());
> +    builder.append(' ');

Looks not correct.  AFAIK, some locales use ', ' for the delimiter, and some locales need the reversed order.
Comment 4 yosin 2012-10-02 02:46:03 PDT
Created attachment 166650 [details]
Patch 2
Comment 5 yosin 2012-10-02 02:49:13 PDT
Comment on attachment 166650 [details]
Patch 2

Could you review this patch?
Thanks in advance.

= Changes since the last review =
* Update WebCore/Source/ChangeLog
* Add FIXME comments for code sharing code of formatDateTimeFieldsState()between DateTimeInputType and DateTimeLocalInputType 
* Add comments about date time format used in setupLayoutParameters in DateTimeInputType and DateTimeLocalInputType.
Comment 6 Kent Tamura 2012-10-02 02:52:55 PDT
Comment on attachment 166650 [details]
Patch 2

ok for now.
Comment 7 yosin 2012-10-02 03:21:21 PDT
Comment on attachment 166650 [details]
Patch 2

Clearing flags on attachment: 166650

Committed r130147: <http://trac.webkit.org/changeset/130147>
Comment 8 yosin 2012-10-02 03:21:26 PDT
All reviewed patches have been landed.  Closing bug.