Hm... I just got this with Thunderbird. There must be a case where meta_window_propagate_focus_appearance() needs to be called, but we're not doing it.

Do you do anything in Firefox that makes it show an attached dialog box? (The only thing I know of that does that is the username/password dialog for sites using HTTP authentication, but there may be others.

Yes, I use a master password and Firefox asks me for it in an (apparently) attached dialog box when it starts.

Created attachment 185899 [details] [review]
window: fix a case of appears-focused getting stuck

Since appears-focus only propagates up from modal dialogs, if an
application removed the modal hint from a dialog before destroying it,
then its parent would be left with a stray reference to it in
attached_focus_window, causing it to be permanently appears-focused.

The obvious fix, calling meta_window_propagate_focus_appearance() when
the modal hint is removed, turns out to be a bad idea, because it
causes the parent window to flash unfocused for a moment before
receiving the focus again. (Plus, we don't track modality properly
anywhere else either; eg, detaching attached dialogs).

So instead we just change meta_window_propagate_focus_appearance() to
check the window type only when focusing in, not when focusing out.

This would also cause flashing, but in this case we can avoid it by
not notifying the change in appears-focus on the parent window if it
is the expected_focus_window (which it will be by this point). (This
does mean though that if something weird happens and the window
doesn't end up becoming the focus window, it won't get redrawn
unfocused until something else forces it to.)

Created attachment 185900 [details] [review]
window: fix stuck appears-focused more generally

Use weak refs to ensure that a window can't mistakenly hang on to an
old attached_focus_window indefinitely.

So, to reproduce in thunderbird: open a compose window, click attach, cancel the attach dialog. The compose window will now be stuck appears-focused.

I am worried about this part:

> (This
> does mean though that if something weird happens and the window
> doesn't end up becoming the focus window, it won't get redrawn
> unfocused until something else forces it to.)

It could be fixed in various ways: either by getting MetaDisplay involved in some way, or by queuing an idle to check the window state again, or something else. I wonder if there isn't a better way to deal with this than using expected_focus_window though.

(The flashing is quite visible and annoying.)

I see this switching between Gedit and a particular Nautilus window as well atm. Nautilus holds on to focus. This does however not happen to newly started Nautilus windows so I can not even reproduce it on my own machine...

Attach image below.

Created attachment 185908 [details]
Nautilus holds on to focus

Review of attachment 185899 [details] [review]:

Discussed this with Dan at quite some length.

It should be noted that the flashing actually occurs for all cases where closing an attached dialog currently *works* - we inherently flash when changing focus from child to parent. It's just for some reason on Dan's machine easy to trigger with Thunderbird and harder to trigger with gedit. So, really, this patch consists of two parts:

A patch to fix a case where attached_focus_window is stale
==========================================================

This patch seems a little awkward to me compared to propagating to the parent when the attached modal dialog changes. I don't really understand why that doesn't work - it seems to me, that the flash reduction code should work as well if we call meta_window_propagate_focus_appearance() from recalc_window_type() - the only issue is that you have to call it *before* setting window->type, which would require swapping old_type for a new_type. *Or*, if you applied the change in this patch, you could avoid that and do:

 if (old_type == META_WINDOW_MODAL_DIALOG && meta_window_appears_focused (window))
    meta_window_propagate_focus_appearance (window, FALSE);

 [...]

 if (window->type == META_WINDOW_MODAL_DIALOG && meta_window_appears_focused (window))
    meta_window_propagate_focus_appearance (window, TRUE);

And get modal dialog changes right always, and not just right before close.

"(Plus, we don't track modality properly anywhere else either; eg, detaching attached dialogs)." is sort of unclear to me - maybe you need to queue a move-resize on the window to make that fully work, but it probably *mostly* works, not saying we need to fix it, but it isn't hundreds of lines of unwritten code.
 

A patch to try and reduce flashing when focusing the parent
===========================================================

I think the right way to address this long term is bug 647706 - but that does seem to be too intrusive. So, maybe this make sense:

I'm not concerned about the race condition in a 3.0.x timescale:

 - The race condition seems like it would be very hard to trigger - we need the sequence
    - App closes a dialog
    - We get the event, and round trip to the X server to get a new timestamp
    - *After* we round trip to the X server but before we call XFocusWindow, the app round trips to the X server to get a new timestamp
    - The app XFocusWindow's something other than the parent
    - We try to XFocusWindow the parent

 - The race condition results in a state where:

   - Until the next time the parent is focused/resized, it draws mistakenly focused
   - The parent may have a frame that has a partial focus shadow and a partial non-focus shadow

   It's not a serious bug even if some app manages to trigger it reliably.

There's also something that Dan didn't note here that while his patch fixes flashing of the titlebar, the shadow will still flash if drawn for some other reason, because that's updated on each frame; the notification is just to reliably get the entire shadow drawn. But I don't think that's a serious problem either. Not completely sure this is 3.0.1 material, but it does seem pretty safe, so I guess a bit of flash prevention is a good thing.

(In reply to comment #6)
> I see this switching between Gedit and a particular Nautilus window as well
> atm. Nautilus holds on to focus. This does however not happen to newly started
> Nautilus windows so I can not even reproduce it on my own machine...
> 
> Attach image below.

This is "easily" reproducible by triggering the file conflict dialog - create another file with the same name as an existing file and drag it into an existing window. Not immediately clear why this would be the case, though it does appear that the file conflict is created mapped without a parent and then *morphed* into an attached dialog by setting the transient for, which would be nautilus bug, but shouldn't trigger this problem.

(In reply to comment #8)
> This patch seems a little awkward to me compared to propagating to the parent
> when the attached modal dialog changes. I don't really understand why that
> doesn't work - it seems to me, that the flash reduction code should work as
> well if we call meta_window_propagate_focus_appearance() from
> recalc_window_type()

No, because at the point recalc_window_type() is called, mutter doesn't know yet that the window is about to be withdrawn, so it hasn't even though about focus yet, and expected_focus_window will be either NULL or stale. (And I guess the PropertyNotify and UnmapNotify arrive far enough apart that we have time to redraw in between.)

The patch fixes the nautilus case as well; I didn't investigate further.

Attachment 185899 [details] pushed as eb0e658 - window: fix a case of appears-focused getting stuck

Looks like the patch landed. Closing.