Bug 62619 – Implement <input type=color> UI behavior WebCore part

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

[Keishi Hattori]

Patch to implement WebCore part of <input type=color> UI behavior.

[Keishi Hattori]

Created attachment 97063 [details]
patch

[Kent Tamura]

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?

[Alexis Menard (darktears)]

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?

[Keishi Hattori]

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.

[Keishi Hattori]

Created attachment 97735 [details]
updated test results

[Keishi Hattori]

Comment on attachment 97735 [details]
updated test results

Sorry. Wrong bug.

[Alexander Pavlov (apavlov)]

Hey folks,

Is there any estimate of when the patch can be landed? Web Inspector could make a good use of this feature...

[Keishi Hattori]

(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.

[Keishi Hattori]

Created attachment 102767 [details]
new patch

[Keishi Hattori]

(In reply to comment #9)
> Created an attachment (id=102767)(apply) [details]
> new patch

Added method to close color chooser.
Better method names.

[Kent Tamura]

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.

[Kent Tamura]

(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.

[Keishi Hattori]

Created attachment 102900 [details]
fixed patch

[Keishi Hattori]

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.

[Kent Tamura]

(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.

[Kent Tamura]

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.

[Keishi Hattori]

Created attachment 103057 [details]
new fixed patch

[Keishi Hattori]

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.

[Kent Tamura]

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

[Keishi Hattori]

Created attachment 103060 [details]
new new fixed patch

[Keishi Hattori]

(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.

[Kent Tamura]

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.

[Keishi Hattori]

Created attachment 103063 [details]
updated changelog and pbxproj

[Kent Tamura]

Comment on attachment 103063 [details]
updated changelog and pbxproj

ok!

[WebKit Review Bot]

Comment on attachment 103063 [details]
updated changelog and pbxproj

Clearing flags on attachment: 103063

Committed r92477: <http://trac.webkit.org/changeset/92477>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Sam Weinig]

This patch introduced a layering violation by using Document.h from the platform directory.  Please consider reverting and fixing this issue.

[Kent Tamura]

(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.

[Kent Tamura]

Reverted r92477 for reason:

Layering violation. We should not use WebCore/dom/ in WebCore/platform/.

Committed r92533: <http://trac.webkit.org/changeset/92533>

[Kent Tamura]

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.

[Keishi Hattori]

(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.

[Keishi Hattori]

Created attachment 103201 [details]
fix layering violation

[Kent Tamura]

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();

[Keishi Hattori]

Created attachment 103210 [details]
new patch

[Keishi Hattori]

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.

[Kent Tamura]

Comment on attachment 103210 [details]
new patch

ok

[WebKit Review Bot]

Comment on attachment 103210 [details]
new patch

Clearing flags on attachment: 103210

Committed r92592: <http://trac.webkit.org/changeset/92592>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.