Allow scrolling a page with selection after long-press on whitespace

Allow scrolling a page with selection after long-press on whitespace

Currently, if there is a selection on a page and you try long-press and
drag on an empty area, it will start longpress-drag gesture for
selecting text, starting at the current selection which might be far
from where the finger is. Better behavior would be to just scroll the
page.

BUG=557034
Committed: https://crrev.com/91e3c85091d54ff42c06fad73c49a414dad3a29e
Cr-Commit-Position: refs/heads/master@{#367646}

[mohsen, 2015-12-21 17:16:17]

Patchset #3 (id:40001) has been deleted

[mohsen, 2015-12-21 18:26:04]

Description was changed from

==========
Allow scrolling a page with selection after long-press on whitespace

Currently, if there is a selection on a page and you try long-press and
drag on an empty area, it will start longpress-drag gesture for
selecting text, starting at the current selection which might be far
from where the finger is. Better behavior would be to just scroll the
page.
.
BUG=557034
==========

to

==========
Allow scrolling a page with selection after long-press on whitespace

Currently, if there is a selection on a page and you try long-press and
drag on an empty area, it will start longpress-drag gesture for
selecting text, starting at the current selection which might be far
from where the finger is. Better behavior would be to just scroll the
page.

BUG=557034
==========

[mohsen, 2015-12-21 18:28:05]

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

[mohsen, 2015-12-21 18:28:05]

Please take a look...

[mfomitchev, 2015-12-22 21:55:53]

mfomitchev@chromium.org changed reviewers:
+ mfomitchev@chromium.org

[mfomitchev, 2015-12-22 21:55:54]

https://codereview.chromium.org/1537493006/diff/60001/ui/touch_selection/long...
File ui/touch_selection/longpress_drag_selector.cc (right):

https://codereview.chromium.org/1537493006/diff/60001/ui/touch_selection/long...
ui/touch_selection/longpress_drag_selector.cc:130: SetState(INACTIVE);
As discussed, lets create a bug describing the right solution and then add a
TODO here referencing the bug.

[mohsen, 2015-12-24 02:35:45]

https://codereview.chromium.org/1537493006/diff/60001/ui/touch_selection/long...
File ui/touch_selection/longpress_drag_selector.cc (right):

https://codereview.chromium.org/1537493006/diff/60001/ui/touch_selection/long...
ui/touch_selection/longpress_drag_selector.cc:130: SetState(INACTIVE);
On 2015/12/22 at 21:55:54, mfomitchev wrote:
> As discussed, lets create a bug describing the right solution and then add a
TODO here referencing the bug.

Done.

[mfomitchev, 2016-01-04 17:07:55]

https://codereview.chromium.org/1537493006/diff/80001/ui/touch_selection/long...
File ui/touch_selection/longpress_drag_selector.h (right):

https://codereview.chromium.org/1537493006/diff/80001/ui/touch_selection/long...
ui/touch_selection/longpress_drag_selector.h:44: // TODO(mohsen): This is a
workaround to avoid starting longpress-drag gesture
It seems like the root of the issue is that (without this fix) TSC calls
OnSelectionActivated() on LongPressDragSelector when a scroll happens. It does
so because it interprets the selection _update_ (resulting from a scroll) as a
_new_ selection - because it has response_pending_input_event_ set to
LONG_PRESS.

Can we please describe this in a bit more detail? Also, I suggest we the
description of the issue to TSC::OnScrollBeginEvent() (in the .cc file). We
should also mention that this happens when the long press does not result in a
selection.

[mohsen, 2016-01-04 18:45:59]

https://codereview.chromium.org/1537493006/diff/80001/ui/touch_selection/long...
File ui/touch_selection/longpress_drag_selector.h (right):

https://codereview.chromium.org/1537493006/diff/80001/ui/touch_selection/long...
ui/touch_selection/longpress_drag_selector.h:44: // TODO(mohsen): This is a
workaround to avoid starting longpress-drag gesture
On 2016/01/04 at 17:07:55, mfomitchev wrote:
> It seems like the root of the issue is that (without this fix) TSC calls
OnSelectionActivated() on LongPressDragSelector when a scroll happens. It does
so because it interprets the selection _update_ (resulting from a scroll) as a
_new_ selection - because it has response_pending_input_event_ set to
LONG_PRESS.
> 
> Can we please describe this in a bit more detail? Also, I suggest we the
description of the issue to TSC::OnScrollBeginEvent() (in the .cc file). We
should also mention that this happens when the long press does not result in a
selection.

Done.

[mfomitchev, 2016-01-04 18:58:44]

Looks good, thanks! LGTM with a nit (I think it would be good to make the
comment a tad more explicit).

https://codereview.chromium.org/1537493006/diff/100001/ui/touch_selection/tou...
File ui/touch_selection/touch_selection_controller.cc (right):

https://codereview.chromium.org/1537493006/diff/100001/ui/touch_selection/tou...
ui/touch_selection/touch_selection_controller.cc:235: // If a scroll happens
after a long-press on an empty area, the long-press
Can we rephrase to be a bit more explicit, e.g.:
If there is an active selection, and the user performs a long press that does
not trigger a new selection (e.g. a long press on an empty area)...

https://codereview.chromium.org/1537493006/diff/100001/ui/touch_selection/tou...
ui/touch_selection/touch_selection_controller.cc:237: // In this case we might
think incorrectly that the selection change was due
Can we replace "might" with "will" here?
Also, perhaps add "(see ActivateSelectionIfNecessary())" in the end of the
sentence?

[mohsen, 2016-01-04 19:23:05]

https://codereview.chromium.org/1537493006/diff/100001/ui/touch_selection/tou...
File ui/touch_selection/touch_selection_controller.cc (right):

https://codereview.chromium.org/1537493006/diff/100001/ui/touch_selection/tou...
ui/touch_selection/touch_selection_controller.cc:235: // If a scroll happens
after a long-press on an empty area, the long-press
On 2016/01/04 at 18:58:44, mfomitchev wrote:
> Can we rephrase to be a bit more explicit, e.g.:
> If there is an active selection, and the user performs a long press that does
not trigger a new selection (e.g. a long press on an empty area)...

Done.

https://codereview.chromium.org/1537493006/diff/100001/ui/touch_selection/tou...
ui/touch_selection/touch_selection_controller.cc:237: // In this case we might
think incorrectly that the selection change was due
On 2016/01/04 at 18:58:44, mfomitchev wrote:
> Can we replace "might" with "will" here?
> Also, perhaps add "(see ActivateSelectionIfNecessary())" in the end of the
sentence?

Done.

[sadrul, 2016-01-04 21:00:42]

https://codereview.chromium.org/1537493006/diff/120001/content/browser/render...
File
content/browser/renderer_host/input/touch_selection_controller_client_aura.cc
(right):

https://codereview.chromium.org/1537493006/diff/120001/content/browser/render...
content/browser/renderer_host/input/touch_selection_controller_client_aura.cc:145:
rwhva_->selection_controller()->OnScrollBeginEvent();
I think this should go in
RenderWidgetHostViewAura::HandleGestureForTouchSelection() instead. RWHVAura and
RWHVAndroid both directly talk to TouchSelectionController for WillHandle*
events. RWHVAndroid directly talks to the TSC for OnScrollBeginEvent(), so
RWHVAura should too (instead of going through this client here).

[mohsen, 2016-01-05 19:46:17]

https://codereview.chromium.org/1537493006/diff/120001/content/browser/render...
File
content/browser/renderer_host/input/touch_selection_controller_client_aura.cc
(right):

https://codereview.chromium.org/1537493006/diff/120001/content/browser/render...
content/browser/renderer_host/input/touch_selection_controller_client_aura.cc:145:
rwhva_->selection_controller()->OnScrollBeginEvent();
On 2016/01/04 at 21:00:42, sadrul wrote:
> I think this should go in
RenderWidgetHostViewAura::HandleGestureForTouchSelection() instead. RWHVAura and
RWHVAndroid both directly talk to TouchSelectionController for WillHandle*
events. RWHVAndroid directly talks to the TSC for OnScrollBeginEvent(), so
RWHVAura should too (instead of going through this client here).

Done.

[sadrul, 2016-01-05 19:59:30]

lgtm

[mohsen, 2016-01-05 20:03:01]

The CQ bit was checked by mohsen@chromium.org

[mohsen, 2016-01-05 20:03:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
mfomitchev@chromium.org
Link to the patchset: https://codereview.chromium.org/1537493006/#ps140001
(title: "Addressed review comments")

[commit-bot: I haz the power, 2016-01-05 20:06:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1537493006/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1537493006/140001

[commit-bot: I haz the power, 2016-01-05 21:08:41]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2016-01-05 21:09:36]

Description was changed from

==========
Allow scrolling a page with selection after long-press on whitespace

Currently, if there is a selection on a page and you try long-press and
drag on an empty area, it will start longpress-drag gesture for
selecting text, starting at the current selection which might be far
from where the finger is. Better behavior would be to just scroll the
page.

BUG=557034
==========

to

==========
Allow scrolling a page with selection after long-press on whitespace

Currently, if there is a selection on a page and you try long-press and
drag on an empty area, it will start longpress-drag gesture for
selecting text, starting at the current selection which might be far
from where the finger is. Better behavior would be to just scroll the
page.

BUG=557034

Committed: https://crrev.com/91e3c85091d54ff42c06fad73c49a414dad3a29e
Cr-Commit-Position: refs/heads/master@{#367646}
==========

[commit-bot: I haz the power, 2016-01-05 21:09:37]

Patchset 7 (id:??) landed as
https://crrev.com/91e3c85091d54ff42c06fad73c49a414dad3a29e
Cr-Commit-Position: refs/heads/master@{#367646}