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

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

InputType to triggers change event when not in focus and input event when in focus.
The change event is triggered only when user stops pressing spin button.

R=tkent, keishi1
BUG=155747
TEST=Change event is only trigger when mouse press is stopped. Only 1 change event is triggered when moving out of focus.

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

[Habib Virji, 2014-03-13 14:42:44]

Resubmission of spin button change event trigger.

The reason for resubmitting issue was found why two change event could have been
triggered. 

When input field loses focus and value is set, it triggers changeEvent from
TextFieldInputType::setValue. DispatchChangeEvent condition input->focused will
be false and this triggers change event. Also spinButtonDispatchChangeEvent
triggers changeEvent as it does not check if element is in focus. This CL fixes
and most changes from previous CL are kept same. 

Test is updated to check if input loses focus no change event is being triggered

[tkent, 2014-03-14 01:51:18]

The patch still dispatches unnecessary 'change' event.
Try
1. Load   data:text/html,<input type=number autofocus onchange="alert('FAIL')">
2. Move the mouse pointer over the spin button.

It's because releaseCapture() in mousemove event handling calls
dispatchFormControlChangeEvent() even though we don't change anything. For now,
we can't call dispatchFormControlChangeEvent() when the value has never updated,
and we need to initialize
HTMLTextFormControlElement::m_textAsOfLastFormControlChangeEvent correctly as I
wrote repeatedly.

[Habib Virji, 2014-03-14 16:10:32]

Thanks for the code. Was missing this and was not able to figure out the reason
for extra event.

Found it is not directly related to m_textAsOfLastFormControlChangeEvent, but
related to wrong value returned by value() in dispatchFormControlChangeEvent().
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...)

Reason for extra event was mismatch of the null empty string. Same code if value
is present does not generate event.
1. In above code m_textAsOfLastFormControlChangeEvent is initialized by
HTMLTextFormControlElement::insertedInto as emptyString()
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...)
2. In HTMLInputElement::value(), String is unitialized and it returns
fallbackValue. FallbackValue was missing for input type number, so it returns
"null". 
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...)
3. "" and "null", mismatch was generating an extra event.

Also updated not to include spinButtonDidReleaseMouseCapture inside m_capturing
in releaseCapture as it could lead to issue with when mouse not moved, value
changed not triggerring change event. 

In HTMLTextFormControlElement.cpp, has dispatchChangeEvent after
setTextAsOfLastFormControlChangeEvent as when JS attribute thread is triggered,
there are two separate threads, one from v8 setting attribute which then trigger
detach and other running on main thread running dispatchChangeEvent. Detach
triggers spinButtonDidReleaseMouseCapture as
m_textAsOfLastFormControlChangeEvent is not set, again a change event is
triggered. Setting before dispatch helps in case event is being processed and JS
also change attribute. This was reproduced via failing test for
number-type-update-by-change-event, which resets type.

Have added extra test to check on hover no change event being generated.

[tkent, 2014-03-18 08:17:00]

On 2014/03/14 16:10:32, Habib Virji wrote:
> 1. In above code m_textAsOfLastFormControlChangeEvent is initialized by
> HTMLTextFormControlElement::insertedInto as emptyString()
>
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...)
> 2. In HTMLInputElement::value(), String is unitialized and it returns
> fallbackValue. FallbackValue was missing for input type number, so it returns
> "null". 
>
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...)
> 3. "" and "null", mismatch was generating an extra event.

Defining fallbackValue() only for NumberInputType doesn't look reasonable.
TextFiledInputType doesn't have fallbackValue(), and NumberInputType is a kind
of TextFieldInputType.  I recommend to change the matching condition of
m_textAsOfLastFormControlChangeEvent in
HTMLTextFormControlElement::dispatchFormControlChangeEvent() like

    if (!equalIgnoringNullity(m_textAsOfLastFormControlChangeEvent, value())) {



> In HTMLTextFormControlElement.cpp, has dispatchChangeEvent after
> setTextAsOfLastFormControlChangeEvent as when JS attribute thread is
triggered,
> there are two separate threads, one from v8 setting attribute which then
trigger
> detach and other running on main thread running dispatchChangeEvent. Detach
> triggers spinButtonDidReleaseMouseCapture as
> m_textAsOfLastFormControlChangeEvent is not set, again a change event is
> triggered. Setting before dispatch helps in case event is being processed and
JS
> also change attribute. This was reproduced via failing test for
> number-type-update-by-change-event, which resets type.

We don't have multiple threads in such case.  Everything happens in a single
thread.  The CL description should have the reason of this non-obvious change.

[tkent, 2014-03-18 08:26:19]

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

https://codereview.chromium.org/196933006/diff/20001/LayoutTests/fast/forms/n...
LayoutTests/fast/forms/number/number-spinbutton-changeevent-trigger.html:33:
shouldBe('numberInput1.value', '"1"');
Use shouldBeEqualToString

https://codereview.chromium.org/196933006/diff/20001/LayoutTests/fast/forms/n...
LayoutTests/fast/forms/number/number-spinbutton-changeevent-trigger.html:39:
shouldBe('numberInput1.value', '"1"');
shouldBeEqualToString

https://codereview.chromium.org/196933006/diff/20001/LayoutTests/fast/forms/n...
LayoutTests/fast/forms/number/number-spinbutton-changeevent-trigger.html:47:
shouldBe('numberInput1.value', '"2"');
shouldBeEqualToString

https://codereview.chromium.org/196933006/diff/20001/LayoutTests/fast/forms/n...
LayoutTests/fast/forms/number/number-spinbutton-changeevent-trigger.html:53:
shouldBe('numberInput1.value', '"2"');
shouldBeEqualToString

https://codereview.chromium.org/196933006/diff/20001/Source/core/html/forms/B...
File Source/core/html/forms/BaseMultipleFieldsDateAndTimeInputType.cpp (right):

https://codereview.chromium.org/196933006/diff/20001/Source/core/html/forms/B...
Source/core/html/forms/BaseMultipleFieldsDateAndTimeInputType.cpp:245: if
(element().focused())
Why is this check needed?

We should dispatch two change events in the following scenario:
 - Mousedown on a spin button
   INPUT gets focus
 - The INPUT looses focus for some reasons.
   The first 'change' event should be dispatched.
 - Mouseup on a spin button
  The second 'change' event should be dispatch if the value is changed.

https://codereview.chromium.org/196933006/diff/20001/Source/core/html/forms/T...
File Source/core/html/forms/TextFieldInputType.cpp (right):

https://codereview.chromium.org/196933006/diff/20001/Source/core/html/forms/T...
Source/core/html/forms/TextFieldInputType.cpp:215: if (element().focused())
Why is the focus check needed? It looks unnecessary.

https://codereview.chromium.org/196933006/diff/20001/Source/core/html/forms/T...
Source/core/html/forms/TextFieldInputType.cpp:567: if (element().focused())
Ditto.

[Habib Virji, 2014-03-18 08:54:37]

Thanks for reviewing. 

"We don't have multiple threads in such case.  Everything happens in a single
thread.  The CL description should have the reason of this non-obvious change."
- Sorry for using threads word, what I meant was changeEvent when trigerred
inside changeEvent handler on JS side, a type is changed of the input type
number. Value is not changed, but since value is updated after dispatch event
function returns, it results in extra change event to be triggered. So point was
since on JS, type can be changed better to set the value before dispatching
event.

[Habib Virji, 2014-03-18 17:37:26]

Removed fallbackValue and added equalIgnoringNullity.

Added a condition to generate changeevent on mousedown and mouseout for 2 change
events. MouseUp change event works from releaseCapture changes.

Mousedown changes are based on when continuous pressed mouseButton triggers
click event not mouseDown event. 

Need to add one more test for testing inputCounter is incremented but not
changeEventCounter. Have not been able to generate click event. Is it possible
to simulate continuous press of mouseButton without calling mouseDown/mouseUp in
tests?

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

https://codereview.chromium.org/196933006/diff/20001/LayoutTests/fast/forms/n...
LayoutTests/fast/forms/number/number-spinbutton-changeevent-trigger.html:33:
shouldBe('numberInput1.value', '"1"');
On 2014/03/18 08:26:20, tkent wrote:
> Use shouldBeEqualToString

Done.

https://codereview.chromium.org/196933006/diff/20001/LayoutTests/fast/forms/n...
LayoutTests/fast/forms/number/number-spinbutton-changeevent-trigger.html:39:
shouldBe('numberInput1.value', '"1"');
On 2014/03/18 08:26:20, tkent wrote:
> shouldBeEqualToString

Done.

https://codereview.chromium.org/196933006/diff/20001/LayoutTests/fast/forms/n...
LayoutTests/fast/forms/number/number-spinbutton-changeevent-trigger.html:47:
shouldBe('numberInput1.value', '"2"');
On 2014/03/18 08:26:20, tkent wrote:
> shouldBeEqualToString

Done.

https://codereview.chromium.org/196933006/diff/20001/LayoutTests/fast/forms/n...
LayoutTests/fast/forms/number/number-spinbutton-changeevent-trigger.html:53:
shouldBe('numberInput1.value', '"2"');
On 2014/03/18 08:26:20, tkent wrote:
> shouldBeEqualToString

Done.

https://codereview.chromium.org/196933006/diff/20001/Source/core/html/forms/B...
File Source/core/html/forms/BaseMultipleFieldsDateAndTimeInputType.cpp (right):

https://codereview.chromium.org/196933006/diff/20001/Source/core/html/forms/B...
Source/core/html/forms/BaseMultipleFieldsDateAndTimeInputType.cpp:245: if
(element().focused())
It is needed since releaseCapture is called from detach, resulting in change
event dispatch.   This is to just make sure dispatch event only when in focus. 

Added condition in SpinButton to dispatch a change event in case of mouseDown
and also if mouseout occurs after being in focus. 

On 2014/03/18 08:26:20, tkent wrote:
> Why is this check needed?
> 
> We should dispatch two change events in the following scenario:
>  - Mousedown on a spin button
>    INPUT gets focus
>  - The INPUT looses focus for some reasons.
>    The first 'change' event should be dispatched.
>  - Mouseup on a spin button
>   The second 'change' event should be dispatch if the value is changed.
>

[tkent, 2014-03-25 02:12:12]

I'll take a look at this in the next week.
This change is risky, and we're about to branch M35.

[Habib Virji, 2014-03-25 09:16:48]

On 2014/03/25 02:12:12, tkent wrote:
> I'll take a look at this in the next week.
> This change is risky, and we're about to branch M35.

Thanks for keeping an eye on this issue. 

No issues.

[tkent, 2014-03-31 07:50:39]

https://codereview.chromium.org/196933006/diff/20001/Source/core/html/forms/B...
File Source/core/html/forms/BaseMultipleFieldsDateAndTimeInputType.cpp (right):

https://codereview.chromium.org/196933006/diff/20001/Source/core/html/forms/B...
Source/core/html/forms/BaseMultipleFieldsDateAndTimeInputType.cpp:245: if
(element().focused())
On 2014/03/18 17:37:27, Habib Virji wrote:
> It is needed since releaseCapture is called from detach, resulting in change
> event dispatch.   This is to just make sure dispatch event only when in focus.

> 
> Added condition in SpinButton to dispatch a change event in case of mouseDown
> and also if mouseout occurs after being in focus. 
> 
> On 2014/03/18 08:26:20, tkent wrote:
> > Why is this check needed?
> > 
> > We should dispatch two change events in the following scenario:
> >  - Mousedown on a spin button
> >    INPUT gets focus
> >  - The INPUT looses focus for some reasons.
> >    The first 'change' event should be dispatched.
> >  - Mouseup on a spin button
> >   The second 'change' event should be dispatch if the value is changed.
> > 
> 

I see.  We should not dispatch any events in detach().
However, if the value of an INPUT element was changed, we have to dispatch a
'change' event.  Hmm we should change the behavior so that we release mouse
capturing when the host INPUT element looses focus.

[Habib Virji, 2014-03-31 15:41:46]

The issue is highlighted in particular in detach called from radioNodeList
scenario(form-collection-radio-node-list.html). 
1. container.parentNode.removeChild(container) invokes spinButton detach event. 
2. m_textAsOfLastFormControlChangeEvent holds value inputRadioValue which is set
by radioButton, this value is set from insertedInto, which is called when
element is created and is not set using setValue, so value() is empty.  
3. SpinButton::detach -> releaseCapture()-> changeEvent. Since value do not
match, change event is tried to be triggered and crashes renderer. 

Did not fathom why exactly removeBetween in ContainerNode invokes SpinButton
with HTMLTextFormControl holding value of radioButton when executing spinButton
detach(), HTMLTextFormControl should have state of SpinButton with value 456 but
it has value of other element. Please suggest is this expected?

SpinButton detach thus need a check whether to releaseCapture, problem
isSpinButton is always true so have used m_capturing, as it can suggest if
mouseEvent occurred. The only scenario where m_capturing falters is when mouse
is not moved and still values are changed, but presumably when detach is called
mouse must be moved or blur should occur. 

Other changes:
- TextFieldInputType has change as setTextAsOfLastFormControlChangeEvent was
being called twice when event is NoDispatchEvent, once by HTMLInputElement and
other by TextFieldInputType. So has made check in TextFieldInputType to avoid
extra call. 
- Removed element isFocused checks.

[tkent, 2014-04-03 04:41:48]

Please follow my previous comment.
'change' event still can be dispatched in detach() with your patch.  It would be
a security issue.

[Habib Virji, 2014-04-03 10:14:14]

On 2014/04/03 04:41:48, tkent wrote:
> Please follow my previous comment.
> 'change' event still can be dispatched in detach() with your patch.  It would
be
> a security issue.

Have moved releaseCapture from inside detach to when input loses focus via
setFocus(false). Reason for removing detach was it only does release mouse which
is now handled when input loses focus. 

Other possibility was keep detach and create a separate function to dispatch
change event and call it all places where releaseCapture is called except in
detach, but thought would be an overhead. 

Hope this patch matches with what you suggested in comment#10.

[tkent, 2014-04-04 01:31:46]

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

https://codereview.chromium.org/196933006/diff/100001/Source/core/html/forms/...
Source/core/html/forms/TextFieldInputType.cpp:189: if (!input->focused() &&
eventBehavior != DispatchNoEvent)
I think this change will make an incompatible behavior change.
e.g.

input.focus();
... a user types into the input.
input.value = "abc"
input.blur();

This should NOT dispatch a change event on blur in the current policy.

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

https://codereview.chromium.org/196933006/diff/100001/Source/core/html/shadow...
Source/core/html/shadow/SpinButtonElement.cpp:66: releaseCapture();
I think this code won't work.
SpinButtonElement never has focus.  We need to do releaseCapture when the host
<input> looses focus.

https://codereview.chromium.org/196933006/diff/100001/Source/core/html/shadow...
Source/core/html/shadow/SpinButtonElement.cpp:136: } else if (event->type() ==
EventTypeNames::mouseout) {
Why is this block necessary? Doesn't the above releaseCapture() work?
Please explain it in the CL description.

[Habib Virji, 2014-04-04 09:24:04]

Comment#1: I did not consider the mentioned scenario. Removed the code.

Comment#2: Moved code to handleBlur inside TextFieldInput. 

Comment#3: Removed the code, the reason added was to address your comments in
comment#5.

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

https://codereview.chromium.org/196933006/diff/100001/Source/core/html/forms/...
Source/core/html/forms/TextFieldInputType.cpp:189: if (!input->focused() &&
eventBehavior != DispatchNoEvent)
On 2014/04/04 01:31:46, tkent wrote:
> I think this change will make an incompatible behavior change.
> e.g.
> 
> input.focus();
> ... a user types into the input.
> input.value = "abc"
> input.blur();
> 
> This should NOT dispatch a change event on blur in the current policy.

Done.

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

https://codereview.chromium.org/196933006/diff/100001/Source/core/html/shadow...
Source/core/html/shadow/SpinButtonElement.cpp:66: releaseCapture();
On 2014/04/04 01:31:46, tkent wrote:
> I think this code won't work.
> SpinButtonElement never has focus.  We need to do releaseCapture when the host
> <input> looses focus.

Done.

https://codereview.chromium.org/196933006/diff/100001/Source/core/html/shadow...
Source/core/html/shadow/SpinButtonElement.cpp:136: } else if (event->type() ==
EventTypeNames::mouseout) {
On 2014/04/04 01:31:46, tkent wrote:
> Why is this block necessary? Doesn't the above releaseCapture() work?
> Please explain it in the CL description.

Done.

[tkent, 2014-04-07 04:31:17]

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

https://codereview.chromium.org/196933006/diff/120001/Source/core/html/shadow...
Source/core/html/shadow/SpinButtonElement.cpp:104:
m_spinButtonOwner->spinButtonDidReleaseMouseCapture();
Why pinButtonDidReleaseMouseCapture is called?  We don't release mouse capture,
and we don't want to dispatch 'change' events on mousedown.

[Habib Virji, 2014-04-07 07:33:18]

Apologise, it was from double change events changes. UPload new patch without
those changes.

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

https://codereview.chromium.org/196933006/diff/120001/Source/core/html/shadow...
Source/core/html/shadow/SpinButtonElement.cpp:104:
m_spinButtonOwner->spinButtonDidReleaseMouseCapture();
On 2014/04/07 04:31:17, tkent wrote:
> Why pinButtonDidReleaseMouseCapture is called?  We don't release mouse
capture,
> and we don't want to dispatch 'change' events on mousedown.

Done.

[tkent, 2014-04-07 07:53:26]

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

https://codereview.chromium.org/196933006/diff/140001/LayoutTests/fast/forms/...
LayoutTests/fast/forms/number/number-spinbutton-changeevent-trigger.html:29:
eventSender.mouseUp();
We should test if mouseDown won't dispatch a change event.
should be:

eventSender.mouseDown();
debug(...)
shouldBe('inputEventCounter', '1');
shouldBe('changeEventCounter', '0');
shouldBeEqualToString('numberInput1.value', '1');
eventSender.mouseUp();
shouldBe('inputEventCounter', '1');
shouldBe('changeEventCounter', '1');
shouldBeEqualToString('numberInput1.value', '1');

[Habib Virji, 2014-04-07 10:02:42]

Thanks updated.

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

https://codereview.chromium.org/196933006/diff/140001/LayoutTests/fast/forms/...
LayoutTests/fast/forms/number/number-spinbutton-changeevent-trigger.html:29:
eventSender.mouseUp();
On 2014/04/07 07:53:26, tkent wrote:
> We should test if mouseDown won't dispatch a change event.
> should be:
> 
> eventSender.mouseDown();
> debug(...)
> shouldBe('inputEventCounter', '1');
> shouldBe('changeEventCounter', '0');
> shouldBeEqualToString('numberInput1.value', '1');
> eventSender.mouseUp();
> shouldBe('inputEventCounter', '1');
> shouldBe('changeEventCounter', '1');
> shouldBeEqualToString('numberInput1.value', '1');
> 

Done.

[tkent, 2014-04-07 23:15:09]

lgtm.

Thank you for the hard work.  Now the code change looks very simple and clean!

[tkent, 2014-04-07 23:15:13]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2014-04-07 23:15:19]

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

[commit-bot: I haz the power, 2014-04-07 23:36:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-07 23:36:05]

Try jobs failed on following builders:
  tryserver.blink on win_blink_compile_dbg

[tkent, 2014-04-07 23:42:51]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2014-04-07 23:42:54]

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

[commit-bot: I haz the power, 2014-04-08 00:08:44]

Change committed as 171006

[Habib Virji, 2014-04-08 05:18:42]

Thanks for your patience it was your comments and directions that made it
possible. Will upload new code for date change event.