Bug 110431 - INPUT_MULTIPLE_FIELDS_UI: Unable to enter "24" to hour field
Summary: INPUT_MULTIPLE_FIELDS_UI: Unable to enter "24" to hour field
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-20 22:07 PST by Kunihiko Sakamoto
Modified: 2013-02-22 12:20 PST (History)
6 users (show)

See Also:


Attachments
Patch (17.30 KB, patch)
2013-02-21 03:50 PST, Kunihiko Sakamoto
no flags Details | Formatted Diff | Diff
Patch 2 (11.79 KB, patch)
2013-02-21 22:31 PST, Kunihiko Sakamoto
no flags Details | Formatted Diff | Diff
Patch 3 (11.77 KB, patch)
2013-02-21 23:20 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-20 22:07:55 PST
http://crbug.com/177405
When hour format is 1-24, hour field does not accept input "24".
Comment 1 Kunihiko Sakamoto 2013-02-21 03:50:40 PST
Created attachment 189503 [details]
Patch
Comment 2 Kunihiko Sakamoto 2013-02-21 03:52:45 PST
Hattori-san, could you please give an informal review for this patch?
Comment 3 Keishi Hattori 2013-02-21 04:28:33 PST
Comment on attachment 189503 [details]
Patch

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

> Source/WebCore/html/HTMLInputElement.cpp:1571
> +    if (m_inputType->isDateField() || m_inputType->isTimeField() || m_inputType->isDateTimeField() || m_inputType->isDateTimeLocalField())

I think we decided not to branch depending on input types in HTMLInputElement.cpp. setDateTimeFormat should be added to InputType.h as a virtual method.
Comment 4 Kent Tamura 2013-02-21 05:40:07 PST
Comment on attachment 189503 [details]
Patch

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

I have another idea.
How about adding pattern attribute to DateTimeEditElement?
In this case, we don't need to add an Internals method, and use getElementByPseudoId(internals.youngestShadowRoot(input), "-webkit-datetime-edit").setAttribute("pattern", "KK:mm a") (then doing something invoking updateInnerTextValue()). Also, we don't need to add extra data member for date/time format.

>> Source/WebCore/html/HTMLInputElement.cpp:1571
>> +    if (m_inputType->isDateField() || m_inputType->isTimeField() || m_inputType->isDateTimeField() || m_inputType->isDateTimeLocalField())
> 
> I think we decided not to branch depending on input types in HTMLInputElement.cpp. setDateTimeFormat should be added to InputType.h as a virtual method.

Keishi is right.
Comment 5 Kunihiko Sakamoto 2013-02-21 22:31:53 PST
Created attachment 189686 [details]
Patch 2
Comment 6 Kunihiko Sakamoto 2013-02-21 22:33:12 PST
Please take another look.

(In reply to comment #4)
> How about adding pattern attribute to DateTimeEditElement?
That's a good idea! Done.
Comment 7 Kent Tamura 2013-02-21 23:08:10 PST
Comment on attachment 189686 [details]
Patch 2

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

> Source/WebCore/html/BaseMultipleFieldsDateAndTimeInputType.cpp:376
> +    const AtomicString pattern = m_dateTimeEditElement->getAttribute(HTMLNames::patternAttr);

getAttribute should be fastGetAttribute because it is not a "style" attribute.
You can remove HTMLNames::.

> Source/WebCore/html/BaseMultipleFieldsDateAndTimeInputType.cpp:377
> +    if (!pattern.isNull() && !pattern.isEmpty())

isNull() check is unnecessary. isEmpty means "null or 0-length"
Comment 8 Kunihiko Sakamoto 2013-02-21 23:20:32 PST
Created attachment 189693 [details]
Patch 3
Comment 9 Kunihiko Sakamoto 2013-02-21 23:25:04 PST
Comment on attachment 189686 [details]
Patch 2

Thank you for reviewing during your absence on leave.

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

>> Source/WebCore/html/BaseMultipleFieldsDateAndTimeInputType.cpp:376
>> +    const AtomicString pattern = m_dateTimeEditElement->getAttribute(HTMLNames::patternAttr);
> 
> getAttribute should be fastGetAttribute because it is not a "style" attribute.
> You can remove HTMLNames::.

Changed to fastGetAttribute.
HTMLNames:: is necessary here - or should I add "using namespace HTMLNames" at the top?

>> Source/WebCore/html/BaseMultipleFieldsDateAndTimeInputType.cpp:377
>> +    if (!pattern.isNull() && !pattern.isEmpty())
> 
> isNull() check is unnecessary. isEmpty means "null or 0-length"

Done.
Comment 10 Kent Tamura 2013-02-22 04:08:08 PST
Comment on attachment 189686 [details]
Patch 2

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

>>> Source/WebCore/html/BaseMultipleFieldsDateAndTimeInputType.cpp:376
>>> +    const AtomicString pattern = m_dateTimeEditElement->getAttribute(HTMLNames::patternAttr);
>> 
>> getAttribute should be fastGetAttribute because it is not a "style" attribute.
>> You can remove HTMLNames::.
> 
> Changed to fastGetAttribute.
> HTMLNames:: is necessary here - or should I add "using namespace HTMLNames" at the top?

We usually declare "using namespace foo" in an implementation file, and omit the namespace prefix, except std::.  This is not mandatory.
Comment 11 WebKit Review Bot 2013-02-22 04:37:58 PST
Comment on attachment 189693 [details]
Patch 3

Rejecting attachment 189693 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=gce-cq-03', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 189693, '--port=chromium-xvfb']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue

Last 500 characters of output:

fatal: read error: Connection reset by peer
Died at Tools/Scripts/update-webkit line 151.

Failed to run "['Tools/Scripts/update-webkit', '--chromium', '--force-update']" exit_code: 2

Updating OpenSource
fatal: read error: Connection reset by peer
Died at Tools/Scripts/update-webkit line 151.

Failed to run "['Tools/Scripts/update-webkit', '--chromium', '--force-update']" exit_code: 2
Updating OpenSource
fatal: read error: Connection reset by peer
Died at Tools/Scripts/update-webkit line 151.

Full output: http://queues.webkit.org/results/16694798
Comment 12 WebKit Review Bot 2013-02-22 12:20:23 PST
Comment on attachment 189693 [details]
Patch 3

Clearing flags on attachment: 189693

Committed r143770: <http://trac.webkit.org/changeset/143770>
Comment 13 WebKit Review Bot 2013-02-22 12:20:27 PST
All reviewed patches have been landed.  Closing bug.