Having menu appearance change when you press Alt adds another form of busyness.

If anything, this can be justified as a platform setting, e.g. to match win32 behaviour.

(In reply to comment #1)
> Having menu appearance change when you press Alt adds another form of busyness.

true, but it's responding to a user-initiated action (pressing Alt) instead of being there all the time.

IANAUID (I am not a user interaction designer) applies, but I had this discussion with one who asked me to file this bug.

> If anything, this can be justified as a platform setting, e.g. to match win32
> behaviour.

maybe as a theme option? I'd rather have this on linux-based platforms as well, like moblin. :-)

You can change settings in a theme. 'Platform setting' is more a categorization by intended use.

FWIW, I am pretty sure we have bugs about this feature somewhere. 
Have you looked ?

(In reply to comment #3)
> FWIW, I am pretty sure we have bugs about this feature somewhere. 
> Have you looked ?
 
I searched some keywords (menu, accelerator) but I could not find anything.

apparently, searching for "menu" worked: bug 581339 asks for the ability to disable (and optionally enable on Alt key-press) the underline.

*** Bug 581339 has been marked as a duplicate of this bug. ***

+1

Makes the accelerator feature more user-discoverable, as it will react to user input (pressing ALT key). Showing the accelerator all the time tends to make it invisible, as users tend to ignore visual hints they don't understand when continuously exposed to it.

I agree we should get rid of this. 

The visual clutter we have now in the menu is really bad. Plus, a it's an advanced user option and it pops up when hitting Alt anyway if we make the proposed change.

It doesn't have to be a hardcoded change. A gtkrc option would be most welcome, :)

"Makes the accelerator feature more user-discoverable, as it will react to user
input (pressing ALT key). Showing the accelerator all the time tends to make it
invisible, as users tend to ignore visual hints they don't understand when
continuously exposed to it."

I totally agree with Henrique here.

Hey Emmanuele!  Sounds like a nice change to me.  Have a patch for it?

From an accessibility point of view, I'm not sure how good or bad this will be.  People with cognitive impairments may like it because it will give them a chance to press/release the Alt key and have the mnemonic highlighted/unhighlighted, perhaps making it easier to find.

For someone with a severe physical disability who needs to focus on the keyboard to type, however, this may present an extra barrier.  The scenario I'm thinking of is:

0) Decide upon an action you want to take (e.g., open the Edit menu)

1) Focus your eyes on the keyboard, locate the alt key, press it (maybe latch it via StickyKeys)

2) Focus your eyes back on the desktop and the Edit menu.  See the mnemonic that is now highlighted.

3) Focus your eyes back to the keyboard, locate the mnemonic, and then press it.

With the mnemonics visible as they are now, step #2 is not necessary.  Come to think of it, even if I didn't have to shift my gaze, my process would still be broken into discrete steps separated by a physical action.  That is, I need to locate the menu item, press Alt, then locate the mnemonic, then press the appropriate key. Without playing with it or seeing a mock up, my first reaction is that this might be awkward and inefficient.

In addition, from my personal standpoint of being a heavy keyboard user, the less distracting flashing I run into, the better.  So, I personally would rather not have every visible object in the window with an accelerator competing for my attention when I press the Alt key.

I'm going to guess you are going to run into very polarized views of this and making it a themable thing will be a way to appease folks.

just to point out another discussion for the same change in windows:

http://blogs.msdn.com/oldnewthing/archive/2006/10/05/792913.aspx

(In reply to comment #9)
> Hey Emmanuele!  Sounds like a nice change to me.  Have a patch for it?

I'm going to work on this for Moblin, and wanted to get some feedback from "upstream" first. :-) but yeah: I'm definitely going to attach a patch as soon as I've finished it.

(In reply to comment #10)
> From an accessibility point of view, I'm not sure how good or bad this will be.

I considered a11y as the potential issue.

> I'm going to guess you are going to run into very polarized views of this and
> making it a themable thing will be a way to appease folks.

high-contrast themes might activate it by default; also, if it is a GtkSettings we can bridge it to GConf/GSettings through XSETTINGS and have the global configuration toggle handled by the assistive technologies control panel applet.

Great!
ebassi, please ping me as soon as it's in, so I can put it in the theme. 

Emmanuele, we've been discussing this change in Ubuntu as well (https://bugs.edge.launchpad.net/gtk/+bug/403691). Much love!

Thomas Wood did a fairly nasty hack to a theme engine, basically listening to Alt key-press-event and key-release-event on a top-level window and changing the state of the gtk-enable-mnemonics flag. this worked nicely, since all the accelerators were still working, but was *impressively* slow. I think it was the connect/disconnect the accelerators and then traversing the top-levels hierarchy to recompute the labels.

one approach would be to have gtk-enable-mnemonics be a tri-state (0: always disable, 1: always enable, -1 o 2: show on key-press); by having a new state we would avoid the connection/disconnection part.

Created attachment 145423 [details] [review]
Patch 1

This a first pass at implementing the automatic-mnemonics feature. There are some things missing (such as the ability to disable the feature), but I'm looking for some comments on the general approach and to better understand the desired behaviour.

---

The basic approach here is to add a new property to the top level window
to determine whether to  show the mnemonics. This is set when alt is
pressed, and labels listen to the notify on the property to show or hide
the underlines. Mnemonic underlines are kept on when a menu is opened
using a mnemonic and are turned off again when the menu is closed.
Mnemonic underlines are also turned off when the window looses focus.

Currently implemented:

GtkWindow:
 * A new property GtkWindow:visible-mnemonics which determines whether
   mnemonics should be displayed inside this window.

 * GtkWindow will disable mnemonics-visible when the window looses focus.

GtkWidget:
 * GtkWidget set the GtkWindow:visible-mnemonics property when Alt_L or
   Alt_R is pressed - this should really use value of the
   mnemonic-modifier, but it was hard to either convert this to a keyval
   or detect when a modifier was pressed/released, as the modifier was not
   always set.

GtkLabel:
 * GtkLabel listens to the GtkWindow:visible-mnemonics property and show/hides the underlines appropriately.

GtkMenu:
 * GtkMenu will copy the visible-mnemonics property from the parent
   GtkMenuShell toplevel onto the new GtkMenu window.

 * GtkMenu swallows key release events to stop visible-mnemonics from being
   disabled once a menu has been opened with mnemonics visible.

GtkMenuShell:
 * GtkMenuShell will disable mnemonics-visible when the menus have been
   closed.


Not implemented:

 * A new property (GtkSettings:automatic-mnemonics) to turn on/off this
   behaviour.

 * Prevent GtkLabel from listening to the property if it has no mnemonics

I should mention, the above patch is against gtk-2-16.

Review of attachment 145423 [details] [review]:

the label should install the mnemonics_visible handler only when setting the :use-underline property, inside gtk_label_set_use_underline_internal(TRUE), and/or checking inside a handler for the mapped signal.

::: gtk/gtklabel.c
@@ +2336,3 @@
+      g_signal_handler_disconnect (priv->toplevel,
+                                   priv->mnemonics_notify_handler);
+      priv->toplevel = 0;

this is a pointer, should be NULL

::: gtk/gtkmenushell.c
@@ +987,3 @@
+        {
+          g_object_set (G_OBJECT (window), "mnemonics-visible", FALSE, NULL);
+        }

do not place curly brackets around single statements

::: gtk/gtkwidget.c
@@ +4509,3 @@
+            {
+              g_object_set (window, "mnemonics-visible", TRUE, NULL);
+            }

again, no curly brackets around single statements here...

@@ +4533,3 @@
+            {
+              g_object_set (window, "mnemonics-visible", FALSE, NULL);
+            }

... and here.

> ::: gtk/gtkmenushell.c
> @@ +987,3 @@
> +        {
> +          g_object_set (G_OBJECT (window), "mnemonics-visible", FALSE, NULL);
> +        }
> 
> do not place curly brackets around single statements

And the cast there is superfluous. The first argument is an untyped pointer.

Created attachment 145529 [details] [review]
Patch 2

Fix the minor issues as mentioned above and only connect the notify handler when the label actually has an active mnemonic.

Review of attachment 145529 [details] [review]:

::: gtk/gtkmenushell.c
@@ +986,3 @@
+      if (GTK_IS_WINDOW (window))
+        g_object_set (window, "mnemonics-visible", FALSE, NULL);
+

no C99, middle-of-the-block declaration

::: gtk/gtkwidget.c
@@ +4501,3 @@
+
+      /* only process each event once */
+      if (last_event != event->time)

is this really necessary?

@@ +4509,3 @@
+            g_object_set (window, "mnemonics-visible", TRUE, NULL);
+
+          last_event = event->time;

can you add this code inside GtkBinding for GtkWidget? or at least abstract it into a GtkWidget gtk_widget_maybe_set_mnemonics_visible() function? this would avoid the same code lying in two functions.

Review of attachment 145529 [details] [review]:

::: gtk/gtkmenushell.c
@@ +986,3 @@
+      if (GTK_IS_WINDOW (window))
+        g_object_set (window, "mnemonics-visible", FALSE, NULL);
+

yes, obviously this will be in an if() block once the GtkSettings property is implemented.

::: gtk/gtkwidget.c
@@ +4501,3 @@
+
+      /* only process each event once */
+      if (last_event != event->time)

Since the event propagates up the scene graph, we don't want to set the property multiple times. However, if we decide that enabling auto-mnemonics means we return true in the GtkWidget key press/release handlers for the mnemonic modifier, this won't be necessary.

@@ +4509,3 @@
+            g_object_set (window, "mnemonics-visible", TRUE, NULL);
+
+          last_event = event->time;

Sounds like a good idea, I'll look into it.

Review of attachment 145529 [details] [review]:

::: gtk/gtklabel.c
@@ +58,3 @@
+
+  gboolean mnemonics_visible;
+  guint mnemonics_notify_handler;

Labels are the most common widgets, which is why I really hate to grow them.
I'm pretty sure there's bits available in the bitfield in the GtkLabel struct itself that can be used for the mnemonics_visible bit. And the toplevel pointer does not really seem to be necessary in the first place - can't we just call get_toplevel again when disconnecting ?

@@ +1341,3 @@
+    {
+      disconnect_mnemonics_visible_notify (GTK_LABEL (widget));
+      goto done;

We also need to disconnect the signal when finalizing the label and when reparenting it, I think.

::: gtk/gtkwidget.c
@@ +4498,3 @@
+  if (event->keyval == GDK_Alt_L || event->keyval == GDK_Alt_R)
+    {
+      static guint32 last_event = 0;

I don't think this is necessary. Setting mnemonics-visible on the toplevel should not do anything if it was already set.

::: gtk/gtkwindow.c
@@ +5217,3 @@
 
+  /* if (auto-mnemonics...) */
+  if (event->keyval == GDK_Alt_L || event->keyval == GDK_Alt_R)

You should really add a setter for mnemonics-visible instead of having the same g_object_notify call in three places. The setter should also check if the value changed before emitting the notify. 

And if we add a setter, we should also add a getter...

Review of attachment 145529 [details] [review]:

::: gtk/gtkwidget.c
@@ +4498,3 @@
+  if (event->keyval == GDK_Alt_L || event->keyval == GDK_Alt_R)
+    {
+      static guint32 last_event = 0;

And even easier approach to avoid double work might be to do the special-casing for Alt-press/release events in gtk_main_do_event before passing it up the widget hierarchy.

One problem I notice is wrt to menus.

When I open the File menu using Alt-F, the menu has the mnemonics visible, and they stay visible even if I release Alt - which is probably fine, since the mnemonics here work without keeping Alt pressed.

But if I open the menu with the mouse, the mnemonics are not visible, and pressing Alt does not make them visible either, so I have no chance to learn what keys I need to press to activate a menuitem.

Review of attachment 145529 [details] [review]:

::: gtk/gtklabel.c
@@ +58,3 @@
+
+  gboolean mnemonics_visible;
+  guint mnemonics_notify_handler;

I think I was unsure whether we would always get the same toplevel if calling it again later, e.g. during finalize.

@@ +1341,3 @@
+    {
+      disconnect_mnemonics_visible_notify (GTK_LABEL (widget));
+      goto done;

gtk_label_setup_mnemonic() gets called from gtk_label_heirarchy_changed(), which presumably gets called during finalize and reparenting?

Created attachment 146928 [details] [review]
Patch 3

Updated patch with some of the previous suggestions implemented. The patch is now against master.

A couple of things I noticed:

* Only Alt_L can be used to activate the mnemonics, so it now only checks for that key.

* Mnemonics can now be turned on in popup menus, but I have not allowed them to turn off on release of the alt key, since it is not possible to activate a mnemonic in a menu with the alt held down.

* Mnemonics only work in menus if the mouse is over the menu, or the arrow keys have been used to navigate into the menu. This is the existing behaviour, although it feels rather odd if you open a menu with the mouse and then want to use a mnemonic.

Here is a summary of the changes introduced in the patch:

* Add a "gtk-auto-mnemonics" property to GtkSettings
* Add a "mnemonics-visible" property to GtkWindow
* Handle Alt press in gtk_main_do_event() and toggle the top-level
  mnemonics-visible property if gtk-auto-mnemonics is enabled
* Hide and show the mnemonic underlines in GtkLabel depending on the value
  of the mnemonics-visible property
* Copy the state of mnemonics-visible when a GtkMenu is opened
* Reset the state of mnemonics-visible when a GtkMenu is closed

(In reply to comment #22)

> Labels are the most common widgets, which is why I really hate to grow them.
> I'm pretty sure there's bits available in the bitfield in the GtkLabel struct
> itself that can be used for the mnemonics_visible bit.

I just checked, and the GtkLabel struct is padded out - there's no padding left; all the bitfields add up to 16 bits, though.

Do they not end up 32-bit aligned in the struct ?

Haven't looked at the latest patch itself, just tried to run it. 

First of all, I do get to activate mnemonics with both Alt_L and Alt_R - not sure why that is not the case for you. So I think both of them should trigger the underlines. Or rather, it needs to be the case that the set of keys that trigger mnemonics is identical to the set of keys that trigger underlines.

The behaviour of menus doesn't seem entirely right to me, yet. Here is one example (both can be seen in gtk-demos 'app window' demo):

1) Use F10 to focus the menubar 
2) The first menu pops up, without underlines
3) Press Alt
4) The menu grows underlines
5) Press Down to go into the menu
6) It loses underlines again

Another example:
1) Press and release Alt-P _quickly_
2) The Preferences menu opens, the Color item is selected, and its submenu pops up
3) The Preferences menu has underlines, the submenu doesn't

Another:
1) Press and hold Alt-P 
2) The Preferences menu opens, the Color item is selected, and its submenu pops up
3) The Preferences menu has underlines, the submenu doesn't
4) Release the keys
5) the submenu gains underlines

Thomas, do you have any update on this ?

Haven't had a chance the last few weeks. I think I need to change the approach to match the same behaviour as propagating the mnemonic action, i.e. use a signal callback to traverse the scene graph, rather than using notify signals. There seems to be an issue in the Evolution preference window where not all buttons get the mnemonic underlines, but the mnemonics do still work.

I think I managed to reproduce all the issues you raised; sub-menus especially were not something I initially tested. I'll put some time aside in the next week to investigate.

Created attachment 148837 [details] [review]
Patch 4

* Rebase against master
* Install only a single notify callback and traverse the widget scene graph
  -> This removes the need for extra members in the GtkLabelPrivate struct
  -> Fixes issues with more complex applications, such as Evolution
* Fix issues with submenus - don't reset the mnemonics-visible value on map if
  it has already been set
* Add back GDK_Alt_R to enable mnemonics
* Fixed compiler warnings

Created attachment 148841 [details] [review]
Path 4 (again)

Attached the correct patch this time.

* Rebase against master
* Install only a single notify callback and traverse the widget scene graph
  -> This removes the need for extra members in the GtkLabelPrivate struct
  -> Fixes issues with more complex applications, such as Evolution
* Fix issues with submenus - don't reset the mnemonics-visible value on map if
  it has already been set
* Add back GDK_Alt_R to enable mnemonics
* Fixed compiler warnings

While we're at it.. why not hide the Control+Commands on drop down menus in the same way? That would reduce a hell of a lot of visual noise for something that - let's be honest here - is very, very rarely used.

Example:

Before (boo, noisy!):
http://imgur.com/wpAR4l.png

After (a breath of fresh air!):
http://imgur.com/cOy1nl.jpg

Rich, I think this wouldn't workout. For one, icons are not related to shortcuts. And the shortcuts are not used while inside the menu but elsewhere, so there is no incentive to hold any particular key while viewing the menu, plus you can't know what keys are used at all. If you dislike menu icons, simply disable them.

I suggest that the people working on removing visual cues that help keyboard users to do one thing: unplug your mouse for a day.

While doing so, ask yourself this question: did eliminating the cues have any effect on the speed and efficiency in which you could accomplish your tasks?  Also imagine you were a new GNOME user.  Did making cues invisible make the interface easier for keyboard only users to learn how to interact with the desktop?

Let's try and keep this bug on track please.

This bug is about (optionally) making the mnemonic underlines appear only when when the alt key is pressed.

We are not discussing removing them altogether (which is already possible) and we are not discussing removing any visual aspects of GTK+.

@Christian - about the icons.. that wasn't my point. I was just on 8.04HH, which still has those enabled.

@Willie - What is a keyboard-only user? Why design a DE for somebody with a basically broken system?

Will move this discussion to another place.

(In reply to comment #38)
> @Willie - What is a keyboard-only user? Why design a DE for somebody with a
> basically broken system?

A keyboard-only user is someone who either does not have a mouse or cannot use the mouse. A typical keyboard-only user is someone who is blind, and others include people with enough mobility to use the keyboard but who don't have enough motor control to use the mouse.

Accessibility by people with disabilities is a core value of GNOME, so we all need to keep accessibility part of our every day design and development practices. :-)

Review of attachment 148841 [details] [review]:

I like the gtklabel and gtkmain parts of this patch.

The menu handling is still unsatisfactory to me.
E.g. look at the appwindow example in gtk-demo. 
Press F10 to move the focus to the menu bar. Use
arrow keys to move the focus to Preferences. Now the
Preferences menu is open, and no mnemonics are visible.
Press Alt. The mnemonics in the preferences menu
show up. This is confusing, since the mnemonics that 
can be used now are actually the ones in the
menu bar, not the ones in the menu. To use the ones
in the menu, you have to enter it first.
Do that now. Oops, the mnemonics in the preferences menu
are gone, even though they can be used now.

::: gtk/gtklabel.c
@@ +4266,3 @@
+                        "notify::mnemonics-visible",
+                        G_CALLBACK (label_mnemonics_visible_changed),
+                        label);

Passing label as data here is just confusing, since the callback is for the entire hierarchy. Just pass NULL.

One more problem noticed in the appwindow example:

Right click in the content pane and look at the context menu. It has a bunch of insensitive and sensitive items. The insensitive items have mnemonic underlines, the sensitive ones don't. Pressing Alt makes them show up on the sensitive items too. So there seems to be something going wrong with updating the mnemonic visibility for insensitive labels. It might be better to just not show mnemonics on invisible labels at all if enable-auto-mnemonics is turned on.

Created attachment 149500 [details] [review]
patch 5

This patch fixes the handling of insensitive labels to what (I think) was intended: never show mnemonics in insensitive labels. Could make it so that we only do this if enable-auto-mnemonics is on...

Some of the problems with menus can be explained by the fact that the grab_widget is somewhat misleading here, since the menu code passes the key events up to the parent menu shell if the menu doesn't have an active item. That is why we still see the menu bar mnemonics being active when the preferences menu is open but without an active item.

Created attachment 149501 [details] [review]
improve menu handling

Here is a patch that makes menu handling a lot more acceptable, I think. It makes the mnemonics appear where they actually take effect. Not so sure about the name _gtk_menu_shell_get_mnemonic_target, though...

Heya, looking really nice.  One case I found that doesn't seem right is this:

Using gedit
1. click File
2. hit alt
3. move mouse focus into the dropped down file menu by hovering (sets focus blue selection)
4. type E
5. type N

The result is that after step 2 the top level menubar has mnemonics showing (ie. the F in File, E in Edit, etc) but the items in the file menu do not.  This is probably right since the file menu is not yet accepting mnemonic acceleration.

However, after step 3 the underscores have not changed.  This seems wrong since step 4 does not result in the Edit menu dropping down and step 5 does activate the File->New item.

If step 3 is replaced with using the down arrow to move focus then things seem to work as I'd expect.

Created attachment 149567 [details] [review]
another take

Here is a somewhat different patch that tries very hard to always underline exactly the currently activatable mnemonics. I think it gets this almost 100% right in menus now. 

The one problem with this approach is that you now have underlines popping in and out as you are mousing over the menus. I need to figure out a way to restrict this to keynav.

Created attachment 149594 [details] [review]
more tweaking

Here is a further elaboration that gives very reasonable behaviour, I think.
The patch needs some cleanup.

I've committed a cleaned up version with some more fixes.

Created attachment 150137 [details] [review]
keep mnemonics visible in menus once activated

I've just tried the committed version.

Unfortunately, I really really don't like the underlines popping in and out on the menus. Although it may demonstrate which mnemonics are activatable, it seems way too busy to me for every day use, especially when switching from keyboard to mouse and vice versa, and when navigating the menus. I much prefer the underlines being shown on all menus once keyboard mode is activated. This would also match the behaviour of other platforms (windows).

I've attached a patch to enable all mnemonics in menus and keep them enabled once keyboard mode in menus is entered.

Re-opening the bug so we can comment on the final behaviour.

I had a quick play with this and sadly it's way to easy to get into a state where rapid movement or navigation causes a horrible strobing effect. This is especially noticeable with the mouse moving through a menu with disabled items.

Design wise there's a number of potential fixes, some easier than others. Probably the three best options would be:

* add a blingy animation fade out to reduce the strobing (this would need to be combined with the next one)
* use timeouts to prevent rapid switching of state
* thomas' patch above

I agree with Thomas. Mathias' latest patch causes worse problems than it solves.

I think the crux of the issue is that when the menu system enters keyboard mode and the user subsequently uses the mouse, the available mnemonics change rapidly from the current menu to the parent menu when the mouse pointer moves over separators or disabled items. This causes the mnemonic underlines to flash on and off.

I can see two possible solutions to this. We could either turn off keyboard mode when the user subsequently uses the mouse, or we could simply always show mnemonics in menus one menu has been opened (as my patch does currently, and as observed in other platforms such as Windows).

Mattias: would you like me to open a new bug to resolve this issue?

Created attachment 155774 [details] [review]
Keep mnemonics in the active menu

Keep the mnemonics available in the active menu, even if a non-selectable item is hovered using the mouse (e.g. separator or insensitive item).

This patch only passes mnemonic key strokes up to the parent shell if the menu is not active, rather than has an active item. The logic for selecting which mnemonics are visible with auto-mnemonics is changed accordingly.

This prevents mnemonics of a menu from flashing on and off as a mouse moves over non-selectable items.

This seems to be breaking menu scrolling.  Filed bug #612611 to track that.

Created attachment 156014 [details] [review]
alternative patch

Here is an alternative patch which avoids the problem of getting stuck in submenus.

Comment on attachment 155774 [details] [review]
Keep mnemonics in the active menu

Matthias' patch works fine.

This definitely improves things, but it's still behaving slightly wrong when gtk-auto-mnemonics is enabled.

1/ Go to the menus test in testgtk
2/ Open the first menu ("test line2")
3/ Key all the way down to the bottom.  The last item is 2-50.  Key down again.

At this point, if gtk-auto-mnemonics is disabled then keying down again will wrap keyboard focus around to the top of the menu and it will scroll up so you can see that selection.

If gtk-auto-mnemonics is enabled you won't see anything.. the view will remain with 2-50 at the bottom, but apparently the actual selected item has wrapped around.  If you keep keying down a few more times you'll find one item with a submenu, and its submenu will popup very strangely.

And if you keep keying down further you will eventually see the selection appear, and then (at least on my laptop screen) if I continue keying down to item 2-30 suddenly the menu will jump so that 2-30 is at the bottom of the menu instead of in the middle.

I've committed the patch. Lets track scrolling breakage in the other bug you created for that.