Created attachment 326747 [details] [review]
Fix hiding popover when focus moves outside

Commit a01fe14 changed the behaviour of popovers when the focus leaves
them to stop child popovers being hidden when the focus leaves their
parent. However they are now a bit too reluctant to hide - if the
focus passes to an unrelated popover the first popover is not
hidden. Also if the focus passes to another widget that does not
perform a gtk grab then the popover isn't hidden until the user
presses a non-movement key or clicks outside the popover.

The solution is to go back to checking if the focused widget is a
descendant of the popover, but to include popovers and their related
widgets in the ancestry chain.

Created attachment 326748 [details]
test program

Sorry I forgot to actually attach the test program to comment 1

Review of attachment 326747 [details] [review]:

::: gtk/gtkpopover.c
@@ +443,3 @@
   GtkPopoverPrivate *priv = gtk_popover_get_instance_private (popover);
 
+  if (!priv->modal || !widget)

It should check if the popover is drawable here

@@ +455,2 @@
     }
+  gtk_widget_hide (popover);

It should probably do something about clearing the saved focus widget here, as the focus goes back to the old focus row rather than the newly focused row it the test program

(In reply to Phillip Wood from comment #0)
> If the focus leaves a modal popover for a popover or widget that is not
> related to it it should close.

Thats a bit of an unsubstantiated assertion.

(In reply to Matthias Clasen from comment #4)
> (In reply to Phillip Wood from comment #0)
> > If the focus leaves a modal popover for a popover or widget that is not
> > related to it it should close.
> 
> Thats a bit of an unsubstantiated assertion.

It's based on
commit 43e88528292b9c786be3de04cdd8b2a6fc8266dd
Author: Carlos Garnacho <carlosg@gnome.org>
Date:   Thu Mar 6 16:58:06 2014 +0100

    popover: Track toplevel's focus widget when visible
    
    If the toplevel focus widget is forced out of the popover (eg. through
    gtk_widget_grab_focus() anywhere else), then dismiss the popover.


If you do 'git log -L :window_set_focus:gtk/gtkpopover.c' you'll see the commit messages are all about hiding the popover if the focus escapes to an unrelated widget.

As it stands if you open a popover from a menu button and then activate another unrelated menu button with a mnemonic the first popover does not close even though the second menu has nothing to do with the first - is this actually the intended behaviour?

The patch is based on the suggestion from Carlos Garnacho in comment 6 in bug 750741 [1]

[1] https://bugzilla.gnome.org/show_bug.cgi?id=750741#c6

Fair enough. Lets see what Carlos thinks

Review of attachment 326747 [details] [review]:

Thanks for the patch, the approach makes sense.

::: gtk/gtkpopover.c
@@ +439,3 @@
 window_set_focus (GtkWindow  *window,
                   GtkWidget  *widget,
+                  gpointer    popover)

I understand gpointer is convenient here, although I'd prefer to keep some typing here, IMO it'd be better to settle with GtkWidget or GtkPopover.

@@ +448,2 @@
+  while (widget &&
+         (widget = gtk_widget_get_ancestor (widget, GTK_TYPE_POPOVER)))

These inline assignments are IMO too easy to go unnoticed when later revisiting the code... I'd prefer having it further explicit with a (...) != NULL check, or doing this (plus the "if null, break" check) inside the while statement.

@@ +455,2 @@
     }
+  gtk_widget_hide (popover);

Right, probably just needs a prior call to popover_unset_prev_focus()

(In reply to Carlos Garnacho from comment #7)
> @@ +455,2 @@
>      }
> +  gtk_widget_hide (popover);
> 
> Right, probably just needs a prior call to popover_unset_prev_focus()

This was meant to be a reply to:

(In reply to Phillip Wood from comment #3)
> @@ +455,2 @@
>      }
> +  gtk_widget_hide (popover);
> 
> It should probably do something about clearing the saved focus widget here,
> as the focus goes back to the old focus row rather than the newly focused
> row it the test program

Created attachment 327073 [details] [review]
Thanks for your comments Carlos, I've updated the patch as you

suggested
 - popover is now GtkPopover* rather than gpointer
 - the assignment is now in the body of the while loop
 - it calls popover_unset_prev_focus() before hiding

One thing I've noticed while testing the patch is that the Tab key
behaves differently depending on how many widgets there are in the
popover. With only one widget pressing Tab moves the focus to the
first widget in the window containing the popover (and so closes the
popover with this patch applied). If there is more than one widget
then the focus cycles between them so it cannot escape from the
popover. It might be better if the single widget case either trapped
the focus or focused priv->prev_focus_widget instead. I can open
another bug if you think it's worth changing it.

Out of interest, I've always wondered: what is an example use-case for not closing the Popover when focus leaves it? Are people using DND onto Popovers or something else? Just curious as personally I've always expected Popovers to close right away and not found a case where I need them to stay open. (In fact, it's meant I've had to do some creative workarounds when handling window events)