Bug 66848 – input color: onchange event is not fired when changing color from color chooser

• Component: Forms
• Status: RESOLVED FIXED

[Keishi Hattori]

Bug 66658 introduced a bug where the onchange event is not fired when changing color from the color chooser.

[Keishi Hattori]

Created attachment 104997 [details]
patch

[WebKit Review Bot]

Comment on attachment 104997 [details]
patch

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

[WebKit Review Bot]

Comment on attachment 104997 [details]
patch

Attachment 104997 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/9490042

[Early Warning System Bot]

Comment on attachment 104997 [details]
patch

Attachment 104997 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9481944

[Gyuyoung Kim]

Comment on attachment 104997 [details]
patch

Attachment 104997 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/9487106

[Collabora GTK+ EWS bot]

Comment on attachment 104997 [details]
patch

Attachment 104997 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9489084

[Keishi Hattori]

Created attachment 105115 [details]
fixed typo and wrapped internals.idl with flag

[Kent Tamura]

Comment on attachment 105115 [details]
fixed typo and wrapped internals.idl with flag

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

> Source/WebCore/html/HTMLInputElement.h:240
> +    // For test purpose.
> +    bool connectToColorChooser();

Can you remove this or move this to WebCore/testing/? The purpose of WebCore/testing/ is to avoid such test-only code.

> Source/WebCore/testing/Internals.cpp:172
> +    HTMLInputElement* inputElement = element->toInputElement();
> +    if (!inputElement || !inputElement->connectToColorChooser())

Who disconnects the ColorChooserClient? Does ColorChooser have a stale pointer to a ColorChooserClient after the ColorChooserClient is deleted?

[Keishi Hattori]

(In reply to comment #8)
> (From update of attachment 105115 [details](apply) [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=105115&action=review
> 
> > Source/WebCore/html/HTMLInputElement.h:240
> > +    // For test purpose.
> > +    bool connectToColorChooser();
> 
> Can you remove this or move this to WebCore/testing/? The purpose of WebCore/testing/ is to avoid such test-only code.

To do this we would need to make HTMLInputElement::inputType() public. The m_inputType is contained inside HTMLInputElement so I believe this is my only option.

> > Source/WebCore/testing/Internals.cpp:172
> > +    HTMLInputElement* inputElement = element->toInputElement();
> > +    if (!inputElement || !inputElement->connectToColorChooser())
> 
> Who disconnects the ColorChooserClient? Does ColorChooser have a stale pointer to a ColorChooserClient after the ColorChooserClient is deleted?

The destructor for ColorChooserClient disconnects the color chooser client, so that won't happen.
http://trac.webkit.org/browser/trunk/Source/WebCore/platform/ColorChooser.cpp#L41

[Darin Adler]

Comment on attachment 105115 [details]
fixed typo and wrapped internals.idl with flag

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

Looks good. A few problems.

> Source/WebCore/WebCore.exp.in:1917
>  __ZNK7WebCore12ColorChooser13colorSelectedERKNS_5ColorE
> +
> +__ZN7WebCore5ColorC1ERKN3WTF6StringE

Why the blank line? Please don’t add that blank line.

> Source/WebCore/html/ColorInputType.h:46
> +    virtual bool isColorControl() const;

Why are we making this public? What code has a pointer to ColorInputType but then has to call isColorControl on it? Why not just assume it’s true in that case?

I think you may have thought you’d need this public to call it on an InputType*, but that is not true. Please don’t make this change.

> Source/WebCore/html/HTMLInputElement.cpp:1901
> +bool HTMLInputElement::isColorControl() const

I see no reason to add this function. Please don’t add it. The one caller is inside the input element class and so can call m_inputType->isColorControl() directly.

> Source/WebCore/html/HTMLInputElement.h:114
> +#if ENABLE(INPUT_COLOR)
> +    virtual bool isColorControl() const;
> +#endif

Please don’t add this function, and if you do add it, don’t make it virtual.

> Source/WebCore/testing/Internals.cpp:171
> +    HTMLInputElement* inputElement = element->toInputElement();

A function like this should check if the element is an input element before casting. The toInputElement function is only legal to call if you already have checked that it’s an input element.

> Source/WebCore/testing/Internals.h:64
> +    void connectColorChooserClient(Element*, ExceptionCode&);

For testing purposes, I think it’s OK to have the function just silently fail. It seems overkill to me to hook up a DOM exception for the test function.

[Keishi Hattori]

Created attachment 105298 [details]
fixed patch

[Keishi Hattori]

Comment on attachment 105115 [details]
fixed typo and wrapped internals.idl with flag

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

I also changed Internals.idl because I noticed #if ENABLE(INPUT_COLOR) was giving errors and it was only working because of the old generated code.

>> Source/WebCore/WebCore.exp.in(edit):1917
>> +__ZN7WebCore5ColorC1ERKN3WTF6StringE
> 
> Why the blank line? Please don’t add that blank line.

Done.

>> Source/WebCore/html/ColorInputType.h(edit):46
>> +    virtual bool isColorControl() const;
> 
> Why are we making this public? What code has a pointer to ColorInputType but then has to call isColorControl on it? Why not just assume it’s true in that case?
> 
> I think you may have thought you’d need this public to call it on an InputType*, but that is not true. Please don’t make this change.

I see!
Done.

>> Source/WebCore/html/HTMLInputElement.cpp(edit):1901
>> +bool HTMLInputElement::isColorControl() const
> 
> I see no reason to add this function. Please don’t add it. The one caller is inside the input element class and so can call m_inputType->isColorControl() directly.

Done.

>> Source/WebCore/html/HTMLInputElement.h(edit):114
>> +#endif
> 
> Please don’t add this function, and if you do add it, don’t make it virtual.

Done.

>> Source/WebCore/testing/Internals.cpp(edit):171
>> +    HTMLInputElement* inputElement = element->toInputElement();
> 
> A function like this should check if the element is an input element before casting. The toInputElement function is only legal to call if you already have checked that it’s an input element.

Done.

>> Source/WebCore/testing/Internals.h(edit):64
>> +    void connectColorChooserClient(Element*, ExceptionCode&);
> 
> For testing purposes, I think it’s OK to have the function just silently fail. It seems overkill to me to hook up a DOM exception for the test function.

Done.

[Kent Tamura]

Comment on attachment 105298 [details]
fixed patch

It seems all of Darin's comments were addressed in the updated patch.

[Darin Adler]

Comment on attachment 105298 [details]
fixed patch

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

> Source/WebCore/html/HTMLInputElement.h:235
> +    // For test purpose.

This should be "test purposes".

> Source/WebCore/testing/Internals.cpp:177
> +    if (!element->hasTagName(HTMLNames::inputTag))
> +        return false;
> +    HTMLInputElement* inputElement = element->toInputElement();
> +    if (!inputElement)
> +        return false;
> +    return inputElement->connectToColorChooser();

Just for the record, I was wrong about toInputElement. It does the check for you. But since we have another bug about getting rid of toInputElement altogether, this will get fixed, so we’re OK if you land the patch as-is. The correct style for future code would be:

    if (!element->hasTagName(HTMLNames::inputTag))
        return false;
    return static_cast<HTMLInputElement*>(element)->connectToColorChooser();

[Keishi Hattori]

Created attachment 105746 [details]
fixed comment & rebased

[Keishi Hattori]

(In reply to comment #14)
> (From update of attachment 105298 [details](apply) [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=105298&action=review
> 
> > Source/WebCore/html/HTMLInputElement.h:235
> > +    // For test purpose.
> 
> This should be "test purposes".
Done.

> Just for the record, I was wrong about toInputElement. It does the check for you. But since we have another bug about getting rid of toInputElement altogether, this will get fixed, so we’re OK if you land the patch as-is. The correct style for future code would be:
> 
>     if (!element->hasTagName(HTMLNames::inputTag))
>         return false;
>     return static_cast<HTMLInputElement*>(element)->connectToColorChooser();
Okay. I've left it as it was.

[Keishi Hattori]

I've left it as it is.

[Keishi Hattori]

Created attachment 105749 [details]
filled out reviewer in changelog

[WebKit Review Bot]

Comment on attachment 105749 [details]
filled out reviewer in changelog

Clearing flags on attachment: 105749

Committed r94158: <http://trac.webkit.org/changeset/94158>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.