User-Agent: Mozilla/4.0 (compatible; MSIE 8.0; Windows NT 6.1; WOW64; Trident/4.0; GTB6.5; SLCC2; .NET CLR 2.0.50727; .NET CLR 3.5.30729; .NET CLR 3.0.30729; Media Center PC 6.0; MS-RTC LM 8; InfoPath.2; OfficeLiveConnector.1.4; OfficeLivePatch.1.3; .NET4.0C; .NET4.0E) Build Identifier: 4.0b7pre In the 32-bit-out-of-process hosting model in 64-bit FF4, the first mouse down in a new or refreshed plugin gets events in this order: -Mouse Down -Control Focus Lost -Control Focus Gained -Mouse Up The two focus events between the mouse-down and mouse-up are causing Silverlight to stop tracking the mouse on the focus lost, causing button click events not to be sent (Silverlight considers a button clicked like the OS does: if the mouse down and up are both within the button). So if the first click in a control is on a button, it just doesn't take, an issue that other browsers and previous FF versions don't manifest. I think there are two issues here: - The Focus Lost message is spurious, as indicated by the fact that it immediately gets a Focus Gained back, and - Even if the focus messages associated with a mousedown are "real," they should be sent BEFORE the mouse down event (per previous behavior) is sent rather than breaking up the down/up pair. Reproducible: Always Steps to Reproduce: 1. Instrument a plugin to echo NPCocoaEvents 2. Click in plugin and note event order. 3. Actual Results: -Mouse Down -Control Focus Lost -Control Focus Gained -Mouse Up Expected Results: Either: -Control Focus Gained -Mouse Down -Mouse Up or: -Mouse Down -Mouse Up

Steven - can you take a look?

If this turns out to be a problem in our code we need to fix it for beta 8 so that plugin vendors can code against the proper behavior ASAP.

Chris, it looks (from your user-agent string in comment #0) that you've only tested on Windows (64-bit Windows), and not on OS X. Is that true?

> Chris, it looks (from your user-agent string in comment #0) that you've only > tested on Windows (64-bit Windows), and not on OS X. Is that true? Never mind. You must have been running Windows when you opened this bug.

Here's what I'm able to reproduce, testing with today's Minefield nightly and my (32-bit) Debug Events Plugin from bug 441880 (which supports the NPAPI Cocoa event model): -Control Focus Gained -Mouse Down -Control Focus Lost -Control Focus Gained -Mouse Up This isn't exactly what you report, but it's definitely buggy behavior. I see the same behavior when the browser runs in 32-bit mode, on both OS X 10.6.4 and 10.5.8. I'll try to find a regression range. Please retry your tests with today's Minefield nightly (ftp://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2010-10-01-03-mozilla-central/firefox-4.0b7pre.en-US.mac64.dmg).

The event sequence I described in comment #5 only happens in Cocoa event mode (when I load "test.html" from the Debug Events Plugin's distro). Here's the event sequence I get in Carbon event mode (when I load "test-carbononly.html"): -Mouse Down -Control Focus Gained This is least closer to being correct.

This bug's regression range is as follows: 2010-04-25-03-mozilla-central 2010-04-26-03-mozilla-central http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2010-04-25+03%3A00%3A00&enddate=2010-04-26+03%3A00%3A00 Or more precisely, the behavior I described in comment #5 started with the 2010-04-26-03-mozilla-central nightly. In this range, it's probably Josh's patch for bug 559758 (http://hg.mozilla.org/mozilla-central/rev/3de70a10af21) that caused the regression.

Turns out Josh's patch for bug 559758 didn't cause this regression -- instead it uncovered a bug in the focus manager (nsFocusManager.cpp). The problem is that nsFocusManager::Focus(), whenever it tries to focus an object, first focuses the "root widget" (if there is one), and only afterwards focuses the object's widget (if it has one). In current code a plugin (an nsObjectFrame object) always has its own widget. So every time a plugin (or more precisely its widget) is focused, it actually gets focused, unfocused, and then refocused. Prior to Josh's patch for bug 559785 on OS X, the browser didn't send the plugin a focus event every time its focus changed. So the focus manager bug stayed hidden. But not informing the plugin of every focus change caused other problems, for which the patch for bug 559785 is needed. Interestingly, the problems the patch for bug 559785 fixed on OS X still exist on Windows and Linux. Which is why this bug hasn't yet been uncovered on those platforms. My patch stops nsFocusManager::Focus() from focusing the root widget when it's been asked to focus a plugin that has its own widget. It shouldn't have any effect when trying to focus anything else. My patch fixes this bug on OS X. It has no effect on Windows or Linux -- it doesn't change how the browser passes focus events to windowless plugins on those platforms. But it will be needed on those platforms when we fix bug 559785 there. (It also, of course, doesn't change the browser's behavior with windowed plugins on Windows and Linux -- since windowed plugins get events on their own, and not from the browser.)

Here are some tryserver builds to test with. The first set just adds logging of focus events sent to windowless plugins (and calls to nsFocusManager::Focus()). The second set adds the same logging plus my fix. (The OS X ones aren't done yet -- I'll post them later.) Logging Only Linux http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/smichaud@pobox.com-862a3eb71533/tryserver-linux/firefox-4.0b8pre.en-US.linux-i686.tar.bz2 Linux64 http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/smichaud@pobox.com-862a3eb71533/tryserver-linux64/firefox-4.0b8pre.en-US.linux-x86_64.tar.bz2 Win32 http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/smichaud@pobox.com-862a3eb71533/tryserver-win32/firefox-4.0b8pre.en-US.win32.zip Fix Plus Logging Linux http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/smichaud@pobox.com-be732dc71e22/tryserver-linux/firefox-4.0b8pre.en-US.linux-i686.tar.bz2 Linux64 http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/smichaud@pobox.com-be732dc71e22/tryserver-linux64/firefox-4.0b8pre.en-US.linux-x86_64.tar.bz2 Win32 http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/smichaud@pobox.com-be732dc71e22/tryserver-win32/firefox-4.0b8pre.en-US.win32.zip Windowless plugins are unusual on Windows and Linux, and so are rather difficult to find. I tested with http://www.communitymx.com/content/source/E5141/wmodetrans.htm (which actually lets you switch between windowless and windowed Flash "movies"). On Windows you won't see any console output unless you run firefox.exe with the "-console" parameter.

Here are steps to reproduce the main problem that was fixed by the patch for bug 559758 on OS X. To see it on OS X you'll need to test with my Debug Events Plugin from bug 441880 and FF 3.6.X, FF 3.5.X, or a trunk nightly older than 2010-04-26-03-mozilla-central. To see the problem on Windows or Linux, test with one of my builds from comment #9 ("logging only" or "fix plus logging") and the windowless Flash "movies" at http://www.communitymx.com/content/source/E5141/wmodetrans.htm. 1) Start FF and load the plugin. 2) Click on the plugin to give it focus. Notice that one "focus gained" event is sent to the plugin (which is correct). 3) Click on the desktop or another open application to make FF lose the application focus. A "focus lost" event is sent to the plugin and it loses focus (which is incorrect). 4) Click on the FF window's titlebar (not on the plugin itself, or anywhere else in the browser window). A "focus gained" event is sent to the plugin and it regains focus. Again this is incorrect: The plugin shouldn't have lost focus in step 3, but clicking on the titlebar shouldn't change the focus of anything in the browser window.

(Following up comment #9) bug 559785 -> bug 559758 :-(

Here are the OS X tryserver builds I promised above. As before, the first just logs focus events sent to plugins. The second contains my fix and also logs focus events. Logging Only http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/smichaud@pobox.com-62590bc4fd81/tryserver-macosx64/firefox-4.0b8pre.en-US.mac64.dmg Fix Plus Logging http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/smichaud@pobox.com-df85c0fee4c9/tryserver-macosx64/firefox-4.0b8pre.en-US.mac64.dmg By the way, my patch had no non-spurious failures in any of the tryserver tests. Chris, please test with the "fix plus logging" build and let us know your results.

Comment on attachment 482984 [details] [diff] [review] Fix >+ nsIWidget* objectFrameWidget = nsnull; >+ if (aContent) { >+ nsIFrame* contentFrame = aContent->GetPrimaryFrame(); >+ nsIObjectFrame* objectFrame = do_QueryFrame(contentFrame); >+ if (objectFrame) >+ objectFrameWidget = objectFrame->GetWidget(); Fix the indenting here. >+ nsIContent *focusedNode = aWindow ? aWindow->GetFocusedNode() : nsnull; aWindow is always non-null here. >+ PRBool dontFocusRootWidget = >+ (objectFrameWidget && (aFocusChanged || (aContent == focusedNode))); I don't think it should matter if aFocusChanged is set or whether aContent == focusedNode. What specific interactions are you thinking of that need to be distinguished here?

> Fix the indenting here. OK > aWindow is always non-null here. OK >>+ PRBool dontFocusRootWidget = >>+ (objectFrameWidget && (aFocusChanged || (aContent == focusedNode))); > > I don't think it should matter if aFocusChanged is set or whether > aContent == focusedNode. What specific interactions are you thinking > of that need to be distinguished here? I was just trying to narrow the focus of my patch as much as possible, to avoid unexpected trouble in code I don't know terribly well :-) But if you think that's not needed, I'm happy to change this to PRBool dontFocusRootWidget = objectFrameWidget ? PR_TRUE : PR_FALSE; or to drop this line and change the following line to if (!objectFrameWidget) { Of course I'd also drop the following line: nsIContent *focusedNode = aWindow ? aWindow->GetFocusedNode() : nsnull;

The "FIX + Log" sample build resolves the issue on our end.

How's this?

Comment on attachment 483574 [details] [diff] [review] Fix rev1 (follow Neil's suggestions) Never mind. While trying (and failing) to write a test for this bug, I discovered that (even with my patch) spurious focus events are still sent when there are multiple plugins on a page, and you switch focus between them. I'll be doing more work on this next week.

> spurious focus events are still sent when there are multiple plugins > on a page, and you switch focus between them. I discovered why this happens: nsFocusManager::Blur() is called from nsFocusManager::SetFocusInner() when there is a currently focused window. But nsFocusManager::Blur() can focus the root widget (thereby unfocusing the currently focused plugin widget). Then the plugin widget gets refocused in a call to nsFocusManager::Focus(). My new patch fixes this by stopping nsFocusManager::Blur() from focusing the root widget (and thereby possibly unfocusing a plugin widget) when the focus is going to stay in the same document (when 'aIsLeavingDocument' is false). New "fix and log" tryserver builds should be available by tomorrow morning.

There's already a test_cocoa_focus.html mochitest that does most of what's needed to test this bug's fix. So rather than write a new test, I've revised test_cocoa_focus.html to also deal with this bug. I had to make a number of generic changes: 1) This bug can only be tested by sending native mouse events, so I changed test_cocoa_focus.html to do that. 2) Native mouse events can't be sent to a plugin that's scrolled out of view, so I had to make the test window large enough (and the plugins small enough) to display both plugins without scrolling. 3) For some reason the onload handler doesn't work when we're sending native mouse events, so I had to substite a call to SimpleTest.waitForFocus(). I also added more calls to "is(plugin1.getFocusEventCount() ...", to ensure that spurious focus events don't get sent to the test plugin.

> My new patch fixes this by stopping nsFocusManager::Blur() from > focusing the root widget (and thereby possibly unfocusing a plugin > widget) when the focus is going to stay in the same document (when > 'aIsLeavingDocument' is false). That doesn't indicate that though. For example, the call to Blur() from ClearFocus is expected to remove focus from the plugin and set it to the document when aIsLeavingDocument is false. Note also that the element which is about to be focused may not be focusable, and focus will never happen leaving the focus on the plugin rather than the document. > -Control Focus Gained > -Mouse Down > -Control Focus Lost > -Control Focus Gained > -Mouse Up Why is the mouse down on a plugin that is already focused attempting to set it again?

>> My new patch fixes this by stopping nsFocusManager::Blur() from >> focusing the root widget (and thereby possibly unfocusing a plugin >> widget) when the focus is going to stay in the same document (when >> 'aIsLeavingDocument' is false). > > That doesn't indicate that though. For example, the call to Blur() > from ClearFocus is expected to remove focus from the plugin and set > it to the document when aIsLeavingDocument is false. > > Note also that the element which is about to be focused may not be > focusable, and focus will never happen leaving the focus on the > plugin rather than the document. I'll see what alternatives I can come up with. Do you have any ideas? >> -Control Focus Gained >> -Mouse Down >> -Control Focus Lost >> -Control Focus Gained >> -Mouse Up > > Why is the mouse down on a plugin that is already focused attempting > to set it again? The short answer is that "it's not". This is the sequence of "events" that's passed to the plugin. Here's the same sequence over again with the addition of actual user events: User presses mouse button Plugin receives "control focus gained" event Plugin receives "mouse down" event Plugin receives "control focus lost" event (from call to nsFocusManager::Focus() or nsFocusManager::Blur()) Plugin receives "control focus gained" event (from call to nsFocusManager::Focus()) User releases mouse button Plugin receives "mouse up" event

> User presses mouse button > Plugin receives "control focus gained" event > Plugin receives "mouse down" event > Plugin receives "control focus lost" event (from call to > nsFocusManager::Focus() or nsFocusManager::Blur()) > Plugin receives "control focus gained" event (from call to > nsFocusManager::Focus()) Do you have stack traces for these two last cases? When the mouse is pressed on an element that is already focused, we shouldn't be changing the focus. Neither nsFocusManager::Blur() nor nsFocusManager::Focus() should be called at all. Or is this a case where the first step of "control focus gained" is focusing the plugin but not telling the focus manager it has done so?

I'll post annotated stack traces of everything in an hour or so.

(In reply to comment #24) > When the mouse is pressed on an element that is already focused, we > shouldn't be changing the focus. Neither nsFocusManager::Blur() nor > nsFocusManager::Focus() should be called at all. > > Or is this a case where the first step of "control focus gained" is > focusing the plugin but not telling the focus manager it has done > so? I suspect the answer is "yes". In step 1 of both traces, the plugin widget sends Gecko an NS_NON_RETARGETED_PLUGIN_EVENT event that wraps an NPCocoaEventFocusChanged event that gets sent to the plugin. But I suspect Gecko doesn't know from this that the plugin is focused.

So what GUI event should the plugin widget be dispatching instead of (or in addition to) the NS_NON_RETARGETED_PLUGIN_EVENT event?

(In reply to comment #29) > So what GUI event should the plugin widget be dispatching instead of > (or in addition to) the NS_NON_RETARGETED_PLUGIN_EVENT event? When does this event get fired? A specific user action? Does it only fire when the window or document is already active or focused?

> When does this event get fired? A specific user action? Does it only fire when > the window or document is already active or focused? It gets fired in step 1 of my traces, when the OS gives the keyboard focus to the plugin's ChildView. This (like everything else) happens after the user clicks on the plugin (on mouse-down).

> Does it only fire when the window or document is already active or focused? In all my tests, the window and document are already focused before the user does anything.

(Following up comment #29) > So what GUI event should the plugin widget be dispatching instead of > (or in addition to) the NS_NON_RETARGETED_PLUGIN_EVENT event? I tried sending an NS_ACTIVATE or NS_DEACTIVATE event in place of and in addition to the NS_NON_RETARGETED_PLUGIN_EVENT event. Neither approach solved the problem. Sending only an NS_ACTIVATE or a NS_DEACTIVATE event (using the [ChildView sendFocusEvent:] method) resulted in the plugin never receiving any focus events at all. (Of course I also added code to the sendFocusEvent: method to set the focus event's pluginEvent field to an appropriate NSCocoaEvent.) Presumably these events were redirected to some other destination. Sending an NS_ACTIVATE or a NS_DEACTIVATE event (without a pluginEvent) in addition to the NS_NON_RETARGETED_PLUGIN_EVENT event made no difference in the first case (clicking on a plugin when the focus was on the same page/document but not in anther plugin), and actually made the second case worse (clicking on a plugin when the focus was in another plugin on the same page/document) -- the plugin got focused, unfocused, focused, unfocused, and then focused. I'm beginning to think there's no reasonable/effective way to tell the focus manager a given plugin widget is focused other than dispatching an NS_MOUSE_BUTTON_DOWN event. It'd be possible for me to work around these problems by suppressing any attempt to send focus events to a plugin (using NS_NON_RETARGETED_PLUGIN_EVENT) while Gecko is processing an NS_MOUSE_BUTTON_DOWN dispatched by the plugin's widget. And this is what I'll do if I have no other choice. But I remain convinced there are bugs in how the focus manager focuses plugin widgets, even if I don't know exactly how to fix them. A Cocoa-widgets-specific workaround would just paper them over.

By the way (in case this wasn't clear), NS_NON_RETARGETED_PLUGIN_EVENT focus events are sent from the [ChildView updateCocoaPluginFocusStatus:] method.

(Following up comment #33) > Sending only an NS_ACTIVATE or a NS_DEACTIVATE event (using the > [ChildView sendFocusEvent:] method) resulted in the plugin never > receiving any focus events at all. (Of course I also added code to > the sendFocusEvent: method to set the focus event's pluginEvent > field to an appropriate NSCocoaEvent.) Presumably these events were > redirected to some other destination. There appear to be only two places in the tree where NS_ACTIVATE or NS_DEACTIVATE events are processed: 1) nsWebShellWindow::HandleEvent(), which calls nsIFocusManager::WindowRaised() on an NS_ACTIVATE event and nsIFocusManger::WindowLowered() on an NS_DEACTIVATE event. 2) nsWebBrowser::HandleEvent(), which calls nsWebBrowser::Activate() on an NS_ACTIVATE event and nsWebBrowser::Deactivate() on an NS_DEACTIVATE event. Neither seems appropriate for trying to focus or unfocus a plugin widget.

I've now discovered the NS_PLUGIN_ACTIVATE event. I'll play with it tomorrow and see if it gets me anywhere.

There isn't a mechanism currently to inform the focus manager that a plugin has received the focus. One would need to be added that would probably be similar to current focusing but would skip the widget changing. That said, the current patch would be fine if it was improved such that it only occurred in this plugin-click situation. We could investigate a better fix later.

> There isn't a mechanism currently to inform the focus manager that a > plugin has received the focus. One would need to be added that would > probably be similar to current focusing but would skip the widget > changing. I came to the same conclusion, and have been working on a Cocoa-widgets-specific patch for this bug (which makes no changes to the focus manager). I've not quite finished it, but am close. > That said, the current patch would be fine if it was improved such > that it only occurred in this plugin-click situation. I'm not sure I understand. You (apparently) have no problems with my changes to nsFocusManager::Focus(). But you do have problems with my changes to nsFocusManger::Blur() ... which I frankly don't know how to address. What would you suggest I do in place of the changes I've made to nsFocusManager::Blur()?

What I meant is to ensure that the change you made to not switch the widget-level focus should only occur in this specific case, where a plugin's widget was already focused and the call to Blur/Focus was due to a mouse click on that same plugin.

Here's the Cocoa-widgets-specific patch I've been working on. It fixes the following two problems, which I mentioned above: 1) Spurious focus events sent when clicking on a plugin if no plugin was previously focused. 2) Spurious focus events sent when clicking on a second plugin if another plugin was previously focused. It also solves the following problem, which would (I think) be impossible to fix in the focus manager: 3) A plugin getting focused (and a focus event being sent to it) on a "failed click-through". On OS X, a left-mouse-click on a non-focused window sometimes just focuses the window (what I call a "failed click-through"), and sometimes focuses both the window the the NSView that was clicked on (what I call a "successful click-through"). Currently a plugin view always gets focused when it's clicked on -- whether the mouse-down event clicks through or not. This is incorrect, and can (in principle) lead to mismatches between what actually has the (Cocoa) keyboard focus and what Gecko thinks is focused. (The spurious focus events that are currently sent tend to paper over this problem.) Recently Markus Stange added code to nsChildView.mm to let Gecko decide whether a click-through should "succeed" or "fail" (see particularly ChildViewMouseTracker::WindowAcceptsEvent()). My patch leverages this code to ensure that clicking on a plugin view only focuses it (gives it the Cocoa keyboard focus) on a "successful" click-through. All three problems are solved by preventing the plugin getting Cocoa keyboard focus unless that should really happen. Doing this also stops spurious focus events being sent to the plugin. As far as I can tell, this patch solves all the problems uncovered by Josh's patch for bug 559758 (including the three mentioned above). And (as I mentioned) problem #3 probably can't be fixed in the focus manager. So I think this is the patch we should go with ... at least until the focus manager interface gets calls that can be used to tell it that some content has been focused externally (by code outside the focus manager). A tryserver build will follow in a few hours.

(In reply to comment #39) > What I meant is to ensure that the change you made to not switch the > widget-level focus should only occur in this specific case, where a > plugin's widget was already focused and the call to Blur/Focus was > due to a mouse click on that same plugin. I actually tried once before to find a way to make my changes to nsFocusManger::Blur() only happen when trying to focus a plugin. But I failed, perhaps because I'm not very familiar with the focus manager code. This is what I was asking your help with, and why I was asking for it. In what remains of today, I'll take another look to see if there's something I missed. But (like I said above), I now think a Cocoa-widgets-specific fix is the way to go.

Here's a tryserver build of my patch from comment #40. http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/smichaud@pobox.com-220441fe1c41/tryserver-macosx64/firefox-4.0b8pre.en-US.mac64.dmg Please try it, Chris, and let us know your results.

This patch is a start on the focus manager changes. It doesn't work so I haven't tested it. I added a new event NS_PLUGIN_FOCUS, but it never reaches the object frame. I'm not sure what the widget or plugin changes that are needed here. Steven, any help here? I might change to using a separate nsFocusManager::FocusPlugin method to just a flag to SetFocus in the future.

I'll start working on your patch tomorrow.

I had our Silverligth test team try out the tryserver build from comment #42; it resolves the issue in our test suite. Testing is continuing, but the priority-zero tests all pass with it, so we're likely good.

Here's a patch that fixes some problems with Neil's patch from comment #43 and combines it with the part of my patch from comment #40 (my rev3 patch) that fixes a problem (problem #3 from comment #40) that can only be fixed in Cocoa-widgets code. > I added a new event NS_PLUGIN_FOCUS, but it never reaches the object > frame. I fixed this by adding the NS_PLUGIN_FOCUS event to those for which NS_IS_ACTIVATION_EVENT returns "true". I also moved the call that sends the NS_PLUGIN_FOCUS event from [ChildView updatePluginTopLevelWindowStatus:] to [ChildView updateCocoaPluginFocusStatus:], where it should be. I found that, with Neil's patch, spurious focus events are still sent under the following circumstances: 1) Run Minefield and load my DebugEventPlugin's distro's test.html. 2) Click on the plugin to give it focus. The plugin receives one "getFocus" event, which is correct. 3) Click on the Desktop (or another browser window). The plugin doesn't receive any focus events, which is correct. 4) Click on the title bar of the window containing the plugin. The plugin receives two focus events -- "loseFocus" and "getFocus". This is incorrect -- the plugin shouldn't receive any focus events. I found I could fix this by setting nsFocusManager::Focus()'s aAdjustWidget parameter to PR_FALSE when called from nsFocusManager::WindowRaised(). I figure that widget focus shouldn't be changed (and shouldn't need to be changed) when a window is raised or lowered. So for good measure I also changed nsFocusManager::Blur()'s aAdjustWidget parameter to PR_FALSE when it's called from nsFocusManager::WindowLowered().

Comment on attachment 487014 [details] [diff] [review] Fix rev4 (fix and combine patches) Neil, please review the non-Cocoa-widgets parts of my patch.

Comment on attachment 487014 [details] [diff] [review] Fix rev4 (fix and combine patches) Josh, please review the Cocoa widgets parts of my patch.

Oops, the previous patch didn't return anything from nsFocusManager::FocusPlugin(). Somehow this only triggered a warning on OS X (which I missed). But it caused my Linux tryserver build to barf.

Comment on attachment 487024 [details] [diff] [review] Fix rev5 (fix stupid mistake) Neil, please review the non-Cocoa-widgets parts of my patch.

Comment on attachment 487024 [details] [diff] [review] Fix rev5 (fix stupid mistake) Josh, please review the Cocoa widgets parts of my patch.

> 3) Click on the Desktop (or another browser window). > > The plugin doesn't receive any focus events, which is correct. The 'loseFocus' you receive at step 4 should be sent at this point. > 4) Click on the title bar of the window containing the plugin. > > The plugin receives two focus events -- "loseFocus" and "getFocus". > This is incorrect -- the plugin shouldn't receive any focus events. It should receive a 'getFocus' event now.

(In reply to comment #52) How is that correct? Why should a plugin (or anything in a browser wind9ow) lose focus when the browser window loses focus, then regain it when the browser window gains focus?

(In reply to comment #53) > (In reply to comment #52) > > How is that correct? > > Why should a plugin (or anything in a browser wind9ow) lose focus when the > browser window loses focus, then regain it when the browser window gains focus? Maybe I'm not understanding what you mean by focus. An element (including a plugin) is focused when pressing a key targets the element. When the browser window is not raised, keys cannot target elements inside it, and, thus, those elements cannot be focused. What is the plugin expecting when it sees the "loseFocus" and "getFocus" notifications you refer to?

"Focus" has different senses, depending on what kind of object you're talking about. Apps, windows and views can all have "focus" (or not). I don't think it makes any sense for a plugin (the equivalent of a view) to lose focus when its window (or its app) loses focus. I'll try to find documentation on this question (next week, as I've run out of time today). But I'm curious what Chris thinks about this.

> I found I could fix this by setting nsFocusManager::Focus()'s > aAdjustWidget parameter to PR_FALSE when called from > nsFocusManager::WindowRaised(). > > I figure that widget focus shouldn't be changed (and shouldn't need to > be changed) when a window is raised or lowered. So for good measure I > also changed nsFocusManager::Blur()'s aAdjustWidget parameter to > PR_FALSE when it's called from nsFocusManager::WindowLowered(). I'm not convinced that either change is correct. The main reason for shifting the widget focus on WindowRaised is to ensure that the right child widget is always focused. Although, maybe now that fewer child widgets are created, this doesn't matter and there shouldn't be a problem. I'm not sure that we have a lot or any plugin related focus tests. Did you test this patch on the try server yet? What happens when: - the window is lowered - a call is made to element.focus() from the plugin to another element, and/or vice versa - the window is raised (via mouse and also via a script) > I don't think it makes any sense for a plugin (the equivalent of a > view) to lose focus when its window (or its app) loses focus. How does the plugin receive this indication that the window has been lowered? (This is needed to remove focus indicators and hide the caret) Note that I can't review this patch as I wrote it.

(In reply to comment #56) > Did you test this patch on the try server yet? Yes. I tested and built on OS X (32-bit and 64-bit), Linux (32-bit and 64-bit) and Windows. There were no non-spurious test failures. > What happens when: > - the window is lowered > - a call is made to element.focus() from the plugin to another > element, and/or vice versa > - the window is raised (via mouse and also via a script) A conventional (non-XPCOM) plugin won't be able to call element.focus(). But I could write a mochitest that focuses a plugin in the second step, and see what happens. With my patch, plugin focus (i.e. widget focus) isn't changed when a window is raised or lowered. So I have every reason to believe that the outcome of step 2 wouldn't be changed by step 3, or by the absence of step 1. >> I don't think it makes any sense for a plugin (the equivalent of a >> view) to lose focus when its window (or its app) loses focus. > > How does the plugin receive this indication that the window has been > lowered? (This is needed to remove focus indicators and hide the > caret) I'm skeptical that plugin writers expect to be able to change their plugin's appearance based on whether its window is focused or not (as opposed to whether the plugin view is focused or not). But it's true that NPAPI currently doesn't support a "window focus" (or an "app focus") event. So the only way to indicate a change in window focus is to send a generic focus event to the plugin (even if its normal use is to indicate a change in the plugin view's focus). I still need to dig up more information on the question of whether the browser should send focus events to a plugin when its window loses or gains focus. That'll probably take me the rest of today and at least part of tomorrow. But so far (using my Debug Events Plugin from bug 441880) I've found that neither Chrome nor Safari sends NPAPI plugin focus events when a window loses or gains focus -- in either Cocoa or Carbon event mode. Opera (which only supports Carbon event mode) does send them, though. So far I've only tested on OS X. (Minefield without my patch has the same buggy behavior described in comment #46 steps 1 through 4, though only in Cocoa event mode. In Carbon event mode it sends an NPAPI lose focus event when a focused plugin's window loses focus, and sends it another gain focus event when the plugin's window regains focus.)

Another thing to consider is what happens to a focused plugin when you change tabs. Here everybody seems to agree that it's correct to send the plugin a lose focus event when you switch to another tab, and send a gain focus event when you switch back. Minefield/Firefox, Safari, Chrome and Opera all behave this way, in both Cocoa event mode (if supported) and Carbon event mode. My patch from comment #49 doesn't change this.

Here's the Mac tryserver build made with my patch from comment #49: http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/smichaud@pobox.com-a9a8fc62fa31/tryserver-macosx64/firefox-4.0b8pre.en-US.mac64.dmg

(In reply to comment #57) > I'm skeptical that plugin writers expect to be able to change their > plugin's appearance based on whether its window is focused or not (as > opposed to whether the plugin view is focused or not). > > But it's true that NPAPI currently doesn't support a "window focus" > (or an "app focus") event. So the only way to indicate a change in > window focus is to send a generic focus event to the plugin (even if > its normal use is to indicate a change in the plugin view's focus). This isn't true. Cocoa NPAPI has "NPCocoaEventWindowFocusChanged", and we support it.

(In reply to comment #) > This isn't true. Cocoa NPAPI has "NPCocoaEventWindowFocusChanged", > and we support it. Oops, you're right. All the more reason not to send a NPCocoaEventFocusChanged event when a window is raised or lowered. I'll add this event to my Debug Events Plugin (which currently doesn't support it), and do some more testing.

Anyone know of a plugin (or plugins) I can use to test focus events on Windows and Linux? Or a Flash "movie" I could use?

(Following up comment #57) > I still need to dig up more information on the question of whether > the browser should send focus events to a plugin when its window > loses or gains focus. I haven't found much. But the little I've found strongly indicates that our Cocoa NPAPI event implementation shouldn't send plugin-focus events to plugins when window focus changes. However we've long done this in Carbon event mode on the Mac (I tested back to FF 2.0.0.20). And we currently do it on Windows and Linux with windowless plugins (as you can see testing from my "logging only" builds and the http://www.communitymx.com/content/source/E5141/wmodetrans.htm link from comment #9). So, despite the fact that I see little justification for this behavior, we risk causing trouble if we change long-established patterns. So I think we should squelch these events not in the focus manager (as my rev5 patch did), but in Cocoa widgets code. The only helpful piece of documentation I found is the following, from https://wiki.mozilla.org/NPAPI:CocoaEventModel#Focus_events: * NPCocoaEventFocusChanged - Fired when the plug-in gains or loses focus. * NPCocoaEventWindowFocusChanged - Fired when the plug-in window gains or loses focus. This clearly distinguishes between window focus and plugin focus. And since we have (and support) both kinds of focus event, there's no reason to send NPCocoaEventFocusChanged when a window is "raised" or "lowered". Both events are also used the same way in Safari and Google Chrome -- which as far as I know are the only other browsers that currently support the Cocoa event model.

Comment on attachment 487478 [details] [diff] [review] Fix rev6 (squelch spurious focus events in Cocoa widgets code) Josh, please review the Cocoa widgets parts of this patch.

Comment on attachment 487478 [details] [diff] [review] Fix rev6 (squelch spurious focus events in Cocoa widgets code) Neil: Since you can't review your own work, please suggest someone to review the non-Cocoa-widgets parts of my patch. Also let us know if you're OK with the (small) remaining changes I've made to it.

I've just posted a new update of my Debug Events Plugin at bug 441880, which adds support for the NPCocoaEventWindowFocusChanged event.

Here's the Mac tryserver build made with my patch from comment #63: http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/smichaud@pobox.com-d56744be8596/tryserver-macosx64/firefox-4.0b8pre.en-US.mac64.dmg Tryserver tests were run for OS X (32-bit and 64-bit), Linux (32-bit and 64-bit) and Windows. There were no non-spurious failures.

Steven - just to make sure I understand, your latest patch has NPCocoaEventWindowFocusChanged and NPCocoaEventFocusChanged behavior matching Safari and Chrome?

(In reply to comment #68) Yes.

(In reply to comment #57) > A conventional (non-XPCOM) plugin won't be able to call > element.focus(). I meant that a script on the page changed the focus. > With my patch, plugin focus (i.e. widget focus) isn't changed when a > window is raised or lowered. So I have every reason to believe that > the outcome of step 2 wouldn't be changed by step 3, or by the absence > of step 1. The point I'm making here is that a different widget is expected to be focused from the point when the window is raised from as to when it was lowered. If these were two ordinary child widgets (when each child frame had a separate widget), then, the following would occur with your patch: - frame 1 is focused and thus, child widget 1 is focused - the window is lowered - a script updates the internal focus in that lowered window to frame 2. - the window is raised If widget focus isn't changed upon raise, then I'm not clear where the widget focus would be at this point. (It should be on the child widget for frame 2) I'd expect the same issue with plugins. >Neil: Since you can't review your own work, please suggest someone to > review the non-Cocoa-widgets parts of my patch. Olli or Mats Palmgren can review this.

(In reply to comment #70) > - frame 1 is focused and thus, child widget 1 is focused > - the window is lowered > - a script updates the internal focus in that lowered window to frame 2. > - the window is raised > > If widget focus isn't changed upon raise, then I'm not clear where > the widget focus would be at this point. (It should be on the child > widget for frame 2) I don't understand what you're saying. Is "the child widget for frame 2" explicitly focused in step 3? If so it should still have the focus after step 4. If not it shouldn't have the focus after step 4. In either case my patches (rev5 and rev6) will do the right thing.

(In reply to comment #71) The focus as stored in the focus manager and the dom window changes. The widget focus or the system focus does not change as the window is not active. For instance: - the url bar is focused - the window is lowered - a script calls element.focus() on 'element' which is in a child frame, or a plugin. - the window is raised If widget focus is not adjusted on raise, how does you the OS or we know to focus 'element' or the plugin?

Just to make sure that this doesn't get lost in the shuffle: fundamentally, our problem is focus events that come between corresponding mouse-down and mouse-up events. In the particular case we sent, these events appear to be spurious, hence the bug title, but even in the case where a real focus change is caused by a mouse down, the other browsers (and you, in the Carbon case) send the focus change FOLLOWED BY the mousedown that generated it (rather than the opposite), so that the mouseown is sent to a focussed widget. We (and probably others) depend on that order, because we stop tracking considering mouse-up/mouse-down pairs to be correlated if focus changes between then. The tryserver builds I've looked at seem to fix this, but I want to make sure that the detail doesn't get lost in the shuffle.

Oh, and assuming I'm the "Chris" referenced in Comment 55: I don't feel strongly about this distinction (between plugin focus vs. window focus) -- from the standpoint of our (Silverlight) plugin code, the behavior is the same at the moment, but we'll adjust as necessary. It sort of seems like maybe we don't care about window focus at all in the in-browser case, since I presume the browser won't deliver us key events, etc. if we're in an unfocussed window.

(In reply to comment #72) Now I understand (I think). Sigh. I'll spend the next few hours trying to think of some way around this problem. But the bottom line is that plugin focus must not routinely be changed in Cocoa event mode whenever its window is raised or lowered (or if it is, the plugin must not be informed of it). The only time plugin-focus events should be sent to plugins on window-focus changes (in Cocoa event mode) is in special cases like what you describe in comment #72.

(In reply to comment #73) > but even in the case where a real focus change is caused by a mouse > down, the other browsers (and you, in the Carbon case) send the > focus change FOLLOWED BY the mousedown that generated it (rather > than the opposite), so that the mouseown is sent to a focussed > widget. We (and probably others) depend on that order, because we > stop tracking considering mouse-up/mouse-down pairs to be correlated > if focus changes between then. The tryserver builds I've looked at > seem to fix this, but I want to make sure that the detail doesn't > get lost in the shuffle. Actually, all my tryserver builds still send the getFocus event before the mouse-down event (on both OS X 10.5.8 and 10.6.4, in both 32-bit and 64-bit mode). Make sure you (and the people you work with) also test with my Debug Events Plugin from bug 441880. I can't imagine we send events in a different order to different plugins. Do you think that's possible, Josh? (In reply to comment #74) Thanks for your response. Good thing you don't have to worry about spurious plugin focus events when a window is raised or lowered :-) But I have to think about all plugins (at least those that use the Cocoa event model), about what other browsers do, and about what the spec says.

(Following up comment #76) > Actually, all my tryserver builds still send the getFocus event > before the mouse-down event (on both OS X 10.5.8 and 10.6.4, in both > 32-bit and 64-bit mode). Make sure you (and the people you work > with) also test with my Debug Events Plugin from bug 441880. Oops, I misunderstood your comment. Sigh again. What I should have said is that we've *always* sent the first (non-spurious) focus event before the mouse-down event. But I'll keep in mind that you (and others) rely on this order, and don't want us to change it.

I now think some variant on my rev5 patch is likely to be the best fix. But I need to do more testing. I'll (probably) post a new patch sometime tomorrow.

I think the focus manager should still continue to update the widget focus when the window is raised. But the widget layer should handle whether an event and what type of event should be sent to the plugin itself. If that event is different for the case when the window is raised and when it isn't then the widget level should handle this. The nsIWidget::SetFocus method could be passed an additional argument indicating this if this makes it easier. What could be changed in the focus manager in nsFocusManager::Focus however, is that a call to nsIWidget::SetFocus is made twice. Once to focus the document's widget and then again to focus the plugin. Is this causing extra events to fire at the plugin? On window lower, nsFocusManager::Blur already doesn't update the widget state without any changes needed, so no extra changes should be needed here (apart from those already in the patch)

I've squared the circle, and come up with a patch that fixes all known problems, including the one mentioned by Neil in comment #72. It actually wasn't that difficult -- instead of starting from my rev5 changes to the focus manager, I went back to my first two patches. > What could be changed in the focus manager in nsFocusManager::Focus > however, is that a call to nsIWidget::SetFocus is made twice. Once > to focus the document's widget and then again to focus the > plugin. Is this causing extra events to fire at the plugin? Yes. This is the problem I addressed in my first two patches. Neil, please let me know if you have any problems with the focus manager part of this patch. I won't ask you to review, since the focus manager changes are still mostly from your patch of comment #43.

I tested Neil's testcase from comment #72 by writing a mochitest. Here's a revision to my patch from comment #21 that adds those tests to cocoa_focus.html.

Comment on attachment 488288 [details] [diff] [review] Fix rev7 (address problem of plugin focus changing while its window is inactive) This looks good. There's an edge case that might need to be dealt with, if the call to CheckIfFocusable returns false in-between the two widget->SetFocus() calling blocks in nsFocusManager:Focus(). This might happen if the plugin node was removed during the window focus event. You might want to add a call at the beginning of the else block (around http://mxr.mozilla.org/mozilla-central/source/dom/base/nsFocusManager.cpp#1704) to put the focus on the window widget if objectFrameWidget is set and mFocusedWindow == aWindow && mFocusedContent == nsnull is true.

Comment on attachment 488485 [details] [diff] [review] Fix rev8 (follow Neil's suggestion) (In reply to comment #82) > There's an edge case that might need to be dealt with, if the call > to CheckIfFocusable returns false in-between the two > widget->SetFocus() calling blocks in nsFocusManager:Focus(). This > might happen if the plugin node was removed during the window focus > event. > > You might want to add a call at the beginning of the else block > (around > http://mxr.mozilla.org/mozilla-central/source/dom/bas/nsFocusManager.cpp#1704) > to put the focus on the window widget if objectFrameWidget is set > and mFocusedWindow == aWindow && mFocusedContent == nsnull is true. How's this?

Comment on attachment 488485 [details] [diff] [review] Fix rev8 (follow Neil's suggestion) That's ok. We might want to refactor the duplicate calls in a separate bug.

Comment on attachment 488485 [details] [diff] [review] Fix rev8 (follow Neil's suggestion) Olli, please review the non-Cocoa-widgets parts of this patch. They're actually mostly written by Neil (from comment #43 above), or I'd have asked him for a review. (I did ask him for feedback on my changes/additions to his code.)

Comment on attachment 488485 [details] [diff] [review] Fix rev8 (follow Neil's suggestion) Josh, please review the Cocoa widgets parts of this patch.

Comment on attachment 488485 [details] [diff] [review] Fix rev8 (follow Neil's suggestion) >+ if (!mClickThroughMouseDownEvent || >+ ([mClickThroughMouseDownEvent type] != NSLeftMouseDown)) No need for parens around the "!=" part of this and the indentation is incorrect with the parens (the two "(" should not be vertically aligned). >+ if (IsChildInFailingLeftClickThrough(aView)) >+ return PR_TRUE; >+ } There is a tab in the indentation here.

Comment on attachment 488485 [details] [diff] [review] Fix rev8 (follow Neil's suggestion) (In reply to comment #88) I'll fix these both on landing.

Comment on attachment 488485 [details] [diff] [review] Fix rev8 (follow Neil's suggestion) >@@ -1661,28 +1682,23 @@ nsFocusManager::Focus(nsPIDOMWindow* aWi > > // inform the EventStateManager so that content state change notifications > // are made. > nsPresContext* presContext = presShell->GetPresContext(); > presContext->EventStateManager()-> > SetContentState(aContent, NS_EVENT_STATE_FOCUS); > > // if this is an object/plug-in, focus the plugin's widget. Note that we might > // no longer be in the same document, due to the events we fired above when > // aIsNewDocument. >- if (presShell->GetDocument() == aContent->GetDocument()) { >- nsIFrame* contentFrame = aContent->GetPrimaryFrame(); >- nsIObjectFrame* objectFrame = do_QueryFrame(contentFrame); >- if (objectFrame) { >- nsIWidget* widget = objectFrame->GetWidget(); >- if (widget) >- widget->SetFocus(PR_FALSE); >- } >+ if (aAdjustWidgets && presShell->GetDocument() == aContent->GetDocument()) { >+ if (objectFrameWidget) >+ objectFrameWidget->SetFocus(PR_FALSE); What guarantees that objectFrameWidget isn't a dead object here? Make objectFrameWidget nsCOMPtr<nIWidget>. >-[scriptable, uuid(5cb91200-53f6-4d35-989d-1d28ad80a0d4)] >+[scriptable, uuid(94a49833-0f6d-4d9b-b859-990546ad9b8d)] So this should go to b7? Please ask the drivers what to do with this.

Comment on attachment 488485 [details] [diff] [review] Fix rev8 (follow Neil's suggestion) > Make objectFrameWidget nsCOMPtr<nIWidget> I'll fix this on landing. >>-[scriptable, uuid(5cb91200-53f6-4d35-989d-1d28ad80a0d4)] >>+[scriptable, uuid(94a49833-0f6d-4d9b-b859-990546ad9b8d)] > So this should go to b7? Please ask the drivers what to do with > this. I've sent the following email to drivers@mozilla.org: Bug 601182 has a patch ready to land, and is targeted at FF4 beta8 (it's marked blocking2.0-beta8). But it adds a method to a public interface (nsIFocusManager), so I'm wondering if you'll allow it to land in beta8 (after the feature freeze). I hope very much that you do. Bug 601182 fixes significant problems with the Cocoa event model on OS X. So a fix for it needs to get into the FF4 release, and should be made available to plugin vendors as soon as possible. So the bug is certainly important enough. My patch could conceivable land in beta7 (which hasn't quite been released yet). But I'd be more comfortable having it land in beta8, after having baked on the trunk for a while. My patch's change to the nsIFocusManager interface is small and narrowly targeted: It adds one non-scriptable method, and doesn't change any of the other methods or properties. I suppose a private nsPIFocusManager interface could be added and this new method added there -- but that seems like overkill.

Comment on attachment 488291 [details] [diff] [review] Mochitest for this bug, rev1 (add tests of changing plugin focus while window is blurred) >- // Give the plugin focus. >- plugin1.focus(); >+ // Give the plugin focus (the window is already focused). >+ utils.sendNativeMouseEvent(plugin1X, plugin1Y, NSLeftMouseDown, 0, plugin1); >+ utils.sendNativeMouseEvent(plugin1X, plugin1Y, NSLeftMouseUp, 0, plugin1); This seems indirect, why not just call 'focus()' here? > expectedEventCount++; > > is(plugin1.getFocusState(), true, "Plugin should have focus."); > is(plugin1.getFocusEventCount(), expectedEventCount, "Focus event count should be " + expectedEventCount); > >- // Make sure window activation state changes don't affect plugin focus. >- // Past this point we can't really count events because of how Gecko's focus works. >- // Only the end result matters anyway. >+ // Make sure window activation state changes don't spontaneously >+ // change plugin focus. > > // Blur the window. >- window.focus(); // start in an active state > window.blur(); > >- is(plugin1.getFocusState(), true, "Plugin should have focus."); >+ is(plugin1.getFocusState(), true, "Plugin should still have focus."); >+ is(plugin1.getFocusEventCount(), expectedEventCount, "Focus event count should be " + expectedEventCount); > > // Focus the window. > window.focus(); > >- is(plugin1.getFocusState(), true, "Plugin should have focus."); >+ is(plugin1.getFocusState(), true, "Plugin should still have focus."); >+ is(plugin1.getFocusEventCount(), expectedEventCount, "Focus event count should be " + expectedEventCount); > > // Take focus from the plugin. >+ utils.sendNativeMouseEvent(plugin2X, plugin2Y, NSLeftMouseDown, 0, plugin2); >+ utils.sendNativeMouseEvent(plugin2X, plugin2Y, NSLeftMouseUp, 0, plugin2); >+ expectedEventCount++; >+ >+ is(plugin1.getFocusState(), false, "Plugin should not have focus."); >+ is(plugin1.getFocusEventCount(), expectedEventCount, "Focus event count should be " + expectedEventCount); >+ >+ // Make sure window activation causes the plugin to be informed of focus >+ // changes that took place while the window was inactive. >+ >+ // Give the plugin focus (the window is already focused). >+ utils.sendNativeMouseEvent(plugin1X, plugin1Y, NSLeftMouseDown, 0, plugin1); >+ utils.sendNativeMouseEvent(plugin1X, plugin1Y, NSLeftMouseUp, 0, plugin1); >+ expectedEventCount++; >+ >+ // Blur the window. >+ window.blur(); >+ >+ // Take focus from the plugin while the window is blurred. > plugin2.focus(); > >+ is(plugin1.getFocusState(), true, "Plugin should still have focus."); >+ is(plugin1.getFocusEventCount(), expectedEventCount, "Focus event count should be " + expectedEventCount); >+ >+ // Focus the window. >+ window.focus(); >+ expectedEventCount++; >+ > is(plugin1.getFocusState(), false, "Plugin should not have focus."); >+ is(plugin1.getFocusEventCount(), expectedEventCount, "Focus event count should be " + expectedEventCount); > > window.opener.testsFinished(); > } >+ >+ // Onload hander doesn't work for these tests -- no events arrive at the plugin. >+ window.opener.SimpleTest.waitForFocus(runTests, window); >+ > </script> > </body> > </html> >diff --git a/modules/plugin/test/mochitest/test_cocoa_focus.html b/modules/plugin/test/mochitest/test_cocoa_focus.html >--- a/modules/plugin/test/mochitest/test_cocoa_focus.html >+++ b/modules/plugin/test/mochitest/test_cocoa_focus.html >@@ -6,21 +6,21 @@ > </head> > > <body onload="runTests()"> > <script class="testbody" type="application/javascript"> > SimpleTest.waitForExplicitFinish(); > > var gOtherWindow; > > function runTests() { > // We have to have two top-level windows in play in order to run these tests. >- gOtherWindow = window.open("cocoa_focus.html", "", "width=200,height=200"); >+ gOtherWindow = window.open("cocoa_focus.html", "", "width=250,height=250"); > } > > function testsFinished() { > // Tests have finished running, close the new window and end tests. > gOtherWindow.close(); > SimpleTest.finish(); > } > </script> > </body> > </html>

>>- // Give the plugin focus. >>- plugin1.focus(); >>+ // Give the plugin focus (the window is already focused). >>+ utils.sendNativeMouseEvent(plugin1X, plugin1Y, NSLeftMouseDown, >>+ 0, plugin1); >>+ utils.sendNativeMouseEvent(plugin1X, plugin1Y, NSLeftMouseUp, >>+ 0, plugin1); > > This seems indirect, why not just call 'focus()' here? Because some of the bad behavior my patch fixes only happens with actual, native mouse events. >>+ // Blur the window. >>+ window.blur(); >>+ >>+ // Take focus from the plugin while the window is blurred. >> plugin2.focus(); I used focus() here to simulate an event handler/listener using a script to focus a plugin while its window is unfocused. So here you can't use native events.

(In reply to comment #93) > > This seems indirect, why not just call 'focus()' here? > > Because some of the bad behavior my patch fixes only happens with > actual, native mouse events. Sounds like a bug, file it? Seems like this could come up in actual web content.

>>> This seems indirect, why not just call 'focus()' here? >> >> Because some of the bad behavior my patch fixes only happens with >> actual, native mouse events. > > Sounds like a bug Not really. There are lots of cases where Mochitests don't work properly because they don't simulate native events. That's why we have nsIDOMWindowUtils.sendNativeMouseEvent() and nsIDOMWindowUtils.sendNativeKeyEvent().

(Following up comment #91) Josh pointed out I should have sent this to release-drivers@mozilla.org. I just forwarded it to that address. Sigh. > Bug 601182 has a patch ready to land, and is targeted at FF4 beta8 > (it's marked blocking2.0-beta8). But it adds a method to a public > interface (nsIFocusManager), so I'm wondering if you'll allow it to > land in beta8 (after the feature freeze). > > I hope very much that you do. > > Bug 601182 fixes significant problems with the Cocoa event model on > OS X. So a fix for it needs to get into the FF4 release, and should > be made available to plugin vendors as soon as possible. So the bug > is certainly important enough. > > My patch could conceivable land in beta7 (which hasn't quite been > released yet). But I'd be more comfortable having it land in beta8, > after having baked on the trunk for a while. > > My patch's change to the nsIFocusManager interface is small and > narrowly targeted: It adds one non-scriptable method, and doesn't > change any of the other methods or properties. I suppose a private > nsPIFocusManager interface could be added and this new method added > there -- but that seems like overkill.

Comment on attachment 488485 [details] [diff] [review] Fix rev8 (follow Neil's suggestion) Landed on trunk: http://hg.mozilla.org/mozilla-central/rev/1950375b2ec2 Just after I resent my email to release-drivers@mozilla.org (as per comment #96), bsmedberg responded that I should use nsIFocusManager_MOZILLA_2_0_BRANCH for the new focusPlugin() method. I've now done that. I also followed Josh's suggestions from comment #88 and Olli's from comment #90. This patch will first appear in tomorrow's trunk (mozilla-central) nightly. Please test it, Chris, when it becomes available.

Comment on attachment 488291 [details] [diff] [review] Mochitest for this bug, rev1 (add tests of changing plugin focus while window is blurred) Landed on trunk: http://hg.mozilla.org/mozilla-central/rev/ac5a1cd933e3 Let's hope this does something for bug 595062.