314130 - input numbers twice when using Unispim IME at chinese input mode

Status: VERIFIED FIXED

(Core :: Widget: Win32, defect)

Description

[zhangtao]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8) Gecko/20051025 Firefox/1.5
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8) Gecko/20051025 Firefox/1.5

When using Chinese Input Method: Unispim IME (also known as ZiGuang PinIn and 紫光拼音输入法) at FireFox 1.5 Beta or RC1, and in chinese input mode, I input ONE 0...9 number, then I got the number twice at the FireFox input area. 

But in FireFox 1.0.x, this bug dose not exist. 

BTW: this bug also exists at ThunderBird 1.5 beta and RC1.

Reproducible: Always

Steps to Reproduce:
1.Run the Unispim IME, and set it running at chinese input mode
2.Let type focus into any input area, such as Location bar
3.type "1"
4.in the Location bar, we get twice "1"

Actual Results:  
we get twice "1"

Expected Results:  
we should get only one "1"

this bug also exists at ThunderBird 1.5 beta and RC1.

So I guess the bug is in the Gecko Engine?

Comment 1

[zhangtao]

Attachment: unispim IME at chinese input mode

Comment 2

[zhangtao]

Attachment: input "1" at unispim IME, but get twice "1"

Comment 3

[zhangtao]

Attachment: unispim IME version 3.0.0.3045

Web page of unispim is: 

http://www.thunisoft.com/unispim/

Comment 4

[wind li]

Bot only numbers but also interpunctions. Interpunctions are also duplicated while activing Unispim IME.

Comment 5

[John Tiger]

This problem is very troublesome to many Firefox lovers in mainland China, because unispim Chinese input method is much widely used in China.

After Firefox 1.5 was released, many chinese users  are complaining about it on local forum. This problem is preventing some unskilled people from switching their browsers to Firefox. So I strongly suggest that mozilla.org pay more attention on this bug and finally fix it.

Comment 6

[Ben Basson]

-> Event Handling? Not sure where to put this one, but it seems like a bug with sufficient reporters to confirm.

Comment 7

[John Tiger]

here is more details on this subject:
http://forums.mozillazine.org/viewtopic.php?t=341183

Comment 8

[Holly Lee]

I fixed the issue today. The cause of this issue is that unispim chinese input method generates a meanless WM_KEYDOWN/WM_KEYUP message pair for keys when it was activated. Most of these keys are handled by input method except digit characters.  In recent versions of Firefox/Seamonkey, they create a NS_KEY_PRESS event for any asciiKey. 

The fix is very simple. See diff output below.

bash-2.05b$ diff widget/src/windows/nsWindow.cpp widget/src/windows/nsWindow.cpp.orig
3586c3586
<   if (asciiKey && !(gotMsg && msg.message == WM_KEYUP))
---
>   if (asciiKey)

Hope somebody having commit provileges can merge it into mainstream. 

Comment 9

[wind li]

(In reply to comment #8)
Hi, Holly
  Two comment for you:
1.
> bash-2.05b$ diff widget/src/windows/nsWindow.cpp
> widget/src/windows/nsWindow.cpp.orig
It should be better if you use: diff -u widget/src/windows/nsWindow.cpp.orig widget/src/windows/nsWindow.cpp
2. Did you try other IMEs after apply your change?   

Comment 10

[Holly Lee]

(In reply to comment #9)

1) OK. 

bash-2.05b$ diff -u widget/src/windows/nsWindow.cpp widget/src/windows/nsWindow.cpp.orig
--- widget/src/windows/nsWindow.cpp     2006-02-10 11:22:20.000000000 +0800
+++ widget/src/windows/nsWindow.cpp.orig        2006-02-10 11:24:30.000000000 +0800
@@ -3578,7 +3578,7 @@
       }
   }

-  if (asciiKey && !(gotMsg && msg.message == WM_KEYUP))
+  if (asciiKey)
     DispatchKeyEvent(NS_KEY_PRESS, asciiKey, 0, aKeyData, extraFlags);
   else
     DispatchKeyEvent(NS_KEY_PRESS, 0, virtualKeyCode, aKeyData, extraFlags);


2) I haven't test other IMEs as I don't have them in my windows box. I think it is better that other users can test it. In theory the patch won't conflict with other IMEs since it just prevents event handler from creating a NS_KEY_PRESS event incorrectly from an abnormal WM_KEYDOWN/WM_KEYUP pair without WM_CHAR for asciiKey. 

3) I compiled two binaries for FireFox 1.5.0.1 and SeaMonkey 1.0. Any user has interests can download them and have a try.

For SeaMonkey 1.0:  http://www.jsfsoft.com/temp/gkwidget.zip
    
    Unzip it and put the uncompressed file gkwidget.dll into your _SystemVolume_\Program Files\Common Files\mozilla.org\GRE\1.8.0.1_xxxx\components to overwrite old file with same name, where the _SystemVolume_ is disk that you installed your windows system.

For FireFox 1.5.0.1:  http://www.jsfsoft.com/temp/firefox.zip
    
    Unzip it and put the uncompressed file firefox.exe into your FireFox installation directory to overwrite your original firefox.exe. 

Comment 11

[John Tiger]

I have test the binary firefox file that compiled by Holly.Lee, it was fine. The problem really desapperaed. 

You've done a great job, Holly, Thank you.

I also made a test on microsoft pinyin imput method, it worked well, just as Lee estimated.

Comment 12

[John Tiger]

But a new problem appears.

The extension Tab preview now stops to work. Appearently it's related to the new   modification made by Lee. I feel it's very strange, but can not figure out why.

Comment 13

[Holly Lee]

(In reply to comment #12)
> But a new problem appears.
> 
> The extension Tab preview now stops to work. Appearently it's related to the
> new   modification made by Lee. I feel it's very strange, but can not figure
> out why.
> 

What is "extension Tab preview"? If so, I may put more codes into patch to distingulish message sending windows so that this patch can be a special work around for unispim IME. It is easy, too.

Comment 14

[Holly Lee]

(In reply to comment #13)
> (In reply to comment #12)
> > But a new problem appears.
> > 
> > The extension Tab preview now stops to work. Appearently it's related to the
> > new   modification made by Lee. I feel it's very strange, but can not figure
> > out why.
> > 
> 
> What is "extension Tab preview"? If so, I may put more codes into patch to
> distingulish message sending windows so that this patch can be a special work
> around for unispim IME. It is easy, too.
> 

Oh I found it. In the page of this extension, it mentioned "Please note that if you build your own Firefox from source, you'll need to have --enable-canvas in your build configuration". I just didn't add this build configuration into my compilation. It is not the issue of patch :-)

I will make a new compilation next week.

Comment 15

[wind]

You can use about:buildconfig to find the offical release build config. But please be sure to remove --enable-official-branding if you don't have the permit to use the firofx brand.

Comment 16

[Holly Lee]

Updated.

Now firefox.zip was compiled as same config as its official distribution excluding --enable-official-branding. The tab preview extension works fine.

Comment 17

[John Tiger]

I reinstalled TAB PREVIEW, it works. Wonderful. I also tested TAB CATALOG, which also used canvas, it also works fine. Now I think Lee's modification contains no problem.

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Holly:

We need to fix on Trunk in first time, and if the patch doesn't have any regressions, we can fix the bug on 1.8.x branch too. You should create the patch for Trunk and you should attach the patch as attachment.(use |cvs diff -u8p|)

Comment 19

[Holly Lee]

Attachment: Patch for Chinese unispim IME compatibility

This is patch on trunk for Chinese unispim IME compatibility.

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Comment on attachment 217530 [details] [diff] [review]
Patch for Chinese unispim IME compatibility

> +  if (numOfUniChars && !(gotMsg && msg.message == WM_KEYUP))

I think that this patch has a large risk. I think that we should check more conditions if we use this way.

But I have a question. This bug happen in composition? or not so?
# OnKeyDown is not called while compositioning...

Comment 21

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: test patch

Holly:

Would you test this patch? I think that this fixes the bug and the risk is lower than your patch.

I think that the switch statement assumes that is the Ctrl or Alt key is pressed always, but your case isn't so. We may not need to rise event in this case.

Comment 22

[Holly Lee]

(In reply to comment #20)
> (From update of attachment 217530 [details] [diff] [review] [edit])
> > +  if (numOfUniChars && !(gotMsg && msg.message == WM_KEYUP))
> 
> I think that this patch has a large risk. I think that we should check more
> conditions if we use this way.
> 
The reason that I patched codes this way is that I don't know anything about Japanese IME and don't want to modify codes of their special processing. Therefore I just filtered one special case (a WM_KEYUP followed WM_KEYDOWN).

> But I have a question. This bug happen in composition? or not so?
> # OnKeyDown is not called while compositioning...
> 
You are right. It is not in composition mode. 

Comment 23

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Comment on attachment 217692 [details] [diff] [review]
test patch

Kimura-san tested this patch and he confirmed this patch fixes this bug.
We should go this.

Comment 24

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

(In reply to comment #22)
> (In reply to comment #20)
> > (From update of attachment 217530 [details] [diff] [review] [edit] [edit])
> > > +  if (numOfUniChars && !(gotMsg && msg.message == WM_KEYUP))
> > 
> > I think that this patch has a large risk. I think that we should check more
> > conditions if we use this way.
> > 
> The reason that I patched codes this way is that I don't know anything about
> Japanese IME and don't want to modify codes of their special processing.
> Therefore I just filtered one special case (a WM_KEYUP followed WM_KEYDOWN).

There are many input assistant applications on the world, I think that your patch may break the messeages from some applications. (Of course, my patch may have same risk.) Therefore, we should fix some difference from Fx1.0(Gecko1.7), my patch do so.

On Fx1.0(Gecko1.7), we didn't dispatch KeyPress event when Ctrl and Alt keys are not pressed and next message is not WM_(SYS|DEAD)CHAR. I think that we should suppress to dispatch event at this time.

Comment 25

[Ere Maijala (slow)]

Comment on attachment 217692 [details] [diff] [review]
test patch

I'm a bit hesitant, but let's test it.

Comment 26

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Comment on attachment 217692 [details] [diff] [review]
test patch

Thanks, Ere.

Roc:
Please sr this. See comment 24 for the detail.

Comment 27

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

checked-in, thanks.

Holly:

Could you test this fix on Trunk nigtly build?

Comment 28

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

We don't have the report of regression.
https://bugzilla.mozilla.org/buglist.cgi?query_format=advanced&short_desc_type=allwordssubstr&short_desc=&product=Core&product=Firefox&product=Mozilla+Application+Suite&product=Other+Applications&product=Thunderbird&product=Toolkit&version=Trunk&long_desc_type=substring&long_desc=&bug_file_loc_type=allwordssubstr&bug_file_loc=&status_whiteboard_type=allwordssubstr&status_whiteboard=&keywords_type=allwords&keywords=regression&resolution=DUPLICATE&resolution=---&op_sys=Windows+2000&op_sys=Windows+XP&op_sys=Windows+Server+2003&emailassigned_to1=1&emailtype1=exact&email1=&emailassigned_to2=1&emailreporter2=1&emailqa_contact2=1&emailtype2=exact&email2=&bugidtype=include&bug_id=&votes=&chfieldfrom=2006-04-24&chfieldto=Now&chfield=%5BBug+creation%5D&chfieldvalue=&cmdtype=doit&order=Reuse+same+sort+as+last+time&field0-0-0=noop&type0-0-0=noop&value0-0-0=

If anybody verify this bug on trunk, I'll work on 1.8 branch and 1.8.0 branch too.

Comment 29

[redkalk]

(In reply to comment #28)
> We don't have the report of regression.
> https://bugzilla.mozilla.org/buglist.cgi?query_format=advanced&short_desc_type=allwordssubstr&short_desc=&product=Core&product=Firefox&product=Mozilla+Application+Suite&product=Other+Applications&product=Thunderbird&product=Toolkit&version=Trunk&long_desc_type=substring&long_desc=&bug_file_loc_type=allwordssubstr&bug_file_loc=&status_whiteboard_type=allwordssubstr&status_whiteboard=&keywords_type=allwords&keywords=regression&resolution=DUPLICATE&resolution=---&op_sys=Windows+2000&op_sys=Windows+XP&op_sys=Windows+Server+2003&emailassigned_to1=1&emailtype1=exact&email1=&emailassigned_to2=1&emailreporter2=1&emailqa_contact2=1&emailtype2=exact&email2=&bugidtype=include&bug_id=&votes=&chfieldfrom=2006-04-24&chfieldto=Now&chfield=%5BBug+creation%5D&chfieldvalue=&cmdtype=doit&order=Reuse+same+sort+as+last+time&field0-0-0=noop&type0-0-0=noop&value0-0-0=
> 
> If anybody verify this bug on trunk, I'll work on 1.8 branch and 1.8.0 branch
> too.
> 

1.8.0 branch has this bug

Comment 30

[redkalk]

sorry
1.6a1 has this bug

Comment 31

[Ginn Chen]

(In reply to comment #30)
> sorry
> 1.6a1 has this bug
> 

The latest trunk should be 3.0a1 now.

Comment 32

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

nobody verify this fix on latest trunk, I cannot go to next step.

Comment 33

[redkalk]

the bug exists in 3.0a1 

Comment 34

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

(In reply to comment #33)
> the bug exists in 3.0a1 

What is your build ID? Would you tell me what key was pressed in its time?

Comment 35

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Ah, are you using numpad?

Comment 36

[redkalk]

(In reply to comment #35)
> Ah, are you using numpad?
> 
yes, when I use numpad and input numbers, the bug exists
when i use keybroad to input number, it works well

Comment 37

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

O.K. thank you for your test.

Comment 38

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: checking numpad key

Comment 39

[Patton]

I downloaded the latest trunk from http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-trunk/
I think the bug is fixed in this trunk, and the numpad key is well too
Thanks all 

Comment 40

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

(In reply to comment #39)
> I downloaded the latest trunk from
> http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-trunk/
> I think the bug is fixed in this trunk, and the numpad key is well too

The new patch doesn't check in to trunk, yet.

Comment 41

[Ere Maijala (slow)]

Comment on attachment 221790 [details] [diff] [review]
checking numpad key

The first test is wrong, it should be >=. Also, could you use the constants (VK_NUMPAD0 etc.)?

Comment 42

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: checking numpad key rv2.0

> The first test is wrong, it should be >=.

What is this comment? The previous patch has following conditions.

0x60 <= aVirtualKey && aVirtualKey <= 0x6F

aVirtualKey is between 0x60 and 0x6F, of course, it includes both numbers.

Comment 43

[Ere Maijala (slow)]

Comment on attachment 222719 [details] [diff] [review]
checking numpad key rv2.0

Yeah, my bad. Too early in the morning or something, sorry about that.

Comment 44

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Comment on attachment 222719 [details] [diff] [review]
checking numpad key rv2.0

O.K. Thank you, Ere.

Comment 45

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

checked-in, thank you.

redkalk:

Could you test this patch on trunk build?
If you will verify it, I'll start to work for 1.8 branch too.

Comment 46

[redkalk]

(In reply to comment #45)
> checked-in, thank you.
> redkalk:
> Could you test this patch on trunk build?
> If you will verify it, I'll start to work for 1.8 branch too.
I didn't find any problem in the lastest trunk.

Comment 47

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Patch rv1.0 for 1.8 branch

Ere:

I think that this patch means same logic of trunk. Would you review this?

Comment 48

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

(In reply to comment #46)
> (In reply to comment #45)
> > checked-in, thank you.
> > redkalk:
> > Could you test this patch on trunk build?
> > If you will verify it, I'll start to work for 1.8 branch too.
> I didn't find any problem in the lastest trunk.
> 

Thank you redkalk. When I'll check in the new patch to 1.8 branch, please test it on nightly build too.

thanks.

Comment 49

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

-> v. (per comment 46)

Comment 50

[Ere Maijala (slow)]

Comment on attachment 224065 [details] [diff] [review]
Patch rv1.0 for 1.8 branch

Fix the comment to say "...we should not the send KeyPress event.". I can't test this, but it seems ok, so give it go.

Comment 51

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Patch rv1.1 for 1.8 branch

Thank you, Ere.

Roc: Would you sr this?

Comment 52

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

checked-in to 1.8 branch.

redkalk:

Please test the fix on 1.8 branch on after tomorrow build.
If it's verified on 1.8 branch too and if we don't have any regression report until 1 week, should we land this to 1.8.0 branch too? I think that this bug is very serious regression for Chinese people, right?

Comment 53

[redkalk]

I download the files from here
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla1.8/
and it works well
thank you

Comment 54

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Thank you, redkalk.

Comment 55

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

I think that we should fix this for Chinese people. Because I can look this bug is very serious regression for the people.

Comment 56

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Comment on attachment 224339 [details] [diff] [review]
Patch rv1.1 for 1.8 branch

I cannot find any regressions by this patch in this week. I think that this bug is very serious for Chinese people. We should go to stable branch too.

Comment 57

[Daniel Veditz [:dveditz]]

Comment on attachment 224339 [details] [diff] [review]
Patch rv1.1 for 1.8 branch

approved for 1.8.0 branch, a=dveditz for drivers

Comment 58

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

checked-in to 1.8.0 branch too.

redkalk:

Would you check this fix on 1.8.0 branch?
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla1.8.0/
# after 15-Jun build.

Comment 59

[Patton]

I downloaded the nightly build (Jun 15) from
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla1.8.0/
I believe this bug is fixed.

Thanks all!

Comment 60

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Thank you Patton for your testing.