Display arrow cursor instead of i-beam when selection is not present

Display arrow cursor instead of i-beam when selection is not present

When selection is not there , range is empty then on drag of mouse,
arrow cursor should be shown.
Currently I-beam cursor is shown visble selection in current code
has selection type as carat.
So we should check if selection type is range.

BUG=735852

[tanvir, 2017-07-04 07:04:31]

The CQ bit was checked by tanvir.rizvi@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2017-07-04 07:04:35]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

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

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

[commit-bot: I haz the power, 2017-07-04 07:04:37]

Dry run: No L-G-T-M from a valid reviewer yet.
CQ run can only be started once the patch has received an L-G-T-M from a full
committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a
full super star committer.
Committers are members of the group "project-chromium-committers".
Note that this has nothing to do with OWNERS files.

[tanvir, 2017-07-04 07:05:51]

Description was changed from

==========
Display arrow cursor instead of i-beam when selection is not present

When selection is not there , range is empty then on drag of mouse,
arrow cursor should be shown.
Currently I-beam cursor is shown visble selection in current code
has selection type as carat.
So we should check if selection type is range.

BUG=735852
==========

to

==========
Display arrow cursor instead of i-beam when selection is not present

When selection is not there , range is empty then on drag of mouse,
arrow cursor should be shown.
Currently I-beam cursor is shown visble selection in current code
has selection type as carat.
So we should check if selection type is range.

BUG=735852
==========

[tanvir, 2017-07-04 07:05:51]

tanvir.rizvi@samsung.com changed reviewers:
+ shanmuga.m@samsung.com, srirama.m@samsung.com

[Shanmuga Pandi, 2017-07-04 07:09:25]

The CQ bit was checked by shanmuga.m@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2017-07-04 07:09:35]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-04 07:59:38]

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

[commit-bot: I haz the power, 2017-07-04 07:59:39]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[tanvir, 2017-07-04 12:58:00]

The CQ bit was checked by tanvir.rizvi@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2017-07-04 12:58:09]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-04 12:58:10]

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

[commit-bot: I haz the power, 2017-07-04 12:58:11]

Dry run: No L-G-T-M from a valid reviewer yet.
CQ run can only be started once the patch has received an L-G-T-M from a full
committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a
full super star committer.
Committers are members of the group "project-chromium-committers".
Note that this has nothing to do with OWNERS files.

[Srirama, 2017-07-04 13:00:20]

The CQ bit was checked by srirama.m@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2017-07-04 13:00:29]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Srirama, 2017-07-04 13:05:00]

https://codereview.chromium.org/2967893002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/events/mouse-cursor-change.html
(right):

https://codereview.chromium.org/2967893002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/events/mouse-cursor-change.html:68:
eventSender.mouseMoveTo(target.offsetLeft+target.offsetWidth+50,
target.offsetTop+target.offsetHeight+100);
nit: add space on either side of operator here and everywhere

[Srirama, 2017-07-04 13:53:09]

The CQ bit was checked by srirama.m@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2017-07-04 13:53:18]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tanvir, 2017-07-04 15:37:11]

tanvir.rizvi@samsung.com changed reviewers:
+ yosin@chromium.org

[tanvir, 2017-07-04 15:38:40]

PTAL!!!
Thanks

[commit-bot: I haz the power, 2017-07-04 15:48:59]

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

[commit-bot: I haz the power, 2017-07-04 15:49:01]

Dry run: This issue passed the CQ dry run.

[yosin_UTC9, 2017-07-05 01:27:45]

https://codereview.chromium.org/2967893002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/EventHandler.cpp (left):

https://codereview.chromium.org/2967893002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:555: !frame_->Selection()
The issue is caused by hidden selection regarding to bisect[1].
Thus, suggested change is checking |!frame->Selection().IsHidden()|.
We should use i-beam for both caret/range selection.


[1] https://bugs.chromium.org/p/chromium/issues/detail?id=735852#c1

[tanvir, 2017-07-06 10:09:06]

On 2017/07/05 01:27:45, yosin_UTC9 wrote:
>
https://codereview.chromium.org/2967893002/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/input/EventHandler.cpp (left):
> 
>
https://codereview.chromium.org/2967893002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/input/EventHandler.cpp:555:
!frame_->Selection()
> The issue is caused by hidden selection regarding to bisect[1].
> Thus, suggested change is checking |!frame->Selection().IsHidden()|.
> We should use i-beam for both caret/range selection.
> 
> 
> [1] https://bugs.chromium.org/p/chromium/issues/detail?id=735852#c1

I tried the suggested change, |frame->Selection().IsHidden()| is coming as
|false| for 1 and 2 testing scenarios.
1) chrome://md-settings/manageProfile
2) https://jsfiddle.net/nmc03ax4/2/.
Analysing more to go to the root cause.

[hugoh_UTC2, 2017-07-06 11:59:33]

hugoh@opera.com changed reviewers:
+ hugoh@opera.com

[hugoh_UTC2, 2017-07-06 11:59:33]

https://codereview.chromium.org/2967893002/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/events/mouse-cursor-change.html
(right):

https://codereview.chromium.org/2967893002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/events/mouse-cursor-change.html:66: // If
selection is not happening and mouse is pressed, then pointer cursor should be
shown.
"When a drag neither modifies nor creates a selection, pointer cursor should be
shown."

https://codereview.chromium.org/2967893002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/events/mouse-cursor-change.html:69: },
function() {
I think this test should go into a new file (because this is a new bug) that
only tests BUG=735852.

Testing 735852 does not need CURSOR_UPDATE_DELAY. No CSS needs to be involved to
test this, so no need to wait for style update.

  "cursor is updated immediately on mouse events"

[hugoh_UTC2, 2017-07-11 09:36:39]

https://codereview.chromium.org/2967893002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/EventHandler.cpp (left):

https://codereview.chromium.org/2967893002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:548: // During selection,
use an I-beam no matter what we're over.
http://crbug.com/735852 shows that this comment is wrong. We do not want to "use
an I-beam no matter what we're over".

Is this block only needed when selecting links ?

I did an experiment here: https://chromium-review.googlesource.com/c/566798/

[yosin_UTC9, 2017-07-20 01:02:38]

On 2017/07/11 at 09:36:39, hugoh wrote:
>
https://codereview.chromium.org/2967893002/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/input/EventHandler.cpp (left):
> 
>
https://codereview.chromium.org/2967893002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/input/EventHandler.cpp:548: // During
selection, use an I-beam no matter what we're over.
> http://crbug.com/735852 shows that this comment is wrong. We do not want to
"use an I-beam no matter what we're over".
> 
> Is this block only needed when selecting links ?
> 
> I did an experiment here: https://chromium-review.googlesource.com/c/566798/

Could you close this review since another patch[1] fixed this issue?
Thanks!

[1] http://crrev.com/c/566798

[tanvir, 2017-07-20 05:28:54]

On 2017/07/20 01:02:38, yosin_UTC9 wrote:
> On 2017/07/11 at 09:36:39, hugoh wrote:
> >
>
https://codereview.chromium.org/2967893002/diff/40001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/input/EventHandler.cpp (left):
> > 
> >
>
https://codereview.chromium.org/2967893002/diff/40001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/input/EventHandler.cpp:548: // During
> selection, use an I-beam no matter what we're over.
> > http://crbug.com/735852 shows that this comment is wrong. We do not want to
> "use an I-beam no matter what we're over".
> > 
> > Is this block only needed when selecting links ?
> > 
> > I did an experiment here: https://chromium-review.googlesource.com/c/566798/
> 
> Could you close this review since another patch[1] fixed this issue?
> Thanks!
> 
> [1] http://crrev.com/c/566798

Done!!!
Thanks.