526941 - Panels with noautohide="true" not taking focus

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Neil Deakin]

Attachment: testcase

This testcase shows two buttons that open panels when pressed. The first panel, for the button 'Panel Button', works OK. The second panel, for the button 'Panel button noautohide' doesn't focus properly. Clicking on the panel doesn't focus the elements inside it.

The difference is that the former is created with:
  gtk_window_new(GTK_WINDOW_TOPLEVEL)
whereas the latter is created with:
  gtk_window_new(GTK_WINDOW_POPUP)

The former is created this way to allow it to hover only above its parent. (gtk_window_set_transient_for() is also called but seems to have no effect).

The problem is that the popup (a widget of type eWindowType_popup) is receiving the focus-in signal and NS_ACTIVATE is firing on the popup. Instead, it should fire on the toplevel window. Focus works properly if the titlebar of the parent window is then clicked.

Comment 1

[Karl Tomlinson (:karlt)]

With my local build based on 2d504f6e2ede, I seem to get the same behavior with 'Panel button noautohide' as with 'Panel Button'.  That is clicking on elements in either panel focuses them.

BTW, GTK thinks of both gtk_window_new(GTK_WINDOW_POPUP) and gtk_window_new(GTK_WINDOW_TOPLEVEL) as toplevels, so I wonder what might be causing the difference you are seeing.

Comment 2

[Neil Deakin]

Oh, I should mention that you need to load the test in a chrome window to see the behaviour, as noautohide is a privileged only feature.

Changing to GTK_WINDOW_POPUP from GTK_WINDOW_TOPLEVEL makes it work, but then the popup floats above all windows, including those of other applications.

Comment 3

[Neil Deakin]

Attachment: focus parent window on button press

This patch just calls SetFocus on the parent of the popup when the mouse button is pressed. Doesn't handle child frames but the patch seems to work ok for popups.

Comment 4

[Karl Tomlinson (:karlt)]

The main difference between GTK_WINDOW_POPUP and GTK_WINDOW_TOPLEVEL is that
GTK_WINDOW_POPUP indicates use of the override-redirect Window attribute.

http://tronche.com/gui/x/icccm/sec-4.html#s-4.1.10

"If the window will be visible for a very short time and should not be
decorated at all, the client can set override-redirect on the window. In
general, this should be done only if the pointer is grabbed while the window
is mapped.  The window manager will never interfere with these windows, which
should be used with caution."

The window manager will not give focus to an override-redirect window.

With noautohide="true", we should not use override-redirect (because the user
/ window manager cannot appropriately manage the popup window while the user
is interacting with other windows).

Comment 5

[Karl Tomlinson (:karlt)]

Attachment: don't let (level=parent) popups accept focus

However we can use input field of WM_HINTS to indicate that the popup should
not be given focus by the window manager.  This would make
GTK_WINDOW_TOPLEVEL windows behave wrt focus like GTK_WINDOW_POPUP windows.
That seems to make sense here.  Does that sound appropriate to you, Neil?

(The gtk_window_set_wmclass change is really just to avoid unncessary code.  We
shouldn't be using this function at all really as the application should use
the same WM_CLASS name and class for all windows, but this is just a small
step in that direction. )

Comment 6

[Karl Tomlinson (:karlt)]

There is still an interesting issue re what happens when the window owning the
panels doesn't have focus.  The user is not able to indicate that the window
manager should give the transient window focus, so must give the owning window
focus to direct keypresses at controls in the transient windows.

This is not really an issue with override-redirect windows because the
keyboard will be grabbed if the windows require input.

kwin (3.5 at least) by default works around this by hiding the transient
utility and toolbar windows when their owner does not have focus.  This
happens whether or not they have the input field of WM_HINTS set.  This
feature can be turned off in the advanced settings.

 (This hiding of windows may seem a bit strange for panels with
  titlebar != false that may be moved far from the owning window.
  For these windows, maybe we should consider using regular
  eWindowType_toplevel windows that do receive focus.)

There probably are other window managers that don't (always) hide the
transient windows when their owner is inactive, so it probably is still
worth doing something like attachment 445756 [details] [diff] [review] to request activation of the
owning window when the user clicks on the popup.  I'm still investigating
whether this is the appropriate way to do that.  (It shouldn't raise the window at least, merely request focus, and I haven't yet worked out the appropriate way to request focus.)

Comment 7

[Karl Tomlinson (:karlt)]

Attachment: don't let (level=parent) popups accept focus

as described in comment 5, but with a missing line restored.

Comment 8

[Karl Tomlinson (:karlt)]

It seems we have two options for noautohide popups:

A) They can be first class citizens receiving focus like their owners.

   This would be different from regular popups.

   With focus-follows-mouse window managers, mousing over the popup would
   activate the popup, but deactivate its parent window.

   With click-to-focus window managers, the user would need to click the popup
   to activate it. 

   Essentially the user would need to choose which window has focus.

B) They can be inferior to their owners.

   These would behave similar to regular popups until the active window
   changes to another app (or window group leader).

   The user can focus the owner as they do with most windows.

   We can do something like attachment 445756 [details] [diff] [review], so that the user can click on
   the popup to activate the owner window (and also focus the element in the
   popup).

   However, (on focus-follows-mouse window managers) mousing over the popup
   would not activate the popup.  (It is the owner that must be moused over.)

My impression is that:

B is best for small panels, where it is likely that the user will mouse over
the owner before trying to send keypresses to the panel.

A is best for titlebar != false that may be moved around.

There is a grey area in between, including panels beside their owners.
Perhaps its best to lean towards B here, and the user would need to click to
focus.

Comment 9

[Neil Deakin]

When I try just this latest patch, clicking on a textbox in a panel doesn't focus the textbox properly. (the window or panel is still inactive in some manner). When I try both patches together, I get the same result.

It seems to work ok with just my patch alone.

I regards to comment 8, method B sounds more like the way to go. What would, say, a popup of that type with a titlebar appearance's be when something was focused?

Comment 10

[Karl Tomlinson (:karlt)]

(In reply to comment #9)
> When I try just this latest patch, clicking on a textbox in a panel doesn't
> focus the textbox properly. (the window or panel is still inactive in some
> manner).

Thanks for testing, Neil.

My understanding is that the focus manager (or something) expects the owning
window (rather than the panel) to be the active window.

The latest patch prevents the panel from becoming active.
That is sufficient (only) for window managers that hide the panel when the
owning window is inactive.  With the panel absent, the user can only activate the owning window to show the panel and then may click on the textbox to focus
that particular element (but it is still the owning window that is active).

(For window managers that do not hide the panel, there is still the problem
that the user would need to activate the owning window before clicking on the
panel, and this doesn't really make sense when the panel is already visible.)

> When I try both patches together, I get the same result.

I assume this is because, with the latest patch, the panel window is not
active so gFocusWindow would be NULL, and so SetFocus() would not get called
with the SetFocus patch as it is now.

> It seems to work ok with just my patch alone.

Here that works immediately after clicking on the panel, but there is then
strange behavior that mousing over the owning window and then back over the
panel removes the textbox focus again.  Mousing back over the owning window
restores the textbox focus, but this is quite counter-intuitive.

Subsequent clicks on the panel flash the owner's decorations inactive for a
moment then back to active.

Comment 11

[Karl Tomlinson (:karlt)]

(In reply to comment #9)
> I regards to comment 8, method B sounds more like the way to go. What would,
> say, a popup of that type with a titlebar appearance's be when something was
> focused?

With kwin, the panel gets window manager decorations with a thinner titlebar.

With the latest patch the decorations always look inactive.

With only the SetFocus patch the panel looks active on mouse over, then
becomes inactive on click (when the owner becomes active).  Clicking again
flashes the decorations switching activity to the panel for a moment then back
to the owner.

I think the issue here is that the window manager only activates at most one
window and indicates activity only on that window.  I think this suggests that
A is a better method for titlebar panels.

Comment 12

[Karl Tomlinson (:karlt)]

There may be an option C that is more complicated but I think might give the
desired behavior (for non-titlebar panels).  Something like:

C) The panels change the input field of WM_HINTS dynamically, but only the
   owner window requests activation.

   When the owner is active, the input field of the panel is false.

     Thus mousing from the owner to the panel or clicking on the panel does
     not change the active window.

   When the owner is inactive, the input field is set to true.

     This ensures that the panel will receive WM_TAKE_FOCUS messages from the
     window manager when the user indicates that they wish to activate the
     panel (either with click-to-focus or focus-follows-mouse).

   When the panel window gets a WM_TAKE_FOCUS message, instead of requesting
   activation of the panel window, it requests activation of the owner window.

I think this should be possible within the bounds of X11/ICCCM/EWMH.  I'll have to see if this flexibility is available through GDK.

Comment 13

[Neil Deakin]

A is an ok option, so let's do just that. This appears to be what, for example, gimp does with its toolbox palette window.

Comment 14

[Karl Tomlinson (:karlt)]

Neil, do you mean A for titlebar panels only?

For situations like the testcase here with non-titlebar panels, A could get confusing with no (titlebar) indication of the active window.

What are the priorities and timeframes here?
Are titlebar panels wanted first or non-titlebar panels?
And which front-end features are needing these?

Comment 15

[Neil Deakin]

(In reply to comment #14)
> Are titlebar panels wanted first or non-titlebar panels?
> And which front-end features are needing these?

The Inspect panel wants floating panels with titlebars. (Tools -> Inspect). 

Various Firefox notification UI wants to use more panels but I think mostly they want non-noautohide panels which already work ok.

Panels should work as follows:

1. Non-noautohide panels disappear when clicking outside of them. They do not support titlebars but can be either at the 'parent' level or 'top' level.

2. 'Parent' level noautohide panels appear just above their parent. Their position is maintained relative to the owning window if the window is moved. Titlebars don't need to be supported on these if it isn't feasible. This type is used for the Customize Toolbar dialog on Mac.

3. 'Floating' level noautohide panels appear above their parent. They may or may not have a titlebar and can be moved independently. An optional feature is to have them disappear while the application or owner window is not active.

4. 'Top' level noautohide panels appear on top of windows of all applications. They may or may not have a titlebar.

The active titlebar look should apply to any panel with a titlebar while focused. The owning window may or may not have the active look (whichever is better to implement). The active titlebar should apply to the owning window for panels without titlebars.

Regardless, clicking on a textbox in any type of popup focuses it such that it can be typed into. This click-to-focus-textbox behaviour is preferred over the active titlebar state being correct.

I don't know enough about the focus-follows-mouse style UI to know if the behaviour should be treated differently.

Comment 16

[Karl Tomlinson (:karlt)]

(In reply to comment #15)
> (In reply to comment #14)
>
> Various Firefox notification UI wants to use more panels but I think mostly
> they want non-noautohide panels which already work ok.

> 1. Non-noautohide panels disappear when clicking outside of them. They do not
> support titlebars but can be either at the 'parent' level or 'top' level.

Yes, these should mostly work fine, though we're getting a bit lucky that bug
568101 doesn't prevent content in iframes from being focused in non-noautohide
panels.

I'm not grasping why anyone would want a 'parent' level non-noautohide panel.
IIUC this is so that the parent could be (partially?) hidden by another
application or browser main window, but there is no way to raise the panel to
make it (completely) visible.  (Clicking to lower the covering window would
initiate hiding of the panel.)

> 2. 'Parent' level noautohide panels appear just above their parent. Their
> position is maintained relative to the owning window if the window is moved.
> Titlebars don't need to be supported on these if it isn't feasible. This type
> is used for the Customize Toolbar dialog on Mac.

I think we can do non-titlebar versions of these with only widget
modifications, probably best with option C, though B is a fallback option that
would require clicking the panel to focus elements.

The window manager may choose whether or not to hide these when the owner is
not active.  Usually we should not try to override the window manager without
good reason, but we may be able to provide some different hints such that the
window manager is less likely to do this, if we think this is necessary.

Comment 17

[Karl Tomlinson (:karlt)]

Comment on attachment 445756 [details] [diff] [review]
focus parent window on button press

(In reply to comment #15)
> Regardless, clicking on a textbox in any type of popup focuses it such that it
> can be typed into. This click-to-focus-textbox behaviour is preferred over the
> active titlebar state being correct.
> 
> I don't know enough about the focus-follows-mouse style UI to know if the
> behaviour should be treated differently.

If the user clicks on textbox, mouses over another window and back to the
window with the textbox, they would expect the textbox to have focus without
requiring another click.

Really we should be using focus-in events rather than clicks to control
toplevel activation (NS_ACTIVATE).  (Clicks to control element focus is
fine/expected.)

r- because of issues in comment 10, and I think we can do much better.

Comment 18

[Karl Tomlinson (:karlt)]

(In reply to comment #15)
> The active titlebar look should apply to any panel with a titlebar while
> focused. The owning window may or may not have the active look (whichever is
> better to implement). The active titlebar should apply to the owning window for
> panels without titlebars.

I don't know any way to give the active look to the titlebars of two different
windows concurrently, so I suggest A here.

However, I think this will require focus manager support.

If the user has clicked in a textbox in a titlebar panel and then activates
the owner window, they would not expect their typing to go into the inactive
panel.

I think this needs the focus manager to consider the titlebar panel as its own
possibly-active toplevel window, and to consider the textbox in the titlebar
panel as being part of the panel window, not the owner window.
Is this possible?

I guess the other possibility is that the app draws the titlebar.  This will
lead to a titlebar that looks different from the rest of the system.  My
initial thoughts, if we were to do this, are that it would make more sense to
use xul to set up the titlebar controls rather than asking widget to do the
drawing.  Client-side decorations are evolving.  They may provide more options in the future.

Comment 19

[Neil Deakin]

> I'm not grasping why anyone would want a 'parent' level non-noautohide panel.
> IIUC this is so that the parent could be (partially?) hidden by another

The initial reason that the 'level' feature was added was because the ime popup on some platform(s) was incorrectly appearing below it. However, some popups wanted to be topmost anyway as they don't have text entry fields in them.


> Really we should be using focus-in events rather than clicks to control
> toplevel activation (NS_ACTIVATE).  (Clicks to control element focus is
> fine/expected.)

Are you treating NS_ACTIVATE as fired when:
 
1. The window level is changed so that the window is now in front
2. The focus is shifted to that window.

?

My interpretation of focus-follows-mouse is that 2 would be used. On my Ubuntu system, in this mode, the titlebar activates when I mouse over it.

Comment 20

[Karl Tomlinson (:karlt)]

(In reply to comment #19)
Yes, 2, NS_ACTIVATE as fired when focus is shifted to that window, which is when the titlebar activates.

(Window stacking order is independent.  I don't know of a good way to get notifications of changes in stacking order.)

Comment 21

[Karl Tomlinson (:karlt)]

Attachment: proof of concept for option C non-titlebar noautohide panels

This applies on top of attachment 447406 [details] [diff] [review] and attachment 447681 [details] [diff] [review].

It needs a more efficient way of finding noautohide transients from their owner.
(I have ~500 toplevel popup windows; I'm guessing they are from menus that are kept for the next time they might get used.)

Comment 22

[Karl Tomlinson (:karlt)]

Attachment: option C non-titlebar noautohide panels

Making the parent window active when noautohide panels receive focus.

Resolved the issues in attachment 447709 [details] [diff] [review].

Comment 23

[Karl Tomlinson (:karlt)]

Attachment: option C non-titlebar noautohide panels v1.1

Comment 24

[Neil Deakin]

I tried this patch and the patch in bugs 568404 and 568101, but I still cannot click on a textbox in a panel.

This is a very simple testcase:

<button label="Panel Button noautohide" type="panel">
  <panel noautohide="true">
    <textbox onfocus="dump('****focus:' + event.originalTarget.localName)"
             onclick="dump('****click')" onblur="dump('****blur')"/>
  </panel>
</button>

When the button is pressed, the panel opens, but the main window becomes deactive. Activating the main window causes clicks to work, but they do not work until then. The click event still fires though.

Comment 25

[Karl Tomlinson (:karlt)]

OK, thanks.
I'll set up metacity here to see if I can work out what is happening there.
(Sounds like focus is not being automatically transferred from popup to the main window.)

Comment 26

[Karl Tomlinson (:karlt)]

Hmm, when the popup gets mapped, metacity (2.24.0) is removing focus from the main window, but not even giving focus to any other window.
That seems strange to me.  I'll need to look at the source code to work out what metacity is trying to do.

Comment 27

[Karl Tomlinson (:karlt)]

(In reply to comment #26)
> Hmm, when the popup gets mapped, metacity (2.24.0) is removing focus from the
> main window, but not even giving focus to any other window.

That seems to be a widespread issue with metacity when transient windows are
shown but don't receive focus.

I've filed https://bugzilla.gnome.org/show_bug.cgi?id=621848 on this.

We could probably work around this by using _NET_WM_WINDOW_TYPE_MENU instead
of _NET_WM_WINDOW_TYPE_UTILITY so that place_on_top_on_map is not set to
FALSE.
http://git.gnome.org/browse/metacity/tree/src/core/window.c?id=b272c4ca7f3a784d011eea5c3fd348011f8558b3#n2014

But _NET_WM_WINDOW_TYPE_UTILITY seems appropriate from the description here:
http://standards.freedesktop.org/wm-spec/1.4/ar01s05.html#id2568975

This is really a separate issue and doesn't need to be fixed to resolve the
click to focus issue reported in comment 0.

Comment 28

[Karl Tomlinson (:karlt)]

Attachment: don't let (level=parent) popups accept focus v2

The biggest issue with the previous patch on metacity was that metacity was
ignoring (within its rights) the request to focus the parent window because
the timestamp was older than the time of last user action.  The place to
obtain the timestamp is the WM_TAKE_FOCUS message.  Filtering for that message
makes all of this much simpler anyway.

Comment 29

[Neil Deakin]

Comment on attachment 451822 [details] [diff] [review]
don't let (level=parent) popups accept focus v2

This works great!

The only issue is that when a panel is opened the parent window loses focus. Not a big concern though and could be investigated in a followup bug.

How feasible is creating noautohide popups that stay on top?

Comment 30

[Karl Tomlinson (:karlt)]

(In reply to comment #29)
> The only issue is that when a panel is opened the parent window loses focus.
> Not a big concern though and could be investigated in a followup bug.

Yes, that's the metacity issue.

We could use _NET_WM_WINDOW_TYPE_MENU instead of _NET_WM_WINDOW_TYPE_UTILITY
when the window manager is metacity.  Should check that any fancy effects that metacity does when compositing look reasonable though.  I'm guessing an animated drop-down effect would probably look OK for most panels.  If a drop-down effect is always sensible for these panels then maybe we could set safely use _NET_WM_WINDOW_TYPE_MENU.  (There's some history here that I can't remember OTTOMH.)

> How feasible is creating noautohide popups that stay on top?

Making them stay on top of all application windows is probably just gtk_window_set_keep_above().
The tricky part might be working out when it should no longer obscure windows from other apps.
I'm not clear on the purpose for staying on top.
Perhaps it makes more sense for popups with titlebars, but those popups I think will need the focus manager to recognize them as a separate toplevel window (comment 18).

Comment 31

[Karl Tomlinson (:karlt)]

http://hg.mozilla.org/mozilla-central/rev/6811406f7845