Bug 66848 - input color: onchange event is not fired when changing color from color chooser
Summary: input color: onchange event is not fired when changing color from color chooser
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Keishi Hattori
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-08-24 03:46 PDT by Keishi Hattori
Modified: 2011-08-31 01:07 PDT (History)
6 users (show)

See Also:


Attachments
patch (10.85 KB, patch)
2011-08-24 07:39 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff
fixed typo and wrapped internals.idl with flag (10.88 KB, patch)
2011-08-24 18:28 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff
fixed patch (9.64 KB, patch)
2011-08-25 20:39 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff
fixed comment & rebased (9.58 KB, patch)
2011-08-30 23:51 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff
filled out reviewer in changelog (9.58 KB, patch)
2011-08-31 00:02 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Keishi Hattori 2011-08-24 03:46:48 PDT
Bug 66658 introduced a bug where the onchange event is not fired when changing color from the color chooser.
Comment 1 Keishi Hattori 2011-08-24 07:39:00 PDT
Created attachment 104997 [details]
patch
Comment 2 WebKit Review Bot 2011-08-24 07:45:19 PDT
Comment on attachment 104997 [details]
patch

Attachment 104997 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9487102
Comment 3 WebKit Review Bot 2011-08-24 07:51:08 PDT
Comment on attachment 104997 [details]
patch

Attachment 104997 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/9490042
Comment 4 Early Warning System Bot 2011-08-24 07:51:37 PDT
Comment on attachment 104997 [details]
patch

Attachment 104997 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9481944
Comment 5 Gyuyoung Kim 2011-08-24 07:55:45 PDT
Comment on attachment 104997 [details]
patch

Attachment 104997 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/9487106
Comment 6 Collabora GTK+ EWS bot 2011-08-24 09:04:15 PDT
Comment on attachment 104997 [details]
patch

Attachment 104997 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9489084
Comment 7 Keishi Hattori 2011-08-24 18:28:34 PDT
Created attachment 105115 [details]
fixed typo and wrapped internals.idl with flag
Comment 8 Kent Tamura 2011-08-24 19:33:20 PDT
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?
Comment 9 Keishi Hattori 2011-08-25 06:53:00 PDT
(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
Comment 10 Darin Adler 2011-08-25 11:04:14 PDT
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.
Comment 11 Keishi Hattori 2011-08-25 20:39:52 PDT
Created attachment 105298 [details]
fixed patch
Comment 12 Keishi Hattori 2011-08-25 20:41:50 PDT
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.
Comment 13 Kent Tamura 2011-08-29 22:47:28 PDT
Comment on attachment 105298 [details]
fixed patch

It seems all of Darin's comments were addressed in the updated patch.
Comment 14 Darin Adler 2011-08-30 10:23:04 PDT
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();
Comment 15 Keishi Hattori 2011-08-30 23:51:18 PDT
Created attachment 105746 [details]
fixed comment & rebased
Comment 16 Keishi Hattori 2011-08-30 23:52:21 PDT
(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.
Comment 17 Keishi Hattori 2011-08-30 23:53:01 PDT
I've left it as it is.
Comment 18 Keishi Hattori 2011-08-31 00:02:50 PDT
Created attachment 105749 [details]
filled out reviewer in changelog
Comment 19 WebKit Review Bot 2011-08-31 01:07:38 PDT
Comment on attachment 105749 [details]
filled out reviewer in changelog

Clearing flags on attachment: 105749

Committed r94158: <http://trac.webkit.org/changeset/94158>
Comment 20 WebKit Review Bot 2011-08-31 01:07:43 PDT
All reviewed patches have been landed.  Closing bug.