Fix issue with longpress drag selection motion

Fix issue with longpress drag selection motion

If the user has already dragged outside the initial selected word region
by the time the browser gets a selection update, or if the selected word
isn't coincident with the longpress location, the subsequently dragged
selection endpoint can be wrong. For such cases, rather than
using the drag direction for drag anchor picking, just use the distance
between the current touch point and the selection endpoints.

BUG=466749
Committed: https://crrev.com/a188bd6dff9c2a0425d1f31e3ab929c373762d83
Cr-Commit-Position: refs/heads/master@{#341791}

[jdduke (slow), 2015-08-03 17:31:14]

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

[jdduke (slow), 2015-08-03 17:31:15]

mohsen@: PTAL, thanks. We'll probably need to cherry-pick this to M45 for the
initial release in Chrome.

[jdduke (slow), 2015-08-04 17:30:45]

jdduke@chromium.org changed reviewers:
+ mfomitchev@chromium.org
- mohsen@chromium.org

[jdduke (slow), 2015-08-04 17:30:46]

-mohsen, +mfomitchev

Mikhail could you take a look?

[mfomitchev, 2015-08-04 18:31:05]

This is because we set the longpress_drag_start_anchor_ to the first move
coordinate instead of touch_down_position_, right? And we can't use
touch_down_position_ because "showing the selection UI may have shifted the
motion coordinates." according to the comment in the code. Is this just because
the appearance of action bar may scroll the content? If so - can we disable that
behavior? From what I see, it's already not happening in Chrome, only in
WebView...

[mfomitchev, 2015-08-04 18:31:36]

This is because we set the longpress_drag_start_anchor_ to the first move
coordinate instead of touch_down_position_, right? And we can't use
touch_down_position_ because "showing the selection UI may have shifted the
motion coordinates." according to the comment in the code. Is this just because
the appearance of action bar may scroll the content? If so - can we disable that
behavior? From what I see, it's already not happening in Chrome, only in
WebView...

https://codereview.chromium.org/1271823002/diff/1/ui/touch_selection/longpres...
File ui/touch_selection/longpress_drag_selector.cc (right):

https://codereview.chromium.org/1271823002/diff/1/ui/touch_selection/longpres...
ui/touch_selection/longpress_drag_selector.cc:93: if (start_dot_product < 0 &&
end_dot_product < 0) {
nit: It makes it a bit harder to parse when the order in which things are
described in the comment (lines 80-83) is different from the order in which
things are handled in code.

Perhaps handle the case when "start_dot_product > 0 || end_dot_product > 0"
first? Or change up the comment to mention the "moving away from both endpoints"
case before the other one?

https://codereview.chromium.org/1271823002/diff/1/ui/touch_selection/longpres...
File ui/touch_selection/longpress_drag_selector_unittest.cc (right):

https://codereview.chromium.org/1271823002/diff/1/ui/touch_selection/longpres...
ui/touch_selection/longpress_drag_selector_unittest.cc:356:
EXPECT_TRUE(selector.WillHandleTouchEvent(event.MovePoint(0, 30, 15)));
would be nice to express the coordinates in terms of kSlop.

[jdduke (slow), 2015-08-04 19:14:51]

On 2015/08/04 18:31:36, mfomitchev wrote:
> This is because we set the longpress_drag_start_anchor_ to the first move
> coordinate instead of touch_down_position_, right? And we can't use
> touch_down_position_ because "showing the selection UI may have shifted the
> motion coordinates." according to the comment in the code. Is this just
because
> the appearance of action bar may scroll the content?

What you say is true, but we also need to handle this case in the event that the
longpress location isn't inside the actual selected word boundary, which
currently can happen with some frequency. I'll make a note of that in the patch
description.

> If so - can we disable that
> behavior? From what I see, it's already not happening in Chrome, only in
> WebView...

Unfortunatley WebView is at the mercy of the embedder for dictating how the
actionbar is drawn. With floating actionbars this isn't an issue, but until such
time we don't really have control over how the WebView moves with respect to the
actionbar.

One thought I had a while ago was to use absolute touch coordinates to determine
the motion, though that might get ugly because the selection coordinates are
still local, so I think we'd still need some fudging.

https://codereview.chromium.org/1271823002/diff/1/ui/touch_selection/longpres...
File ui/touch_selection/longpress_drag_selector.cc (right):

https://codereview.chromium.org/1271823002/diff/1/ui/touch_selection/longpres...
ui/touch_selection/longpress_drag_selector.cc:93: if (start_dot_product < 0 &&
end_dot_product < 0) {
On 2015/08/04 18:31:36, mfomitchev wrote:
> nit: It makes it a bit harder to parse when the order in which things are
> described in the comment (lines 80-83) is different from the order in which
> things are handled in code.
> 
> Perhaps handle the case when "start_dot_product > 0 || end_dot_product > 0"
> first? Or change up the comment to mention the "moving away from both
endpoints"
> case before the other one?

Done.

https://codereview.chromium.org/1271823002/diff/1/ui/touch_selection/longpres...
File ui/touch_selection/longpress_drag_selector_unittest.cc (right):

https://codereview.chromium.org/1271823002/diff/1/ui/touch_selection/longpres...
ui/touch_selection/longpress_drag_selector_unittest.cc:356:
EXPECT_TRUE(selector.WillHandleTouchEvent(event.MovePoint(0, 30, 15)));
On 2015/08/04 18:31:36, mfomitchev wrote:
> would be nice to express the coordinates in terms of kSlop.

Done.

[mfomitchev, 2015-08-04 19:22:28]

LGTM

[jdduke (slow), 2015-08-04 19:27:04]

The CQ bit was checked by jdduke@chromium.org

[commit-bot: I haz the power, 2015-08-04 19:28:35]

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

[commit-bot: I haz the power, 2015-08-04 21:05:19]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-08-04 21:08:05]

Patchset 2 (id:??) landed as
https://crrev.com/a188bd6dff9c2a0425d1f31e3ab929c373762d83
Cr-Commit-Position: refs/heads/master@{#341791}