586656 - [e10s] Support windowless plugin key event handling

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Steffen Imhof]

To feed the NPAPI the nsWindow implementation store some native key event data on the nsGUIEvent.

With e10s this native data gets lost and never reaches the nsObjectFrame thus disabling the key events for windowless plugins.

I observed the behaviour with a Qt/Fennec setup, but it should break in the same manner for at least GTK (dunno about Windows and Mac).

Comment 1

[Florian Hänel [:heeen]]

I'll look into this.

Comment 2

[Florian Hänel [:heeen]]

Still working on this. So far lower and upper case letters work. Special keys and symbols still need investigation.
The idea is to *not* use the void* pluginEvent of nsKeyEvent, since this doesn't make it through ipc, but to use the provided state and key information instead.

Comment 3

[Florian Hänel [:heeen]]

Attachment: WIP

This is a work in progress. On the platform I tested it with it at least works for latin characters, shift, control and some punctuation. This should be tested on a desktop build, I guess.

Comment 4

[Florian Hänel [:heeen]]

Some more insights: I notice non-US keyboard layouts seem to be broken in respect to key events on not only mozilla, but also chromium and opera. On the German keyboard # and 3 are two different keys, yet pressing them yields the same keycode.
Lacking the native event, we can only synthesize a new event from the keycode, making it impossible to distinguish 3 and #.

I feel like using the charcode will be the best plan of action, but for some reason it is nulled in keyup/down events, where the keycode is nulled in keypress events. According to http://www.quirksmode.org/js/keys.html we are wrong in nulling charcode on up/down events anyways.

Comment 5

[Florian Hänel [:heeen]]

Addendum: the above observations hold on linux. On windows firefox, safari and chrome generate different keycodes for # (191) and 3 (51).

Comment 6

[Florian Hänel [:heeen]]

Attachment: WIP

Comment 7

[Benjamin Smedberg]

Could you be more specific about what we're doing differently? There should be no differences at all in the nsWindow -> nsGUIEvent -> nsObjectFrame code for e10s or not.

Comment 8

[Florian Hänel [:heeen]]

nsKeyEvent contains a 
void* pluginEvent
which contains the native Gdk/Qt/Windows?/Mac? Event as received by nsWindow. This is used to get to the native keycode to generate the nsapi event.
e10s events go through javascript which is where the void* gets lost.

Comment 9

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

heeen and I discussed on IRC.

I think we should provide a way to duplicate a DOM event from js.

I think we should create 2 methods for this:

nsIDOMEvent nsIDOMWindowUtils::duplicateEvent(nsIDOMEvent);
[noscript] nsIDOMEvent nsIDOMNSEvent::duplicate();

The former method just calls the latter method. It hides the method from content's script.

And the latter method is implemented on each nsDOM*Event. Then, we can dispatch a lossless event to content process.

How about you, smaug?

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

IMO this all should happen without any JS, and since the event contains
platform specific and event non-DOM specific data, couldn't we just
duplicate nsKeyEvent and send it to the right process?

Comment 11

[Florian Hänel [:heeen]]

Attachment: fennec part - cedar/mobile2 branch

Here's the fennec part for adding nativeKeyCodes to key events. I spent a day trying to figure out how to send the event directly to the right process instead of through js, but alas I couldn't find a solution. So for this patch, the event still goes through js as before.

In the nativeKeyCode I store the keycode as received by X11/the platform.

Comment 12

[Florian Hänel [:heeen]]

Attachment: mozilla part - cedar/mobile2 branch

mozilla part

Comment 13

[Oleg Romashin (:romaxa)]

>  I spent a day
> trying to figure out how to send the event directly to the right process
> instead of through js, but alas I couldn't find a solution.
you should register  DOMEvent  "keypress/keydown/keyrelease" listener in in TabParent, catch events, extract nsKeyEvent, serialize it (see example probably NPWindow serialization). Send it to TabChild (probably better to create new IPC method)
On tab child you just do whatever you do in existing patch

Comment 14

[Florian Hänel [:heeen]]

Attachment: not through js anymore

Comment 15

[Florian Hänel [:heeen]]

Attachment: not through js anymore

Changed as per romaxa's request, not routing key events through js anymore.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 476817 [details] [diff] [review]
not through js anymore

Just few random comments.


>@@ -261,16 +261,17 @@ interface nsIDOMWindowUtils : nsISupport
You must update iid when modifying an interface

>    *
>    * @return false if the event had preventDefault() called on it,
>    *               true otherwise.  In other words, true if and only if the
>    *               default action was taken.
>    */
>   boolean sendKeyEvent(in AString aType,
>                        in long aKeyCode,
>                        in long aCharCode,
>+                       in long aNativeKeyCodes,
>                        in long aModifiers,
>                        [optional] in boolean aPreventDefault);
This change breaks probably all the key event handling tests.
You could add the new parameter as an optional parameter after
aPreventDefault.

>@@ -179,25 +179,27 @@ interface nsIDOMKeyEvent : nsIDOMUIEvent
Update iid

>   const unsigned long DOM_VK_BACK_SLASH     = 0xDC;
>   const unsigned long DOM_VK_CLOSE_BRACKET  = 0xDD;
>   const unsigned long DOM_VK_QUOTE          = 0xDE;
> 
>   const unsigned long DOM_VK_META           = 0xE0;
> 
>   readonly attribute unsigned long    charCode;
>   readonly attribute unsigned long    keyCode;
>+  readonly attribute unsigned long    nativeKeyCode;
We certainly don't want to expose this to web content this way.
If this can't be [noscript], then at least mozNativeKeyCode

>   void                      initKeyEvent(in DOMString typeArg,
>                                          in boolean canBubbleArg,
>                                          in boolean cancelableArg,
>                                          in nsIDOMAbstractView viewArg,
>                                          in boolean ctrlKeyArg,
>                                          in boolean altKeyArg,
>                                          in boolean shiftKeyArg,
>                                          in boolean metaKeyArg,
>                                          in unsigned long keyCodeArg,
>-                                         in unsigned long charCodeArg);
>+                                         in unsigned long charCodeArg,
>+                                         in unsigned long nativeKeyCodeArg);
No way. This would break all sorts of web sites.
Use [optional] or add new mozInitKeyEvent or something.

>+nsresult
>+TabParent::HandleEvent(nsIDOMEvent *aEvent)
>+{
>+    nsString str_event;
>+    aEvent->GetType(str_event);
>+    if(str_event.EqualsLiteral("keydown")
>+            || str_event.EqualsLiteral("keypress")
>+            || str_event.EqualsLiteral("keyup"))
>+    {

Strange indentation. And || should be in the end of a line.
'{' shouldn't start a new line after 'if'



>+        nsCOMPtr<nsIDOMKeyEvent> keyEvent = do_QueryInterface(aEvent);
>+        PRUint32 modifiers=0;
Missing space before and after 0

>+        SendKeyEvent(
>+                str_event,
>+                keyCode,
>+                charCode,
>+                nativeKeyCode,
>+                modifiers,
>+                false //?? no prevent defaukt in nsIDOMKeyEvent
>+                );
Strange indentation

Comment 17

[Florian Hänel [:heeen]]

Attachment: moz part - incorporated smaug's suggestions

Comment 18

[Doug Turner (:dougt)]

Florian, when you want a review from someone, you must specify them in the reviewer field.  Otherwise, no one sees it.

Comment 19

[Doug Turner (:dougt)]

Comment on attachment 477104 [details] [diff] [review]
moz part - incorporated smaug's suggestions

maybe smaug?

Comment 20

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 476816 [details] [diff] [review]
not through js anymore

Ben - want your opinion here

Comment 21

[Benjamin Stover (:stechz)]

Can't argue with any of that!

I haven't looked to closely at how preventDefault or stopPropagation could possibly work... are keyboard shortcuts going to work after this fix?

Comment 22

[Florian Hänel [:heeen]]

Maybe it would be wiser to use the X SymKey to send through the dom and translating it back to a X keycode at the last possible step, so someone could make some sense from the value.

The regular keycode is broken by design: on a German keyboard " and 2 generate two different dom keycodes since on the english keyboard they are different keys, while 3 and # generate the same code since they are the same key, but are seperate keys on a German layout. Ä,Ö and Ü generate a zero keycode since they are nonexistant on a US Layout. Noone can make any sense from this mess.

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 477104 [details] [diff] [review]
moz part - incorporated smaug's suggestions

>diff --git a/content/base/public/nsIFrameLoader.idl b/content/base/public/nsIFrameLoader.idl
>--- a/content/base/public/nsIFrameLoader.idl
>+++ b/content/base/public/nsIFrameLoader.idl
>@@ -118,17 +118,18 @@ interface nsIFrameLoader : nsISupports

You change an interface, so update the iid.


>   void sendCrossProcessKeyEvent(in AString aType,
>                                 in long aKeyCode,
>                                 in long aCharCode,
>                                 in long aModifiers,
>-                                [optional] in boolean aPreventDefault);
>+                                [optional] in boolean aPreventDefault,
>+                                [optional] in long aNativeKeyCode);

> NS_IMETHODIMP
> nsFrameLoader::SendCrossProcessKeyEvent(const nsAString& aType,
>                                         PRInt32 aKeyCode,
>                                         PRInt32 aCharCode,
>+                                        PRInt32 aNativeKeyCode,
>                                         PRInt32 aModifiers,
>                                         PRBool aPreventDefault)
Huh, I assume this compiles just because PRBool is internally PRInt32 or something.
In the .idl you're adding new parameter as the last parameter, here you're doing something else.
I assume this all wasn't tested properly.

>+nsDOMKeyboardEvent::GetMozNativeKeyCode(PRUint32* aKeyCode)
>+{
>+  NS_ENSURE_ARG_POINTER(aKeyCode);
>+  switch (mEvent->message) {
>+  case NS_KEY_UP:
>+  case NS_KEY_PRESS:
>+  case NS_KEY_DOWN:
>+    *aKeyCode = ((nsKeyEvent*)mEvent)->nativeKeyCode;
>+    break;
>+  default:
>+    ReportWrongPropertyAccessWarning("nativeKeyCode");
I think this is not really needed.
mozNativeKeyCode isn't anything for web developers, I hope.

> NS_IMETHODIMP
> nsDOMKeyboardEvent::InitKeyEvent(const nsAString& aType, PRBool aCanBubble, PRBool aCancelable,
>                                  nsIDOMAbstractView* aView, PRBool aCtrlKey, PRBool aAltKey,
>                                  PRBool aShiftKey, PRBool aMetaKey,
>-                                 PRUint32 aKeyCode, PRUint32 aCharCode)
>+                                 PRUint32 aKeyCode, PRUint32 aCharCode, PRUint32 aNativeKeyCode)
Actually, I think I'd prefer a new Init method for this. Similar to what MouseEvent has.



> NS_IMETHODIMP
> nsDOMWindowUtils::SendKeyEvent(const nsAString& aType,
>                                PRInt32 aKeyCode,
>                                PRInt32 aCharCode,
>                                PRInt32 aModifiers,
>                                PRBool aPreventDefault,
>+                               PRInt32 aNativeKeyCode,
>                                PRBool* aDefaultActionTaken)
> {
>   if (!IsUniversalXPConnectCapable()) {
>     return NS_ERROR_DOM_SECURITY_ERR;
>   }
> 
>   // get the widget to send the event to
>   nsCOMPtr<nsIWidget> widget = GetWidget();

>@@ -262,17 +262,18 @@ interface nsIDOMWindowUtils : nsISupport
>    * @return false if the event had preventDefault() called on it,
>    *               true otherwise.  In other words, true if and only if the
>    *               default action was taken.
>    */
>   boolean sendKeyEvent(in AString aType,
>                        in long aKeyCode,
>                        in long aCharCode,
>                        in long aModifiers,
>-                       [optional] in boolean aPreventDefault);
>+                       [optional] in boolean aPreventDefault,
>+                       [optional] in long aNativeKeyCodes);

Again, .idl attributes don't map to .cpp method properly 

>   void                      initKeyEvent(in DOMString typeArg,
>                                          in boolean canBubbleArg,
>                                          in boolean cancelableArg,
>                                          in nsIDOMAbstractView viewArg,
>                                          in boolean ctrlKeyArg,
>                                          in boolean altKeyArg,
>                                          in boolean shiftKeyArg,
>                                          in boolean metaKeyArg,
>                                          in unsigned long keyCodeArg,
>-                                         in unsigned long charCodeArg);
>+                                         in unsigned long charCodeArg,
>+                                         [optional] in unsigned long mozNativeKeyCodeArg);

So if you could add a new method. And if possible, make it [noscript]
Also, could .mozNativeKeyCode be [noscript]?

> TabParent::TabParent()
>   : mSecurityState(nsIWebProgressListener::STATE_IS_INSECURE)
> {
>+  nsCOMPtr<nsIWindowWatcher> ww = do_GetService(NS_WINDOWWATCHER_CONTRACTID);
>+  nsCOMPtr<nsIDOMWindow> window;
>+  ww->GetActiveWindow( getter_AddRefs(window) );
>+  nsCOMPtr<nsIDOMEventTarget> target=do_QueryInterface(window);
So you add event listeners to somewhat random window?!?
And why window? What if focus is in the location bar. Do you really want to send
event to content process?


>+TabParent::HandleEvent(nsIDOMEvent *aEvent)
>+{
>+    nsString str_event;
nsAutoString eventType;
or something like that. using '_' in the variable names isn't common.

>+                     false); //?? no prevent defaukt in nsIDOMKeyEvent
'defaukt' -> 'default'


>+QtKeyCodeToXKeySym(int qtkey)
So QT doesn't provide this mapping anywhere?

>+static bool sAltGrModifier=false; //quick fix for pmo 189373
space before and after =

>+    if(sAltGrModifier) {
>+        aEvent.isControl=PR_TRUE;
>+        aEvent.isAlt=PR_TRUE;
>+    }
ditto

Someone more familiar with QT key events should look at this too.

Comment 24

[Benjamin Stover (:stechz)]

Olli,

How close is this platform patch to what we want in https://bugzilla.mozilla.org/show_bug.cgi?id=583976?

Comment 26

[Mike Conley (:mconley) (:⚙️)]

Yoink.

Comment 28

[Jim Mathies [:jimm]]

Attachment: test cases.zip

for bjacob - windowed and windowless flash test cases, including a text input applet.

Comment 29

[Tom S [:evilpie]]

Attachment: wip

This recreates XKeyEvent.keycode based on the mCodeNameIndex, which from my understanding isn't always possible. I will probably have to add a field like mNativeKeyCode to WidgetKeyboardEvent and handle that in ipdl.

Comment 30

[Benoit Jacob [:bjacob] (mostly away)]

Thanks for the patch. Somehow, it doesn't apply on current mozilla-central. If you could elaborate in some more detail on the multiple things that this patch is changing, that would be very interesting.

Comment 31

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: WIP: properly serialize key events, GTK-only strawman for now

Evilpie: sorry about any duplication of effort; we had sort of informally assigned this bug to me during the last e10s meeting, and I forgot to update the assignee field. We want this fixed asap though, so it doesn't hurt to have more hands on it! Especially as I'm a n00b in this area.

This fixes the bug for me on linux/GTK: Jim's above "editor - windowless.html" works in an e10s tab, with this patch.

The problem was that in the child process we were hitting this warning:

http://hg.mozilla.org/mozilla-central/file/161fadfcbadc/dom/plugins/base/nsPluginInstanceOwner.cpp#l2162

That was because anEvent.pluginEvent was null. That field is how we pass the fields of a GdkEventKey (https://developer.gnome.org/gdk3/stable/gdk3-Event-Structures.html#GdkEventKey) that are not reflected by any field in WidgetKeyboardEvent (http://hg.mozilla.org/mozilla-central/file/161fadfcbadc/widget/TextEvents.h#l68).

Indeed, see this code and comment in the place where we encode this event info on GTK:

http://hg.mozilla.org/mozilla-central/file/161fadfcbadc/widget/gtk/nsGtkKeyUtils.cpp#l1004

1004 // The transformations above and in gdk for the keyval are not invertible
1005 // so link to the GdkEvent (which will vanish soon after return from the
1006 // event callback) to give plugins access to hardware_keycode and state.
1007 // (An XEvent would be nice but the GdkEvent is good enough.)
1008 aKeyEvent.pluginEvent = (void *)aGdkKeyEvent;

Indeed, in the code that we fail to execute (for lack of pluginEvent serialization) on deserialization on the child side, we do try to use that pluginEvent field to retrieve the hardware_keycode, state and also "is_modifier" field of the GdkEventKey:

http://hg.mozilla.org/mozilla-central/file/161fadfcbadc/dom/plugins/base/nsPluginInstanceOwner.cpp#l2120

Thus it seems that as far as GTK is concerned, we can solve our problems by dropping the hack that consists in using the pluginEvent field to pass the GdkEventKey, and instead just add these 3 fields from GdkEvent to WidgetKeyboardEvent: hardware_keycode, state and is_modifier.

The attached patch does that. It adds a "guard" boolean field, mHasExtraTastyBits, to tell whether these 3 extra fields are present. The silly name was intentionally chosen so that we would feel compelled to think of a better name.

If this looks reasonable, then I'll look at what fields need to be serialized on Windows, and hopefully we'll be able to settle on a small number of extra fields to add to WidgetKeyboardEvent to be able to cover all platforms.

Comment 32

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: WIP: properly serialize key events, GTK-only strawman for now

Was not properly recording GdkEventKey::is_modifier in KeymapWrapper::InitKeyEvent.

Comment 33

[Tom S [:evilpie]]

Attachment: plugin-key-event

Nice work! I actually have an other patch that takes almost the same approach. The patch looks pretty good to me, I just wouldn't do that conditional serialization. I would also put these fields in some native data struct. You also correctly have mIsModifier, which I haven't.

Comment 34

[Jim Mathies [:jimm]]

Comment on attachment 8455032 [details] [diff] [review]
WIP: properly serialize key events, GTK-only strawman for now

Code looks ok, for the event changes I'm interested in getting masayuki's feedback.

Comment 35

[Jim Mathies [:jimm]]

I think we'll probably want to isolate those platform specific fields somehow, I don't think we want those exposed to the rest of the code base. Maybe we should wrap those in some sort of "pluginEvent" type struct within WidgetKeyboardEvent?

Comment 36

[Benoit Jacob [:bjacob] (mostly away)]

Thanks for the feedback. Meanwhile, I'm setting up a Windows dev env on my new laptop, and trying to fix the issue there (I can already confirm that this reproduces). Once we know what the Windows-specific patch would look like, we can think of how to combine that with the above Linux-specific patch and get the cross-platformness right.

Comment 37

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

Comment on attachment 8455032 [details] [diff] [review]
WIP: properly serialize key events, GTK-only strawman for now

First, why don't you use mNativeKeyEvent? I think that it's safe to copy its data across process boundary. So, I think that you don't need to add additional members to WidgetKeyboardEvent.

Anyway, I comment my suggestions below:

>diff --git a/widget/gtk/nsGtkKeyUtils.cpp b/widget/gtk/nsGtkKeyUtils.cpp
>--- a/widget/gtk/nsGtkKeyUtils.cpp
>+++ b/widget/gtk/nsGtkKeyUtils.cpp
>@@ -997,20 +997,22 @@ KeymapWrapper::InitKeyEvent(WidgetKeyboa
>          GetBoolName(aKeyEvent.IsShift()), GetBoolName(aKeyEvent.IsControl()),
>          GetBoolName(aKeyEvent.IsAlt()), GetBoolName(aKeyEvent.IsMeta())));
> 
>     if (aKeyEvent.message == NS_KEY_PRESS) {
>         keymapWrapper->InitKeypressEvent(aKeyEvent, aGdkKeyEvent);
>     }
> 
>     // The transformations above and in gdk for the keyval are not invertible
>-    // so link to the GdkEvent (which will vanish soon after return from the
>-    // event callback) to give plugins access to hardware_keycode and state.
>-    // (An XEvent would be nice but the GdkEvent is good enough.)
>-    aKeyEvent.pluginEvent = (void *)aGdkKeyEvent;
>+    // so write extra fields to give access to the hardware_keycode and state.
>+    aKeyEvent.mHasExtraTastyBits = true;

Why do we need this flag? Don't you set it in all key event dispatcher?

>+    aKeyEvent.mIsModifier = aGdkKeyEvent->is_modifier;

For emulating all key events strictly, we need this flag, indeed. However, is this really necessary in actual usage? Isn't it enough to check mKeyNameIndex value?

>+    aKeyEvent.mHardwareKeyCode = aGdkKeyEvent->hardware_keycode;

Why don't you use mCodeNameIndex? It does not cover all keys of unknown keyboards. However, I guess that current support level is enough for the most users (except Sun keyboard users, see bug 1020139). If it were not enough, we could add original values for specific keys with empty string as its code value. If you will use it on other platforms too, mScanCode sounds better to me.

>+    aKeyEvent.mState = aGdkKeyEvent->state;

Unfortunately, we need this at least for GTK. However, as far as possible. So, I believe that you should copy the data of mNativeKeyEvent.

>@@ -321,18 +327,27 @@ struct ParamTraits<mozilla::WidgetKeyboa
>         ReadParam(aMsg, aIter, &codeNameIndex) &&
>         ReadParam(aMsg, aIter, &aResult->mKeyValue) &&
>         ReadParam(aMsg, aIter, &aResult->mCodeValue) &&
>         ReadParam(aMsg, aIter, &aResult->keyCode) &&
>         ReadParam(aMsg, aIter, &aResult->charCode) &&
>         ReadParam(aMsg, aIter, &aResult->isChar) &&
>         ReadParam(aMsg, aIter, &aResult->mIsRepeat) &&
>         ReadParam(aMsg, aIter, &aResult->location) &&
>-        ReadParam(aMsg, aIter, &aResult->mUniqueId))
>+        ReadParam(aMsg, aIter, &aResult->mUniqueId) &&
>+        ReadParam(aMsg, aIter, &aResult->mHasExtraTastyBits) )

nit: extra space between ")"s.

>     {
>+      if (aResult->mHasExtraTastyBits) {
>+        if (!(ReadParam(aMsg, aIter, &aResult->mIsModifier) &&
>+              ReadParam(aMsg, aIter, &aResult->mHardwareKeyCode) &&
>+              ReadParam(aMsg, aIter, &aResult->mState)))
>+        {

nit: "{" should be the end of the previous line.

Comment 38

[Benoit Jacob [:bjacob] (mostly away)]

Many thanks for the feedback Masayuki.

Notice that the existing code that I touched, was not using mNativeKeyEvent, mCodeNameIndex or mKeyName. Instead it was using the pluginEvent pointer to reference the GdkEventKey directly. That's what I was starting from.

I do understand that the fields that you point out (mNativeKeyEvent, mCodeNameIndex, mKeyName) sound like fields that I should be using, instead of introducing new fields redundant with these. Patch coming. Though I'll first try to get something working on Windows.

Comment 39

[Tom S [:evilpie]]

Comment on attachment 8455032 [details] [diff] [review]
WIP: properly serialize key events, GTK-only strawman for now

I think it would be nice if we could recreate keycode and state with just the current information, specifically mCodeNameIndex (or what it's called). I however kind of doubt this is possible, because that conversion isn't loseless and there is a comment that hints at that.

Comment 40

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: WIP proof of concept, working on Windows and Linux

This new version handles Windows as well as Linux.

We had to propagate 3 fields from the Windows MSG struct: message, wParam and lParam. For this strawman patch, I just added new fields for that to WidgetKeyboardEvent. Could any of these fields reuse existing WidgetKeyboardEvent fields instead?

In widget/windows/KeyboardLayout.cpp, I had to remove the condition on mIMEState.mEnabled being PLUGIN, as that was not the case - instead, it was DISABLED. Should I worry about that?

Comment 41

[Jim Mathies [:jimm]]

Comment on attachment 8456520 [details] [diff] [review]
WIP proof of concept, working on Windows and Linux

Review of attachment 8456520 [details] [diff] [review]:
-----------------------------------------------------------------

Have you tested on OSX yet? Looks like we use that pluginEvent struct there as well.

::: dom/plugins/base/nsPluginInstanceOwner.cpp
@@ +2126,5 @@
>     //XXX case NS_MOUSE_SCROLL_EVENT: not received.
>  
> +   case NS_KEY_EVENT: {
> +      const WidgetKeyboardEvent& keyEvent = *anEvent.AsKeyboardEvent();
> +      if (keyEvent.mHasExtraTastyBits)

since this replaces a valid pointer check on 'pluginEvent', lets name mHasExtraTastyBits a bit better, how about something like mValidNativeEvent or mValidPluginEvent?

::: widget/TextEvents.h
@@ +156,5 @@
> +  uint16_t mHardwareKeyCode;
> +  uint32_t mState;
> +  uint32_t mWindowsMessage;
> +  uintptr_t mWindowsWParam;
> +  uintptr_t mWindowsLParam;

If we can't share these among platforms, we should ifdef this by platform and define only the fields we need so we don't have a bunch of loose platform event related variables hanging around that are unused. (note there's also a type different between wparam and lparam.).


typedef unsigned int    UINT;

typedef UINT_PTR            WPARAM;
typedef LONG_PTR            LPARAM;

typedef _W64 unsigned int UINT_PTR, *PUINT_PTR; // A 32-bit unsigned integer. 
typedef _W64 long LONG_PTR, *PLONG_PTR;   // A 32-bit signed integer.

::: widget/gtk/nsGtkKeyUtils.cpp
@@ +1005,5 @@
> +    // so write extra fields to give access to the hardware_keycode and state.
> +    aKeyEvent.mHasExtraTastyBits = true;
> +    aKeyEvent.mIsModifier = aGdkKeyEvent->is_modifier;
> +    aKeyEvent.mHardwareKeyCode = aGdkKeyEvent->hardware_keycode;
> +    aKeyEvent.mState = aGdkKeyEvent->state;

We also appear to be storing the raw gdk event in WidgetKeyboardEvent's mNativeKeyEvent. Can we get rid of mNativeKeyEvent and coalesce the native storage into a single set of variables?

::: widget/windows/KeyboardLayout.cpp
@@ -1094,5 @@
>    KeyboardLayout::NotifyIdleServiceOfUserActivity();
>  
> -  NPEvent pluginEvent;
> -  if (aMsgSentToPlugin &&
> -      mWidget->GetInputContext().mIMEState.mEnabled == IMEState::PLUGIN) {

Honestly I don't understand the IME check here either, masayuki would be a good reviewer for this change.

Comment 42

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Patch allowing to set focus on plugin on OSX e10s, allowing to reproduce on OSX

So I spent the day on OSX, but not even writing the OSX key events patch: at the moment, on OSX e10s, we can't even set the focus on a windowless plugin by clicking on it. So I couldn't even get to where I could reproduce the bug.

This patches fixes that for me, but I have no idea whether it's the right approach. What it does is handle NS_FOCUS_CONTENT (which was unhandled on OSX) and make it use some Cocoa Objective C call found in nsChildView.mm that sets the focus.

With that I can finally reproduce, so I'll be writing the OSX patch for key events tomorrow...

Comment 43

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Handle plugin focus events on Mac / e10s

Here is a much simpler patch to handle focus events on Mac, that naively looks like it might be ready for review already? Masayuki / Steven , I don't know who would be the right reviewer for this?

Comment 44

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

Comment on attachment 8458267 [details] [diff] [review]
Handle plugin focus events on Mac / e10s

I think that this should be reviewed by josh.

Comment 45

[Josh Aas]

Comment on attachment 8458267 [details] [diff] [review]
Handle plugin focus events on Mac / e10s

Review of attachment 8458267 [details] [diff] [review]:
-----------------------------------------------------------------

Definitely want Steven to review this.

Comment 46

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8458267 [details] [diff] [review]
Handle plugin focus events on Mac / e10s

I will take a look at this next week.

Comment 47

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Steven Michaud from comment #46)
> Comment on attachment 8458267 [details] [diff] [review]
> Handle plugin focus events on Mac / e10s
> 
> I will take a look at this next week.

Note that this patch doesn't work as intended; your help welcome to understand why.

This patch does visually give the focus to the plugin area, e.g. in Jimm's editor testcase it gives the focus with the blinking cursor.... BUT key events are still not directed to the plugin view. As such, I still can't get key events working in plugins on Mac, even with the platform-agnostic patch which I'm going to attach below.

Comment 48

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Turn the pluginEvent field into a buffer and serialize it

This time, here is a really cross-platform patch that really "should" fix this entire class of problems (not just in key events but all GUI events) and indeed it does fix this bug on Windows and on Linux. (On Mac, I still can't properly handle focus, as noted above, and also another patch seems to be needed at least, see next patch).

With this approach, we are not anymore looking at changing fields in WidgetKeyboardEvent. Instead, we embrace the existing approach of the pluginEvent generic field, and just make it serializable, so it works in e10s just as well as in non-e10s.

Please refer to the comment on the nested PluginEvent struct in WidgetGUIEvent.

Comment 49

[Jim Mathies [:jimm]]

Comment on attachment 8460197 [details] [diff] [review]
Turn the pluginEvent field into a buffer and serialize it

Review of attachment 8460197 [details] [diff] [review]:
-----------------------------------------------------------------

Overall looks good. Not a fan of the copying of local pluginEvents for set up, but since this is keyboard handling the overhead shouldn't be an issue.

::: dom/plugins/base/nsPluginInstanceOwner.cpp
@@ +1878,5 @@
>  
>    int16_t response = kNPEventNotHandled;
>    void* window = FixUpPluginWindow(ePluginPaintEnable);
>    if (window || (eventModel == NPEventModelCocoa)) {
> +    mInstance->HandleEvent(const_cast<NPCocoaEvent*>(event), &response, NS_PLUGIN_CALL_SAFE_TO_REENTER_GECKO);

nit - wrap me

Comment 50

[Benoit Jacob [:bjacob] (mostly away)]

Here is an update on my attempts to debug the focus issues on Mac.

I've added some logging and compared what happened between e10s and non-e10s.

Here is the basic difference:

In non-e10s (where things work), when I click on the plugin area, I get the becomeFirstResponder callback called (as defined in nsChildView.mm) on the plugin's view. That in turn generates the focus events.

In e10s (where things do not work), that becomeFirstResponder callback (in nsChildView.mm) is never called - neither for the plugin view nor for any other view, it seems.

I'm failing to understand that so I think I'll work on other stuff until someone with Mac widget knowledge can help me understand that.

Comment 51

[Steven Michaud [:smichaud] (Retired)]

I'm hoping to help Benoit with this later this week.  But it will involve hours of work, and I can't spare the time right now.

Comment 52

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

Comment on attachment 8460197 [details] [diff] [review]
Turn the pluginEvent field into a buffer and serialize it

Looks like that this patch (almost?) all of the users of WidgetGUIEvent::pluginEvent. So, this is a good change to rename it. Could you rename it to mPluginEvent?

And I think that mBuffer should be private and ParamTraits<WidgetGUIEvent> should be a friend of PluginEvent.

Although, looks like that PluginEvent struct isn't good name since it can work with anything, i.e., very general. However, I have no idea for any other points. So, It's okay to keep this here in this bug.

PluginEvent could have AsNPEvent(), AsNPCocoaEvent() and AsANPEvent() for making callers simpler. But I don't want them so strongly.

Comment 53

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #52)
> Comment on attachment 8460197 [details] [diff] [review]
> Turn the pluginEvent field into a buffer and serialize it
> 
> Looks like that this patch (almost?) all of the users of
> WidgetGUIEvent::pluginEvent. So, this is a good change to rename it. Could
> you rename it to mPluginEvent?

Done.

> 
> And I think that mBuffer should be private and ParamTraits<WidgetGUIEvent>
> should be a friend of PluginEvent.

Done.

> 
> Although, looks like that PluginEvent struct isn't good name since it can
> work with anything, i.e., very general. However, I have no idea for any
> other points. So, It's okay to keep this here in this bug.

OK, let me know if you can think of a better name. Kept the name PluginEvent for now.

> 
> PluginEvent could have AsNPEvent(), AsNPCocoaEvent() and AsANPEvent() for
> making callers simpler. But I don't want them so strongly.

Ideally, PluginEvent would remember of type of the data structure that was given to Copy(), and would only allow getting that type back. In that case, As*() would be a good name for the getter. For now though, PluginEvent does not remember any type information and only stores an untyped buffer; so it does not perform any type checking when getting the structure back; I would be afraid of exposing something so dangerous under an innocuous name like As*(). As long as it's unsafe, I'd rather have it feel like an unsafe pointer.

Comment 54

[Benoit Jacob [:bjacob] (mostly away)]

Landed the serialization patch, so we now have e10s plugin events working on Linux and Windows at least.

On Mac, they might well be working too, but the above-discussed focus bug prevents checking that. Leaving open until that is resolved.

https://hg.mozilla.org/integration/mozilla-inbound/rev/8581ea023552

Comment 55

[Benoit Jacob [:bjacob] (mostly away)]

Filed bug 1044182 for the mac focus issue.

Comment 56

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8581ea023552

Comment 57

[Benoit Jacob [:bjacob] (mostly away)]

Mac focus work is going on on bug 1044182; the original issue discussed here is fixed, so let's close this bug. Of course, until bug 1044182 is fixed, we can't confirm that it's actually fixed on Mac, but the fact that the same cross-platform patch did fix it on Linux and Windows makes it reasonable to default to assuming so.

Comment 58

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8458267 [details] [diff] [review]
Handle plugin focus events on Mac / e10s

I now think we need to deal with bug 1044182 before we can tackle this bug.