Dispatch change event for input type=checkbox when checked but value is null

Dispatch change event for input type=checkbox when checked but value is null

Checkbox dispatches change event when checked and dispatch event is not
based on value being null or not.

BUG=366989
R=tkent,keishi1
Test=If value attribute is null, when checkbox is checked a change event
is triggered.

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=173530

[tkent, 2014-04-29 23:43:40]

https://codereview.chromium.org/255163002/diff/1/Source/core/html/HTMLTextFor...
File Source/core/html/HTMLTextFormControlElement.cpp (right):

https://codereview.chromium.org/255163002/diff/1/Source/core/html/HTMLTextFor...
Source/core/html/HTMLTextFormControlElement.cpp:187: if
(!equalIgnoringNullity(m_textAsOfLastFormControlChangeEvent, value()) || (type()
== "checkbox" && m_textAsOfLastFormControlChangeEvent != value())) {
Please do not add input-type checking to HTMLTextFormControlElement.
We should have new virtual function for this line, or CheckboxInputType should
not use HTMLTextFormControlElement::dispatchFormControlChangeEvent.

[Habib Virji, 2014-05-01 10:42:44]

On 2014/04/29 23:43:40, tkent wrote:
>
https://codereview.chromium.org/255163002/diff/1/Source/core/html/HTMLTextFor...
> File Source/core/html/HTMLTextFormControlElement.cpp (right):
> 
>
https://codereview.chromium.org/255163002/diff/1/Source/core/html/HTMLTextFor...
> Source/core/html/HTMLTextFormControlElement.cpp:187: if
> (!equalIgnoringNullity(m_textAsOfLastFormControlChangeEvent, value()) ||
(type()
> == "checkbox" && m_textAsOfLastFormControlChangeEvent != value())) {
> Please do not add input-type checking to HTMLTextFormControlElement.
> We should have new virtual function for this line, or CheckboxInputType should
> not use HTMLTextFormControlElement::dispatchFormControlChangeEvent.

Updated to use virtual function instead of input type checking.

[keishi, 2014-05-02 07:44:54]

https://codereview.chromium.org/255163002/diff/20001/Source/core/html/HTMLTex...
File Source/core/html/HTMLTextFormControlElement.cpp (right):

https://codereview.chromium.org/255163002/diff/20001/Source/core/html/HTMLTex...
Source/core/html/HTMLTextFormControlElement.cpp:192: if
(!equalIgnoringNullity(m_textAsOfLastFormControlChangeEvent, value()) ||
(isCheckbox() && m_textAsOfLastFormControlChangeEvent != value())) {
HTMLTextFormControlElement exists to share code between the HTMLTextareaElement
and HTMLInputElement so I don't think it should know about input types.

I think we should make a virtual method for this line and do something like
this.
- Add HTMLTextFormControlElement::shouldDispatchFormControlChangeEvent(oldValue,
newValue)
- Override it in HTMLInputElement::shouldDispatchFormControlChangeEvent
- HTMLInputElement::shouldDispatchFormControlChangeEvent calls
InputType::shouldDispatchChangeEvent
- Override InputType::shouldDispatchChangeEvent in CheckboxInputType

[Habib Virji, 2014-05-02 15:07:39]

Thanks keishi. Done as suggested.

https://codereview.chromium.org/255163002/diff/20001/Source/core/html/HTMLTex...
File Source/core/html/HTMLTextFormControlElement.cpp (right):

https://codereview.chromium.org/255163002/diff/20001/Source/core/html/HTMLTex...
Source/core/html/HTMLTextFormControlElement.cpp:192: if
(!equalIgnoringNullity(m_textAsOfLastFormControlChangeEvent, value()) ||
(isCheckbox() && m_textAsOfLastFormControlChangeEvent != value())) {
On 2014/05/02 07:44:55, keishi1 wrote:
> HTMLTextFormControlElement exists to share code between the
HTMLTextareaElement
> and HTMLInputElement so I don't think it should know about input types.
> 
> I think we should make a virtual method for this line and do something like
> this.
> - Add
HTMLTextFormControlElement::shouldDispatchFormControlChangeEvent(oldValue,
> newValue)
> - Override it in HTMLInputElement::shouldDispatchFormControlChangeEvent
> - HTMLInputElement::shouldDispatchFormControlChangeEvent calls
> InputType::shouldDispatchChangeEvent
> - Override InputType::shouldDispatchChangeEvent in CheckboxInputType

Done.

[keishi, 2014-05-02 15:58:33]

On 2014/05/02 15:07:39, Habib Virji wrote:
> Thanks keishi. Done as suggested.
> 
>
https://codereview.chromium.org/255163002/diff/20001/Source/core/html/HTMLTex...
> File Source/core/html/HTMLTextFormControlElement.cpp (right):
> 
>
https://codereview.chromium.org/255163002/diff/20001/Source/core/html/HTMLTex...
> Source/core/html/HTMLTextFormControlElement.cpp:192: if
> (!equalIgnoringNullity(m_textAsOfLastFormControlChangeEvent, value()) ||
> (isCheckbox() && m_textAsOfLastFormControlChangeEvent != value())) {
> On 2014/05/02 07:44:55, keishi1 wrote:
> > HTMLTextFormControlElement exists to share code between the
> HTMLTextareaElement
> > and HTMLInputElement so I don't think it should know about input types.
> > 
> > I think we should make a virtual method for this line and do something like
> > this.
> > - Add
> HTMLTextFormControlElement::shouldDispatchFormControlChangeEvent(oldValue,
> > newValue)
> > - Override it in HTMLInputElement::shouldDispatchFormControlChangeEvent
> > - HTMLInputElement::shouldDispatchFormControlChangeEvent calls
> > InputType::shouldDispatchChangeEvent
> > - Override InputType::shouldDispatchChangeEvent in CheckboxInputType
> 
> Done.

Is this CL working? I think dispatchFormControlChangeEvent will do nothing when
called from HTMLTextAreaElement::setValueCommon.
My intention was to make
HTMLTextFormControlElement::shouldDispatchFormControlChangeEvent return a bool.
HTMLTextFormControlElement::dispatchFormControlChangeEvent should call
shouldDispatchFormControlChangeEvent and checks that it returns true and then
fires the change event.

[Habib Virji, 2014-05-02 16:07:05]

On 2014/05/02 15:58:33, keishi1 wrote:
> On 2014/05/02 15:07:39, Habib Virji wrote:
> > Thanks keishi. Done as suggested.
> > 
> >
>
https://codereview.chromium.org/255163002/diff/20001/Source/core/html/HTMLTex...
> > File Source/core/html/HTMLTextFormControlElement.cpp (right):
> > 
> >
>
https://codereview.chromium.org/255163002/diff/20001/Source/core/html/HTMLTex...
> > Source/core/html/HTMLTextFormControlElement.cpp:192: if
> > (!equalIgnoringNullity(m_textAsOfLastFormControlChangeEvent, value()) ||
> > (isCheckbox() && m_textAsOfLastFormControlChangeEvent != value())) {
> > On 2014/05/02 07:44:55, keishi1 wrote:
> > > HTMLTextFormControlElement exists to share code between the
> > HTMLTextareaElement
> > > and HTMLInputElement so I don't think it should know about input types.
> > > 
> > > I think we should make a virtual method for this line and do something
like
> > > this.
> > > - Add
> > HTMLTextFormControlElement::shouldDispatchFormControlChangeEvent(oldValue,
> > > newValue)
> > > - Override it in HTMLInputElement::shouldDispatchFormControlChangeEvent
> > > - HTMLInputElement::shouldDispatchFormControlChangeEvent calls
> > > InputType::shouldDispatchChangeEvent
> > > - Override InputType::shouldDispatchChangeEvent in CheckboxInputType
> > 
> > Done.
> 
> Is this CL working? I think dispatchFormControlChangeEvent will do nothing
when
> called from HTMLTextAreaElement::setValueCommon.
> My intention was to make
> HTMLTextFormControlElement::shouldDispatchFormControlChangeEvent return a
bool.
> HTMLTextFormControlElement::dispatchFormControlChangeEvent should call
> shouldDispatchFormControlChangeEvent and checks that it returns true and then
> fires the change event.

Yes i tried fast/forms and everything worked. But as you guessed textarea has
test in fast/events and tried now they seem to be failing. 

I did check for checkbox and input types and they seemed to work.

[Habib Virji, 2014-05-02 16:34:38]

On 2014/05/02 15:58:33, keishi1 wrote:
> On 2014/05/02 15:07:39, Habib Virji wrote:
> > Thanks keishi. Done as suggested.
> > 
> >
>
https://codereview.chromium.org/255163002/diff/20001/Source/core/html/HTMLTex...
> > File Source/core/html/HTMLTextFormControlElement.cpp (right):
> > 
> >
>
https://codereview.chromium.org/255163002/diff/20001/Source/core/html/HTMLTex...
> > Source/core/html/HTMLTextFormControlElement.cpp:192: if
> > (!equalIgnoringNullity(m_textAsOfLastFormControlChangeEvent, value()) ||
> > (isCheckbox() && m_textAsOfLastFormControlChangeEvent != value())) {
> > On 2014/05/02 07:44:55, keishi1 wrote:
> > > HTMLTextFormControlElement exists to share code between the
> > HTMLTextareaElement
> > > and HTMLInputElement so I don't think it should know about input types.
> > > 
> > > I think we should make a virtual method for this line and do something
like
> > > this.
> > > - Add
> > HTMLTextFormControlElement::shouldDispatchFormControlChangeEvent(oldValue,
> > > newValue)
> > > - Override it in HTMLInputElement::shouldDispatchFormControlChangeEvent
> > > - HTMLInputElement::shouldDispatchFormControlChangeEvent calls
> > > InputType::shouldDispatchChangeEvent
> > > - Override InputType::shouldDispatchChangeEvent in CheckboxInputType
> > 
> > Done.
> 
> Is this CL working? I think dispatchFormControlChangeEvent will do nothing
when
> called from HTMLTextAreaElement::setValueCommon.
> My intention was to make
> HTMLTextFormControlElement::shouldDispatchFormControlChangeEvent return a
bool.
> HTMLTextFormControlElement::dispatchFormControlChangeEvent should call
> shouldDispatchFormControlChangeEvent and checks that it returns true and then
> fires the change event.

Updated as suggested. Please have a look.

[Habib Virji, 2014-05-06 11:09:41]

@keishi: Are the updates okay?

[tkent, 2014-05-07 01:23:37]

Do we have a similar bug for type=radio?  If so, we should fix it too.

[tkent, 2014-05-07 01:24:56]

https://codereview.chromium.org/255163002/diff/80001/Source/core/html/HTMLTex...
File Source/core/html/HTMLTextFormControlElement.cpp (right):

https://codereview.chromium.org/255163002/diff/80001/Source/core/html/HTMLTex...
Source/core/html/HTMLTextFormControlElement.cpp:193: if
(!equalIgnoringNullity(m_textAsOfLastFormControlChangeEvent, newValue)
The equalIgnoringNullity check should be in
HTMLTextFormControlElement::shouldDispatchFormControlChangeEvent and
InputType::shouldDispatchFormControlChangeEvent.

[keishi, 2014-05-07 01:26:11]

https://codereview.chromium.org/255163002/diff/80001/Source/core/html/HTMLTex...
File Source/core/html/HTMLTextFormControlElement.cpp (right):

https://codereview.chromium.org/255163002/diff/80001/Source/core/html/HTMLTex...
Source/core/html/HTMLTextFormControlElement.cpp:193: if
(!equalIgnoringNullity(m_textAsOfLastFormControlChangeEvent, newValue)
I think equalIgnoringNullity is part of the check required before dispatching
the change event. Is there any reason why this shouldn't be in
HTMLTextFormControlElement::shouldDispatchFormControlChangeEvent?

https://codereview.chromium.org/255163002/diff/80001/Source/core/html/forms/C...
File Source/core/html/forms/CheckboxInputType.cpp (right):

https://codereview.chromium.org/255163002/diff/80001/Source/core/html/forms/C...
Source/core/html/forms/CheckboxInputType.cpp:112: return (oldValue != newValue)
? true: false;
Couldn't this just be "oldValue != newValue"?

[Habib Virji, 2014-05-07 10:05:31]

On 2014/05/07 01:23:37, tkent wrote:
> Do we have a similar bug for type=radio?  If so, we should fix it too.

Yes same error was in input type radio, code updated and test added.

[Habib Virji, 2014-05-07 10:06:39]

Thanks Keishi and Tkent, code updated as suggested.

https://codereview.chromium.org/255163002/diff/80001/Source/core/html/HTMLTex...
File Source/core/html/HTMLTextFormControlElement.cpp (right):

https://codereview.chromium.org/255163002/diff/80001/Source/core/html/HTMLTex...
Source/core/html/HTMLTextFormControlElement.cpp:193: if
(!equalIgnoringNullity(m_textAsOfLastFormControlChangeEvent, newValue)
On 2014/05/07 01:24:57, tkent wrote:
> The equalIgnoringNullity check should be in
> HTMLTextFormControlElement::shouldDispatchFormControlChangeEvent and
> InputType::shouldDispatchFormControlChangeEvent.

Done.

https://codereview.chromium.org/255163002/diff/80001/Source/core/html/HTMLTex...
Source/core/html/HTMLTextFormControlElement.cpp:193: if
(!equalIgnoringNullity(m_textAsOfLastFormControlChangeEvent, newValue)
On 2014/05/07 01:26:11, keishi1 wrote:
> I think equalIgnoringNullity is part of the check required before dispatching
> the change event. Is there any reason why this shouldn't be in
> HTMLTextFormControlElement::shouldDispatchFormControlChangeEvent?

Done.

https://codereview.chromium.org/255163002/diff/80001/Source/core/html/forms/C...
File Source/core/html/forms/CheckboxInputType.cpp (right):

https://codereview.chromium.org/255163002/diff/80001/Source/core/html/forms/C...
Source/core/html/forms/CheckboxInputType.cpp:112: return (oldValue != newValue)
? true: false;
On 2014/05/07 01:26:11, keishi1 wrote:
> Couldn't this just be "oldValue != newValue"?

Done.

[tkent, 2014-05-07 10:09:56]

https://codereview.chromium.org/255163002/diff/120001/Source/core/html/HTMLIn...
File Source/core/html/HTMLInputElement.h (right):

https://codereview.chromium.org/255163002/diff/120001/Source/core/html/HTMLIn...
Source/core/html/HTMLInputElement.h:282: virtual bool
shouldDispatchFormControlChangeEvent(String&, String&) OVERRIDE;
This should be private.

https://codereview.chromium.org/255163002/diff/120001/Source/core/html/HTMLTe...
File Source/core/html/HTMLTextFormControlElement.h (right):

https://codereview.chromium.org/255163002/diff/120001/Source/core/html/HTMLTe...
Source/core/html/HTMLTextFormControlElement.h:75: virtual bool
shouldDispatchFormControlChangeEvent(String&, String&);
This should be protected.

https://codereview.chromium.org/255163002/diff/120001/Source/core/html/forms/...
File Source/core/html/forms/CheckboxInputType.h (right):

https://codereview.chromium.org/255163002/diff/120001/Source/core/html/forms/...
Source/core/html/forms/CheckboxInputType.h:52: virtual bool
shouldDispatchFormControlChangeEvent(String&, String&) OVERRIDE FINAL;
This should be moved to BaseCheckableInputType, and
RadioInputType::shouldDispatchFormControlChangeEvent should be removed.

[Habib Virji, 2014-05-07 10:27:09]

Done. Thanks.

https://codereview.chromium.org/255163002/diff/120001/Source/core/html/HTMLIn...
File Source/core/html/HTMLInputElement.h (right):

https://codereview.chromium.org/255163002/diff/120001/Source/core/html/HTMLIn...
Source/core/html/HTMLInputElement.h:282: virtual bool
shouldDispatchFormControlChangeEvent(String&, String&) OVERRIDE;
On 2014/05/07 10:09:56, tkent wrote:
> This should be private.

Done.

https://codereview.chromium.org/255163002/diff/120001/Source/core/html/HTMLTe...
File Source/core/html/HTMLTextFormControlElement.h (right):

https://codereview.chromium.org/255163002/diff/120001/Source/core/html/HTMLTe...
Source/core/html/HTMLTextFormControlElement.h:75: virtual bool
shouldDispatchFormControlChangeEvent(String&, String&);
On 2014/05/07 10:09:56, tkent wrote:
> This should be protected.

Done.

https://codereview.chromium.org/255163002/diff/120001/Source/core/html/forms/...
File Source/core/html/forms/CheckboxInputType.h (right):

https://codereview.chromium.org/255163002/diff/120001/Source/core/html/forms/...
Source/core/html/forms/CheckboxInputType.h:52: virtual bool
shouldDispatchFormControlChangeEvent(String&, String&) OVERRIDE FINAL;
On 2014/05/07 10:09:56, tkent wrote:
> This should be moved to BaseCheckableInputType, and
> RadioInputType::shouldDispatchFormControlChangeEvent should be removed.

Done.

[tkent, 2014-05-07 13:49:35]

lgtm

[commit-bot: I haz the power, 2014-05-07 13:50:24]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/habib.virji@samsung.com/255163002/130001

[commit-bot: I haz the power, 2014-05-07 14:32:57]

Change committed as 173530