Implement datalist UI for input type color for Chromium

• Component: Forms
• Status: RESOLVED FIXED

[Keishi Hattori]

Implement datalist UI for input type color for Chromium

[Keishi Hattori]

Created attachment 153993 [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 153993 [details]
Patch

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

> Source/WebCore/ChangeLog:13
> +        * Resources/colorPicker.css: Added.

I don't think this should be called 'color picker'.  It's confusing with color chooser.
Maybe 'color suggestion picker' or something?

> Source/WebCore/ChangeLog:53
> +        * html/shadow/CalendarPickerElement.cpp:
> +        (WebCore::CalendarPickerElement::writeDocument): Modified to use PagePopupUtil.
> +        * page/PagePopupUtil.cpp: Added.
> +        (WebCore): Moved helper functions from CalendarPickerElement.cpp.
> +        (WebCore::PagePopupUtil::addJavaScriptString):
> +        (WebCore::PagePopupUtil::addProperty):
> +        * page/PagePopupUtil.h: Copied from Source/WebCore/html/ColorInputType.h.
> +        (WebCore):
> +        (PagePopupUtil):
> +        (WebCore::PagePopupUtil::addString):

These changes should be done in a separated patch.

> Source/WebKit/chromium/ChangeLog:53
> +        * src/WebPagePopupImpl.h:
> +        (WebKit::WebPagePopupImpl::hasSamePopupClient): Returns true if the page popup client
> +        is the same.
> +        * src/WebViewImpl.cpp:
> +        (WebKit::WebViewImpl::handleMouseDown): We don't want to reopen the same popup so we
> +        check hasSamePopupClient().

This should be done in another separated patch.

> Source/WebCore/Resources/colorPicker.css:14
> + *     * Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + *     * Redistributions in binary form must reproduce the above
> + * copyright notice, this list of conditions and the following disclaimer
> + * in the documentation and/or other materials provided with the
> + * distribution.
> + *     * Neither the name of Google Inc. nor the names of its

Please use two-clauses version.

> Source/WebCore/Resources/colorPicker.js:14
> + *     * Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + *     * Redistributions in binary form must reproduce the above
> + * copyright notice, this list of conditions and the following disclaimer
> + * in the documentation and/or other materials provided with the
> + * distribution.
> + *     * Neither the name of Google Inc. nor the names of its

ditto.

> Source/WebCore/Resources/colorPicker.js:51
> +function getScrollbarWidth() {
> +  if (scrollbarWidth === null) {
> +    var scrollDiv = document.createElement("div");

wrong indentation

> Source/WebCore/Resources/colorPicker.js:101
> +var DEFAULT_COLOR_PALETTE = ["#000000", "#404040", "#808080", "#c0c0c0", "#ffffff", "#980000",
> +"#ff0000", "#ff9900", "#ffff00", "#00ff00", "#00ffff", "#4a86e8", "#0000ff", "#9900ff",
> +"#ff00ff"];

The constant name should be CamelCase, right?
Need a comment about why DEFAULT_COLOR_PALETTE is needed.

> Source/WebCore/Resources/colorPicker.js:156
> +var SWATCH_SIZE = 24; // keep in sync with CSS
> +var SWATCHES_PER_ROW = 5;
> +var SWATCHES_MAX_ROWS = 4;

Should be CamelCase.

> Source/WebCore/Resources/colorPicker.js:170
> +    if (this.config.values.length > SWATCHES_PER_ROW * SWATCHES_MAX_ROWS)
> +      containerWidth += getScrollbarWidth();

wrong indentation.

> Source/WebCore/WebCore.gyp/WebCore.gyp:942
> +            '--condition=ENABLE(CALENDAR_PICKER)',

should be ENABLE(INPUT_TYPE_COLOR) && ENABLE(DATALIST_ELEMENT)

> Source/WebCore/html/ColorInputType.cpp:225
> +    HTMLDataListElement* dataList = static_cast<HTMLDataListElement*>(element()->list());

You might want to make HTMLInputElement::dataList() public.

> Source/WebCore/html/ColorInputType.cpp:228
> +        for (unsigned i = 0; HTMLOptionElement* option = static_cast<HTMLOptionElement*>(options->item(i)); i++)

We prefer ++i.

> Source/WebCore/page/PagePopupUtil.h:14
> + *     * Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + *     * Redistributions in binary form must reproduce the above
> + * copyright notice, this list of conditions and the following disclaimer
> + * in the documentation and/or other materials provided with the
> + * distribution.
> + *     * Neither the name of Google Inc. nor the names of its

Please use two-clauses version.

> Source/WebKit/chromium/src/ChromeClientImpl.cpp:1012
> -    return m_pagePopupDriver->openPagePopup(client, originBoundsInRootView);
> +    PagePopup* pp = m_pagePopupDriver->openPagePopup(client, originBoundsInRootView);
> +    return pp;

This looks unnecessary.

[Kent Tamura]

Also, I recommend to separate the patch to the WebCore part and the Chromium WebKit part.

[Kent Tamura]

Comment on attachment 153993 [details]
Patch

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

> Source/WebCore/page/PagePopupUtil.h:47
> +class PagePopupUtil {
> +public:
> +    static void addString(const String&, DocumentWriter&);
> +    static void addJavaScriptString(const String&, DocumentWriter&);
> +    static void addProperty(const char* name, const String& value, DocumentWriter&);
> +    static void addProperty(const char* name, unsigned value, DocumentWriter&);
> +    static void addProperty(const char* name, bool value, DocumentWriter&);
> +    static void addProperty(const char* name, const Vector<String>& values, DocumentWriter&);

You don't need to introduce new class.  These function can be protected static methods of PagePopupClient.

[Keishi Hattori]

Created attachment 154612 [details]
Patch

[WebKit Review Bot]

Comment on attachment 154612 [details]
Patch

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

[Kent Tamura]

Comment on attachment 154612 [details]
Patch

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

> Source/WebKit/chromium/ChangeLog:23
> +        (WebKit::ColorChooserUI::ColorChooserUI): Controls the UI for color chooser. Opens the color suggestion picker popup or color chooser depending on the ColorChooserClient.

The class name ending with 'UI' sounds strange though I don't have a counter proposal.

> Source/WebCore/platform/LocalizedStrings.h:224
> +#if ENABLE(INPUT_TYPE_COLOR)
> +    String otherColorLabel();
>  #endif

This function isn't used by WebCore.  We should remove it.
ColorChooserUI should call WebKit::Platform::current()->queryLocalizedString() directly.

> LayoutTests/ChangeLog:9
> +        * fast/forms/color/color-suggestion-picker-appearance.html: Added.

We need to update TestExpectations for non-Mac platforms.

[Keishi Hattori]

Created attachment 154822 [details]
Patch

[Kent Tamura]

Comment on attachment 154822 [details]
Patch

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

> Source/WebKit/chromium/ChangeLog:21
> +        * src/ColorChooserUIController.cpp: Added.

The patch doen't have this file.

> Source/WebCore/platform/LocalizedStrings.h:221
> -#endif
> +#endif    

Do not touch this file.

> LayoutTests/ChangeLog:9
> +        * fast/forms/color/color-suggestion-picker-appearance-expected.txt: Added. Tests color suggestion picker popup appearance.
> +        * fast/forms/color/color-suggestion-picker-appearance.html: Added.

Probably we had better move the test to platform/chromium/fast/forms/color/ because the color suggestion UI might be used only in Chromium.

> LayoutTests/platform/chromium/TestExpectations:3650
>  BUGWK92297 DEBUG : media/video-set-rate-from-pause.html = PASS CRASH
> +
> +// Needs rebaseline.
> +BUGWK92444 WIN LINUX : fast/forms/color/color-suggestion-picker-appearance.html = IMAGE

I don't recommend to append an entry at the bottom of this file.  It is very easy to cause a conflict.

[Keishi Hattori]

Created attachment 154834 [details]
Patch

[Kent Tamura]

Comment on attachment 154834 [details]
Patch

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

> Source/WebKit/chromium/WebKit.gyp:492
>                  'src/WebCache.cpp',
> -                'src/WebColorChooserClientImpl.cpp',
> -                'src/WebColorChooserClientImpl.h',
> +                'src/ColorChooserUIController.cpp',
> +                'src/ColorChooserUIController.h',
>                  'src/WebColorName.cpp',

Should be sorted.

> Source/WebKit/chromium/src/ChromeClientImpl.cpp:699
> +    ColorChooserUIController* colorChooserUI = new ColorChooserUIController(this, chooserClient);
> +    return adoptPtr(colorChooserUI);

The local variable 'colorChooserUI' is unnecessary.

> Source/WebKit/chromium/src/ChromeClientImpl.h:62
> +#if ENABLE(INPUT_TYPE_COLOR)
> +class WebColorChooser;
> +class WebColorChooserClient;
> +#endif

You don't need #if - #endif for class name declaration.

> Source/WebKit/chromium/src/ColorChooserUIController.cpp:48
> +enum ColorPickerPopupAction {
> +    ChooseOtherColorAction = -2,
> +    CancelAction = -1,
> +    SetValueAction = 0
> +};

Need a comment about synchronization with colorSuggestionPicker.js.

We usually use prefixes for enum symbols, not suffix.
  ColorPickerPopupActoinChooseOtherColor
  ColorPickerPopupActoinCancelAction
  ColorPickerPopupActoinSetValueAction

> Source/WebKit/chromium/src/ColorChooserUIController.cpp:69
> +    WebColor webColor = static_cast<WebColor>(color.rgb());
> +    m_chooser->setSelectedColor(webColor);

The local variable 'webColor' is unnecessary.

> Source/WebKit/chromium/src/ColorChooserUIController.h:45
> +class WebColorChooser;
> +class ChromeClientImpl;

should be sorted.

> Source/WebKit/chromium/src/ColorChooserUIController.h:56
> +    virtual void setSelectedColor(const WebCore::Color&) OVERRIDE;
> +    virtual void endChooser() OVERRIDE;
> +
> +    virtual void didChooseColor(const WebColor&) OVERRIDE;
> +    virtual void didEndChooser() OVERRIDE;

Would you add comments like "// *** functions:" like "PagePopupClient functions:" below?

> Source/WebKit/chromium/src/ColorChooserUIController.h:64
> +    void closePopup();

This should be private?

[Keishi Hattori]

Created attachment 154848 [details]
Patch

[Keishi Hattori]

Comment on attachment 154834 [details]
Patch

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

>> Source/WebKit/chromium/WebKit.gyp:492
>>                  'src/WebColorName.cpp',
> 
> Should be sorted.

Done.

>> Source/WebKit/chromium/src/ChromeClientImpl.cpp:699
>> +    return adoptPtr(colorChooserUI);
> 
> The local variable 'colorChooserUI' is unnecessary.

Done.

>> Source/WebKit/chromium/src/ChromeClientImpl.h:62
>> +#endif
> 
> You don't need #if - #endif for class name declaration.

Done.

>> Source/WebKit/chromium/src/ColorChooserUIController.cpp:48
>> +};
> 
> Need a comment about synchronization with colorSuggestionPicker.js.
> 
> We usually use prefixes for enum symbols, not suffix.
>   ColorPickerPopupActoinChooseOtherColor
>   ColorPickerPopupActoinCancelAction
>   ColorPickerPopupActoinSetValueAction

Done.

>> Source/WebKit/chromium/src/ColorChooserUIController.cpp:69
>> +    m_chooser->setSelectedColor(webColor);
> 
> The local variable 'webColor' is unnecessary.

Done.

>> Source/WebKit/chromium/src/ColorChooserUIController.h:45
>> +class ChromeClientImpl;
> 
> should be sorted.

Done.

>> Source/WebKit/chromium/src/ColorChooserUIController.h:56
>> +    virtual void didEndChooser() OVERRIDE;
> 
> Would you add comments like "// *** functions:" like "PagePopupClient functions:" below?

Done.

>> Source/WebKit/chromium/src/ColorChooserUIController.h:64
>> +    void closePopup();
> 
> This should be private?

Done.

[Keishi Hattori]

Chromium patch to add localized string:
https://chromiumcodereview.appspot.com/10824058/

[Kent Tamura]

Comment on attachment 154848 [details]
Patch

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

> Source/WebKit/chromium/src/ColorChooserUIController.cpp:138
> +    m_popup = m_chromeClient->openPagePopup(this, m_client->elementRectRelativeToWindow());

PagePopup API assumes the second argument is a rectangle relative to the RootView, not Window.  See WebCore/platform/ScrollView.h.

> Source/WebKit/chromium/src/ColorChooserUIController.cpp:152
> +    WebColor webColor = static_cast<WebColor>(m_client->currentColor().rgb());
> +    m_chooser = m_chromeClient->createWebColorChooser(this, webColor);

The local variable 'webColor' is unnecessary.

> Source/WebKit/chromium/src/ColorChooserUIController.h:35
> +#define ColorChooserUIController_h
> +
> +#include "ColorChooser.h"
> +#include "PagePopupClient.h"
> +#include "WebColorChooserClient.h"
> +#include <wtf/OwnPtr.h>
> +#include <wtf/PassOwnPtr.h>
> +
> +#if ENABLE(INPUT_TYPE_COLOR)

#if ENABLE(INPUT_TYPE_COLOR) should be put between #define ColorChooserUIController_h and #include "ColorChooser.h".

> LayoutTests/platform/chromium/TestExpectations:3474
> +BUGWK92444 : fast/forms/color/color-suggestion-picker-appearance.html = MISSING IMAGE

needs to prepend platform/chromium/

[Keishi Hattori]

Created attachment 154867 [details]
Patch

[Kent Tamura]

Comment on attachment 154867 [details]
Patch

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

> Source/WebCore/platform/ColorChooserClient.h:23
> +    virtual IntRect elementRectRelativeToRootView() const = 0;
>      virtual IntRect elementRectRelativeToWindow() const = 0;

Adding another similar API is not acceptable.

ColorChooserClient::elementRectRelativeToWindow() is not used yet, and Pierre Rossi is about to use it in Bug 91664.  You need to discuss this with him.

[Keishi Hattori]

Created attachment 154899 [details]
Patch

[Keishi Hattori]

I got Pierre Rossi's approval and removed elementRectRelativeToWindow.

[Kent Tamura]

Comment on attachment 154899 [details]
Patch

you should make a separated patch for the WebCore change.

[Keishi Hattori]

Created attachment 154911 [details]
Patch

[Kent Tamura]

Comment on attachment 154911 [details]
Patch

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

> Source/WebKit/chromium/src/ColorChooserUIController.cpp:95
> +    return WebCore::IntSize(0, 0);

It should have non-0 height.  If not, the popup might be invisible in a case that <input type=color> is at the bottom of the view.

> LayoutTests/platform/chromium/fast/forms/color/color-suggestion-picker-appearance.html:21
> +<datalist id=gray>
> +    <option>#ffffff</option>
> +    <option>#eeeeee</option>
> +    <option>#dddddd</option>
> +    <option>#cccccc</option>
> +    <option>#bbbbbb</option>
> +    <option>#aaaaaa</option>
> +    <option>#999999</option>
> +    <option>#888888</option>
> +    <option>#777777</option>
> +    <option>#666666</option>
> +    <option>#555555</option>
> +    <option>#444444</option>
> +    <option>#333333</option>
> +    <option>#222222</option>
> +    <option>#111111</option>
> +    <option>#000000</option>
> +</datalist>

It should contain some invalid color strings.

[Keishi Hattori]

Created attachment 154933 [details]
Patch

[Keishi Hattori]

> It should contain some invalid color strings.

It revealed and error in my code (Bug 92488)

[Keishi Hattori]

(In reply to comment #25)
> > It should contain some invalid color strings.
> 
> It revealed and error in my code (Bug 92488)

I mean Bug 92502

[WebKit Review Bot]

Comment on attachment 154933 [details]
Patch

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

New failing tests:
fast/forms/datalist/input-list.html

[WebKit Review Bot]

Created attachment 154956 [details]
Archive of layout-test-results from gce-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Keishi Hattori]

Created attachment 155209 [details]
Patch

[WebKit Review Bot]

Comment on attachment 155209 [details]
Patch

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

New failing tests:
fast/forms/datalist/input-list.html

[WebKit Review Bot]

Created attachment 155213 [details]
Archive of layout-test-results from gce-cr-linux-07

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-07  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Kent Tamura]

Comment on attachment 155209 [details]
Patch

r- because of a test failure

[Keishi Hattori]

Created attachment 155216 [details]
Patch

[Kent Tamura]

Comment on attachment 155216 [details]
Patch

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

> LayoutTests/ChangeLog:12
> +        * platform/chromium/fast/forms/datalist/input-list-expected.txt:

The patch doesn't contain input-list-expected.txt.
Please re-check what you'll upload before uploading patches.

[Keishi Hattori]

Created attachment 155220 [details]
Patch

[Kent Tamura]

Comment on attachment 155220 [details]
Patch

ok

[WebKit Review Bot]

Comment on attachment 155220 [details]
Patch

Clearing flags on attachment: 155220

Committed r124004: <http://trac.webkit.org/changeset/124004>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by 92622

[Keishi Hattori]

Created attachment 155239 [details]
Patch

[WebKit Review Bot]

Comment on attachment 155239 [details]
Patch

Clearing flags on attachment: 155239

Committed r124025: <http://trac.webkit.org/changeset/124025>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by 92658

[Keishi Hattori]

Created attachment 155423 [details]
Patch

[WebKit Review Bot]

Comment on attachment 155423 [details]
Patch

Clearing flags on attachment: 155423

Committed r124176: <http://trac.webkit.org/changeset/124176>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.