571453 - [e10s] Virtual keyboard is not invoked for input fields in Qt fennec

Status: RESOLVED INVALID

(Core :: IPC, defect)

Description

[Sudarsana Nagineni]

Using e10s fennec(Qt port).

STEPS LEADING TO PROBLEM: 
1. Open e10s Fennec
2. Go to www.google.com page
3. Tap on search input field

Result:
Virtual keyboard is not getting invoked for input fields. 

Additional Info:
VKB is not getting invoked when SetIMEEnabled called from content process as there is no focused widget. 
(http://mxr.mozilla.org/mozilla-central/source/widget/src/qt/mozqwidget.cpp#281)

I guess we need to remote SetIMEEnabled call when it is called from the content process to fix this issue.

Comment 1

[Sudarsana Nagineni]

Attachment: Remote SetIMEEnabled call when it is called from content

Comment 2

[Doug Turner (:dougt)]

I think there must be more to remote than just this API, right?

Masayuki, do you know what other APIs that might have to be remoted?

Comment 3

[Brad Lassey [:blassey] (use needinfo?)]

is there any reason to have this ifdef'd to QT only?

Comment 4

[Doug Turner (:dougt)]

no.

Comment 5

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

Um, yes. And the patch looks bad for me.

I want to confirm a couple of things about the process model.

* Each content is on a content process's widget?
* Native (system) focus is never set to the content widget?
* Parent widget has focus and it dispatch the gecko's event to the content process? (i.e., native IME event handling is processed by parent process?)

Following comment depends on the above ideas.

I think that this approach will cause simple mistakes in very many points and make the IME related code more complex, besides, the patch needs more changes.

How about that each IME related API in nsIWidget calls parent process's API? Then, you will not create any regressions.

Probably, you need to redirect following APIs if Qt widget implemented:

* ResetInputState()
* SetIMEOpenState()
* GetIMEOpenState()
* SetIMEEnabled()
* GetIMEEnabled()
* CancelIMEComposition()
* OnIMEFocusChange()
* OnIMETextChange()
* OnIMESelectionChange()

Comment 6

[(inactive) Jim Chen [:jchen] [:darchons]]

On which platforms is this an issue? So far I've seen Qt and Android.

I think there are two general ways to remote IME calls: on the content side and on the widget side. This patch remotes one of the IME functions on the content side.

In theory, if we remote on the content side, everything will be transparent for widget and all platforms will automatically become e10s compatible. On the other hand, remoting on the widget side requires each platform to have its own code for remoting. That adds clutter and makes it easier to make mistakes.

However, from my understanding of e10s, on some platforms remoting might not be necessary if the child process can handle IME input by itself. In that case remoting on the content side would introduce unnecessary overhead and might cause regressions. Many platforms only implement a subset of the IME functions and remoting a function just for the other side to return NS_OK or NS_ERROR_NOT_IMPLEMENTED seems not very ideal.

Considering that, I think it would be worth it to remote IME calls on the widget side, for each platform that requires it.

Comment 7

[Sudarsana Nagineni]

I am not sure about the list of platforms which requires remote IME calls. In Qt this is really an issue and we need to post the following events through the parent process to show/hide the virtual keyboard.

show: QEvent::RequestSoftwareInputPanel
hide: QEvent::CloseSoftwareInputPanel 

I checked the list of IME calls implemented in Qt port and there are only two.

* SetIMEEnabled 
* OnIMEFocusChange

The patch remotes the SetIMEEnabled call and that fixes the problem of showing/hiding the virtual keyboard.

Remote part was added at content side to make it available for other platforms also, if it requires. But, I have not tested this on other platforms so I added ifdef to QT to make sure that it will not break IME functionality on other platforms.

IME interactions generally involve a user, and the user will be slower than the remoting overhead, so having each widget implementation with its own set of remoting behaviors seems like eagerly optimizing a case which doesn't need to be optimized at the risk of introducing inconsistencies and bugs in each implementation.

Comment 8

[Sudarsana Nagineni]

Attachment: Remote SetIMEEnabled at widget side

We can consider this patch, if we decide that remoting IME calls at widget side is a best approach.

Comment 9

[Doug Turner (:dougt)]

Comment on attachment 450611 [details] [diff] [review]
Remote SetIMEEnabled call when it is called from content

using the window manager here is probably the wrong thing to do.  Instead, you would use the tabparent/tabchild which has the right window (docshell) as a member.

Comment 10

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

Why doesn't SetIMEEnabled() just call the parent process's one? In child process, it should do nothing.

And GetIMEEnabled() *MUST* be implemented if the platform supports SetIMEEnabled(). You should do it. The XP part doesn't assume that only SetIMEEnabled() is implemented.

Comment 11

[(inactive) Jim Chen [:jchen] [:darchons]]

After talking to dougt and mwu, ignore what I said before :)

cjones is working on replacing real widgets in content processes with puppet widgets (Bug 582057). In that case all IME handling should be done in the chrome process. The puppet widget would be a nice place to forward IME calls to the chrome process where the real widget handles them (similar to Masayuki's idea) This would work for all platforms.

Comment 12

[Sudarsana Nagineni]

(In reply to comment #10)
> Why doesn't SetIMEEnabled() just call the parent process's one? In child
> process, it should do nothing.

You are right. It should be Ok to remote the call and return from the child process call.

> And GetIMEEnabled() *MUST* be implemented if the platform supports
> SetIMEEnabled(). You should do it. The XP part doesn't assume that only
> SetIMEEnabled() is implemented.

true. There is a bug already for this issue.
https://bugzilla.mozilla.org/show_bug.cgi?id=555019

Currently we have a workaround for this bug but, we need to solve the issue properly.
workaround: http://hg.mozilla.org/users/romaxa_gmail.com/meego-xulrunner/file/009059cc01f0/meegotouch/pmo156419.meegotouch_xulrunner.diff#l341

Comment 13

[Sudarsana Nagineni]

(In reply to comment #11)
> After talking to dougt and mwu, ignore what I said before :)
> 
> cjones is working on replacing real widgets in content processes with puppet
> widgets (Bug 582057). In that case all IME handling should be done in the
> chrome process. The puppet widget would be a nice place to forward IME calls to
> the chrome process where the real widget handles them (similar to Masayuki's
> idea) This would work for all platforms.

Thanks for the information. This looks better approach to me.

Comment 14

[Brad Lassey [:blassey] (use needinfo?)]

This has changed since it was filed, should be covered by bug 591047 now