Closed Bug 498143 Opened 15 years ago Closed 15 years ago

Flash in background tab consumes input events

Categories

(Core :: XUL, defect)

Product:
Core
Component:
XUL
Platform:
All
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla1.9.2a1

People

(Reporter: sparky, Assigned: enndeakin)

References

Details

(Keywords: regression)

Attachments

(1 file, 2 obsolete files)

fix v3
1.62 KB, patch
enndeakin
: review+
Details | Diff | Splinter Review 
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090612 Minefield/3.6a1pre  Firefox/3.6
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090612 Minefield/3.6a1pre  Firefox/3.6 ID:20090612030821

If a plugin (Flash) has focus, clicking on the browser chrome (or HTML form elements) will not change focus. This is particularly problematic when switching tabs via clicking on the tabbar.

Reproducible: Always

Steps to Reproduce:
1. Focus a Flash element
2. Click on any part of the browser UI or change tabs via clicking the tabbar
3. Attempt to use keyboard shortcuts, the URL bar, or form elements
4. Close background tab which has focused Flash element
5. Attempt to use keyboard shortcuts, the URL bar, or form elements
Actual Results:  
After step 1, all input is consumed by the Flash element, even after closing its tab. You have to click on page content in order to switch focus.

Expected Results:  
Clicking on any non-flash element should switch focus
Blocks: 178324
Keywords: regression
Version: unspecified → Trunk
Don't see this problem myself on Mac or Linux.
WFM also on windows.

Please reopen if you can reproduce using a new profile.
Status: UNCONFIRMED → RESOLVED
Closed: 15 years ago
Resolution: --- → WORKSFORME
I tried with a fresh download, clean profile, and disabled all plugins except Flash. I still get the problem.
Status: RESOLVED → UNCONFIRMED
Resolution: WORKSFORME → ---
I can reproduce this. It doesn't happen with all Flash elements, but I see this with Youtube videos. I tested with plugin version "10.0 r22" (both 32 and 64 bit).
Status: UNCONFIRMED → NEW
Ever confirmed: true
Hardware: x86 → All
No longer blocks: 178324
Status: NEW → RESOLVED
Closed: 15 years ago → 15 years ago
Resolution: --- → DUPLICATE
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Neil, why this is not a dupe of bug 498075 or at least the other way around?
This bug seems to be Linux specific, and I can't reproduce bug 498075 on Linux.
(In reply to comment #7)
> Neil, why this is not a dupe of bug 498075 or at least the other way around?

Why do you think they would be? Both have different symptoms and one only occurs on Windows and the other has only been reported on Linux.
Ok, you are right. My fault, sorry.
Neil, the tryserver build you have created on bug 498075 doesn't solve this problem for me.
I still can't reproduce this bug.

1. Focus a flash element
2. Click on another tab or click on a textbox
3. Keyboard entry works fine
This bug is reproducible on Windows (NOT fixed by patch in bug 498075) as follows:

1. Visit youtube.com
2. Click a video to start it playing
3. Click the video to give the flash object focus
4. Press CTRL + L (ordinarily to focus the address bar)
5. BUG: Address bar is not focused.
6. Click a blank space on the page (i.e. other than the video)
7. Press CTRL + L
8. Now address bar is focused.
Yeah, I managed to reproduce it and am investigating.
(In reply to comment #13)
> 1. Visit youtube.com
> 2. Click a video to start it playing
> 3. Click the video to give the flash object focus
> 4. Press CTRL + L (ordinarily to focus the address bar)
> 5. BUG: Address bar is not focused.

This is not the same issue, but rather bug 78414 which is not a regression from the focus refactoring.
(In reply to comment #15)
> This is not the same issue, but rather bug 78414 which is not a regression from
> the focus refactoring.

Man, that bug is oooooooooold.  Who knows.  Maybe a portion of that is contributing to this bug.
Attached patch fix (obsolete) — — Splinter Review 
Make sure when clicking on a window to activate the top level window
Assignee: nobody → enndeakin
Status: REOPENED → ASSIGNED
Tryserver build result on Windows:

1. Comment 0: Not fixed. (see note)
2. Comment 13: Not fixed.

NOTE: I was finally able to reproduce Comment 0 on Windows XP, but only if the reporter's Step 2 read, "Click on any part of the browser UI, [BUT DO NOT] change tabs or click on the content area."  Changing tabs, just like clicking the content area, cancels the effects of the for me.
This is a linux specific bug and patch. If there's some other bug on Windows that isn't already filed, please file a different bug. Thanks!
All I'm pointing out is that it is fully reproducible on Windows, with the caveat concerning the reporter's Step 2.
Are you sure you want me to dupe this for Windows?
Tryserver build fixes the problem for me. This also fixes a problem I just ran in to, where some flash video players (roosterteeth.com for example) will immediately exit out of fullscreen mode.

Thanks :)
This is not a regression on Windows! This is Linux only. And yes, the tryserver build fixes the problem in using shortcuts while a plugin in a background tab has focus.
Comment on attachment 383524 [details] [diff] [review]
fix

This patch changes nsWindow::OnButtonPressEvent so that it looks for the toplevel window and activates it when a plugin is focused and the button is pressed elsewhere.
Attachment #383524 - Flags: review?(mozbugz)
The cause of the problem here seems to be a mismatch between GtkWidget
focus-in/out-event signals, which indicate which *widget* has keyboard focus,
and NS_ACTIVATE/DEACTIVATE events which seem to be associated with which
toplevel *window* is active.

When the GtkWidget is given focus, the container widget loses focus.  This
causes a focus-out-event on Mozilla's container widget and
OnContainerFocusOutEvent() responds to this by sending a NS_DEACTIVATE event,
deactivating the window.

When the mouse is clicked outside the plugin, sometimes a nsIWidget::SetFocus
is called, but, when it doesn't get called, subsequent keyboard events are lost.

This could probably be resolved by listening for notify::is-active or
notify::has-toplevel-focus (or set-focus) signals on the toplevel GtkWidget
mShell (which is a GtkWindow) instead of the focus-in/out-event signals, and
so the NS_DEACTIVATE event would not be sent until the toplevel window is
deactivated.  However that may be more of a change than you want to try right
now.

The work-around for this mismatch used to be the DispatchActivateEvent() call
from OnButtonPressEvent().  The assumption was that, if a button press was
received by Mozilla, then Mozilla should assume it is active.

What stopped the work-around from working was the change in bug 178324 to
DispatchActivateEvent so that it does nothing on non-toplevel windows.

(This bug was sometimes present even before the change in bug 178324.  If a
document was focused before clicking to focus the plugin, then clicking on a
space in the bookmarks toolbar or the menu bar didn't activate the
window.  I don't know what was up here, but changes in bug 178324 seem to have
fixed something here.)

The container window is the window at the top of Gecko's window hierarchy so
the code in this patch to get the toplevel window is duplicating much of what
GetContainerWindow() does.  containerWindow->DispatchActivateEvent() would be
less code and would work just as well.  (It is equivalent because of the check
for mIsTopLevel in DispatchActivateEvent.)

Even with this fix, there is still a behavior change in OnButtonPressEvent
from bug 178324 for embedded windows.  Previously they would send the activate
event, but now they do not (because the toplevel window is not an nsWindow).
I think that is probably a sensible change as OnContainerFocusInEvent did not
send the NS_ACTIVATE event when embedded, so embedding apps probably use their
own method for detecting activation, which would probably not need this
workaround.  Does that seem a reasonable assumption to you?

SetFocus() is currently also calling DispatchActivateEvent on non-container
windows (which will do nothing).  Does SetFocus need to dispatch the activate
event?  Before bug 178324 was fixed, it only sent an activate event during DispatchGotFocusEvent() from OnContainerFocusInEvent(), which now no-longer happens.  I expect the DispatchActivateEvent calls can be removed from SetFocus.

Can an assert be added to DispatchActivateEvent to check it is only called on
container windows?  (mContainer || mIsDestroyed) should be true.  It seems an
error to call this on non-container windows.

Also, the removal of LoseFocus in bug 178324 means that mKeyDownFlags is not
cleared on focus change.  That would affect whether NS_KEY_DOWN events are
sent or only NS_KEY_PRESS (iff the key is one that repeats) when focus changes
while a key is down.  Was that intentional?
Blocks: 178324
(In reply to comment #26)
> The cause of the problem here seems to be a mismatch between GtkWidget
> focus-in/out-event signals, which indicate which *widget* has keyboard focus,
> and NS_ACTIVATE/DEACTIVATE events which seem to be associated with which
> toplevel *window* is active.
> 

blizzard's comments in bug 178324 implied that the active status wasn't relevant. Is this not the case?

> When the mouse is clicked outside the plugin, sometimes a nsIWidget::SetFocus
> is called, but, when it doesn't get called, subsequent keyboard events are
> lost.

When would each situation occur? Does it depend on what was clicked?

> The container window is the window at the top of Gecko's window hierarchy so
> the code in this patch to get the toplevel window is duplicating much of what
> GetContainerWindow() does.  containerWindow->DispatchActivateEvent() would be
> less code and would work just as well.  (It is equivalent because of the check
> for mIsTopLevel in DispatchActivateEvent.)

I tried that but it didn't seem to work.

> SetFocus() is currently also calling DispatchActivateEvent on non-container
> windows (which will do nothing).  Does SetFocus need to dispatch the activate
> event?  Before bug 178324 was fixed, it only sent an activate event during
> DispatchGotFocusEvent() from OnContainerFocusInEvent(), which now no-longer
> happens.  I expect the DispatchActivateEvent calls can be removed from
> SetFocus.

Possibly. I assume it works fine when calling setfocus on a toplevel window? Or any window?


> Also, the removal of LoseFocus in bug 178324 means that mKeyDownFlags is not
> cleared on focus change.  That would affect whether NS_KEY_DOWN events are
> sent or only NS_KEY_PRESS (iff the key is one that repeats) when focus changes
> while a key is down.  Was that intentional?

No, seems like something to fix.

Thanks for the detailed analysis!
(In reply to comment #27)
> (In reply to comment #26)
> 
> blizzard's comments in bug 178324 implied that the active status wasn't
> relevant. Is this not the case?

It seems that (some) code in Mozilla is using the active status to determine
how to respond to button events.  I'm assuming that because sending
NS_ACTIVATE seems to change whether SetFocus is called (sometimes).

In bug 178324 comment 6 blizzard said:

  o remove activation/deactivation:

  X doesn't really have this concept and I think we could save a lot of code,
  both for unix and for other platforms, by using focus changes to the
  toplevel window instead of activation.

  If you are worried about having to check to see whether or not the window
  can be activated before raising the window, this can be done by always
  calling down to the nsIWidget::SetFocus();

I'm not clear exactly what activation/deactivation meant at the time of this
comment.

You probably already know much of what I'll describe here, but half the reason
that I'm writing it down is so that you can let me know if my picture of
things is different to yours.

In X, the keyboard is always attached to some window, which is typically a
toplevel level (or, in GDK, a proxy window acting on behalf of the toplevel
window).  http://tronche.com/gui/x/xlib/events/input-focus/
So, in this sense there is at most one active toplevel window.

GTK also keeps track of which child widget within the toplevel window it would
pass keyboard events, and GTK notifies that widget when it receives focus.
That notification (focus-in-event) happens only when the widget is (or
becomes) the widget that would receive the toplevel window's keyboard events
and the toplevel window is (or becomes) active.

Events that are not handled by the widget propagate to the parent (but it
looks like the Flash plugin may be preventing all (or at least most) of the
keyboard events from propagating.

The notification on the widget is sufficient for the widget to determine
whether it should show a cursor or other visual focus feedback, so there is
rarely a need to know (independently of widget focus) whether the toplevel
window is active.  (Maybe this is what blizzard was referring to.)  The window
manager usually provides the visual active-toplevel-window feedback.

If there are multiple regions within the widget that can receive keyboard
events, then the widget can also keep track of which part of the widget would
receive keyboard events (and Mozilla does this within the container widget).

Almost everything in Mozilla is within one container widget, so for most
things there is no difference between widget focus and toplevel window's
active state.

The one exception is windowed plugins.  These have their own GtkWidget.  If
the plugin widget grabs focus (on a button event, f.e.), then Mozilla's
container loses focus.  The way that the container currently gets the focus
back is through gtk_widget_grab_focus from nsWindow::SetFocus.  If
nsIWidget::SetFocus is not called the container does not get focus.

Is it reasonable to expect that nsIWidget::SetFocus should get called?

> > When the mouse is clicked outside the plugin, sometimes a
> > nsIWidget::SetFocus is called, but, when it doesn't get called, subsequent
> > keyboard events are lost.
> 
> When would each situation occur? Does it depend on what was clicked?

Yes, it depends on what is clicked.  Clicking in content (to focus the whole
document) causes SetFocus to be called.  The only places in chrome that I have
found where clicking causes SetFocus to be called (when not active) are the
urlbar icon and the search bar menu (and the home and new tab buttons, but
that's kind of expected).

> > containerWindow->DispatchActivateEvent() would be
> > less code and would work just as well.
> 
> I tried that but it didn't seem to work.

That seems strange.  This seemed to work for me:

@@ -2773,5 +2773,5 @@ nsWindow::OnButtonPressEvent(GtkWidget *
     if (!gFocusWindow && containerWindow) {
         gFocusWindow = this;
-        DispatchActivateEvent();
+        containerWindow->DispatchActivateEvent();
     }

I tested on youtube and
http://www.communitymx.com/content/source/E5141/wmodenone.htm

Can you describe what didn't work for you?

I don't really mind of the patch looks for a toplevel window instead of a
container window, so long as it doesn't do both.
The GetToplevelWidget/get_window_for_gtk_widget pattern is repeated a bit, so
I'd prefer having a GetToplevelWindow method over adding another repetition.

(The only time they should be different though is if the toplevel window is a
Gecko window but there is a non-Gecko window between that and the child
window.  Embedded Gecko in a plugin perhaps - not sure what we'd want there.)

> > SetFocus() is currently also calling DispatchActivateEvent on
> > non-container windows (which will do nothing).  Does SetFocus need to
> > dispatch the activate event?  Before bug 178324 was fixed, it only sent an
> > activate event during DispatchGotFocusEvent() from
> > OnContainerFocusInEvent(), which now no-longer happens.  I expect the
> > DispatchActivateEvent calls can be removed from SetFocus.
> 
> Possibly. I assume it works fine when calling setfocus on a toplevel window?
> Or any window?

SetFocus seems to be achieving what it needs to when called on any window.
The purpose of SetFocus when called on a toplevel window is the raise the window. For child windows, its purpose is to ensure that the native platform should treat that child window as focused (this latter point also applies to toplevel windows of course).

There are several times when SetFocus can be called.

- when an dom element in a window/child window is focused, SetFocus is called on that window.
- when a plugin is focused, its widget is focused.
- when a plugin is blurred without lowering the window, to shift native focus to the parent.
- when a toplevel window should be raised.
- a special case to ensure the right native widget is focused when receiving a toplevel window activation. This is needed on Windows to handle minimization/restoring properly. 

It's only necessary to call SetFocus if the underlying platform's notion of what is focused isn't what the mozilla focus system thinks it should be.
 
I think the issue here is that the mouse event on the parent while the plugin widget is focused isn't being treated as an attempt to shift focus to the parent window.
Actually now that I think about it, I seem to remember that an attempt to focus a windowed plugin on Linux is treated as an attempt to lower the window. This in fact seems like the bug. This should instead just be treated as an attempt to focus the plugin.
(In reply to comment #29)
> The purpose of SetFocus when called on a toplevel window is the raise the
> window. For child windows, its purpose is to ensure that the native platform
> should treat that child window as focused (this latter point also applies to
> toplevel windows of course).

Very interesting, thanks.

The implementation of SetFocus for MOZ_WIDGET_TOOLKIT=gtk2 currently does not
understand this distinction between toplevel and child windows.  It instead
uses the aRaise parameter in deciding whether the window should be raised -
only this parameter always seems to be true.

This means that:

a) SetFocus will try to raise the window even when called for child windows.

   The implementation actually looks like it contain a partial work-around due
   to thinking it was being asked to raise windows too often: it only raises
   the window if the container widget does not already have focus (which only
   partially works around the bug here).

b) SetFocus will change the widget that receives keyboard events even when
   called on toplevel windows.  As there is usually only one widget, this
   would only steal focus from a plugin widget.

> - when a plugin is focused, its widget is focused.

That actually won't currently behave as expected with MOZ_WIDGET_TOOLKIT=gtk2,
but I haven't seen any problems (though I haven't looked).  The plugin's
nsIWidget/nsWindow is actually considered part of the container GtkWidget.  So
calling SetFocus on the nsIWidget will actually shift focus away from the
plugin's GtkWidget.

> I think the issue here is that the mouse event on the parent while the plugin
> widget is focused isn't being treated as an attempt to shift focus to the
> parent window.

Yes, and the fix here suggests that this is because the active window has been
marked inactive.
(In reply to comment #30)
> Actually now that I think about it, I seem to remember that an attempt to
> focus a windowed plugin on Linux is treated as an attempt to lower the
> window.

Actually it is being treated as notification that the toplevel window has been
made inactive.  This is because the focus-out-event on the container GtkWidget
is sending the NS_DEACTIVATE event.

Activation and window zlevel are states that change independently in X11.

> This in fact seems like the bug. This should instead just be treated
> as an attempt to focus the plugin.

Yes, this is a bug.  That was why i was suggesting using the is-active or
has-toplevel-focus properties for detecting activation better.

Detecting that the plugin has focus may be more difficult.

Notifying the focus manager that nothing has focus would probably be an
improvement over the current situation, but guess we'd really want to notify
which object/embed element has focus.

It looks like the set-focus signal on the top-level GtkWidget would tell us
when the focus widget changes.  (It wouldn't tell us about
activation/deactivation while that widget is the focus widget).
However, getting from that GtkWidget to the object/embed element is a bit
inconvenient.

Possibly better would be to detect this from the plugin code using the
set-focus-child signal (which is also independent of activation) on the
GtkSocket.

Fixing this may also illuminate some latent bugs.
e.g. Calling SetFocus on the plugin nsIWidget may start to become a problem.
We should be able respond to SetFocus differently on plugin nsWindows though.

There may be other ways that a MozContainer GtkWidget can lose focus, in which
case the focus manager should be told about it.
(In reply to comment #29)
> It's only necessary to call SetFocus if the underlying platform's notion of
> what is focused isn't what the mozilla focus system thinks it should be.

It seems the MOZ_WIDGET_TOOLKIT=gtk2 code was also relying on the SetFocus call when focus moved between DOM elements within the same nsIWidget, to clear KeyDownFlags, though I don't know whether this was happening, or even what the correct behavior really is.
(In reply to comment #33)
> It seems the MOZ_WIDGET_TOOLKIT=gtk2 code was also relying on the SetFocus call
> when focus moved between DOM elements within the same nsIWidget, to clear
> KeyDownFlags, though I don't know whether this was happening, or even what the
> correct behavior really is.

Argh.  It wasn't doing that before anyway (except when the GtkWidget changed).
(In reply to comment #34)
> Argh.  It wasn't doing that before anyway (except when the GtkWidget changed).

Err, I mean except when the nsIWidget changed.
It might be good to find out how Windows deals with this case. I don't recall offhand but won't have access to a Windows machine for a bit.

But it sounds like separating out the concept of focus and activation may be a way to go.
Yes, doing something closer to what the code for Windows plugins does is probably sensible.

(In reply to comment #32)
> Possibly better would be to detect this from the plugin code using the
> set-focus-child signal (which is also independent of activation) on the
> GtkSocket.

The GtkSocket actually won't get the set-focus-child signal.
IIUC when the GtkPlug (or a child widget) in the plugin gets focused, this is considered a separate toplevel widget/window (even when in the same process).  The GtkSocket becomes the focus widget in its toplevel window.

So there are a few useful signals on the GtkSocket.

The is_focus property indicates whether the GtkSocket is the focus widget (plugin is the focus element), and is independent of activation.

The has_focus property is true when both the GtkSocket is the focus widget and the toplevel window is active.

notify::is_focus and notify::has_focus signals indicate that there (may) have been changes in these properties.

The GtkSocket should also receive focus-in-event/focus-out-event signals when the has_focus property changes (unless another handler has stopped propagation, which would be unlikely).
Karl, are you still looking at this, or do you want a new patch? It seems you might be better a writing a better patch here.
Attached patch fix v2 (obsolete) — — Splinter Review 
Let's just restore the old workaround for now.

This seems to work for me, but, Neil, you said it didn't seem to work for you.
Can you test this, please, and let me know what doesn't work?
Attachment #383524 - Attachment is obsolete: true
Attachment #386618 - Flags: review?(enndeakin)
Attachment #383524 - Flags: review?(mozbugz)
Filed bug 502123 and bug 502128 on some of the other issues discussed here.
Attachment #386618 - Flags: review?(enndeakin) → review+
http://hg.mozilla.org/mozilla-central/rev/a2d8f3384b3c
Status: ASSIGNED → RESOLVED
Closed: 15 years ago → 15 years ago
Resolution: --- → FIXED
Backed out due to test failures in
browser/toolkit/components/passwordmgr/test/browser/browser_passwordmgrdlg.js
and
tests/content/events/test/test_bug299673.html
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch fix v3 — — Splinter Review 
It seems that focusing of toplevel windows (involving their activation) is not
complete until an NS_ACTIVATE event is received in response to a
nsIWidget::SetFocus(), if the toplevel window is not already active.  I think
this makes sense.

It seems a little strange however to generate the NS_ACTIVATE event from
SetFocus before the window has actually become active.  The MS windows code
looks like it only sends activate events in response to system events, but I
guess those are synchronous on that platform.

On X11, the toplevel window becomes active asynchronously.  Tests don't wait
before sending synthetic events, so I'm not going to try to change the
behavior and the tests now.

I'm not sure what we should be aiming for in the future though.  Are there any
fundamental reasons why we shouldn't wait to receive notification of active
status from the window manager before sending the NS_ACTIVE event?

Note that this patch still removes the DispatchActivateEvent call when GTK_WIDGET_HAS_FOCUS(owningWidget) as that can only be true if the toplevel window is already active.
Attachment #386618 - Attachment is obsolete: true
Attachment #387611 - Flags: review?(enndeakin)
(In reply to comment #44)
> I'm not sure what we should be aiming for in the future though.  Are there any
> fundamental reasons why we shouldn't wait to receive notification of active
> status from the window manager before sending the NS_ACTIVE event?

The NS_ACTIVATE/NS_DEACTIVATE events should only be fired when the system tells us to. A number of tests rely on the opposite though, however we should fix these tests. We should fix bug 502128 and fix up anything we need to.
Attachment #387611 - Flags: review?(enndeakin) → review+
http://hg.mozilla.org/mozilla-central/rev/c54bfda91dc1
Status: REOPENED → RESOLVED
Closed: 15 years ago → 15 years ago
Resolution: --- → FIXED
(In reply to comment #44)
> It seems a little strange however to generate the NS_ACTIVATE event from
> SetFocus before the window has actually become active.

Filed bug 506173.
Verified fixed with Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090728 Minefield/3.6a1pre ID:20090728032035

Can this be tested by an automated test?
Status: RESOLVED → VERIFIED
Flags: in-testsuite?
Target Milestone: --- → mozilla1.9.2a1
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: