Update of change event for input type number

Do not dispatch 'change' events during pressing spin buttons for input[type=number].

TextFieldInputType sends change event for its derived class. Change is
done for DispatchInputAndChangeEvent to behave similar to
DispatchChangeEvent.
DispatchChangeEvent is generated by Range, Radio, File and Checkbox
input type.
DispatchInputAndChangeEvent is generated by inputtype for step up and
down, BaseChooser/BaseMultipleFieldsDateandTime also generates
DispatchInputAndChangeEvent.
Since it is different set of inputs that generate DispatchChange and
DispatchInputAndChange, both are kept but behavior is changed; this
allows spin button to handle mouseup event.
Keyboard events are also handled for spin button.

R=tkent
BUG=155747
TEST=automated

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

[Habib Virji, 2014-01-08 12:24:31]

TextFieldInputType sends change event for its derived class. Change is done for
DispatchInputAndChangeEvent to behave similar to DispatchChangeEvent.
DispatchChangeEvent is generated by Range, Radio, File and Checkbox input
type.While DispatchInputAndChangeEvent is generated by input type for step up
and down, BaseChooser/BaseMultipleFieldsDateandTime also generates
DispatchInputAndChangeEvent.

Since it is different set of inputs that generate DispatchChange and
DispatchInputAndChange, both are kept but behavior is changed; this allows spin
button to handle mouseup event. Keyboard events are also handled for spin
button.

[tkent, 2014-01-14 01:06:46]

https://codereview.chromium.org/128133002/diff/1/Source/core/html/forms/TextF...
File Source/core/html/forms/TextFieldInputType.cpp (right):

https://codereview.chromium.org/128133002/diff/1/Source/core/html/forms/TextF...
Source/core/html/forms/TextFieldInputType.cpp:181: case
DispatchInputAndChangeEvent:
This is not acceptable as I already wrote.

[Habib Virji, 2014-01-14 16:07:09]

On 2014/01/14 01:06:46, tkent wrote:
>
https://codereview.chromium.org/128133002/diff/1/Source/core/html/forms/TextF...
> File Source/core/html/forms/TextFieldInputType.cpp (right):
> 
>
https://codereview.chromium.org/128133002/diff/1/Source/core/html/forms/TextF...
> Source/core/html/forms/TextFieldInputType.cpp:181: case
> DispatchInputAndChangeEvent:
> This is not acceptable as I already wrote.

Updated event generated in InputType.cpp::StepUpFromRenderer to send
DispatchChangeEvent instead of DispatchInputAndChangeEvent as
DispatchChangeEvent, exhibits correct behavior i,e. If in focus, then change
event is not generated and if out of focus change event is generated. 

EventHandling for mouse is handled in SpinButtonElement. TextFieldInputType
handles keyboard event for spin button, updated to send changeEvent on any
change.

Agree with your suggestion of removing DispatchInputAndChangeEvent event, as
DispatchChangeEvent also handles dispatchFormControlInputEvent. 

This CL covers removing of DispatchInputAndChangeEvent in StepUpFromRenderer,
other place it is used in DateTime. If agreed in next CL(128153002); can remove
DispatchFormControlInputEvent event.

[tkent, 2014-01-15 00:19:44]

We need a test to confirm that 'change' events are not dispatched during
pressing a spin button.

https://codereview.chromium.org/128133002/diff/60001/Source/core/html/shadow/...
File Source/core/html/shadow/SpinButtonElement.cpp (right):

https://codereview.chromium.org/128133002/diff/60001/Source/core/html/shadow/...
Source/core/html/shadow/SpinButtonElement.cpp:118:
input->dispatchFormControlChangeEvent();
We need to call it in SpinButtonElement::releaseCapture because we have to
dispatch 'change' event when the element gets disabled.
BTW, probably the stopRepeatingTimer() here should be releaseCapture().

[Habib Virji, 2014-01-15 09:50:53]

On 2014/01/15 00:19:44, tkent wrote:
> We need a test to confirm that 'change' events are not dispatched during
> pressing a spin button.
> 
>
https://codereview.chromium.org/128133002/diff/60001/Source/core/html/shadow/...
> File Source/core/html/shadow/SpinButtonElement.cpp (right):
> 
>
https://codereview.chromium.org/128133002/diff/60001/Source/core/html/shadow/...
> Source/core/html/shadow/SpinButtonElement.cpp:118:
> input->dispatchFormControlChangeEvent();
> We need to call it in SpinButtonElement::releaseCapture because we have to
> dispatch 'change' event when the element gets disabled.
> BTW, probably the stopRepeatingTimer() here should be releaseCapture().

Updated as per review comments. 

Moved onchange to release capture.
Added test case in number type update for checking mouse event.

[tkent, 2014-01-16 06:05:19]

https://codereview.chromium.org/128133002/diff/110001/LayoutTests/fast/forms/...
File LayoutTests/fast/forms/number/number-type-update-by-change-event.html
(right):

https://codereview.chromium.org/128133002/diff/110001/LayoutTests/fast/forms/...
LayoutTests/fast/forms/number/number-type-update-by-change-event.html:24:
function testMouseChangeEvent(){
Please do not modify this test.  This test is for 'type-update-by-change-event',
and we shouldn't add other purposes.

https://codereview.chromium.org/128133002/diff/110001/LayoutTests/fast/forms/...
LayoutTests/fast/forms/number/number-type-update-by-change-event.html:30:
eventSender.mouseDown();
Why two mouseDown()s?

https://codereview.chromium.org/128133002/diff/110001/LayoutTests/fast/forms/...
LayoutTests/fast/forms/number/number-type-update-by-change-event.html:31:
shouldBe('changeEventCounter', '1');
We should press the mouse button for a while, and confirm 'input' events are
dispatched and no 'change' events are dispatched.

https://codereview.chromium.org/128133002/diff/110001/Source/core/html/shadow...
File Source/core/html/shadow/SpinButtonElement.cpp (right):

https://codereview.chromium.org/128133002/diff/110001/Source/core/html/shadow...
Source/core/html/shadow/SpinButtonElement.cpp:202: HTMLInputElement* input =
toHTMLInputElement(shadowHost());
Please do not add dependency to HTMLInputElement.
We should add new callback to SpinButtonOwner class, and owners should implement
it.

[Habib Virji, 2014-01-16 09:49:18]

https://codereview.chromium.org/128133002/diff/110001/LayoutTests/fast/forms/...
File LayoutTests/fast/forms/number/number-type-update-by-change-event.html
(right):

https://codereview.chromium.org/128133002/diff/110001/LayoutTests/fast/forms/...
LayoutTests/fast/forms/number/number-type-update-by-change-event.html:24:
function testMouseChangeEvent(){
On 2014/01/16 06:05:19, tkent wrote:
> Please do not modify this test.  This test is for
'type-update-by-change-event',
> and we shouldn't add other purposes.

Is adding a new test file for the purpose okay? As in CL 128153002, I have
updated a common-spinbutton-change-and-input-events.js, to check input and
change event trigger.

https://codereview.chromium.org/128133002/diff/110001/LayoutTests/fast/forms/...
LayoutTests/fast/forms/number/number-type-update-by-change-event.html:30:
eventSender.mouseDown();
On 2014/01/16 06:05:19, tkent wrote:
> Why two mouseDown()s?

Wanted to show that multiple mouseDown does not trigger change event. It is
triggered only when mouseUp is called.

https://codereview.chromium.org/128133002/diff/110001/LayoutTests/fast/forms/...
LayoutTests/fast/forms/number/number-type-update-by-change-event.html:31:
shouldBe('changeEventCounter', '1');
On 2014/01/16 06:05:19, tkent wrote:
> We should press the mouse button for a while, and confirm 'input' events are
> dispatched and no 'change' events are dispatched.

Was not sure how to simulate press mouse button, that's why implemented above 2
mouseDown.

https://codereview.chromium.org/128133002/diff/110001/Source/core/html/shadow...
File Source/core/html/shadow/SpinButtonElement.cpp (right):

https://codereview.chromium.org/128133002/diff/110001/Source/core/html/shadow...
Source/core/html/shadow/SpinButtonElement.cpp:202: HTMLInputElement* input =
toHTMLInputElement(shadowHost());
On 2014/01/16 06:05:19, tkent wrote:
> Please do not add dependency to HTMLInputElement.
> We should add new callback to SpinButtonOwner class, and owners should
implement
> it.
OK will update code accordingly and send a new patch.

[Habib Virji, 2014-01-16 12:25:12]

Uploaded new patch set

[tkent, 2014-01-17 02:19:19]

Some minor comments.

https://codereview.chromium.org/128133002/diff/390002/LayoutTests/fast/forms/...
File LayoutTests/fast/forms/number/number-spinbutton-changeevent-trigger.html
(right):

https://codereview.chromium.org/128133002/diff/390002/LayoutTests/fast/forms/...
LayoutTests/fast/forms/number/number-spinbutton-changeevent-trigger.html:11:
++changeEventCounter;
We usually use four-space indentation in JavaScript code.

https://codereview.chromium.org/128133002/diff/390002/LayoutTests/fast/forms/...
LayoutTests/fast/forms/number/number-spinbutton-changeevent-trigger.html:25:
shouldBe('changeEventCounter', '0');
Good.  This check demonstrates that change events are not dispatched with
'input' events.

https://codereview.chromium.org/128133002/diff/390002/Source/core/html/forms/...
File Source/core/html/forms/TextFieldInputType.cpp (right):

https://codereview.chromium.org/128133002/diff/390002/Source/core/html/forms/...
Source/core/html/forms/TextFieldInputType.cpp:225:
spinButtonDispatchChangeEvent();
Calling a SpinButtonOwner function here looks strange (including
spinButtonStepUp/Down above).  We had better call
element().dispatchFormControlChangeEvent directly.

https://codereview.chromium.org/128133002/diff/390002/Source/core/html/shadow...
File Source/core/html/shadow/SpinButtonElement.h (right):

https://codereview.chromium.org/128133002/diff/390002/Source/core/html/shadow...
Source/core/html/shadow/SpinButtonElement.h:52: virtual void
spinButtonDispatchChangeEvent() = 0;
The function name isn't good. SpinButton class should not be aware of what will
be done in this function.
I propose 'spinButtonDidReleaseMouseCapture.'

[Habib Virji, 2014-01-17 08:35:48]

On 2014/01/17 02:19:19, tkent wrote:
> Some minor comments.
> 
>
https://codereview.chromium.org/128133002/diff/390002/LayoutTests/fast/forms/...
> File LayoutTests/fast/forms/number/number-spinbutton-changeevent-trigger.html
> (right):
> 
>
https://codereview.chromium.org/128133002/diff/390002/LayoutTests/fast/forms/...
> LayoutTests/fast/forms/number/number-spinbutton-changeevent-trigger.html:11:
> ++changeEventCounter;
> We usually use four-space indentation in JavaScript code.
> 
>
https://codereview.chromium.org/128133002/diff/390002/LayoutTests/fast/forms/...
> LayoutTests/fast/forms/number/number-spinbutton-changeevent-trigger.html:25:
> shouldBe('changeEventCounter', '0');
> Good.  This check demonstrates that change events are not dispatched with
> 'input' events.
> 
>
https://codereview.chromium.org/128133002/diff/390002/Source/core/html/forms/...
> File Source/core/html/forms/TextFieldInputType.cpp (right):
> 
>
https://codereview.chromium.org/128133002/diff/390002/Source/core/html/forms/...
> Source/core/html/forms/TextFieldInputType.cpp:225:
> spinButtonDispatchChangeEvent();
> Calling a SpinButtonOwner function here looks strange (including
> spinButtonStepUp/Down above).  We had better call
> element().dispatchFormControlChangeEvent directly.
> 
>
https://codereview.chromium.org/128133002/diff/390002/Source/core/html/shadow...
> File Source/core/html/shadow/SpinButtonElement.h (right):
> 
>
https://codereview.chromium.org/128133002/diff/390002/Source/core/html/shadow...
> Source/core/html/shadow/SpinButtonElement.h:52: virtual void
> spinButtonDispatchChangeEvent() = 0;
> The function name isn't good. SpinButton class should not be aware of what
will
> be done in this function.
> I propose 'spinButtonDidReleaseMouseCapture.'

- Updated test file to 4 spaces.
- Updated callback name to spinButtonDidReleaseMouseCapture.
- Can you please review too, corresponding change event for date time input
type: https://codereview.chromium.org/128153002/

[tkent, 2014-01-17 09:39:17]

lgtm

[commit-bot: I haz the power, 2014-01-17 09:39:36]

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

[commit-bot: I haz the power, 2014-01-17 10:45:48]

Change committed as 165300

[tkent, 2014-01-20 03:44:11]

A revert of this CL has been created in
https://codereview.chromium.org/140793012/ by tkent@chromium.org.

The reason for reverting is: I'm afraid It made
time-multiple-fields-spinbutton-change-and-input-events.html falky.
.

[tkent, 2014-01-20 03:52:19]

On 2014/01/20 03:44:11, tkent wrote:
> time-multiple-fields-spinbutton-change-and-input-events.html falky.

falky -> flaky

[tkent, 2014-01-20 03:53:34]

See
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40...

[tkent, 2014-01-20 04:30:07]

I'm sorry that the revert didn't fix the flakiness.  I'll revert the revert.

[tkent, 2014-02-12 07:37:46]

A revert of this CL has been created in
https://codereview.chromium.org/159483004/ by tkent@chromium.org.

The reason for reverting is: INPUT element dispatches extra events.
.

[tkent, 2014-02-12 08:02:28]

On 2014/02/12 07:37:46, tkent wrote:
> A revert of this CL has been created in
> https://codereview.chromium.org/159483004/ by mailto:tkent@chromium.org.
> 
> The reason for reverting is: INPUT element dispatches extra events.
> .

Reverted manually: https://codereview.chromium.org/157813010/