712018 - International hardware keyboards input text as English qwerty (need implement IME support for hardware keyboard)

Status: RESOLVED FIXED

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

Description

[Augure]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:8.0) Gecko/20100101 Firefox/8.0
Build ID: 20111104165243

Steps to reproduce:

I typed some url in the url bar.
I tried filling web forms with the latest nightly build.
My hardware is : ASUS TRANSFORMER with physical french keyboard.
The keyboard was correctly set in Android parameters.


Actual results:

The url was correctly typed ( a key wrote "a" in the bar)
"a" key was "q" in the textbox in the web form (Same in hidden textbox for password)


Expected results:

"a" key should have been "a" character in textboxes

Comment 1

[Makoto Kato [:m_kato]]

Augure, is this Fennec Native only?  

If this doesn't occurs on Fennec XUL, this is a regression of bug 699792.

Comment 2

[Augure]

(In reply to Makoto Kato from comment #1)
> Augure, is this Fennec Native only?  
> 
> If this doesn't occurs on Fennec XUL, this is a regression of bug 699792.

Hi, didn't have the time to test with the latest version but i didn't notice this bug in Fennec.
I will post this evening a clear answer to that question.
Lionel

Comment 3

[Augure]

(In reply to Augure from comment #2)
> (In reply to Makoto Kato from comment #1)
> > Augure, is this Fennec Native only?  
> > 
> > If this doesn't occurs on Fennec XUL, this is a regression of bug 699792.
> 
> Hi, didn't have the time to test with the latest version but i didn't notice
> this bug in Fennec.
> I will post this evening a clear answer to that question.
> Lionel

Hello,
tested on the Firefox release on Android Market and the "bug" is not here. I only have it (tested also this evening) with this alpha version.

Lionel

Comment 4

[Chris Peterson [:cpeterson]]

Lionel, thanks for testing. This information is a big help.

Comment 5

[Martijn Wargers (dead)]

Augure, can you still reproduce it?

Comment 6

[Makoto Kato [:m_kato]]

(In reply to Martijn Wargers [:mw22] (QA - IRC nick: mw22) from comment #5)
> Augure, can you still reproduce it?

Yes.  This is a regression of bug 699792.  Some HKB IMEs (HTC Touch Input on Desire Z and etc) have a bug, we have added workaround.  But the workaround causes this bug...

Comment 7

[Augure]

(In reply to Makoto Kato from comment #1)
> Augure, is this Fennec Native only?  
> 
> If this doesn't occurs on Fennec XUL, this is a regression of bug 699792.

Hi,
sorry for the delay.

I tested one week ago and this problem was still there.

Lionel

Comment 9

[Makoto Kato [:m_kato]]

Attachment: wip

Comment 10

[Chris Peterson [:cpeterson]]

Comment on attachment 637031 [details] [diff] [review]
wip

Makoto, since we are now finding bugs that require workarounds for specific IMEs' "quirks", we should think about consolidating the IME identification code into a common module. I'm not sure what that will look like, but I know that "com.htc.android.htcime" will not be the last IME we need to identify. :)

Comment 11

[Makoto Kato [:m_kato]]

(In reply to Chris Peterson (:cpeterson) from comment #10)
> Comment on attachment 637031 [details] [diff] [review]
> wip
> 
> Makoto, since we are now finding bugs that require workarounds for specific
> IMEs' "quirks", we should think about consolidating the IME identification
> code into a common module. I'm not sure what that will look like, but I know
> that "com.htc.android.htcime" will not be the last IME we need to identify.
> :)

Bug 685537. HTC Touch Input sets invalid META/SHIFT state for composition string.

Android framework's BaseInputConnection.commitText uses sendKeyEvent() like https://bugzilla.mozilla.org/attachment.cgi?id=566740&action=diff. So this problem doesn't occurs on TextView.

Comment 12

[Makoto Kato [:m_kato]]

Also, https://bugzilla.mozilla.org/attachment.cgi?id=566740&action=diff doesn't work on SHARP 007SH.  Because KeyCharacterMap.BUILT_IN_KEYBOARD isn't equal to physical keyboard mapping.

Comment 13

[Chris Peterson [:cpeterson]]

Also, Swype-specific feature request in bug 767597 and Galaxy Note-specific bug 758427.

Comment 16

[Norbert]

Has been fixed for the TF300T by Asus Firmware Update which introduces Android 4.1. !!!

Comment 17

[Norbert]

So we can close this bug?

Comment 18

[Augure]

Problem still there for TF 101 with latest Firefox Beta.
Android 4.0
ASUS

Lionel

Comment 19

[Norbert]

@Augure: Is there no Android 4.1 firmware update available for your device?

Comment 20

[Makoto Kato [:m_kato]]

 Norbert, Keep open status because HKB implementation has no IME support, so this depends on current keyboard system.

Comment 21

[Augure]

@Norbert : no, sadly. 
 Asus didn't found it relevant to publish JB on TF101.
Lionel

Comment 22

[Denis]

I have the same problem on my Asus TF700 with Firefox 15.0 installed today from Google Play: using the azerty hardware keyboard, URL is typed in azerty, forms are typed with querty.

Comment 23

[Makoto Kato [:m_kato]]

Attachment: fix

Comment 24

[Makoto Kato [:m_kato]]

Comment on attachment 673135 [details] [diff] [review]
fix

add white list to support hkb with IME.  Several IMEs don't support on HKB well.

Comment 25

[Chris Peterson [:cpeterson]]

Comment on attachment 673135 [details] [diff] [review]
fix

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

LGTM with a question and a nit:

::: mobile/android/base/GeckoInputConnection.java
@@ +887,5 @@
>          return this;
>      }
>  
>      public boolean onKeyPreIme(int keyCode, KeyEvent event) {
> +        if (hasBuggyHardwareKeyboardLayout() || InputMethods.canUseInputMethodOnHKB(mCurrentInputMethod))

Makoto, does it make sense to merge hasBuggyHardwareKeyboardLayout() (which checks for Asus Transformer tablets running pre-Jelly Bean OS) into canUseInputMethodOnHKB()? Or do you think they are separate concerns that should be kept separate?

::: mobile/android/base/InputMethods.java
@@ +20,5 @@
>      public static final String METHOD_SIMEJI = "com.adamrocker.android.input.simeji/.OpenWnnSimeji";
>      public static final String METHOD_SWYPE = "com.swype.android.inputmethod/.SwypeInputMethod";
>      public static final String METHOD_SWYPE_BETA = "com.nuance.swype.input/.IME";
> +    public static final String METHOD_ATOK = "com.justsystems.atokmobile.service/.AtokInputMethodService";
> +    public static final String METHOD_IWNN = "jp.co.omronsoft.iwnnime.ml/.standardcommon.IWnnLanguageSwitcher";

Please order these new IME names alphabetically by their "METHOD_*" variable name.

Comment 26

[Makoto Kato [:m_kato]]

(In reply to Chris Peterson (:cpeterson) from comment #25)
> Comment on attachment 673135 [details] [diff] [review]
> fix
> 
> Review of attachment 673135 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> LGTM with a question and a nit:
> 
> ::: mobile/android/base/GeckoInputConnection.java
> @@ +887,5 @@
> >          return this;
> >      }
> >  
> >      public boolean onKeyPreIme(int keyCode, KeyEvent event) {
> > +        if (hasBuggyHardwareKeyboardLayout() || InputMethods.canUseInputMethodOnHKB(mCurrentInputMethod))
> 
> Makoto, does it make sense to merge hasBuggyHardwareKeyboardLayout() (which
> checks for Asus Transformer tablets running pre-Jelly Bean OS) into
> canUseInputMethodOnHKB()? Or do you think they are separate concerns that
> should be kept separate?

I will merge this into InputMethods's function.

> 
> ::: mobile/android/base/InputMethods.java
> @@ +20,5 @@
> >      public static final String METHOD_SIMEJI = "com.adamrocker.android.input.simeji/.OpenWnnSimeji";
> >      public static final String METHOD_SWYPE = "com.swype.android.inputmethod/.SwypeInputMethod";
> >      public static final String METHOD_SWYPE_BETA = "com.nuance.swype.input/.IME";
> > +    public static final String METHOD_ATOK = "com.justsystems.atokmobile.service/.AtokInputMethodService";
> > +    public static final String METHOD_IWNN = "jp.co.omronsoft.iwnnime.ml/.standardcommon.IWnnLanguageSwitcher";
> 
> Please order these new IME names alphabetically by their "METHOD_*" variable
> name.

OK.

Comment 27

[Makoto Kato [:m_kato]]

Also, several IMEs will break on physical keyboard.  (ex. HTC touch input.)  I don't know whether non-English model of ASUS uses Android keyboard or not, so I keep ASUS hacks.

Comment 28

[Makoto Kato [:m_kato]]

Attachment: v2

could you review again?

Since I have no all models of ASUS transformer, I keep ASUS hacks.

Comment 29

[Chris Peterson [:cpeterson]]

Comment on attachment 675467 [details] [diff] [review]
v2

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

LGTM!

Do you think we should uplift your fix to Aurora 18? Your fix looks pretty safe and tablet IME users will appreciate it.

Comment 30

[Makoto Kato [:m_kato]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8586bd350875

If possible, we should uplift this for 18.  I have tested several IMEs with bluetooth keyboard.  So I will request approval-aurora after merging into m-c.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8586bd350875

Comment 32

[Makoto Kato [:m_kato]]

Comment on attachment 675467 [details] [diff] [review]
v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): no.  Although IME on xul fennec works even if physical keyboard, on native fennec doesn't.
User impact if declined: IME doesn't work on physical keyboard such as bluetooth keyboard.
Testing completed (on m-c, etc.): landed on mc-c.
Risk to taking this patch (and alternatives if risky): row.  If software keyboard, keyboard event isn't called almost.
String or UUID changes made by this patch: no

Comment 33

[Alex Keybl [:akeybl]]

We can take this into FF18 given the low risk evaluation, how far we are from uplift (3wks), and the fact that we should get specific QA testing around this. If we find a major regression, we'll back this out immediately due to the touchiness of IME changes.

Comment 34

[hashiz]

Not fixed. HTC Desire Z CM9 + FlickWnn(com.pm9.flickwnn)
fixed. HTC Desire Z CM9 + OpenWnn Plus(com.owplus.ime.openwnnplus)

This solution(white list) does not seem a good idea.

Comment 35

[Makoto Kato [:m_kato]]

(In reply to hashiz from comment #34)
> Not fixed. HTC Desire Z CM9 + FlickWnn(com.pm9.flickwnn)
> fixed. HTC Desire Z CM9 + OpenWnn Plus(com.owplus.ime.openwnnplus)
> 
> This solution(white list) does not seem a good idea.

please file new bug

Comment 36

[Makoto Kato [:m_kato]]

hashiz, although I test a lot of IME, a lot of IME doesn't work on physical keyboard well except to Android EditView.  So, example, Opera uses EditView for text box.  Since we doesn't use android framework for control, it is no way to support all IMEs. If Google has CTP test for IME deeply, we will be able to support all...

Comment 37

[Makoto Kato [:m_kato]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3357927c7aca

Comment 38

[hashiz]

> please file new bug

I file new bug.
https://bugzilla.mozilla.org/show_bug.cgi?id=807204