624163 - Compatibility: IME input does not work in rich editors

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Thomas Arend [:tarend]]

Steps to reproduce:
(1) go to docs.google.com
(2) sign in with your google account
(3) click existing doc or create new doc

Result: user can not edit document. Typing doesn't show characters in document. Changing User Agent (with Phony) doesn't help.
Expected: user can edit documents and spreadsheets

The stock browser works fine!

Reported by user Andrew Rush (thanks, Andrew!):
"I love the app on my evo. It's a little slow when I have multiple tabs open, but that's to be expected.

I have only one major complaint, and it's pretty specific.  I use Google docs to write screenplays, and I want to use it on my evo.  The onboard browser doesn't allow this (even though it's supposed to), and while FireFox allows me to use my keyboard, the text I type does not show up on the page, and the flashing vertical bar does not move.

There are some apps that claim to get around this but they're all very buggy. 
Please help me! Thank you for your work."

Comment 1

[Aakash Desai [:aakashd]]

I could have sworn we have a bug on this....oh well. Nominating for blocking-fennec

Comment 2

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

It similar some other bugs where text doesn't show up in the edit box

Comment 3

[Thomas Arend [:tarend]]

Attachment: typing in Google Docs

sorry this is mirrored and blurry. I type and the characters are not put in the document, except for BACKSPACE and ENTER

Comment 4

[Thomas Arend [:tarend]]

Attachment: script error in Google Docs

and about a minute into trying to edit Google Docs I get a "script unresponsive" error: https://docs.google.com/doclist/client/js/4268954178-doclist_modularized-gecko-core.js:592

Comment 5

[Matt Brubeck (:mbrubeck)]

I was able to edit a Google spreadsheet successfully, using both the mobile interface (in Fennec on Android) and the desktop interface (in Fennec on desktop).

In the Google Docs word processor, I get the desktop UI on both desktop and Android.  The content is fixed-width and centered, which causes the left and right edges to be cut off.  (This is not a Fennec bug; the same thing happens in desktop browsers with narrow windows.)  Zooming is disabled with a meta viewport tag.

I can reproduce the keyboard problems from comment 3 (maybe caused by bug 630576).

Changing Fennec's UA to match the stock Android browser results in a mobile version that still has some display problems (content is centered and the left and right edges are cut off).  Pressing the "Edit" button in the mobile version leads to a blank screen.

We can maybe fix the key events (bug 630576), but the display issues and compatibility with the mobile UI will need an evangelism bug.

Comment 6

[Matt Brubeck (:mbrubeck)]

Based on my testing, the typing problem is indeed bug 630576.

Marking this bug as duplicate.  Feel free to open evangelism bugs to fix Google Docs in narrow windows and/or make the mobile version compatible with Firefox.

Comment 7

[Alex Pakhotin (:alexp)]

I don't think this bug should be marked as a duplicate of bug 630576. The latter is actually questionable, there might be a different fix for Google Docs without replacing IME events with keypresses (which is not quite correct, I believe).
Anyway, the bugs are not really the same, so I'm reopening this one.

Comment 8

[Alex Pakhotin (:alexp)]

I did some testing using IBus IME on Linux and a modified test page from Quirksmode: http://people.mozilla.com/~alexp/fennec/events/
The results: Firefox 4 and Fennec seem to generate the same composition events with IME, and no keydown/keypress/keyup events (as expected), but Google Docs works in Firefox, and doesn't in Fennec.
I'll keep investigating.

Comment 10

[Alex Pakhotin (:alexp)]

I checked this on Etherpad as well - seems to be the same issue.

Fennec detects the editable field on the page, opens a screen keyboard.
When typing IME events are received by the widget code as usual, but do not propagate to the content: specifically - they are not coming to TabParent, and TabChild.

During those IME events processing NS_QUERY_TEXT_CONTENT and NS_QUERY_SELECTED_TEXT events are dispatched to get data from the content, but they both fail, which might be the cause of the problem.

Comment 11

[Martijn Wargers (dead)]

This is also a problem on http://www.mozilla.org/editor/midasdemo/
Not being able to type in the desingMode iframe on the Droid.

Comment 12

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

same issue with ietherpad.com and sync.in

Comment 13

[Thomas Arend [:tarend]]

Attachment: Input Issue

User Ronnie A. reports a similar input issue on , ecampus.phoenix.edu
He can't type any text - see attachment

Comment 14

[Alex Pakhotin (:alexp)]

Apparently the problem is in our interaction with Midas, Gecko's built-in rich text editor, which is enabled by setting document.designMode property to "on".
If anyone has any thoughts on this, can you please comment.

Comment 15

[Thomas Arend [:tarend]]

User Philipp T. reports this issue also with vBulletin boards, e.g. pcmasters.de/forum.
What is the status of this bug?

Comment 16

[Alex Pakhotin (:alexp)]

The issue seems to be with all the sites, which use the rich text editor.
I'm looking at this again. Have some leads, but need more debugging.

Comment 17

[Alex Pakhotin (:alexp)]

Some more info.

First of all, I have the simplest page, where this issue is reproducible:
http://people.mozilla.com/~alexp/bug/624163-RichEdit/richedit.htm

The problem might be related to incorrect initialization of the selection ranges, and inability to get the selection after that.
When the IME composition events are being processing we try to get a selection using NS_QUERY_SELECTED_TEXT event, but that fails.
The specific place, where an error code is returned, and after that the composition event processing stops, is here:
http://mxr.mozilla.org/mozilla-central/source/content/events/src/nsContentEventHandler.cpp#102
mSelection->GetRangeAt() fails because the selection ranges array is empty.
According to the comment in the code, the presentation shell, which contains the editor object, doesn't support selection.

I'm trying to figure out why the ranges are not initialized properly, and what else might be wrong with that presentation shell, but it's very time consuming, as I basically have to do reverse engineering without good understanding of the components interaction and the normal workflow.
It might help if someone, who is familiar with PresShell and all that content machinery, could take a look and probably give some clues.

Please note: the issue is not Android specific. It is 100% reproducible in the Linux build of Fennec as well. I've been testing it on Ubuntu using IBus IME system.

Comment 18

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

Made the summary more generic to fit the scope of the bug

Comment 19

[Alex Pakhotin (:alexp)]

A bit more info.

IME input works in the main Fennec process, but does not in the plugin-container process, which means this is e10s issue.

DispatchCompositionStart() tries to get the selection using NS_QUERY_SELECTED_TEXT, which gets processed, but in nsEventStateManager::PreHandleEvent() the call to nsEventStateManager::RemoteQueryContentEvent() returns false because IsTargetCrossProcess() checks TabParent::GetIMETabParent(), and that is NULL.
TabParent::mIMETabParent has to be set in TabParent::RecvNotifyIMEFocus(), which does not get called.
SendNotifyIMEFocus() is supposed to be called from PuppetWidget::OnIMEFocusChange(), but that doesn't get called either.
Looks like nsIMEStateManager::OnTextStateFocus() should call that OnIMEFocusChange(), but it happens only when GetRootEditableNode() returns an existing node, which is not the case in this situation - it just cannot find an editable node for the presentation context passed to the function, and returns NULL.

I also checked - when design mode is enabled for the nsHTMLDocument, the NODE_IS_EDITABLE flag is actually set.

I also debugged ActivateRemoteFrame as suggested by Olli, here are the functions called:
nsFrameLoader::ActivateRemoteFrame() --> TabParent::SendActivate -->...--> nsWebBrowser::Activate --> nsFocusManager::WindowRaised --> nsFocusManager::EnsureCurrentWidgetFocused --> PuppetWidget::SetFocus()...

In that SetFocus function I found the following comment (http://mxr.mozilla.org/mozilla-central/source/widget/src/xpwidgets/PuppetWidget.cpp#226):
  // XXX/cjones: someone who knows about event handling needs to
  // decide how this should work.

I have a suspicion this might be related to the input problem. Bug 583976 deals with the focus issue, but the patch from that bug doesn't yet resolve this issue.


(CC'ing other people who may have some thoughts on this.)

Comment 20

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

Attachment: patch

Comment 21

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 532417 [details] [diff] [review]
patch

># HG changeset patch
># User Brad Lassey <[e-mail]>
># Date 1305354248 14400
># Node ID 17a3a4ba04d248c431de7f476642bc3f13e23455
># Parent  6105fb36f61301dbbd903a568170b329d62b75e2
>[mq]: rich_editors
>
>diff --git a/dom/ipc/TabParent.cpp b/dom/ipc/TabParent.cpp
>--- a/dom/ipc/TabParent.cpp
>+++ b/dom/ipc/TabParent.cpp
>@@ -527,6 +527,7 @@ TabParent::RecvGetIMEEnabled(PRUint32* a
> bool
> TabParent::RecvSetInputMode(const PRUint32& aValue, const nsString& aType, const nsString& aAction, const PRUint32& aReason)
> {
>+  mIMETabParent = aValue ? this : nsnull;
>   nsCOMPtr<nsIWidget> widget = GetWidget();
>   if (!widget || !AllowContentIME())
>     return true;

I have no idea what this patch is doing ... can you please add a comment?  What does aValue==0 mean?

Comment 22

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

Comment on attachment 532417 [details] [diff] [review]
patch

aValue is the status ield of an IMEContext struct and 0 is the value of IME_STATUS_NONE. I can make it aValue != IME_STATUS_NONE to make it more clear.

Comment 23

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #22)
> Comment on attachment 532417 [details] [diff] [review] [review]
> patch
> 
> aValue is the status ield of an IMEContext struct and 0 is the value of
> IME_STATUS_NONE. I can make it aValue != IME_STATUS_NONE to make it more
> clear.

Can you please do that and add a comment describing why nulling out mIMEParent is necessary here?

Ehsan: do you happen to know if we have existing tests of IME composition in "rich editors" that change focus across content/UI?  (I.e., might we have already caught this bug if we were running those tests.)

Comment 24

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

Attachment: patch

Comment 25

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

Comment on attachment 532548 [details] [diff] [review]
patch

moving the review over to dougt, since Chris is apparently gone for the next 2 weeks

Comment 26

[(no longer active)]

(In reply to comment #23)
> Ehsan: do you happen to know if we have existing tests of IME composition in
> "rich editors" that change focus across content/UI?  (I.e., might we have
> already caught this bug if we were running those tests.)

I don't know of any, no.  But shouldn't this patch add one?

Comment 27

[Doug Turner (:dougt)]

Comment on attachment 532548 [details] [diff] [review]
patch

we also set mIMETabParent in RecvNotifyIMEFocus() for similar reasons.

Comment 28

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

pushed http://hg.mozilla.org/mozilla-central/rev/c9999bd821ca

Comment 29

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

Comment on attachment 532548 [details] [diff] [review]
patch

requesting approval for beta. The risk is near zero for desktop as this is an e10s only change, so essentially mobile-only. For mobile it is a relatively small change, has been baking on trunk over the weekend and IMO within our risk tolerance. The benefit is it makes rich text editors (like google docs and etherpad) work in content.

Comment 30

[Daniel Veditz [:dveditz]]

Comment on attachment 532548 [details] [diff] [review]
patch

Approved for the mozilla-beta repository, a=dveditz for release-drivers

Comment 31

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #26)
> (In reply to comment #23)
> > Ehsan: do you happen to know if we have existing tests of IME composition in
> > "rich editors" that change focus across content/UI?  (I.e., might we have
> > already caught this bug if we were running those tests.)
> 
> I don't know of any, no.  But shouldn't this patch add one?

If existing tests we're not running would have caught this bug, then no; instead we should push on running the existing tests.  Otherwise, yes.  Though really adding tests never hurts ;).

Comment 32

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

was pushed to beta:
http://hg.mozilla.org/releases/mozilla-beta/rev/886dce5aad91

Comment 33

[Aaron Train [:aaronmt]]

Verified fixed on 5.0b6 build 1
Mozilla/5.0 (Android; Linux armv7l; rv:5.0) Gecko/20110613 Firefox/5.0 Fennec/5.0