568101 - gFocusWindow not set on SetFocus(PR_FALSE); focus issues with iframes in noautohide panels

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Karl Tomlinson (:karlt)]

STR:
1. Save attachment 410702 [details] to a local file "paneltest-simple.xul".
2. (Check that it looks safe to run as chrome.)
3. firefox -chrome paneltest-simple.xul
4. Click on "Panel Button noautohide".
   (A panel opens.)
5. Click on the text input (below "Hello") in the iframe in the panel.
6. Focus the parent window.
   (Text input appears focussed.)
7. Type.

Actual results: nothing.
Expected results: characters appear in the text input.

I suspect this might be a regression from some of the changes in bug 506173, where I didn't grasp that the focus manager was using SetFocus to indicate which
nsIWidget should dispatch key events.  (In GTK there is only one GtkWidget
within each toplevel that receives key events.)

The issue shows up with noautohide panels because, when the parent window is
activated, it call SetFocus with the nsIWidget in the panel.  SetFocus notices
that the GtkWidget associated with the panel does not have focus (because the
panel window is not the active window), and so (incorrectly) delays setting
gFocusWindow until the panel becomes active.  (The focus manager does not call SetFocus for the input when the panel becomes active because it - or something - thinks of the input as being associated with the parent window.)

(The textbox not in the iframe seems to work OK because the textbox seems to
know that it is associated with an nsIWidget in the parent window and so
SetFocus is called on an nsIWidget in the active parent window.)

(The text input in the iframe happens to work OK for regular override-redirect
popups with mouse and keyboard grab ("Panel Button"), because the parent
window never becomes inactive and so the old value for gFocusWindow is never unset.)

Comment 1

[Karl Tomlinson (:karlt)]

Attachment: use the value of aRaise in SetFocus to determine whether to set gFocusWindow

When aRaise is false it is because the focus manager knows that the appropriate toplevel window is already active, so gFocusWindow can always be set.

Much of my understand here comes from bug 502123 comment 4.

(Will do some testing with this.)

Comment 2

[Karl Tomlinson (:karlt)]

There's a similar issue in OnContainerFocusOutEvent.  It is currently only sending NS_DEACTIVATE if gFocusWindow is a child of the container widget in the previously active parent window.  That test fails because gFocusWindow is in a different toplevel window.

I might try just always sending NS_DEACTIVATE from OnContainerFocusOutEvent.
AFAICS that's what it should do anyway.

Comment 3

[Neil Deakin]

(In reply to comment #0)
> STR:
> 1. Save attachment 410702 [details] to a local file "paneltest-simple.xul".
> 2. (Check that it looks safe to run as chrome.)
> 3. firefox -chrome paneltest-simple.xul
> 4. Click on "Panel Button noautohide".
>    (A panel opens.)
> 5. Click on the text input (below "Hello") in the iframe in the panel.
> 6. Focus the parent window.
>    (Text input appears focussed.)
> 7. Type.

Step 6 shouldn't be there. The textbox should be focused at step 5.

Comment 4

[Karl Tomlinson (:karlt)]

(In reply to comment #3)
> > 5. Click on the text input (below "Hello") in the iframe in the panel.
> > 6. Focus the parent window.
> >    (Text input appears focussed.)
> > 7. Type.
> 
> Step 6 shouldn't be there. The textbox should be focused at step 5.

Yes, the textbox should be focused at step 5, but step 6 should be there for this bug report.
I'm assuming bug 526941 covers the focus issues on step 5.

This bug covers an additional problem (the STR are really just a way to demonstrate the problem that I saw in the code), and I think it might be one that will need to be fixed to sort out bug 526941.

I assume that for non-titlebar panels we'd like the textbox to remain focused when the parent window becomes active (through mouseover or titlebar click).

Comment 5

[Karl Tomlinson (:karlt)]

Filed bug 568404 for the OnContainerFocusOutEvent/NS_DEACTIVATE issue in comment 2.

Comment 6

[Karl Tomlinson (:karlt)]

Attachment: use the value of aRaise in SetFocus to determine whether to set gFocusWindow v2

Similar to version 1 but updated for removing of mContainerBlockFocus in attachment 448390 [details] [diff] [review] and some minor tidying.

The key thing in this patch is that SetFocus can now behave appropriately when the widget from which keyboard events will be dispatched is not a descendant of the active toplevel window.

Comment 7

[Karl Tomlinson (:karlt)]

Attachment: use the value of aRaise in SetFocus to determine whether to set gFocusWindow v2.1

merged with m-c.

Comment 8

[Karl Tomlinson (:karlt)]

http://hg.mozilla.org/mozilla-central/rev/ad7472d7c040