Closed Bug 617298 Opened 14 years ago Closed 13 years ago

Text input causes SwiftKey keyboard to force close

Categories

(Firefox for Android Graveyard :: General, defect, P1)

ARM
Android

Tracking

(firefox5- fixed, fennec4.0.2+)

VERIFIED FIXED
Firefox 5
Tracking Status
firefox5 - fixed
fennec 4.0.2+ ---

People

(Reporter: geeknik, Assigned: mwu)

References

Details

(Keywords: relnote, Whiteboard: [vkb])

Attachments

(4 files, 5 obsolete files)

Any text input with the latest nightly Fennec build using the SwiftKey keyboard causes the keyboard to force close. I can get 2-3 characters out before this occurs. I will attach logfiles as soon as I can.
Attached file Log file
This log file was grabbed right after SwiftKey crashed while using Fennec. I tried to grab the /data/data/com.touchtype.swiftkey/files/1.0.951-52907.stacktrace which was referenced in the log, but was told it didn't exist.
How do we want to handle external android keyboards in fennec?   I'm assuming this works fine against the android browser?
With the latest version of SwiftKey and the latest nightly build of Firefox Mobile 4.0b4pre, I'm not seeing a crash any more. Closing as WORKSFORME.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → WORKSFORME
I spoke too soon, just as I was closing this and typing something into the Google search box, SwiftKey force closed. I re-opened the browser and chose Swype instead, and this keyboard works fine with Firefox Mobile. Typing in the location bar is fine, no force close, typing in the search box @ google.com though, does cause a force close.
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
Same to me with SwiftKey.
Sometimes it works and sometimes its crashing.

Please let me know if i could help. I use Beta 4 from the Android Market on the Samsung Galaxy Tab ! Firmware 2.2
Despite http://www.swiftkey.net/blog/?p=148, it sounds like there are still reports of swiftkey and fennec having compatibility issues today.   Flagging for blocking radar as it affects users.

Related reports:
- http://support.mozilla.com/sr-CYRL/questions/757791
- http://groups.google.com/group/mozilla.feedback.firefox.mobile.prerelease/browse_thread/thread/2ebe0c0ecaaf4774
- http://support.mozilla.com/en-US/questions/758463
- bug 598617
tracking-fennec: --- → ?
Once again I thought that this had fixed itself, however, using the latest nightly build of Fennec and the latest market version of Swiftkey, this problem is still crashing the keyboard. I have access to the latest beta build of Swiftkey and will report back if it works.
brian, can you attach a more recent logfile during the crash?   Thanks.
Attached file Log File 01.25.11
Okay, this is a logfile generated by the Android SendLog app. I tried to update Fennec already installed, but Fennec started Force Closing, so I just uninstalled and reinstalled the latest nightly build (Mozilla/5.0 (Android; Linux armv7l; rv:2.0b10pre) Gecko/20110125 Firefox/4.0b10pre Fennec/4.0b4pre. Swiftkey is the latest market version, v1.1.91.
The log file references Stacktrace in file '/data/data/com.touchtype.swiftkey/files/1.1.91-58713.stacktrace' belongs to version 1.1.91, however, by the time I got there, the directory was empty.
tracking-fennec: ? → 2.0+
Assignee: nobody → crowderbt
adding relnote since this users are seeing this
Keywords: relnote
Okay, I've downloaded the SwiftKey Beta v1.98.1.172 from their VIP forum, and it still crashes with Fennect. Log file attached.
Pretty sure this is a SwiftKey bug, so I'm not sure how we can block on it.
if you are certain this is external, we'll need to hand this over to tech evangelism or partners.  Do we have someone that deals with mobile applications?   cc'ing kev and blizzard

More searches on feedback indicates its an issue worth following up with.  Some of these already have comments by mbrubeck:

- http://input.mozilla.com/en-US/beta/search?product=mobile&version=&date_start=&date_end=&q=swiftkey
- http://support.mozilla.com/te/questions/781301
- http://support.mozilla.com/en-US/questions/781093
- https://support.mozilla.com/en-US/questions/781272
I had an email from one of the SwiftKey devs who is going to be looking at it soon, I hope.  They have multiple reports regarding it in their database.  The Java stack for the SwiftKey crash doesn't even include us, so while something we're doing may be causing trouble for them (and indeed may even be non-standard in some way), I don't think we'll be able to track it down without their help.  For now, at least, the ball is pretty solidly in their court.
Text input isn't even working in the latest Fennec nightly. I can load Google.com and select the search field, but nothing I type in Swiftkey even shows up. Switching to the stock keyboard and Swype work just fine. If I remember correctly, text input worked briefly sometime between 1/25 and I'd say 2/1 maybe 2/5. I haven't had time to keep up with testing on the mobile side, but if someone can link an archive to all of the Fennec nightly builds, I'll spend some time this weekend looking for builds over the last 2 weeks that work and don't work and then maybe we can narrow down the changes that might be conflicting with the keyboard.
(In reply to comment #17)
> but if someone can link an archive to all of the Fennec nightly builds, I'll
> spend some time this weekend looking for builds over the last 2 weeks that work
> and don't work and then maybe we can narrow down the changes that might be
> conflicting with the keyboard.

You'll want to use the mozilla-central-android-r7 builds from here:
http://ftp.mozilla.org/pub/mozilla.org/mobile/nightly/

This shell script may be helpful for quickly donwloading nightlies for specific dates (requires the "adb" tool from the Android SDK):
http://www.lassey.us/getnightly.sh
Status: REOPENED → NEW
Tried this again and SwiftKey is still crashing, though less frequently.  Pinged SwiftKey on their support page again.
I don't think we can or should block on a crack in SwiftKey, esp. considering how difficult they seem to be to talk with.
tracking-fennec: 2.0+ → ?
All,

I'm Paul Butcher - Chief Software Architect here at TouchType (makers of SwiftKey). We're aware of this issue, and it is on our list of things to look at. Unfortunately it's on a very long list of things to look at (which hasn't been helped by the interest generated by Mobile World Congress last week).

We will get to this, I promise, but I can't give you a date right at the moment.

Sorry that I can't be more definitive.
Thanks, Paul!  If we can help by providing reduced test cases or other information, let us know.
tracking-fennec: ? → 2.0-
tracking-fennec: 2.0- → 4.0.1+
Attached patch patch (obsolete) — Splinter Review
Attachment #524228 - Flags: review?(crowderbt)
Attachment #524228 - Flags: review?(crowderbt) → review?(mwu)
Comment on attachment 524228 [details] [diff] [review]
patch

r- for not actually working. swiftkey still crashes here.
Attachment #524228 - Flags: review?(mwu) → review-
So, certain operations requires synchronization. We have an IME specific synchronization mechanism between Java and Gecko, but there is no synchronization between the parent and child process. This patch has the code wait and hope that the child process updates the parent process before the end of 20ms so that NS_QUERY_TEXT_CONTENT/mIMECacheText is accurate. I wish there were some way to ensure that mIMECacheText has been updated recently..
Assignee: crowderbt → mwu
Attachment #524228 - Attachment is obsolete: true
Attachment #526144 - Flags: review?(jimnchen+bmo)
Attachment #526144 - Flags: feedback?(jones.chris.g)
that's working for me
(In reply to comment #26)
> that's working for me

I just tested it again and it failed after typing 4 characters on google.com. Tested on a trunk build with a copy of the swiftkey trial I downloaded this morning on my N1.
Attached patch alternative patch (obsolete) — Splinter Review
this patch waits for the child process to catch up
Attachment #526317 - Flags: review?(mwu)
Attached patch alternative patch v2 (obsolete) — Splinter Review
might as well use this for selection as well
Assignee: mwu → blassey.bugs
Attachment #526317 - Attachment is obsolete: true
Attachment #526319 - Flags: review?(mwu)
Attachment #526317 - Flags: review?(mwu)
Comment on attachment 526319 [details] [diff] [review]
alternative patch v2

Needs docs.  look at the comments about |seqno| to see what I am after.

> 
>+    async AckTextEvent(PRUint32 seqno);
>+



>+{
>+  if (seqno > mAckedIMESeqno)
>+    mAckedIMESeqno = seqno;
>+  return true;

Worry about overflow?



>     case AndroidGeckoEvent::IME_GET_TEXT:
>         {
>+            if (XRE_GetProcessType() == GeckoProcessType_Default &&
>+                mozilla::dom::TabParent::GetIMETabParent() &&
>+                !mozilla::dom::TabParent::GetIMETabParent()->AllSeqNosAcked()) {
>+                nsAppShell::gAppShell->PostEvent(new AndroidGeckoEvent(ae));
>+                usleep(5000);
>+                return;
>+            }



I don't understand the usleep() here.


Also, won't this cause events to get out over order?
Not a regression from FF4, this can land on m-c when there is a reviewed patch.  If its important enough it can be re-nomed or be requested for approval when there is a reviewed patch.
(In reply to comment #30)

> >+{
> >+  if (seqno > mAckedIMESeqno)
> >+    mAckedIMESeqno = seqno;
> >+  return true;
> 
> Worry about overflow?
nope, seqno would have to overflow first

> >     case AndroidGeckoEvent::IME_GET_TEXT:
> >         {
> >+            if (XRE_GetProcessType() == GeckoProcessType_Default &&
> >+                mozilla::dom::TabParent::GetIMETabParent() &&
> >+                !mozilla::dom::TabParent::GetIMETabParent()->AllSeqNosAcked()) {
> >+                nsAppShell::gAppShell->PostEvent(new AndroidGeckoEvent(ae));
> >+                usleep(5000);
> >+                return;
> >+            }
> 
> 
> 
> I don't understand the usleep() here.
just want to give the content process time to run, sched_yield works as well in my testing if you prefer that
> 
> Also, won't this cause events to get out over order?
the java main thread is blocking until we call ReturnIMEQueryResult, so we shouldn't get anymore IME events until this gets processed. I can add some code to protect against that.
Attached patch alternative patch v3 (obsolete) — Splinter Review
Attachment #526319 - Attachment is obsolete: true
Attachment #526345 - Flags: review?(doug.turner)
Attachment #526319 - Flags: review?(mwu)
Comment on attachment 526144 [details] [diff] [review]
Bandaid over unreliable text extraction from child process

Canceling mwu's review request assuming we are going with blassey's patch.
Attachment #526144 - Flags: review?(jimnchen+bmo)
Attachment #526144 - Flags: feedback?(jones.chris.g)
Comment on attachment 526345 [details] [diff] [review]
alternative patch v3



comment needed above here.
> 
>+    async AckTextEvent(PRUint32 seqno);
>+
>     /** 
>      * Starts an offline application cache update.

You can probably drop this comment and just more fully document |AckTextEvent|.


>+    AndroidGeckoEvent(AndroidGeckoEvent* ae):
>+        mAction(ae->mAction), mType(ae->mType), mTime(ae->mTime), mP0(ae->mP0),
>+        mP1(ae->mP1), mRect(ae->mRect), mFlags(ae->mFlags), mMetaState(ae->mMetaState),
>+        mKeyCode(ae->mKeyCode), mUnicodeChar(ae->mUnicodeChar), mOffset(ae->mOffset),
>+        mCount(ae->mCount), mRangeType(ae->mRangeType), mRangeStyles(ae->mRangeStyles),
>+        mRangeForeColor(ae->mRangeForeColor), mRangeBackColor(ae->mRangeBackColor),
>+        mX(ae->mX), mY(ae->mY), mZ(ae->mZ), mCharacters(ae->mCharacters),
>+        mGeoPosition(ae->mGeoPosition), mGeoAddress(ae->mGeoAddress)
>+    {
>+    }

is there a better way, like a copy constructor or some such thing?


>@@ -1572,6 +1575,14 @@ nsWindow::OnIMEAddRange(AndroidGeckoEven
> void
> nsWindow::OnIMEEvent(AndroidGeckoEvent *ae)
> {
>+    if (mDelayedEvent) {
>+        if (mDelayedEvent == ae) {
>+            mDelayedEvent = nsnull;
>+        } else {
>+            nsAppShell::gAppShell->PostEvent(new AndroidGeckoEvent(ae));
>+            return;
>+        }
>+    }

I do not understand this change.


>     switch (ae->Action()) {
>     case AndroidGeckoEvent::IME_COMPOSITION_END:
>         {
>@@ -1614,6 +1625,13 @@ nsWindow::OnIMEEvent(AndroidGeckoEvent *
>         return;
>     case AndroidGeckoEvent::IME_GET_TEXT:
>         {
>+            if (XRE_GetProcessType() == GeckoProcessType_Default &&
>+                mozilla::dom::TabParent::GetIMETabParent() &&
>+                !mozilla::dom::TabParent::GetIMETabParent()->AllSeqNosAcked()) {
>+                nsAppShell::gAppShell->PostEvent(new AndroidGeckoEvent(ae));
>+                sched_yield();
>+                return;
>+            }

Not sure what sched_yield() gets you.
> 
> >@@ -1572,6 +1575,14 @@ nsWindow::OnIMEAddRange(AndroidGeckoEven
> > void
> > nsWindow::OnIMEEvent(AndroidGeckoEvent *ae)
> > {
> >+    if (mDelayedEvent) {
> >+        if (mDelayedEvent == ae) {
> >+            mDelayedEvent = nsnull;
> >+        } else {
> >+            nsAppShell::gAppShell->PostEvent(new AndroidGeckoEvent(ae));
> >+            return;
> >+        }
> >+    }
> 
> I do not understand this change.
this is to protect against dougt's concern in comment 32 that IME events might get out of order. The intent here is that is any IME events come in after we've delayed an IME event they'll get shuffled to back of the event queue so they all get processed in the right order.

> 
> 
> >     switch (ae->Action()) {
> >     case AndroidGeckoEvent::IME_COMPOSITION_END:
> >         {
> >@@ -1614,6 +1625,13 @@ nsWindow::OnIMEEvent(AndroidGeckoEvent *
> >         return;
> >     case AndroidGeckoEvent::IME_GET_TEXT:
> >         {
> >+            if (XRE_GetProcessType() == GeckoProcessType_Default &&
> >+                mozilla::dom::TabParent::GetIMETabParent() &&
> >+                !mozilla::dom::TabParent::GetIMETabParent()->AllSeqNosAcked()) {
> >+                nsAppShell::gAppShell->PostEvent(new AndroidGeckoEvent(ae));
> >+                sched_yield();
> >+                return;
> >+            }
> 
> Not sure what sched_yield() gets you.

this was usleep(5000) in the previous patch, I just want to make sure the child process has the opportunity to run.
Comment on attachment 526345 [details] [diff] [review]
alternative patch v3

>+bool TabParent::RecvAckTextEvent(const PRUint32& seqno)
>+{
>+  if (seqno > mAckedIMESeqno)
>+    mAckedIMESeqno = seqno;
>+  return true;
>+}

"if (seqno > mAckedIMESeqno)" not necessary?

>@@ -1614,6 +1625,13 @@ nsWindow::OnIMEEvent(AndroidGeckoEvent *
>         return;
>     case AndroidGeckoEvent::IME_GET_TEXT:
>         {
>+            if (XRE_GetProcessType() == GeckoProcessType_Default &&
>+                mozilla::dom::TabParent::GetIMETabParent() &&
>+                !mozilla::dom::TabParent::GetIMETabParent()->AllSeqNosAcked()) {
>+                nsAppShell::gAppShell->PostEvent(new AndroidGeckoEvent(ae));
>+                sched_yield();
>+                return;
>+            }
>             ALOGIME("IME: IME_GET_TEXT: o=%u, l=%u", ae->Offset(), ae->Count());
> 
>             nsQueryContentEvent event(PR_TRUE, NS_QUERY_TEXT_CONTENT, this);
>@@ -1664,6 +1682,13 @@ nsWindow::OnIMEEvent(AndroidGeckoEvent *
>     case AndroidGeckoEvent::IME_GET_SELECTION:
>         {
>             ALOGIME("IME: IME_GET_SELECTION");
>+            if (XRE_GetProcessType() == GeckoProcessType_Default &&
>+                mozilla::dom::TabParent::GetIMETabParent() &&
>+                !mozilla::dom::TabParent::GetIMETabParent()->AllSeqNosAcked()) {
>+                nsAppShell::gAppShell->PostEvent(new AndroidGeckoEvent(ae));
>+                sched_yield();
>+                return;
>+            }
> 

Maybe refactor into one block of code before the switch?

>+
>+    mozilla::AndroidGeckoEvent* mDelayedEvent;
> };
> 

Is mDelayedEvent assigned to somewhere?
Attachment #526345 - Flags: review?(jones.chris.g)
thanks for pointing that out, this:
> nsAppShell::gAppShell->PostEvent(new AndroidGeckoEvent(ae)); 
is supposed to be:
> nsAppShell::gAppShell->PostEvent(mDelayedEvent = new AndroidGeckoEvent(ae));

also, swift key is crashing when I hit the delete key with the latest patch, so I think I broke something between v1 and v3
Comment on attachment 526144 [details] [diff] [review]
Bandaid over unreliable text extraction from child process

Given that one change between v1 and v3 of the alternate patch was the removal of a 20ms sleep, and v3 is broken, I don't know if the alternate patch is actually doing any better. A proper synchronization option is welcome, but only if it works. Either way, reviewing this patch is fine. It's a backup in case we want a fix sooner.
Attachment #526144 - Flags: review?(jimnchen+bmo)
i'd much rather not have to add more ipc code in the TabChild/TabParent for IME.
again.....
 
I'd much rather not to have to add more IME handling code in the TabChild/TabParent.  If mwu's patch fixes this ime app, lets do that.
let's review the bandaid patch and get it landed for 4.0.1. We can work on a better fix for a future release.
Comment on attachment 526144 [details] [diff] [review]
Bandaid over unreliable text extraction from child process

>+        // 1. Sleep for 20 milliseconds and hope the child updates us with the new text.
>+        //    Very evil and, consequentially, most effective.
>+        try {
>+            Thread.sleep(20);
>+        } catch (InterruptedException e) {}
>+

For a future followup, we can probably get rid of this by updating the cache on the chrome side, before sending text events to content.

>+            // 2. Use mComposingText instead of what gecko gives us, since this lie works
>+            //    better for swiftkey
>+            if (extract.selectionEnd > extract.text.length())
>+                extract.text = mComposingText;

Do you know how well / why this works? We should return the entire content of the textfield, but mComposingText is only a portion of the whole text.
This missed the 4.0.1 release. Re-nominating for Firefox 5.
tracking-fennec: 4.0.1+ → ?
no reason to drop this back to a nom
tracking-fennec: ? → 4.0.1+
note to self: this patch missed 4.0.1 build 1, but continues to be flagged to blocking-fennec 4.0.1+ to track future respin work.
Assignee: blassey.bugs → mwu
(In reply to comment #43)
> Comment on attachment 526144 [details] [diff] [review]
> Bandaid over unreliable text extraction from child process
> 
> >+        // 1. Sleep for 20 milliseconds and hope the child updates us with the new text.
> >+        //    Very evil and, consequentially, most effective.
> >+        try {
> >+            Thread.sleep(20);
> >+        } catch (InterruptedException e) {}
> >+
> 
> For a future followup, we can probably get rid of this by updating the cache on
> the chrome side, before sending text events to content.
> 

I'll file it.

> >+            // 2. Use mComposingText instead of what gecko gives us, since this lie works
> >+            //    better for swiftkey
> >+            if (extract.selectionEnd > extract.text.length())
> >+                extract.text = mComposingText;
> 
> Do you know how well / why this works? We should return the entire content of
> the textfield, but mComposingText is only a portion of the whole text.

Swiftkey wants to know what the current word is, and if the info from gecko is wrong, we'd rather have the current word be accurate than pass info about all the text in the field.
Attachment #526345 - Attachment is obsolete: true
Attachment #526345 - Flags: review?(jones.chris.g)
Attachment #526345 - Flags: review?(doug.turner)
This is pretty ridiculous.  There's really still no word about what's wrong on swiftkey's end?

I'm not familiar with the Java code being patched here but a 20ms sleep on an input-event path seems like a potentially noticeable lag.  Can we somehow detect swiftkey and only do these hacks?  Or better yet, blacklist swiftkey somehow?
(In reply to comment #48)
> This is pretty ridiculous.  There's really still no word about what's wrong on
> swiftkey's end?
> 

Oh I know what's wrong. We're giving them some text from the field and two indexes indicating where the selection starts and ends. Under certain circumstances, the parent process will not give the right text for the field and we end up giving the IME some text with an out of bound selection. Swiftkey dies because it doesn't bounds check.

> I'm not familiar with the Java code being patched here but a 20ms sleep on an
> input-event path seems like a potentially noticeable lag.  Can we somehow
> detect swiftkey and only do these hacks?  Or better yet, blacklist swiftkey
> somehow?

I've tested it and noticed no difference. I suspect this will only be noticeable if you hit the next key within 20ms since this call is executed after every key entry instead of before. The gecko thread itself isn't blocked.
(In reply to comment #49)
> Oh I know what's wrong. We're giving them some text from the field and two
> indexes indicating where the selection starts and ends. Under certain
> circumstances, the parent process will not give the right text for the field
> and we end up giving the IME some text with an out of bound selection. Swiftkey
> dies because it doesn't bounds check.

Wait --- the cached text and indices in the chrome process should always be consistent, even if they're slightly out-of-date compared to what's in the content process.  Or is there some other problem?

> 
> > I'm not familiar with the Java code being patched here but a 20ms sleep on an
> > input-event path seems like a potentially noticeable lag.  Can we somehow
> > detect swiftkey and only do these hacks?  Or better yet, blacklist swiftkey
> > somehow?
> 
> I've tested it and noticed no difference. I suspect this will only be
> noticeable if you hit the next key within 20ms since this call is executed
> after every key entry instead of before. The gecko thread itself isn't blocked.

That's the case I had in mind, successive events hitting this code.  This hack would allow fast typists to build up an unbounded UI delay.  That sounds bad.

If there's really truly no other way to fix or work around this, can we please only do the hacks for swiftkey?
(In reply to comment #50)
> (In reply to comment #49)
> > Oh I know what's wrong. We're giving them some text from the field and two
> > indexes indicating where the selection starts and ends. Under certain
> > circumstances, the parent process will not give the right text for the field
> > and we end up giving the IME some text with an out of bound selection. Swiftkey
> > dies because it doesn't bounds check.
> 
> Wait --- the cached text and indices in the chrome process should always be
> consistent, even if they're slightly out-of-date compared to what's in the
> content process.  Or is there some other problem?
> 

That's the problem - they're inconsistent.

> > 
> > > I'm not familiar with the Java code being patched here but a 20ms sleep on an
> > > input-event path seems like a potentially noticeable lag.  Can we somehow
> > > detect swiftkey and only do these hacks?  Or better yet, blacklist swiftkey
> > > somehow?
> > 
> > I've tested it and noticed no difference. I suspect this will only be
> > noticeable if you hit the next key within 20ms since this call is executed
> > after every key entry instead of before. The gecko thread itself isn't blocked.
> 
> That's the case I had in mind, successive events hitting this code.  This hack
> would allow fast typists to build up an unbounded UI delay.  That sounds bad.
> 

20ms puts an upper limit of 50 characters per second. Lets say the real limit is around 20 characters per second. Wikipedia says "The fastest typing speed ever, 216 words per minute, was achieved by Stella Pajunas-Garnand from Chicago in 1946 in one minute on an IBM electric." Assuming each word is 5 characters, that's 18 characters per second.

20 ms is the simplest fix I can come with. 10ms fails sometimes. Maybe 15ms could work. I don't consider this issue particularly swiftkey specific. I'd like the text reporting to be reasonably accurate for all soft keyboards. Simply fixing the selection indexes causes odd typos on swiftkey. Perhaps there are keyboards which correct the selection indexes and then screw things up.

Ideally, we'd just have the information be consistent. I suspect you or jchen would know how to fix it right.
(In reply to comment #51)
> (In reply to comment #50)
> > Wait --- the cached text and indices in the chrome process should always be
> > consistent, even if they're slightly out-of-date compared to what's in the
> > content process.  Or is there some other problem?
> > 
> 
> That's the problem - they're inconsistent.

That just sounds like a plain-jane PuppetWidget bug then.  Why not fix that?

> 20ms puts an upper limit of 50 characters per second. Lets say the real limit
> is around 20 characters per second. Wikipedia says "The fastest typing speed
> ever, 216 words per minute, was achieved by Stella Pajunas-Garnand from Chicago
> in 1946 in one minute on an IBM electric." Assuming each word is 5 characters,
> that's 18 characters per second.

What happens if I'm playing a game and hold down a key for repeated input?  Also, a 20ms pause will cause us to drop frames in 60fps animations/videos on key input.  That sucks.

> Ideally, we'd just have the information be consistent. I suspect you or jchen
> would know how to fix it right.

I'd sure prefer to do that.
(In reply to comment #52)
> (In reply to comment #51)
> > (In reply to comment #50)
> > > Wait --- the cached text and indices in the chrome process should always be
> > > consistent, even if they're slightly out-of-date compared to what's in the
> > > content process.  Or is there some other problem?
> > > 
> > 
> > That's the problem - they're inconsistent.
> 
> That just sounds like a plain-jane PuppetWidget bug then.  Why not fix that?
> 

I don't know how. I also don't like the cache to be inconsistent - I suspect that will cause typos.

> > 20ms puts an upper limit of 50 characters per second. Lets say the real limit
> > is around 20 characters per second. Wikipedia says "The fastest typing speed
> > ever, 216 words per minute, was achieved by Stella Pajunas-Garnand from Chicago
> > in 1946 in one minute on an IBM electric." Assuming each word is 5 characters,
> > that's 18 characters per second.
> 
> What happens if I'm playing a game and hold down a key for repeated input? 

You're generally not using an IME in that case because IMEs can't be counted on to fire keyevents.

> Also, a 20ms pause will cause us to drop frames in 60fps animations/videos on
> key input.  That sucks.
> 

Drawing is on the gecko thread so drawing isn't affected.
(In reply to comment #53)
> Drawing is on the gecko thread so drawing isn't affected.

We composite to window on the java main thread though, right?
(In reply to comment #54)
> (In reply to comment #53)
> > Drawing is on the gecko thread so drawing isn't affected.
> 
> We composite to window on the java main thread though, right?

We specifically use the API (SurfaceView) that android provides for doing graphics operations on a separate thread. Ideally such an API would not block on the main thread. The docs describe GLSurfaceView, a subclass of SurfaceView, as rendering "on a dedicated thread to decouple rendering performance from the UI thread."

I changed the 20 ms delay to a 1500 delay to check this assumption and it holds. The caret continues to blink during the delay.
Comment on attachment 526144 [details] [diff] [review]
Bandaid over unreliable text extraction from child process

(In reply to comment #47)
> (In reply to comment #43)
> > Comment on attachment 526144 [details] [diff] [review]
> > Bandaid over unreliable text extraction from child process
> > 
> > >+            // 2. Use mComposingText instead of what gecko gives us, since this lie works
> > >+            //    better for swiftkey
> > >+            if (extract.selectionEnd > extract.text.length())
> > >+                extract.text = mComposingText;
> > 
> > Do you know how well / why this works? We should return the entire content of
> > the textfield, but mComposingText is only a portion of the whole text.
> 
> Swiftkey wants to know what the current word is, and if the info from gecko is
> wrong, we'd rather have the current word be accurate than pass info about all
> the text in the field.

Sorry, I'm still a little wary about this, Maybe something like this?

> if (mComposing && extract.selectionEnd > extract.text.length())
>   extract.text = extract.text.subSequence(0, mCompositionStart) + mComposingText;

This tries to retain everything before the composition. Everything else looks ok to me.
Attachment #526144 - Flags: review?(jimnchen+bmo) → review+
(In reply to comment #56)
> Sorry, I'm still a little wary about this, Maybe something like this?
> 
> > if (mComposing && extract.selectionEnd > extract.text.length())
> >   extract.text = extract.text.subSequence(0, mCompositionStart) + mComposingText;
> 
> This tries to retain everything before the composition. Everything else looks
> ok to me.

Gotcha. I like this better too.
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/e365bff10705
Status: NEW → RESOLVED
Closed: 13 years ago13 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 6
Comment on attachment 529002 [details] [diff] [review]
Bandaid over unreliable text extraction from child process, v2

Nominating this patch for Aurora.  This is one of our top 10 user issues on Android [1].  It makes Firefox completely unusable with SwiftKey, one of the most popular Android IMEs.  It was previous triaged as blocking 4.0.1+, though it was dropped from that release because the patch was not ready on time.

The patch touches only Android Java code, so there is zero risk for desktop Firefox.

1. https://wiki.mozilla.org/Mobile/StatusMeetings/04-14-2011#Feedback_Summary

(Carrying r=jchen)
Attachment #529002 - Flags: review+
Attachment #529002 - Flags: approval-mozilla-aurora?
I'll approve this after its baked on trunk for a few days
I've filed bug 653895 for a real fix.
Comment on attachment 529002 [details] [diff] [review]
Bandaid over unreliable text extraction from child process, v2

haven't heard of any issues resulting from this, approval-mozilla-aurora=blassey
Attachment #529002 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Pushed to Aurora (Firefox 5).
http://hg.mozilla.org/mozilla-aurora/rev/689679008349

Nominating for tracking-fennec:4.0.2 since it missed 4.0.1.
tracking-fennec: 4.0.1+ → ?
Keywords: checkin-needed
Target Milestone: Firefox 6 → Firefox 5
tracking-fennec: ? → 4.0.2+
Verified fixed on build: Mozilla /5.0 (Android;Linux armv7l;rv:6.0a1) Gecko/20110505 Firefox/6.0a1 Fennec/6.0a1 
Device: LG Optimus 2X
Status: RESOLVED → VERIFIED
Verified on Aurora Branch:
Mozilla/5.0 (Android; Linux armv71; rv5.0a2) Gecko/20110509 Firefox/5.0a2 Fennec/5.0a2
Device: Thunderbolt
OS: Android 2.2
Could this be backported to a Firefox 4 Mobile minor release?
This has approval to be included in a 4.0.2 release if there is one, but there is no such release currently planned.  The next planned update to Firefox is the Firefox 5 release next month.
Hi guys,

We just tested Firefox and Firefox Beta from the Android market and with the new SwiftKey Beta (not yet available to the world). It seems to work well apart from two things. On >= Android 2.3 we use InputConnection.setComposingRegion to set the current word being edited (e.g. after backspacing) but Firefox ignores it! On < 2.3 we delete the characters and re-insert the text to mimic this behaviour, but this seems to cause Firefox to get stuck in a loop of entering and removing the text.

It seems like Firefox has its own implementation of InputConnection. Can you update your implementation to resolve these issues?

Regards,
Chetan. (Software Engineer @ TouchType)
(In reply to comment #69)
> Hi guys,
> 
> We just tested Firefox and Firefox Beta from the Android market and with the
> new SwiftKey Beta (not yet available to the world). It seems to work well
> apart from two things. On >= Android 2.3 we use
> InputConnection.setComposingRegion to set the current word being edited
> (e.g. after backspacing) but Firefox ignores it! On < 2.3 we delete the
> characters and re-insert the text to mimic this behaviour, but this seems to
> cause Firefox to get stuck in a loop of entering and removing the text.
> 
> It seems like Firefox has its own implementation of InputConnection. Can you
> update your implementation to resolve these issues?
> 
> Regards,
> Chetan. (Software Engineer @ TouchType)

The change to make isn't obvious to me, can you provide a patch to illustrate it? Our implementation is here https://mxr.mozilla.org/mozilla-central/source/embedding/android/GeckoInputConnection.java
Currently Firefox builds with Android API level 5 (see: https://mxr.mozilla.org/mozilla-central/source/embedding/android/AndroidManifest.xml.in). InputConnection.setComposingRegion was introduced in API level 9.

Firefox must be compiled against API >= 9 and https://mxr.mozilla.org/mozilla-central/source/embedding/android/GeckoInputConnection.java must override and implement setComposingRegion. See http://developer.android.com/reference/android/view/inputmethod/BaseInputConnection.html#setComposingRegion(int,%20int) for documentation.
(BaseInputConnection already implements this but Firefox must override this implementation to pass the event to Gecko.)

Regards,
Chetan.
I have tested this with the latest SwiftKey X (released last week) with Firefox 6.0 Beta and the problem still exists. It is reproducible on any >= Android 2.3 device. (User video of issue here: http://www.youtube.com/watch?v=n-DWDFPjiuM)
Please note: All SwiftKey users get a free upgrade to SwiftKey X.

Please reopen this bug!
Chetan, thanks for the detailed video showing the problems you're experiencing. However, it shows different symptoms than the ones that this bug report was addressing. Would you mind filing a new bug for your report, so we can keep the issues and fixes logically separate?
I've started testing Swiftkey X, and I've filed bug 672661 for one problem I found.  I'll continue filing bugs as I reproduce more problems.
You need to log in before you can comment on or make changes to this bug.