Bug 109555 - INPUT_MULTIPLE_FIELDS_UI: Step-up/-down of hour field should respect min/max attributes
Summary: INPUT_MULTIPLE_FIELDS_UI: Step-up/-down of hour field should respect min/max ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-02-12 02:17 PST by Kunihiko Sakamoto
Modified: 2013-02-27 22:46 PST (History)
5 users (show)

See Also:


Attachments
Patch (39.67 KB, patch)
2013-02-12 02:25 PST, Kunihiko Sakamoto
no flags Details | Formatted Diff | Diff
Patch 2 (69.64 KB, patch)
2013-02-27 02:06 PST, Kunihiko Sakamoto
no flags Details | Formatted Diff | Diff
Patch 3 (69.76 KB, patch)
2013-02-27 22:07 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 2013-02-12 02:17:46 PST
Use min/max attributes of <input type=time> and <input type=datetime-local> to limit values selectable by step-up/down.
Comment 1 Kunihiko Sakamoto 2013-02-12 02:25:59 PST
Created attachment 187813 [details]
Patch
Comment 2 Kent Tamura 2013-02-12 18:34:56 PST
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").
Comment 3 Kunihiko Sakamoto 2013-02-27 02:06:12 PST
Created attachment 190473 [details]
Patch 2
Comment 4 Kunihiko Sakamoto 2013-02-27 02:09:15 PST
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.
Comment 5 Kent Tamura 2013-02-27 21:17:19 PST
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.
Comment 6 Kunihiko Sakamoto 2013-02-27 22:07:06 PST
Created attachment 190653 [details]
Patch 3
Comment 7 Kunihiko Sakamoto 2013-02-27 22:08:00 PST
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.
Comment 8 WebKit Review Bot 2013-02-27 22:46:38 PST
Comment on attachment 190653 [details]
Patch 3

Clearing flags on attachment: 190653

Committed r144263: <http://trac.webkit.org/changeset/144263>
Comment 9 WebKit Review Bot 2013-02-27 22:46:42 PST
All reviewed patches have been landed.  Closing bug.