355071 - Flash stops keyboard input in other FF windows (TSM doc problem)

Status: RESOLVED FIXED

(Firefox :: Shell Integration, defect)

Description

[Jake Olefsky]

If some flash content has focus and you click on another firefox window, you will not be able to type into that window.  The only way to get the keyboard to type into the other window is to first put firefox in the background (click on the desktop or other application) and then click on the window you want to bring to the foreground.

To reproduce:
1. Open two windows.
2. In one window open a page that has a form.
3. In the other window, open anything that has flash content. For example http://video.google.com/
4. Click on the flash content to give it focus.
5. Click on your other window and try to type into the form input, the location bar, or the search field.  You will not be able to type anything.
6. Click on the desktop and then click on the window again. You will now be able to type.

This problem does not happen if you are switching tabs. This problem does not happen for java applets.  It must be flash and it must be switching windows.

Comment 1

[Jake Olefsky]

I should have mentioned that I am using 2.0RC1

Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.1) Gecko/20060918 Firefox/2.0

Comment 2

[Steven Michaud [:smichaud] (Retired)]

This bug may be a dup of bug 345010.

Comment 3

[Steven Michaud [:smichaud] (Retired)]

I was able to confirm this with FF 2.0 RC1 on Mac OS X 10.3.9, using
either one of the videos at http://video.google.com/ or the Flash
object at http://www.rogerdean.com/ (at the top of the page).

Comment 4

[Jake Olefsky]

*** Bug 356365 has been marked as a duplicate of this bug. ***

Comment 5

[Allan]

This bug really needs attention before 2.0 ships. Perhaps if it didn't have severity: normal, Jake?

Comment 6

[Steven Michaud [:smichaud] (Retired)]

Quite frankly, I doubt that it's possible to resolve this bug before
2.0 ships -- since it's likely that a fix for bug 318139 will be
required, and will involve changes both to Mozilla.org browsers and to
the Flash plugin.

But it's <a href="mailto:mark&#64;moxienet.com">Mark Mentovai</a> and
<a href="mailto:msintov&#64;macromedia.com">Michelle Sintov</a> who
know most about this.

Comment 7

[Marcia Knous [:marcia]]

I have seen reports of people hitting a variant of this bug in reporter and have hit it myself. It is particularly frustrating for users because when they think they can't login, they abandon the browser for another browser.

The site that I have seen it on recently is http://www.sirius.com/. If I click on "Listen Online" and then try to fill in my login information, I can't type anything in the space. Sometimes after some period of time passes, I am able to fill in the form.

Comment 8

[Steven Michaud [:smichaud] (Retired)]

I just tried http://www.sirius.com/ and was unable to enter any text
in the window popped up by clicking "Listen Online" -- until I clicked
on the desktop and again on that window.  So yes, Marcia, it does seem
like you've seen this bug.

I'm curious what Mark has to say, but I do think this will take a
while to fix (since it will likely require changes to the Flash plugin
as well as to the Mozilla.org browsers).

Comment 9

[Jake Olefsky]

I dont know the details of how the code works, but this problem only appears on 2.0. When I switch to 1.5, I never encounter this problem.  So, I have a hard time believing that this is a problem with Flash or that it is difficult to fix.  Something changed between 1.5 and 2.0 that caused this to happen.

Comment 10

[Steven Michaud [:smichaud] (Retired)]

If this is a dup of bug 345010 (which it very likely is), the fact
that keyboard input is only blocked FF 2.0 (and not in FF 1.5) is due
to the fact that Carbon-based browsers on the 1.8 and 1.8.1 branches
(but not the 1.8.0 branch) have been getting their their input via TSM
(from the kUnicodeNotFromInputMethod Apple event), since the fix for
bug 337199 was landed.  (I believe this fix was also landed on the
trunk.)

The bad interaction between the Flash plugin and Mozilla.org's
Carbon-based browsers (involving those programs' conflicting use of
the "TSM Document") still happens on other branches ... it just
doesn't (usually) result in keyboard input being blocked.  The
symptoms can include crashes (as described at bug 318139).

Note: I understand the problem(s) pretty well, but I can't fix them.
On the Mozilla.org side, I suspect only Mark Mentovai could do so.
(Though, as I've mentioned a couple of times above, even he might not
be able to fix it/them without same changes to the Flash plugin.)

Mark? :-)

Comment 11

[Steven Michaud [:smichaud] (Retired)]

I finally had a chance to test the effect of this bug's URLs on the
current "TSM document" -- and they didn't have any (bad) effect.  So I
was wrong -- this bug _isn't_ a dup of bug 345010, or (presumably)
related in any way to bug 318139.

I can't begin to say what _is_ the cause of this bug.  I've only been
able to reproduce it with Flash plugins, so I don't think it's a
straightforward keyboard focus issue.  (I also tried the QuickTime,
PDF and Shockwave plugins.)

Comment 12

[Marcia Knous [:marcia]]

nominating to get this on the radar. A bad user experience on the Mac end, especially in light of the fact it isn't present on 1.5.0.x. Even if clicking outside of the browser window allows you to enter form information, users are not going to know to do that.

Also able to reproduce this on www.zoomzoomlive.com. Click on results/photos and try to login to see what happens.

Comment 13

[Marcia Knous [:marcia]]

changing flags to indicate I want to block for the next release. sorry for the bugspam.

Comment 14

[(not reading, please use [e-mail] instead)]

Marcia showed me the problem on her mac (I'm not sure the browser/os/flash versions she had), but for For me, results/photos on https://www.zoomzoomlive.com/index.htm works with:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1) Gecko/20061023 BonEcho/2.0

Shockwave Flash

    File name: Flash Player.plugin
    Shockwave Flash 9.0 r20 

Mac OS X 10.4.8 Mac Intel

Comment 15

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #14)

I see the problem every time, with exactly the same software as you
(except that I'm using FF 2.0 RC3), following the procedure in comment
#0 and using http://www.sirius.com/ (as per comment #8).

Comment 16

[Steven Michaud [:smichaud] (Retired)]

(Following up comment #11)

> I finally had a chance to test the effect of this bug's URLs on the
> current "TSM document" -- and they didn't have any (bad) effect.

Wrong again :-(

This bug _is_ caused by problems with the "TSM document" -- just not
exactly the same ones reported at bug 345010 and bug 318139.  And it
_is_ related to those two bugs, though it isn't a dup.

As described at bug 318139 comment #0 and bug 345010 comment #0 (first
two paragraphs), the Flash plugin saves and restores the TSM document
for the browser window that it's running in.  But sometimes it
restores the wrong one.  That's what's happening here.  But unlike the
situations reported at those other two bugs, in this case the "wrong
TSM document" is still valid -- so you don't see crashes (as per bug
318139), and the browser's current TSM document doesn't get NULLed (as
per bug 345010).

Here's what happens when you follow the steps in comment #0.  I'll
call the "one window" of step 2 "window A", and the "other window" of
step 3 "window B".  And let's say window A's TSM document is
0xaaaaaaaa, while window B's is 0xbbbbbbbb.  For keyboard input to
work properly in Mozilla.org Carbon-based browsers with the fix for
bug 337199, the "current TSM document" needs to be set to a given
browser window's TSM document while the keyboard focus is in one of
that window's text input fields.

When, in step 4, you click on the flash content to give it focus, the
Flash plugin saves the then current TSM document (0xbbbbbbbb, window
B's TSM document) and makes its own TSM document current (call it
0x12345678).  But when in step 5 you click on window A, the Flash
plugin "restores" window B's TSM document (makes it the current TSM
document) -- i.e. it restores it incorrectly.  Keyboard input won't
work in window A unless it's own TSM document is the current one.
Clicking on the desktop and again on window A (as per step 6) makes
window A's TSM document current -- so keyboard input starts working
again in window A.

(By the way, this problem can't be fixed by simply backing out the fix
for bug 337199 -- Chinese, Japanese and Korean input would still be
effected by this bug.)

Comment 17

[(not reading, please use [e-mail] instead)]

steven, my mistake.

if I follow the steps in comment #0, I can reproduce this bug with the latest FX2 using https://www.zoomzoomlive.com/index.htm 

my mistake was I didn't realize this bug was about one browser window (with flash) preventing you from typing into a text field on another browser window.

I thought this bug was about not being able to type into the "zoomzoomlive" flash app, which is what marcia was seeing.

marcia, are you able to reproduce this bug?  is your zoomzoomlive bug something else?

Comment 18

[(not reading, please use [e-mail] instead)]

removing ispike's dupme, as I don't think is a dup (please correct me if I'm wrong).  adding [Fx 2.0.0.1] so that we can get it on the radar

Comment 19

[Adam Guthrie]

*** Bug 358940 has been marked as a duplicate of this bug. ***

Comment 20

[Marcia Knous [:marcia]]

I did notice today that when I go to www.sirius.com and click on the "Listen Online" link, I get this in the Error Console:

Error: [Exception... "'Permission denied to get property XULElement.accessKey' when calling method: [nsIDOMXULLabelElement::accessKey]"  nsresult: "0x8057001e (NS_ERROR_XPC_JS_THREW_STRING)"  location: "JS frame :: http://cdn.sirius.com/c/scripts/common.js :: launchPlayer :: line 77"  data: no]
Source File: http://cdn.sirius.com/c/scripts/common.js
Line: 77

Comment 21

[Scott Susslin]

I have unfortunately run across this bug too, and my company is about to launch a site that depends on Flash launching a popup window that contains a form that users must fill out. So needless to say I'm kinda freaking out, as the site goes live in 4 days. Is there a work-around out there? 

I would LOVE to see the priority of this bug go up as well. Thanks all. 

Comment 22

[Daniel Veditz [:dveditz]]

Not realistic to block 1.8.1.1 for this since there's not been any progress. Really want it, though.

Comment 23

[Hiro]

*** Bug 354952 has been marked as a duplicate of this bug. ***

Comment 24

[(not reading, please use [e-mail] instead)]

update:  bret reckard sees this bug when using yahoo sports, which pops up a window that has a flash ap.

Comment 25

[Håkan Waara]

Is this bug only seen in Firefox 2, _not_ trunk?

Comment 26

[Steven Michaud [:smichaud] (Retired)]

It _did_ happen on the trunk, but stopped happening when the switch to
Cocoa widgets was landed (on 2006-09-28, according to Josh Aas's blog
http://weblogs.mozillazine.org/josh/archives/2006/09/).  (It happens
with Minefield 2006-09-27-trunk, but not with the next available full
trunk build, 2006-10-10-04.)

It doesn't happen in Camino (1.0.3 or on the trunk).

Comment 27

[John M. Black]

Is this the same as bug 363616?  (Not sure.)  Interested in hearing the resolution to this.

Comment 28

[John M. Black]

(In reply to comment #25)
> Is this bug only seen in Firefox 2, _not_ trunk?
> 

I also get it in an Embed project we're working on, using the ActiveX wrapper that (I think) is based on 1.7.2.

Comment 29

[Steven Michaud [:smichaud] (Retired)]

> Is this the same as bug 363616?

I doubt it.  But bug 363616 is very ill defined ... so it's difficult
to be sure.

Comment 30

[(not reading, please use [e-mail] instead)]

over to nobody, as mark doesn't have cycles for this.

if someone does have cycles, steven's comment #16 looks like a good place to start.

Comment 31

[Jay Patel [:jay]]

Clearing blocking flag, but would be nice to get this fixed.  Assigning to Josh Aas for more investigation.

Comment 32

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix for this bug 355071 and bug 345010

Here's a fix for this bug (355071) and bug 345010.  It's a patch
against the Firefox 2.0.0.1 source, and should still apply without
offsets to the 1.8.1 branch (the relevant files haven't changed since
2.0.0.1 came out).

Once this patch is landed (or something like it), plugins that (like
the Flash plugin) do IME input (and set their own TSM documents) won't
need to worry about setting the current TSM document back (to one the
browser can use).  In fact it won't even be a problem if these plugins
set the TSM document incorrectly (as the Flash plugin now does for
this bug (355071) and bug 345010).

The basic logic is very simple:  On every raw key down event, I check
if the keyboard focus is in a plugin or not.  If it isn't and the
current TSM document isn't the browser's TSM document for the current
page, I activate the browser's TSM document (for the current page).

But it was a real challenge figuring out how to tell when a plugin has
the keyboard focus.  I think my patch does this reliably and
efficiently, but it's still _quite_ complex.  If anyone knows a
simpler method, please pass it along.

Comment 34

[Steven Michaud [:smichaud] (Retired)]

Attachment: Revised fix (more optimizations)

It turns out that the "focused widget" returned by
mEventDispatchHandler->GetActive() is usually (always?) a top-level
widget -- which means that calls to WidgetContentIsPlugin() using this
widget can cause the entire widget hierarchy to be browsed.  So I've
optimized away unnecessary calls to WidgetContentIsPlugin().

Comment 35

[Steven Michaud [:smichaud] (Retired)]

Attachment: Revised fix (works with shared build)

Here's yet another revision of my patch.

I found that I needed to fiddle with Makefile.in to get the patch to
compile in a shared-libary build, and I made a couple of other minor
changes.

Comment 36

[Steven Michaud [:smichaud] (Retired)]

Josh, if you don't have time for this, or think someone else should
review my patch, please suggest a name.

If possible I'd like to get the patch into Firefox 2.0.0.2.  And I
don't want to still be making changes to it at the last minute :-)

Comment 37

[Josh Aas]

Comment on attachment 251000 [details] [diff] [review]
Revised fix (works with shared build)

I'd like to have roc look at this if he's willing to. If there is a faster way to do some of this he'd probably know.

Comment 38

[Steven Michaud [:smichaud] (Retired)]

Attachment: Revised fix (simpler)

I dug around in nsViewManager and nsPresShell, and found a simpler way
to detect when a plugin has the keyboard focus.  It may not be any
faster (nsFrameManager::GetPrimaryFrameFor(nsIContent* aContent) still
occasionally needs to browse the entire nsIContent hierarchy), but I'm
sure it's better in the long run (GetPrimaryFrameFor() seems to lie at
the heart of the event-processing infrastructure, which means that a
lot of attention is spent on keeping it optimized).

Comment 39

[Steven Michaud [:smichaud] (Retired)]

Change to more accurate summary.

Comment 40

[Steven Michaud [:smichaud] (Retired)]

roc, if you can, please review this patch sometime in the next few
days ... or please suggest someone else for the job :-)

If possible I'd like to get this patch into Firefox 2.0.0.2.  This bug
was nominated (as a blocker) and rejected a couple of times ... but
that's (presumably) because a patch didn't yet exist.

Comment 41

[John M. Black]

I'm not in a position to build right now, but as I seem to have the problem in FF  2.0.0.1 like 100% of the time, I'd be happy to test a release dll for you...

Comment 42

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #41)

Unfortunately, I can't give you anything to test until this patch gets
landed (on the mozilla1.8 branch).

Comment 43

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+ifdef BUILD_SHARED_LIBS
+SHARED_LIBRARY_LIBS = \
+		$(DIST)/lib/libxpwidgets_s.a \
+		$(DIST)/lib/libgkview_s.a \
+		$(NULL)
+else

This is bad. You're linking in an extra copy of views here with no guarantee that things will actually work properly with two copies.

When a plugin is focused, aren't there OS-level focus notifications that you can observe to detect that we're in a plugin? If you just know which widget is focused, we can easily add an API to nsIWidget to determine whether the widget is for a plugin or not.

Comment 44

[Steven Michaud [:smichaud] (Retired)]

> aren't there OS-level focus notifications that you can observe to
> detect that we're in a plugin?

I don't think so ... but I'm looking into it.

> You're linking in an extra copy of views

As best I can tell, there's just one views dependency that requires me
to explicitly link in libgkview_s.a (and then only when doing a shared
build) -- nsIView::GetViewFor().

Do you know of a way I can get an nsIDocument from an nsIWidget
without using nsIView::GetViewFor(nsIWidget)?

(I can't just copy the code for nsIView::GetViewFor(nsIWidget), since
that uses the private ViewWrapper class.)

> If you just know which widget is focused, we can easily add an API
> to nsIWidget to determine whether the widget is for a plugin or not.

I _do_ have a widget ... but it seems to always be a top-level widget.

Comment 45

[Steven Michaud [:smichaud] (Retired)]

> we can easily add an API to nsIWidget to determine whether the
> widget is for a plugin or not

Could you add an API to get the nsIDocument that corresponds to an
nsIWidget?  Something like the nsIDocument * GetDocumentFor(nsIWidget
*aWidget) I have in my patch?

Comment 46

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I could, but in the future I'd like to have documents that aren't associated with widgets.

I think the best thing to do is to figure out which native widget has focus somehow, then see whether that's a plugin.

Comment 47

[Steven Michaud [:smichaud] (Retired)]

> I think the best thing to do is to figure out which native widget
> has focus somehow, then see whether that's a plugin.

By "native widget" do you mean something like a ControlRef (a Carbon
object -- something that can have the keyboard focus)?  If so, I don't
think it will be possible to know whether or not this corresponds to
(or is inside) a plugin, using only information garnered from the OS
-- the notion of "plugin" is a browser concept, not an OS concept.

Comment 48

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Can we map from the Carbon object to an nsIWidget?

Comment 49

[Steven Michaud [:smichaud] (Retired)]

> Can we map from the Carbon object to an nsIWidget?

Not as things currently stand.  I could add code to do this, but it
would require a hashtable or linked list.

I've now found another place to look for a solution (one that doesn't
use the nsIWidget hierarchy, nsIDocument, or any of that stuff):
There's an nsIWidget::SetFocus() method, which is implemented in
widget/src/mac/nsWindow.cpp.  It doesn't currently keep track of which
nsIWidget has been focused by the browser, but I could make it do so.
The question is whether this method gets called whenever an nsIFrame
is focused (on the nsIWidget corresponding to that nsIFrame).

Comment 50

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix, rev4

After beating my head against several walls for more than a day, I've
discovered that it's impossible to know whether or not a plugin is
focused without using the nsIDocument associated with the nsIWidget
provided by mEventDispatchHandler->GetActive().  More about this
below.

So my current patch isn't much changed from the previous version.  But
I did figure out how to stop using nsIView::GetViewFor(nsIWidget).  So
we no longer need to link in libgkview_s.a, even when doing a shared
library build.

I also moved the static "utility" methods to nsToolkit -- it seemed a
better place to put them.

I found a clever way to associate an nsIWidget with each ControlRef
created by the Mac-specific widget code (you can set an arbitrary
"property" on a ControlRef).  But then I found that the Control
Manager GetKeyboardFocus() method (which is supposed to tell you which
ControlRef is focused) always returns NULL (and these objects never
receive any of the kEventClassControl "focus events").  The comment on
SetKeyboardFocus() says that "keyboard focus is only available if an
embedding hierarchy has been established in the focusable control's
window".  I assume this means that the focus methods only work (and
the associated Carbon events are only sent) when your controls are
HIView objects.  It'd be a great deal of trouble to change to using
HIView controls -- and certainly not worth doing in a dead-end branch
like the mozilla1.8 branch.

Then I took a look at the nsWindow::SetFocus() method.  But I
discovered that this method is only ever called on fairly high-level
widgets (though not usually the top-level widget) -- so near the top
of the widget hierarchy that, when I load a plugin into the browser
page, _every widget_ contains the plugin.  It took me several hours,
but I finally tracked this problem down to how nsIWidget::SetFocus()
is called in nsEventStateManager::SendFocusBlur() (in the "if
(aEnsureWindowHasFocus)" block) -- it's called on the widget
associated with the nsViewManager's root view, not on the widget
associated with the relevant nsGUIEvent.

Comment 51

[Steven Michaud [:smichaud] (Retired)]

Well, roc, do you want me to make any more changes?

Comment 52

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't like this approach because it's really ugly and scary-fragile. It's branch only, so I guess we can take it if we have no other choice, but I'd like to find another choice if we possibly can.

What if we just set the current TSM document to our window's TSM document when our window gets focused, would that work?

Comment 53

[Steven Michaud [:smichaud] (Retired)]

> What if we just set the current TSM document to our window's TSM
> document when our window gets focused, would that work?

No.  If the focus is in a plugin, this would break IME input in the
plugin (the Flash plugin does support IME input).

> it's really ugly and scary

What are you talking about here?

Do you think my code doesn't reliably determine when a plugin is
focused?  Are you worried about false negatives?  False positives?

Or are there cases when the TSM doc shouldn't be corrected (set to the
browser's window's TSM doc) even when a plugin isn't focused?

Comment 54

[Steven Michaud [:smichaud] (Retired)]

>> What if we just set the current TSM document to our window's TSM
>> document when our window gets focused, would that work?
>
> No.  If the focus is in a plugin, this would break IME input in the
> plugin (the Flash plugin does support IME input).

Actually, this is already being done (in
nsMacEventHandler::HandleActivateEvent()), and the Flash plugin gets
around the problem of broken IME input by setting its own TSM document
(when the window gets activated, I assume).

But what nsMacEventHandler::HandleActivateEvent() does clearly isn't
enough -- we've still got this bug and bug 345010.

Comment 55

[Steven Michaud [:smichaud] (Retired)]

Moreover (speaking of fragility and robustness), my fix has the
advantage that it doesn't matter what plugins set the TSM document to
-- when the focus keyboard focus is in a browser widget (as opposed to
a plugin widget), the TSM document will always be set correctly (to
the browser window's TSM document).

Comment 56

[Robert O'Callahan (:roc) (email my personal email if necessary)]

What's ugly is the fact that you need to grope around the nsIWidget hierarchy looking for a document. Including scary code like this:

+    if (entered > 1)
+      nextSibling = widgetLocal->GetNextSibling();

Why is that there? Merely because it worked, I suppose. What does it depend on? No-one knows. So if we changed the widget hierarchy a little bit, boom, his no longer works and whoever made the change will take a long time to figure out what happened. That's scary.

And of course generally, widget code should not know about views, content, frames and documents.

Just because code works (today) doesn't mean it's good.

Comment 57

[Robert O'Callahan (:roc) (email my personal email if necessary)]

oops, I understand that bit of code better now and it makes more sense.

However, GetDocumentFor should be rewritten so you don't need this "entered" hack.

Comment 58

[Robert O'Callahan (:roc) (email my personal email if necessary)]

In particular, you should check whether aWidget has a document, and then iterate through all its children calling GetDocumentFor on each one. It's still really ugly that you to have to mess with views, content, documents etc but at least it'll make more sense.

The caching you do in IsPluginFocused is dangerous. If you're very unlucky, prevWidget could be destroyed and a new widget created with the same address and passed in as aWidget. Then you would try to use a deleted document. Boom. You're also assuming that the document associated with a widget never changes, which actually isn't true. You need to get rid of that caching. (Besides, this only gets called once per input event so it is almost certainly not worth optimizing for speed).

Comment 59

[Robert O'Callahan (:roc) (email my personal email if necessary)]

If we were doing this on trunk I would suggest that we modify nsEventStateManager to notify the widget for a focused frame that it has been focused. I don't know why it doesn't call SetFocus, actually, but even if we don't want to call SetFocus we could call some new method that you could then track to know whether a plugin has been focused.

But apparently on trunk we don't need this anyway, and we don't want to change interfaces on branch, especially cross-platform ones, so we'll go with your approach, if you make the changes I mentioned. But it's still ugly :-)

Comment 60

[Steven Michaud [:smichaud] (Retired)]

> However, GetDocumentFor should be rewritten so you don't need this
> "entered" hack.

I'll do that ... though there's really no need.

> And of course generally, widget code should not know about views,
> content, frames and documents.

Generally speaking I agree with you.  But in this case I have no
choice.  And I've seen many similar cases throughout the Mozilla.org
source tree.  I suppose the best solution would be an nsIView
isPluginFocused() method, arranged so that (unlike
nsIView::GetViewFor(nsIWidget) it doesn't have to be statically linked
in.  But in the meantime ...

> Just because code works (today) doesn't mean it's good.

When/if the widget/frame/view/document hierarchy does change, you'll
also need to make _lots_ of changes elsewhere.

Comment 61

[Steven Michaud [:smichaud] (Retired)]

> In particular, you should check whether aWidget has a document, and
> then iterate through all its children calling GetDocumentFor on each
> one.

I don't understand.  I've assumed that each browser page has (at any
one time) exactly one nsIDocument (or at least that whatever
nsIDocument I find will lead me to a PresShell (and so forth) that
will tell me which frame is focused).

Comment 62

[Steven Michaud [:smichaud] (Retired)]

> The caching you do in IsPluginFocused is dangerous.

OK, I'll get rid of it.

Comment 63

[Steven Michaud [:smichaud] (Retired)]

> But apparently on trunk we don't need this anyway,

Actually, I suspect that we might.  Flash IME input is currently
broken on the trunk, and I suspect the reason is the "fix" in
nsPluginInstanceOwner::ProcessEvent() (Cocoa-widgets only) for bug
183313 that calls DeactivateTSMDocument(TSMGetActiveDocument()) on an
NS_FOCUS_CONTENT event.  Removing this "fix" will probably require
something like what I've done here.

But I also suspect that in src/mac/cocoa I'll be able to find out from
the OS exactly which nsIWidget is focused.

Comment 64

[Steven Michaud [:smichaud] (Retired)]

>> In particular, you should check whether aWidget has a document, and
>> then iterate through all its children calling GetDocumentFor on each
>> one.
>
> I don't understand.  I've assumed that each browser page has (at any
> one time) exactly one nsIDocument (or at least that whatever
> nsIDocument I find will lead me to a PresShell (and so forth) that
> will tell me which frame is focused).

Now I understand.  You're just telling me how I should get rid of
"entered".

Comment 65

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yeah.

> And I've seen many similar cases throughout the Mozilla.org
> source tree.

Sure, but we try to make things better by removing badness, not make things worse by adding it.

Comment 66

[Steven Michaud [:smichaud] (Retired)]

> Sure, but we try to make things better by removing badness, not make
> things worse by adding it.

Sometimes "badness" isn't so bad.  Sometimes the best is enemy of the
good.

But I'd also agree that the good should be replaced by the better when
that comes along ... and oftentimes we're far too busy when that
happens, and don't do it because it isn't urgent.

Comment 67

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix, rev5

OK, here's the next patch revision.

roc, let me know what you think.

There's still an outside chance my patch can make it into Firefox
2.0.0.2.

Comment 68

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+    frame = nsnull;
+    content = nsnull;
+    document = nsnull;
+    view = nsToolkit::GetViewFor(widgetChild);
+    if (view)
+      frame = NS_STATIC_CAST(nsIFrame*, view->GetClientData());
+    if (frame)
+      content = frame->GetContent();
+    if (content)
+      document = content->GetCurrentDoc();
+    if (document)
+      return document;
+    widgetChild = nextChild;

Why are you doing this? This will be checked when you call GetDocumentFor recursively on widgetChild. 

Comment 69

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix, rev6

Sorry.  Here's the fix.

Comment 70

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 251994 [details] [diff] [review]
Fix, rev6

ok!

Comment 71

[Steven Michaud [:smichaud] (Retired)]

What's next?  Who should do the superreview?  Should I ask for 1.8.1.2
approval?

Comment 72

[Josh Aas]

If roc is willing to have his review count as sr, then you should request 1.8.1.2 approval.

Comment 73

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 251994 [details] [diff] [review]
Fix, rev6

> If roc is willing to have his review count as sr

On the assumption this is ok with roc, here goes.

Comment 74

[Robert O'Callahan (:roc) (email my personal email if necessary)]

That's ok.

Comment 75

[Daniel Veditz [:dveditz]]

Comment on attachment 251994 [details] [diff] [review]
Fix, rev6

approved for 1.8 branch, a=dveditz for drivers

Comment 76

[Steven Michaud [:smichaud] (Retired)]

Just so that people know:  I don't know how to land this patch, and
even if I did I'm not sure I have the permissions.

Comment 77

[:Gavin Sharp [email: [e-mail]]]

I've landed this patch on the 1.8 branch for Firefox 2.0.0.2. Should this land on the trunk, given that widget/mac/src isn't built by default there?

mozilla/widget/src/mac/nsMacEventHandler.cpp 	1.172.4.24
mozilla/widget/src/mac/nsMacEventHandler.h 	1.41.4.13
mozilla/widget/src/mac/nsToolkit.cpp 	1.62.12.5
mozilla/widget/src/mac/nsToolkit.h 	1.33.20.5

Comment 78

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This shouldn't land on trunk.

Comment 79

[Steven Michaud [:smichaud] (Retired)]

For people who want to test my fix:

The first mozilla1.8-branch nightly containing it
(firefox-2007-01-22-07-mozilla1.8) has appeared at

ftp://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/

Comment 80

[Marcia Knous [:marcia]]

verified fixed on the 1.8 branch using Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.2pre) Gecko/20070125 BonEcho/2.0.0.2pre. I can't test the sirius.com site since it doesn't like the fact the build is branded Bon Echo (browser not supported...), but several other tests with variants of Comment #1 and using www.zoomzoomlive.com worked fine for me. adding fixed keyword.

Comment 83

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Steven, why is this still open?

Comment 84

[Steven Michaud [:smichaud] (Retired)]

I probably just forgot to close it ... so I'll do that now.