middle mouse button injection code in input_injector_evdev.cc looks wrong |
|||
Issue descriptionhttps://codereview.chromium.org/755883002/diff/120001/ui/events/ozone/evdev/input_injector_evdev.cc added: 32 switch(button) { 33 case EF_LEFT_MOUSE_BUTTON: 34 changed_button = EVDEV_MODIFIER_LEFT_MOUSE_BUTTON; 35 break; 36 case EF_RIGHT_MOUSE_BUTTON: 37 changed_button = EVDEV_MODIFIER_RIGHT_MOUSE_BUTTON; 38 break; 39 case EF_MIDDLE_MOUSE_BUTTON: 40 changed_button = EVDEV_MODIFIER_MIDDLE_MOUSE_BUTTON; 41 default: 42 LOG(WARNING) << "Invalid flag: " << button << " for the button parameter"; 43 return; 44 } The MIDDLE_MOUSE_BUTTON case misses a break and hence falls through to the early return. I suppose this isn't intentional? What are the ramifications of this? What is this injector used for?
,
Feb 2 2018
Oh dear. Yeah, this would prevent sending a middle click when remoting into a Chrome OS device.
,
Feb 2 2018
That does look wrong. Though I'm not exactly sure what, if anything, middle-mouse-button does in Chrome OS. If nothing, that could explain why the bug has gone unnoticed?
,
Feb 2 2018
Doesn't it open a link in a new tab? That's what it does on Windows and Linux.
,
Feb 2 2018
I think you can't use middle-click unless you are using an external mouse on a chromebook, which is probably not very often?
,
Feb 2 2018
If you remote from a device with a three-button mouse to a chromebook, you could middleclick on your local mouse I suppose (?)
,
Feb 2 2018
You could remote in, then middle-click. But then the bug you've found means that nothing would happen :) I've just tried plugging my mouse into my Chromebook. And middle-click does indeed open links in a new tab. And I've just tried remoting in. And indeed, middle-click fails, though the other 2 buttons work as expected! So, thanks and well done for spotting this issue!
,
Feb 2 2018
Since you found the issue in the code, and it's trivial, please could you upload a patch and send it to one of us (or whoever owns this code)?
,
Feb 2 2018
Can do, but can't write a test (so you might want to do that part yourself).
,
Feb 3 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a09247deb0958f075c47c76201a2e47bd74fda01 commit a09247deb0958f075c47c76201a2e47bd74fda01 Author: Nico Weber <thakis@chromium.org> Date: Sat Feb 03 01:42:23 2018 Enable -Wimplicit-fallthrough on (non-Chrome OS) Ozone. Fixes a bug where middle mouse clicks where ignored when remoting into a Chrome OS device. Also found https://chromium-review.googlesource.com/c/chromium/src/+/899585 Bug: 177475 , 808537 Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_vr;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Change-Id: I64e259f1e2d2c817364ee9249e72cb24abc395be Reviewed-on: https://chromium-review.googlesource.com/899984 Commit-Queue: Nico Weber <thakis@chromium.org> Reviewed-by: Michael Spang <spang@chromium.org> Reviewed-by: Lambros Lambrou <lambroslambrou@chromium.org> Cr-Commit-Position: refs/heads/master@{#534234} [modify] https://crrev.com/a09247deb0958f075c47c76201a2e47bd74fda01/build/config/compiler/BUILD.gn [modify] https://crrev.com/a09247deb0958f075c47c76201a2e47bd74fda01/ui/events/ozone/evdev/input_injector_evdev.cc [modify] https://crrev.com/a09247deb0958f075c47c76201a2e47bd74fda01/ui/ozone/demo/surfaceless_gl_renderer.cc
,
Feb 3 2018
Do you want to close this out or keep it open for adding test coverage?
,
Feb 5 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/885eeb25383608a29df6b4dd40cd70cf3e068035 commit 885eeb25383608a29df6b4dd40cd70cf3e068035 Author: Lambros Lambrou <lambroslambrou@chromium.org> Date: Mon Feb 05 18:24:10 2018 Add unittest for middle-mouse-button injection This adds missing unittest coverage for ozone evdev input-injection. Bug: 808537 Change-Id: Iac1bc5544d68b37e45aaab6325b1c370ec926ee8 Reviewed-on: https://chromium-review.googlesource.com/902185 Reviewed-by: Nico Weber <thakis@chromium.org> Reviewed-by: Michael Spang <spang@chromium.org> Commit-Queue: Michael Spang <spang@chromium.org> Commit-Queue: Lambros Lambrou <lambroslambrou@chromium.org> Cr-Commit-Position: refs/heads/master@{#534427} [modify] https://crrev.com/885eeb25383608a29df6b4dd40cd70cf3e068035/ui/events/ozone/evdev/input_injector_evdev_unittest.cc
,
Feb 5 2018
|
|||
►
Sign in to add a comment |
|||
Comment 1 by sadrul@chromium.org
, Feb 2 2018Labels: -OS-Linux OS-Chrome