GNOME Bugzilla – Bug 693570
Search entry flickers when opening/closing popup menus
Last modified: 2013-02-14 18:18:44 UTC
This is a regression introduced by the GrabHelper port of PopupMenuManager. The first patch is correct in my opinion, but may introduce regressions elsewhere; the second patch just works around this particular issue (in case we consider the first one too dangerous).
Created attachment 235679 [details] [review] grabHelper: Restore the actually saved focus on ungrab GrabHelper saves the actor that had key focus when taking over the grab (if any). On ungrab, the key focus is either restored or moved to some child of the saved actor. The latter is unexpected and causes some odd behavior, so don't be fancy and only restore the actual focus.
Created attachment 235680 [details] [review] overview: Set can_focus on main box layout When closing a popup menu while in the overview, the search entry will erroneously end up focused. This is a regression from moving PopupMenuManager to GrabHelper, which will only move focus back to the previously focused actor if the can_focus property is set, and move focus to a child otherwise. To work around this, set can_focus of the main overview actor passed to Main.pushModal().
Review of attachment 235679 [details] [review]: I'm fine with this if we test the message tray and child popup menus thoroughly. It's obviously the saner behavior to do. You could also probably move the rest of _navigateActor inline into the one place it's used: if (newFocus && (hadFocus || params.grabFocus)) if (!newFocus.navigate_focus(null, Gtk.DirectionType.TAB_FORWARD, false)) newFocus.grab_key_focus(); We shouldn't be grabbing any non-Widgets any time soon.
Created attachment 236087 [details] [review] grabHelper: Merge _navigateActor() with its only user (In reply to comment #3) > I'm fine with this if we test the message tray and child popup menus > thoroughly. It's obviously the saner behavior to do. I haven't encountered any issues using the message tray in the overview, and no additional ones for child popup menus. > You could also probably move the rest of _navigateActor inline into the one > place it's used Yeah, here's the patch for that (assuming that actor is an instance of St.Widget)
Review of attachment 236087 [details] [review]: Not quite. If the actor fails to navigate a focus, you should grab it yourself.
(In reply to comment #5) > Not quite. If the actor fails to navigate a focus, you should grab it yourself. That's what the patch does?
(to be fair, I'm definitively guilty of exceeding a reasonable line length here, in particular as I like to complain about this elsewhere ...)
I didn't even *notice* the ||. We don't put side effects that in ||s, so I'd recommend the explicit if.
Attachment 235679 [details] pushed as 60257f4 - grabHelper: Restore the actually saved focus on ungrab Attachment 236087 [details] pushed as ad1b9b7 - grabHelper: Merge _navigateActor() with its only user I'm pretty sure there were precedences of using '||' that way, but yeah, it's not the best place to be concise; changed to use an explicit if.