[Webkit-unassigned] [Bug 109555] INPUT_MULTIPLE_FIELDS_UI: Step-up/-down of hour field should respect min/max attributes

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=109555


Kent Tamura <tkent at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #187813|review?                     |review-
               Flag|                            |




--- Comment #2 from Kent Tamura <tkent at chromium.org>  2013-02-12 18:34:56 PST ---
(From update of attachment 187813)
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").

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.