OnChange event should fire if the multiple selection changes in listbox using mouse

listboxdefaulteventhandler in htmlselectelement is not called when mouse is released outside if listbox is non scrollable.
Hence onchange event is not fired for htmlselectelement. 
But when listbox is scrollable, Autoscrollcontroller is used to generate on change event.
Forcing Autoscrollcontroller to be used even when listbox is not scrollable. 
By commenting defaulthandled for mousemove in listboxdefaulteventhandler will cause handlemousedraggedevent to be called. 
This will start the Autoscrollcontroller for listbox selection. 
Which inturn will generate the onchange event.
The changes didnot cause any side effect and layout test failure.

BUG=317809, 94986
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=164563

[gnana, 2013-12-28 15:44:33]

Please have a look at this.

[eseidel, 2013-12-28 19:56:59]

I don't understand what you're trying to do here.  Can you please explain better
in your CL description more exactly what you're attempting to change?

https://codereview.chromium.org/120373005/diff/200001/LayoutTests/fast/events...
File LayoutTests/fast/events/multiselect-in-listbox-mouse-release-outside.html
(right):

https://codereview.chromium.org/120373005/diff/200001/LayoutTests/fast/events...
LayoutTests/fast/events/multiselect-in-listbox-mouse-release-outside.html:10:
function lstSelect_change(s, e)
What is "lst"?  Is that short for "list"?  or "listbox"?  Also the style of
function name is odd here.  Do you mean listboxSelectionChanged?

https://codereview.chromium.org/120373005/diff/200001/LayoutTests/fast/events...
LayoutTests/fast/events/multiselect-in-listbox-mouse-release-outside.html:12:
log("PASSED");
nit: typically tests say "PASS" or "FAIL", but "PASSED" is also OK.

https://codereview.chromium.org/120373005/diff/200001/Source/core/html/HTMLSe...
File Source/core/html/HTMLSelectElement.cpp (right):

https://codereview.chromium.org/120373005/diff/200001/Source/core/html/HTMLSe...
Source/core/html/HTMLSelectElement.cpp:77: , m_onChangeFired(true)
When the element is created the "on change" has already fired??

https://codereview.chromium.org/120373005/diff/200001/Source/core/html/HTMLSe...
Source/core/html/HTMLSelectElement.cpp:1328: // event->setDefaultHandled();
Please don't commit commented out code.

[gnana, 2014-01-03 07:50:02]

Thanks eric for the review.
I have updated the CL description.

https://codereview.chromium.org/120373005/diff/200001/LayoutTests/fast/events...
File LayoutTests/fast/events/multiselect-in-listbox-mouse-release-outside.html
(right):

https://codereview.chromium.org/120373005/diff/200001/LayoutTests/fast/events...
LayoutTests/fast/events/multiselect-in-listbox-mouse-release-outside.html:10:
function lstSelect_change(s, e)
On 2013/12/28 19:57:00, eseidel wrote:
> What is "lst"?  Is that short for "list"?  or "listbox"?  Also the style of
> function name is odd here.  Do you mean listboxSelectionChanged?
Yes. i changed it

https://codereview.chromium.org/120373005/diff/200001/LayoutTests/fast/events...
LayoutTests/fast/events/multiselect-in-listbox-mouse-release-outside.html:12:
log("PASSED");
On 2013/12/28 19:57:00, eseidel wrote:
> nit: typically tests say "PASS" or "FAIL", but "PASSED" is also OK.

Done.

https://codereview.chromium.org/120373005/diff/200001/Source/core/html/HTMLSe...
File Source/core/html/HTMLSelectElement.cpp (right):

https://codereview.chromium.org/120373005/diff/200001/Source/core/html/HTMLSe...
Source/core/html/HTMLSelectElement.cpp:77: , m_onChangeFired(true)
On 2013/12/28 19:57:00, eseidel wrote:
> When the element is created the "on change" has already fired??
In my latest patch i dont use it any more so i removed it.

https://codereview.chromium.org/120373005/diff/200001/Source/core/html/HTMLSe...
Source/core/html/HTMLSelectElement.cpp:1328: // event->setDefaultHandled();
On 2013/12/28 19:57:00, eseidel wrote:
> Please don't commit commented out code.

Done.

[tkent, 2014-01-06 03:04:58]

https://codereview.chromium.org/120373005/diff/300004/LayoutTests/fast/events...
File LayoutTests/fast/events/multiselect-in-listbox-mouse-release-outside.html
(right):

https://codereview.chromium.org/120373005/diff/300004/LayoutTests/fast/events...
LayoutTests/fast/events/multiselect-in-listbox-mouse-release-outside.html:1:
<html>
I prefer putting this file in LayoutTests/fast/forms/select/ because this is
very specific to <select>.

https://codereview.chromium.org/120373005/diff/300004/LayoutTests/fast/events...
LayoutTests/fast/events/multiselect-in-listbox-mouse-release-outside.html:12:
log("PASS");
Please use testPassed() defined in LayoutTests/resources/js-test.js.

https://codereview.chromium.org/120373005/diff/300004/LayoutTests/fast/events...
LayoutTests/fast/events/multiselect-in-listbox-mouse-release-outside.html:20:
testRunner.waitUntilDone();
Please import js-test.js and use jsTestIsAsync=true.

https://codereview.chromium.org/120373005/diff/300004/LayoutTests/fast/events...
LayoutTests/fast/events/multiselect-in-listbox-mouse-release-outside.html:21:
testRunner.dumpAsText();
Please import js-test.js and remove this line.

https://codereview.chromium.org/120373005/diff/300004/LayoutTests/fast/events...
LayoutTests/fast/events/multiselect-in-listbox-mouse-release-outside.html:28:
var input = document.getElementById('listBoxSelect');
Using the name |input| for <select> looks unreasonable.

https://codereview.chromium.org/120373005/diff/300004/LayoutTests/fast/events...
LayoutTests/fast/events/multiselect-in-listbox-mouse-release-outside.html:35:
eventSender.mouseMoveTo(x, y + 20);
Does this work if we move the mouse pointer horizontally, not vertically?

https://codereview.chromium.org/120373005/diff/300004/LayoutTests/fast/events...
LayoutTests/fast/events/multiselect-in-listbox-mouse-release-outside.html:46:
var input = document.getElementById('shortTextField');
This variable is not used.

https://codereview.chromium.org/120373005/diff/300004/LayoutTests/fast/events...
LayoutTests/fast/events/multiselect-in-listbox-mouse-release-outside.html:49:
testRunner.notifyDone();
Please import js-test.js and use finishJSTest().

[gnana, 2014-01-06 09:15:24]

Thanks!
Please have a look.

https://codereview.chromium.org/120373005/diff/300004/LayoutTests/fast/events...
File LayoutTests/fast/events/multiselect-in-listbox-mouse-release-outside.html
(right):

https://codereview.chromium.org/120373005/diff/300004/LayoutTests/fast/events...
LayoutTests/fast/events/multiselect-in-listbox-mouse-release-outside.html:1:
<html>
On 2014/01/06 03:04:58, tkent wrote:
> I prefer putting this file in LayoutTests/fast/forms/select/ because this is
> very specific to <select>.

Done.

https://codereview.chromium.org/120373005/diff/300004/LayoutTests/fast/events...
LayoutTests/fast/events/multiselect-in-listbox-mouse-release-outside.html:12:
log("PASS");
On 2014/01/06 03:04:58, tkent wrote:
> Please use testPassed() defined in LayoutTests/resources/js-test.js.

Done.

https://codereview.chromium.org/120373005/diff/300004/LayoutTests/fast/events...
LayoutTests/fast/events/multiselect-in-listbox-mouse-release-outside.html:20:
testRunner.waitUntilDone();
On 2014/01/06 03:04:58, tkent wrote:
> Please import js-test.js and use jsTestIsAsync=true.

Done.

https://codereview.chromium.org/120373005/diff/300004/LayoutTests/fast/events...
LayoutTests/fast/events/multiselect-in-listbox-mouse-release-outside.html:21:
testRunner.dumpAsText();
On 2014/01/06 03:04:58, tkent wrote:
> Please import js-test.js and remove this line.
> 

Done.

https://codereview.chromium.org/120373005/diff/300004/LayoutTests/fast/events...
LayoutTests/fast/events/multiselect-in-listbox-mouse-release-outside.html:28:
var input = document.getElementById('listBoxSelect');
On 2014/01/06 03:04:58, tkent wrote:
> Using the name |input| for <select> looks unreasonable.

Done.

https://codereview.chromium.org/120373005/diff/300004/LayoutTests/fast/events...
LayoutTests/fast/events/multiselect-in-listbox-mouse-release-outside.html:35:
eventSender.mouseMoveTo(x, y + 20);
On 2014/01/06 03:04:58, tkent wrote:
> Does this work if we move the mouse pointer horizontally, not vertically?

yes, it works

https://codereview.chromium.org/120373005/diff/300004/LayoutTests/fast/events...
LayoutTests/fast/events/multiselect-in-listbox-mouse-release-outside.html:46:
var input = document.getElementById('shortTextField');
On 2014/01/06 03:04:58, tkent wrote:
> This variable is not used.

Done.

https://codereview.chromium.org/120373005/diff/300004/LayoutTests/fast/events...
LayoutTests/fast/events/multiselect-in-listbox-mouse-release-outside.html:49:
testRunner.notifyDone();
On 2014/01/06 03:04:58, tkent wrote:
> Please import js-test.js and use finishJSTest().

Done.

[tkent, 2014-01-06 23:33:21]

https://codereview.chromium.org/120373005/diff/350001/LayoutTests/fast/forms/...
File
LayoutTests/fast/forms/select/multiselect-in-listbox-mouse-release-outside.html
(right):

https://codereview.chromium.org/120373005/diff/350001/LayoutTests/fast/forms/...
LayoutTests/fast/forms/select/multiselect-in-listbox-mouse-release-outside.html:14:
var input = document.getElementById('listBoxInput');
Using a variable name |input| is not reasonable for <select>.

https://codereview.chromium.org/120373005/diff/350001/LayoutTests/fast/forms/...
LayoutTests/fast/forms/select/multiselect-in-listbox-mouse-release-outside.html:24:
setTimeout(autoscrollTestPart1, 0);
Do we need this setTimeout()?

https://codereview.chromium.org/120373005/diff/350001/LayoutTests/fast/forms/...
LayoutTests/fast/forms/select/multiselect-in-listbox-mouse-release-outside.html:37:
setTimeout(autoscrollTestPart2, 100);
How did you choose '100'?

https://codereview.chromium.org/120373005/diff/350001/LayoutTests/fast/forms/...
LayoutTests/fast/forms/select/multiselect-in-listbox-mouse-release-outside.html:43:
setTimeout(autoscrollTestPart3, 100);
Do we need this setTimeout()?

https://codereview.chromium.org/120373005/diff/350001/LayoutTests/fast/forms/...
LayoutTests/fast/forms/select/multiselect-in-listbox-mouse-release-outside.html:56:
setTimeout(autoscrollTestPart4, 100);
How did you choose '100'?

[gnana, 2014-01-07 08:35:18]

Thanks tkent.
Please have a look.

https://codereview.chromium.org/120373005/diff/350001/LayoutTests/fast/forms/...
File
LayoutTests/fast/forms/select/multiselect-in-listbox-mouse-release-outside.html
(right):

https://codereview.chromium.org/120373005/diff/350001/LayoutTests/fast/forms/...
LayoutTests/fast/forms/select/multiselect-in-listbox-mouse-release-outside.html:14:
var input = document.getElementById('listBoxInput');
On 2014/01/06 23:33:21, tkent wrote:
> Using a variable name |input| is not reasonable for <select>.
I misunderstood it previously. Corrected now.
Done.

https://codereview.chromium.org/120373005/diff/350001/LayoutTests/fast/forms/...
LayoutTests/fast/forms/select/multiselect-in-listbox-mouse-release-outside.html:24:
setTimeout(autoscrollTestPart1, 0);
On 2014/01/06 23:33:21, tkent wrote:
> Do we need this setTimeout()?
This timeout was not necessary. removed
Done.

https://codereview.chromium.org/120373005/diff/350001/LayoutTests/fast/forms/...
LayoutTests/fast/forms/select/multiselect-in-listbox-mouse-release-outside.html:37:
setTimeout(autoscrollTestPart2, 100);
On 2014/01/06 23:33:21, tkent wrote:
> How did you choose '100'?
This timeout was not necessary. removed
Done.

https://codereview.chromium.org/120373005/diff/350001/LayoutTests/fast/forms/...
LayoutTests/fast/forms/select/multiselect-in-listbox-mouse-release-outside.html:43:
setTimeout(autoscrollTestPart3, 100);
On 2014/01/06 23:33:21, tkent wrote:
> Do we need this setTimeout()?
Before running horizontal test we need to give a delay for Autoscrollcontroller
timer to finish.
Also choose 100 randomly.

https://codereview.chromium.org/120373005/diff/350001/LayoutTests/fast/forms/...
LayoutTests/fast/forms/select/multiselect-in-listbox-mouse-release-outside.html:56:
setTimeout(autoscrollTestPart4, 100);
On 2014/01/06 23:33:21, tkent wrote:
> How did you choose '100'?
This timeout was not necessary so removed.

https://codereview.chromium.org/120373005/diff/410001/LayoutTests/fast/forms/...
File
LayoutTests/fast/forms/select/multiselect-in-listbox-mouse-release-outside.html
(right):

https://codereview.chromium.org/120373005/diff/410001/LayoutTests/fast/forms/...
LayoutTests/fast/forms/select/multiselect-in-listbox-mouse-release-outside.html:32:
setTimeout(HorizontalTest, 100);
Before running horizontal test we need to give a delay for Autoscrollcontroller
timer to finish. There is no specific reason for choosing 100. It is just to
make sure Autoscrollcontroller timer is finished for vertical test.

[tkent, 2014-01-07 09:41:33]

lgtm

[commit-bot: I haz the power, 2014-01-07 09:56:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gnanasekar.s@samsung.com/120373005/410001

[commit-bot: I haz the power, 2014-01-07 11:43:12]

Change committed as 164563

[Inactive, 2014-04-20 00:43:41]

https://codereview.chromium.org/120373005/diff/410001/Source/core/html/HTMLSe...
File Source/core/html/HTMLSelectElement.cpp (right):

https://codereview.chromium.org/120373005/diff/410001/Source/core/html/HTMLSe...
Source/core/html/HTMLSelectElement.cpp:1280: bool dragSelection = false;
I don't understand how the changes to this file could have any effect..

https://codereview.chromium.org/120373005/diff/410001/Source/core/html/HTMLSe...
Source/core/html/HTMLSelectElement.cpp:1326: dragSelection = true;
The clang static analyzer (rightfully) complains about this assignment being
useless since dragSelection is never used later.

https://codereview.chromium.org/120373005/diff/410001/Source/core/html/HTMLSe...
Source/core/html/HTMLSelectElement.cpp:1335: if (!dragSelection) {
dragSelection will always be false here since it is set to true in an "else if"
scope.

[Inactive, 2014-04-20 01:22:36]

I uploaded a partial revert in https://codereview.chromium.org/242113014/