GNOME Bugzilla – Bug 678113
Improve touch interaction on overflowing menus
Last modified: 2012-07-13 17:30:38 UTC
Created attachment 216429 [details] [review] patch Specially visible on comboboxes close to the upper monitor edge, overflowing menus are a bit funky with touch devices, as soon as you touch to scroll, the whole menu is scrolled to the visible area at once, and when you release, the option that's been scrolled under your finger gets selected as the scrolling arrows disappear. This can be seen for example on fullscreen evince's zoom control. I'm attaching a patch to improve this, scrolling is fixed to work only in the direction towards the displayable area, and no spurious selection happens.
Review of attachment 216429 [details] [review]: "Specially in the case of comboboxes, those menus could enable scrolling even if the contents could fit in the work area" Shouldn't we fix this instead? It's not completely clear to me what this patch does and how it changes the current behavior, but I've seen several times a bug (with normal pointer devices, no touch) where combobox menus that could fit the screen, decide instead not to, and show up as a big half-empty scrolling window. Would this patch fix it? ::: gtk/gtkmenu.c @@ +4314,3 @@ case GDK_TOUCH_UPDATE: case GDK_MOTION_NOTIFY: + if ((!gdk_event_get_state (event, &state) || (state & GDK_BUTTON1_MASK) != 0) && This looks like a coding style fix, so probably should go in a separate patch? @@ +5003,3 @@ + { + gdk_window_move (priv->bin_window, 0, -offset); + gdk_window_move_resize (priv->view_window, x, y, view_width, view_height); Same for this.
(In reply to comment #1) > Review of attachment 216429 [details] [review]: > > "Specially in the case of comboboxes, those menus could enable scrolling even > if the contents could fit in the work area" > > Shouldn't we fix this instead? I believe this has a rationale, this is so the currently selected menu item always stays on top of the combobox, it unfortunately makes enough blank room to fit the items that are offscreen, probably scrolling was considered easier than both moving/scrolling contents, the device position might need to be queried too for scroll control as I don't think you'd get events if the window moves beneath your pointer. The other option would be giving up on showing the selected item on top of the combobox on those situations, but sounds a bit misleading... > It's not completely clear to me what this patch does and how it changes the > current behavior, but I've seen several times a bug (with normal pointer > devices, no touch) where combobox menus that could fit the screen, decide > instead not to, and show up as a big half-empty scrolling window. Would this > patch fix it? No, this fixes a touch specific behavior on that situation, whereas you can usually pan-to-scroll smoothly on a menu with touch devices, in this situation it rather jumps within the screen and enters an inconsistent state that makes you select the currently hovered option on GDK_TOUCH_END > > ::: gtk/gtkmenu.c > @@ +4314,3 @@ > case GDK_TOUCH_UPDATE: > case GDK_MOTION_NOTIFY: > + if ((!gdk_event_get_state (event, &state) || (state & GDK_BUTTON1_MASK) > != 0) && > > This looks like a coding style fix, so probably should go in a separate patch? > > @@ +5003,3 @@ > + { > + gdk_window_move (priv->bin_window, 0, -offset); > + gdk_window_move_resize (priv->view_window, x, y, view_width, > view_height); > > Same for this. I can commit those separately
(In reply to comment #2) > I believe this has a rationale, this is so the currently selected menu item > always stays on top of the combobox, it unfortunately makes enough blank room > to fit the items that are offscreen, probably scrolling was considered easier > than both moving/scrolling contents, the device position might need to be > queried too for scroll control as I don't think you'd get events if the window > moves beneath your pointer. > > The other option would be giving up on showing the selected item on top of the > combobox on those situations, but sounds a bit misleading... I understand the rationale, but I think I also saw a situation/bug where we show a huge blank space *before* the selected item...imagine you have a long list of items in a combobox and your currently selected item is the second in the list; when we show the menu, it should probably look something like --------------------------- | First Item | {{ (selected) Second Item }} | Third Item | | ... | | ... | | ... | | scroll | --------------------------- instead of --------------------------- | scroll | | | | huge white box | | | | | | First Item | {{ (selected) Second Item }} | Third Item | | ... | | ... | | ... | | scroll | --------------------------- Maybe we're just talking about two different things here though :) > No, this fixes a touch specific behavior on that situation, whereas you can > usually pan-to-scroll smoothly on a menu with touch devices, in this situation > it rather jumps within the screen and enters an inconsistent state that makes > you select the currently hovered option on GDK_TOUCH_END Okay, still it's not really obvious to me how exactly those two changes in your patch map to the bug you're solving...since the code here is quite complicated already, could you explain the rationale in the commit message? > I can commit those separately Thanks!
Comment on attachment 216429 [details] [review] patch Removing from the patch queue
Created attachment 218721 [details] [review] patch v2 I've pushed the two style fixes separately, this new patch doesn't have those changes
(In reply to comment #3) > Maybe we're just talking about two different things here though :) We're talking about the same thing :), that's the "blank space" I was referring to, my point is that doing the nifty thing here means either move+resize or resize+scroll the window (depending on the monitor bound the menu hits, and where the blank space lies), and as scrolling happens when you hover top/bottom regions of the menu window and it's dynamically moving, you need to requery the device position each time the window moves underneath to check whether scrolling still needs to continue or stop. Anyway, drifting offtopic :) > > > No, this fixes a touch specific behavior on that situation, whereas you can > > usually pan-to-scroll smoothly on a menu with touch devices, in this situation > > it rather jumps within the screen and enters an inconsistent state that makes > > you select the currently hovered option on GDK_TOUCH_END > > Okay, still it's not really obvious to me how exactly those two changes in your > patch map to the bug you're solving...since the code here is quite complicated > already, could you explain the rationale in the commit message? From the commit message: >"When such thing happens, improve behavior by not having the contents >jump to comply with the content size limit, allowing only to scroll >in the right direction instead" That refers to the second change in the patch, the CLAMP as-is brings the menu entirely onscreen immediately, so the patch accounts for the scroll_offset too. The MIN/MAX are so scrolling is only allowed in the direction that brings the menu onscreen, and it's blocked in the other. "and by not leaving scrolling mode immediately when the arrows disappear." That's the first chunk of the patch, if drag_start_y is >= 0 there's a scroll operation going on. Note that menu scrolling for touch devices happens entirely on captured devices, so if it stops capturing when no scroll arrows are displayed (may happen as you scroll), the GDK_TOUCH_END would be handled by the menu item, so the scrolling operation ends up selecting an item.
(In reply to comment #6) > We're talking about the same thing :), that's the "blank space" I was referring > to, my point is that doing the nifty thing here means either move+resize or > resize+scroll the window (depending on the monitor bound the menu hits, and > where the blank space lies), and as scrolling happens when you hover top/bottom > regions of the menu window and it's dynamically moving, you need to requery the > device position each time the window moves underneath to check whether > scrolling still needs to continue or stop. Anyway, drifting offtopic :) I understand what you mean now, yeah...tricky. I still think we should do something better than showing blank space, but that's a separate bug then. > From the commit message: > >"When such thing happens, improve behavior by not having the contents > >jump to comply with the content size limit, allowing only to scroll > >in the right direction instead" > > That refers to the second change in the patch, the CLAMP as-is brings the menu > entirely onscreen immediately, so the patch accounts for the scroll_offset too. > The MIN/MAX are so scrolling is only allowed in the direction that brings the > menu onscreen, and it's blocked in the other. > > "and by not leaving scrolling mode immediately when the arrows disappear." > > That's the first chunk of the patch, if drag_start_y is >= 0 there's a scroll > operation going on. Note that menu scrolling for touch devices happens entirely > on captured devices, so if it stops capturing when no scroll arrows are > displayed (may happen as you scroll), the GDK_TOUCH_END would be handled by the > menu item, so the scrolling operation ends up selecting an item. I don't think I have any hardware to test the patch with at this point...if it improves things on touch and doesn't change the behavior for regular mouse devices, feel free to push...it would be useful for future reference if you could integrate this comment in the commit message. Thanks!
I've reworded the commit message and pushed, thanks! commit bd3ca2b30efc534f8b7c18dfd8a9f072592044c7 Author: Carlos Garnacho <carlos@lanedo.com> Date: Fri Jul 13 15:03:30 2012 +0200 menu: Fix touch scrolling on menus close to the monitor edge Specially in the case of comboboxes, those menus could enable scrolling even if the contents could fit in the work area, and could show blank space in order to line up the selected item with the combobox. When such thing happens, take into account scroll_offset when relocating the menu contents so contents don't jump directly onscreen, and apply it so scrolling is allowed in the direction that brings the menu onscreen and blocked in the opposite direction. Also, wait for cancelling the scroll operation until the touch is released even if the scrolling arrows disappeared, so the menu item underneath isn't selected right away. https://bugzilla.gnome.org/show_bug.cgi?id=678113