Summary: | Implement datalist UI for input type color for Chromium | ||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Keishi Hattori <keishi> | ||||||||||||||||||||||||||||||||||
Component: | Forms | Assignee: | Keishi Hattori <keishi> | ||||||||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||||
Severity: | Normal | CC: | abarth, dglazkov, eric, fishd, jamesr, mifenton, tkent, tkent+wkapi, webkit.review.bot | ||||||||||||||||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||||||||||||
Bug Depends on: | 92092, 92096, 92109, 92114, 92488, 92622, 92658 | ||||||||||||||||||||||||||||||||||||
Bug Blocks: | 91593 | ||||||||||||||||||||||||||||||||||||
Attachments: |
|
Description
Keishi Hattori
2012-07-24 00:59:02 PDT
Created attachment 153993 [details]
Patch
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 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. Also, I recommend to separate the patch to the WebCore part and the Chromium WebKit part. 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. Created attachment 154612 [details]
Patch
Comment on attachment 154612 [details] Patch Attachment 154612 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13361501 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. Created attachment 154822 [details]
Patch
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. Created attachment 154834 [details]
Patch
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? Created attachment 154848 [details]
Patch
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. Chromium patch to add localized string: https://chromiumcodereview.appspot.com/10824058/ 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/ Created attachment 154867 [details]
Patch
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. Created attachment 154899 [details]
Patch
I got Pierre Rossi's approval and removed elementRectRelativeToWindow. Comment on attachment 154899 [details]
Patch
you should make a separated patch for the WebCore change.
Created attachment 154911 [details]
Patch
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. Created attachment 154933 [details]
Patch
> It should contain some invalid color strings. It revealed and error in my code (Bug 92488) (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 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 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
Created attachment 155209 [details]
Patch
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 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
Comment on attachment 155209 [details]
Patch
r- because of a test failure
Created attachment 155216 [details]
Patch
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. Created attachment 155220 [details]
Patch
Comment on attachment 155220 [details]
Patch
ok
Comment on attachment 155220 [details] Patch Clearing flags on attachment: 155220 Committed r124004: <http://trac.webkit.org/changeset/124004> All reviewed patches have been landed. Closing bug. Re-opened since this is blocked by 92622 Created attachment 155239 [details]
Patch
Comment on attachment 155239 [details] Patch Clearing flags on attachment: 155239 Committed r124025: <http://trac.webkit.org/changeset/124025> All reviewed patches have been landed. Closing bug. Re-opened since this is blocked by 92658 Created attachment 155423 [details]
Patch
Comment on attachment 155423 [details] Patch Clearing flags on attachment: 155423 Committed r124176: <http://trac.webkit.org/changeset/124176> All reviewed patches have been landed. Closing bug. |