Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2054193002:
Android mouse events shouldn't appear as TouchEvents (Closed)
DescriptionAndroid mouse events shouldn't appear as TouchEvents
Android low-level mouse events in Chrome used to follow the
"default" MotionEvent path which is designed for touches. In
particular in Blink, mouse events appeared as PlatformTouchEvents
which had some bad side-effects:
A. TouchEvents were fired for mouse.
B. Mouse events were suppressed through prevent-defaulting touch
events.
C. Mouse event suppression thru canceled touches apply only to
non-hovering mouse.
This CL fixes the mouse issues by routing Android mouse events
through a dedicated Web/PlatformMouseEvent path. The stylus
properties are also plumbed, to make Android stylus support
easier (follow-up CL).
Design doc: https://docs.google.com/document/d/1mpBR7J7kgTXvp0QACVjhxtwNJ7bgGoTMmxfxN2dupGg/edit?usp=sharing
BUG=468806, 587550
Committed: https://crrev.com/5b679e43949bc52452422b453e81fb8e48d0594f
Cr-Commit-Position: refs/heads/master@{#431571}
Patch Set 1 #Patch Set 2 : WIP: Added Android ME plumbing. #Patch Set 3 : Added button-changed-state #Patch Set 4 : Working hacky patch, includes pen hack. #Patch Set 5 : Added working mouse buttons. #
Total comments: 2
Patch Set 6 : Added meta_state plumbing. #
Total comments: 13
Patch Set 7 : Added enter/exit event plumbing. Rebased. #Patch Set 8 : Added hover_move enums, skipped ME firing for them enter/leave. #Patch Set 9 : Moved button state to render_widget_host_view_android. Rebased. #Patch Set 10 : Added ACTION_BUTTON* plumbing, removed button state from rwhv. #Patch Set 11 : Added ACTION_BUTTON* plumbing, removed button state from rwhv. #Patch Set 12 : Fixed compile failures. #Patch Set 13 : Kept old touch-like behavior in pre-M. #
Total comments: 18
Patch Set 14 : Addressed comments. #Patch Set 15 : Unconditional call to getActionButton #Patch Set 16 : Fixed a test, etc. #Messages
Total messages: 64 (26 generated)
Description was changed from ========== Bypassed TouchEvents for MotionEvents of type mouse. BUG=587550 ========== to ========== WIP: MouseEvents in Android: Complete mouse event plumbing MouseEvents used to be plumbed as TouchEvent. Moreover, existing mouse event plumbing had hardcoded assumptions about mouse-moves because it was added /only/ to support hovering mouse since TouchEvent has no notion of hovering. BUG=587550 ==========
Patchset #5 (id:80001) has been deleted
Description was changed from ========== WIP: MouseEvents in Android: Complete mouse event plumbing MouseEvents used to be plumbed as TouchEvent. Moreover, existing mouse event plumbing had hardcoded assumptions about mouse-moves because it was added /only/ to support hovering mouse since TouchEvent has no notion of hovering. BUG=587550 ========== to ========== WIP: MouseEvents in Android: Complete mouse event plumbing MouseEvents used to be plumbed as TouchEvent. Moreover, existing mouse event plumbing had hardcoded assumptions about mouse-moves because it was added /only/ to support hovering mouse since TouchEvent has no notion of hovering. BUG=468806,587550 ==========
Description was changed from ========== WIP: MouseEvents in Android: Complete mouse event plumbing MouseEvents used to be plumbed as TouchEvent. Moreover, existing mouse event plumbing had hardcoded assumptions about mouse-moves because it was added /only/ to support hovering mouse since TouchEvent has no notion of hovering. BUG=468806,587550 ========== to ========== Android mouse events shouldn't appear as TouchEvent Android low-level mouse events in Chrome used to follow the "default" MotionEvent path which is designed for touches. In particular in Blink, mouse events appeared as PlatformTouchEvents which had some bad side-effects: A. It caused firing of touchstart/touchmove/touchend which doesn't make sense. B. The mouse events fired to the page are actually the compat mouse events from touch gestures. Thus cancelling touch gestures (by prevent-defaulting touch-start) suppresses mouse event firing. C. Since TouchEvents inherently lack hovering support, to support a hovering mouse/stylus for the web, a minimal MouseEvent path was added a while ago only for android hover events. So mouse event suppression thru canceled touches apply only to non-hovering mouse. This CL fixed the mouse issues by routing Android mouse events through a dedicated Web/PlatformMouseEvent path. BUG=468806,587550 ==========
Description was changed from ========== Android mouse events shouldn't appear as TouchEvent Android low-level mouse events in Chrome used to follow the "default" MotionEvent path which is designed for touches. In particular in Blink, mouse events appeared as PlatformTouchEvents which had some bad side-effects: A. It caused firing of touchstart/touchmove/touchend which doesn't make sense. B. The mouse events fired to the page are actually the compat mouse events from touch gestures. Thus cancelling touch gestures (by prevent-defaulting touch-start) suppresses mouse event firing. C. Since TouchEvents inherently lack hovering support, to support a hovering mouse/stylus for the web, a minimal MouseEvent path was added a while ago only for android hover events. So mouse event suppression thru canceled touches apply only to non-hovering mouse. This CL fixed the mouse issues by routing Android mouse events through a dedicated Web/PlatformMouseEvent path. BUG=468806,587550 ========== to ========== Android mouse events shouldn't appear as TouchEvent Android low-level mouse events in Chrome used to follow the "default" MotionEvent path which is designed for touches. In particular in Blink, mouse events appeared as PlatformTouchEvents which had some bad side-effects: A. TouchEvents were fired for mouse. B. Mouse events were suppressed through prevent-defaulting touch events. C. Mouse event suppression thru canceled touches apply only to non-hovering mouse. This CL fixes the mouse issues by routing Android mouse events through a dedicated Web/PlatformMouseEvent path. The stylus properties are also plumbed, to make Android stylus support easier (follow-up CL). BUG=468806,587550 ==========
Description was changed from ========== Android mouse events shouldn't appear as TouchEvent Android low-level mouse events in Chrome used to follow the "default" MotionEvent path which is designed for touches. In particular in Blink, mouse events appeared as PlatformTouchEvents which had some bad side-effects: A. TouchEvents were fired for mouse. B. Mouse events were suppressed through prevent-defaulting touch events. C. Mouse event suppression thru canceled touches apply only to non-hovering mouse. This CL fixes the mouse issues by routing Android mouse events through a dedicated Web/PlatformMouseEvent path. The stylus properties are also plumbed, to make Android stylus support easier (follow-up CL). BUG=468806,587550 ========== to ========== Android mouse events shouldn't appear as TouchEvents Android low-level mouse events in Chrome used to follow the "default" MotionEvent path which is designed for touches. In particular in Blink, mouse events appeared as PlatformTouchEvents which had some bad side-effects: A. TouchEvents were fired for mouse. B. Mouse events were suppressed through prevent-defaulting touch events. C. Mouse event suppression thru canceled touches apply only to non-hovering mouse. This CL fixes the mouse issues by routing Android mouse events through a dedicated Web/PlatformMouseEvent path. The stylus properties are also plumbed, to make Android stylus support easier (follow-up CL). BUG=468806,587550 ==========
mustaq@chromium.org changed reviewers: + dtapuska@chromium.org
ptal
https://codereview.chromium.org/2054193002/diff/100001/content/browser/androi... File content/browser/android/content_view_core_impl.cc (left): https://codereview.chromium.org/2054193002/diff/100001/content/browser/androi... content/browser/android/content_view_core_impl.cc:978: const JavaParamRef<jobject>& obj, Can we pass const JavaParamRef<jobject>& motion_event, in here to make it look similar to touch? and maybe the meta state?
mustaq@chromium.org changed reviewers: + aelias@chromium.org
aelias@chromium.org: ptal for early comments. I think it bypasses all TE generation. I am not sure which part of the plumbing could be affected on a pre-M Android.
https://codereview.chromium.org/2054193002/diff/100001/content/browser/androi... File content/browser/android/content_view_core_impl.cc (left): https://codereview.chromium.org/2054193002/diff/100001/content/browser/androi... content/browser/android/content_view_core_impl.cc:978: const JavaParamRef<jobject>& obj, On 2016/09/20 20:56:16, dtapuska wrote: > Can we pass const JavaParamRef<jobject>& motion_event, in here to make it look > similar to touch? > > and maybe the meta state? - Done adding meta state. - Skipped passing motion_event ref because touch is the only exception here that it may pack multiple pointers. Like most other cases here (SendMouseWheelEvent, ScrollBegin, SingleTap), unpacked cases don't need the Java ref. MotionEventAndroid already allows that through DCHECKs.
On 2016/09/21 19:31:29, mustaq wrote: > https://codereview.chromium.org/2054193002/diff/100001/content/browser/androi... > File content/browser/android/content_view_core_impl.cc (left): > > https://codereview.chromium.org/2054193002/diff/100001/content/browser/androi... > content/browser/android/content_view_core_impl.cc:978: const > JavaParamRef<jobject>& obj, > On 2016/09/20 20:56:16, dtapuska wrote: > > Can we pass const JavaParamRef<jobject>& motion_event, in here to make it look > > similar to touch? > > > > and maybe the meta state? > > - Done adding meta state. > > - Skipped passing motion_event ref because touch is the only exception here that > it may pack multiple pointers. Like most other cases here (SendMouseWheelEvent, > ScrollBegin, SingleTap), unpacked cases don't need the Java ref. > MotionEventAndroid already allows that through DCHECKs. seems ok; but I'll defer to aelias@
The code structure seems OK at a glance, but my concerns around this are mainly around substantive issues: A) what UX are we going with for right-click and middle-click, B) what happens on older Android devices which give us weirder events. You haven't described precisely what you've observed to happen in all those scenarios, and it's hard to guess just by looking at the code, so I'll probably need to try out locally and form opinions. This might also call for a one-pager design doc laying out all the scenarios possible (matrix of mouse buttons and Android versions) and what you intend to happen from the user's point of view and from the developer's point of view, and why. Then I could debate specific behaviors in doc comments.
On 2016/09/21 22:08:45, aelias wrote: > The code structure seems OK at a glance, but my concerns around this are mainly > around substantive issues: A) what UX are we going with for right-click and > middle-click, B) what happens on older Android devices which give us weirder > events. You haven't described precisely what you've observed to happen in all > those scenarios, and it's hard to guess just by looking at the code, so I'll > probably need to try out locally and form opinions. > > This might also call for a one-pager design doc laying out all the scenarios > possible (matrix of mouse buttons and Android versions) and what you intend to > happen from the user's point of view and from the developer's point of view, and > why. Then I could debate specific behaviors in doc comments. It certainly needs more tests. So far tested left clicks on M, seems working fine both for Chrome UI & content. Silly hardware issues blocked my testing other clicks, will so soon. Documenting behavior diffs across Andoird versions sounds like a good plan. Any suggestion on which pre-M version(s) should be tested?
On 2016/09/22 18:46:54, mustaq wrote: > On 2016/09/21 22:08:45, aelias wrote: > > The code structure seems OK at a glance, but my concerns around this are > mainly > > around substantive issues: A) what UX are we going with for right-click and > > middle-click, B) what happens on older Android devices which give us weirder > > events. You haven't described precisely what you've observed to happen in all > > those scenarios, and it's hard to guess just by looking at the code, so I'll > > probably need to try out locally and form opinions. > > > > This might also call for a one-pager design doc laying out all the scenarios > > possible (matrix of mouse buttons and Android versions) and what you intend to > > happen from the user's point of view and from the developer's point of view, > and > > why. Then I could debate specific behaviors in doc comments. > > It certainly needs more tests. So far tested left clicks on M, seems working > fine > both for Chrome UI & content. Silly hardware issues blocked my testing other > clicks, will so soon. > > Documenting behavior diffs across Andoird versions sounds like a good plan. Any > suggestion on which pre-M version(s) should be tested? As we discussed in the doc, mouse behavior seems consistent across Android L & M except that Samsung devices hide right & middle mousedowns from all apps to "replace" these events with "back" and "home" button presses respectively.
On 2016/10/20 14:13:20, mustaq wrote: > On 2016/09/22 18:46:54, mustaq wrote: > > On 2016/09/21 22:08:45, aelias wrote: > > > The code structure seems OK at a glance, but my concerns around this are > > mainly > > > around substantive issues: A) what UX are we going with for right-click and > > > middle-click, B) what happens on older Android devices which give us weirder > > > events. You haven't described precisely what you've observed to happen in > all > > > those scenarios, and it's hard to guess just by looking at the code, so I'll > > > probably need to try out locally and form opinions. > > > > > > This might also call for a one-pager design doc laying out all the scenarios > > > possible (matrix of mouse buttons and Android versions) and what you intend > to > > > happen from the user's point of view and from the developer's point of view, > > and > > > why. Then I could debate specific behaviors in doc comments. > > > > It certainly needs more tests. So far tested left clicks on M, seems working > > fine > > both for Chrome UI & content. Silly hardware issues blocked my testing other > > clicks, will so soon. > > > > Documenting behavior diffs across Andoird versions sounds like a good plan. > Any > > suggestion on which pre-M version(s) should be tested? > > As we discussed in the doc, mouse behavior seems consistent across Android L & M > except that Samsung devices hide right & middle mousedowns from all apps to > "replace" these events with "back" and "home" button presses respectively. Here is the doc: https://docs.google.com/document/d/1mpBR7J7kgTXvp0QACVjhxtwNJ7bgGoTMmxfxN2dup...
https://codereview.chromium.org/2054193002/diff/120001/content/browser/androi... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/2054193002/diff/120001/content/browser/androi... content/browser/android/content_view_core_impl.cc:1024: // Ignore button state in MouseMove events: we have seen cases that a If the mouse is pressed outside the ContentViewCore and then the cursor enters while still pressed, wouldn't the state be inaccurate? https://codereview.chromium.org/2054193002/diff/120001/content/browser/androi... File content/browser/android/content_view_core_impl.h (right): https://codereview.chromium.org/2054193002/diff/120001/content/browser/androi... content/browser/android/content_view_core_impl.h:483: int button_state_; FWIW, this state doesn't really belong here, although I understand it's needed to work around a bug... I'm mulling over if there's any way it could be avoided. Could you rebase this patch so that I can apply it locally and observe the buggy early-pressed MouseMoves? https://codereview.chromium.org/2054193002/diff/120001/ui/events/android/moti... File ui/events/android/motion_event_android.cc (right): https://codereview.chromium.org/2054193002/diff/120001/ui/events/android/moti... ui/events/android/motion_event_android.cc:203: if (cached_pointer_count_ > 1) This looks kind of weird (I understand you expect this value to be exactly 2?). Could you replace this with a for loop up to cached_pointer_count_? You would also need to replace the arguments passed in to this method with an array pointers[MAX_POINTERS_TO_CACHE].
https://codereview.chromium.org/2054193002/diff/120001/content/browser/androi... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/2054193002/diff/120001/content/browser/androi... content/browser/android/content_view_core_impl.cc:1024: // Ignore button state in MouseMove events: we have seen cases that a On 2016/10/20 23:26:29, aelias wrote: > If the mouse is pressed outside the ContentViewCore and then the cursor enters > while still pressed, wouldn't the state be inaccurate? 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. https://codereview.chromium.org/2054193002/diff/120001/content/browser/androi... File content/browser/android/content_view_core_impl.h (right): https://codereview.chromium.org/2054193002/diff/120001/content/browser/androi... content/browser/android/content_view_core_impl.h:483: int button_state_; On 2016/10/20 23:26:29, aelias wrote: > FWIW, this state doesn't really belong here, although I understand it's needed > to work around a bug... I'm mulling over if there's any way it could be > avoided. Could you rebase this patch so that I can apply it locally and observe > the buggy early-pressed MouseMoves? The latest patch is after a rebase. I have also added enter/leave plumbing there, checked only on M. Still couldn't locate an N device here :( https://codereview.chromium.org/2054193002/diff/120001/ui/events/android/moti... File ui/events/android/motion_event_android.cc (right): https://codereview.chromium.org/2054193002/diff/120001/ui/events/android/moti... ui/events/android/motion_event_android.cc:203: if (cached_pointer_count_ > 1) On 2016/10/20 23:26:29, aelias wrote: > This looks kind of weird (I understand you expect this value to be exactly 2?). > Could you replace this with a for loop up to cached_pointer_count_? You would > also need to replace the arguments passed in to this method with an array > pointers[MAX_POINTERS_TO_CACHE]. Just realized that the name is a bit confusing here: it is the cached value of pointer-count, not the count of cached-pointers! The name was there before, and I didn't see the second interpretation until your comment. So cached_pointer_count_ can be any value supported by the platform (e.g. 3 for a three finger interaction). The parameters of upto 2 pointers are cached in this C++ class to save jni round trips. Accessing the parameters of the 3rd or a later pointer causes jni calls; see MotionEventAndroid::GetPointerId(). Do you still prefer passing an array of size 2, instead of two params?
https://codereview.chromium.org/2054193002/diff/120001/content/browser/androi... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/2054193002/diff/120001/content/browser/androi... content/browser/android/content_view_core_impl.cc:1024: // Ignore button state in MouseMove events: we have seen cases that a On 2016/10/21 at 19:19:08, mustaq wrote: > On 2016/10/20 23:26:29, aelias wrote: > > If the mouse is pressed outside the ContentViewCore and then the cursor enters > > while still pressed, wouldn't the state be inaccurate? > > 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. 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. https://codereview.chromium.org/2054193002/diff/120001/ui/events/android/moti... File ui/events/android/motion_event_android.cc (right): https://codereview.chromium.org/2054193002/diff/120001/ui/events/android/moti... ui/events/android/motion_event_android.cc:203: if (cached_pointer_count_ > 1) On 2016/10/21 at 19:19:08, mustaq wrote: > On 2016/10/20 23:26:29, aelias wrote: > > This looks kind of weird (I understand you expect this value to be exactly 2?). > > Could you replace this with a for loop up to cached_pointer_count_? You would > > also need to replace the arguments passed in to this method with an array > > pointers[MAX_POINTERS_TO_CACHE]. > > Just realized that the name is a bit confusing here: it is the cached value of pointer-count, not the count of cached-pointers! The name was there before, and I didn't see the second interpretation until your comment. > > So cached_pointer_count_ can be any value supported by the platform (e.g. 3 for a three finger interaction). The parameters of upto 2 pointers are cached in this C++ class to save jni round trips. Accessing the parameters of the 3rd or a later pointer causes jni calls; see MotionEventAndroid::GetPointerId(). > > Do you still prefer passing an array of size 2, instead of two params? OK then, fine as is.
ptal, had to skip hover exit/enter events... https://codereview.chromium.org/2054193002/diff/120001/content/browser/androi... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/2054193002/diff/120001/content/browser/androi... content/browser/android/content_view_core_impl.cc:1024: // Ignore button state in MouseMove events: we have seen cases that a On 2016/10/21 21:26:54, aelias wrote: > On 2016/10/21 at 19:19:08, mustaq wrote: > > On 2016/10/20 23:26:29, aelias wrote: > > > If the mouse is pressed outside the ContentViewCore and then the cursor > enters > > > while still pressed, wouldn't the state be inaccurate? > > > > 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. > > 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. 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? https://codereview.chromium.org/2054193002/diff/120001/content/browser/androi... File content/browser/android/content_view_core_impl.h (right): https://codereview.chromium.org/2054193002/diff/120001/content/browser/androi... content/browser/android/content_view_core_impl.h:483: int button_state_; On 2016/10/21 19:19:08, mustaq wrote: > On 2016/10/20 23:26:29, aelias wrote: > > FWIW, this state doesn't really belong here, although I understand it's needed > > to work around a bug... I'm mulling over if there's any way it could be > > avoided. Could you rebase this patch so that I can apply it locally and > observe > > the buggy early-pressed MouseMoves? > > The latest patch is after a rebase. I have also added enter/leave plumbing > there, checked only on M. Still couldn't locate an N device here :( Discovered today that my last patch was unusable for unhandled hover-moves. Fixed.
https://codereview.chromium.org/2054193002/diff/120001/content/browser/androi... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/2054193002/diff/120001/content/browser/androi... content/browser/android/content_view_core_impl.cc:1024: // Ignore button state in MouseMove events: we have seen cases that a On 2016/10/26 at 17:45:09, mustaq wrote: > [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? 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?
On 2016/10/27 03:09:37, aelias wrote: > https://codereview.chromium.org/2054193002/diff/120001/content/browser/androi... > File content/browser/android/content_view_core_impl.cc (right): > > https://codereview.chromium.org/2054193002/diff/120001/content/browser/androi... > content/browser/android/content_view_core_impl.cc:1024: // Ignore button state > in MouseMove events: we have seen cases that a > On 2016/10/26 at 17:45:09, mustaq wrote: > > [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? > > 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? Unfortunately not. Pointer index is related to pointing devices (mouse, finger#1, 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.
https://codereview.chromium.org/2054193002/diff/120001/content/browser/androi... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/2054193002/diff/120001/content/browser/androi... content/browser/android/content_view_core_impl.cc:1024: // Ignore button state in MouseMove events: we have seen cases that a > Unfortunately not. Pointer index is related to pointing devices (mouse, finger#1, 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.
On 2016/10/27 18:27:57, aelias wrote: > https://codereview.chromium.org/2054193002/diff/120001/content/browser/androi... > File content/browser/android/content_view_core_impl.cc (right): > > https://codereview.chromium.org/2054193002/diff/120001/content/browser/androi... > content/browser/android/content_view_core_impl.cc:1024: // Ignore button state > in MouseMove events: we have seen cases that a > > Unfortunately not. Pointer index is related to pointing devices (mouse, > finger#1, > 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. Looks like getActionButton() is defined only for a separate class of (deprecated) 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?
https://codereview.chromium.org/2054193002/diff/120001/content/browser/androi... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/2054193002/diff/120001/content/browser/androi... content/browser/android/content_view_core_impl.cc:1024: // Ignore button state in MouseMove events: we have seen cases that a > Looks like getActionButton() is defined only for a separate class of (deprecated) 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).
On 2016/10/27 23:36:48, aelias wrote: > https://codereview.chromium.org/2054193002/diff/120001/content/browser/androi... > File content/browser/android/content_view_core_impl.cc (right): > > https://codereview.chromium.org/2054193002/diff/120001/content/browser/androi... > content/browser/android/content_view_core_impl.cc:1024: // Ignore button state > in MouseMove events: we have seen cases that a > > Looks like getActionButton() is defined only for a separate class of > (deprecated) > 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). I agree with all your points (and C is new to me). I was trying to add ACTION_BUTTON_* events on top of existing plumbing but you are suggesting to have them /replace/ ACTION_DOWN & ACTION_UP. This looks reasonable. - Current ContentViewCore doesn't even receive ACTION_BUTTON_* events. I am not familiar with Android code upstream, any clue where they are getting dropped? - If we want to file a bug to the Android team to "upgrade" MotionEvent API to cover our use case of changed buttons, bringing back ACTION_BUTTON_* events doesn't seem to be the best solution. Instead, just making getActionButton() available for ACTION_{DOWN,UP} (and all ACTION_*_{DOWN,UP}) should be enough. Just a thought.
The CQ bit was checked by mustaq@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
ptal: - Moved the button state to render_widget_host_view_android. - Android codesearch didn't suggest anything about where action_button* events are getting dropped.
OK, I looked into it a bit myself then. I'm not sure why you claimed ACTION_BUTTON_PRESS/RELEASE/getAction() are deprecated because not only are they not deprecated, they are new in API level 23 (Android M). Maybe you were looking at the docs with an older API level selected and misinterpreted the grayed-out "too new to use" coloring as deprecation? They were added in https://android.googlesource.com/platform/frameworks/base/+/5bd69e6e6164c59a0... (I found that via https://android.googlesource.com/platform/frameworks/base/+blame/master/core/... ) Were you testing on M or above? Maybe you weren't seeing them because your test device was on L or below?
On 2016/11/08 01:22:25, aelias wrote: > OK, I looked into it a bit myself then. I'm not sure why you claimed > ACTION_BUTTON_PRESS/RELEASE/getAction() are deprecated because not only are they > not deprecated, they are new in API level 23 (Android M). Maybe you were > looking at the docs with an older API level selected and misinterpreted the > grayed-out "too new to use" coloring as deprecation? They were added in > https://android.googlesource.com/platform/frameworks/base/+/5bd69e6e6164c59a0... > (I found that via > https://android.googlesource.com/platform/frameworks/base/+blame/master/core/... > ) > > Were you testing on M or above? Maybe you weren't seeing them because your test > device was on L or below? Yes, they appeared grayed out when I followed the spec link (developer.android.com/reference/android/view/MotionEvent.html) from Google search. My hidden left panel had API level set to 21 for whatever reason (I might have set it too), and the setting is "sticky" (thru cookies I believe)! My test device was M, so not sure why I didn't see ACTION_BUTTON_* events. Looking into int again.
ptal: this now works w/o any stored state, wohoo. Two caveats though: 1. The compiler now complains about the use of getActionButton(). I suppressed it following this doc, still not sure if the bots will be happy about it: https://chromium.googlesource.com/chromium/src/+/master/build/android/docs/li... 2. This will probably throw an exception on a pre-M Android. Any version-specific annotation I can use here?
On 2016/11/08 19:54:32, mustaq wrote: > ptal: this now works w/o any stored state, wohoo. > > Two caveats though: > 1. The compiler now complains about the use of getActionButton(). I suppressed > it following this doc, still not sure if the bots will be happy about it: > https://chromium.googlesource.com/chromium/src/+/master/build/android/docs/li... > > 2. This will probably throw an exception on a pre-M Android. Any > version-specific annotation I can use here? Confirmed that my point #2 above is a non-issue: Android-L devices don't receive ACTION_BUTTON_* events, so the code for getActionButton() is never executed.
On 2016/11/08 21:08:30, mustaq wrote: > On 2016/11/08 19:54:32, mustaq wrote: > > ptal: this now works w/o any stored state, wohoo. > > > > Two caveats though: > > 1. The compiler now complains about the use of getActionButton(). I suppressed > > it following this doc, still not sure if the bots will be happy about it: > > > https://chromium.googlesource.com/chromium/src/+/master/build/android/docs/li... > > > > 2. This will probably throw an exception on a pre-M Android. Any > > version-specific annotation I can use here? > > Confirmed that my point #2 above is a non-issue: Android-L devices don't receive > ACTION_BUTTON_* events, so the code for getActionButton() is never executed. Preserved touch-like behavior in pre-M Android to make clicks work.
https://codereview.chromium.org/2054193002/diff/260001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2054193002/diff/260001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1025: // Pre-Andorid-L mouse button info is incomplete "Mouse button info is incomplete on L and below" https://codereview.chromium.org/2054193002/diff/260001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1036: @SuppressLint("NewApi") I think it would work to do @TargetApi(Build.VERSION_CODES.M) instead? https://codereview.chromium.org/2054193002/diff/260001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1042: mContainerView.removeCallbacks(mFakeMouseMoveRunnable); That reminds me that this thing exists... could you add a "TODO(mustaq): Delete this, see http://crbug.com/492738" next to uses of it in this file? https://codereview.chromium.org/2054193002/diff/260001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1043: if (mNativeContentViewCore != 0) { Please do "if (mNativeContentViewCore == 0) return false;" in order to avoid the indentation, and also because returning false seems more correct than returning true. https://codereview.chromium.org/2054193002/diff/260001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1054: // The method getActionButton() is defined only for What happens if you call getActionButton() on one of the other event types? Does it crash/assert/log? If it doesn't have any undesirable side effects and just returns 0 anyway, then please delete the unnecessary if statement below. https://codereview.chromium.org/2054193002/diff/260001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1627: // Work around Android bug where the x, y coordinates of a hover exit This if statement is now no-op given your other early-return below. https://codereview.chromium.org/2054193002/diff/260001/ui/events/gesture_dete... File ui/events/gesture_detection/gesture_provider.cc (right): https://codereview.chromium.org/2054193002/diff/260001/ui/events/gesture_dete... ui/events/gesture_detection/gesture_provider.cc:31: #define CASE_ACTION_TO_STRING(action) \ I'm not a big fan of clever macro magic unless there is a really good reason, please just write it how it was before.
https://codereview.chromium.org/2054193002/diff/260001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2054193002/diff/260001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1025: // Pre-Andorid-L mouse button info is incomplete On 2016/11/09 02:43:14, aelias wrote: > "Mouse button info is incomplete on L and below" Done. https://codereview.chromium.org/2054193002/diff/260001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1036: @SuppressLint("NewApi") On 2016/11/09 02:43:14, aelias wrote: > I think it would work to do @TargetApi(Build.VERSION_CODES.M) instead? Done, thanks. https://codereview.chromium.org/2054193002/diff/260001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1042: mContainerView.removeCallbacks(mFakeMouseMoveRunnable); On 2016/11/09 02:43:14, aelias wrote: > That reminds me that this thing exists... could you add a "TODO(mustaq): Delete > this, see http://crbug.com/492738%22 next to uses of it in this file? Commented the main usage instead. Does it look enough? https://codereview.chromium.org/2054193002/diff/260001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1043: if (mNativeContentViewCore != 0) { On 2016/11/09 02:43:14, aelias wrote: > Please do "if (mNativeContentViewCore == 0) return false;" in order to avoid the > indentation, and also because returning false seems more correct than returning > true. Done. https://codereview.chromium.org/2054193002/diff/260001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1043: if (mNativeContentViewCore != 0) { On 2016/11/09 02:43:14, aelias wrote: > Please do "if (mNativeContentViewCore == 0) return false;" in order to avoid the > indentation, and also because returning false seems more correct than returning > true. Done. https://codereview.chromium.org/2054193002/diff/260001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1054: // The method getActionButton() is defined only for On 2016/11/09 02:43:14, aelias wrote: > What happens if you call getActionButton() on one of the other event types? > Does it crash/assert/log? If it doesn't have any undesirable side effects and > just returns 0 anyway, then please delete the unnecessary if statement below. Assuming it neither throws nor emits any logs, even if we found the return value to be 0 in our test cases, there is no guarantee of it across all devices or in future OSes. Since MouseEvent.button (=action button value) is crucial for web compat, I would prefer avoiding the unspeced behavior here. Wdyt? https://codereview.chromium.org/2054193002/diff/260001/ui/events/gesture_dete... File ui/events/gesture_detection/gesture_provider.cc (right): https://codereview.chromium.org/2054193002/diff/260001/ui/events/gesture_dete... ui/events/gesture_detection/gesture_provider.cc:31: #define CASE_ACTION_TO_STRING(action) \ On 2016/11/09 02:43:14, aelias wrote: > I'm not a big fan of clever macro magic unless there is a really good reason, > please just write it how it was before. Done.
The CQ bit was checked by mustaq@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2054193002/diff/260001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2054193002/diff/260001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1054: // The method getActionButton() is defined only for On 2016/11/09 at 17:04:32, mustaq wrote: > On 2016/11/09 02:43:14, aelias wrote: > > What happens if you call getActionButton() on one of the other event types? > > Does it crash/assert/log? If it doesn't have any undesirable side effects and > > just returns 0 anyway, then please delete the unnecessary if statement below. > > Assuming it neither throws nor emits any logs, even if we found the return value to be 0 in our test cases, there is no guarantee of it across all devices or in future OSes. Since MouseEvent.button (=action button value) is crucial for web compat, I would prefer avoiding the unspeced behavior here. Wdyt? That seems excessively paranoid. Almost no Android API is specced or tested, therefore you could make a similar argument about any Android API whatsoever and justify an infinite amount of unnecessary filtering cruft that way. Also, I have no reason to believe OEMs or future Android releases will touch this and would be fine to triage a bug arising from this if they did (it would likely come along with other interesting mouse changes we'd want to pay attention to). A common cause of hairy code complexity is fear -- weird branches accumulate because people are afraid of breaking a hypothetical scenario. I'm OK taking a low risk of an unknown bug to keep the code healthy.
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
https://codereview.chromium.org/2054193002/diff/260001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2054193002/diff/260001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1054: // The method getActionButton() is defined only for On 2016/11/09 18:57:03, aelias wrote: > On 2016/11/09 at 17:04:32, mustaq wrote: > > On 2016/11/09 02:43:14, aelias wrote: > > > What happens if you call getActionButton() on one of the other event types? > > > Does it crash/assert/log? If it doesn't have any undesirable side effects > and > > > just returns 0 anyway, then please delete the unnecessary if statement > below. > > > > Assuming it neither throws nor emits any logs, even if we found the return > value to be 0 in our test cases, there is no guarantee of it across all devices > or in future OSes. Since MouseEvent.button (=action button value) is crucial for > web compat, I would prefer avoiding the unspeced behavior here. Wdyt? > > That seems excessively paranoid. Almost no Android API is specced or tested, > therefore you could make a similar argument about any Android API whatsoever and > justify an infinite amount of unnecessary filtering cruft that way. Also, I > have no reason to believe OEMs or future Android releases will touch this and > would be fine to triage a bug arising from this if they did (it would likely > come along with other interesting mouse changes we'd want to pay attention to). > > A common cause of hairy code complexity is fear -- weird branches accumulate > because people are afraid of breaking a hypothetical scenario. I'm OK taking a > low risk of an unknown bug to keep the code healthy. Just tested on an N7/M, even with chorded button presses, and it just works. So let's go for the simplest code.
lgtm modulo comment https://codereview.chromium.org/2054193002/diff/260001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2054193002/diff/260001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1627: // Work around Android bug where the x, y coordinates of a hover exit On 2016/11/09 at 02:43:14, aelias wrote: > This if statement is now no-op given your other early-return below. Looks like you missed my previous comment. Please delete this if statement before you land.
Also fixed a constructor test. https://codereview.chromium.org/2054193002/diff/260001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2054193002/diff/260001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1627: // Work around Android bug where the x, y coordinates of a hover exit On 2016/11/09 20:08:51, aelias wrote: > On 2016/11/09 at 02:43:14, aelias wrote: > > This if statement is now no-op given your other early-return below. > > Looks like you missed my previous comment. Please delete this if statement > before you land. Oops, done.
mustaq@chromium.org changed reviewers: + dtrainor@chromium.org, skyostil@chromium.org
Need approval. dtrainor@chromium.org: Please review changes in blimp/client skyostil@chromium.org: Please review changes in content/browser/android
The CQ bit was checked by mustaq@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Android mouse events shouldn't appear as TouchEvents Android low-level mouse events in Chrome used to follow the "default" MotionEvent path which is designed for touches. In particular in Blink, mouse events appeared as PlatformTouchEvents which had some bad side-effects: A. TouchEvents were fired for mouse. B. Mouse events were suppressed through prevent-defaulting touch events. C. Mouse event suppression thru canceled touches apply only to non-hovering mouse. This CL fixes the mouse issues by routing Android mouse events through a dedicated Web/PlatformMouseEvent path. The stylus properties are also plumbed, to make Android stylus support easier (follow-up CL). BUG=468806,587550 ========== to ========== Android mouse events shouldn't appear as TouchEvents Android low-level mouse events in Chrome used to follow the "default" MotionEvent path which is designed for touches. In particular in Blink, mouse events appeared as PlatformTouchEvents which had some bad side-effects: A. TouchEvents were fired for mouse. B. Mouse events were suppressed through prevent-defaulting touch events. C. Mouse event suppression thru canceled touches apply only to non-hovering mouse. This CL fixes the mouse issues by routing Android mouse events through a dedicated Web/PlatformMouseEvent path. The stylus properties are also plumbed, to make Android stylus support easier (follow-up CL). Design doc: https://docs.google.com/document/d/1mpBR7J7kgTXvp0QACVjhxtwNJ7bgGoTMmxfxN2dup... BUG=468806,587550 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
blimp/client lgtm!
lgtm
content/browser/android/ lgtm.
The CQ bit was checked by mustaq@chromium.org
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/2054193002/#ps320001 (title: "Fixed a test, etc.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Android mouse events shouldn't appear as TouchEvents Android low-level mouse events in Chrome used to follow the "default" MotionEvent path which is designed for touches. In particular in Blink, mouse events appeared as PlatformTouchEvents which had some bad side-effects: A. TouchEvents were fired for mouse. B. Mouse events were suppressed through prevent-defaulting touch events. C. Mouse event suppression thru canceled touches apply only to non-hovering mouse. This CL fixes the mouse issues by routing Android mouse events through a dedicated Web/PlatformMouseEvent path. The stylus properties are also plumbed, to make Android stylus support easier (follow-up CL). Design doc: https://docs.google.com/document/d/1mpBR7J7kgTXvp0QACVjhxtwNJ7bgGoTMmxfxN2dup... BUG=468806,587550 ========== to ========== Android mouse events shouldn't appear as TouchEvents Android low-level mouse events in Chrome used to follow the "default" MotionEvent path which is designed for touches. In particular in Blink, mouse events appeared as PlatformTouchEvents which had some bad side-effects: A. TouchEvents were fired for mouse. B. Mouse events were suppressed through prevent-defaulting touch events. C. Mouse event suppression thru canceled touches apply only to non-hovering mouse. This CL fixes the mouse issues by routing Android mouse events through a dedicated Web/PlatformMouseEvent path. The stylus properties are also plumbed, to make Android stylus support easier (follow-up CL). Design doc: https://docs.google.com/document/d/1mpBR7J7kgTXvp0QACVjhxtwNJ7bgGoTMmxfxN2dup... BUG=468806,587550 ==========
Message was sent while issue was closed.
Committed patchset #16 (id:320001)
Message was sent while issue was closed.
Description was changed from ========== Android mouse events shouldn't appear as TouchEvents Android low-level mouse events in Chrome used to follow the "default" MotionEvent path which is designed for touches. In particular in Blink, mouse events appeared as PlatformTouchEvents which had some bad side-effects: A. TouchEvents were fired for mouse. B. Mouse events were suppressed through prevent-defaulting touch events. C. Mouse event suppression thru canceled touches apply only to non-hovering mouse. This CL fixes the mouse issues by routing Android mouse events through a dedicated Web/PlatformMouseEvent path. The stylus properties are also plumbed, to make Android stylus support easier (follow-up CL). Design doc: https://docs.google.com/document/d/1mpBR7J7kgTXvp0QACVjhxtwNJ7bgGoTMmxfxN2dup... BUG=468806,587550 ========== to ========== Android mouse events shouldn't appear as TouchEvents Android low-level mouse events in Chrome used to follow the "default" MotionEvent path which is designed for touches. In particular in Blink, mouse events appeared as PlatformTouchEvents which had some bad side-effects: A. TouchEvents were fired for mouse. B. Mouse events were suppressed through prevent-defaulting touch events. C. Mouse event suppression thru canceled touches apply only to non-hovering mouse. This CL fixes the mouse issues by routing Android mouse events through a dedicated Web/PlatformMouseEvent path. The stylus properties are also plumbed, to make Android stylus support easier (follow-up CL). Design doc: https://docs.google.com/document/d/1mpBR7J7kgTXvp0QACVjhxtwNJ7bgGoTMmxfxN2dup... BUG=468806,587550 Committed: https://crrev.com/5b679e43949bc52452422b453e81fb8e48d0594f Cr-Commit-Position: refs/heads/master@{#431571} ==========
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/5b679e43949bc52452422b453e81fb8e48d0594f Cr-Commit-Position: refs/heads/master@{#431571} |
