Fix behavior of label associated with control element

Fix behavior of label associated with control element

Continous clicking on label was not able to transfer the click
event to associated control element. This was because we were
not passing the events if there is selection on label.
    
Now, the behaviour of label element is as follows:
 - If there is double click, two clicks will be passed to control
   element. Control element will *not* be focused.
 - If there is selection of label element by dragging, no click
   event is passed. Also, no focus on control element.
 - If there is already a selection on label element and then label
   is clicked, then click event is passed to control element and
   control element is focused.
    
The above mentioned behaviour is same as Firefox.

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

[deepak.sa, 2014-09-09 11:33:14]

deepak.sa@samsung.com changed reviewers:
+ esprehn@chromium.org, keishi@chromium.org, tkent@chromium.org

[deepak.sa, 2014-09-09 11:33:15]

PTAL?
Thanks!

[esprehn, 2014-09-09 20:32:45]

https://codereview.chromium.org/556813002/diff/20001/Source/core/html/HTMLLab...
File Source/core/html/HTMLLabelElement.cpp (right):

https://codereview.chromium.org/556813002/diff/20001/Source/core/html/HTMLLab...
Source/core/html/HTMLLabelElement.cpp:133: isDragged = false;
Static vars like this seem pretty sketchy.

https://codereview.chromium.org/556813002/diff/20001/Source/core/html/HTMLLab...
Source/core/html/HTMLLabelElement.cpp:184: isDragged = true;
This doesn't seem like the right way to detect dragging.

[tkent, 2014-09-10 00:24:31]

> This behavior was due to the
> selection of label on continous clicks. This patch first saves the
> selection,

Why doesn't the existing |frame->selection().isRange()| work in this case?  Why
do we need to save&restore?

https://codereview.chromium.org/556813002/diff/20001/LayoutTests/fast/forms/l...
File LayoutTests/fast/forms/label/continous-click-on-label.html (right):

https://codereview.chromium.org/556813002/diff/20001/LayoutTests/fast/forms/l...
LayoutTests/fast/forms/label/continous-click-on-label.html:40: element.click();
Does this test fail without your C++ code change?

[deepak.sa, 2014-09-10 15:16:32]

This is my understanding of events on label element (with associated control
element) :

1) If the label is double clicked:
   a) Pass two click events to control element.
   b) Select the label text.
   c) Do not focus the control element. 

2) If the label is selected by drag:
   a) *Do not* pass click event to control element.
   b) Select the dragged text.
   c) Do not focus the control element.

Every click event should be passed to control element except during drag.
This enables the alternative checking-unchecking of checkbox without worrying
about whether there is selected text or not.
Only thing we have to check is whether to set the focus on control element or
not.

[deepak.sa, 2014-09-10 15:29:10]

On 2014/09/10 00:24:31, tkent (overloaded) wrote:
> > This behavior was due to the
> > selection of label on continous clicks. This patch first saves the
> > selection,
> 
> Why doesn't the existing |frame->selection().isRange()| work in this case? 
Why
> do we need to save&restore?

This doesn't work because we *do not* send the click event if there is any
selection. In this case, there is empty selection, so this check was failing. So
there is no alternate checking-unchecking of checkbox.

AFAIK, once we set the focus on control element(in case of input type="text"),
previous selection is gone. So, may be we only have to place to check on
focusing of control element. I'll upload the next patch considering this
scenario.


>
https://codereview.chromium.org/556813002/diff/20001/LayoutTests/fast/forms/l...
> LayoutTests/fast/forms/label/continous-click-on-label.html:40:
element.click();
> Does this test fail without your C++ code change?

No, this test is not failing even without this change. But, it fails if
"click()" is replaced with "mousedown and mouseup" events. I'll also change the
test case in the next patch.

@tkent Can you please suggest any other way in which we can detect dragging over
the label text, because Elliot suggested the current way is not good? I've
checked in other classes also, but i couldn't find anything other than handling
of different type of mouse events.

[deepak.sa, 2014-09-10 15:32:46]

On 2014/09/10 15:16:32, deepak.sa wrote:
> This is my understanding of events on label element (with associated control
> element) :
> 
> 1) If the label is double clicked:
>    a) Pass two click events to control element.
>    b) Select the label text.
>    c) Do not focus the control element. 
> 
> 2) If the label is selected by drag:
>    a) *Do not* pass click event to control element.
>    b) Select the dragged text.
>    c) Do not focus the control element.
> 
> Every click event should be passed to control element except during drag.
> This enables the alternative checking-unchecking of checkbox without worrying
> about whether there is selected text or not.
> Only thing we have to check is whether to set the focus on control element or
> not.

Also, this above scenario is consistent with Firefox.

[deepak.sa, 2014-09-15 16:17:17]

Uploaded a new patchset.
PTAL?
Thanks!

[tkent, 2014-09-16 01:53:38]

https://codereview.chromium.org/556813002/diff/40001/LayoutTests/fast/forms/l...
File LayoutTests/fast/forms/label/continous-click-on-label.html (right):

https://codereview.chromium.org/556813002/diff/40001/LayoutTests/fast/forms/l...
LayoutTests/fast/forms/label/continous-click-on-label.html:25: // intially
unchecked
The comment should be replaced with code:
    shouldBeFalse('checkbox.checked', true);

https://codereview.chromium.org/556813002/diff/40001/LayoutTests/fast/forms/l...
LayoutTests/fast/forms/label/continous-click-on-label.html:26: // after 3 clicks
it should be checked
This comment should be shown in the test result:
  debug('The checkbox should be checked after three clicks.');

https://codereview.chromium.org/556813002/diff/40001/LayoutTests/fast/forms/l...
LayoutTests/fast/forms/label/continous-click-on-label.html:30: // As checkbox is
checked, after 10 clicks it will be checked.
Ditto.

https://codereview.chromium.org/556813002/diff/40001/Source/core/html/HTMLLab...
File Source/core/html/HTMLLabelElement.cpp (left):

https://codereview.chromium.org/556813002/diff/40001/Source/core/html/HTMLLab...
Source/core/html/HTMLLabelElement.cpp:128: // If the click is not simulated and
the text of label element is
Why do you move the code?

https://codereview.chromium.org/556813002/diff/40001/Source/core/html/HTMLLab...
File Source/core/html/HTMLLabelElement.cpp (right):

https://codereview.chromium.org/556813002/diff/40001/Source/core/html/HTMLLab...
Source/core/html/HTMLLabelElement.cpp:150: // If selection is there and is
single click i.e. text is selected
nit: I recommend to wrap code comments in 80 columns.

https://codereview.chromium.org/556813002/diff/40001/Source/core/html/HTMLLab...
Source/core/html/HTMLLabelElement.cpp:152: if (isLabelTextSelected &&
frame->eventHandler().clickCount() == 1)
Need a comment why clickCount>=2 should not skip the process.

https://codereview.chromium.org/556813002/diff/40001/Source/core/html/HTMLLab...
Source/core/html/HTMLLabelElement.cpp:161: // If there is selection, do not
focus the control element.
This comment is helpless. It just says what the code does.
Please write why we don't focus on the control in that case.

https://codereview.chromium.org/556813002/diff/40001/Source/core/page/EventHa...
File Source/core/page/EventHandler.h (right):

https://codereview.chromium.org/556813002/diff/40001/Source/core/page/EventHa...
Source/core/page/EventHandler.h:202: bool isMouseDownWasSingleClickInSelection()
{ return m_mouseDownWasSingleClickInSelection; }
The function names is wrong.  It should reflect the data member name, and should
be mouseDownWasSingleClickInSelection().

[deepak.sa, 2014-09-16 10:31:16]

https://codereview.chromium.org/556813002/diff/40001/LayoutTests/fast/forms/l...
File LayoutTests/fast/forms/label/continous-click-on-label.html (right):

https://codereview.chromium.org/556813002/diff/40001/LayoutTests/fast/forms/l...
LayoutTests/fast/forms/label/continous-click-on-label.html:25: // intially
unchecked
On 2014/09/16 01:53:37, tkent (overloaded) wrote:
> The comment should be replaced with code:
>     shouldBeFalse('checkbox.checked', true);

Done.

https://codereview.chromium.org/556813002/diff/40001/LayoutTests/fast/forms/l...
LayoutTests/fast/forms/label/continous-click-on-label.html:26: // after 3 clicks
it should be checked
On 2014/09/16 01:53:37, tkent (overloaded) wrote:
> This comment should be shown in the test result:
>   debug('The checkbox should be checked after three clicks.');

Done.

https://codereview.chromium.org/556813002/diff/40001/LayoutTests/fast/forms/l...
LayoutTests/fast/forms/label/continous-click-on-label.html:30: // As checkbox is
checked, after 10 clicks it will be checked.
On 2014/09/16 01:53:37, tkent (overloaded) wrote:
> Ditto.

Done.

https://codereview.chromium.org/556813002/diff/40001/Source/core/html/HTMLLab...
File Source/core/html/HTMLLabelElement.cpp (left):

https://codereview.chromium.org/556813002/diff/40001/Source/core/html/HTMLLab...
Source/core/html/HTMLLabelElement.cpp:128: // If the click is not simulated and
the text of label element is
On 2014/09/16 01:53:37, tkent (overloaded) wrote:
> Why do you move the code?

All the checks should happen once it is clear that click event is generated on
label element.

https://codereview.chromium.org/556813002/diff/40001/Source/core/html/HTMLLab...
File Source/core/html/HTMLLabelElement.cpp (right):

https://codereview.chromium.org/556813002/diff/40001/Source/core/html/HTMLLab...
Source/core/html/HTMLLabelElement.cpp:150: // If selection is there and is
single click i.e. text is selected
On 2014/09/16 01:53:38, tkent (overloaded) wrote:
> nit: I recommend to wrap code comments in 80 columns.

Added additional comments.

https://codereview.chromium.org/556813002/diff/40001/Source/core/html/HTMLLab...
Source/core/html/HTMLLabelElement.cpp:152: if (isLabelTextSelected &&
frame->eventHandler().clickCount() == 1)
On 2014/09/16 01:53:38, tkent (overloaded) wrote:
> Need a comment why clickCount>=2 should not skip the process.

Added more comments.

https://codereview.chromium.org/556813002/diff/40001/Source/core/html/HTMLLab...
Source/core/html/HTMLLabelElement.cpp:161: // If there is selection, do not
focus the control element.
On 2014/09/16 01:53:38, tkent (overloaded) wrote:
> This comment is helpless. It just says what the code does.
> Please write why we don't focus on the control in that case.

Replaced this comment.

https://codereview.chromium.org/556813002/diff/40001/Source/core/page/EventHa...
File Source/core/page/EventHandler.h (right):

https://codereview.chromium.org/556813002/diff/40001/Source/core/page/EventHa...
Source/core/page/EventHandler.h:202: bool isMouseDownWasSingleClickInSelection()
{ return m_mouseDownWasSingleClickInSelection; }
On 2014/09/16 01:53:38, tkent (overloaded) wrote:
> The function names is wrong.  It should reflect the data member name, and
should
> be mouseDownWasSingleClickInSelection().
> 

Done.

[tkent, 2014-09-17 00:19:14]

The CQ bit was checked by tkent@chromium.org

[tkent, 2014-09-17 00:19:15]

lgtm

https://codereview.chromium.org/556813002/diff/60001/Source/core/html/HTMLLab...
File Source/core/html/HTMLLabelElement.cpp (right):

https://codereview.chromium.org/556813002/diff/60001/Source/core/html/HTMLLab...
Source/core/html/HTMLLabelElement.cpp:139: //   Behaviour of label element is as
follows:
Very good comment.  It is helpful to understand the code.

[commit-bot: I haz the power, 2014-09-17 00:19:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/556813002/60001

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

Committed patchset #4 (id:60001) as 182131