GNOME Bugzilla – Bug 646001
wifi network list can extend off the bottom of the screen
Last modified: 2012-06-28 13:38:58 UTC
In some places (like my house), you can see enough wifi networks that the network menu will extend off the bottom of the screen when "More" is expanded. We need to do something about that. Possibilities: - Scrolling - An "Even more >" submenu - Some indication that the list has been truncated, and that the full list of networks is available in the network settings panel
Created attachment 184946 [details] [review] popupMenu: make submenus scrollable if needed Here's a "desparation measure" fix taking advantage of the fact that our menus use standard boxes and packing. It's certainly not ideal, but better than overflowing, I think. I think. ==== Right now, the network menu will overflow the screen if More... is selected with many access points. As a short-term workaround for this, add a scrollbar for submenus of panel dropdown menus if they would cause the toplevel menu to overflow the screen. - Put the actors in a PopupSubMenu in a StScrollView so we get a scrollbar if the allocated space is smaller than the height of the menu. - When we pop up a panel menu, set a max-height style property on the panel menu to limit it to the height of the screen. - Hack event handling while the scrollbar is dragged to make the scrollbar work properly.
Created attachment 184947 [details] Screenshot of "fix"
Created attachment 184959 [details] [review] popupMenu: make submenus scrollable if needed Last version had some issues: - Animation was screwy both when the menu fit and when it didn't - Sub-menus that fit still got space for the vertical scrollbar causing the toplevel to jump wider when you opened the expander. So in this case, we leave the mode of the scroll-view as NEVER/NEVER unless we actually need the scrollbar when we open the menu. This doesn't properly handle changes while the menu is open (from access point rescanning, perhaps) but there's a limit to how complex I want to make this. Part of the reason this new one is longer is reindentation of existing code for animating open.
Comment on attachment 184959 [details] [review] popupMenu: make submenus scrollable if needed i don't see anything obviously wrong with the code... (I'll try to test it some more tomorrow.)
(In reply to comment #4) > (From update of attachment 184959 [details] [review]) > i don't see anything obviously wrong with the code... > > (I'll try to test it some more tomorrow.) That would be appreciated, and if Giovanni or Florian would do a second look at the code that would also be appreciated. I'd like to get this in for 3.0.0 because running off the screen just looks bad and people may not be able to find the workaround of selecting the network in Network Settings.
Review of attachment 184959 [details] [review]: ::: js/ui/popupMenu.js @@ -1062,2 @@ this.actor.show(); - let [naturalHeight, minHeight] = this.actor.get_preferred_height(-1); Here we have "natural, min ="... @@ +1123,3 @@ + + if (animate) { + let [minHeight, naturalHeight] = this.actor.get_preferred_height(-1); ... and here we have "min, natural =". Sounds weird to me.
(In reply to comment #6) > Review of attachment 184959 [details] [review]: > > ::: js/ui/popupMenu.js > @@ -1062,2 @@ > this.actor.show(); > - let [naturalHeight, minHeight] = this.actor.get_preferred_height(-1); > > Here we have "natural, min ="... > > @@ +1123,3 @@ > + > + if (animate) { > + let [minHeight, naturalHeight] = > this.actor.get_preferred_height(-1); > > ... and here we have "min, natural =". > > Sounds weird to me. Indeed. But it's the original code which gets it wrong :-)
Comment on attachment 184959 [details] [review] popupMenu: make submenus scrollable if needed It seems to work. It's very subtle... we might want to color the scrollbar more obviously? I am pretty sure I would never notice the scrollbar if I didn't already know it was there. (This may be the monitors-with-different gamma issue again.) >As a short-term workaround Is the patch 100% short-term workaround, or are there any parts of it you expect to keep? >+ // StScrollbar plays dirty tricks with events, calling >+ // clutter_set_motion_events_enabled (FALSE) during the scroll; this >+ // confuses our event tracking, so we just turn it off during the >+ // scroll. Not for 3.0.0, but the StScrollbar code seems totally wrong; it should be using clutter_grab_pointer(), not stage.connect('captured-event'), which would then make the set_motion_events_enabled call unnecessary, which would make passEvents unnecessary.
depends on the patch in bug 645949
(In reply to comment #8) > (From update of attachment 184959 [details] [review]) > It seems to work. It's very subtle... we might want to color the scrollbar more > obviously? I am pretty sure I would never notice the scrollbar if I didn't > already know it was there. (This may be the monitors-with-different gamma issue > again.) > > >As a short-term workaround > > Is the patch 100 [details]% short-term workaround, or are there any parts of it you > expect to keep? Not sure really. I don't think it's a horrible approach other than the visual murkiness of the scrollbar. Espciailly if the user sometimes needs to scan through the whole list. I'm not sure it's great to look at one list of networks, then need to hop over to Network Settings... and start from scratch with a GtkCombobox dropdown. If we don't want something like this, my instinct would be to say we should have a "Connect to new network" dialog and only have actually previously connected to items in the main menu. > >+ // StScrollbar plays dirty tricks with events, calling > >+ // clutter_set_motion_events_enabled (FALSE) during the scroll; this > >+ // confuses our event tracking, so we just turn it off during the > >+ // scroll. > > Not for 3.0.0, but the StScrollbar code seems totally wrong; it should be using > clutter_grab_pointer(), not stage.connect('captured-event'), which would then > make the set_motion_events_enabled call unnecessary, which would make > passEvents unnecessary. Seems like that would work in the longer-term-can-rewrite-event-handling future.
Review of attachment 184959 [details] [review]: Code looks reasonable to me as well - one issue I noticed in testing is missing scroll adjustment when navigating the menu with arrow keys (not a blocker, but something to keep in mind for 3.0.x). ::: js/ui/popupMenu.js @@ +1050,3 @@ + // Since a function of a submenu might be to provide a "More.." expander, + // we make it scrollable - the scrollbar will only take affect if + // the height is constrained, by, e.g., the max-height of the top menu. Is this comment really true? _needsScrollbar() explicitly checks max-height, so I don't see how it would work with other height constrains.
Created attachment 185106 [details] [review] popup-sub-menu: Adjust scrollbar style Slightly adjust the style, so that scrollbars in submenus look slightly less disconnected from the scrolled content. This is a try to better connect the scrollbar with the content - I think it's an improvement, though I'm not entirely happy with it; not sure there's time to handle it to Jakub/Jon, who should be able to do better than me :-) (The patch is intended for squashing, so I didn't try to come up with a good commit message)
Created attachment 185108 [details] Screenshot with Florian's CSS change
Created attachment 185110 [details] [review] popup-sub-menu: Adjust scrollbar style Added some padding on request of Jakub.
Created attachment 185120 [details] [review] popup-menu: Tweak submenu style Tone down the background color and use an inset shadow to give the subsection some depth.
Created attachment 185121 [details] [review] popup-sub-menu: Adjust scrollbar style Adjusted for Jakub's style tweaks, and some padding adjustments which came up during design review on IRC.
Review of attachment 185120 [details] [review]: Good. commit message should possibly mention "Expander menu item is always highlighted when open."
Review of attachment 185121 [details] [review]: CSS only change, safe and things look considerably better with this. Have OK from docs/marketing teams that CSS changes here won't cause problems. There's a repaint bug that's exposed (not introduced) by this change in RTL mode, but I consider the improvement in LTR to be more important than a small visual glitch in RTL mode. We'll get a fix for the glitch in for 3.0.1.
Attachment 184959 [details] pushed as 50951d1 - popupMenu: make submenus scrollable if needed Attachment 185120 [details] pushed as a7df1a3 - popup-menu: Tweak submenu style Attachment 185121 [details] pushed as d25418b - popup-sub-menu: Adjust scrollbar style
*** Bug 678934 has been marked as a duplicate of this bug. ***