Bug 109555 – INPUT_MULTIPLE_FIELDS_UI: Step-up/-down of hour field should respect min/max attributes

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

[Kunihiko Sakamoto]

Use min/max attributes of <input type=time> and <input type=datetime-local> to limit values selectable by step-up/down.

[Kunihiko Sakamoto]

Created attachment 187813 [details]
Patch

[Kent Tamura]

Comment on attachment 187813 [details]
Patch

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

> Source/WebCore/html/shadow/DateTimeEditElement.cpp:161
>          DateTimeNumericFieldElement::Parameters parameters = createNumericFieldParameters(static_cast<int>(msPerHour), static_cast<int>(msPerHour * 12));
> -        RefPtr<DateTimeFieldElement> field = DateTimeHourFieldElement::create(document, m_editElement, 0, 11, parameters);
> +        int minimumHour23, maximumHour23;
> +        getRangeOfHourField(minimumHour23, maximumHour23);
> +        RefPtr<DateTimeFieldElement> field = DateTimeHour11FieldElement::create(document, m_editElement, minimumHour23, maximumHour23, parameters);

How about introducing a subclass of DateTimeNumericFieldElement::Parameters to add minimum/maximum members?
Repeating the same code four times looks not good.

> Source/WebCore/html/shadow/DateTimeEditElement.cpp:163
>          if (shouldHourFieldDisabled()) {

This patch doesn't disable the field in a case of minimumHour23 == maximumHour23.
It's ok if you think it's out of scope.

> LayoutTests/fast/forms/time-multiple-fields/time-multiple-fields-stepup-stepdown-from-renderer.html:175
> +shouldBeEqualToString('stepDown("15:00", 1, "13:00", "13:00")', '13:00');
> +shouldBeEqualToString('stepUp("12:00", 1, "12:00", "15:00")', '13:00');
> +shouldBeEqualToString('stepDown("12:00", 1, "12:00", "15:00")', '23:00');
> +shouldBeEqualToString('stepUp("15:00", 1, "12:00", "15:00")', '16:00');

We should have tests with
 - max="12:00" 
 - min="00:00"
 - min is in the morning, and max is in the afternoon.  e.g. min="10:00" max="15:00"

> LayoutTests/fast/forms/time-multiple-fields/time-multiple-fields-stepup-stepdown-from-renderer.html:182
>  debug('Hours, 0-23');

I have a concern about test coverage. The test doesn't cover hour11 and hour24 cases.

It seems there are no locales with these hour formats in ICU and Windows. However we need support them because users can set these formats in OSX.
Ideally, we had better introduce test-only function to set date/time format.  It would be something like window.internals.setDateTimeFormat(input, "kk:mm").

[Kunihiko Sakamoto]

Created attachment 190473 [details]
Patch 2

[Kunihiko Sakamoto]

Comment on attachment 187813 [details]
Patch

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

>> Source/WebCore/html/shadow/DateTimeEditElement.cpp:161
>> +        RefPtr<DateTimeFieldElement> field = DateTimeHour11FieldElement::create(document, m_editElement, minimumHour23, maximumHour23, parameters);
> 
> How about introducing a subclass of DateTimeNumericFieldElement::Parameters to add minimum/maximum members?
> Repeating the same code four times looks not good.

Because minHour/maxHour are used in several places (e.g. am/pm field uses them too), I have made them data fields of DateTimeEditBuilder.
I think minimum and maximum should be included in DateTimeNumericFieldElement. I'd like to do it later.

>> Source/WebCore/html/shadow/DateTimeEditElement.cpp:163
>>          if (shouldHourFieldDisabled()) {
> 
> This patch doesn't disable the field in a case of minimumHour23 == maximumHour23.
> It's ok if you think it's out of scope.

Changed to disable Hour/AMPM fields when possible.

>> LayoutTests/fast/forms/time-multiple-fields/time-multiple-fields-stepup-stepdown-from-renderer.html:175
>> +shouldBeEqualToString('stepUp("15:00", 1, "12:00", "15:00")', '16:00');
> 
> We should have tests with
>  - max="12:00" 
>  - min="00:00"
>  - min is in the morning, and max is in the afternoon.  e.g. min="10:00" max="15:00"

Added.

>> LayoutTests/fast/forms/time-multiple-fields/time-multiple-fields-stepup-stepdown-from-renderer.html:182
>>  debug('Hours, 0-23');
> 
> I have a concern about test coverage. The test doesn't cover hour11 and hour24 cases.
> 
> It seems there are no locales with these hour formats in ICU and Windows. However we need support them because users can set these formats in OSX.
> Ideally, we had better introduce test-only function to set date/time format.  It would be something like window.internals.setDateTimeFormat(input, "kk:mm").

We added the pattern attribute to DateTimeEditElement in r143770; now we can test hour11/hour24.

[Kent Tamura]

Comment on attachment 190473 [details]
Patch 2

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

Almost good.

> Source/WebCore/html/shadow/DateTimeEditElement.cpp:80
> +    int m_minDay, m_maxDay, m_minHour23, m_maxHour23;

nit: We usually declare one data member per one line.

[Kunihiko Sakamoto]

Created attachment 190653 [details]
Patch 3

[Kunihiko Sakamoto]

Comment on attachment 190473 [details]
Patch 2

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

Thanks for the review.

>> Source/WebCore/html/shadow/DateTimeEditElement.cpp:80
>> +    int m_minDay, m_maxDay, m_minHour23, m_maxHour23;
> 
> nit: We usually declare one data member per one line.

Done.

[WebKit Review Bot]

Comment on attachment 190653 [details]
Patch 3

Clearing flags on attachment: 190653

Committed r144263: <http://trac.webkit.org/changeset/144263>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.