Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2690573002:
MacViews: Handle three-finger swipe gestures for navigation. (Closed)
DescriptionMacViews: Handle three-finger swipe gestures for navigation.
It's possible to enable three-finger swipes in System Preferences,
and they result in immediate -swipeWithEvent: calls, unlike two-finger
swipes which should be tracked.
Added code to convert them to ET_GESTURE_SWIPE and to handle them
in BrowserView. Cocoa code handled these events in
BrowserWindowController in a similar fashion.
BUG=691558
Review-Url: https://codereview.chromium.org/2690573002
Cr-Commit-Position: refs/heads/master@{#453199}
Committed: https://chromium.googlesource.com/chromium/src/+/fbb7fb60fb7ceaf4f737c638bbd502d26fe129b3
Patch Set 1 #
Total comments: 37
Patch Set 2 : Address CL isues. #
Total comments: 3
Patch Set 3 : Address CL issues. #Patch Set 4 : Add ViewMacTest.HandlesThreeFingerSwipeGestures. #
Total comments: 30
Patch Set 5 : Address CL issues. #
Total comments: 7
Patch Set 6 : Address sky's CL issues. #Patch Set 7 : Fix compilation on Windows. #
Total comments: 2
Patch Set 8 : GetGestureCommand: return early in happy path. #
Messages
Total messages: 24 (7 generated)
Description was changed from ========== MacViews: Handle three-finger swipe gestures for navigation. It's possible to enable three-finger swipes in System Preferences, and they result in immediate -swipeWithEvent: calls, unlike two-finger swipes which should be tracked. Added code to convert them to ET_GESTURE_SWIPE and to handle them in BrowserView. Cocoa code handled these events in BrowserWindowController in a similar fashion. BUG=None ========== to ========== MacViews: Handle three-finger swipe gestures for navigation. It's possible to enable three-finger swipes in System Preferences, and they result in immediate -swipeWithEvent: calls, unlike two-finger swipes which should be tracked. Added code to convert them to ET_GESTURE_SWIPE and to handle them in BrowserView. Cocoa code handled these events in BrowserWindowController in a similar fashion. BUG=None ==========
mblsha@yandex-team.ru changed reviewers: + sky@chromium.org, tapted@chromium.org
I want to add a test as well, but haven't found a place for it yet. Both src/ui/views/cocoa/bridged_native_widget_unittest.mm and src/ui/views/widget/native_widget_mac_interactive_uitest.mm seem off to me. Wanted to generate a synthesized event and see that it's correctly converted to the ui::GestureEvent and properly passed off to views::View.
https://codereview.chromium.org/2690573002/diff/1/chrome/browser/ui/views/fra... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/2690573002/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/browser_view.cc:1985: if (event->details().type() == ui::ET_GESTURE_SWIPE) { Can you structure this code to be a bit more readable, e.g.: int command = NO_COMMAND (hopefully there is a define for it). if (GetGestureCommand(*event, &command) && IsCommandE...) ExecuteCommand... https://codereview.chromium.org/2690573002/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/browser_view.cc:1986: unsigned int command = 0; Aren't command ids ints?
Can you add a BUG=? There's probably more to do for this, and we should have a bug to track. E.g. - handling of 0.5 threshold - handling of 2-finger swipe (when 'Swipe between pages' is configured for it in prefs - providing UI feedback the way we do on Mac currently for 2-finger swipe (but not 3-finger) Note parts of the above may need solutions before this lands. On 2017/02/10 15:45:51, themblsha wrote: > I want to add a test as well, but haven't found a place for it yet. Both > src/ui/views/cocoa/bridged_native_widget_unittest.mm and > src/ui/views/widget/native_widget_mac_interactive_uitest.mm seem off to me. > > Wanted to generate a synthesized event and see that it's correctly converted to > the ui::GestureEvent and properly passed off to views::View. I'm pretty sure Cocoa doesn't have a way to synthesize these events, but we can probably inherit from NSEvent* and override the methods we are interested in. Also we should limit the role Cocoa event dispatch plays, since it's configurable in the system. i.e. dispatch to [[GetNativeWindow() contentView] swipeWithEvent:fake_event] or similar. For location, I suggest view_unittest_mac.mm -- view_unittest.cc has most of the EventHandler tests currently, and there is already view_unittest_aura.cc https://codereview.chromium.org/2690573002/diff/1/chrome/browser/ui/views/fra... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/2690573002/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/browser_view.cc:1987: if (event->details().swipe_left()) -[BrowserWindowController swipeWithEvent:] only triggers these commands when the delta is over a threshold (i.e. 0.5). but GestureEvent sets these bools with any strictly positive value - I don't think this will give a good UX. https://codereview.chromium.org/2690573002/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/browser_view.cc:1989: else if (event->details().swipe_right()) I really think left/right around the right are around the wrong way here. "left" should go back, "right" should go forward. The problem is that GestureEventDetails does data_.swipe.left = delta_x < 0; data_.swipe.right = delta_x > 0; but BrowserWindowController does if (deltaX > 0.5) { command = IDC_BACK; } else if (deltaX < -0.5) { command = IDC_FORWARD; } I'd say it's the NSEvents that are "weird" so you'll need to put on a negative sign before passing [event deltaX] to the GestureEvent constructor. Please test deltaY manually to see what should be done with that (i.e. ensure swipe_up()/down() actually correspond to whether you're swiping up or down https://codereview.chromium.org/2690573002/diff/1/chrome/browser/ui/views/fra... File chrome/browser/ui/views/frame/browser_view.h (right): https://codereview.chromium.org/2690573002/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/browser_view.h:444: void OnGestureEvent(ui::GestureEvent* event) override; nit: sort according to view.h order (double-check, but I think this should move up a line) https://codereview.chromium.org/2690573002/diff/1/ui/views/cocoa/bridged_cont... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/2690573002/diff/1/ui/views/cocoa/bridged_cont... ui/views/cocoa/bridged_content_view.mm:803: // Called when we get a three-finger swipe, and they're enabled in System I think the pref can be set to 2-fingers or 3-fingers (or both). https://codereview.chromium.org/2690573002/diff/1/ui/views/cocoa/bridged_cont... ui/views/cocoa/bridged_content_view.mm:805: - (void)swipeWithEvent:(NSEvent*)event { This should bail out early when hostedView_ is null, similar to -scrollWheel: https://codereview.chromium.org/2690573002/diff/1/ui/views/cocoa/bridged_cont... ui/views/cocoa/bridged_content_view.mm:806: ui::GestureEventDetails swipe_details(ui::ET_GESTURE_SWIPE, [event deltaX], swipe_details -> swipeDetails (This is between @foo/@end, so variables need to be camelCase) https://codereview.chromium.org/2690573002/diff/1/ui/views/cocoa/bridged_cont... ui/views/cocoa/bridged_content_view.mm:806: ui::GestureEventDetails swipe_details(ui::ET_GESTURE_SWIPE, [event deltaX], EventTypeFromNative needs to be updated -- NSEventTypeSwipe doesn't currently return ET_GESTURE_SWIPE https://codereview.chromium.org/2690573002/diff/1/ui/views/cocoa/bridged_cont... ui/views/cocoa/bridged_content_view.mm:809: set_touch_points? https://codereview.chromium.org/2690573002/diff/1/ui/views/cocoa/bridged_cont... ui/views/cocoa/bridged_content_view.mm:811: ui::GestureEvent gesture_event(location.x(), location.y(), gestureEvent, or just |event| https://codereview.chromium.org/2690573002/diff/1/ui/views/cocoa/bridged_cont... ui/views/cocoa/bridged_content_view.mm:812: [event modifierFlags], EventFlagsFromNative - we can't pass the Cocoa flags as is https://codereview.chromium.org/2690573002/diff/1/ui/views/cocoa/bridged_cont... ui/views/cocoa/bridged_content_view.mm:813: ui::EventTimeFromNative(event), swipe_details); -[NSEvent eventNumber] is probably an appropriate thing to pass for unique_touch_event_id -- the comment for that arg suggests to me that it should only be 0 in tests https://codereview.chromium.org/2690573002/diff/1/ui/views/cocoa/bridged_cont... ui/views/cocoa/bridged_content_view.mm:817: ->GetWidget() hostedView_->GetWidget()
Description was changed from ========== MacViews: Handle three-finger swipe gestures for navigation. It's possible to enable three-finger swipes in System Preferences, and they result in immediate -swipeWithEvent: calls, unlike two-finger swipes which should be tracked. Added code to convert them to ET_GESTURE_SWIPE and to handle them in BrowserView. Cocoa code handled these events in BrowserWindowController in a similar fashion. BUG=None ========== to ========== MacViews: Handle three-finger swipe gestures for navigation. It's possible to enable three-finger swipes in System Preferences, and they result in immediate -swipeWithEvent: calls, unlike two-finger swipes which should be tracked. Added code to convert them to ET_GESTURE_SWIPE and to handle them in BrowserView. Cocoa code handled these events in BrowserWindowController in a similar fashion. BUG=691558 ==========
Still need to work on the test :-) https://codereview.chromium.org/2690573002/diff/1/chrome/browser/ui/views/fra... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/2690573002/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/browser_view.cc:1985: if (event->details().type() == ui::ET_GESTURE_SWIPE) { On 2017/02/10 19:08:33, sky wrote: > Can you structure this code to be a bit more readable, e.g.: > > int command = NO_COMMAND (hopefully there is a define for it). > if (GetGestureCommand(*event, &command) && IsCommandE...) > ExecuteCommand... base::Optional<int> should be a better way to do it rather than a NO_COMMAND (and there seem to be no such thing in chrome_command_ids.h). https://codereview.chromium.org/2690573002/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/browser_view.cc:1986: unsigned int command = 0; On 2017/02/10 19:08:33, sky wrote: > Aren't command ids ints? The code was adapted from the browser_window_controller.mm. Updated to an int. https://codereview.chromium.org/2690573002/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/browser_view.cc:1987: if (event->details().swipe_left()) On 2017/02/13 11:26:24, tapted wrote: > -[BrowserWindowController swipeWithEvent:] only triggers these commands when the > delta is over a threshold (i.e. 0.5). but GestureEvent sets these bools with any > strictly positive value - I don't think this will give a good UX. In my testing the -swipeWithEvent: is always called with a value of "+/-1" without taking gesture speed into account. This matches Apple's documentation that the delta should be simply taken as positive/negative. Only a single event is send per three-finger swipe. The UX is exactly the same as with the other three-finger swiping apps :-) https://codereview.chromium.org/2690573002/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/browser_view.cc:1989: else if (event->details().swipe_right()) On 2017/02/13 11:26:24, tapted wrote: > I really think left/right around the right are around the wrong way here. > > "left" should go back, "right" should go forward. > > The problem is that GestureEventDetails does > > data_.swipe.left = delta_x < 0; > data_.swipe.right = delta_x > 0; > > but BrowserWindowController does > > if (deltaX > 0.5) { > command = IDC_BACK; > } else if (deltaX < -0.5) { > command = IDC_FORWARD; > } > > I'd say it's the NSEvents that are "weird" so you'll need to put on a negative > sign before passing [event deltaX] to the GestureEvent constructor. > > Please test deltaY manually to see what should be done with that (i.e. ensure > swipe_up()/down() actually correspond to whether you're swiping up or down I originally tested the behaviour to match Safari and Cocoa Chrome. Inverted the deltaX / deltaY in the constructor call, thanks! https://codereview.chromium.org/2690573002/diff/1/chrome/browser/ui/views/fra... File chrome/browser/ui/views/frame/browser_view.h (right): https://codereview.chromium.org/2690573002/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/browser_view.h:444: void OnGestureEvent(ui::GestureEvent* event) override; On 2017/02/13 11:26:24, tapted wrote: > nit: sort according to view.h order (double-check, but I think this should move > up a line) Sorting them all correctly will introduce a major diff, as I would also need to re-shuffle the .cc methods. This is probably an overkill for this CL. --- a/chrome/browser/ui/views/frame/browser_view.h +++ b/chrome/browser/ui/views/frame/browser_view.h @@ -433,15 +433,15 @@ class BrowserView : public BrowserWindow, bool DrawInfoBarArrows(int* x) const override; // Overridden from views::View: - const char* GetClassName() const override; void Layout() override; + const char* GetClassName() const override; + void OnGestureEvent(ui::GestureEvent* event) override; + void GetAccessibleNodeData(ui::AXNodeData* node_data) override; + void ChildPreferredSizeChanged(View* child) override; void ViewHierarchyChanged( const ViewHierarchyChangedDetails& details) override; - void ChildPreferredSizeChanged(View* child) override; - void GetAccessibleNodeData(ui::AXNodeData* node_data) override; void OnThemeChanged() override; void OnNativeThemeChanged(const ui::NativeTheme* theme) override; - void OnGestureEvent(ui::GestureEvent* event) override; // Overridden from ui::AcceleratorTarget: bool AcceleratorPressed(const ui::Accelerator& accelerator) override; https://codereview.chromium.org/2690573002/diff/1/ui/views/cocoa/bridged_cont... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/2690573002/diff/1/ui/views/cocoa/bridged_cont... ui/views/cocoa/bridged_content_view.mm:803: // Called when we get a three-finger swipe, and they're enabled in System On 2017/02/13 11:26:25, tapted wrote: > I think the pref can be set to 2-fingers or 3-fingers (or both). Yep. But two-finger swipes are already handled by HistoryOverlayController. By default three-finger swipes are disabled. https://codereview.chromium.org/2690573002/diff/1/ui/views/cocoa/bridged_cont... ui/views/cocoa/bridged_content_view.mm:805: - (void)swipeWithEvent:(NSEvent*)event { On 2017/02/13 11:26:25, tapted wrote: > This should bail out early when hostedView_ is null, similar to -scrollWheel: Done. https://codereview.chromium.org/2690573002/diff/1/ui/views/cocoa/bridged_cont... ui/views/cocoa/bridged_content_view.mm:806: ui::GestureEventDetails swipe_details(ui::ET_GESTURE_SWIPE, [event deltaX], On 2017/02/13 11:26:24, tapted wrote: > EventTypeFromNative needs to be updated -- NSEventTypeSwipe doesn't currently > return ET_GESTURE_SWIPE It's technically the same type of NSEvent that starts smooth scrolling, but -swipeWithEvent: is only called for momentary gestures which couldn't be tracked: you're simply notified that they happened and you handle them. EventTypeFromNative currently returns ET_SCROLL_FLING_START for NSEventTypeSwipe and I think this is appropriate, as the two-finger swipes are a different beast altogether and deserve special treatment :-) https://codereview.chromium.org/2690573002/diff/1/ui/views/cocoa/bridged_cont... ui/views/cocoa/bridged_content_view.mm:806: ui::GestureEventDetails swipe_details(ui::ET_GESTURE_SWIPE, [event deltaX], On 2017/02/13 11:26:25, tapted wrote: > swipe_details -> swipeDetails (This is between @foo/@end, so variables need to > be camelCase) Done. https://codereview.chromium.org/2690573002/diff/1/ui/views/cocoa/bridged_cont... ui/views/cocoa/bridged_content_view.mm:809: On 2017/02/13 11:26:24, tapted wrote: > set_touch_points? I don't think it's applicable to the instant three-finger swipes. https://codereview.chromium.org/2690573002/diff/1/ui/views/cocoa/bridged_cont... ui/views/cocoa/bridged_content_view.mm:811: ui::GestureEvent gesture_event(location.x(), location.y(), On 2017/02/13 11:26:25, tapted wrote: > gestureEvent, or just |event| Done. https://codereview.chromium.org/2690573002/diff/1/ui/views/cocoa/bridged_cont... ui/views/cocoa/bridged_content_view.mm:812: [event modifierFlags], On 2017/02/13 11:26:25, tapted wrote: > EventFlagsFromNative - we can't pass the Cocoa flags as is Done. https://codereview.chromium.org/2690573002/diff/1/ui/views/cocoa/bridged_cont... ui/views/cocoa/bridged_content_view.mm:813: ui::EventTimeFromNative(event), swipe_details); On 2017/02/13 11:26:24, tapted wrote: > -[NSEvent eventNumber] is probably an appropriate thing to pass for > unique_touch_event_id -- the comment for that arg suggests to me that it should > only be 0 in tests This throws an exception: Invalid message sent to event "NSEvent: type=Swipe loc=(411.582,723.938) time=86773.1 flags=0 win=0x10031dbf0 winNum=1671 ctxt=0x0 phase: 4 axis:1 amount=0.052 velocity={-0, -0}". And instant events cannot be tracked after all, so this doesn't seem as a trouble spot. https://codereview.chromium.org/2690573002/diff/1/ui/views/cocoa/bridged_cont... ui/views/cocoa/bridged_content_view.mm:817: ->GetWidget() On 2017/02/13 11:26:24, tapted wrote: > hostedView_->GetWidget() Done.
https://codereview.chromium.org/2690573002/diff/1/chrome/browser/ui/views/fra... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/2690573002/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/browser_view.cc:1987: if (event->details().swipe_left()) On 2017/02/15 17:35:34, themblsha wrote: > On 2017/02/13 11:26:24, tapted wrote: > > -[BrowserWindowController swipeWithEvent:] only triggers these commands when > the > > delta is over a threshold (i.e. 0.5). but GestureEvent sets these bools with > any > > strictly positive value - I don't think this will give a good UX. > > In my testing the -swipeWithEvent: is always called with a value of "+/-1" > without taking gesture speed into account. This matches Apple's documentation > that the delta should be simply taken as positive/negative. Can you comment about this where you construct the GestureEvent? - that's a great breadcrumb to leave for a future reader. > > Only a single event is send per three-finger swipe. The UX is exactly the same > as with the other three-finger swiping apps :-) oh weird - yeah even Safari has no feedback for this. (yuck) https://codereview.chromium.org/2690573002/diff/1/chrome/browser/ui/views/fra... File chrome/browser/ui/views/frame/browser_view.h (right): https://codereview.chromium.org/2690573002/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/browser_view.h:444: void OnGestureEvent(ui::GestureEvent* event) override; On 2017/02/15 17:35:34, themblsha wrote: > On 2017/02/13 11:26:24, tapted wrote: > > nit: sort according to view.h order (double-check, but I think this should > move > > up a line) > > Sorting them all correctly will introduce a major diff, as I would also need to > re-shuffle the .cc methods. This is probably an overkill for this CL. > > --- a/chrome/browser/ui/views/frame/browser_view.h > +++ b/chrome/browser/ui/views/frame/browser_view.h > @@ -433,15 +433,15 @@ class BrowserView : public BrowserWindow, > bool DrawInfoBarArrows(int* x) const override; > > // Overridden from views::View: > - const char* GetClassName() const override; > void Layout() override; > + const char* GetClassName() const override; > + void OnGestureEvent(ui::GestureEvent* event) override; > + void GetAccessibleNodeData(ui::AXNodeData* node_data) override; > + void ChildPreferredSizeChanged(View* child) override; > void ViewHierarchyChanged( > const ViewHierarchyChangedDetails& details) override; > - void ChildPreferredSizeChanged(View* child) override; > - void GetAccessibleNodeData(ui::AXNodeData* node_data) override; > void OnThemeChanged() override; > void OnNativeThemeChanged(const ui::NativeTheme* theme) override; > - void OnGestureEvent(ui::GestureEvent* event) override; > > // Overridden from ui::AcceleratorTarget: > bool AcceleratorPressed(const ui::Accelerator& accelerator) override; Acknowledged. https://codereview.chromium.org/2690573002/diff/1/ui/views/cocoa/bridged_cont... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/2690573002/diff/1/ui/views/cocoa/bridged_cont... ui/views/cocoa/bridged_content_view.mm:803: // Called when we get a three-finger swipe, and they're enabled in System On 2017/02/15 17:35:34, themblsha wrote: > On 2017/02/13 11:26:25, tapted wrote: > > I think the pref can be set to 2-fingers or 3-fingers (or both). > > Yep. But two-finger swipes are already handled by HistoryOverlayController. By > default three-finger swipes are disabled. ohh interesting. We'll eventually want to remove HistoryOverlayController from the mac_views_browser build, since it's mashing native views into the hierarchy (instead we should use gesture_nav_simple.cc). But we can worry about this later (but there may be a large degree of "luck" that HistoryOverlayController continues work overlaid on top of a BrowserView) https://codereview.chromium.org/2690573002/diff/1/ui/views/cocoa/bridged_cont... ui/views/cocoa/bridged_content_view.mm:806: ui::GestureEventDetails swipe_details(ui::ET_GESTURE_SWIPE, [event deltaX], On 2017/02/15 17:35:34, themblsha wrote: > On 2017/02/13 11:26:24, tapted wrote: > > EventTypeFromNative needs to be updated -- NSEventTypeSwipe doesn't currently > > return ET_GESTURE_SWIPE > > It's technically the same type of NSEvent that starts smooth scrolling, but > -swipeWithEvent: is only called for momentary gestures which couldn't be > tracked: you're simply notified that they happened and you handle them. > > EventTypeFromNative currently returns ET_SCROLL_FLING_START for NSEventTypeSwipe > and I think this is appropriate, as the two-finger swipes are a different beast > altogether and deserve special treatment :-) Acknowledged. https://codereview.chromium.org/2690573002/diff/1/ui/views/cocoa/bridged_cont... ui/views/cocoa/bridged_content_view.mm:809: On 2017/02/15 17:35:34, themblsha wrote: > On 2017/02/13 11:26:24, tapted wrote: > > set_touch_points? > > I don't think it's applicable to the instant three-finger swipes. Acknowledged. (I'd also be fine with just setting it to 3, similar to how we set `2` for trackpad scrolling in events_mac.mm) https://codereview.chromium.org/2690573002/diff/1/ui/views/cocoa/bridged_cont... ui/views/cocoa/bridged_content_view.mm:813: ui::EventTimeFromNative(event), swipe_details); On 2017/02/15 17:35:34, themblsha wrote: > On 2017/02/13 11:26:24, tapted wrote: > > -[NSEvent eventNumber] is probably an appropriate thing to pass for > > unique_touch_event_id -- the comment for that arg suggests to me that it > should > > only be 0 in tests > > This throws an exception: Invalid message sent to event "NSEvent: type=Swipe > loc=(411.582,723.938) time=86773.1 flags=0 win=0x10031dbf0 winNum=1671 ctxt=0x0 > phase: 4 axis:1 amount=0.052 velocity={-0, -0}". > > And instant events cannot be tracked after all, so this doesn't seem as a > trouble spot. Can you comment about this? (~ "Note this uses the default unique_touch_event_id of 0 (Swipe events do not support -[NSEvent eventNumber]"). I think it will matter if these events are ever sent on to Blink/WebContents (but currently ui::CreateWebGestureEvent will drop them on the floor anyway) https://codereview.chromium.org/2690573002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/2690573002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_view.cc:1943: auto get_gesture_command = [](ui::GestureEvent* event) { Inferring the return type of a lambda is a C++14 feature.. I doubt MSVC will like this. This is #if guarded so MSVC might not see it, but I don't think we should be relying on this just yet. I think sky's suggestion is more idiomatic for chrome -- just pass an int* and return a bool. https://codereview.chromium.org/2690573002/diff/20001/ui/views/cocoa/bridged_... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/2690573002/diff/20001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:811: auto invert = [](CGFloat val) { return val * -1.; }; The comment is good, but (unless I'm missing something) the lambda isn't necessary -- just do ui::GestureEventDetails swipeDetails( ui::ET_GESTURE_SWIPE, -[event deltaX], -[event deltaY]);
Started work on ui/views/view_unittest_mac.mm, but not finished yet. https://codereview.chromium.org/2690573002/diff/1/chrome/browser/ui/views/fra... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/2690573002/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/browser_view.cc:1987: if (event->details().swipe_left()) On 2017/02/15 22:25:52, tapted wrote: > On 2017/02/15 17:35:34, themblsha wrote: > > On 2017/02/13 11:26:24, tapted wrote: > > > -[BrowserWindowController swipeWithEvent:] only triggers these commands when > > the > > > delta is over a threshold (i.e. 0.5). but GestureEvent sets these bools with > > any > > > strictly positive value - I don't think this will give a good UX. > > > > In my testing the -swipeWithEvent: is always called with a value of "+/-1" > > without taking gesture speed into account. This matches Apple's documentation > > that the delta should be simply taken as positive/negative. > > Can you comment about this where you construct the GestureEvent? - that's a > great breadcrumb to leave for a future reader. Done. > > Only a single event is send per three-finger swipe. The UX is exactly the same > > as with the other three-finger swiping apps :-) > > oh weird - yeah even Safari has no feedback for this. (yuck) They're probably from the ancient times when the GestureRecognizers haven't been invented yet. And some people probably enjoy the immediacy and relative lack of feedback :-) https://codereview.chromium.org/2690573002/diff/1/ui/views/cocoa/bridged_cont... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/2690573002/diff/1/ui/views/cocoa/bridged_cont... ui/views/cocoa/bridged_content_view.mm:809: On 2017/02/15 22:25:52, tapted wrote: > On 2017/02/15 17:35:34, themblsha wrote: > > On 2017/02/13 11:26:24, tapted wrote: > > > set_touch_points? > > > > I don't think it's applicable to the instant three-finger swipes. > > Acknowledged. (I'd also be fine with just setting it to 3, similar to how we set > `2` for trackpad scrolling in events_mac.mm) Ah. Okay, added setting it to `3` :-) https://codereview.chromium.org/2690573002/diff/1/ui/views/cocoa/bridged_cont... ui/views/cocoa/bridged_content_view.mm:813: ui::EventTimeFromNative(event), swipe_details); On 2017/02/15 22:25:52, tapted wrote: > On 2017/02/15 17:35:34, themblsha wrote: > > On 2017/02/13 11:26:24, tapted wrote: > > > -[NSEvent eventNumber] is probably an appropriate thing to pass for > > > unique_touch_event_id -- the comment for that arg suggests to me that it > > should > > > only be 0 in tests > > > > This throws an exception: Invalid message sent to event "NSEvent: type=Swipe > > loc=(411.582,723.938) time=86773.1 flags=0 win=0x10031dbf0 winNum=1671 > ctxt=0x0 > > phase: 4 axis:1 amount=0.052 velocity={-0, -0}". > > > > And instant events cannot be tracked after all, so this doesn't seem as a > > trouble spot. > > Can you comment about this? (~ "Note this uses the default unique_touch_event_id > of 0 (Swipe events do not support -[NSEvent eventNumber]"). I think it will > matter if these events are ever sent on to Blink/WebContents (but currently > ui::CreateWebGestureEvent will drop them on the floor anyway) Done. https://codereview.chromium.org/2690573002/diff/20001/ui/views/cocoa/bridged_... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/2690573002/diff/20001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:811: auto invert = [](CGFloat val) { return val * -1.; }; On 2017/02/15 22:25:53, tapted wrote: > The comment is good, but (unless I'm missing something) the lambda isn't > necessary -- just do > > ui::GestureEventDetails swipeDetails( > ui::ET_GESTURE_SWIPE, -[event deltaX], -[event deltaY]); You've opened my eyes yet again. Thanks :-)
The test is here :-)
https://codereview.chromium.org/2690573002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/2690573002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_view.cc:1964: } I'm undecided about base::Optional and the lambda here. Older code would use int* and a function in an anonymous namespace. The new approach has merit, but it will be unfamiliar for some developers. So, no objection from me, but I am not owner here :) https://codereview.chromium.org/2690573002/diff/60001/ui/views/cocoa/bridged_... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/2690573002/diff/60001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:822: // support -[NSEvent eventNumber]. This doesn't seem like a real omission nit: add closing paren: "... do not support -[NSEvent eventNumber])." https://codereview.chromium.org/2690573002/diff/60001/ui/views/view_unittest_... File ui/views/view_unittest_mac.mm (right): https://codereview.chromium.org/2690573002/diff/60001/ui/views/view_unittest_... ui/views/view_unittest_mac.mm:7: #include "base/mac/scoped_nsobject.h" nit: import https://codereview.chromium.org/2690573002/diff/60001/ui/views/view_unittest_... ui/views/view_unittest_mac.mm:11: #include "ui/views/view.h" nit: move this one all the way to the top (above Cocoa), with a blank line under. https://codereview.chromium.org/2690573002/diff/60001/ui/views/view_unittest_... ui/views/view_unittest_mac.mm:37: return static_cast<NSEventSubtype>(0); 0 is `NSEventSubtypeWindowExposed` - maybe `return NSEventSubtypeTouch` ? https://codereview.chromium.org/2690573002/diff/60001/ui/views/view_unittest_... ui/views/view_unittest_mac.mm:42: add anonymous namespace { https://codereview.chromium.org/2690573002/diff/60001/ui/views/view_unittest_... ui/views/view_unittest_mac.mm:49: // views::View: nit: remove `views::` https://codereview.chromium.org/2690573002/diff/60001/ui/views/view_unittest_... ui/views/view_unittest_mac.mm:51: DCHECK_EQ(event->details().type(), ui::ET_GESTURE_SWIPE); nit: EXPECT_EQ(ui::ET_GESTURE_SWIPE, event->details().type()); - DCHECK crashes the test process, which can fail unrelated tests when the test harness runs in a threaded mode - For EXPECT, `EXPECT_FOO(<expected>, <actual>);` gives more readable messages. https://codereview.chromium.org/2690573002/diff/60001/ui/views/view_unittest_... ui/views/view_unittest_mac.mm:58: DCHECK_EQ(dx, 0); EXPECT_EQ(0, dx) https://codereview.chromium.org/2690573002/diff/60001/ui/views/view_unittest_... ui/views/view_unittest_mac.mm:66: DCHECK_EQ(dy, 0); EXPECT_EQ(0, dy); https://codereview.chromium.org/2690573002/diff/60001/ui/views/view_unittest_... ui/views/view_unittest_mac.mm:82: conventionally, the } // namespace would go here. https://codereview.chromium.org/2690573002/diff/60001/ui/views/view_unittest_... ui/views/view_unittest_mac.mm:88: swipe_event.get().deltaX = dx; [swipe_event setDeltaX:dx] same for all the stuff below (pretty sure that works. We don't use Objc2 dot notation for properties in Chrome [yet]). https://codereview.chromium.org/2690573002/diff/60001/ui/views/view_unittest_... ui/views/view_unittest_mac.mm:110: view_->SetBoundsRect(bounds); This will make the view bigger than the window's content area due to the titlebar. I'd probably do: widget_->SetBounds(gfx::Rect(0, 0, 100, 100)); .. view_->SetSize(widget_->GetClientAreaBoundsInScreen().size()); (CreateTopLevelFramelessPlatformWidget() above is another option) https://codereview.chromium.org/2690573002/diff/60001/ui/views/view_unittest_... ui/views/view_unittest_mac.mm:122: }; nit: DISALLOW_COPY_AND_ASSIGN(..) (you'll need to add a default constructor too) https://codereview.chromium.org/2690573002/diff/60001/ui/views/view_unittest_... ui/views/view_unittest_mac.mm:126: EXPECT_EQ(gfx::Point(1, 0), *SwipeGestureVector(-1, 0)); nit: a comment like "Note that positive delta is left and up for NSEvent, which is the inverse of ui::GestureEventDetails."
https://codereview.chromium.org/2690573002/diff/60001/ui/views/cocoa/bridged_... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/2690573002/diff/60001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:822: // support -[NSEvent eventNumber]. This doesn't seem like a real omission On 2017/02/20 23:14:21, tapted wrote: > nit: add closing paren: "... do not support -[NSEvent eventNumber])." Done. https://codereview.chromium.org/2690573002/diff/60001/ui/views/view_unittest_... File ui/views/view_unittest_mac.mm (right): https://codereview.chromium.org/2690573002/diff/60001/ui/views/view_unittest_... ui/views/view_unittest_mac.mm:7: #include "base/mac/scoped_nsobject.h" On 2017/02/20 23:14:21, tapted wrote: > nit: import Done. https://codereview.chromium.org/2690573002/diff/60001/ui/views/view_unittest_... ui/views/view_unittest_mac.mm:11: #include "ui/views/view.h" On 2017/02/20 23:14:21, tapted wrote: > nit: move this one all the way to the top (above Cocoa), with a blank line > under. Done. https://codereview.chromium.org/2690573002/diff/60001/ui/views/view_unittest_... ui/views/view_unittest_mac.mm:37: return static_cast<NSEventSubtype>(0); On 2017/02/20 23:14:21, tapted wrote: > 0 is `NSEventSubtypeWindowExposed` - maybe `return NSEventSubtypeTouch` ? In my testing the Swipe events all had 0 as their subtype. Added comment. https://codereview.chromium.org/2690573002/diff/60001/ui/views/view_unittest_... ui/views/view_unittest_mac.mm:42: On 2017/02/20 23:14:21, tapted wrote: > add anonymous > > namespace { Done. https://codereview.chromium.org/2690573002/diff/60001/ui/views/view_unittest_... ui/views/view_unittest_mac.mm:49: // views::View: On 2017/02/20 23:14:21, tapted wrote: > nit: remove `views::` Done. https://codereview.chromium.org/2690573002/diff/60001/ui/views/view_unittest_... ui/views/view_unittest_mac.mm:51: DCHECK_EQ(event->details().type(), ui::ET_GESTURE_SWIPE); On 2017/02/20 23:14:21, tapted wrote: > nit: EXPECT_EQ(ui::ET_GESTURE_SWIPE, event->details().type()); > > - DCHECK crashes the test process, which can fail unrelated tests when the test > harness runs in a threaded mode > - For EXPECT, `EXPECT_FOO(<expected>, <actual>);` gives more readable messages. Thanks! I fixed the EXPECTs at the bottom of the file before submitting but missed these ones. https://codereview.chromium.org/2690573002/diff/60001/ui/views/view_unittest_... ui/views/view_unittest_mac.mm:58: DCHECK_EQ(dx, 0); On 2017/02/20 23:14:21, tapted wrote: > EXPECT_EQ(0, dx) Done. https://codereview.chromium.org/2690573002/diff/60001/ui/views/view_unittest_... ui/views/view_unittest_mac.mm:66: DCHECK_EQ(dy, 0); On 2017/02/20 23:14:21, tapted wrote: > EXPECT_EQ(0, dy); Done. https://codereview.chromium.org/2690573002/diff/60001/ui/views/view_unittest_... ui/views/view_unittest_mac.mm:82: On 2017/02/20 23:14:21, tapted wrote: > conventionally, the > > } // namespace > > would go here. Also works when placed at the end of the ViewMacTest, but conventions are to be adhered to. https://codereview.chromium.org/2690573002/diff/60001/ui/views/view_unittest_... ui/views/view_unittest_mac.mm:88: swipe_event.get().deltaX = dx; On 2017/02/20 23:14:21, tapted wrote: > [swipe_event setDeltaX:dx] > > same for all the stuff below > > (pretty sure that works. We don't use Objc2 dot notation for properties in > Chrome [yet]). Done. https://codereview.chromium.org/2690573002/diff/60001/ui/views/view_unittest_... ui/views/view_unittest_mac.mm:110: view_->SetBoundsRect(bounds); On 2017/02/20 23:14:21, tapted wrote: > This will make the view bigger than the window's content area due to the > titlebar. I'd probably do: > > widget_->SetBounds(gfx::Rect(0, 0, 100, 100)); > .. > view_->SetSize(widget_->GetClientAreaBoundsInScreen().size()); > > > > (CreateTopLevelFramelessPlatformWidget() above is another option) There seems to be a lot of tests which set up the widgets/views without regard to the titlebar, but still work because they generally target the central portion of the widget. So I thought it would be fine. But your suggestion is quite simple as well and correct to boot. Thanks! https://codereview.chromium.org/2690573002/diff/60001/ui/views/view_unittest_... ui/views/view_unittest_mac.mm:122: }; On 2017/02/20 23:14:21, tapted wrote: > nit: DISALLOW_COPY_AND_ASSIGN(..) (you'll need to add a default constructor > too) Done. https://codereview.chromium.org/2690573002/diff/60001/ui/views/view_unittest_... ui/views/view_unittest_mac.mm:126: EXPECT_EQ(gfx::Point(1, 0), *SwipeGestureVector(-1, 0)); On 2017/02/20 23:14:21, tapted wrote: > nit: a comment like "Note that positive delta is left and up for NSEvent, which > is the inverse of ui::GestureEventDetails." Done.
lgtm, but you need sky@ for browser_view.cc https://codereview.chromium.org/2690573002/diff/60001/ui/views/view_unittest_... File ui/views/view_unittest_mac.mm (right): https://codereview.chromium.org/2690573002/diff/60001/ui/views/view_unittest_... ui/views/view_unittest_mac.mm:37: return static_cast<NSEventSubtype>(0); On 2017/02/21 13:00:51, themblsha wrote: > On 2017/02/20 23:14:21, tapted wrote: > > 0 is `NSEventSubtypeWindowExposed` - maybe `return NSEventSubtypeTouch` ? > > In my testing the Swipe events all had 0 as their subtype. Added comment. Acknowledged.
Just style stuff. https://codereview.chromium.org/2690573002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/2690573002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_view.cc:1943: auto get_gesture_command = [](ui::GestureEvent* event, IMO using a closure for this is overkill. How about a normal function? https://codereview.chromium.org/2690573002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_view.cc:1954: base::Optional<int> command; Why bother with a base_optional instead of just the return value? Using an optional make this less understandable.
https://codereview.chromium.org/2690573002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/2690573002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_view.cc:1943: auto get_gesture_command = [](ui::GestureEvent* event, On 2017/02/22 00:37:17, sky wrote: > IMO using a closure for this is overkill. How about a normal function? Ok. I thought that this lambda serves only a very specific purpose on macOS so it should be visually close to the OnGestureEvent call, and anonymous namespace is pretty far away. https://codereview.chromium.org/2690573002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_view.cc:1954: base::Optional<int> command; On 2017/02/22 00:37:17, sky wrote: > Why bother with a base_optional instead of just the return value? Using an > optional make this less understandable. I thought that it would provide a nice additional level of safety: it would DCHECK if you would try to access the value while there's none. And there's no specific command id to signify that it's not initialized, that was its second purpose.
LGTM https://codereview.chromium.org/2690573002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/2690573002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_view.cc:1943: auto get_gesture_command = [](ui::GestureEvent* event, On 2017/02/22 11:49:50, themblsha wrote: > On 2017/02/22 00:37:17, sky wrote: > > IMO using a closure for this is overkill. How about a normal function? > > Ok. I thought that this lambda serves only a very specific purpose on macOS so > it should be visually close to the OnGestureEvent call, and anonymous namespace > is pretty far away. You can always use an anonymous namespace above this function, which gives you most of the scoping you are concerned about. https://codereview.chromium.org/2690573002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_view.cc:1954: base::Optional<int> command; On 2017/02/22 11:49:50, themblsha wrote: > On 2017/02/22 00:37:17, sky wrote: > > Why bother with a base_optional instead of just the return value? Using an > > optional make this less understandable. > > I thought that it would provide a nice additional level of safety: it would > DCHECK if you would try to access the value while there's none. And there's no > specific command id to signify that it's not initialized, that was its second > purpose. Like I said, I don't think it really buys you much in this case. https://codereview.chromium.org/2690573002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/2690573002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_view.cc:280: *command = IDC_BACK; I would early return here and on 282, then 285 becomes return false.
https://codereview.chromium.org/2690573002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/2690573002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_view.cc:1943: auto get_gesture_command = [](ui::GestureEvent* event, On 2017/02/22 17:34:10, sky wrote: > On 2017/02/22 11:49:50, themblsha wrote: > > On 2017/02/22 00:37:17, sky wrote: > > > IMO using a closure for this is overkill. How about a normal function? > > > > Ok. I thought that this lambda serves only a very specific purpose on macOS so > > it should be visually close to the OnGestureEvent call, and anonymous > namespace > > is pretty far away. > > You can always use an anonymous namespace above this function, which gives you > most of the scoping you are concerned about. Ah, there seems to be no restriction on where the anonymous namespace should be placed, but someone scolded me about it once before when I tried to add one right before the test that needed it. A single anonymous namespace at the top of the file is the Chromium convention, so it's safe :-) https://codereview.chromium.org/2690573002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/2690573002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_view.cc:280: *command = IDC_BACK; On 2017/02/22 17:34:10, sky wrote: > I would early return here and on 282, then 285 becomes return false. Ok, I thought my code would make it easier to be right as there's less chance of doing an early 'return false' with a correctly initialized command, but maybe it's too clever.
The CQ bit was checked by mblsha@yandex-team.ru
The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2690573002/#ps140001 (title: "GetGestureCommand: return early in happy path.")
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": 140001, "attempt_start_ts": 1488196161617880,
"parent_rev": "02ba668f1ae4306618c08ef19ed5d85ed4d481d2", "commit_rev":
"fbb7fb60fb7ceaf4f737c638bbd502d26fe129b3"}
Message was sent while issue was closed.
Description was changed from ========== MacViews: Handle three-finger swipe gestures for navigation. It's possible to enable three-finger swipes in System Preferences, and they result in immediate -swipeWithEvent: calls, unlike two-finger swipes which should be tracked. Added code to convert them to ET_GESTURE_SWIPE and to handle them in BrowserView. Cocoa code handled these events in BrowserWindowController in a similar fashion. BUG=691558 ========== to ========== MacViews: Handle three-finger swipe gestures for navigation. It's possible to enable three-finger swipes in System Preferences, and they result in immediate -swipeWithEvent: calls, unlike two-finger swipes which should be tracked. Added code to convert them to ET_GESTURE_SWIPE and to handle them in BrowserView. Cocoa code handled these events in BrowserWindowController in a similar fashion. BUG=691558 Review-Url: https://codereview.chromium.org/2690573002 Cr-Commit-Position: refs/heads/master@{#453199} Committed: https://chromium.googlesource.com/chromium/src/+/fbb7fb60fb7ceaf4f737c638bbd5... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/fbb7fb60fb7ceaf4f737c638bbd5... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
