Closed Bug 355071 Opened 18 years ago Closed 13 years ago

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

Categories

(Firefox :: Shell Integration, defect)

Product:
Firefox
Component:
Shell Integration
Version:
2.0 Branch
Platform:
PowerPC
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: offsky, Assigned: smichaud)

References

Details

(Keywords: inputmethod, regression, verified1.8.1.2, Whiteboard: [Fx 2.0.0.1])

Attachments

(1 file, 6 obsolete files)

Fix, rev6
14.82 KB, patch
roc
: review+
jaas
: review+
dveditz
: approval1.8.1.2+
Details | Diff | Splinter Review 
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.
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
Whiteboard: DUPEME
This bug may be a dup of bug 345010.

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).
Status: UNCONFIRMED → NEW
Ever confirmed: true
*** Bug 356365 has been marked as a duplicate of this bug. ***
This bug really needs attention before 2.0 ships. Perhaps if it didn't have severity: normal, Jake?
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.
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.
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).

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.
Keywords: regression
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? :-)

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.)

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.
Flags: blocking-firefox2?
changing flags to indicate I want to block for the next release. sorry for the bugspam.
Flags: blocking-firefox2? → blocking1.8.1.1?
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


(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).

(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.)

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?
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
Whiteboard: DUPEME → [Fx 2.0.0.1]
*** Bug 358940 has been marked as a duplicate of this bug. ***
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
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. 
Not realistic to block 1.8.1.1 for this since there's not been any progress. Really want it, though.
Assignee: nobody → mark
Flags: wanted1.8.1.x+
Flags: blocking1.8.1.1?
Flags: blocking1.8.1.1-
*** Bug 354952 has been marked as a duplicate of this bug. ***
Flags: blocking1.8.1.2?
update:  bret reckard sees this bug when using yahoo sports, which pops up a window that has a flash ap.
Is this bug only seen in Firefox 2, _not_ trunk?
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).

Is this the same as bug 363616?  (Not sure.)  Interested in hearing the resolution to this.
(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.
> Is this the same as bug 363616?

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

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.
Assignee: mark → nobody
Flags: blocking1.8.1.2? → blocking1.8.1.2+
Flags: blocking1.8.1.2+
Clearing blocking flag, but would be nice to get this fixed.  Assigning to Josh Aas for more investigation.
Assignee: nobody → joshmoz
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.
Attached patch Revised fix (more optimizations) (obsolete) — — Splinter Review 
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().
Attachment #250807 - Attachment is obsolete: true
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.
Attachment #250893 - Attachment is obsolete: true
Attachment #251000 - Flags: review?(joshmoz)
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 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.
Attachment #251000 - Flags: review?(roc)
Attached patch Revised fix (simpler) (obsolete) — — Splinter Review 
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).
Attachment #251000 - Attachment is obsolete: true
Attachment #251000 - Flags: review?(roc)
Attachment #251000 - Flags: review?(joshmoz)
Attachment #251396 - Flags: review?(roc)
Attachment #251396 - Flags: review?(joshmoz)
Assignee: joshmoz → smichaud
Change to more accurate summary.
Summary: Flash prevents other FF windows from gaining keyboard focus → Flash stops keyboard input in other FF windows (TSM doc problem)
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.

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...
(In reply to comment #41)

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

+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.
> 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.

> 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?

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.
> 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.

Can we map from the Carbon object to an nsIWidget?
> 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).
Attached patch Fix, rev4 (obsolete) — — Splinter Review 
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.
Attachment #251396 - Attachment is obsolete: true
Attachment #251396 - Flags: review?(roc)
Attachment #251396 - Flags: review?(joshmoz)
Attachment #251723 - Flags: review?(roc)
Attachment #251723 - Flags: review?(joshmoz)
Well, roc, do you want me to make any more changes?

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?
> 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?

>> 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.

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).

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.
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.
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).
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 :-)
> 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.

> 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).

> The caching you do in IsPluginFocused is dangerous.

OK, I'll get rid of it.

> 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.

>> 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".

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.
> 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.
Attached patch Fix, rev5 (obsolete) — — Splinter Review 
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.
Attachment #251723 - Attachment is obsolete: true
Attachment #251723 - Flags: review?(roc)
Attachment #251723 - Flags: review?(joshmoz)
Attachment #251969 - Flags: review?(roc)
Attachment #251969 - Flags: review?(joshmoz)
+    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.
Attached patch Fix, rev6 — — Splinter Review 
Sorry.  Here's the fix.
Attachment #251969 - Attachment is obsolete: true
Attachment #251969 - Flags: review?(roc)
Attachment #251969 - Flags: review?(joshmoz)
Attachment #251994 - Flags: review?(roc)
Attachment #251994 - Flags: review?(joshmoz)
Attachment #251994 - Flags: review?(joshmoz) → review+
What's next?  Who should do the superreview?  Should I ask for 1.8.1.2
approval?

If roc is willing to have his review count as sr, then you should request 1.8.1.2 approval.
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.
Attachment #251994 - Flags: approval1.8.1.2?
Comment on attachment 251994 [details] [diff] [review]
Fix, rev6

approved for 1.8 branch, a=dveditz for drivers
Attachment #251994 - Flags: approval1.8.1.2? → approval1.8.1.2+
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.
Whiteboard: [Fx 2.0.0.1] → [Fx 2.0.0.1][checkin needed]
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
Keywords: fixed1.8.1.2
Whiteboard: [Fx 2.0.0.1][checkin needed] → [Fx 2.0.0.1]
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/

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.
Keywords: fixed1.8.1.2 → verified1.8.1.2
Steven, why is this still open?
I probably just forgot to close it ... so I'll do that now.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: