Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2667293002:
[ash-md] Adds support for gesture to move selection in overview mode (Closed)
|
Created:
3 years, 10 months ago by varkha Modified:
3 years, 10 months ago Reviewers:
tdanderson CC:
chromium-reviews, kalyank, sadrul Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[ash-md] Adds support for gesture to move selection in overview mode
Make it possible to use overview mode without getting fingers off the
touchpad. Scroll up with 3 fingers enters overview mode. Scroll right
or left with 3 fingers moves selection. Scroll down dismisses overview
mode if no selection was made. If however the selection was moved with
a horizontal scroll, scroll down will accept this selection making a
new window active.
BUG=687773
TEST=OverviewGestureHandlerTest.HorizontalScrollInOverview
Review-Url: https://codereview.chromium.org/2667293002
Cr-Commit-Position: refs/heads/master@{#449687}
Committed: https://chromium.googlesource.com/chromium/src/+/525b0d69b50807ab7c9901f52acf858c0e5e1ba8
Patch Set 1 : [ash-md] Adds support for gesture to move selection in overview mode #
Total comments: 18
Patch Set 2 : [ash-md] Adds support for gesture to move selection in overview mode (comments) #
Total comments: 12
Patch Set 3 : [ash-md] Adds support for gesture to move selection in overview mode (comments) #Patch Set 4 : [ash-md] Adds support for gesture to move selection in overview mode (tuning) #
Total comments: 6
Patch Set 5 : [ash-md] Adds support for gesture to move selection in overview mode (no skipping 1st) #
Messages
Total messages: 59 (48 generated)
The CQ bit was checked by varkha@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 varkha@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...
Patchset #1 (id:1) has been deleted
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Patchset #1 (id:20001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
varkha@chromium.org changed reviewers: + tdanderson@chromium.org
tdanderson@, can you please take a first look. I think I will try to get it on device to see how it really feels with a touchpad and maybe calibrate the swipe sensitivity but otherwise I think I have the behaviour that I wanted. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Please see my comments below. https://codereview.chromium.org/2667293002/diff/40001/ash/common/wm/overview/... File ash/common/wm/overview/window_selector.h (right): https://codereview.chromium.org/2667293002/diff/40001/ash/common/wm/overview/... ash/common/wm/overview/window_selector.h:65: // |increment| move the selection forward, negative values mov it backward. nit: "move" https://codereview.chromium.org/2667293002/diff/40001/ash/common/wm/overview/... File ash/common/wm/overview/window_selector_controller.cc (right): https://codereview.chromium.org/2667293002/diff/40001/ash/common/wm/overview/... ash/common/wm/overview/window_selector_controller.cc:84: DCHECK(IsSelecting()); Is this necessary given how you've documented AcceptSelection(), and given line 418 in window_selector.cc? https://codereview.chromium.org/2667293002/diff/40001/ash/wm/gestures/overvie... File ash/wm/gestures/overview_gesture_handler.cc (right): https://codereview.chromium.org/2667293002/diff/40001/ash/wm/gestures/overvie... ash/wm/gestures/overview_gesture_handler.cc:21: const float kHorizontalSwipeThresholdPixels = 150; As I understand it you're processing ui::ET_SCROLL* event types in order to perform all of this behavior, not ui::ET_GESTURE_SWIPE events. So for lines 15-21 (and elsewhere in this CL) I think it would be better to use the term "three-finger touchpad scroll" or similar instead of "swipe" to avoid confusion about which event types we're using. https://codereview.chromium.org/2667293002/diff/40001/ash/wm/gestures/overvie... ash/wm/gestures/overview_gesture_handler.cc:37: scroll_y_ += event.y_offset(); In the .h these two members are documented as "The total distance scrolled with three fingers" which I don't think is correct. It seems as though these are just to track how much has been scrolled in either direction up to but not including the point where an action is triggered (enter overview, close overview, move selection widget). When any of those three things happen then the values are reset. https://codereview.chromium.org/2667293002/diff/40001/ash/wm/gestures/overvie... ash/wm/gestures/overview_gesture_handler.cc:74: if (window_selector_controller->IsSelecting() && Is checking IsSelecting() necessary here given how you've documented AcceptSelection(), and given line 418 in window_selector.cc? https://codereview.chromium.org/2667293002/diff/40001/ash/wm/gestures/overvie... File ash/wm/gestures/overview_gesture_handler_unittest.cc (right): https://codereview.chromium.org/2667293002/diff/40001/ash/wm/gestures/overvie... ash/wm/gestures/overview_gesture_handler_unittest.cc:79: 0, -500, 100, 3); nit: It might be nice to make use of kVerticalSwipeThresholdPixels here (and similarly kHorizontalSwipeThresholdPixels elsewhere in this test) instead of magic numbers. https://codereview.chromium.org/2667293002/diff/40001/ash/wm/gestures/overvie... ash/wm/gestures/overview_gesture_handler_unittest.cc:82: // Long swipe right moves selection to the fourth window. Consider adding a test to verify that horizontal three-finger scrolling is a no-op in overview mode if you didn't initiate overview from three-finger scroll upwards. (Or come to think of it, maybe we would want three-finger horizontal scroll to always have the same behavior?) https://codereview.chromium.org/2667293002/diff/40001/ash/wm/gestures/overvie... ash/wm/gestures/overview_gesture_handler_unittest.cc:97: // Swiping down exits. nit: swiping down exists and selects the currently-highlighted window.
Description was changed from ========== [ash-md] Adds support for gesture to move selection in overview mode Make it possible to use overview mode without getting fingers off the touchpad. Swipe up with 3 fingers enters overview mode. Swipe right or left with 3 fingers moves selection. Swipe down dismisses overview mode if no selection was made. If however the selection was moved with a horizontal swipe, swipe down will accept this selection making a new window active. BUG=687773 TEST=OverviewGestureHandlerTest.VerticalThenHorizontalSwipe ========== to ========== [ash-md] Adds support for gesture to move selection in overview mode Make it possible to use overview mode without getting fingers off the touchpad. Swipe up with 3 fingers enters overview mode. Swipe right or left with 2 or 3 fingers moves selection. Swipe down dismisses overview mode if no selection was made. If however the selection was moved with a horizontal swipe, swipe down will accept this selection making a new window active. BUG=687773 TEST=OverviewGestureHandlerTest.VerticalThenHorizontalSwipe ==========
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
https://codereview.chromium.org/2667293002/diff/40001/ash/common/wm/overview/... File ash/common/wm/overview/window_selector.h (right): https://codereview.chromium.org/2667293002/diff/40001/ash/common/wm/overview/... ash/common/wm/overview/window_selector.h:65: // |increment| move the selection forward, negative values mov it backward. On 2017/02/06 20:13:57, tdanderson wrote: > nit: "move" Done. https://codereview.chromium.org/2667293002/diff/40001/ash/common/wm/overview/... File ash/common/wm/overview/window_selector_controller.cc (right): https://codereview.chromium.org/2667293002/diff/40001/ash/common/wm/overview/... ash/common/wm/overview/window_selector_controller.cc:84: DCHECK(IsSelecting()); On 2017/02/06 20:13:57, tdanderson wrote: > Is this necessary given how you've documented AcceptSelection(), and given line > 418 in window_selector.cc? WindowSelectorController::IsSelecting is true when we are in overview mode, not necessarily when a selection is made (selector widget present). So yes, it confirms that we are in overview mode and can dereference window_selector_. Maybe somewhat redundant but the crash won't happen immediately, it will happen lower in stack and might be not obvious to grok, hence the DCHECK. https://codereview.chromium.org/2667293002/diff/40001/ash/wm/gestures/overvie... File ash/wm/gestures/overview_gesture_handler.cc (right): https://codereview.chromium.org/2667293002/diff/40001/ash/wm/gestures/overvie... ash/wm/gestures/overview_gesture_handler.cc:21: const float kHorizontalSwipeThresholdPixels = 150; On 2017/02/06 20:13:57, tdanderson wrote: > As I understand it you're processing ui::ET_SCROLL* event types in order to > perform all of this behavior, not ui::ET_GESTURE_SWIPE events. So for lines > 15-21 (and elsewhere in this CL) I think it would be better to use the term > "three-finger touchpad scroll" or similar instead of "swipe" to avoid confusion > about which event types we're using. Done. https://codereview.chromium.org/2667293002/diff/40001/ash/wm/gestures/overvie... ash/wm/gestures/overview_gesture_handler.cc:37: scroll_y_ += event.y_offset(); On 2017/02/06 20:13:58, tdanderson wrote: > In the .h these two members are documented as "The total distance scrolled with > three fingers" which I don't think is correct. It seems as though these are just > to track how much has been scrolled in either direction up to but not including > the point where an action is triggered (enter overview, close overview, move > selection widget). When any of those three things happen then the values are > reset. Done. https://codereview.chromium.org/2667293002/diff/40001/ash/wm/gestures/overvie... ash/wm/gestures/overview_gesture_handler.cc:74: if (window_selector_controller->IsSelecting() && On 2017/02/06 20:13:58, tdanderson wrote: > Is checking IsSelecting() necessary here given how you've documented > AcceptSelection(), and given line 418 in window_selector.cc? Yes, we can get here when not already in selection mode and it would be wrong to call AcceptSelection (but right to skip and ToggleOverview(). https://codereview.chromium.org/2667293002/diff/40001/ash/wm/gestures/overvie... File ash/wm/gestures/overview_gesture_handler_unittest.cc (right): https://codereview.chromium.org/2667293002/diff/40001/ash/wm/gestures/overvie... ash/wm/gestures/overview_gesture_handler_unittest.cc:79: 0, -500, 100, 3); On 2017/02/06 20:13:58, tdanderson wrote: > nit: It might be nice to make use of kVerticalSwipeThresholdPixels here (and > similarly kHorizontalSwipeThresholdPixels elsewhere in this test) instead of > magic numbers. Done. https://codereview.chromium.org/2667293002/diff/40001/ash/wm/gestures/overvie... ash/wm/gestures/overview_gesture_handler_unittest.cc:82: // Long swipe right moves selection to the fourth window. On 2017/02/06 20:13:58, tdanderson wrote: > Consider adding a test to verify that horizontal three-finger scrolling is a > no-op in overview mode if you didn't initiate overview from three-finger scroll > upwards. (Or come to think of it, maybe we would want three-finger horizontal > scroll to always have the same behavior?) (Or come to think of it, maybe we would want three-finger horizontal scroll to always have the same behavior? ^^ this ^^ https://codereview.chromium.org/2667293002/diff/40001/ash/wm/gestures/overvie... ash/wm/gestures/overview_gesture_handler_unittest.cc:97: // Swiping down exits. On 2017/02/06 20:13:58, tdanderson wrote: > nit: swiping down exists and selects the currently-highlighted window. Done.
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.
Please see my next round of comments below. https://codereview.chromium.org/2667293002/diff/40001/ash/common/wm/overview/... File ash/common/wm/overview/window_selector_controller.cc (right): https://codereview.chromium.org/2667293002/diff/40001/ash/common/wm/overview/... ash/common/wm/overview/window_selector_controller.cc:84: DCHECK(IsSelecting()); On 2017/02/08 20:41:13, varkha wrote: > On 2017/02/06 20:13:57, tdanderson wrote: > > Is this necessary given how you've documented AcceptSelection(), and given > line > > 418 in window_selector.cc? > > WindowSelectorController::IsSelecting is true when we are in overview mode, not > necessarily when a selection is made (selector widget present). So yes, it > confirms that we are in overview mode and can dereference window_selector_. > Maybe somewhat redundant but the crash won't happen immediately, it will happen > lower in stack and might be not obvious to grok, hence the DCHECK. Acknowledged. https://codereview.chromium.org/2667293002/diff/40001/ash/wm/gestures/overvie... File ash/wm/gestures/overview_gesture_handler.cc (right): https://codereview.chromium.org/2667293002/diff/40001/ash/wm/gestures/overvie... ash/wm/gestures/overview_gesture_handler.cc:74: if (window_selector_controller->IsSelecting() && On 2017/02/08 20:41:13, varkha wrote: > On 2017/02/06 20:13:58, tdanderson wrote: > > Is checking IsSelecting() necessary here given how you've documented > > AcceptSelection(), and given line 418 in window_selector.cc? > > Yes, we can get here when not already in selection mode and it would be wrong to > call AcceptSelection (but right to skip and ToggleOverview(). Acknowledged. https://codereview.chromium.org/2667293002/diff/60001/ash/wm/gestures/overvie... File ash/wm/gestures/overview_gesture_handler.cc (right): https://codereview.chromium.org/2667293002/diff/60001/ash/wm/gestures/overvie... ash/wm/gestures/overview_gesture_handler.cc:28: (event.finger_count() != 2 && event.finger_count() != 3)) { If you're going to start permitting two-finger scrolling to move the selection widget, given that the grid is two-dimensional, I would expect two-finger horizontal scrolls to be able to move the selection widget up/down. I think the general model of two-finger scrolling is that it works in both dimensions (think two-finger touchpad scrolling of a webpage); do you know of any counterexamples to this in our UI? The only other three-finger scrolling action I can think of is tab scrubbing in Ash, and that only works in a single dimension, so I see no inconsistencies with how you're making use of the three-finger scroll gesture in this CL. https://codereview.chromium.org/2667293002/diff/60001/ash/wm/gestures/overvie... ash/wm/gestures/overview_gesture_handler.cc:36: // Only allow swipe up to enter overview, down to exit. Ignore extra swiping nit: update documentation and probably just merge this with the documentation on line 41 https://codereview.chromium.org/2667293002/diff/60001/ash/wm/gestures/overvie... ash/wm/gestures/overview_gesture_handler.cc:49: return false; nit: newline https://codereview.chromium.org/2667293002/diff/60001/ash/wm/gestures/overvie... File ash/wm/gestures/overview_gesture_handler.h (right): https://codereview.chromium.org/2667293002/diff/60001/ash/wm/gestures/overvie... ash/wm/gestures/overview_gesture_handler.h:37: static float vertical_threshold_pixels_; I'm not sure why you have made these members static. Can't they just be constants? https://codereview.chromium.org/2667293002/diff/60001/ash/wm/gestures/overvie... File ash/wm/gestures/overview_gesture_handler_unittest.cc (right): https://codereview.chromium.org/2667293002/diff/60001/ash/wm/gestures/overvie... ash/wm/gestures/overview_gesture_handler_unittest.cc:78: // Tests a swipe up with three fingers to enter overview and a swipe right or The OverviewGestureHandlerTest.VerticalSwipes test already covers the case of 'three-finger scroll up/down will enter/exit overview mode'. In this test, consider engaging overview mode by some other means to make it clear that the horizontal n-finger scrolling while in overview mode does not have the vertical 3-finger scroll as a prerequisite. https://codereview.chromium.org/2667293002/diff/60001/ash/wm/gestures/overvie... ash/wm/gestures/overview_gesture_handler_unittest.cc:119: // Tests that a mostly horizontal swipe does not trigger overview. nit: while you're here it would be good to reword 'swipe' as you have done elsewhere so that all documentation is consistent.
Patchset #3 (id:80001) has been deleted
The CQ bit was checked by varkha@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 ========== [ash-md] Adds support for gesture to move selection in overview mode Make it possible to use overview mode without getting fingers off the touchpad. Swipe up with 3 fingers enters overview mode. Swipe right or left with 2 or 3 fingers moves selection. Swipe down dismisses overview mode if no selection was made. If however the selection was moved with a horizontal swipe, swipe down will accept this selection making a new window active. BUG=687773 TEST=OverviewGestureHandlerTest.VerticalThenHorizontalSwipe ========== to ========== [ash-md] Adds support for gesture to move selection in overview mode Make it possible to use overview mode without getting fingers off the touchpad. Swipe up with 3 fingers enters overview mode. Swipe right or left with 3 fingers moves selection. Swipe down dismisses overview mode if no selection was made. If however the selection was moved with a horizontal swipe, swipe down will accept this selection making a new window active. BUG=687773 TEST=OverviewGestureHandlerTest.VerticalThenHorizontalSwipe ==========
Description was changed from ========== [ash-md] Adds support for gesture to move selection in overview mode Make it possible to use overview mode without getting fingers off the touchpad. Swipe up with 3 fingers enters overview mode. Swipe right or left with 3 fingers moves selection. Swipe down dismisses overview mode if no selection was made. If however the selection was moved with a horizontal swipe, swipe down will accept this selection making a new window active. BUG=687773 TEST=OverviewGestureHandlerTest.VerticalThenHorizontalSwipe ========== to ========== [ash-md] Adds support for gesture to move selection in overview mode Make it possible to use overview mode without getting fingers off the touchpad. Scroll up with 3 fingers enters overview mode. Scroll right or left with 3 fingers moves selection. Scroll down dismisses overview mode if no selection was made. If however the selection was moved with a horizontal scroll, scroll down will accept this selection making a new window active. BUG=687773 TEST=OverviewGestureHandlerTest.VerticalThenHorizontalSwipe ==========
Description was changed from ========== [ash-md] Adds support for gesture to move selection in overview mode Make it possible to use overview mode without getting fingers off the touchpad. Scroll up with 3 fingers enters overview mode. Scroll right or left with 3 fingers moves selection. Scroll down dismisses overview mode if no selection was made. If however the selection was moved with a horizontal scroll, scroll down will accept this selection making a new window active. BUG=687773 TEST=OverviewGestureHandlerTest.VerticalThenHorizontalSwipe ========== to ========== [ash-md] Adds support for gesture to move selection in overview mode Make it possible to use overview mode without getting fingers off the touchpad. Scroll up with 3 fingers enters overview mode. Scroll right or left with 3 fingers moves selection. Scroll down dismisses overview mode if no selection was made. If however the selection was moved with a horizontal scroll, scroll down will accept this selection making a new window active. BUG=687773 TEST=OverviewGestureHandlerTest.HorizontalScrollInOverview ==========
https://codereview.chromium.org/2667293002/diff/60001/ash/wm/gestures/overvie... File ash/wm/gestures/overview_gesture_handler.cc (right): https://codereview.chromium.org/2667293002/diff/60001/ash/wm/gestures/overvie... ash/wm/gestures/overview_gesture_handler.cc:28: (event.finger_count() != 2 && event.finger_count() != 3)) { On 2017/02/08 23:14:19, tdanderson wrote: > If you're going to start permitting two-finger scrolling to move the selection > widget, given that the grid is two-dimensional, I would expect two-finger > horizontal scrolls to be able to move the selection widget up/down. I think the > general model of two-finger scrolling is that it works in both dimensions (think > two-finger touchpad scrolling of a webpage); do you know of any counterexamples > to this in our UI? > > The only other three-finger scrolling action I can think of is tab scrubbing in > Ash, and that only works in a single dimension, so I see no inconsistencies with > how you're making use of the three-finger scroll gesture in this CL. Convinced - going back to requiring 3-finger gestures to scrub and enter / exit. https://codereview.chromium.org/2667293002/diff/60001/ash/wm/gestures/overvie... ash/wm/gestures/overview_gesture_handler.cc:36: // Only allow swipe up to enter overview, down to exit. Ignore extra swiping On 2017/02/08 23:14:19, tdanderson wrote: > nit: update documentation and probably just merge this with the documentation on > line 41 Done. https://codereview.chromium.org/2667293002/diff/60001/ash/wm/gestures/overvie... ash/wm/gestures/overview_gesture_handler.cc:49: return false; On 2017/02/08 23:14:19, tdanderson wrote: > nit: newline Done. https://codereview.chromium.org/2667293002/diff/60001/ash/wm/gestures/overvie... File ash/wm/gestures/overview_gesture_handler.h (right): https://codereview.chromium.org/2667293002/diff/60001/ash/wm/gestures/overvie... ash/wm/gestures/overview_gesture_handler.h:37: static float vertical_threshold_pixels_; On 2017/02/08 23:14:19, tdanderson wrote: > I'm not sure why you have made these members static. Can't they just be > constants? I feel that initialization in cc and outside of ctor hides the values better. Added const. https://codereview.chromium.org/2667293002/diff/60001/ash/wm/gestures/overvie... File ash/wm/gestures/overview_gesture_handler_unittest.cc (right): https://codereview.chromium.org/2667293002/diff/60001/ash/wm/gestures/overvie... ash/wm/gestures/overview_gesture_handler_unittest.cc:78: // Tests a swipe up with three fingers to enter overview and a swipe right or On 2017/02/08 23:14:19, tdanderson wrote: > The OverviewGestureHandlerTest.VerticalSwipes test already covers the case of > 'three-finger scroll up/down will enter/exit overview mode'. In this test, > consider engaging overview mode by some other means to make it clear that the > horizontal n-finger scrolling while in overview mode does not have the vertical > 3-finger scroll as a prerequisite. Done. https://codereview.chromium.org/2667293002/diff/60001/ash/wm/gestures/overvie... ash/wm/gestures/overview_gesture_handler_unittest.cc:119: // Tests that a mostly horizontal swipe does not trigger overview. On 2017/02/08 23:14:19, tdanderson wrote: > nit: while you're here it would be good to reword 'swipe' as you have done > elsewhere so that all documentation is consistent. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I've made the horizontal threshold less sensitive (330 dp seemed most comfortable) and also I am now skipping the first item but only the first time when the selector appears on screen. Seems to be an improvement. PTAL.
The CQ bit was checked by varkha@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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by varkha@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 varkha@chromium.org
Patchset #4 (id:120001) has been deleted
The CQ bit was checked by varkha@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.
Patchset #4 (id:140001) has been deleted
The CQ bit was checked by varkha@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.
https://codereview.chromium.org/2667293002/diff/160001/ash/common/wm/overview... File ash/common/wm/overview/window_selector.cc (right): https://codereview.chromium.org/2667293002/diff/160001/ash/common/wm/overview... ash/common/wm/overview/window_selector.cc:275: if (first_grid) { I'm not 100% convinced this is the right thing to do, actually - what's your reasoning for skipping the first item? Is this to be consistent with Alt+Tab? There may be a user expectation (especially from an a11y point of view) to be able to cycle through the MRU beginning with the current window. https://codereview.chromium.org/2667293002/diff/160001/ash/wm/gestures/overvi... File ash/wm/gestures/overview_gesture_handler.h (right): https://codereview.chromium.org/2667293002/diff/160001/ash/wm/gestures/overvi... ash/wm/gestures/overview_gesture_handler.h:30: // The total distance scrolled with 2 or three fingers up to the point when nit: update to just "three fingers" https://codereview.chromium.org/2667293002/diff/160001/ash/wm/gestures/overvi... File ash/wm/gestures/overview_gesture_handler_unittest.cc (right): https://codereview.chromium.org/2667293002/diff/160001/ash/wm/gestures/overvi... ash/wm/gestures/overview_gesture_handler_unittest.cc:125: // Tests that a mostly horizontal scroll does not trigger overview. nit: three-finger scroll
I have rolled back the skipping first item change - it might be too controversial to look at now. https://codereview.chromium.org/2667293002/diff/160001/ash/common/wm/overview... File ash/common/wm/overview/window_selector.cc (right): https://codereview.chromium.org/2667293002/diff/160001/ash/common/wm/overview... ash/common/wm/overview/window_selector.cc:275: if (first_grid) { On 2017/02/10 16:05:19, tdanderson wrote: > I'm not 100% convinced this is the right thing to do, actually - what's your > reasoning for skipping the first item? Is this to be consistent with Alt+Tab? > There may be a user expectation (especially from an a11y point of view) to be > able to cycle through the MRU beginning with the current window. Yes, I'll take this offline. https://codereview.chromium.org/2667293002/diff/160001/ash/wm/gestures/overvi... File ash/wm/gestures/overview_gesture_handler.h (right): https://codereview.chromium.org/2667293002/diff/160001/ash/wm/gestures/overvi... ash/wm/gestures/overview_gesture_handler.h:30: // The total distance scrolled with 2 or three fingers up to the point when On 2017/02/10 16:05:19, tdanderson wrote: > nit: update to just "three fingers" Done. https://codereview.chromium.org/2667293002/diff/160001/ash/wm/gestures/overvi... File ash/wm/gestures/overview_gesture_handler_unittest.cc (right): https://codereview.chromium.org/2667293002/diff/160001/ash/wm/gestures/overvi... ash/wm/gestures/overview_gesture_handler_unittest.cc:125: // Tests that a mostly horizontal scroll does not trigger overview. On 2017/02/10 16:05:19, tdanderson wrote: > nit: three-finger scroll Done.
The CQ bit was checked by varkha@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...
Patch Set 5 LGTM
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 varkha@chromium.org
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": 180001, "attempt_start_ts": 1486754057601820,
"parent_rev": "41536401d4edd0ed574ca771532b6f5f49c8fc7c", "commit_rev":
"525b0d69b50807ab7c9901f52acf858c0e5e1ba8"}
Message was sent while issue was closed.
Description was changed from ========== [ash-md] Adds support for gesture to move selection in overview mode Make it possible to use overview mode without getting fingers off the touchpad. Scroll up with 3 fingers enters overview mode. Scroll right or left with 3 fingers moves selection. Scroll down dismisses overview mode if no selection was made. If however the selection was moved with a horizontal scroll, scroll down will accept this selection making a new window active. BUG=687773 TEST=OverviewGestureHandlerTest.HorizontalScrollInOverview ========== to ========== [ash-md] Adds support for gesture to move selection in overview mode Make it possible to use overview mode without getting fingers off the touchpad. Scroll up with 3 fingers enters overview mode. Scroll right or left with 3 fingers moves selection. Scroll down dismisses overview mode if no selection was made. If however the selection was moved with a horizontal scroll, scroll down will accept this selection making a new window active. BUG=687773 TEST=OverviewGestureHandlerTest.HorizontalScrollInOverview Review-Url: https://codereview.chromium.org/2667293002 Cr-Commit-Position: refs/heads/master@{#449687} Committed: https://chromium.googlesource.com/chromium/src/+/525b0d69b50807ab7c9901f52acf... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:180001) as https://chromium.googlesource.com/chromium/src/+/525b0d69b50807ab7c9901f52acf... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
