Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2943383002:
Android double/triple click (Closed)
|
Created:
3 years, 6 months ago by amaralp Modified:
3 years, 6 months ago Reviewers:
aelias_OOO_until_Jul13 CC:
chromium-reviews, darin-cc_chromium.org, jam, nona+watch_chromium.org, shuchen+watch_chromium.org, James Su, yusukes+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAndroid double/triple click
Unlike most OSes Android's mouse click events don't come with the click count.
This CL keeps track of the click count for Android mouse clicks so that double
and triple clicks can work. From experimentation on Linux and online research
I found that the click count depends both on the time between the clicks and
the distance between them. Windows has a max time delay of 0.5 seconds which is
what I've used in this CL. I couldn't find a max distance value but from my
experimentation 5 DIPs seems reasonable.
BUG=666072
Review-Url: https://codereview.chromium.org/2943383002
Cr-Commit-Position: refs/heads/master@{#481405}
Committed: https://chromium.googlesource.com/chromium/src/+/30fe79726764ec6ce93001069bee890ceb529a20
Patch Set 1 #Patch Set 2 : Use point and convert to DIP #Patch Set 3 : Removed accidental function #Patch Set 4 : fixing DIP #
Total comments: 12
Patch Set 5 : Addressing aelias's comments #Patch Set 6 : move incrememnt outside if #
Total comments: 8
Patch Set 7 : Addressing aelias's comments #
Total comments: 1
Patch Set 8 : Fixing mouse up scenario #
Total comments: 1
Patch Set 9 : removing unnecessary comment #
Messages
Total messages: 48 (38 generated)
The CQ bit was checked by amaralp@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: This issue passed the CQ dry run.
The CQ bit was checked by amaralp@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: This issue passed the CQ dry run.
The CQ bit was checked by amaralp@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 checked by amaralp@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: This issue passed the CQ dry run.
Description was changed from ========== Android double/triple click BUG= ========== to ========== Android double/triple click BUG=666072 ==========
Description was changed from ========== Android double/triple click BUG=666072 ========== to ========== Android double/triple click Unlike most OSes Android's mouse click events don't come with the click count. This CL keeps track of the click count for Android mouse clicks so that double and triple clicks can work. From experimentation on Linux and online research I found that the click count depends both on the time between the clicks and the distance between them. Windows has a time delay of 0.5 seconds which is what I've used in this CL. The BUG=666072 ==========
Description was changed from ========== Android double/triple click Unlike most OSes Android's mouse click events don't come with the click count. This CL keeps track of the click count for Android mouse clicks so that double and triple clicks can work. From experimentation on Linux and online research I found that the click count depends both on the time between the clicks and the distance between them. Windows has a time delay of 0.5 seconds which is what I've used in this CL. The BUG=666072 ========== to ========== Android double/triple click Unlike most OSes Android's mouse click events don't come with the click count. This CL keeps track of the click count for Android mouse clicks so that double and triple clicks can work. From experimentation on Linux and online research I found that the click count depends both on the time between the clicks and the distance between them. Windows has a max time delay of 0.5 seconds which is what I've used in this CL. I couldn't find a max distance value but from my experimentation 5 DIPs seems reasonable. BUG=666072 ==========
amaralp@chromium.org changed reviewers: + aelias@chromium.org
PTAL
https://codereview.chromium.org/2943383002/diff/60001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2943383002/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:105: static const double kMaxClickDelay = 0.5; Would prefer names "kClickCountInterval", "kClickCountRadiusDIP" https://codereview.chromium.org/2943383002/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:1808: if (webMouseEventType == blink::WebInputEvent::kMouseDown) { This also should take into account which button was pressed, for a left-click quickly followed by a right click. https://codereview.chromium.org/2943383002/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:1814: click_count_ = 1; 1-based counting system feels weird. I'd prefer: if ([invalid]) click_count_ = 0; click_count_++; https://codereview.chromium.org/2943383002/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:1825: motion_event.GetButtonState() ? click_count_ : 0 /* click count */, I suggest integrating this condition in the core click_count_ setting logic above instead. You'll need to check button state to distinguish left/right buttons anyway. https://codereview.chromium.org/2943383002/diff/60001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.h (right): https://codereview.chromium.org/2943383002/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.h:431: double prev_mousedown_timestamp_; base::TimeTicks https://codereview.chromium.org/2943383002/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.h:433: int click_count_; = 0;
The CQ bit was checked by amaralp@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: This issue passed the CQ dry run.
The CQ bit was checked by amaralp@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/2943383002/diff/60001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2943383002/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:105: static const double kMaxClickDelay = 0.5; On 2017/06/20 at 18:57:52, aelias wrote: > Would prefer names "kClickCountInterval", "kClickCountRadiusDIP" Done. https://codereview.chromium.org/2943383002/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:1808: if (webMouseEventType == blink::WebInputEvent::kMouseDown) { On 2017/06/20 at 18:57:52, aelias wrote: > This also should take into account which button was pressed, for a left-click quickly followed by a right click. Made only count left-clicks. https://codereview.chromium.org/2943383002/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:1814: click_count_ = 1; On 2017/06/20 at 18:57:52, aelias wrote: > 1-based counting system feels weird. I'd prefer: > > if ([invalid]) > click_count_ = 0; > > click_count_++; Done. https://codereview.chromium.org/2943383002/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:1825: motion_event.GetButtonState() ? click_count_ : 0 /* click count */, On 2017/06/20 at 18:57:52, aelias wrote: > I suggest integrating this condition in the core click_count_ setting logic above instead. You'll need to check button state to distinguish left/right buttons anyway. The problem is that I don't want to set click_count_ to 0 when GetButtonState() == 0 because a mouse hover shouldn't reset the click_count_. Basically this is saying that if no buttons are pressed then we shouldn't be using the click-count. https://codereview.chromium.org/2943383002/diff/60001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.h (right): https://codereview.chromium.org/2943383002/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.h:431: double prev_mousedown_timestamp_; On 2017/06/20 at 18:57:52, aelias wrote: > base::TimeTicks Done. https://codereview.chromium.org/2943383002/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.h:433: int click_count_; On 2017/06/20 at 18:57:52, aelias wrote: > = 0; Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm modulo comments https://codereview.chromium.org/2943383002/diff/100001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2943383002/diff/100001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1813: const float distance = (mousedown_point - prev_mousedown_point_).Length(); Although this doesn't happen on mousemove I still don't really like having a unnecessary sqrt call in an input critical path because I've seen that bubble up in profiles before -- I would prefer "LengthSquared() < kClickCountRadiusDIPSquared)". I might go back and refactor vector2d.h later to have a call LengthLessThan() instead of LengthSquared() to make the code nicer, but for this patch let's go with that. https://codereview.chromium.org/2943383002/diff/100001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1821: // Reset state if left button isn't pressed. "if middle or right button was pressed" https://codereview.chromium.org/2943383002/diff/100001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1824: prev_mousedown_point_ = gfx::Point(); gfx::Point(0, 0) can be a legal offset or close to it, so it's not a principled invalid value. I'd suggest deleting this line as it doesn't do anything, to avoid giving the impression that there might be a bug with double-clicks near the top-left of the screen. https://codereview.chromium.org/2943383002/diff/100001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.h (right): https://codereview.chromium.org/2943383002/diff/100001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.h:433: int click_count_ = 0; left_click_count_
The CQ bit was checked by amaralp@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/2943383002/diff/100001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2943383002/diff/100001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1813: const float distance = (mousedown_point - prev_mousedown_point_).Length(); On 2017/06/21 at 01:13:12, aelias wrote: > Although this doesn't happen on mousemove I still don't really like having a unnecessary sqrt call in an input critical path because I've seen that bubble up in profiles before -- I would prefer "LengthSquared() < kClickCountRadiusDIPSquared)". I might go back and refactor vector2d.h later to have a call LengthLessThan() instead of LengthSquared() to make the code nicer, but for this patch let's go with that. Done. https://codereview.chromium.org/2943383002/diff/100001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1821: // Reset state if left button isn't pressed. On 2017/06/21 at 01:13:12, aelias wrote: > "if middle or right button was pressed" Done. https://codereview.chromium.org/2943383002/diff/100001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1824: prev_mousedown_point_ = gfx::Point(); On 2017/06/21 at 01:13:12, aelias wrote: > gfx::Point(0, 0) can be a legal offset or close to it, so it's not a principled invalid value. I'd suggest deleting this line as it doesn't do anything, to avoid giving the impression that there might be a bug with double-clicks near the top-left of the screen. Done. https://codereview.chromium.org/2943383002/diff/100001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.h (right): https://codereview.chromium.org/2943383002/diff/100001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.h:433: int click_count_ = 0; On 2017/06/21 at 01:13:12, aelias wrote: > left_click_count_ Done.
https://codereview.chromium.org/2943383002/diff/120001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2943383002/diff/120001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1833: motion_event.GetButtonState() ? left_click_count_ : 0 /* click count */, Hmm, I realized click_count needs to be set on MouseUp events as well according to https://bugs.webkit.org/show_bug.cgi?id=49290. I think that doesn't work with this. Also, it's confusing to use left_click_count_ for middle and right buttons now. How about a local variable click_count to distinguish?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by amaralp@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...
On 2017/06/21 at 19:59:01, aelias wrote: > https://codereview.chromium.org/2943383002/diff/120001/content/browser/render... > File content/browser/renderer_host/render_widget_host_view_android.cc (right): > > https://codereview.chromium.org/2943383002/diff/120001/content/browser/render... > content/browser/renderer_host/render_widget_host_view_android.cc:1833: motion_event.GetButtonState() ? left_click_count_ : 0 /* click count */, > Hmm, I realized click_count needs to be set on MouseUp events as well according to https://bugs.webkit.org/show_bug.cgi?id=49290. I think that doesn't work with this. > > Also, it's confusing to use left_click_count_ for middle and right buttons now. How about a local variable click_count to distinguish? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2943383002/diff/140001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2943383002/diff/140001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1823: click_count /* click count */, motion_event.GetPointerId(0), no need for "/* click count */" anymore
The CQ bit was checked by amaralp@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/2943383002/#ps160001 (title: "removing unnecessary comment")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1498094722178990,
"parent_rev": "dd7e6ab9dfc665952edef91877734d711de971ef", "commit_rev":
"30fe79726764ec6ce93001069bee890ceb529a20"}
Message was sent while issue was closed.
Description was changed from ========== Android double/triple click Unlike most OSes Android's mouse click events don't come with the click count. This CL keeps track of the click count for Android mouse clicks so that double and triple clicks can work. From experimentation on Linux and online research I found that the click count depends both on the time between the clicks and the distance between them. Windows has a max time delay of 0.5 seconds which is what I've used in this CL. I couldn't find a max distance value but from my experimentation 5 DIPs seems reasonable. BUG=666072 ========== to ========== Android double/triple click Unlike most OSes Android's mouse click events don't come with the click count. This CL keeps track of the click count for Android mouse clicks so that double and triple clicks can work. From experimentation on Linux and online research I found that the click count depends both on the time between the clicks and the distance between them. Windows has a max time delay of 0.5 seconds which is what I've used in this CL. I couldn't find a max distance value but from my experimentation 5 DIPs seems reasonable. BUG=666072 Review-Url: https://codereview.chromium.org/2943383002 Cr-Commit-Position: refs/heads/master@{#481405} Committed: https://chromium.googlesource.com/chromium/src/+/30fe79726764ec6ce93001069bee... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/chromium/src/+/30fe79726764ec6ce93001069bee... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
