Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12660016:
Make long press & release to trigger context menu on tab (Closed)
|
Created:
7 years, 9 months ago by Yufeng Shen (Slow to review) Modified:
7 years, 9 months ago CC:
chromium-reviews, tfarina Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionMake long tap to trigger context menu on tab
Showing context menu after long tap (long press & finger release) on tab.
BUG=170693
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=190111
Patch Set 1 #
Total comments: 2
Patch Set 2 : long press - immdediately show context menu in non-stacking mode; delay until finger lift in stacki… #Patch Set 3 : showing context menu after long press & finger lift (long tap) #
Total comments: 2
Patch Set 4 : fix for comments #
Total comments: 2
Patch Set 5 : addressed sky's comments #
Total comments: 2
Patch Set 6 : EndDrag() on LONG_TAP to be safe #Messages
Total messages: 27 (0 generated)
hey sky, could you please take a look at this ?
https://codereview.chromium.org/12660016/diff/1/chrome/browser/ui/views/tabs/... File chrome/browser/ui/views/tabs/tab_strip.cc (right): https://codereview.chromium.org/12660016/diff/1/chrome/browser/ui/views/tabs/... chrome/browser/ui/views/tabs/tab_strip.cc:1512: ShowContextMenuForTab(tab, local_point); The finger may have moved a significant distance after the long-press event. I don't think we should show the context menu in such scenario. You should should the context menu on GESTURE_LONG_TAP gesture instead. (so you won't need any of the new states, i.e. was_dragging_, had_gesture_long_press_ or last_long_press_location)
https://codereview.chromium.org/12660016/diff/1/chrome/browser/ui/views/tabs/... File chrome/browser/ui/views/tabs/tab_strip.cc (right): https://codereview.chromium.org/12660016/diff/1/chrome/browser/ui/views/tabs/... chrome/browser/ui/views/tabs/tab_strip.cc:1512: ShowContextMenuForTab(tab, local_point); On 2013/03/19 22:17:31, sadrul wrote: > The finger may have moved a significant distance after the long-press event. I > don't think we should show the context menu in such scenario. > If the finger has moved a significant distance, then it would have triggered ui::ET_GESTURE_SCROLL_UPDATE and marked as dragging I think. > You should should the context menu on GESTURE_LONG_TAP gesture instead. (so you > won't need any of the new states, i.e. was_dragging_, had_gesture_long_press_ or > last_long_press_location) The tricky part is that we currently use GESTURE_LONG_PRESS as switch between dragging and scrolling mode on the tab strip (see issue 161634). So showing context menu after long press but before finger lift would somehow make the whole thing even more confusing.
On 2013/03/19 23:37:38, Yufeng Shen wrote: > https://codereview.chromium.org/12660016/diff/1/chrome/browser/ui/views/tabs/... > File chrome/browser/ui/views/tabs/tab_strip.cc (right): > > https://codereview.chromium.org/12660016/diff/1/chrome/browser/ui/views/tabs/... > chrome/browser/ui/views/tabs/tab_strip.cc:1512: ShowContextMenuForTab(tab, > local_point); > > > On 2013/03/19 22:17:31, sadrul wrote: > > The finger may have moved a significant distance after the long-press event. I > > don't think we should show the context menu in such scenario. > > > > If the finger has moved a significant distance, then it would have triggered > ui::ET_GESTURE_SCROLL_UPDATE and marked as dragging I think. > > > You should should the context menu on GESTURE_LONG_TAP gesture instead. (so > you > > won't need any of the new states, i.e. was_dragging_, had_gesture_long_press_ > or > > last_long_press_location) > > The tricky part is that we currently use GESTURE_LONG_PRESS > as switch between dragging and scrolling mode on the tab strip (see issue > 161634). So showing context menu after long press but before finger lift would > somehow make the whole thing even more confusing. LOG_TAP happens after finger-lift.
Long press is only going to matter when the tabs are actually stacked (see https://codereview.chromium.org/12886030/ ). So, you could show on long press if not stacked, but you'll have to wait for release if stacked. On Tue, Mar 19, 2013 at 4:37 PM, <miletus@chromium.org> wrote: > > https://codereview.chromium.**org/12660016/diff/1/chrome/** > browser/ui/views/tabs/tab_**strip.cc<https://codereview.chromium.org/12660016/diff/1/chrome/browser/ui/views/tabs/tab_strip.cc> > File chrome/browser/ui/views/tabs/**tab_strip.cc (right): > > https://codereview.chromium.**org/12660016/diff/1/chrome/** > browser/ui/views/tabs/tab_**strip.cc#newcode1512<https://codereview.chromium.org/12660016/diff/1/chrome/browser/ui/views/tabs/tab_strip.cc#newcode1512> > chrome/browser/ui/views/tabs/**tab_strip.cc:1512: > ShowContextMenuForTab(tab, local_point); > > > On 2013/03/19 22:17:31, sadrul wrote: > >> The finger may have moved a significant distance after the long-press >> > event. I > >> don't think we should show the context menu in such scenario. >> > > > If the finger has moved a significant distance, then it would have > triggered ui::ET_GESTURE_SCROLL_UPDATE and marked as dragging I think. > > > You should should the context menu on GESTURE_LONG_TAP gesture >> > instead. (so you > >> won't need any of the new states, i.e. was_dragging_, >> > had_gesture_long_press_ or > >> last_long_press_location) >> > > The tricky part is that we currently use GESTURE_LONG_PRESS > as switch between dragging and scrolling mode on the tab strip (see > issue 161634). So showing context menu after long press but before > finger lift would somehow make the whole thing even more confusing. > > https://codereview.chromium.**org/12660016/<https://codereview.chromium.org/1... >
Make this happen: In non-stacking mode, context menu shows immediately after long press. In stacking mode, delay showing the context menu until finger lift if there was no dragging.
On 2013/03/20 01:45:44, Yufeng Shen wrote: > Make this happen: > > In non-stacking mode, context menu shows immediately after long press. > In stacking mode, delay showing the context menu until finger lift > if there was no dragging. We discussed offline, and decided that showing context-menu always on LONG_TAP makes most sense (changing the behaviour of long-press/tap based on the stacking-mode is likely going to be confusing).
showing context menu after long press & finger lift (long tap)
The code in OnGestureEvent LG after this: https://codereview.chromium.org/12660016/diff/10002/chrome/browser/ui/views/t... File chrome/browser/ui/views/tabs/tab_strip.cc (right): https://codereview.chromium.org/12660016/diff/10002/chrome/browser/ui/views/t... chrome/browser/ui/views/tabs/tab_strip.cc:1517: } Missing a break; here
https://codereview.chromium.org/12660016/diff/10002/chrome/browser/ui/views/t... File chrome/browser/ui/views/tabs/tab_strip.cc (right): https://codereview.chromium.org/12660016/diff/10002/chrome/browser/ui/views/t... chrome/browser/ui/views/tabs/tab_strip.cc:1517: } On 2013/03/20 02:15:52, sadrul wrote: > Missing a break; here Done.
https://codereview.chromium.org/12660016/diff/11002/chrome/browser/ui/views/t... File chrome/browser/ui/views/tabs/tab_strip.cc (right): https://codereview.chromium.org/12660016/diff/11002/chrome/browser/ui/views/t... chrome/browser/ui/views/tabs/tab_strip.cc:2506: Tab* tab = NULL; Nuke 'tab' and return where necessary, eg 2514 should return as should 2519.
https://codereview.chromium.org/12660016/diff/11002/chrome/browser/ui/views/t... File chrome/browser/ui/views/tabs/tab_strip.cc (right): https://codereview.chromium.org/12660016/diff/11002/chrome/browser/ui/views/t... chrome/browser/ui/views/tabs/tab_strip.cc:2506: Tab* tab = NULL; On 2013/03/20 14:21:25, sky wrote: > Nuke 'tab' and return where necessary, eg 2514 should return as should 2519. Done.
https://codereview.chromium.org/12660016/diff/18001/chrome/browser/ui/views/t... File chrome/browser/ui/views/tabs/tab_strip.cc (right): https://codereview.chromium.org/12660016/diff/18001/chrome/browser/ui/views/t... chrome/browser/ui/views/tabs/tab_strip.cc:1511: gfx::Point local_point = event->location(); One more question. If we get here, is drag_controller_ still non-null?
https://codereview.chromium.org/12660016/diff/18001/chrome/browser/ui/views/t... File chrome/browser/ui/views/tabs/tab_strip.cc (right): https://codereview.chromium.org/12660016/diff/18001/chrome/browser/ui/views/t... chrome/browser/ui/views/tabs/tab_strip.cc:1511: gfx::Point local_point = event->location(); On 2013/03/20 16:42:08, sky wrote: > One more question. If we get here, is drag_controller_ still non-null? I assume if you get LONG_TAP, it means you did not drag during press, so drag_controller_ was never created ?
Can you verify that. I'm worried since menus run a nested message loop and I would rather destroy drag_controller_ before running a nested message loop. On Wed, Mar 20, 2013 at 10:18 AM, <miletus@chromium.org> wrote: > > https://codereview.chromium.**org/12660016/diff/18001/** > chrome/browser/ui/views/tabs/**tab_strip.cc<https://codereview.chromium.org/12660016/diff/18001/chrome/browser/ui/views/tabs/tab_strip.cc> > File chrome/browser/ui/views/tabs/**tab_strip.cc (right): > > https://codereview.chromium.**org/12660016/diff/18001/** > chrome/browser/ui/views/tabs/**tab_strip.cc#newcode1511<https://codereview.chromium.org/12660016/diff/18001/chrome/browser/ui/views/tabs/tab_strip.cc#newcode1511> > chrome/browser/ui/views/tabs/**tab_strip.cc:1511: gfx::Point local_point = > event->location(); > On 2013/03/20 16:42:08, sky wrote: > >> One more question. If we get here, is drag_controller_ still non-null? >> > > I assume if you get LONG_TAP, it means you did not drag during press, so > drag_controller_ was never created ? > > https://codereview.chromium.**org/12660016/<https://codereview.chromium.org/1... >
On 2013/03/20 17:26:17, sky wrote: > Can you verify that. I'm worried since menus run a nested message loop and > I would rather destroy drag_controller_ before running a nested message > loop. > To be safe, added EndDrag() on LONG_TAP in case drag_controller_ was ever created. > > On Wed, Mar 20, 2013 at 10:18 AM, <mailto:miletus@chromium.org> wrote: > > > > > https://codereview.chromium.**org/12660016/diff/18001/** > > > chrome/browser/ui/views/tabs/**tab_strip.cc<https://codereview.chromium.org/12660016/diff/18001/chrome/browser/ui/views/tabs/tab_strip.cc> > > File chrome/browser/ui/views/tabs/**tab_strip.cc (right): > > > > https://codereview.chromium.**org/12660016/diff/18001/** > > > chrome/browser/ui/views/tabs/**tab_strip.cc#newcode1511<https://codereview.chromium.org/12660016/diff/18001/chrome/browser/ui/views/tabs/tab_strip.cc#newcode1511> > > chrome/browser/ui/views/tabs/**tab_strip.cc:1511: gfx::Point local_point = > > event->location(); > > On 2013/03/20 16:42:08, sky wrote: > > > >> One more question. If we get here, is drag_controller_ still non-null? > >> > > > > I assume if you get LONG_TAP, it means you did not drag during press, so > > drag_controller_ was never created ? > > > > > https://codereview.chromium.**org/12660016/%3Chttps://codereview.chromium.org...> > >
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/miletus@chromium.org/12660016/17002
Retried try job too often on win_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/miletus@chromium.org/12660016/17002
Retried try job too often on win7_aura for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/miletus@chromium.org/12660016/17002
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/miletus@chromium.org/12660016/17002
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/miletus@chromium.org/12660016/17002
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/miletus@chromium.org/12660016/17002
Message was sent while issue was closed.
Change committed as 190111 |
