Bug 101889 - Enable calendar picker for input types datetime/datetime-local
Summary: Enable calendar picker for input types datetime/datetime-local
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 101913 101916
Blocks: 29359
  Show dependency treegraph
 
Reported: 2012-11-11 21:50 PST by Kunihiko Sakamoto
Modified: 2012-11-13 22:30 PST (History)
4 users (show)

See Also:


Attachments
Patch (42.92 KB, patch)
2012-11-11 23:23 PST, Kunihiko Sakamoto
no flags Details | Formatted Diff | Diff
Patch 2 (15.85 KB, patch)
2012-11-13 21:00 PST, Kunihiko Sakamoto
no flags Details | Formatted Diff | Diff
Patch 3 (15.84 KB, patch)
2012-11-13 21:35 PST, Kunihiko Sakamoto
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kunihiko Sakamoto 2012-11-11 21:50:59 PST
Implement calendar picker support for input type=datetime/datetime-local.
Comment 1 Kunihiko Sakamoto 2012-11-11 23:23:30 PST
Created attachment 173568 [details]
Patch
Comment 2 Kent Tamura 2012-11-12 00:24:54 PST
Comment on attachment 173568 [details]
Patch

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

> Source/WebCore/html/shadow/DateTimeEditElement.cpp:487
> +void DateTimeEditElement::setValueFromCalendarPicker(const DateComponents& date)

nit: It's not a good name because it doesn't represent what the function does.  My proposal is setOnlyYearMonthDay.

> Source/WebCore/html/shadow/DateTimeEditElement.cpp:498
> +    setValueAsDateTimeFieldsState(dateTimeFieldsState);

Can you avoid to change the setValueAsDateTimeFieldsState signature?
I think passing a DateComponents for the old value as dateForReadOnlyField argument works well though I'm not 100% sure.

> Source/WebCore/html/shadow/PickerIndicatorElement.h:53
> +    class PickerIndicatorOwner {
> +    public:
> +        virtual ~PickerIndicatorOwner() { }
> +        virtual void pickerIndicatorChooseValue(const String&) = 0;
> +    };

Introducing PickerIndicatorOwner interface is nice.  However it's not great that it has only one function.
If you'd like to introduce PickerIndicatorOwner, you should remove HTMLInputElement (hostInput()) dependency from PickerIndicatorElement, and PickerIndicatorOwner should replace the role of HTMLInputElement in PickerIndicatorElement.  Such change should be done *before* this bug.

> Source/WebCore/rendering/RenderThemeChromiumCommon.cpp:56
>      return type == InputTypeNames::date()
>          || type == InputTypeNames::month()
> -        || type == InputTypeNames::week();
> +        || type == InputTypeNames::week()
> +        || type == InputTypeNames::datetime()
> +        || type == InputTypeNames::datetimelocal();

nit: please sort in alphabetical order
Comment 3 Kent Tamura 2012-11-12 00:53:40 PST
Comment on attachment 173568 [details]
Patch

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

>> Source/WebCore/html/shadow/DateTimeEditElement.cpp:498
>> +    setValueAsDateTimeFieldsState(dateTimeFieldsState);
> 
> Can you avoid to change the setValueAsDateTimeFieldsState signature?
> I think passing a DateComponents for the old value as dateForReadOnlyField argument works well though I'm not 100% sure.

Hmm, It won't work if there is a read-only field and fields has empty values.
Comment 4 Kent Tamura 2012-11-12 01:02:15 PST
Comment on attachment 173568 [details]
Patch

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

>>> Source/WebCore/html/shadow/DateTimeEditElement.cpp:498
>>> +    setValueAsDateTimeFieldsState(dateTimeFieldsState);
>> 
>> Can you avoid to change the setValueAsDateTimeFieldsState signature?
>> I think passing a DateComponents for the old value as dateForReadOnlyField argument works well though I'm not 100% sure.
> 
> Hmm, It won't work if there is a read-only field and fields has empty values.

The logic same as BaseMultipleFieldsDateAndTimeInputType::updateInnerTextValue should work.
i.e. Use the previous value for dateForReadOnlyField if it's not empty, otherwise use StepRange::minimum().
Comment 5 Kunihiko Sakamoto 2012-11-12 02:23:36 PST
Comment on attachment 173568 [details]
Patch

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

>> Source/WebCore/html/shadow/PickerIndicatorElement.h:53
>> +    };
> 
> Introducing PickerIndicatorOwner interface is nice.  However it's not great that it has only one function.
> If you'd like to introduce PickerIndicatorOwner, you should remove HTMLInputElement (hostInput()) dependency from PickerIndicatorElement, and PickerIndicatorOwner should replace the role of HTMLInputElement in PickerIndicatorElement.  Such change should be done *before* this bug.

OK, I will do that first. I've filed another bug for this:
https://bugs.webkit.org/show_bug.cgi?id=101913
Comment 6 Kunihiko Sakamoto 2012-11-13 21:00:57 PST
Created attachment 174064 [details]
Patch 2
Comment 7 Kunihiko Sakamoto 2012-11-13 21:03:44 PST
Comment on attachment 173568 [details]
Patch

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

>> Source/WebCore/html/shadow/DateTimeEditElement.cpp:487
>> +void DateTimeEditElement::setValueFromCalendarPicker(const DateComponents& date)
> 
> nit: It's not a good name because it doesn't represent what the function does.  My proposal is setOnlyYearMonthDay.

Renamed it to setOnlyYearMonthDay.

>>>> Source/WebCore/html/shadow/DateTimeEditElement.cpp:498
>>>> +    setValueAsDateTimeFieldsState(dateTimeFieldsState);
>>> 
>>> Can you avoid to change the setValueAsDateTimeFieldsState signature?
>>> I think passing a DateComponents for the old value as dateForReadOnlyField argument works well though I'm not 100% sure.
>> 
>> Hmm, It won't work if there is a read-only field and fields has empty values.
> 
> The logic same as BaseMultipleFieldsDateAndTimeInputType::updateInnerTextValue should work.
> i.e. Use the previous value for dateForReadOnlyField if it's not empty, otherwise use StepRange::minimum().

Done.
Thanks to your refactoring, now setValueAsDateTimeFieldsState does not require dateForReadOnlyField.

>> Source/WebCore/rendering/RenderThemeChromiumCommon.cpp:56
>> +        || type == InputTypeNames::datetimelocal();
> 
> nit: please sort in alphabetical order

Done.
Comment 8 Kent Tamura 2012-11-13 21:06:24 PST
Comment on attachment 174064 [details]
Patch 2

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

> LayoutTests/ChangeLog:12
> +        * platform/chromium/fast/forms/calendar-picker/calendar-picker-datetime-local-expected.txt: Added.
> +        * platform/chromium/fast/forms/calendar-picker/calendar-picker-datetime-local.html: Added.

nit: Please rename calendar-picker-datetime-local.html to calendar-picker-datetimelocal.html.  We omit '-' of datetime-local for test names.
Comment 9 Kunihiko Sakamoto 2012-11-13 21:35:50 PST
Created attachment 174071 [details]
Patch 3
Comment 10 Kunihiko Sakamoto 2012-11-13 21:36:29 PST
Comment on attachment 174064 [details]
Patch 2

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

>> LayoutTests/ChangeLog:12
>> +        * platform/chromium/fast/forms/calendar-picker/calendar-picker-datetime-local.html: Added.
> 
> nit: Please rename calendar-picker-datetime-local.html to calendar-picker-datetimelocal.html.  We omit '-' of datetime-local for test names.

Done.
Comment 11 Kent Tamura 2012-11-13 21:37:43 PST
Comment on attachment 174071 [details]
Patch 3

ok
Comment 12 WebKit Review Bot 2012-11-13 22:30:36 PST
Comment on attachment 174071 [details]
Patch 3

Clearing flags on attachment: 174071

Committed r134546: <http://trac.webkit.org/changeset/134546>
Comment 13 WebKit Review Bot 2012-11-13 22:30:40 PST
All reviewed patches have been landed.  Closing bug.