After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 646001 - wifi network list can extend off the bottom of the screen
wifi network list can extend off the bottom of the screen
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: network-indicator
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
: 678934 (view as bug list)
Depends on: 645949
Blocks:
 
 
Reported: 2011-03-28 19:16 UTC by Dan Winship
Modified: 2012-06-28 13:38 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
popupMenu: make submenus scrollable if needed (4.84 KB, patch)
2011-04-02 16:42 UTC, Owen Taylor
none Details | Review
Screenshot of "fix" (43.32 KB, image/png)
2011-04-02 16:45 UTC, Owen Taylor
  Details
popupMenu: make submenus scrollable if needed (8.58 KB, patch)
2011-04-02 17:55 UTC, Owen Taylor
committed Details | Review
popup-sub-menu: Adjust scrollbar style (2.26 KB, patch)
2011-04-04 13:21 UTC, Florian Müllner
none Details | Review
Screenshot with Florian's CSS change (50.06 KB, image/png)
2011-04-04 13:57 UTC, Owen Taylor
  Details
popup-sub-menu: Adjust scrollbar style (2.32 KB, patch)
2011-04-04 14:31 UTC, Florian Müllner
none Details | Review
popup-menu: Tweak submenu style (1.05 KB, patch)
2011-04-04 16:19 UTC, Florian Müllner
committed Details | Review
popup-sub-menu: Adjust scrollbar style (2.50 KB, patch)
2011-04-04 16:20 UTC, Florian Müllner
committed Details | Review

Description Dan Winship 2011-03-28 19:16:21 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
Comment 1 Owen Taylor 2011-04-02 16:42:54 UTC
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.
Comment 2 Owen Taylor 2011-04-02 16:45:40 UTC
Created attachment 184947 [details]
Screenshot of "fix"
Comment 3 Owen Taylor 2011-04-02 17:55:36 UTC
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 4 Dan Winship 2011-04-02 21:47:07 UTC
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.)
Comment 5 Owen Taylor 2011-04-03 02:11:29 UTC
(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.
Comment 6 Vincent Untz 2011-04-03 08:00:58 UTC
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.
Comment 7 Florian Müllner 2011-04-03 21:25:49 UTC
(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 8 Dan Winship 2011-04-04 00:30:05 UTC
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.
Comment 9 Dan Winship 2011-04-04 00:49:33 UTC
depends on the patch in bug 645949
Comment 10 Owen Taylor 2011-04-04 04:40:32 UTC
(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.
Comment 11 Florian Müllner 2011-04-04 13:15:50 UTC
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.
Comment 12 Florian Müllner 2011-04-04 13:21:38 UTC
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)
Comment 13 Owen Taylor 2011-04-04 13:57:45 UTC
Created attachment 185108 [details]
Screenshot with Florian's CSS change
Comment 14 Florian Müllner 2011-04-04 14:31:43 UTC
Created attachment 185110 [details] [review]
popup-sub-menu: Adjust scrollbar style

Added some padding on request of Jakub.
Comment 15 Florian Müllner 2011-04-04 16:19:00 UTC
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.
Comment 16 Florian Müllner 2011-04-04 16:20:16 UTC
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.
Comment 17 Owen Taylor 2011-04-04 19:12:29 UTC
Review of attachment 185120 [details] [review]:

Good. commit message should possibly mention "Expander menu item is always highlighted when open."
Comment 18 Owen Taylor 2011-04-04 19:17:05 UTC
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.
Comment 19 Owen Taylor 2011-04-04 19:58:20 UTC
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
Comment 20 Jiri Klimes 2012-06-28 13:38:58 UTC
*** Bug 678934 has been marked as a duplicate of this bug. ***