Bug 101889 – Enable calendar picker for input types datetime/datetime-local

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

[Kunihiko Sakamoto]

Implement calendar picker support for input type=datetime/datetime-local.

[Kunihiko Sakamoto]

Created attachment 173568 [details]
Patch

[Kent Tamura]

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

[Kent Tamura]

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.

[Kent Tamura]

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

[Kunihiko Sakamoto]

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

[Kunihiko Sakamoto]

Created attachment 174064 [details]
Patch 2

[Kunihiko Sakamoto]

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.

[Kent Tamura]

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.

[Kunihiko Sakamoto]

Created attachment 174071 [details]
Patch 3

[Kunihiko Sakamoto]

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.

[Kent Tamura]

Comment on attachment 174071 [details]
Patch 3

ok

[WebKit Review Bot]

Comment on attachment 174071 [details]
Patch 3

Clearing flags on attachment: 174071

Committed r134546: <http://trac.webkit.org/changeset/134546>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.