Selection should not set the cursor type to text over the explicitly set cursor type.

Selection should not set the cursor type to text over the explicitly set cursor type.

When tried to select some text in an area that has style
cursor:default set, cursor type changes to text cursor ignoring
the cursor style that is explicitly set.

When the cursor style is explicitly set as default(or something else),
we should not change it to text cursor no matter what we are over
or what operation we are performing (be it hovering over the text
or selecting the text).

We change the cursor type to text during selection. But if there is
an explicit cursor style set then this explicitly set cursor style
should be given priority over the text style cursor during selection.

This makes the blink behavior compatible with that of FF.
This also makes the behavior uniform within blink i.e. currently
if the area on which cursor:default is set is contenteditable, in this
particular case we do not change the cursor style to text on
selection. This should be the behavior irrespective of editability.

BUG=134999
R=eseidel, leviw, ojan, yosin

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

[vanihegde, 2013-09-10 11:35:12]

I tried in vain to automate a test for this. Appreciate any help on this.

Note on IE bahavior:
IE bahaviour is not uniform either, if the area on which cursor:default is set
is contenteditable then it ignores the default type that is set and changes to
text cursor (both on hovering over the text and on selecting the text).
On the other hand, it maintains the explicitly set cursor type if the area is
not contenteditable. Hence FF behaviour seems more appropriate.

[yosin_UTC9, 2013-09-13 03:42:44]

You can use window.internals.getCurrentCursorInfo(document), which returns
current cursor information, e.g. type=IBeam hotSpot=0,0, for layout test.

Note: You need to call eventSender.mouseMoveTo() before calling
internals.getCurrentCursorInfo().

https://codereview.chromium.org/23461040/diff/1/Source/core/page/EventHandler...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/23461040/diff/1/Source/core/page/EventHandler...
Source/core/page/EventHandler.cpp:1182: && m_frame->selection().isCaretOrRange()
&& !m_capturingMouseEventsNode)
nit: Please align "&&" to m_mousePressed
nit: Please use curly-brace when condition expression laid out more than one
line even if then-clause is only one line.

if (m_mousePressed && m_mouseDownMayStartSelect
    && !m_mouseDownMayStartDrag
    && m_frame->...) {
    return iBeam;
}

[vanihegde, 2013-09-13 03:57:49]

On 2013/09/13 03:42:44, Yoshifumi Inoue wrote:
> You can use window.internals.getCurrentCursorInfo(document), which returns
> current cursor information, e.g. type=IBeam hotSpot=0,0, for layout test.
> 
> Note: You need to call eventSender.mouseMoveTo() before calling
> internals.getCurrentCursorInfo().
> 
>
https://codereview.chromium.org/23461040/diff/1/Source/core/page/EventHandler...
> File Source/core/page/EventHandler.cpp (right):
> 
>
https://codereview.chromium.org/23461040/diff/1/Source/core/page/EventHandler...
> Source/core/page/EventHandler.cpp:1182: &&
m_frame->selection().isCaretOrRange()
> && !m_capturingMouseEventsNode)
> nit: Please align "&&" to m_mousePressed
> nit: Please use curly-brace when condition expression laid out more than one
> line even if then-clause is only one line.
> 
> if (m_mousePressed && m_mouseDownMayStartSelect
>     && !m_mouseDownMayStartDrag
>     && m_frame->...) {
>     return iBeam;
> }

Thanks for the review, I shall work on the comments and upload a patch.

[yosin_UTC9, 2013-09-17 09:46:38]

C++ changes is ACK.
See comments for a test script.

https://chromiumcodereview.appspot.com/23461040/diff/9001/LayoutTests/editing...
File LayoutTests/editing/caret/caret-type-for-user-select-none.html (right):

https://chromiumcodereview.appspot.com/23461040/diff/9001/LayoutTests/editing...
LayoutTests/editing/caret/caret-type-for-user-select-none.html:4: <p><b>Tests
whether explicitly set caret style is retained on performing text
selection</b></p>
nit: We may want to use description('Tests where...') and having <p
id="description></p> HTML fragment.

https://chromiumcodereview.appspot.com/23461040/diff/9001/LayoutTests/editing...
LayoutTests/editing/caret/caret-type-for-user-select-none.html:31: var
currentCursorType = cursorInfo.substring(cursorInfo.indexOf('=')+1,
cursorInfo.lastIndexOf(' '));
nit: put spaces around "+".

https://chromiumcodereview.appspot.com/23461040/diff/9001/LayoutTests/editing...
LayoutTests/editing/caret/caret-type-for-user-select-none.html:32:
shouldBe('currentCursorType', 'cursorType');
shouldBeEqualToString('currentCurosrType', cursorType);

[vanihegde, 2013-09-17 09:56:46]

On 2013/09/17 09:46:38, Yoshifumi Inoue wrote:
> C++ changes is ACK.
> See comments for a test script.
> 
>
https://chromiumcodereview.appspot.com/23461040/diff/9001/LayoutTests/editing...
> File LayoutTests/editing/caret/caret-type-for-user-select-none.html (right):
> 
>
https://chromiumcodereview.appspot.com/23461040/diff/9001/LayoutTests/editing...
> LayoutTests/editing/caret/caret-type-for-user-select-none.html:4: <p><b>Tests
> whether explicitly set caret style is retained on performing text
> selection</b></p>
> nit: We may want to use description('Tests where...') and having <p
> id="description></p> HTML fragment.
Do you want me to change the description here or just add the id="description"?
> 
>
https://chromiumcodereview.appspot.com/23461040/diff/9001/LayoutTests/editing...
> LayoutTests/editing/caret/caret-type-for-user-select-none.html:31: var
> currentCursorType = cursorInfo.substring(cursorInfo.indexOf('=')+1,
> cursorInfo.lastIndexOf(' '));
> nit: put spaces around "+".

Will do.
> 
>
https://chromiumcodereview.appspot.com/23461040/diff/9001/LayoutTests/editing...
> LayoutTests/editing/caret/caret-type-for-user-select-none.html:32:
> shouldBe('currentCursorType', 'cursorType');
> shouldBeEqualToString('currentCurosrType', cursorType);

Will do

[ojan, 2013-09-17 23:45:59]

lgtm once you address yosin's comments.

[vanihegde, 2013-09-18 03:26:12]

I think I have addressed all the review comments in my last patch. 
I did not quite get the comment regarding description though.
So, yosin please confirm if it's addressed. Thanks.

[yosin_UTC9, 2013-09-18 03:42:26]

https://codereview.chromium.org/23461040/diff/16001/LayoutTests/editing/caret...
File LayoutTests/editing/caret/caret-type-for-user-select-none.html (right):

https://codereview.chromium.org/23461040/diff/16001/LayoutTests/editing/caret...
LayoutTests/editing/caret/caret-type-for-user-select-none.html:4: <p
id="description">Tests whether explicitly set caret style is retained on
performing text selection</p>

See below for example of how to use description().
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

https://codereview.chromium.org/23461040/diff/16001/LayoutTests/editing/caret...
LayoutTests/editing/caret/caret-type-for-user-select-none.html:23:
eventSender.mouseMoveTo(div.offsetLeft + 5, y);
We can also use leapForwardAndMove() here.

https://codereview.chromium.org/23461040/diff/16001/LayoutTests/editing/caret...
LayoutTests/editing/caret/caret-type-for-user-select-none.html:35: </html>
You should load js-test-post.js, and start test steps by onload, just in case.

window.onload = function() {
  if (!window.eventSender)
      return;
  ...
};

[vanihegde, 2013-09-18 05:29:11]

On 2013/09/18 03:42:26, Yoshifumi Inoue wrote:
>
https://codereview.chromium.org/23461040/diff/16001/LayoutTests/editing/caret...
> File LayoutTests/editing/caret/caret-type-for-user-select-none.html (right):
> 
>
https://codereview.chromium.org/23461040/diff/16001/LayoutTests/editing/caret...
> LayoutTests/editing/caret/caret-type-for-user-select-none.html:4: <p
> id="description">Tests whether explicitly set caret style is retained on
> performing text selection</p>
> 
> See below for example of how to use description().
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
    DONE

>
https://codereview.chromium.org/23461040/diff/16001/LayoutTests/editing/caret...
> LayoutTests/editing/caret/caret-type-for-user-select-none.html:23:
> eventSender.mouseMoveTo(div.offsetLeft + 5, y);
> We can also use leapForwardAndMove() here.
   DONE

>
https://codereview.chromium.org/23461040/diff/16001/LayoutTests/editing/caret...
> LayoutTests/editing/caret/caret-type-for-user-select-none.html:35: </html>
> You should load js-test-post.js, and start test steps by onload, just in case.
> 
> window.onload = function() {
>   if (!window.eventSender)
>       return;
>   ...
> };
 Used js-test-post.js
 Moved the script within body instead of using onload, just to make the code
look little simpler and to get well ordered expectation file (i.e. TEST COMPLETE
log at the end). Should serve the purpose I believe.

[yosin_UTC9, 2013-09-18 05:34:45]

ACK

[commit-bot: I haz the power, 2013-09-18 05:34:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vani.hegde@samsung.com/23461040/22001

[commit-bot: I haz the power, 2013-09-18 08:54:34]

Change committed as 157946