672857 - Make IM working for IPC plugins in remote browser

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Oleg Romashin (:romaxa)]

In remote browser (fennec) it is a bit hard to extract XKeyEvents and pass to plugins.

I'm proposing to send nsGUIEvents key/text down to plugin process and then do some magic according to the platform implementation...
1) For Linux platform (desktop) we probably should do XKeyEvent generation from Key (gtk/qt mapping) and Text events (unicode -> Xsym mapping)
2) another approach which we can do for Qt plugins is just generate QKeyEvent or QInputMethod event and post it to plugin-process event loop (plugin will handle it as platform Key/Text events).

Comment 1

[Oleg Romashin (:romaxa)]

Attachment: Pass nsKey/Text event down to plugin-process

Similar to what has been done in bug 582644

Comment 2

[Oleg Romashin (:romaxa)]

Attachment: part 2: Quick impl, convert nsKey/Text to QKey/Text events and post to plugin process event loop

Comment 3

[Oleg Romashin (:romaxa)]

Have not started yet DomKey/Unicode ->XKey mapping table implementation, just wanted to ask if there are any existing table or similar table available?

Comment 4

[Oleg Romashin (:romaxa)]

For harmattan we can probably live with QKey/QInputMethodEvent(Text) trick in flash, but for desktop FF/plugins it make sense to do mapping table

Comment 5

[Oleg Romashin (:romaxa)]

Comment on attachment 547148 [details] [diff] [review]
Pass nsKey/Text event down to plugin-process

Still trying to figure out is it ok to pass DOMEvents down to plugin process and implement platform specific handling inside plugin process?

Comment 6

[Karl Tomlinson (:karlt)]

I don't really have a good feel for the best solution here, but I fear that DomKey/Unicode -> XKey is not going to work very well, particularly if an IME was required to generate the text.

It may be worth considering a plugin side solution.
Perhaps either:

1) The plugin listens for dom events, or

2) When the plugin receives focus, it notifies the IME and registers for events
   directly from the IME (perhaps via the toolkit library).

Comment 7

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

I'm not familiar with Qt's IM specification. However, if IM events need to some results, e.g., cursor position or surround text information, it's impossible that plugins return correct results due to async connection.

E.g., on GTK, I think that the plugins should create its owning IMContext and use it. See bug 674456. Of course, this approach isn't good for DOM Events specification, e.g., Web application cannot listen DOM composition events on plugins. However, I think that this should be allowed since it's a limitation by platform.

Comment 8

[Oleg Romashin (:romaxa)]

we have two options here:
1) store native toolkit events and send it across processes.
2) current approach - Convert dom-key events into platform key events (QKeyEvents), and for text just send platform text event directly to plugin (QInputMethodEvent)

Not sure about full IME implementation, for that we probably should go with 1) path, and plugin can listen all native IM events (composition/cursor position, pre-edit/commit et.c.) but in that case we anyway should send over IPC 2 times all native IME events...


But I think it would be better just pass DOM events down to plugin process and deal with platformization only in plugin-process instead of hacking Fennec UI keyModule, or serializing platform events via TabParent/Child PluginParent/Child

Comment 9

[Oleg Romashin (:romaxa)]

Attachment: part 1: Pass TextEvent API down to plugin process

My suggestion is to make basic typing into plugins works first...
Not sure how it is possible to handle all composition tricks with closed source plugins... and how many people are really interested in that...
Possibly we can expose more API's later and do IME surrounding/cursor position properly in there...

Comment 10

[Doug Turner (:dougt)]

Comment on attachment 553602 [details] [diff] [review]
part 1: Pass TextEvent API down to plugin process

josh, could you take a look here?

Comment 11

[Oleg Romashin (:romaxa)]

> 2) When the plugin receives focus, it notifies the IME and registers for
> events
>    directly from the IME (perhaps via the toolkit library).
Plugins cannot listen UI process IME events directly, so that is why this patch send events over IPC and generate platform toolkit events to plugin in plugin process

Comment 12

[Oleg Romashin (:romaxa)]

Can we get some review comments here?

Comment 13

[Karl Tomlinson (:karlt)]

I'm not able to do a quick review here.
I don't know much about the issues involved.

The patches deal with both Key events and Text events.
Which does the virtual keyboard usually produce?

Are the NS_KEY_EVENT events missing QKeyEvent info because this hasn't been passed from UI to Content process or is there something else making generation of XKeyEvents difficult?

Comment 14

[Karl Tomlinson (:karlt)]

(In reply to Karl Tomlinson (:karlt) from comment #13)
> The patches deal with both Key events and Text events.
> Which does the virtual keyboard usually produce?

Oh, I had the answer to this one via private email:
"mostly Qt-TextEvents + Qt-Key events for backspace, enter [etc.]"

Comment 15

[Karl Tomlinson (:karlt)]

Masayuki, do you know how well mapping NS_KEY_EVENT and NS_TEXT_EVENT to Qt events and dispatching to the event loop is going to work?

Comment 16

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

Um, I want to know the answer for:

> Are the NS_KEY_EVENT events missing QKeyEvent info because this hasn't been passed from UI to Content process

Basically, we should pass all native events to plugin as far as possible. So, if we didn't pass Qt events now, why we didn't do so?

Comment 17

[Oleg Romashin (:romaxa)]

Problem is that with current mobile browser we are using http://mxr.mozilla.org/mozilla-central/source/mobile/chrome/content/browser.js#1976  API for key events which does not have any native events in place... it could be solved by killing ContentCustomKeySender from mobile UI, but I'm not sure if there are any problem with that

Comment 18

[Oleg Romashin (:romaxa)]

another possible option is to create additional parameter to SendKeyEvent, like aNativeKeyCode, expose it in private KeyEvent allow fennec to read that and forward to RemoteBrowser... and in plugin code we can generate XEvent using that nativeKeyCode...

We have patch long time ago with that... but that did not go anywhere...

we do have currently Content nsKeyEvents forwarding, and if fennec disable own keyForwarder, then we could probably serialize native platform event and pass it down as it is to plugins...

Comment 19

[Karl Tomlinson (:karlt)]

I wonder whether using a QKeyEvent in sending NS_KEY_EVENTs may actually make things worse by increasing the likelihood of the event loop dispatch causing NS_TEXT_EVENTs to get out of order with related events.

Comment 20

[Oleg Romashin (:romaxa)]

For text events I don't see other option... conversion of TextEvents into XKeyEvents not gonna work. only if we do not do some hooks and not creating hacked XKeyEvent with Text char* stored somewhere into serial param...

Any other suggestions? or is this the way to go?

Comment 21

[Oleg Romashin (:romaxa)]

also if we do this such kind of hack, then we should do it somewhere in plugin process in order to not create special XKeyEvent serialization with that hacked char* array...

so we can send NP_HandleEvent for key events if we generate XKeyEvent in TabParent and push it down to Plugin.
For text events we can send text event to plugin using hacked XKeyEvent, in order to not disconnect PluginEvent fron NS_TEXT_EVENT

Comment 22

[Oleg Romashin (:romaxa)]

ok, with current fennec CustomKeySender I believe current approach is the only way go.
As soon CustomKeySender removed from fennec we can do more nice implementation for NS_KEY_EVENT (by having native .pluginEvent)

Comment 23

[Karl Tomlinson (:karlt)]

(In reply to Oleg Romashin (:romaxa) from comment #11)
> > 2) When the plugin receives focus, it notifies the IME and registers for
> > events
> >    directly from the IME (perhaps via the toolkit library).
> Plugins cannot listen UI process IME events directly, so that is why this
> patch send events over IPC and generate platform toolkit events to plugin in
> plugin process

I understand that the plugin can't listen to events directly from the browser's IME connection.

Can you explain for me please why the plugin can't launch its own IME/VKB directly, instead of asking the browser to start its IME (and relay events back to the plugin)?

Comment 24

[Karl Tomlinson (:karlt)]

(In reply to Masayuki Nakano (Mozilla Japan) from comment #7)
> E.g., on GTK, I think that the plugins should create its owning IMContext
> and use it. See bug 674456.

Plugins could register an event listener on the plugin element for the DOM text event.  Why do you think it is better than plugins use their own IMContext?
Is it because DOM methods do not provide enough control over the IME?

Comment 25

[Oleg Romashin (:romaxa)]

Because in order to open VKB and initialize IME context you should have Qwidget, create Meego Input context and provide focused widget to that context... otherwise it is not possible to get VKB working... I guess it is common that VKB and IME want to have platform focused widget (remember Gtk Meaemo5) also android does not allow to initialize IME without active JavaAppshell window...

Comment 26

[Karl Tomlinson (:karlt)]

Sorry, in comment 24, I meant "Why do you think it is better that plugins use their own IMContext?"

Comment 27

[Oleg Romashin (:romaxa)]

Attachment: Text/Key events for Qt maemo6 IPC plugins

Comment 28

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

(In reply to Karl Tomlinson (:karlt) from comment #23)
> (In reply to Oleg Romashin (:romaxa) from comment #11)
> > > 2) When the plugin receives focus, it notifies the IME and registers for
> > > events
> > >    directly from the IME (perhaps via the toolkit library).
> > Plugins cannot listen UI process IME events directly, so that is why this
> > patch send events over IPC and generate platform toolkit events to plugin in
> > plugin process
> 
> I understand that the plugin can't listen to events directly from the
> browser's IME connection.
> 
> Can you explain for me please why the plugin can't launch its own IME/VKB
> directly, instead of asking the browser to start its IME (and relay events
> back to the plugin)?

Ah, I might be wrong. I didn't think about the wall of processes. On GTK, cannot a process which doesn't have focused widget set focus to its owning IM context? If the plugin process did it, it could handle IM signals itself.

The GTK's IM issue is difficult because there are no GDK events for IM. There are only signals. Therefore, we cannot send native events for GTK plugin.

Comment 29

[Karl Tomlinson (:karlt)]

(In reply to Masayuki Nakano (Mozilla Japan) from comment #28)
> On GTK,
> cannot a process which doesn't have focused widget set focus to its owning
> IM context?

I don't know, but I could imagine that not working well, at least for some IMEs, if they don't open their own window and take focus, but want keyboard input.

> The GTK's IM issue is difficult because there are no GDK events for IM.
> There are only signals. Therefore, we cannot send native events for GTK
> plugin.

Signals may in fact be better than events, if we want to take this path of proxying text messages, because signals are synchronous.  We could dispatch a GTK signal if we can find the right object, but it could all get very complicated.

Comment 30

[Karl Tomlinson (:karlt)]

Comment on attachment 555890 [details] [diff] [review]
Text/Key events  for Qt maemo6 IPC plugins

I'm OK with this provided we recognize that it is a temporary solution for a
specific class of plugins.  Long term the solution might be to dispatch the
DOM event to the plugin.  Perhaps npruntime can be used for that (from the
plugin side, with browser-side modifications to make sure the events propagate
correctly), but if that can't deal with necessary trusted event issues, then
perhaps https://wiki.mozilla.org/NPAPI:ExtendedXEventModel can provide a way
to dispatch DOM events.

>+    if (privateEvent) {
>+      nsTextEvent *textEvent = (nsTextEvent *) privateEvent->GetInternalNSEvent();
>+      if (textEvent) {
>+        nsEventStatus rv = ProcessEvent(*textEvent);
>+        if (nsEventStatus_eConsumeNoDefault == rv) {
>+          aTextEvent->PreventDefault();
>+          aTextEvent->StopPropagation();
>+        }
>+      }

Other methods are checking event->eventStructType for the nsEvent* from
GetInternalNSEvent, so safest to follow that style.  Then static_cast to
nsGUIEvent* for the ProcessEvent call.

>-          NS_WARNING("Synthesized key event not sent to plugin");
>+          // NS_WARNING("Synthesized key event not sent to plugin");
>+#if defined(MOZ_WIDGET_QT) && (MOZ_PLATFORM_MAEMO == 6)
>+          mInstance->HandleGUIEvent(anEvent);
>+#endif
>         }
>       break;
>-
>+#if defined(MOZ_WIDGET_QT) && (MOZ_PLATFORM_MAEMO == 6)
>+   case NS_TEXT_EVENT:
>+        {
>+          mInstance->HandleGUIEvent(anEvent);

Leave the NS_WARNING for other platforms.

These leave ProcessEvent returning nsEventStatus_eIgnore but I think we can
actually get this right.  See below.

>+#if defined(MOZ_WIDGET_QT) && (MOZ_PLATFORM_MAEMO == 6)
>+#include <QEvent>
>+#include <QKeyEvent>
>+#include <QApplication>
>+#include <QInputMethodEvent>
>+#include "nsQtKeyUtils.h"
>+# undef KeyPress
>+# undef KeyRelease
>+#endif

I would normally be in favour of indentation, but I think it's better to leave
out the space for consistency with the neighbouring lines.  Similarly above.

> #elif defined(MOZ_WIDGET_QT)
> #include <QX11Info>
>+#include "nsQtKeyUtils.h"
>+# undef KeyPress
>+# undef KeyRelease

This is repeated.  I assume that is not necessary.

>+    if (aKeyEvent.message == NS_KEY_DOWN) {
>+        type = QEvent::KeyPress;
>+    } else if (aKeyEvent.message == NS_KEY_UP) {
>+        type = QEvent::KeyRelease;
>+    } else {
>+        return false;

Doesn't returning false indicate that an error happened?
It wouldn't be an error if an NS_KEY_PRESS was received here would it?

>+    QApplication::sendEvent(qApp, &keyEv);

According to sendEvent documentation, this doesn't post the event to the event
loop but sends the event directly.  That is good.

I assume that means it's easy enough to check the return code for deciding
whether to stop propagation, so I think that should be done...

>+  async HandleTextEvent(nsTextEvent event);
>+  async HandleKeyEvent(nsKeyEvent event);

... which would make these rpc, which would be more consistent with the
existing HandleEvent.

(Or does the qApp post the event to the event loop?  I'm a little surprised it
even knows which object needs the event.)

>+    QInputMethodEvent *event = new QInputMethodEvent();
>+    event->setCommitString(QString((const QChar *)aEvent.theText.get(), aEvent.theText.Length()));
>+    QApplication::sendEvent(qApp, event);
>+    delete (event);

Probably similarly here I guess.

Can the QInputMethodEvent be on the stack (like QKeyEvent above)?

There's a line over 80 characters.

> class nsKeyEvent : public nsInputEvent
> {
> private:
>   friend class mozilla::dom::PBrowserParent;
>   friend class mozilla::dom::PBrowserChild;
>+  friend class mozilla::plugins::PPluginInstanceChild;
> 
> public:
>   nsKeyEvent()
>   {
>   }
>@@ -1120,10 +1124,11 @@ typedef nsTextRange* nsTextRangeArray;
> class nsTextEvent : public nsInputEvent
> {
> private:
>   friend class mozilla::dom::PBrowserParent;
>   friend class mozilla::dom::PBrowserChild;
>+  friend class mozilla::plugins::PPluginInstanceChild;

Can you explain the need for friend classes, please?

Comment 31

[Oleg Romashin (:romaxa)]

Attachment: Text/Key events for Qt maemo6 IPC plugins

> Can you explain the need for friend classes, please?
in order to make TextEvent() private ctor visible to ipdl generated classes

Comment 32

[Karl Tomlinson (:karlt)]

Comment on attachment 556084 [details] [diff] [review]
Text/Key events for Qt maemo6 IPC plugins

Can you change the type of the bool& "handled" out-parameters to bool*,
please?  It's a convention we use to make it clearer which are the
out-parameters.

>+          bool handled = false;
>+          if (NS_SUCCEEDED(mInstance->HandleGUIEvent(anEvent, handled)) &&
>+              handled) {

It shouldn't be necessary to initialize out-parameters if, like here and
below, the result is used only when the method succeeds.

nsNPAPIPluginInstance::HandleGUIEvent will need to set *handled to false when
it returns early with NS_OK.

>+    if (aKeyEvent.message == NS_KEY_DOWN) {
>+        type = QEvent::KeyPress;
>+    } else if (aKeyEvent.message == NS_KEY_UP) {
>+        type = QEvent::KeyRelease;
>+    } else {
>+        return true;

Set "*handled = false" on successful return here.

The rest looks fine, thanks.

Comment 33

[Oleg Romashin (:romaxa)]

Attachment: Text/Key events for Qt maemo6 IPC plugins

Comment 34

[Karl Tomlinson (:karlt)]

> nsNPAPIPluginInstance::HandleGUIEvent will need to set *handled to false when
> it returns early with NS_OK.

Missed this.

Comment 35

[Oleg Romashin (:romaxa)]

Attachment: Text/Key events for Qt maemo6 IPC plugins

Ough. fixed

Comment 36

[Ed Morley [:emorley]]

In my queue with a few other bits that are being sent to try first and then onto inbound.

Comment 37

[Ed Morley [:emorley]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/4fc1f5dd4105

Comment 38

[Ed Morley [:emorley]]

http://hg.mozilla.org/mozilla-central/rev/4fc1f5dd4105