Closed Bug 601182 Opened 10 years ago Closed 10 years ago

Spurious focus events sent to NPAPI plugins on OS X

Categories

(Core :: Plug-ins, defect)

All
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: christopher.kempke, Assigned: smichaud)

References

Details

Attachments

(6 files, 10 obsolete files)

5.11 KB, patch
Details | Diff | Splinter Review
37.33 KB, text/plain
Details
34.65 KB, text/plain
Details
19.60 KB, patch
Details | Diff | Splinter Review
6.90 KB, patch
jaas
: review+
Details | Diff | Splinter Review
36.54 KB, patch
smaug
: review+
jaas
: review+
enndeakin
: feedback+
Details | Diff | Splinter Review
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
Component: Extension Compatibility → Plug-ins
Product: Firefox → Core
QA Contact: extension.compatibility → plugins
Steven - can you take a look?
Assignee: nobody → smichaud
blocking2.0: --- → ?
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.
blocking2.0: ? → beta8+
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
Attached patch Fix (obsolete) — Splinter Review
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.)
Attachment #482984 - Flags: review?(enndeakin)
Summary: 32 bit Cocoa NPAPI plugin hosted in 64-bit gets events out of order on first mouse down → Spurious focus events sent to NPAPI plugins on OS X
Hardware: x86_64 → All
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?
Attachment #482984 - Attachment is obsolete: true
Attachment #483574 - Flags: review?(enndeakin)
Attachment #482984 - Flags: review?(enndeakin)
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.
Attachment #483574 - Flags: review?(enndeakin)
> 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.
Attachment #483574 - Attachment is obsolete: true
Attachment #484137 - Flags: review?(enndeakin)
Attached patch Mochitest for this bug (obsolete) — Splinter Review
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.
Attachment #484147 - Flags: review?(joshmoz)
> 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.
Attachment #484137 - Attachment is obsolete: true
Attachment #485368 - Flags: review?(joshmoz)
Attachment #484137 - Flags: review?(enndeakin)
(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.
Attachment #484147 - Flags: review?(joshmoz) → review+
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().
Attachment #485368 - Attachment is obsolete: true
Attachment #485368 - Flags: review?(joshmoz)
Comment on attachment 487014 [details] [diff] [review]
Fix rev4 (fix and combine patches)

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

Josh, please review the Cocoa widgets parts of my patch.
Attachment #487014 - Flags: review?(joshmoz)
Attachment #487014 - Attachment description: FIx rev4 (fix and combine patches) → Fix rev4 (fix and combine patches)
Attached patch Fix rev5 (fix stupid mistake) (obsolete) — Splinter Review
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.
Attachment #487014 - Attachment is obsolete: true
Attachment #487014 - Flags: review?(joshmoz)
Attachment #487014 - Flags: review?(enndeakin)
Comment on attachment 487024 [details] [diff] [review]
Fix rev5 (fix stupid mistake)

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

Josh, please review the Cocoa widgets parts of my patch.
Attachment #487024 - Flags: review?(joshmoz)
> 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.
Attachment #487024 - Flags: review?(joshmoz) → review+
> 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.
(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.
Attachment #487024 - Attachment is obsolete: true
Attachment #487024 - Flags: review?(enndeakin)
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.
Attachment #487478 - Flags: review?(joshmoz)
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.
Attachment #487478 - Attachment is obsolete: true
Attachment #487478 - Flags: review?(joshmoz)
(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.
Attachment #488291 - Flags: review?(joshmoz)
Attachment #484147 - Attachment is obsolete: true
Attachment #482993 - Attachment is obsolete: true
Attachment #488288 - Flags: feedback?(enndeakin)
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.
Attachment #488288 - Flags: feedback?(enndeakin) → feedback+
Attachment #488288 - Attachment is obsolete: 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?
Attachment #488485 - Flags: feedback?(enndeakin)
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.
Attachment #488485 - Flags: feedback?(enndeakin) → feedback+
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.)
Attachment #488485 - Flags: review?(Olli.Pettay)
Comment on attachment 488485 [details] [diff] [review]
Fix rev8 (follow Neil's suggestion)

Josh, please review the Cocoa widgets parts of this patch.
Attachment #488485 - Flags: review?(joshmoz)
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.
Attachment #488485 - Flags: review?(joshmoz) → review+
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.
Attachment #488485 - Flags: review?(Olli.Pettay) → review+
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>
Attachment #488291 - Flags: review?(joshmoz) → review+
>>-      // 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().
blocking2.0: beta8+ → betaN+
(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.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
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.
Depends on: 627649
No longer depends on: 627649
Depends on: 627649
You need to log in before you can comment on or make changes to this bug.