Bug 96085

Summary: Datalist UI for input type=date for Chromium port
Product: WebKit Reporter: Keishi Hattori <keishi>
Component: FormsAssignee: Keishi Hattori <keishi>
Status: RESOLVED FIXED    
Severity: Normal CC: eric, mifenton, tkent, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 97193, 97200, 97201, 97292, 97541, 97551    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Keishi Hattori 2012-09-07 02:27:50 PDT
Implement datalist UI for input type=date with page popup for chromium.
Comment 1 Keishi Hattori 2012-09-07 03:02:50 PDT
Created attachment 162723 [details]
Patch
Comment 2 Kent Tamura 2012-09-07 03:20:26 PDT
Comment on attachment 162723 [details]
Patch

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

> Source/WebCore/ChangeLog:69
> +2012-09-05  Keishi Hattori  <keishi@webkit.org>

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
> +2012-09-05  Keishi Hattori  <keishi@webkit.org>

Two ChangeLog entries
Comment 3 Keishi Hattori 2012-09-19 04:25:38 PDT
Created attachment 164709 [details]
Patch
Comment 4 WebKit Review Bot 2012-09-19 04:29:27 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 5 Kent Tamura 2012-09-19 07:27:16 PDT
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.
Comment 6 Keishi Hattori 2012-09-25 00:55:56 PDT
Created attachment 165546 [details]
Patch