Patch to implement WebCore part of <input type=color> UI behavior.
Created attachment 97063 [details] patch
Comment on attachment 97063 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=97063&action=review > Source/WebCore/html/ColorInputType.cpp:116 > + element()->setValue(color.serialized(), true); Because this line dispatches a 'change' event and a JavaScript code can run, it's possible that the JavaScript code change the input type and this ColorInputType instance might be deleted. We must not access 'this' after this line. > Source/WebCore/html/ColorInputType.cpp:120 > + element()->document()->page()->chrome()->runUpdateColorPanel(Color(element()->value())); Why do we need to call runUpdateColorPanel() in colorChange()? colorChanged() is called from the color panel, isn't it? > Source/WebCore/page/Chrome.cpp:461 > +void Chrome::runUpdateColorPanel(const Color color) > +{ > + m_client->runUpdateColorPanel(color); The name 'runUpdateColorPanel' sounds curious. 'updateSelectedColorOfColorPanel'? > Source/WebCore/page/ChromeClient.h:228 > + virtual void runColorPanel() = 0; You need to add a comment of the function behavior. What should we do if runColorPanel() is called twice? When should we close the color panel opened by runColorPanel()? How to obtain ColorChooser instance to which the color panel send the selected color?
Comment on attachment 97063 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=97063&action=review >> Source/WebCore/html/ColorInputType.cpp:120 >> + element()->document()->page()->chrome()->runUpdateColorPanel(Color(element()->value())); > > Why do we need to call runUpdateColorPanel() in colorChange()? colorChanged() is called from the color panel, isn't it? Well I believe in JS you can change the color, and therefore the panel should update itself no? Though I'm not very sure if the JS events ends up in colorChanged. > Source/WebCore/page/Chrome.cpp:456 > + m_client->runColorPanel(); What about showColorPanel()? At the end we "show" a popup. To create the <select> popup the method's name is createSelectPopup(), perhaps we should stick with createColorChooserPanel() >> Source/WebCore/page/Chrome.cpp:461 >> + m_client->runUpdateColorPanel(color); > > The name 'runUpdateColorPanel' sounds curious. > 'updateSelectedColorOfColorPanel'? updatePanelCurrentColor()? >> Source/WebCore/page/ChromeClient.h:228 >> + virtual void runColorPanel() = 0; > > You need to add a comment of the function behavior. > What should we do if runColorPanel() is called twice? > When should we close the color panel opened by runColorPanel()? > How to obtain ColorChooser instance to which the color panel send the selected color? I believe if it's run twice, it doesn't matter the popup will still be shown no? I think when a color is selected we should close the panel no? http://www.devbeginners.com/index.php/html5/108-html5-color-picker-input-type has an example. I couldn't find in the spec *how* the popup should look like, is there some kind of guidance for that? > Source/WebCore/platform/ColorChooser.h:46 > + I believe ports will inherits from that?
Thanks for taking a look! (In reply to comment #3) > (From update of attachment 97063 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=97063&action=review > > >> Source/WebCore/html/ColorInputType.cpp:120 > >> + element()->document()->page()->chrome()->runUpdateColorPanel(Color(element()->value())); > > > > Why do we need to call runUpdateColorPanel() in colorChange()? colorChanged() is called from the color panel, isn't it? > Well I believe in JS you can change the color, and therefore the panel should update itself no? Though I'm not very sure if the JS events ends up in colorChanged. Sorry, I got mixed up when dividing the patch. I will fix it. > > Source/WebCore/page/Chrome.cpp:456 > > + m_client->runColorPanel(); > > What about showColorPanel()? At the end we "show" a popup. > > To create the <select> popup the method's name is createSelectPopup(), perhaps we should stick with createColorChooserPanel() I tried to match the name with Chrome::runOpenPanel, but I think showColorPanel is much better. I'd gladly change it. > >> Source/WebCore/page/Chrome.cpp:461 > >> + m_client->runUpdateColorPanel(color); > > > > The name 'runUpdateColorPanel' sounds curious. > > 'updateSelectedColorOfColorPanel'? > > updatePanelCurrentColor()? Ditto. > >> Source/WebCore/page/ChromeClient.h:228 > >> + virtual void runColorPanel() = 0; > > > > You need to add a comment of the function behavior. > > What should we do if runColorPanel() is called twice? > > When should we close the color panel opened by runColorPanel()? > > How to obtain ColorChooser instance to which the color panel send the selected color? > > I believe if it's run twice, it doesn't matter the popup will still be shown no? I don't think we should ever show multiple color choose dialogs. It is impossible to tell which dialog is from which input element. Therefore I think it is safe to implement it for only one instance of ColorChooser. > I think when a color is selected we should close the panel no? > http://www.devbeginners.com/index.php/html5/108-html5-color-picker-input-type Mac OS color picker is usually persistant. However now that I think about it, it probably should close when the page changes or something. > has an example. I couldn't find in the spec *how* the popup should look like, is there some kind of guidance for that? I don't think there is. > > Source/WebCore/platform/ColorChooser.h:46 > > I believe ports will inherits from that? Hopefully they wouldn't have to. I think implementing their UI stuff in ChromeClient::runColorPanel/runUpdateColorPanel should be enough.
Created attachment 97735 [details] updated test results
Comment on attachment 97735 [details] updated test results Sorry. Wrong bug.
Hey folks, Is there any estimate of when the patch can be landed? Web Inspector could make a good use of this feature...
(In reply to comment #7) > Is there any estimate of when the patch can be landed? Web Inspector could make a good use of this feature... This is my top priority but because there are UIs to decide on and many platforms to implement, it will take at least a few more months.
Created attachment 102767 [details] new patch
(In reply to comment #9) > Created an attachment (id=102767)(apply) [details] > new patch Added method to close color chooser. Better method names.
Comment on attachment 102767 [details] new patch View in context: https://bugs.webkit.org/attachment.cgi?id=102767&action=review > Source/WebCore/WebCore.xcodeproj/project.pbxproj:19091 > E1E1BEFF115FF6FB006F52CA /* WindowsKeyboardCodes.h */, > + C3EEE07713E7F9DE005F7460 /* ColorChooser.h */, > + C3EEE07813E7F9DE005F7460 /* ColorChooser.cpp */, They should be in alphabetical order. > Source/WebCore/WebCore.xcodeproj/project.pbxproj:20281 > 41E1B1D10FF5986900576B3B /* AbstractWorker.h in Headers */, > + C3EEE07913E7F9DE005F7460 /* ColorChooser.h in Headers */, > 29A8122E0FBB9C1D00510293 /* AccessibilityARIAGridCell.h in Headers */, ditto. > Source/WebCore/WebCore.xcodeproj/project.pbxproj:26071 > BCAB418113E356E800D8AAF3 /* Region.cpp in Sources */, > + C3EEE07A13E7F9DE005F7460 /* ColorChooser.cpp in Sources */, ditto. > Source/WebCore/css/html.css:640 > width: 100%; > - height: 100% > + height: 100%; This change is unrelated to this bug. > Source/WebCore/html/ColorInputType.cpp:147 > + ExceptionCode ec; > + setValueAsColor(color, ec); > +} We shouldn't ignore the resultant ExceptionCode. Please see the comment about setValueAsColor() in ColorInputType.h > Source/WebCore/html/ColorInputType.h:53 > + virtual void setValueAsColor(const Color&, ExceptionCode&) const; We should have the default argument for ExceptionCode&. virtual void setValueAsColor(const Color&, ExceptionCode& = ASSERT_NO_EXCEPTION) const; Then, you can omit the ExceptionCode argument for call sites. > Source/WebCore/html/ColorInputType.h:60 > + > + virtual void colorSelected(const Color&); Please add a comment that this function is for ColorChooserClient. > Source/WebCore/html/HTMLInputElement.h:157 > + Color valueAsColor() const; > + void setValueAsColor(const Color&, ExceptionCode&); Why we need them in HTMLInputElement? > Source/WebCore/page/Chrome.h:163 > + void runOpenColorChooser(ColorChooser*, const Color&); > + void runCloseColorChooser(); > + void runSetSelectedColorInColorChooser(const Color&); I still don't like these function names. 'run' + verb sounds very strange. I guess "runOpenPanel" means "run an OpenPanel dialog". So we don't need to follow it. > Source/WebCore/platform/ColorChooser.h:60 > + ColorChooser() > + : m_client(0) Indentation is wrong.
(In reply to comment #11) > I still don't like these function names. 'run' + verb sounds very strange. > I guess "runOpenPanel" means "run an OpenPanel dialog". So we don't need to follow it. I found the names of the file chooser dialog in OS X is NSOpenPanel. So "OpenPanel" is a noun.
Created attachment 102900 [details] fixed patch
Comment on attachment 102767 [details] new patch View in context: https://bugs.webkit.org/attachment.cgi?id=102767&action=review >> Source/WebCore/WebCore.xcodeproj/project.pbxproj(edit):19091 >> + C3EEE07813E7F9DE005F7460 /* ColorChooser.cpp */, > > They should be in alphabetical order. Done. >> Source/WebCore/css/html.css(edit):640 >> + height: 100%; > > This change is unrelated to this bug. Done. >> Source/WebCore/html/ColorInputType.cpp(edit):147 >> +} > > We shouldn't ignore the resultant ExceptionCode. > Please see the comment about setValueAsColor() in ColorInputType.h Removed ec. >> Source/WebCore/html/ColorInputType.h(edit):53 >> + virtual void setValueAsColor(const Color&, ExceptionCode&) const; > > We should have the default argument for ExceptionCode&. > virtual void setValueAsColor(const Color&, ExceptionCode& = ASSERT_NO_EXCEPTION) const; > Then, you can omit the ExceptionCode argument for call sites. Done. >> Source/WebCore/html/ColorInputType.h(edit):60 >> + virtual void colorSelected(const Color&); > > Please add a comment that this function is for ColorChooserClient. Done. >> Source/WebCore/html/HTMLInputElement.h(edit):157 >> + void setValueAsColor(const Color&, ExceptionCode&); > > Why we need them in HTMLInputElement? Done. Is HTMLInputElement::valueAsDate() something left over from the past? >> Source/WebCore/page/Chrome.h(edit):163 >> + void runSetSelectedColorInColorChooser(const Color&); > > I still don't like these function names. 'run' + verb sounds very strange. > I guess "runOpenPanel" means "run an OpenPanel dialog". So we don't need to follow it. Removed run part. I happened to be looking at parts of the code in WebKit and Chromium with lots of run*. >> Source/WebCore/platform/ColorChooser.h(edit):60 >> + : m_client(0) > > Indentation is wrong. Done.
(In reply to comment #14) > > Why we need them in HTMLInputElement? > > Done. > Is HTMLInputElement::valueAsDate() something left over from the past? valueAsDate() is exposed to JavaScript.
Comment on attachment 102900 [details] fixed patch View in context: https://bugs.webkit.org/attachment.cgi?id=102900&action=review > Source/WebCore/html/ColorInputType.cpp:128 > +void ColorInputType::willRemove() > +{ > + if (ColorChooser::chooser()->client() == this) { > + if (Chrome* chrome = this->chrome()) > + chrome->closeColorChooser(); I think Node::willRemove() is wrong for this task. We should close the chooser when the element disappears. e.g. display:none; > Source/WebCore/html/ColorInputType.h:53 > + virtual Color valueAsColor() const; > + virtual void setValueAsColor(const Color&, ExceptionCode& = ASSERT_NO_EXCEPTION) const; I realized that valueAsColor() and setValueAsColor() were used only by ColorInputType. So, - we dont need InputType::valueAsColor() and setValueAsColor(). - They should not be virtual. - They should be private. - setValueAsColor() doesn't need the ExceptionCode& argument at all.
Created attachment 103057 [details] new fixed patch
Comment on attachment 102900 [details] fixed patch View in context: https://bugs.webkit.org/attachment.cgi?id=102900&action=review >> Source/WebCore/html/ColorInputType.cpp(edit):128 >> + chrome->closeColorChooser(); > > I think Node::willRemove() is wrong for this task. > We should close the chooser when the element disappears. e.g. display:none; Done. Moved "close color chooser" call to ColorInputType::detach. >> Source/WebCore/html/ColorInputType.h(edit):53 >> + virtual void setValueAsColor(const Color&, ExceptionCode& = ASSERT_NO_EXCEPTION) const; > > I realized that valueAsColor() and setValueAsColor() were used only by ColorInputType. > So, > - we dont need InputType::valueAsColor() and setValueAsColor(). > - They should not be virtual. > - They should be private. > - setValueAsColor() doesn't need the ExceptionCode& argument at all. Done. Also added ColorChooser::closeColorChooserIfClientInDocument(). It is called from WebCore::FrameLoader::transitionToCommitted so it closes the color chooser when the another page is loaded in the frame.
Comment on attachment 103057 [details] new fixed patch View in context: https://bugs.webkit.org/attachment.cgi?id=103057&action=review > Source/WebCore/html/HTMLInputElement.cpp:890 > +#if ENABLE(INPUT_COLOR) > + if (ColorChooser::chooser()->client() == m_inputType) { > + if (m_inputType->isColorControl) { > + if (Page* page = frame->page()) > + page->chrome()->closeColorChooser(); > + } > + } > +#endif We should not add type-specific code into HTMLInputElement. Please add InputType::elementWasDetached(). > Source/WebCore/platform/ColorChooser.h:46 > + virtual void closeColorChooserIfClientInDocument(Document*) = 0; IfClientInDocument -> IfClientIsInDocument
Created attachment 103060 [details] new new fixed patch
(In reply to comment #19) > (From update of attachment 103057 [details](apply) [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=103057&action=review > > > Source/WebCore/html/HTMLInputElement.cpp:890 > > +#if ENABLE(INPUT_COLOR) > > + if (ColorChooser::chooser()->client() == m_inputType) { > > + if (m_inputType->isColorControl) { > > + if (Page* page = frame->page()) > > + page->chrome()->closeColorChooser(); > > + } > > + } > > +#endif > > We should not add type-specific code into HTMLInputElement. > Please add InputType::elementWasDetached(). This shouldn't have been there. I got my git branches mixed up. I added InputType::detach to be in line with the existing InputType::attach, but elementWasDetached would be better. Should I rename InputType::attach too? > > Source/WebCore/platform/ColorChooser.h:46 > > + virtual void closeColorChooserIfClientInDocument(Document*) = 0; > > IfClientInDocument -> IfClientIsInDocument Done.
Comment on attachment 103060 [details] new new fixed patch View in context: https://bugs.webkit.org/attachment.cgi?id=103060&action=review > Source/WebCore/ChangeLog:27 > + * html/HTMLInputElement.cpp: > + (WebCore::HTMLInputElement::detach): Close color chooser. Called when input element or its ancestors have "display:none" > + or is removed from DOM. ChangeLog is out-of-sync.
Created attachment 103063 [details] updated changelog and pbxproj
Comment on attachment 103063 [details] updated changelog and pbxproj ok!
Comment on attachment 103063 [details] updated changelog and pbxproj Clearing flags on attachment: 103063 Committed r92477: <http://trac.webkit.org/changeset/92477>
All reviewed patches have been landed. Closing bug.
This patch introduced a layering violation by using Document.h from the platform directory. Please consider reverting and fixing this issue.
(In reply to comment #27) > This patch introduced a layering violation by using Document.h from the platform directory. Please consider reverting and fixing this issue. You're right! Thank you for pointing it out. I'll roll out this change.
Reverted r92477 for reason: Layering violation. We should not use WebCore/dom/ in WebCore/platform/. Committed r92533: <http://trac.webkit.org/changeset/92533>
Comment on attachment 103063 [details] updated changelog and pbxproj View in context: https://bugs.webkit.org/attachment.cgi?id=103063&action=review > Source/WebCore/loader/FrameLoader.cpp:1825 > + if (m_frame->document()) > + ColorChooser::chooser()->closeColorChooserIfClientIsInDocument(m_frame->document()); Are there no other ways to close the color chooser when the document is navigated out? Is not the document detached? Some ideas to remove the layering violation: * Introduce ColorChooserContext interface class, and Document inherits it, or * Register/unregister unload(?) callback to Document or Frame. A ColorChooserClient registers itself to it. > Source/WebCore/platform/ColorChooser.cpp:47 > + staticChooser = new ColorChooser(); We should use adoptPtr(new ColorChooser()).leakPtr() to inform no one will delete this object.
(In reply to comment #30) > (From update of attachment 103063 [details](apply) [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=103063&action=review > > > Source/WebCore/loader/FrameLoader.cpp:1825 > > + if (m_frame->document()) > > + ColorChooser::chooser()->closeColorChooserIfClientIsInDocument(m_frame->document()); > > Are there no other ways to close the color chooser when the document is navigated out? Is not the document detached? Document::detach and ColorInputType::detach do get called when navigating to another page, but it is like 10 seconds after the new page is displayed. It seems to be called when a timer is fired and PageCache::releaseAutoreleasedPagesNowOrReschedule is called. We can close the color chooser much faster if we call from FrameLoader. > Some ideas to remove the layering violation: > * Introduce ColorChooserContext interface class, and Document inherits it, or > * Register/unregister unload(?) callback to Document or Frame. A ColorChooserClient registers itself to it. I tried to make the change minimal in my next patch. However it still uses the word "ColorInputType" inside a method name so I think it is still layering violation. > We should use > adoptPtr(new ColorChooser()).leakPtr() > to inform no one will delete this object. Done.
Created attachment 103201 [details] fix layering violation
Comment on attachment 103201 [details] fix layering violation View in context: https://bugs.webkit.org/attachment.cgi?id=103201&action=review > Source/WebCore/ChangeLog:39 > + (WebCore::FrameLoader::transitionToCommitted): Close color chooser when > + navigating away from the page. Adding the reason that detach() doesn't work would be helpful. > Source/WebCore/html/ColorInputType.cpp:203 > + if (ColorChooser::chooser()->client() == this) { We prefer early exit. if (ColorChooser::chooser()->client() != this) return; > Source/WebCore/loader/FrameLoader.cpp:1828 > + ColorInputType* colorInputType = (ColorInputType*)colorChooserClient; > + colorInputType->closeColorChooserIfCurrentClient(); Should use C++-style cast instead of C-style cast. static_cast<ColorInputType*>(colorChooserClient)->closeColorChooserIfCurrenClient();
Created attachment 103210 [details] new patch
Comment on attachment 103201 [details] fix layering violation View in context: https://bugs.webkit.org/attachment.cgi?id=103201&action=review >> Source/WebCore/ChangeLog(edit):39 >> + navigating away from the page. > > Adding the reason that detach() doesn't work would be helpful. Done. >> Source/WebCore/html/ColorInputType.cpp(edit):203 >> + if (ColorChooser::chooser()->client() == this) { > > We prefer early exit. > > if (ColorChooser::chooser()->client() != this) > return; Done. >> Source/WebCore/loader/FrameLoader.cpp(edit):1828 >> + colorInputType->closeColorChooserIfCurrentClient(); > > Should use C++-style cast instead of C-style cast. > > static_cast<ColorInputType*>(colorChooserClient)->closeColorChooserIfCurrenClient(); Done.
Comment on attachment 103210 [details] new patch ok
Comment on attachment 103210 [details] new patch Clearing flags on attachment: 103210 Committed r92592: <http://trac.webkit.org/changeset/92592>