Tapping/longpressing should result in INSERTION_HANDLE_SHOWN

Tapping/longpressing should result in INSERTION_HANDLE_SHOWN

If a touch insertion handle was visible inside of a editable text box tapping/long-
pressing somewhere else in the box caused a INSERTION_HANDLE_MOVED. This CL changes
it to cause a INSERTION_HANDLE_SHOWN. This is already how touch selection handles
work. If there is a selection and you long-press a different select this triggers
SELECTION_HANDLES_SHOWN and not SELECTION_HANDLES_MOVED.

BUG=698376
Review-Url: https://codereview.chromium.org/2733603003
Cr-Commit-Position: refs/heads/master@{#455214}
Committed: https://chromium.googlesource.com/chromium/src/+/f5bcdf6b4e9f48cdbac1c3621653319e573b9b58

[amaralp, 2017-03-03 21:35:38]

The CQ bit was checked by amaralp@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-03 21:35:56]

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-03-03 22:19:50]

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

[commit-bot: I haz the power, 2017-03-03 22:19:51]

Dry run: This issue passed the CQ dry run.

[amaralp, 2017-03-03 22:24:30]

Description was changed from

==========
Tapping/longpressing should result in INSERTION_HANDLE_SHOWN


BUG=698376
==========

to

==========
Tapping/longpressing should result in INSERTION_HANDLE_SHOWN

If a touch insertion handle was visible inside of a editable text box
tapping/long-pressing somewhere else in the box caused a INSERTION_HANDLE_MOVED.
This CL changes it to cause a INSERTION_HANDLE_SHOWN. This is already how touch
selection handles work. If there is a selection and you long-press a different
select this triggers SELECTION_HANDLES_SHOWN and not SELECTION_HANDLES_MOVED.

BUG=698376
==========

[amaralp, 2017-03-03 22:24:30]

amaralp@chromium.org changed reviewers:
+ mohsen@chromium.org

[amaralp, 2017-03-03 22:24:44]

Description was changed from

==========
Tapping/longpressing should result in INSERTION_HANDLE_SHOWN

If a touch insertion handle was visible inside of a editable text box
tapping/long-pressing somewhere else in the box caused a INSERTION_HANDLE_MOVED.
This CL changes it to cause a INSERTION_HANDLE_SHOWN. This is already how touch
selection handles work. If there is a selection and you long-press a different
select this triggers SELECTION_HANDLES_SHOWN and not SELECTION_HANDLES_MOVED.

BUG=698376
==========

to

==========
Tapping/longpressing should result in INSERTION_HANDLE_SHOWN

If a touch insertion handle was visible inside of a editable text box
tapping/long-
pressing somewhere else in the box caused a INSERTION_HANDLE_MOVED. This CL
changes
it to cause a INSERTION_HANDLE_SHOWN. This is already how touch selection
handles
work. If there is a selection and you long-press a different select this
triggers
SELECTION_HANDLES_SHOWN and not SELECTION_HANDLES_MOVED.

BUG=698376
==========

[amaralp, 2017-03-03 22:25:45]

PTAL

[mohsen, 2017-03-06 21:57:55]

What are the consequences of this change on touch selection behavior? In other
words, what are you trying to accomplish with this change? Is there a bug you
are trying to fix?

[amaralp, 2017-03-06 22:09:50]

On 2017/03/06 at 21:57:55, mohsen wrote:
> What are the consequences of this change on touch selection behavior? In other
words, what are you trying to accomplish with this change? Is there a bug you
are trying to fix?

I am trying to fix a concurrency bug in crbug.com/697934.

[mohsen, 2017-03-07 03:30:10]

https://codereview.chromium.org/2733603003/diff/1/ui/touch_selection/touch_se...
File ui/touch_selection/touch_selection_controller_unittest.cc (left):

https://codereview.chromium.org/2733603003/diff/1/ui/touch_selection/touch_se...
ui/touch_selection/touch_selection_controller_unittest.cc:485: OnTapEvent();
Can you explain why this is removed. We are simulating a tap, so it seems that
OnTapEvent() should be called. BTW, is this test
(InsertionNotResetByRepeatedTapOrPress) still relevant after your change?

[amaralp, 2017-03-07 04:17:35]

The CQ bit was checked by amaralp@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-07 04:18:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[amaralp, 2017-03-07 04:20:38]

https://codereview.chromium.org/2733603003/diff/1/ui/touch_selection/touch_se...
File ui/touch_selection/touch_selection_controller_unittest.cc (left):

https://codereview.chromium.org/2733603003/diff/1/ui/touch_selection/touch_se...
ui/touch_selection/touch_selection_controller_unittest.cc:485: OnTapEvent();
On 2017/03/07 at 03:30:10, mohsen wrote:
> Can you explain why this is removed. We are simulating a tap, so it seems that
OnTapEvent() should be called. BTW, is this test
(InsertionNotResetByRepeatedTapOrPress) still relevant after your change?

Tapping on the handle will cause
|TouchSelectionController::WillHandleTouchEvent()| to return |true| which means
that the tap will not get propagated. So |OnTapEvent()| shouldn't be called in
this scenario.
Since |OnTapEvent()| and |OnLongpressEvent()| shouldn't be called when the event
hits a touch handle |InsertionNotResetByRepeatedTapOrPress| is covered by
|InsertionTapped|.

[mohsen, 2017-03-07 05:43:08]

Thanks. LGTM

[commit-bot: I haz the power, 2017-03-07 06:41:50]

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

[commit-bot: I haz the power, 2017-03-07 06:41:50]

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

[amaralp, 2017-03-07 18:21:33]

The CQ bit was checked by amaralp@chromium.org

[commit-bot: I haz the power, 2017-03-07 18:23:14]

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-03-07 20:57:59]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1488910893315840,
"parent_rev": "88c753c103d76fd3697f7fcef962011588e3975f", "commit_rev":
"f5bcdf6b4e9f48cdbac1c3621653319e573b9b58"}

[commit-bot: I haz the power, 2017-03-07 20:58:45]

Description was changed from

==========
Tapping/longpressing should result in INSERTION_HANDLE_SHOWN

If a touch insertion handle was visible inside of a editable text box
tapping/long-
pressing somewhere else in the box caused a INSERTION_HANDLE_MOVED. This CL
changes
it to cause a INSERTION_HANDLE_SHOWN. This is already how touch selection
handles
work. If there is a selection and you long-press a different select this
triggers
SELECTION_HANDLES_SHOWN and not SELECTION_HANDLES_MOVED.

BUG=698376
==========

to

==========
Tapping/longpressing should result in INSERTION_HANDLE_SHOWN

If a touch insertion handle was visible inside of a editable text box
tapping/long-
pressing somewhere else in the box caused a INSERTION_HANDLE_MOVED. This CL
changes
it to cause a INSERTION_HANDLE_SHOWN. This is already how touch selection
handles
work. If there is a selection and you long-press a different select this
triggers
SELECTION_HANDLES_SHOWN and not SELECTION_HANDLES_MOVED.

BUG=698376

Review-Url: https://codereview.chromium.org/2733603003
Cr-Commit-Position: refs/heads/master@{#455214}
Committed:
https://chromium.googlesource.com/chromium/src/+/f5bcdf6b4e9f48cdbac1c3621653...
==========

[commit-bot: I haz the power, 2017-03-07 20:58:46]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/f5bcdf6b4e9f48cdbac1c3621653...