502123 - Switching focus out of a windowed plugin raises the toplevel window

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[Karl Tomlinson (:karlt)]

STR:

1. Ensure your window manager is configured so that clicking in a window does
   not raise the window.

2. Cover part (e.g. right hand edge) of a browser window viewing about:blank.

3. Click on the document and on the search bar.
   (Appropriately neither click raises the window.)

4. Load http://www.communitymx.com/content/source/E5141/wmodenone.htm (with
   a flash-capable plugin enabled).

5. Click on the plugin.  (Window is still not raised as expected.)

6. Click on the document outside the plugin.

Actual results: window raised.

Expected: window not raised.

Comment 1

[Karl Tomlinson (:karlt)]

This seems to be due to an inconsistency in expectations of the behavior of
nsIWidgetSetFocus(PRBool aRaise).

The MOZ_ENABLE_GTK2 implementation assumes that the argument to the method
indicates whether the window should be raised.

Bug 498143 comment 29 seems to indicate that there is an expectation that only
toplevel windows will effect a raise.

Comment 2

[Neil Deakin]

(In reply to comment #1)
> The MOZ_ENABLE_GTK2 implementation assumes that the argument to the method
> indicates whether the window should be raised.
> 
> Bug 498143 comment 29 seems to indicate that there is an expectation that only
> toplevel windows will effect a raise.

By that comment I meant that when SetFocus is called on a child window, a raise doesn't need to occur (as it is either already raised, or can be ignored).

That said, I'm not sure the parameter to SetFocus is relevant. It's only ever called with a value of PR_TRUE, both before and after the focus changes in bug 178324.

Comment 3

[Karl Tomlinson (:karlt)]

There needs to be some way of knowing whether to raise the window on SetFocus (because SetFocus seems to be the way to raise windows).

Perhaps we are fortunate at the moment that focus only ever needs to be given to child nsIWidgets, so we can assume that a SetFocus on a toplevel nsIWidget indicates that the window should be raised?

But if we can't be sure that toplevel nsIWidgets will never receive focus, then some other mechanism we need to be used.  The aRaise parameter seems a possible mechanism, if we make sure that the callers pass an appropriate value.

Currently, some callers must be passing aRaise == PR_TRUE when the window should not be raised.

This bug existed (probably long) before bug 178324 was fixed.

Comment 4

[Neil Deakin]

For reference, here are the callers of SetFocus, all within nsFocusManager.cpp:

1. When an element is focused, ensure the widget for its window is focused.
2. When a plugin is focused, focus the plugin's widget.
3. When a plugin is blurred, but not because the window is lowered, focus the parent widget of the plugin.
4. After a window is raised, ensure that the correct child widget is focused. This is a special case that was added to handle a Windows issue, although it still occurs on all platforms.
5. Called on a toplevel widget to raise it. (window.focus() for instance)

In cases 1-4, SetFocus is called on a child widget and the window is known to already be raised.
In cases 5, SetFocus is called on a toplevel widget that is not already raised.

Comment 5

[Neil Deakin]

Attachment: pass false to SetFocus if window raising should not be done

Seems to work ok but I haven't tested it extensively.

Comment 6

[Neil Deakin]

Karl, does this patch work for you?

Comment 7

[Karl Tomlinson (:karlt)]

Comment on attachment 401225 [details] [diff] [review]
pass false to SetFocus if window raising should not be done

Yes, looks and works great, thanks.

windows/nsWindow.cpp and cocoa/nsChildView.mm ignore the aRaise parameter so there should be no effect there.

cocoa/nsCocoaWindow.mm pays attention to the parameter, which seems right to me but I'm not really qualified to comment on the cocoa widgets.

Comment 8

[Jim Mathies [:jimm]]

http://hg.mozilla.org/mozilla-central/rev/25254f456f03

Comment 9

[Jim Mathies [:jimm]]

Comment on attachment 401225 [details] [diff] [review]
pass false to SetFocus if window raising should not be done

This is part of a fix for blocking 1.9.2 bug 509449 - "Minimized browser window restores during page loads"

Comment 10

[Robert Sayre]

no test?

Comment 11

[Jim Mathies [:jimm]]

(In reply to comment #10)
> no test?

I would think all the focus tests we already have would run this through it paces, but i can look at adding some. Neil, any suggestions?

Comment 12

[Neil Deakin]

We don't have any plugin focus tests currently. Testing this presumably would just involve focusing a plugin, then making sure that FocusManager.activeWindow and focusedWindow were correct.

Comment 13

[Karl Tomlinson (:karlt)]

Testing based on the STR in comment 0 would be difficult, because Mozilla doesn't record z-order (bug 156333).
However, if switching focus to and then from a windowed plugin used to cause a minimized window to restore then that could probably be tested with FocusManager.activeWindow.  Remember, though, the activation of the window would be asynchronous, so the test would need to wait some undetermined length of time to see whether the window was (incorrectly) activated.

Comment 14

[Neil Deakin]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/64924698dedc