756429 - Accented characters are substituted when entering other characters and can also remove the content before it on mobile.twitter.com when using HKB

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Keyboards and IME, defect)

Description

[Adrian Tamas (:AdrianT)]

Nightly 15.0a1 2012-05-18 / Aurora 14.0a2 2012-05-18
Device: HTC Desire Z / Motorola Droid Pro
OS: Android 2.3.3 / Android 2.3.4

Steps to reproduce:
1. Go to mobile.twitter.com.
2. Sign in. In the "What's happening?" field start typing some text.
3. Pres and hold a key to bring up the alternative values (use a,e,i,c or o which have accented characters) and choose one.
4. Continue typing.

Expected results:
The user can type words with accented characters in them. 

Actual results:
Sometimes when the accented character is entered everything before it is removed. When entering characters after the accented character the accented character is substituted.

Please see the video capture: http://youtu.be/ZCyDxwJ4v0U

Comment 1

[Chris Peterson [:cpeterson]]

Is this a regression? I thought I had fixed this bug on Nightly 2012-04-28, but I can now repro on Droid Pro.

Comment 2

[Adrian Tamas (:AdrianT)]

The issue was fixed in Nightly 2012-05-16 and has reappeared on Nightly 2012-05-18.

Possible regression range:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=762e95608da3&tochange=e794cef56df6

Comment 3

[Chris Peterson [:cpeterson]]

Thanks, Adrian. In that regression range, I suspect this Nightly regression was my fix for bug 751513:

https://hg.mozilla.org/mozilla-central/rev/43aa1f392da4

Comment 4

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

Let's test aurora and beta to make sure this is not showing up there. It should be backed out from those channels.

Comment 5

[Tony Chung [:tchung]]

(In reply to Mark Finkle (:mfinkle) from comment #4)
> Let's test aurora and beta to make sure this is not showing up there. It
> should be backed out from those channels.

Adrian, can you retest as above?  if its fixed on those channels, comment and remove qawanted.

Comment 6

[Adrian Tamas (:AdrianT)]

The issue is reproducible on Firefox Beta 14.0b2 and Aurora 14.0a2 2012-05-21.

Comment 7

[Chris Peterson [:cpeterson]]

Attachment: part-1-commit-on-reinitialization.patch

Commit composition string when IME is reinitialized from VKB popup window.

1. onCreateInputConnection() can be called during composition when input focus is restored from a VKB popup window (such as for entering accented characters) back to our IME. We want to commit our active composition string.

2. If Gecko cancels the current composition from underneath us, abandon our active composition string WITHOUT committing it.

Comment 8

[Chris Peterson [:cpeterson]]

Attachment: part-2-assert-composition-sanity.patch

Assert sanity of composition string's state changes. These asserts will fail without the fix from patch part 1.

I used MOZ_ASSERT (which is fatal in debug builds) for conditions I know should be true. I used NS_ASSERTION (which merely logs warnings in debug builds) for conditions I think are true, but I'm not completely sure yet.

Comment 9

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

Comment on attachment 628973 [details] [diff] [review]
part-1-commit-on-reinitialization.patch

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

::: mobile/android/base/GeckoInputConnection.java
@@ +534,5 @@
> +    private void resetCompositionState() {
> +        if (DEBUG && hasCompositionString()) {
> +            Log.d(LOGTAG, "resetCompositionState() is abandoning an active composition string, "
> +                          + "mCompositionStart=" + mCompositionStart + " -> -1");
> +        }

why can't this go in the debug subclass?

@@ -1266,5 @@
> -    @Override
> -    public void notifyIMEChange(String text, int start, int end, int newEnd) {
> -        Log.d(LOGTAG, String.format("IME: >notifyIMEChange(\"%s\", start=%d, end=%d, newEnd=%d)",
> -                                    text, start, end, newEnd));
> -        super.notifyIMEChange(text, start, end, newEnd);

why remove this logging?

Comment 10

[Chris Peterson [:cpeterson]]

(In reply to Brad Lassey [:blassey] from comment #9)

> > +    private void resetCompositionState() {
> > +        if (DEBUG && hasCompositionString()) {
> > +            Log.d(LOGTAG, "resetCompositionState() is abandoning an active composition string, "
> > +                          + "mCompositionStart=" + mCompositionStart + " -> -1");
> > +        }
> 
> why can't this go in the debug subclass?

Good point. I've fix that.


> > -    public void notifyIMEChange(String text, int start, int end, int newEnd) {
> > -        Log.d(LOGTAG, String.format("IME: >notifyIMEChange(\"%s\", start=%d, end=%d, newEnd=%d)",
> > -                                    text, start, end, newEnd));
> > -        super.notifyIMEChange(text, start, end, newEnd);
> 
> why remove this logging?

For what seems like convenience of not adding a new function, Gecko calls notifyIMEChange() for both text changes and selection changes. A negative `newEnd` sentinel value indicates a selection change. The GeckoInputConnection super class demultiplexes notifyIMEChange() to notifyIMETextChange() or notifyIMESelectionChange(). Since those functions log their own more-specific calls, I removed the redundant less-specific logging in notifyIMEChange().

Comment 11

[Chris Peterson [:cpeterson]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9a6a5cc5a4a5
https://hg.mozilla.org/integration/mozilla-inbound/rev/180b78b6cec4

Comment 12

[Chris Peterson [:cpeterson]]

Comment on attachment 628973 [details] [diff] [review]
part-1-commit-on-reinitialization.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): N/A
User impact if declined: HKB users will not be able to enter accented characters.
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky): Low risk of incomplete composition strings being entered. Android Java only. blocking-fennec1.0=+
String or UUID changes made by this patch: N/A

Patch part 2 is NOT needed for Aurora because it just adds some debug code.

Comment 13

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/9a6a5cc5a4a5
https://hg.mozilla.org/mozilla-central/rev/180b78b6cec4

Comment 14

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

Comment on attachment 628973 [details] [diff] [review]
part-1-commit-on-reinitialization.patch

[Triage Comment]

Comment 15

[Chris Peterson [:cpeterson]]

https://hg.mozilla.org/releases/mozilla-beta/rev/ac2ef12535c0