Bug 96085 – Datalist UI for input type=date for Chromium port

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

[Keishi Hattori]

Implement datalist UI for input type=date with page popup for chromium.

[Keishi Hattori]

Created attachment 162723 [details]
Patch

[Kent Tamura]

Comment on attachment 162723 [details]
Patch

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

> Source/WebCore/ChangeLog:69
> [phone]  Keishi Hattori  <[e-mail]>

Two ChangeLog entries

> Source/WebCore/Resources/pagepopups/pickerCommon.js:53
> +    console.log(width + "-" + height);

Remove debug code

> Source/WebCore/html/DateInputType.cpp:148
> +    Document* document = element()->document();
> +    RefPtr<RenderTheme> theme = document->page() ? document->page()->theme() : RenderTheme::defaultTheme();
> +    if (theme->f4OpensDateTimeChooser() && event->keyIdentifier() == "F4") {
> +        if (m_pickerElement)
> +            m_pickerElement->openPopup();
> +        event->setDefaultHandled();
> +        return;
> +    }

Opening by F4 should be a separated patch.

> Source/WebCore/rendering/RenderTheme.h:239
> +    virtual bool f4OpensDateTimeChooser() const;

The function name looks really strange.  How about opensDateTimeChooserByF4Key()?

> Source/WebKit/chromium/src/DateTimeChooserImpl.cpp:115
> +        addProperty("showOtherDateEntry", m_parameters.type == "date", writer);

Use InputTypeNames::date()

BTW, I think we had better split InputTypeNames out from InputType.{cpp.h} because InputType is internal class for HTMLInputElement.

> Source/WebKit/chromium/src/DateTimeChooserImpl.cpp:116
> +        addProperty("otherDateLabel", Platform::current()->queryLocalizedString(WebLocalizedString::OtherColorLabel), writer);

Using Other*Color*Label() is not appropriate.

> LayoutTests/ChangeLog:12
> [phone]  Keishi Hattori  <[e-mail]>

Two ChangeLog entries

[Keishi Hattori]

Created attachment 164709 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Kent Tamura]

Comment on attachment 164709 [details]
Patch

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

I agree with the basic structure of this patch. However it's too large to reivew.  Let's split into multiple pieces.

For example,
1. Preparation in calendarPicker.{css.js}
  Adding .calendar-picker, rename validateArguments to CalendarPicker.validateConfig, adding CalendarPicker.cleanup(), etc.

2. Adding DateTimeChooserParameters::localizedSuggestionValues, and <datalist> support in CalendarPickerElement::openPopup().

3. Update DateTimeChooserImpl.cpp and WebLocalizedString.h

4. suggestionPicker.{css.js} and remaining JavaScript changes

5. Enable <datalist> support for date type in RenderThemeChromiumCommon.cpp without hilightFirstSuggestionEntry feature , and adding tests.

6. Adding hilightFirstSuggestionEntry feature.

> Source/WebCore/html/DateInputType.cpp:-147
> -    if (m_pickerElement)
> -        m_pickerElement->closePopup();
> -

Don't change the existing behavior for a test crash.
You should fix the testing framework, or probably you can avoid the crash by opening the picker without focus.  See fast/forms/date/calendar-picker-appearance.html.

[Keishi Hattori]

Created attachment 165546 [details]
Patch