506173 - don't dispatch activate events from nsIWidget::SetFocus() because the window is not active

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Karl Tomlinson (:karlt)]

In bug 178324, the GTK version of nsIWidget::SetFocus() was changed so that
when called on a toplevel window it always marks the toplevel as active (having
toplevel focus) even though it is not yet active.

http://hg.mozilla.org/mozilla-central/annotate/74959aad851c/widget/src/gtk2/nsWindow.cpp#l1396

This could result in what Mozilla's focus manager thinks is the active window
getting out of sync with what the window manager and user understand as the
active window.  If this happens and the user sends keyboard input to the
window that is active (as determined by the window manager) then the focus
manager can redirect the event to what it thinks is the active window (which
would be the wrong window).

It seems that this code is only here to make tests pass.  A number of tests
are calling focus() on the window in the expectation that this will make the
window respond to synthesizeKey().  The code essentially emulates the window
being active.  The window may not necessarily become active depending on how
the window manager responds to gtk_window_present() and what else is
happening on the Display.

Comment 1

[Karl Tomlinson (:karlt)]

Attachment: testcase

Testcase (derived from that of bug 299673)

STR:

1. With kwin-3.5 configure Desktop -> Window Behavior -> Advanced -> "Focus
   stealing prevention level": Extreme.

2. Make the browser window wide but short.

3. Open test case in browser.

4. Click "Click here then type Alt-F" button.

   new window opens but first window is still active.

3. Type "Alt-F".

Expected results: File menu in active window should open.

Actual results: File menu in new (inactive) window opens.

Comment 2

[Karl Tomlinson (:karlt)]

Attachment: fix

mContainerBlockFocus is should now only be needed because SetModal sets it to
true.  The code has changed somewhat since this was added here:

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/widget/src/gtk2&command=DIFF_FRAMESET&file=nsWindow.cpp&rev2=1.8&rev1=1.7

but I can't imagine why this was needed to prevent recursion as
gtk_widget_grab_focus will not be called if the widget already has focus.

If gtk_widget_grab_focus() causes a focus-in signal (due to taking focus from
a plugin widget) then we should dispatch the activate event (through
OnContainerFocusInEvent) due to bug 502128 (because the toplevel window is
active).

gFocusWindow is a bit strange because it is a global focus-and-toplevel-active
window but gtk_widget_grab_focus() only sets the focus widget for the
associated toplevel window (without making it active).
OnContainerFocusInEvent() sets gFocusWindow (and that is probably the best
place to set it at least until it gets a rethink for bug 502128).

Comment 3

[Neil Deakin]

(In reply to comment #2)
> Created an attachment (id=390603) [details]
> fix

Karl, do you want to someone to review this, or is there still work to be done here?

Comment 4

[Karl Tomlinson (:karlt)]

It's ready for review if you'd like to, Neil, but we can't land it before bug 506175 is fixed.

Comment 5

[Neil Deakin]

OK, so this is ready for check in.

Comment 6

[Karl Tomlinson (:karlt)]

http://hg.mozilla.org/mozilla-central/rev/329ff0fcd420