122501 - [viewpoint] keyboard events aren't passed to windowless plugin correctly

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect, P1)

Description

[Ari Berger]

From Bugzilla Helper:
User-Agent: Mozilla/4.0 (compatible; MSIE 5.01; Windows NT 5.0)
BuildID:    0.9.4 branch

1) I only get keyboard events when I the plugin's page is in a frame.
View the following URL:
http://cole.viewpoint.com/~compuserve/bluestreak_testad2/
We get the keyboard events (NPP_HandleEvent is called)
View the following:
http://cole.viewpoint.com/~compuserve/bluestreak_testad2/bluestreak.htm
(exact same content, just not viewed through index, which creates the frame)
No keyboard events.

2) Note that either way, you will not see the effects of typing.
The second problem is that we are getting the wrong info in wparam and lparam.
Here is MSDN:
wParam - Specifies the virtual-key code of the nonsystem key. 
lParam - Specifies the repeat count, scan code, extended-key flag, context 
code, previous key-state flag, and transition-state flag, as shown in the 
following table. 
Here is your code in nsWindow.cpp line 2420:
  pluginEvent.wParam = 0;
  pluginEvent.wParam |= (event.isShift) ? MK_SHIFT : 0;
  pluginEvent.wParam |= (event.isControl) ? MK_CONTROL : 0;
  pluginEvent.lParam = aVirtualCharCode;
Looks to me like lParam is geting virtual char info, and wParam is getting 
shift and control info. First of all, I think they are flipped.  Second, is the 
shift and control info in the same form that WM_KEYDOWN or WM_KEYUP expects?



Reproducible: Always

---
Peter's comment from 116392:
I'll take a look at the frameset problem, but I don't see it in my simple
testcase. My key events get to my windowless plugin without a frameset after I
give the plugin focus by clicking in it's area. Perhaps that's the problem?
---
I assume it is not just framesets that would allow the events to get through- 
probably other things would do- maybe div tags?
You should be able to reproduce the problem at the URL that I gave.
I did make sure to give the window the focus first.

I will try your patch for flipping lParam and wParam.

Comment 1

[Peter Lubczynski]

This bug is spun off from bug 116392. See this patch for problem #2:
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=66994

Problem #1 is a little bit more complicated. It appears that it's the margins on
the BODY tag which are causing the problem. Add this attribute to the BODY tag
and the problem goes away:
style="margin: 0em"

That's not a very good workaround. So, here's what I see in the debugger:

The event is dispatched through the views but never makes it to the plugin's
view because the key event's coordinates (0,0) aren't "inside" the CanvasFrame's
view when it has a margin:
http://lxr.mozilla.org/mozilla/source/view/src/nsView.cpp#336

Since the canvas frame's default origin is offset by 8 pixels but the key event
is at zero, the event is given the document to handle instead of the child view.

I'm not quite sure what can be done here. Anyone have any ideas? Do I need to
"target" the key event so its coordinates are that of my frame's? It looks like
there was some code commented out that did something like that a long time ago:
http://lxr.mozilla.org/mozilla/source/layout/html/base/src/nsPresShell.cpp#5839

Comment 2

[Peter Lubczynski]

Actually, looking at |nsViewManager::DispatchEvent| where it looks like we
target the event based on widget may be a problem with windowless plugins which
have no widget. :( The event is correctly sent to the document since that's the
widget that got the key event. Strange that it works when we have no margins.

Since my plugin has no widget, how can I get my key event? Could I simply attach
my nsIDOMKeyListener on the document instead of the plugin in the case of
windowless? Or is there a special kind of widget that can bet used with this
kind of plugin?

Comment 3

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

While I was working on the view manager event code recently I wondered why
non-mouse events are dispatched specifically to (0,0), but I was afraid to
change it :-). Presumably only mouse events should go through the
coordinate-based dispatch path, but who knows what might break if we changed it.

I think this is a focus problem. I don't really know how DOM/layout focus works
but I think somehow you should be telling nsEventStateManager to make your
plugin the focus so it will forward key events to you.

nsIViewManager::GrabKeyEvents lets you tell the view manager to send events
directly to a particular view, but no-one uses that at the moment so if I were
you I'd only use it as a last resort.

Comment 4

[Peter Lubczynski]

Attachment: possible patch? (listening to key events on the documents)

Here's a possible patch that seems to work for me in the debugger in getting
windowless plugins key events. It attaches an extra DOM key event listener to
the document. The only draw back is that the plugin now gets ALL the key events
of the document which I'm not quite sure is correct. This patch also flips
wparam and propigates lparam in widget code for key events. Ari, can you try
this out and let me know if I'm close?

I also noticed that my plugin isn't getting a DOM focus/blur event when there
is a margin on the document. This could be causing the problem with key events,
the content never gets the focus. But it still may be the same underlying
problem that windowless plugins don't have widgets. I'll continue to
investigate.

Comment 5

[Michael Dunn]

What is tthe status

Comment 6

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

Plugins certainly should not just steal all key events for the document. They
should participate in focus management with the rest of the document. Think
about what's going to happen if there are multiple plugins in a page, or they're
in a page with form controls.

Have you looked at the way form controls get focused? Maybe you can copy that.
Many form controls don't have widgets.

Comment 7

[Peter Lubczynski]

Attachment: have plugin set focus

Yeah, scratch that last patch. Ari, try this one, it's a little better.

So I compared form controls and noticed it was really a focus problem. I guess
since we don't have a widget, we need to set focus ourselves. In any case,
doing this at MouseDown time seems to fire all the listeners.

Comment 8

[Michael Dunn]

Ari, Does the patch work? And is this a stopper?

Comment 9

[Ari Berger]

The patch fixes this test case.  Tomorrow we will test it in a few more 
situations.  But it looks good.

Comment 10

[Peter Lubczynski]

Attachment: new patch

Ari let me know that I he was getting double key events in some cases. This was
because I made keypress events go through as well as keydown in my patch in bug
116392. NPAPI doesn't say anything about keypress, so they should just be
consumed.

Comment 11

[Peter Lubczynski]

please review

Comment 12

[Kevin McCluskey (gone)]

Comment on attachment 67479 [details] [diff] [review]
new patch

r=[e-mail]

Comment 13

[Patrick C. Beard]

Comment on attachment 67479 [details] [diff] [review]
new patch

sr=beard
Does this patch prevent JS type event handlers from getting keyboard eve ts if
a plugin consumes the events?

Comment 14

[Michael Dunn]

Plusing for 0.9.4

Comment 15

[Peter Lubczynski]

Yes, the 'onkeypress' handler appears to still fire correctly despite me
consuming it.

Patch in trunk.

Comment 16

[Michael Dunn]

Has this landed in 094 yet? Please add "fixed0.9.4" KW as expected.

Comment 17

[Peter Lubczynski]

fixed0.9.4

Comment 18

[shrirang khanzode]

testcase works ok on 0206 branch build. 

Comment 19

[shrirang khanzode]

hey, the layout on both the testcases is messed up  on the trunk (0306) . can
anyone confirm ? However, the keybd events work fine ( textfields seen
partially..butI can type in it) in both tests...

Comment 20

[Ari Berger]

I have been waiting for bug 118721 to get fixed before I start trying to figure 
out how we work against the trunk- that causes layout problems on most of our 
content.

Comment 21

[shrirang khanzode]

keyboard events do work great on this test now. verif with 0812 trunk.