[Mac] Some events incorrectly passed to a plugin that occur over the plugin

RESOLVED FIXED

Status

()

RESOLVED FIXED
8 years ago
5 years ago

People

(Reporter: justinf.utest, Assigned: smichaud)

Tracking

(Depends on: 1 bug, {regression})

unspecified
x86
Mac OS X
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 final+)

Details

(Whiteboard: [hardblocker][has patch], URL)

Attachments

(3 attachments, 5 obsolete attachments)

(Reporter)

Description

8 years ago
User-Agent:       Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b9) Gecko/20100101 Firefox/4.0b9
Build Identifier: Firefox 4.0b9

Go to http;//youtube.com
enter "Geor" in the search box
the drop down box appears with "George Michale Faith"
Tried to click it. Wouldn't click. 

Reproducible: Always

Steps to Reproduce:
1. Enter "Geor" in the search box
2. See "George Michael Faith" in drop Down box
3. Try to click that link
Actual Results:  
it is invisible you click anything below.  Not going to video George Michael Faith.   

Expected Results:  
Should click and go to video linked

http://screencast.com/t/pbR1Q1wtF
(Reporter)

Updated

8 years ago
Summary: Enter something in the search box " Geor" for George Michael Faith Try and click it. → YouTube Search: Enter something in the search box " Geor" for George Michael Faith Try and click it.
Yeah, definitely happening.  Looks like we end up targeting whatever is below the dropdown with the click.

Regression from 3.6.

A regression window would be really useful here.
Status: UNCONFIRMED → NEW
blocking2.0: --- → ?
Component: General → Layout: View Rendering
Ever confirmed: true
Keywords: regression, regressionwindow-wanted
QA Contact: general → layout.view-rendering
OSX only? Can't reproduce on Linux. I'll try on OSX.
Does the problem happen only when there is a video (Flash) loaded in the page before the search?

When I load http://youtube.com, there are just preview images of some
videos, no actual video.
No, I get this even with the images: I click on the list entry and the image's click handler is triggered, replacing itself with a video, etc.

And yes, I'm on OSX.
Can't reproduce on OSX either :/
Is youtube perhaps giving me different content.
It's possible, yes....  :(

Comment 7

8 years ago
I was able to reproduce on OSX this morning

Comment 8

8 years ago
Now that the ad is gone, it is difficult to reproduce. It looks like it occurs only when there is an ad with a playable video that is covered by the suggested search results.
Boris, can you you still reproduce? If so, please take the bug and we'll block on it. If you can't reproduce it anymore, then we probably won't block on it.
blocking2.0: ? → final+
Yeah, I can still reproduce this.

The regression range I see is http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=edf41ff32f08&tochange=a42e9b001bc8

My money is on bug 601182; trying to confirm now.
Keywords: regressionwindow-wanted
Olli, were you testing a nightly, or a self-build?  If you were using a 64-bit-only self-build (i.e. not 32-bit and not a build with a 32-bit plugin-container), you wouldn't see the issue, since you would have no Flash then.
I was testing self-build on 32bit osx, and on 32 bit linux.
OK.  For what it's worth, my 32-bit self-build on OSX shows the problem, while the 64-bit does not (no Flash).  So you not seeing it could still be a difference in the content youtube serves....
hg bisect says:

changeset:   57526:1950375b2ec2
user:        Steven Michaud <smichaud@pobox.com>
date:        Mon Nov 15 15:12:50 2010 -0600
summary:     Bug 601182 - Spurious focus events sent to NPAPI plugins on OS X in Cocoa event mode. Patch partly by enndeakin. r=josh,smaug a=blocking2.0BetaN+
Blocks: 601182
Boris:  How, exactly, do you reproduce this bug?

(I haven't tried yet.  I just want to save myself some trouble.)
Steven, I just use the steps in comment 0 and click any of the entries near the bottom of the dropdown (the ones that are over the ad).
And I can confirm that when I do that I get a FocusPlugin call for the plug-in that's below the dropdown, with this stack:

#0  nsFocusManager::FocusPlugin (this=0x5128880, aContent=0x1ec8cc60) at /Users/bzbarsky/mozilla/vanilla/mozilla/dom/base/nsFocusManager.cpp:1003
#1  0x004388ae in nsObjectFrame::HandleEvent (this=0x6785da8, aPresContext=0x6766000, anEvent=0xbfffd668, anEventStatus=0xbfffd1cc) at /Users/bzbarsky/mozilla/vanilla/mozilla/layout/generic/nsObjectFrame.cpp:2227
#2  0x003745c1 in nsPresShellEventCB::HandleEvent (this=0xbfffd290, aVisitor=@0xbfffd1c0) at /Users/bzbarsky/mozilla/vanilla/mozilla/layout/base/nsPresShell.cpp:1466
#3  0x007a73c4 in nsEventTargetChainItem::HandleEventTargetChain (this=0x5a58a40, aVisitor=@0xbfffd1c0, aFlags=6, aCallback=0xbfffd290, aMayHaveNewListenerManagers=0, aPusher=0xbfffd1d8) at /Users/bzbarsky/mozilla/vanilla/mozilla/content/events/src/nsEventDispatcher.cpp:386
#4  0x007a7eef in nsEventDispatcher::Dispatch (aTarget=0x1ec8cc60, aPresContext=0x6766000, aEvent=0xbfffd668, aDOMEvent=0x0, aEventStatus=0xbfffd428, aCallback=0xbfffd290, aTargets=0x0) at /Users/bzbarsky/mozilla/vanilla/mozilla/content/events/src/nsEventDispatcher.cpp:628
#5  0x0035eba1 in PresShell::HandleEventInternal (this=0x23057e60, aEvent=0xbfffd668, aView=0x1ec8d6d0, aStatus=0xbfffd428) at /Users/bzbarsky/mozilla/vanilla/mozilla/layout/base/nsPresShell.cpp:6996
#6  0x0036f63b in PresShell::HandleEvent (this=0x23057e60, aView=0x1ec8d6d0, aEvent=0xbfffd668, aDontRetargetEvents=0, aEventStatus=0xbfffd428) at /Users/bzbarsky/mozilla/vanilla/mozilla/layout/base/nsPresShell.cpp:6735
#7  0x0098aad4 in nsViewManager::HandleEvent (this=0x1fbbeb20, aView=0x1ec8fd50, aEvent=0xbfffd668) at /Users/bzbarsky/mozilla/vanilla/mozilla/view/src/nsViewManager.cpp:1095
#8  0x0098efe8 in nsViewManager::DispatchEvent (this=0x1fbbeb20, aEvent=0xbfffd668, aView=0x1ec8fd50, aStatus=0xbfffd57c) at /Users/bzbarsky/mozilla/vanilla/mozilla/view/src/nsViewManager.cpp:1073
#9  0x0098811c in HandleEvent (aEvent=0xbfffd668) at /Users/bzbarsky/mozilla/vanilla/mozilla/view/src/nsView.cpp:161
#10 0x01429be4 in nsChildView::DispatchEvent (this=0x1ec8fdc0, event=0xbfffd668, aStatus=@0xbfffd69c) at /Users/bzbarsky/mozilla/vanilla/mozilla/widget/src/cocoa/nsChildView.mm:1797
#11 0x01428b29 in -[ChildView sendFocusEvent:] (self=0x1ec90da0, _cmd=0x1d30ed2, eventType=163) at /Users/bzbarsky/mozilla/vanilla/mozilla/widget/src/cocoa/nsChildView.mm:2549
#12 0x0142a3f2 in -[ChildView updateCocoaPluginFocusStatus:] (self=0x1ec90da0, _cmd=0x1d333d9, hasFocus=1 '\001') at /Users/bzbarsky/mozilla/vanilla/mozilla/widget/src/cocoa/nsChildView.mm:5749
#13 0x0142dd3d in -[ChildView becomeFirstResponder] (self=0x1ec90da0, _cmd=0x991cdddc) at /Users/bzbarsky/mozilla/vanilla/mozilla/widget/src/cocoa/nsChildView.mm:5761
#14 0x98a2fa8c in -[NSWindow makeFirstResponder:] ()
#15 0x98b4cb29 in -[NSWindow sendEvent:] ()
#16 0x014195e8 in -[ToolbarWindow sendEvent:] (self=0x51d92d0, _cmd=0x991c8a58, anEvent=0x1fbd9000) at /Users/bzbarsky/mozilla/vanilla/mozilla/widget/src/cocoa/nsCocoaWindow.mm:2439
#17 0x98a65817 in -[NSApplication sendEvent:] ()
#18 0x989f92a7 in -[NSApplication run] ()

In becomeFirstResponder mIsPluginView is true.  So for some reason the cocoa widgetry decides to send a focus event to the plug-in widget(?), which transfers focus manager focus to the plug-in, etc.  I bet that causes us to mistarget the click event....

Steven, could you or Josh take a look at this?  I don't understand the cocoa widgetry well enough to deal here, I suspect.

Updated

8 years ago
Assignee: bzbarsky → smichaud
Thanks, Boris, for the stack.

I just tried a 32-bit self-build on OS X 10.5.8 and was unable to reproduce -- either with Flash or with my DebugEventsPlugin from bug 441880.

Later I'll try in other configurations.

I'll definitely take a look at this -- probably tomorrow.
OK. If you want to do some remote debugging, have me try logging patches, whatever, I'm happy to help out!
Did some more digging.  The menu goes away when clicked, in the failing case, due to script that's handling an NS_BLUR_CONTENT event, with the currentTaret an HTMLInputElement.  This happens _after_ the FocusPlugin call (in fact that call is what causes the blur to happen).
Created attachment 506913 [details]
Testcase, probably not minimal
Steven, can you reproduce the issue with the attached testcase?  For me it shows the problem every other click or so...
Created attachment 506914 [details]
Testcase with clearer directions
Attachment #506913 - Attachment is obsolete: true
> Steven, can you reproduce the issue with the attached testcase?

Yes, but only on OS X 10.6.X (10.6.6) in 32-bit mode.

I can't on OS X 10.5.8, or on OS X 10.6.6 in 64-bit mode (the
default).

I tested with today's Minefield nightly and with my 32-bit self build
(made with code pulled this morning).

I'll try to make sense of this later, when I start digging into this
bug myself.

My 64-bit tests were with today's nightly -- which *can* load 32-bit
plugins (and of course only runs 64-bit on 10.6.X).
(In reply to comment #24)
> > Steven, can you reproduce the issue with the attached testcase?
> 
> Yes, but only on OS X 10.6.X (10.6.6) in 32-bit mode.
> 
> I can't on OS X 10.5.8, or on OS X 10.6.6 in 64-bit mode (the
> default).

Ah, this could be the reason why I can't reproduce.
I have only 10.5.x
(Following up comment #24)

>> Steven, can you reproduce the issue with the attached testcase?
>
> Yes, but only on OS X 10.6.X (10.6.6) in 32-bit mode.

Actually this is wrong -- I was too hasty.  Using Boris's testcase
from comment #23, I can reproduce this bug on OS X 10.5.8 ... on the
second try.  And in 64-bit mode on 10.6.6, I get other wierdness (a
new blank tab opens up), which turns out to be related.

Testing with a variant of Boris's testcase that loads my
DebugEventsPlugin from bug 441880, my results are much clearer:  Those
tests show that this bug exists "everywhere" on OS X, and that
variants of it existed on the trunk (in Cocoa event mode) before my
patch for bug 601182 landed, and even on the branches (in Carbon event
mode).

The basic problem is that we incorrectly pass some events to the
plugin that occur in a drop-down list above the plugin -- which can
include mouse up and down events and focus events.  This doesn't occur
with every kind of drop-down list.  But it does always occur with the
kind of drop-down list contained in Boris's testcase.
No longer blocks: 601182
Summary: YouTube Search: Enter something in the search box " Geor" for George Michael Faith Try and click it. → [Mac] Some events incorrectly passed to a plugin that occur over the plugin
This is really a Plugins bug ... or possibly a Cocoa widgets bug.

But I hesitate to reclassify it because doing that may cause it to lose its blocking flag.
> Testing with a variant of Boris's testcase that loads my
> DebugEventsPlugin from bug 441880, my results are much clearer:
> Those tests show that this bug exists "everywhere" on OS X, and that
> variants of it existed on the trunk (in Cocoa event mode) before my
> patch for bug 601182 landed, and even on the branches (in Carbon
> event mode).

This bug doesn't exist in Safari, Chrome or Opera.
Assignee: smichaud → nobody
Component: Layout: View Rendering → Plug-ins
QA Contact: layout.view-rendering → plugins
Assignee: nobody → smichaud
> But I hesitate to reclassify it because doing that may cause it to lose its
> blocking flag.

I did reclassify it, and the blocker flag is still there.
The fact that the event delivery changes focus and hence causes the misbehavior observed on this testcase is still a regression from bug 601182, no?
I haven't yet thoroughly investigated this bug.  But I'm pretty sure
that the behavior changes caused by my patch for bug 601182 (and there
were some) will turn out to be minor ... though they might have made
this bug more visible.
By the way, to really see what's going on here you do need to test with my DebugEventsPlugin from bug 441880.
Oh, the _plugin_ parts of this were not much changed by the patch in question.  But it caused a change in the focus behavior on web pages, which caused a change in what focus events fire when, which caused a change in what scripts run when, which is what broke the youtube search box.  At least as far as I can tell.
I'm still mostly preoccupied with bug 626016 and bug 629593.  But
Windows tryserver builds are so slow, and I'm having such horrendous
trouble getting usable Windows builds of my own, that I sometimes have
gaps of time in which I can work on this bug.

I *hope* I can fix this bug by making changes to Mac-specific plugin
code (and possibly also to Cocoa widgets code).  But if that doesn't
suffice, I may need your help figuring out what else is wrong here :-)
Created attachment 513651 [details] [diff] [review]
Fix

I was right -- this bug is mostly Mac-specific.

The problem is that, as Boris' testcase shows, a DOM element can be
displayed over a plugin that *isn't* (technically) a popup (something
managed by the nsXULPopupManager).

Popups are displayed in their own NSWindow, and can overlay a plugin
without causing trouble.  But when the element from Boris' example (an
absolutely positioned table) overlays a plugin, clicking anywhere
above the plugin (even those parts obscured by the table) causes the
plugin's ChildView (a Cocoa object that's a subclass of NSView) to be
focused.  Or at least that's the case in current code.

My patch changes this behavior, and thereby (in Boris' testcase) stops
the plugin from receiving a focus event and mouse-down event when
clicking on "george lopez".

It also makes a cross-platform change to
nsPluginInstanceOwner::MouseUp() (in nsObjectFrame.cpp) to stop a
mouse-up event from being sent to the plugin (which won't have been
focused by the previous mouse-down event).  This part of my patch
should be correct on all platforms, but at this late date (so soon
before the FF4 release) we might want to play safe and ifdef it to
XP_MACOSX.

A tryserver build will follow in a few hours.
Attachment #513651 - Flags: review?(joshmoz)
The stopPropagation there looks wrong.  It'll confuse whatever mouse listeners the page has, because they will never see the event.  Why is that needed?
Also, the ElementFromPointHelper call seems to be passing device pixels to a function that I think expects CSS pixels.

And frame destruction should null out the content pointer on the widget, right?
> Also, the ElementFromPointHelper call seems to be passing device
> pixels to a function that I think expects CSS pixels.

nsIDocument::ElementFromPointHelper does in fact expect device pixels.
Which is a little odd, but I expect probably is explained by the fact
that it's often called from user-level JavaScript code.

> And frame destruction should null out the content pointer on the
> widget, right?

Yes.  My patch does this when the plugin widget is destroyed (in
[ChildView widgetDestroyed], on a call to nsIWidget::Destroy() from
nsObjectFrame.cpp).

> The stopPropagation there looks wrong.  It'll confuse whatever mouse
> listeners the page has, because they will never see the event.  Why
> is that needed?

I'm not completely sure it's needed, but swallowing the event sounds
right to me:  The DOM is in fact delivering the event to the correct
destination (to the plugin element, rather than to the table element
that's now disappeared).  But it's incorrect to deliver that event to
the plugin (which might be confused by it).

If we didn't swallow it, it might be delivered somewhere else, which I
don't think would be correct.
> nsIDocument::ElementFromPointHelper does in fact expect device pixels.

What makes you say that?  It certainly looks to me like it expects CSS pixels.  Near the beginning of that function:

2825   nscoord x = nsPresContext::CSSPixelsToAppUnits(aX);
2826   nscoord y = nsPresContext::CSSPixelsToAppUnits(aY);

> that it's often called from user-level JavaScript code.

User-level JS code never has access to device pixels.

> on a call to nsIWidget::Destroy() from nsObjectFrame.cpp).

Ah, ok.  I thought that was being triggered from ~ChildView.  OK, this works.

> The DOM is in fact delivering the event to the correct destination

Yes.

> But it's incorrect to deliver that event to the plugin

Then we should block _that_, not block event delivery to everything in the DOM altogether.

> which I don't think would be correct.

It would be correct.  The mouseup is in fact happening.  It's happening over the <embed> or whatever.  That needs to fire a mouseup event.  If we have particular broken consumers who can't handle that, we should fix those consumers or keep them from seeing the event, not break the world.
>> nsIDocument::ElementFromPointHelper does in fact expect device
>> pixels.
>
> What makes you say that?  It certainly looks to me like it expects
> CSS pixels.  Near the beginning of that function:
>
> 2825   nscoord x = nsPresContext::CSSPixelsToAppUnits(aX);
> 2826   nscoord y = nsPresContext::CSSPixelsToAppUnits(aY);

Oops, you're right (I think).  New patch coming up.

I was confused by the fact that (in your testcase) device pixels are
the same size as CSS pixels.  Also, I usually only think about app
units and dev pixels :-(

>> But it's incorrect to deliver that event to the plugin
>
> Then we should block _that_, not block event delivery to everything
> in the DOM altogether.

You know a lot more about this stuff than I do, so I'm willing to
trust your judgement.

Should I get rid of the calls to PreventDefault() and
StopPropagation(), or just the call to StopPropagation()?
It's probably fine to leave the preventDefault() in if that's enough to fix the issue here.  I don't expect that to break websites.  The stopPropagation() almost certainly would.

If we do run into problems with it, we can consider doing something in the plugin code, but that would probably be more risky.
Created attachment 513707 [details] [diff] [review]
Fix rev1 (follow Boris' suggestions)
Attachment #513651 - Attachment is obsolete: true
Attachment #513707 - Flags: review?(joshmoz)
Attachment #513651 - Flags: review?(joshmoz)

Updated

8 years ago
Attachment #513707 - Flags: review?(roc)
This seems like a pretty bad layering violation, to have widget code that mucks with the DOM.

It seems to me a more logical structure would be for nsObjectFrame::HandleEvent to be responsible for focusing the plugin. If the user clicks on content over the plugin, nsObjectFrame::HandleEvent will not be called.
> This seems like a pretty bad layering violation, to have widget code
> that mucks with the DOM.

It's not mucking with the DOM (in the sense of changing it) -- it's
querying it.  We already have an nsQueryContentEvent that does
something similar.  Would you be more comfortable if this kind of
query was implemented using an nsQueryContentEvent (or something like
it)?

> It seems to me a more logical structure would be for
> nsObjectFrame::HandleEvent to be responsible for focusing the
> plugin. If the user clicks on content over the plugin,
> nsObjectFrame::HandleEvent will not be called.

Maybe once we get rid of plugin widgets (and have only one widget for
everything).  But for now focusing a plugin's NSView/ChildView will
have to focus the plugin itself.  And there will be times when we have
to stop a ChildView from gaining the keyboard focus in order to stop a
plugin from being focused.
At what stage of event processing is becomeFirstResponder called?
> At what stage of event processing is becomeFirstResponder called?

Very early -- before the NSLeftMouseDown event has even been sent to
[ChildView mouseDown:] (and of course before any NS_MOUSE_BUTTON_DOWN
event has been sent to Gecko).
I see. That makes things difficult.

What about the case where an ancestor of the plugin element installs a capturing event listener for "mouse down" and calls preventDefault in its event handler? I think that means the plugin should not take focus, but your patch (in fact, this whole approach) won't stop that.
Created attachment 513897 [details]
testcase

On Windows clicking on the input or the plugin won't give them focus. But I'm guessing with your patch, on Mac, the plugin would still get focus.

(There is a related bug on Windows, which is that calling event.preventDefault() but not event.stopPropagation() still lets the plugin take focus, because our plugin focus code is listening for DOM events, not the frame event handler.)

So, I wonder if there is any approach to controlling plugin focus on Mac that would let us fix this case as well.
E.g. what if we always return false from becomeFirstResponder for plugin views, so that plugin views are never the first responder? Does that actually break plugins? Gecko should continue delivering keyboard and other events to the plugin if we think it's focused.

If we do need to make the plugin first-responder, can we return false from becomeFirstResponder and then later make the plugin view first-responder when our normal mouse event code decides to focus the plugin?
(In reply to comment #48)

> So, I wonder if there is any approach to controlling plugin focus on
> Mac that would let us fix this case as well.

I will try to find out.

(In reply to comment #49)

> E.g. what if we always return false from becomeFirstResponder for
> plugin views, so that plugin views are never the first responder?
> Does that actually break plugins?

This would change a basic assumption of Cocoa widgets code, and would
break any number of things in nsChildView.mm (and possibly elsewhere).

> If we do need to make the plugin first-responder, can we return
> false from becomeFirstResponder and then later make the plugin view
> first-responder when our normal mouse event code decides to focus
> the plugin?

This might work.  I'll look into it.

By the way, my life is a bit complicated this week because I have to
deal with a bad bug in the JEP.  But I should have *some* time to work
on this bug.

Comment 51

8 years ago
I'll try to help with this tomorrow but it's getting pretty late for Firefox 4, any patch here is going to be risky, and this isn't a hard blocker. Unless we can come up with a solution we're feeling pretty confident about by Monday we'll probably have to punt to the first update to Firefox 4.

Comment 52

8 years ago
I think this needs to hard block. It is 100% reproducible, it occurs on the YouTube front page. It's a terrible user experience, at best a user's action will result in nothing and at worst it invokes an unintended action.

Updated

8 years ago
blocking2.0: final+ → ?
Whiteboard: [softblocker]

Updated

8 years ago
Blocks: 601182
blocking2.0: ? → final+
Whiteboard: [hardblocker]

Comment 53

8 years ago
I think we should try to take Steven's existing patch. I think roc is right about the remaining issues, and that we don't want to do this in the long run, but for right now I think Steven's current patch has the lowest risk for the highest reward.

If we do this, we should remember to file a bug about replacing this with a better fix after Firefox 4.
If we take this as a temporary hack, who is going to fix it for real and when?
>> If we do need to make the plugin first-responder, can we return
>> false from becomeFirstResponder and then later make the plugin view
>> first-responder when our normal mouse event code decides to focus
>> the plugin?
>
> This might work.  I'll look into it.

I'd like to pursue this when I have the time (post FF4).
(Following up comment #55)

Of course the problem will go away when/if we get rid of plugin
widgets.  But doing that will be a lot of work (at least on the Mac).
In the current patch, it looks like we never clear mPluginContent. Seems like that could lead to use of a dangling pointer.

A safer way to do this might be to send a new kind of event, which includes the mouse position and the nsIWidget for the plugin. The event would return true if the mouse event at that position hits the nsObjectFrame whose widget is the given widget.
> In the current patch, it looks like we never clear mPluginContent.

Boris thought the same, but it's not true.  See comment #38.
> A safer way to do this might be to send a new kind of event, which
> includes the mouse position and the nsIWidget for the plugin. The
> event would return true if the mouse event at that position hits the
> nsObjectFrame whose widget is the given widget.

I should have time to do this tomorrow ... if you still think it's
necessary.
Ah OK. That's relying on mDestroyWidget always being true on Mac. If you can do the refactoring I suggested in comment #57 tomorrow, I think you should. Thanks!

Updated

8 years ago
Attachment #513707 - Flags: review?(roc)
Attachment #513707 - Flags: review?(joshmoz)
Created attachment 515816 [details] [diff] [review]
Fix rev2 (follow Roc's suggestion)
Attachment #513707 - Attachment is obsolete: true
Attachment #515816 - Flags: review?(roc)
Attachment #515816 - Flags: review?(joshmoz)

Updated

8 years ago
Whiteboard: [hardblocker] → [hardblocker][has patch]
+  nsIntPoint eventLoc =
+    aEvent->refPoint + aEvent->widget->WidgetToScreenOffset();
+  nsRect docFrameRect = docFrame->GetScreenRectInAppUnits();
+  eventLoc.x -= nsPresContext::AppUnitsToIntCSSPixels(docFrameRect.x);
+  eventLoc.y -= nsPresContext::AppUnitsToIntCSSPixels(docFrameRect.y);

You're mixing device pixels and CSS pixels here. I was actually hoping you'd go through PresShell::HandleEvent and use the mouse coordinate processing that happens there.
> I was actually hoping you'd go through PresShell::HandleEvent and
> use the mouse coordinate processing that happens there.

I'll look into it, but it'll probably take all day again.  These are
parts of the tree I'm not very familiar with.
> I'll look into it, but it'll probably take all day again.  These are
> parts of the tree I'm not very familiar with.

Actually, it might take me as long as a week.

I just looked at nsPresShell::HandleEvent(), and I can't figure out
where to intervene, or find any other event I might use as a model (as
I did with NS_QUERY_CHARACTER_AT_POINT for my rev2 patch).  So, if you
don't mind, I'm going to keep using the strategy of my rev2 patch.

>+  nsIntPoint eventLoc =
>+    aEvent->refPoint + aEvent->widget->WidgetToScreenOffset();
>+  nsRect docFrameRect = docFrame->GetScreenRectInAppUnits();
>+  eventLoc.x -= nsPresContext::AppUnitsToIntCSSPixels(docFrameRect.x);
>+  eventLoc.y -= nsPresContext::AppUnitsToIntCSSPixels(docFrameRect.y);
>
> You're mixing device pixels and CSS pixels here.

As Boris pointed out in comment #39,
nsIDocument::ElementFromPointHelper expects CSS pixels.  I'll try to
rewrite my patch to use something else.
Created attachment 515945 [details] [diff] [review]
Fix rev3 (fix various problems)

> As Boris pointed out in comment #39,
> nsIDocument::ElementFromPointHelper expects CSS pixels.  I'll try to
> rewrite my patch to use something else.

This is much better.

Note that I also fixed a couple of other problems:  I replaced
GetScreenRectInAppUnits() with GetScreenRect() (which returns CSS
pixels).  And I made sure targetFrame is null-checked before use.
Attachment #515816 - Attachment is obsolete: true
Attachment #515945 - Flags: review?(roc)
Attachment #515945 - Flags: review?(joshmoz)
Attachment #515816 - Flags: review?(roc)
Attachment #515816 - Flags: review?(joshmoz)
Comment on attachment 515945 [details] [diff] [review]
Fix rev3 (fix various problems)

Oops, stupid mistake.  Another patch coming up very shortly.
Attachment #515945 - Flags: review?(roc)
Attachment #515945 - Flags: review?(joshmoz)
Created attachment 515947 [details] [diff] [review]
Fix rev4 (fix another problem)
Attachment #515945 - Attachment is obsolete: true
Attachment #515947 - Flags: review?(roc)
Attachment #515947 - Flags: review?(joshmoz)
Comment on attachment 515947 [details] [diff] [review]
Fix rev4 (fix another problem)

Because you're not using the HandleEvent path, this patch will fail in at least the following circumstances:
-- If the plugin is in an IFRAME, and the parent document contains another IFRAME over the plugin's IFRAME, ElementFromPoint will report that the mouse is over the plugin when it's actually not. We'll focus the plugin when we shouldn't. (This is not a regression for this patch though.)
-- If mouse capturing is happening, events might be directed to the plugin while the plugin thinks the events are not over the plugin. This shouldn't cause any problems though since it's only mouse-down events we care about and we shouldn't be holding capture while the button is up.

So I think this is worth taking as-is.
Attachment #515947 - Flags: review?(roc) → review+

Updated

8 years ago
Attachment #515947 - Flags: review?(joshmoz) → review+
Comment on attachment 515947 [details] [diff] [review]
Fix rev4 (fix another problem)

Landed on the trunk:
http://hg.mozilla.org/mozilla-central/rev/75082063f8e6
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Depends on: 639194
Depends on: 641057
(In reply to comment #55)
> >> If we do need to make the plugin first-responder, can we return
> >> false from becomeFirstResponder and then later make the plugin view
> >> first-responder when our normal mouse event code decides to focus
> >> the plugin?
> >
> > This might work.  I'll look into it.
> 
> I'd like to pursue this when I have the time (post FF4).

Do you have time yet? :-)
> Do you have time yet? :-)

I probably won't have time for at least as month -- what with work
I've got to do on the JEP (concerning a security bug and Lion
compatibility), and with further truly urgent FF4 bugs that are likely
to turn up without warning (like bug 629030 on the 1.9.2 branch).

It doesn't make sense to work on this until we're comfortably "post
FF4".  But I truly am interested in working on it, and I suspect
that once I get started, it might take me about a week.
Depends on: 650080
Depends on: 644110
Depends on: 649021
> Do you have time yet? :-)

I do now ... or at least I seem to for the moment.

I've opened bug 658550 to follow up on your suggestion.

Updated

7 years ago
Depends on: 677895
You need to log in before you can comment on or make changes to this bug.