GNOME Bugzilla – Bug 785236
Mnemonics for GtkLabels in GtkPopovers interfere with other accelerators
Last modified: 2018-05-02 18:46:06 UTC
Created attachment 356123 [details] [review] Check if label belongs to popover before adding mnemonic. This bug report stems from an issue I noticed recent versions of Evince, where certain keyboard shortcuts don’t work. Further investigation revealed that the problem is rooted in how GtkLabel sets up mnemonics. In Evince, the accelerators “<alt>P” and “<alt>N” are supposed to jump to the previous and next item, respectively, in the navigation history (https://github.com/GNOME/evince/blob/master/shell/ev-application.c#L1051). Such shortcuts are handy if for example one jumped to a reference in an article by clicking a link and wanted to go back. Some time ago I filed a bug report about this against Evince (https://bugzilla.gnome.org/show_bug.cgi?id=782451). To make a long story short, Evince changed its menus to use GtkPopover instead of GtkMenu (https://bugzilla.gnome.org/show_bug.cgi?id=760527), and mnemonics for labels inside GtkPopover interfere with other keyboard accelerators. Indeed the problem goes away if I undo this change. The mechanism for this conflict appears to be as follows: *Mnemonics take precedence over accelerators in gtk_window_activate_key(). *GtkLabels buried inside popovers assign their mnemonics to the top level window in gtk_label_setup_mnemonic() (https://github.com/GNOME/gtk/blob/master/gtk/gtklabel.c#L1756). I am attaching a simple patch based on the GTK 3.22 branch which appears to fix the problem; namely, just check if the label lives inside a GtkPopover, and if so, don’t add its mnemonic to the top level window. The existing code does something similar in case the label lives inside a GtkMenu. Any comments would be appreciated.
Created attachment 356892 [details] test case This is something that has bugged me for a while, but perhaps from the converse side: I'm not so much worried about mnemonics in Popovers getting in the way of other mnemonics, but about them being impossible to activate. Here's a test case for my scenario, which may be illuminating for yours too. * Open the Popover and hold Alt then press B. * Expected result: the Button in the Popover activates. * Actual result with released GTK+ 3: the mnemonic cycling starts, by moving to - but not activating - the Button in the Window, which thus closes the Popover... * Actual result with your patch applied: the Button in the Window is activated, and the Popover stays visible, but its Button cannot be activated by mnemonic So I think the patch needs work. I mean, you don't add the mnemonic to anything. So it's lost. Shouldn't it be added to the Popover? Note how the MenuShell code you're emulating does add the mnemonic to the MenuShell. I also think you might also need to consider what should happen if, through some abomination of design, a Popover contains a Menu... then won't the Label have both as ancestors?
I'm working up a fixed patch at the moment. You should provide a test case for the specific situation you reported, so that we can test it on that too. (In reply to Daniel Boles from comment #1) > * Actual result with released GTK+ 3: the mnemonic cycling starts, > by moving to - but not activating - the Button in the Window, > which thus closes the Popover... Hm, sometimes the Popover stays open, and might be cycling, but as soon as you cycle back to the Window it closes. > * Actual result with your patch applied: the Button in the Window is > activated, > and the Popover stays visible, but its Button cannot be activated by > mnemonic Also, on startup: > (a.out:20263): Gtk-CRITICAL **: gtk_window_add_mnemonic: assertion 'GTK_IS_WINDOW (window)' failed Dunno how it gets there, but it's wrong either way.
Sorry, the critical was the fault of my patch. :( Also, (In reply to Daniel Boles from comment #1) > I mean, you don't add the mnemonic to anything. So it's lost. Shouldn't it > be added to the Popover? Note how the MenuShell code you're emulating does > add the mnemonic to the MenuShell. I dunno why I thought GtkPopover was derived from GtkWindow.
The problem with your example is that you assigned the same mnemonic to two buttons. When the popup is activated and both GtkLabels with mnemonic "B" are visible, the default handler for the mnemonic (https://github.com/GNOME/gtk/blob/master/gtk/gtkwidget.c#L6215) will be called with "group_cycling=TRUE"; see also (https://developer.gnome.org/gtk3/stable/GtkWidget.html#gtk-widget-mnemonic-activate). Thus the first time "<alt>B" is pressed the Button inside the popover will merely be focused. However, when an overloaded mnemonic is activated the target widget is pushed to the back of the mnemonic hash (https://github.com/GNOME/gtk/blob/master/gtk/gtkmnemonichash.c#L109). Thus the next invocation of "<alt>B" focuses the Button in the window and closes the popover. However, you did identify an oversight of my patch. If you change the mnemonic for the Button inside the popover to something else (like B_utton), the mnemonic works as expected without my patch and (understandably) does not work with my patch. A better fix might be to dynamically add the mnemonics of GtkLabel to the top level window when the popover is activated, and remove the mnemonics when the popover is dismissed.
Another alternative would be to simply make clear in the developer docs that <alt>+ key combinations are likely to conflict with mnemonics. Note that this has only become a problem recently as various GNOME applications are switching from GtkMenu to GtkPopover, since the mnemonics for labels inside GtkMenu are not assigned to the top level window.
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/gtk/issues/860.