Android mouse events shouldn't appear as TouchEvents

content/browser/android/content_view_core_impl.cc

Index: content/browser/android/content_view_core_impl.cc
diff --git a/content/browser/android/content_view_core_impl.cc b/content/browser/android/content_view_core_impl.cc
index a7f92639fa22c313db88252bc64615f8ebf06c4a..58e5b98bce28e6e849ddf1cd65c2ba9f79837974 100644
--- a/content/browser/android/content_view_core_impl.cc
+++ b/content/browser/android/content_view_core_impl.cc
@@ -61,6 +61,7 @@
 #include "ui/events/blink/blink_event_util.h"
 #include "ui/events/blink/web_input_event_traits.h"
 #include "ui/events/event_utils.h"
+#include "ui/events/gesture_detection/motion_event.h"
 #include "ui/gfx/android/java_bitmap.h"
 #include "ui/gfx/geometry/point_conversions.h"
 #include "ui/gfx/geometry/size_conversions.h"
@@ -217,7 +218,8 @@ ContentViewCoreImpl::ContentViewCoreImpl(
       page_scale_(1),
       dpi_scale_(ui::GetScaleFactorForNativeView(&view_)),
       device_orientation_(0),
-      accessibility_enabled_(false) {
+      accessibility_enabled_(false),
+      button_state_(0) {
   CHECK(web_contents) <<
       "A ContentViewCoreImpl should be created with a valid WebContents.";
   DCHECK(window_android);
@@ -966,31 +968,80 @@ jboolean ContentViewCoreImpl::OnTouchEvent(
                            android_meta_state,
                            raw_pos_x - pos_x_0,
                            raw_pos_y - pos_y_0,
-                           pointer0,
-                           pointer1);
+                           &pointer0,
+                           &pointer1);
 
   return is_touch_handle_event ? rwhv->OnTouchHandleEvent(event)
                                : rwhv->OnTouchEvent(event);
 }
 
-jboolean ContentViewCoreImpl::SendMouseMoveEvent(
+jboolean ContentViewCoreImpl::SendMouseEvent(
     JNIEnv* env,
     const JavaParamRef<jobject>& obj,
     jlong time_ms,
+    jint android_action,
     jfloat x,
     jfloat y,
-    jint tool_type) {
+    jint pointer_id,
+    jfloat pressure,
+    jfloat orientation,
+    jfloat tilt,
+    jint android_button_state,
+    jint android_meta_state,
+    jint android_tool_type) {
+
   RenderWidgetHostViewAndroid* rwhv = GetRenderWidgetHostViewAndroid();
   if (!rwhv)
     return false;
 
-  blink::WebMouseEvent event = WebMouseEventBuilder::Build(
-      WebInputEvent::MouseMove,
-      blink::WebMouseEvent::Button::NoButton,
-      time_ms / 1000.0, x / dpi_scale(), y / dpi_scale(), 0, 1,
-      ui::ToWebPointerType(static_cast<ui::MotionEvent::ToolType>(tool_type)));
-
-  rwhv->SendMouseEvent(event);
+  // Construct a motion_event object minimally, only to convert the raw
+  // parameters to ui::MotionEvent values. Since we used only the cached values
+  // at index=0, it is okay to even pass a null event to the constructor.
+  ui::MotionEventAndroid::Pointer pointer0(
+      pointer_id, x, y, 0.0f /* touch_major */, 0.0f /* touch_minor */,
+      orientation, tilt, android_tool_type);
+
+  ui::MotionEventAndroid motion_event(1.f / dpi_scale(),
+      env,
+      nullptr /* event */,
+      time_ms,
+      android_action,
+      1 /* pointer_count */,
+      0 /* history_size */,
+      0 /* action_index */,
+      android_button_state,
+      android_meta_state,
+      0 /* raw_offset_x_pixels */,
+      0 /* raw_offset_y_pixels */,
+      &pointer0,
+      nullptr);
+
+  blink::WebInputEvent::Type webMouseEventType =
+      ui::ToWebMouseEventType(motion_event.GetAction());
+
+  int old_button_state = button_state_;
+
+  // Ignore button state in MouseMove events: we have seen cases that a

[aelias_OOO_until_Jul13, 2016/10/20 23:26:29]

If the mouse is pressed outside the ContentViewCore and then the cursor enters
while still pressed, wouldn't the state be inaccurate?

[mustaq, 2016/10/21 19:19:08]

Right. I am not sure how to find the info correctly.

I realized that the bug I saw was on a Samsung device. I should try a
non-Samsung one, will do next week.

[aelias_OOO_until_Jul13, 2016/10/21 21:26:54]

Well, regardless of Samsung-specific or not, given that you're adding complexity
to trade off one MouseMove button-press state bug for another, I'd prefer we
land this without the workaround, for now.  I'm not sure in practice how many
websites would have a problem with the early MouseMove button-press, I suspect
it would rarely cause an issue.

My bias is to transparently pass input data just as given to us from the OS
unless there is a very strong reason to alter/sanitize.  I would prefer to wait
to discover a real issue on a website before introducing a complex workaround
for the odd Android behavior.

[mustaq, 2016/10/26 17:45:09]

Looks like we were talking about two different things here, correct me if I
missed something:

[A]
My suspected Samsung-only behavior was the occasional mousemoves with correct
button values before a mousedown. It turned out to be a pair of hover exit/enter
events before & after a mouse down (both Samsung & non-Samsung devices). These
appeared as redundant mousemoves before in the doc and here. Skipped firing the
hover enter/exit event firing for now, affects only the |if| statement here.

[B]
Re storing mouse button state: I agree that passing on the input from OS as
transparently as possible is the best approach in general. But in this case a
fundamental difference between a MotionEvent and a MouseEvent (hence its
subclasses PointerEvent & WheelEvent) comes into the scene: the only info about
buttons we get from a MotionEvent is current state while MouseEvent needs both
the current state (.buttons) and the button that changed (.button). One "clear
breaker" here is mouse-release: MotionEvent.getButtonState() returns 0 here (and
we need this for MouseEvent.buttons) but MouseEvent.button should report the
button that has got released.

Added a comment in the .h file. Does it address your concern now?

[aelias_OOO_until_Jul13, 2016/10/27 03:09:37]

OK, thanks for the explanation.  I really would prefer us to do that without
storing state if possible, in my experience statefulness leads to bugs. 
Wouldn't it work to use
https://developer.android.com/reference/android/view/MotionEvent.html#ACTION_...
to determine what buttons changed on the current event?

[aelias_OOO_until_Jul13, 2016/10/27 18:27:56]

finger#2, stylus#1 etc), hence less "granular" than buttons for a given mouse or
stylus. The deprecated getActionButton() method seems to have provided info
about
changed buttons, not sure why they deprecated it w/o a equivalent or better
replacement.

OK, then please try to use the deprecated getActionButton() values.  It has been
deprecated for a long time, but not hidden/removed so I have no reason to think
it no longer works.  (Android-team is not very systematic about following up on
deprecation.)  Assuming it works everywhere, we can file a bug against
Android-team to undeprecate since there is no proper replacement.

[aelias_OOO_until_Jul13, 2016/10/27 23:36:48]

events (ACTION_BUTTON_PRESS & _RELEASE), and undefined for our events here (e.g.
_DOWN). So this would still need a state variable to pass on this value to next
mouse events. This is in the spec, I missed it before sorry! The reliance on a
dead API plus additional plumbing needed for the dead event class makes it worse
than the current "state". Wdyt?

Then I think we should go all-in on the superior deprecated API and switch to
ACTION_BUTTON_PRESS/RELEASE unless that would cause a substantive problem.

I realize this recommendation is counterintuitive so here's where I'm coming
from:
A) Deprecation is just some Android engineer's opinion three years ago that this
API has been obsoleted by their new thing, without actually checking with any
users before making that decision.  And it's probably been long forgotten about
and no actual deletion of the API is planned anytime soon.  That's my experience
of how deprecations work in Android-land, they're very scattershot.  So that
designation doesn't carry much weight for me.
B) I've seen enough input bugs caused by hysteresis that I'm very allergic to
introducing any that's not strictly necessary.  You're tracking state about
history of mouse events in a particular view that doesn't have full visibility
into the true history (the past events may have been sent to a different view
for various reasons).
C) ContentViewCore is slated for deletion meaning that its contents will need to
be moved elsewhere.  Moving stateless methods is trivial, whereas moving around
state means difficult reasoning about lifetimes (made more difficult by the fact
that the lifetime is unprincipled in the first place, see point B).

+  // MouseMove with updated button state often precedes a MouseDown/MouseUp.
+  if (webMouseEventType != blink::WebInputEvent::MouseMove)
+    button_state_ = motion_event.GetButtonState();
+
+  blink::WebMouseEvent mouse_event = WebMouseEventBuilder::Build(
+      webMouseEventType,
+      time_ms / 1000.0,
+      motion_event.GetX(0),
+      motion_event.GetY(0),
+      motion_event.GetFlags(),
+      button_state_ ? 1 : 0 /* click count */,
+      motion_event.GetPointerId(0),
+      motion_event.GetPressure(0),
+      motion_event.GetOrientation(0),
+      motion_event.GetTilt(0),
+      old_button_state,
+      button_state_,
+      motion_event.GetToolType(0));
+
+  rwhv->SendMouseEvent(mouse_event);
   return true;
 }