Bug 98240 – Implement DataList UI for input type time on chromium

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

[Keishi Hattori]

Implement DataList UI for input type time on chromium

[Keishi Hattori]

Created attachment 166825 [details]
Patch

[Kent Tamura]

Comment on attachment 166825 [details]
Patch

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

> Source/WebCore/css/html.css:573
> +    display: -webkit-box;
> +    -webkit-box-align: center;

nit:
Can you use the standard flex box instead of the legacy one?

  display: -webkit-flex;
  -webkit-align-content: center;

I thinks this works.

> Source/WebCore/html/BaseMultipleFieldsDateAndTimeInputType.cpp:96
>      , m_dateTimeEditElement(0)
> +    , m_pickerIndicatorIsVisible(false)

should initialize m_pickerIndicatorElement.

> Source/WebCore/html/BaseMultipleFieldsDateAndTimeInputType.cpp:118
> +{    

trailing whitespace

> Source/WebCore/html/BaseMultipleFieldsDateAndTimeInputType.cpp:119
> +    DEFINE_STATIC_LOCAL(AtomicString, dateAndTimeInputContainerPseudoId, ("-webkit-date-and-time-container"));

should be ("-webkit-date-and-time-container", AtomicString::ConstructFromLiteral)

> Source/WebCore/html/BaseMultipleFieldsDateAndTimeInputType.cpp:275
> +    HTMLDataListElement* dataList = element()->dataList();
> +    if (dataList) {

nit:
if (HTMLDataListElement* dataList = element()->dataList()) {

> Source/WebCore/html/BaseMultipleFieldsDateAndTimeInputType.cpp:292
> +    m_pickerIndicatorElement->setInlineStyleProperty(CSSPropertyDisplay, CSSValueNone);

nice to have ASSERT(m_pickerIndicatorElement) ?

> Source/WebCore/html/BaseMultipleFieldsDateAndTimeInputType.cpp:300
> +    m_pickerIndicatorElement->removeInlineStyleProperty(CSSPropertyDisplay);

ditto.

> LayoutTests/ChangeLog:32
> +        * platform/chromium/fast/forms/time/time-suggestion-picker-appearance-expected.txt: Added.
> +        * platform/chromium/fast/forms/time/time-suggestion-picker-appearance-rtl-expected.txt: Added.
> +        * platform/chromium/fast/forms/time/time-suggestion-picker-appearance-rtl.html: Added.
> +        * platform/chromium/fast/forms/time/time-suggestion-picker-appearance-with-scroll-bar-expected.txt: Added.
> +        * platform/chromium/fast/forms/time/time-suggestion-picker-appearance-with-scroll-bar.html: Added.
> +        * platform/chromium/fast/forms/time/time-suggestion-picker-appearance.html: Added.
> +        * platform/chromium/fast/forms/time/time-suggestion-picker-key-operations-expected.txt: Added.
> +        * platform/chromium/fast/forms/time/time-suggestion-picker-key-operations.html: Added.
> +        * platform/chromium/fast/forms/time/time-suggestion-picker-mouse-operations-expected.txt: Added.
> +        * platform/chromium/fast/forms/time/time-suggestion-picker-mouse-operations.html: Added.

should update chromium/TestExpectations for new tests on non-Mac.

> LayoutTests/fast/forms/time-multiple-fields/time-multiple-fields-spinbutton-click-in-iframe.html:5
> +<script src="../resources/common.js"></script>

Why did you add common.js to this?

> LayoutTests/platform/chromium/fast/forms/time/time-suggestion-picker-appearance-rtl.html:31
> +function sendKey(input, keyName, ctrlKey, altKey) {
> +    var event = document.createEvent('KeyboardEvent');
> +    event.initKeyboardEvent('keydown', true, true, document.defaultView, keyName, 0, ctrlKey, altKey);
> +    input.dispatchEvent(event);
> +}

suggestion-picker-common.js has it.

> LayoutTests/platform/chromium/fast/forms/time/time-suggestion-picker-appearance-with-scroll-bar.html:32
> +function sendKey(input, keyName, ctrlKey, altKey) {
> +    var event = document.createEvent('KeyboardEvent');
> +    event.initKeyboardEvent('keydown', true, true, document.defaultView, keyName, 0, ctrlKey, altKey);
> +    input.dispatchEvent(event);
> +}

ditto.

> LayoutTests/platform/chromium/fast/forms/time/time-suggestion-picker-appearance.html:9
> +    <option>01:02</option>

We should have some option values with seconds, milliseconds, afternoon times.

> LayoutTests/platform/chromium/fast/forms/time/time-suggestion-picker-appearance.html:34
> +function sendKey(input, keyName, ctrlKey, altKey) {
> +    var event = document.createEvent('KeyboardEvent');
> +    event.initKeyboardEvent('keydown', true, true, document.defaultView, keyName, 0, ctrlKey, altKey);
> +    input.dispatchEvent(event);
> +}

suggestion-picker-common.js has it.

[Keishi Hattori]

Created attachment 166859 [details]
Patch

[Kent Tamura]

Comment on attachment 166859 [details]
Patch

Please update LayoutTests/platform/chromium/TestExpectations so that all of new tests are expected to fail on all platforms including Mac. Your Bug 98237 will update the image results of these tests.

[Keishi Hattori]

Created attachment 166863 [details]
Patch

[Keishi Hattori]

Created attachment 166865 [details]
Patch

[WebKit Review Bot]

Comment on attachment 166865 [details]
Patch

Rejecting attachment 166865 [details] from commit-queue.

New failing tests:
fast/forms/datalist/input-list.html
platform/chromium/fast/forms/date/date-suggestion-picker-mouse-operations.html
fast/forms/number/number-spinbutton-click-in-iframe.html
Full output: http://queues.webkit.org/results/14136611

[WebKit Review Bot]

Comment on attachment 166865 [details]
Patch

Attachment 166865 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14125792

New failing tests:
fast/forms/datalist/input-list.html
platform/chromium/fast/forms/date/date-suggestion-picker-mouse-operations.html
fast/forms/number/number-spinbutton-click-in-iframe.html

[Keishi Hattori]

Created attachment 166890 [details]
Patch

[WebKit Review Bot]

Comment on attachment 166890 [details]
Patch

Clearing flags on attachment: 166890

Committed r130293: <http://trac.webkit.org/changeset/130293>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.