Bug 110431 – INPUT_MULTIPLE_FIELDS_UI: Unable to enter "24" to hour field

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

[Kunihiko Sakamoto]

http://crbug.com/177405
When hour format is 1-24, hour field does not accept input "24".

[Kunihiko Sakamoto]

Created attachment 189503 [details]
Patch

[Kunihiko Sakamoto]

Hattori-san, could you please give an informal review for this patch?

[Keishi Hattori]

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.

[Kent Tamura]

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.

[Kunihiko Sakamoto]

Created attachment 189686 [details]
Patch 2

[Kunihiko Sakamoto]

Please take another look.

(In reply to comment #4)
> How about adding pattern attribute to DateTimeEditElement?
That's a good idea! Done.

[Kent Tamura]

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"

[Kunihiko Sakamoto]

Created attachment 189693 [details]
Patch 3

[Kunihiko Sakamoto]

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.

[Kent Tamura]

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.

[WebKit Review Bot]

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

[WebKit Review Bot]

Comment on attachment 189693 [details]
Patch 3

Clearing flags on attachment: 189693

Committed r143770: <http://trac.webkit.org/changeset/143770>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.