Fixed Android text selection by redefining |button| value.

Fixed Android text selection by redefining |button| value.

The |button| field in WebPointerProperties (used in WebMouseEvent and
WebTouchPoint) follows the dom UI Events spec which is well-defined for
mousedown/up events and for single-button-presses in other events. For
Android WebMouseEvents (enabled recently), we initially set this value
following the PointerEvent spec (which covers the multi-button-press
case perfectly) then discovered that text selection is not working. We
also discovered a bunch of other functionalities that depend on UI
Events like assumptions.

This CL refines Android |button| field to match other platforms, thus
fixes text selection.

BUG=666060
Review-Url: https://codereview.chromium.org/2639623002
Cr-Commit-Position: refs/heads/master@{#452150}
Committed: https://chromium.googlesource.com/chromium/src/+/58aea4974d2230c5900f79d9d6344747033a6500

[mustaq, 2017-02-16 21:59:18]

Description was changed from

==========
WIP: Trying to trigger text selection through mouse.

BUG=666060
==========

to

==========
Fixed mouse button assumptions in SelectionController for Android

Chromium for Android sets |WebMouseEvents.button| a bit differently from
other platforms: it represents the "button changed from the last event"
in Android, not the "last button pressed" as in the non-Android cases.
Blink fixes the JS-visible |button| value right before dispatching dom
events so web is not affected by this difference. But Selection controller
gets confused by this.

This CL uses |buttons| field instead to solve the problem.

BUG=666060
==========

[mustaq, 2017-02-16 22:03:43]

mustaq@chromium.org changed reviewers:
+ aelias@chromium.org, dtapuska@chromium.org

[mustaq, 2017-02-16 22:03:44]

ptal. Text selection now works nicely together with keyboard ctrl-c/ctrl-v for
basic copy-paste support.

Double/triple click doesn't work because MotionEvent provides no data about
click-count. We will need to add a timer-based counter somewhere. Will do it in
a follow up CL.

[aelias_OOO_until_Jul13, 2017-02-17 00:11:33]

Approach looks good.  For consistency and simplicity, please also delete the
code that sets WebMouseEvent::button for WM_MOUSEMOVE on
web_input_event_builders_win.cc and for
NSLeftMouseDragged/NSOtherMouseDragged/NSRightMouseDragged in
web_input_event_builders_mac.mm.  I think that's better than making Android
match the other platforms, since it's less code overall and, in the case of the
Windows code especially, it prioritizes leftmost button in a really arbitrary
manner.

Then we can be sure that if there is anything else subtly buggy about the
Android event model, we will hear about it :).

[aelias_OOO_until_Jul13, 2017-02-17 00:13:53]

Also, please add a test for this in WebViewTest.cpp.

[mustaq, 2017-02-21 17:39:06]

On 2017/02/17 00:11:33, aelias wrote:
> Approach looks good.  For consistency and simplicity, please also delete the
> code that sets WebMouseEvent::button for WM_MOUSEMOVE on
> web_input_event_builders_win.cc and for
> NSLeftMouseDragged/NSOtherMouseDragged/NSRightMouseDragged in
> web_input_event_builders_mac.mm.  I think that's better than making Android
> match the other platforms, since it's less code overall and, in the case of
the
> Windows code especially, it prioritizes leftmost button in a really arbitrary
> manner.
> 
> Then we can be sure that if there is anything else subtly buggy about the
> Android event model, we will hear about it :).

Thanks for pointing out the ui event conversion code for Windows & Mac. Linux
does
the exact same thing---prioritizes the L-button when the mouse is dragged with
multiple buttons down:
https://cs.chromium.org/chromium/src/ui/events/blink/web_input_event.cc?rcl=c...

Looks like my original assumption was wrongly biased towards the PointerEvents
model which is similar to Android!

The idea you mentioned---make Android match all other platforms---now seems
more reasonable to me. Further digging into the "users" of Button::Left
convinced me that changing |WebMouseEvent.button| (on drag) into Android-like
behavior would affect drag-and-drop, plugins, default event handlers for many
html elements and touch-emulation.

I will give it (make Android match others) a try, at least this is an easier
fix.
I will update you when done.

[aelias_OOO_until_Jul13, 2017-02-21 22:11:05]

I specifically said to please make the other platforms match Android.  It's a
better model.  There's no information added by setting "button" arbitrarily to
one of the buttons pressed during a move.  That variable is only relevant when
an event is sent specifically because a button is newly pressed.

[aelias_OOO_until_Jul13, 2017-02-21 22:20:49]

Hmm, thinking about this a bit more, lgtm as is.  "plugins" especially is an
intractable one to change -- that will force us to bake in this redundant field
forever, and it's not much uglier to have it live in the platform code.

Please just update the patch description before landing.  Not worth maintaining
an integration test for this as the risk of regression seems low.

[mustaq, 2017-02-22 15:39:52]

Description was changed from

==========
Fixed mouse button assumptions in SelectionController for Android

Chromium for Android sets |WebMouseEvents.button| a bit differently from
other platforms: it represents the "button changed from the last event"
in Android, not the "last button pressed" as in the non-Android cases.
Blink fixes the JS-visible |button| value right before dispatching dom
events so web is not affected by this difference. But Selection controller
gets confused by this.

This CL uses |buttons| field instead to solve the problem.

BUG=666060
==========

to

==========
Fixed Android mouse button assumptions to match other platforms.

The |button| field in WebPointerProperties (used in WebMouseEvent and
WebTouchPoint) follows the dom UI Events spec which is well-defined for
mousedown/up events and for single-button-presses in other events. For
Android WebMouseEvents (enabled recently), we initially set this value
following the PointerEvent spec (which covers the multi-button-press
case perfectly) then discovered that text selection is not working. We
also discovered a bunch of other functionalities that depend on the
UI Events like assumptions.

This refines Android |button| field to match other platforms, thus
fixes text selection.

BUG=666060
==========

[mustaq, 2017-02-22 15:42:24]

Description was changed from

==========
Fixed Android mouse button assumptions to match other platforms.

The |button| field in WebPointerProperties (used in WebMouseEvent and
WebTouchPoint) follows the dom UI Events spec which is well-defined for
mousedown/up events and for single-button-presses in other events. For
Android WebMouseEvents (enabled recently), we initially set this value
following the PointerEvent spec (which covers the multi-button-press
case perfectly) then discovered that text selection is not working. We
also discovered a bunch of other functionalities that depend on the
UI Events like assumptions.

This refines Android |button| field to match other platforms, thus
fixes text selection.

BUG=666060
==========

to

==========
Fixed Android text selection by redefining |button| value.

The |button| field in WebPointerProperties (used in WebMouseEvent and
WebTouchPoint) follows the dom UI Events spec which is well-defined for
mousedown/up events and for single-button-presses in other events. For
Android WebMouseEvents (enabled recently), we initially set this value
following the PointerEvent spec (which covers the multi-button-press
case perfectly) then discovered that text selection is not working. We
also discovered a bunch of other functionalities that depend on UI
Events like assumptions.

This CL refines Android |button| field to match other platforms, thus
fixes text selection.

BUG=666060
==========

[mustaq, 2017-02-22 15:43:41]

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

[mustaq, 2017-02-22 15:44:07]

The CQ bit was unchecked by mustaq@chromium.org

[commit-bot: I haz the power, 2017-02-22 15:44:13]

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

[mustaq, 2017-02-22 15:47:06]

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

[commit-bot: I haz the power, 2017-02-22 15:47:27]

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

[mustaq, 2017-02-22 15:50:23]

On 2017/02/21 22:20:49, aelias wrote:
> Hmm, thinking about this a bit more, lgtm as is.  "plugins" especially is an
> intractable one to change -- that will force us to bake in this redundant
field
> forever, and it's not much uglier to have it live in the platform code.
> 
> Please just update the patch description before landing.  Not worth
maintaining
> an integration test for this as the risk of regression seems low.

Done.

The |button| fields was not documented in our code, so added a comment to avoid
similar confusions in future. Also updated a few comments/identifiers.

[mustaq, 2017-02-22 15:51:40]

mustaq@chromium.org changed reviewers:
+ skyostil@chromium.org

[mustaq, 2017-02-22 15:51:40]

skyostil@chromium.org:
Please review changes in content/browser/android

[commit-bot: I haz the power, 2017-02-22 17:46:10]

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

[commit-bot: I haz the power, 2017-02-22 17:46:11]

Dry run: This issue passed the CQ dry run.

[Sami, 2017-02-22 17:49:50]

lgtm.

[mustaq, 2017-02-22 18:37:54]

The CQ bit was checked by mustaq@chromium.org

[mustaq, 2017-02-22 18:37:55]

The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org
Link to the patchset: https://codereview.chromium.org/2639623002/#ps80001
(title: "Fixed WebPointerProperties comment.")

[commit-bot: I haz the power, 2017-02-22 18:38:52]

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-02-22 18:56:06]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1487788674224930,
"parent_rev": "4ebd84e40a1c791117a8d3f7b85c13cfdf816864", "commit_rev":
"58aea4974d2230c5900f79d9d6344747033a6500"}

[commit-bot: I haz the power, 2017-02-22 18:56:47]

Description was changed from

==========
Fixed Android text selection by redefining |button| value.

The |button| field in WebPointerProperties (used in WebMouseEvent and
WebTouchPoint) follows the dom UI Events spec which is well-defined for
mousedown/up events and for single-button-presses in other events. For
Android WebMouseEvents (enabled recently), we initially set this value
following the PointerEvent spec (which covers the multi-button-press
case perfectly) then discovered that text selection is not working. We
also discovered a bunch of other functionalities that depend on UI
Events like assumptions.

This CL refines Android |button| field to match other platforms, thus
fixes text selection.

BUG=666060
==========

to

==========
Fixed Android text selection by redefining |button| value.

The |button| field in WebPointerProperties (used in WebMouseEvent and
WebTouchPoint) follows the dom UI Events spec which is well-defined for
mousedown/up events and for single-button-presses in other events. For
Android WebMouseEvents (enabled recently), we initially set this value
following the PointerEvent spec (which covers the multi-button-press
case perfectly) then discovered that text selection is not working. We
also discovered a bunch of other functionalities that depend on UI
Events like assumptions.

This CL refines Android |button| field to match other platforms, thus
fixes text selection.

BUG=666060

Review-Url: https://codereview.chromium.org/2639623002
Cr-Commit-Position: refs/heads/master@{#452150}
Committed:
https://chromium.googlesource.com/chromium/src/+/58aea4974d2230c5900f79d9d634...
==========

[commit-bot: I haz the power, 2017-02-22 18:56:48]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/58aea4974d2230c5900f79d9d634...