Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
289373009:
Support tap_count in ui::GestureProvider. (Closed)
|
Created:
6 years, 7 months ago by tdresser Modified:
6 years, 7 months ago CC:
chromium-reviews, ben+aura_chromium.org, tdresser+watch_chromium.org, jam, sadrul, darin-cc_chromium.org, kalyank, jdduke+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionSupport tap_count in ui::GestureProvider.
BUG=357270
TEST=GestureProviderTest.AuraTripleTap
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=272310
Patch Set 1 #Patch Set 2 : Small cleanup. #
Total comments: 8
Patch Set 3 : tap_count wraps around after triple tap. #Patch Set 4 : Move to GestureProviderAura. #
Total comments: 5
Patch Set 5 : Address jdduke comments. #
Total comments: 2
Patch Set 6 : Use touch release locations instead of touch press locations #
Total comments: 7
Patch Set 7 : Address Jared's comments. #
Total comments: 2
Patch Set 8 : Use DCHECK_EQ #
Messages
Total messages: 20 (0 generated)
PTAL. This is a little ugly - crbug.com/357265 discusses cleaning things up. I don't really like having to pass around an unused |tap_count| on Android, but it feels cleaner than the other approaches I've thought about.
On 2014/05/20 19:32:02, tdresser wrote: > PTAL. > > This is a little ugly - crbug.com/357265 discusses cleaning things up. > > I don't really like having to pass around an unused |tap_count| on Android, but > it feels cleaner than the other approaches I've thought about. Can you point me to some references on how the tap count is used, either in Aura UI or in blink?
On 2014/05/20 19:37:13, jdduke wrote: > On 2014/05/20 19:32:02, tdresser wrote: > > PTAL. > > > > This is a little ugly - crbug.com/357265 discusses cleaning things up. > > > > I don't really like having to pass around an unused |tap_count| on Android, > but > > it feels cleaner than the other approaches I've thought about. > > Can you point me to some references on how the tap count is used, either in Aura > UI or in blink? Double tap on window titlebar to maximize/minimize: https://code.google.com/p/chromium/codesearch#chromium/src/ash/wm/panels/pane... Double tap on folder in bookmark organization view to expand it: https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/controls/... Select text in Aura: https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/we... Text selection in Aura may use triple tap for paragraph selection eventually - there has been some debate there. We could switch to only supporting up to double tap, but it isn't much extra work for triple tap, and there's a reasonable chance we'll start using it fairly shortly. Based on a little grepping, I'm not convinced we use the tap count anywhere in blink, in which case we should probably stop sending it...
On 2014/05/20 19:57:40, tdresser wrote: > On 2014/05/20 19:37:13, jdduke wrote: > > On 2014/05/20 19:32:02, tdresser wrote: > > > PTAL. > > > > > > This is a little ugly - crbug.com/357265 discusses cleaning things up. > > > > > > I don't really like having to pass around an unused |tap_count| on Android, > > but > > > it feels cleaner than the other approaches I've thought about. > > > > Can you point me to some references on how the tap count is used, either in > Aura > > UI or in blink? > > Double tap on window titlebar to maximize/minimize: > https://code.google.com/p/chromium/codesearch#chromium/src/ash/wm/panels/pane... > > Double tap on folder in bookmark organization view to expand it: > https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/controls/... > > Select text in Aura: > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/we... > > Text selection in Aura may use triple tap for paragraph selection eventually - > there has been some debate there. We could switch to only supporting up to > double tap, but it isn't much extra work for triple tap, and there's a > reasonable chance we'll start using it fairly shortly. > > Based on a little grepping, I'm not convinced we use the tap count anywhere in > blink, in which case we should probably stop sending it... Thanks. Yeah, if it's not used in blink, I'm thinking we could rip out the tap count and have the Aura GestureProvider implementation simply listen to the event stream and update tap gestures appropriately.
On 2014/05/20 20:01:36, jdduke wrote: > On 2014/05/20 19:57:40, tdresser wrote: > > On 2014/05/20 19:37:13, jdduke wrote: > > > On 2014/05/20 19:32:02, tdresser wrote: > > > > PTAL. > > > > > > > > This is a little ugly - crbug.com/357265 discusses cleaning things up. > > > > > > > > I don't really like having to pass around an unused |tap_count| on > Android, > > > but > > > > it feels cleaner than the other approaches I've thought about. > > > > > > Can you point me to some references on how the tap count is used, either in > > Aura > > > UI or in blink? > > > > Double tap on window titlebar to maximize/minimize: > > > https://code.google.com/p/chromium/codesearch#chromium/src/ash/wm/panels/pane... > > > > Double tap on folder in bookmark organization view to expand it: > > > https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/controls/... > > > > Select text in Aura: > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/we... > > > > Text selection in Aura may use triple tap for paragraph selection eventually - > > there has been some debate there. We could switch to only supporting up to > > double tap, but it isn't much extra work for triple tap, and there's a > > reasonable chance we'll start using it fairly shortly. > > > > Based on a little grepping, I'm not convinced we use the tap count anywhere in > > blink, in which case we should probably stop sending it... > > Thanks. Yeah, if it's not used in blink, I'm thinking we could rip out the tap > count and have the Aura GestureProvider implementation simply listen to the > event stream and update tap gestures appropriately. Took a closer look, and did find a place we're using tap_count in blink. https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... A double tap will fire the dblclick event. Actually, here's an example of why we do in fact need triple tap. A triple tap will fire the click event handler with an event with |detail| = 3. Here's the fun part, if you repeatedly click with the mouse, you'll get |detail| set to: 1,2,3,1,2,3,1,2,3 whereas with repeated taps, you'll get: 1,2,3,3,3,3,3,3,3
On 2014/05/20 20:40:27, tdresser wrote: > On 2014/05/20 20:01:36, jdduke wrote: > > On 2014/05/20 19:57:40, tdresser wrote: > > > On 2014/05/20 19:37:13, jdduke wrote: > > > > On 2014/05/20 19:32:02, tdresser wrote: > > > > > PTAL. > > > > > > > > > > This is a little ugly - crbug.com/357265 discusses cleaning things up. > > > > > > > > > > I don't really like having to pass around an unused |tap_count| on > > Android, > > > > but > > > > > it feels cleaner than the other approaches I've thought about. > > > > > > > > Can you point me to some references on how the tap count is used, either > in > > > Aura > > > > UI or in blink? > > > > > > Double tap on window titlebar to maximize/minimize: > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/ash/wm/panels/pane... > > > > > > Double tap on folder in bookmark organization view to expand it: > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/controls/... > > > > > > Select text in Aura: > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/we... > > > > > > Text selection in Aura may use triple tap for paragraph selection eventually > - > > > there has been some debate there. We could switch to only supporting up to > > > double tap, but it isn't much extra work for triple tap, and there's a > > > reasonable chance we'll start using it fairly shortly. > > > > > > Based on a little grepping, I'm not convinced we use the tap count anywhere > in > > > blink, in which case we should probably stop sending it... > > > > Thanks. Yeah, if it's not used in blink, I'm thinking we could rip out the > tap > > count and have the Aura GestureProvider implementation simply listen to the > > event stream and update tap gestures appropriately. > > Took a closer look, and did find a place we're using tap_count in blink. > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > A double tap will fire the dblclick event. > > Actually, here's an example of why we do in fact need triple tap. > A triple tap will fire the click event handler with an event with |detail| = 3. > > Here's the fun part, if you repeatedly click with the mouse, you'll get |detail| > set to: > 1,2,3,1,2,3,1,2,3 > whereas with repeated taps, you'll get: > 1,2,3,3,3,3,3,3,3 jdduke, do you think it makes sense to pass the require configuration parameters to the AuraGestureProvider, and also keep track of the require MotionEvents there? I'm inclined to leave this in the GestureDetector for now, and clean it up for crbug.com/357265.
If we plan to use a unified tap count model, it probably makes sense to have this in GestureDetector as you say. Otherwise, the GestureProviderAura could simply examine the ui::GestureConfiguration() constants directly, probably be a 10 line change to store the previous tap and check with a new tap (or cancel with tap cancel). I don't see in this patch where we limit the tap count on Android? https://codereview.chromium.org/289373009/diff/20001/ui/events/gesture_detect... File ui/events/gesture_detection/gesture_detector.cc (right): https://codereview.chromium.org/289373009/diff/20001/ui/events/gesture_detect... ui/events/gesture_detection/gesture_detector.cc:67: const MotionEvent& e, int tap_count) { Maybe tap_repeat_count? Hmm, but that's slightly ambiguous... https://codereview.chromium.org/289373009/diff/20001/ui/events/gesture_detect... ui/events/gesture_detection/gesture_detector.cc:432: tap_count_ = std::min(tap_count_, 3); I'd prefer the GestureDetector not have any awareness of this restriction. Could we push this logic downstream? https://codereview.chromium.org/289373009/diff/20001/ui/events/gesture_detect... ui/events/gesture_detection/gesture_detector.cc:441: double_tap_listener_->OnSingleTapConfirmed(ev, 1); Might as well just use |tap_count_| here instead of 1 for consistency. https://codereview.chromium.org/289373009/diff/20001/ui/events/gesture_detect... ui/events/gesture_detection/gesture_detector.cc:545: still_down_ = false; We should probably reset tap_count_ both here and in |CancelTaps()|. Actually, I've been meaning to refactor these two methods, it appears we can implement Cancel() in terms of CancelTaps(), so we'd have: Cancel() { CancelTaps(); velocity_tracker_.Clear(); still_down_ = false; }
https://codereview.chromium.org/289373009/diff/20001/ui/events/gesture_detect... File ui/events/gesture_detection/gesture_detector.cc (right): https://codereview.chromium.org/289373009/diff/20001/ui/events/gesture_detect... ui/events/gesture_detection/gesture_detector.cc:67: const MotionEvent& e, int tap_count) { On 2014/05/21 15:07:14, jdduke wrote: > Maybe tap_repeat_count? Hmm, but that's slightly ambiguous... We use tap_count throughout (GestureEventDetails, PlatformGestureEvent, WebInputEvent...) https://codereview.chromium.org/289373009/diff/20001/ui/events/gesture_detect... ui/events/gesture_detection/gesture_detector.cc:432: tap_count_ = std::min(tap_count_, 3); On 2014/05/21 15:07:14, jdduke wrote: > I'd prefer the GestureDetector not have any awareness of this restriction. > Could we push this logic downstream? Done. https://codereview.chromium.org/289373009/diff/20001/ui/events/gesture_detect... ui/events/gesture_detection/gesture_detector.cc:441: double_tap_listener_->OnSingleTapConfirmed(ev, 1); On 2014/05/21 15:07:14, jdduke wrote: > Might as well just use |tap_count_| here instead of 1 for consistency. Done. https://codereview.chromium.org/289373009/diff/20001/ui/events/gesture_detect... ui/events/gesture_detection/gesture_detector.cc:545: still_down_ = false; On 2014/05/21 15:07:14, jdduke wrote: > We should probably reset tap_count_ both here and in |CancelTaps()|. Actually, > I've been meaning to refactor these two methods, it appears we can implement > Cancel() in terms of CancelTaps(), so we'd have: > > Cancel() { > CancelTaps(); > velocity_tracker_.Clear(); > still_down_ = false; > } Done. https://codereview.chromium.org/289373009/diff/60001/ui/events/gestures/gestu... File ui/events/gestures/gesture_provider_aura.cc (right): https://codereview.chromium.org/289373009/diff/60001/ui/events/gestures/gestu... ui/events/gestures/gesture_provider_aura.cc:63: scoped_ptr<GestureEventDetails> tap_details; Do you have any other ideas on how to avoid constructing a new GestureEventDetails when it isn't necessary? Alternatively, do you think it would be better to just always construct a new GestureEventDetails? This approach feels a little confusing.
https://codereview.chromium.org/289373009/diff/60001/ui/events/gestures/gestu... File ui/events/gestures/gesture_provider_aura.cc (right): https://codereview.chromium.org/289373009/diff/60001/ui/events/gestures/gestu... ui/events/gestures/gesture_provider_aura.cc:63: scoped_ptr<GestureEventDetails> tap_details; On 2014/05/21 16:39:16, tdresser wrote: > Do you have any other ideas on how to avoid constructing a new > GestureEventDetails when it isn't necessary? > > Alternatively, do you think it would be better to just always construct a new > GestureEventDetails? This approach feels a little confusing. Hmm, yeah, the details object is pretty small, might as well copy I guess? https://codereview.chromium.org/289373009/diff/60001/ui/events/gestures/gestu... ui/events/gestures/gesture_provider_aura.cc:96: const base::TimeTicks previous_tap_time, Hmm, could we just store the previous_tap_event_? If we get another tap (without an intervening tap cancel) that's sufficiently close within the appropriate time window, shouldn't that be enough data to make the determination? In that case, we could store a base::Optional<GestureEventDetails> previous_tap_, and clear it when necessary? That would also store the accumulated tap count for us.
https://codereview.chromium.org/289373009/diff/60001/ui/events/gestures/gestu... File ui/events/gestures/gesture_provider_aura.cc (right): https://codereview.chromium.org/289373009/diff/60001/ui/events/gestures/gestu... ui/events/gestures/gesture_provider_aura.cc:63: scoped_ptr<GestureEventDetails> tap_details; On 2014/05/21 17:21:48, jdduke wrote: > On 2014/05/21 16:39:16, tdresser wrote: > > Do you have any other ideas on how to avoid constructing a new > > GestureEventDetails when it isn't necessary? > > > > Alternatively, do you think it would be better to just always construct a new > > GestureEventDetails? This approach feels a little confusing. > > Hmm, yeah, the details object is pretty small, might as well copy I guess? Done. https://codereview.chromium.org/289373009/diff/60001/ui/events/gestures/gestu... ui/events/gestures/gesture_provider_aura.cc:96: const base::TimeTicks previous_tap_time, On 2014/05/21 17:21:48, jdduke wrote: > Hmm, could we just store the previous_tap_event_? If we get another tap (without > an intervening tap cancel) that's sufficiently close within the appropriate time > window, shouldn't that be enough data to make the determination? > > In that case, we could store a base::Optional<GestureEventDetails> > previous_tap_, and clear it when necessary? That would also store the > accumulated tap count for us. Done, with a scoped_ptr.
https://codereview.chromium.org/289373009/diff/80001/ui/events/gestures/gestu... File ui/events/gestures/gesture_provider_aura.cc (right): https://codereview.chromium.org/289373009/diff/80001/ui/events/gestures/gestu... ui/events/gestures/gesture_provider_aura.cc:66: IsConsideredDoubleTap( The reason I suggested just caching the tap details was to prevent cloning previous/current down events. Why do we need the down events if we know the previous tap position, the previous tap time, and that no intervening tap cancel has occurred?
https://codereview.chromium.org/289373009/diff/80001/ui/events/gestures/gestu... File ui/events/gestures/gesture_provider_aura.cc (right): https://codereview.chromium.org/289373009/diff/80001/ui/events/gestures/gestu... ui/events/gestures/gesture_provider_aura.cc:66: IsConsideredDoubleTap( On 2014/05/21 19:00:30, jdduke wrote: > The reason I suggested just caching the tap details was to prevent cloning > previous/current down events. Why do we need the down events if we know the > previous tap position, the previous tap time, and that no intervening tap cancel > has occurred? I hesitated because it does change the behavior (We previously looked at down locations, not up). I think you're right that it's fine though. Done.
lgtm with nits, thanks! https://codereview.chromium.org/289373009/diff/90001/ui/events/gesture_detect... File ui/events/gesture_detection/gesture_detector.cc (right): https://codereview.chromium.org/289373009/diff/90001/ui/events/gesture_detect... ui/events/gesture_detection/gesture_detector.cc:522: CancelTaps(); Thanks! https://codereview.chromium.org/289373009/diff/90001/ui/events/gestures/gestu... File ui/events/gestures/gesture_provider_aura.cc (right): https://codereview.chromium.org/289373009/diff/90001/ui/events/gestures/gestu... ui/events/gestures/gesture_provider_aura.cc:58: int tap_count = 1; Nit: Move tap_count into the if case? https://codereview.chromium.org/289373009/diff/90001/ui/events/gestures/gestu... ui/events/gestures/gesture_provider_aura.cc:62: details = GestureEventDetails(ET_GESTURE_TAP, tap_count, 0); Feel free to add a GestureEventDetails::set_tap_count() setter instead of recreating (if you prefer), looks like there's precedent with set_touch_points() and set_bounding_box. https://codereview.chromium.org/289373009/diff/90001/ui/events/gestures/gestu... ui/events/gestures/gesture_provider_aura.cc:63: previous_tap_.reset(new GestureEventData(gesture)); if (!previous_tap_) previous_tap_.reset(new gestureEventData(gesture)); else *previous_tap_ = gesture;
Sadrul, can you take a look at: ui/aura/gestures/gesture_recognizer_unittest.cc ui/events/gesture_event_details.h https://codereview.chromium.org/289373009/diff/90001/ui/events/gestures/gestu... File ui/events/gestures/gesture_provider_aura.cc (right): https://codereview.chromium.org/289373009/diff/90001/ui/events/gestures/gestu... ui/events/gestures/gesture_provider_aura.cc:58: int tap_count = 1; On 2014/05/21 20:02:00, jdduke wrote: > Nit: Move tap_count into the if case? Done. https://codereview.chromium.org/289373009/diff/90001/ui/events/gestures/gestu... ui/events/gestures/gesture_provider_aura.cc:62: details = GestureEventDetails(ET_GESTURE_TAP, tap_count, 0); On 2014/05/21 20:02:00, jdduke wrote: > Feel free to add a GestureEventDetails::set_tap_count() setter instead of > recreating (if you prefer), looks like there's precedent with set_touch_points() > and set_bounding_box. Done. https://codereview.chromium.org/289373009/diff/90001/ui/events/gestures/gestu... ui/events/gestures/gesture_provider_aura.cc:63: previous_tap_.reset(new GestureEventData(gesture)); On 2014/05/21 20:02:00, jdduke wrote: > if (!previous_tap_) > previous_tap_.reset(new gestureEventData(gesture)); > else > *previous_tap_ = gesture; Done.
https://codereview.chromium.org/289373009/diff/120001/ui/events/gesture_event... File ui/events/gesture_event_details.h (right): https://codereview.chromium.org/289373009/diff/120001/ui/events/gesture_event... ui/events/gesture_event_details.h:58: DCHECK(type_ == ET_SCROLL_FLING_START); Super nit, should we make the checks here and in velocity_y DCHECK_EQ?
https://codereview.chromium.org/289373009/diff/120001/ui/events/gesture_event... File ui/events/gesture_event_details.h (right): https://codereview.chromium.org/289373009/diff/120001/ui/events/gesture_event... ui/events/gesture_event_details.h:58: DCHECK(type_ == ET_SCROLL_FLING_START); On 2014/05/22 14:43:33, jdduke wrote: > Super nit, should we make the checks here and in velocity_y DCHECK_EQ? Done.
LGTM
The CQ bit was checked by tdresser@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tdresser@chromium.org/289373009/140001
Message was sent while issue was closed.
Change committed as 272310 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
