194559 - Entered Japanese text doesn't go back to preedit text by Ctrl+BS

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Description

[Koike Kazuhiko]

Entered Japanese text doesn't go back to preedit text by Ctrl+BS.

Steps to reproduce:
(1) Type <kanji> with IME and hit and Enter key.
(2) Press Ctrl+BS.

Expected result: <kanji> goes back to preedit text.

Actual result: <kanji> is displayed as an another preedit text.

Original report in Bugzilla-jp:
http://bugzilla.mozilla.gr.jp/show_bug.cgi?id=2885

Comment 1

[Koike Kazuhiko]

This bug is reproduced with ATOK14,ATOK15 and WXG 4.03.

Comment 2

[Hidehiro Kozawa]

Mager Japanese IME ATOK recent version and some other IME throws messages
direffent order from MS-IME.
When we push reconvert key, MS-IME send
1. WM_KEYDOWN (bs)
2. WM_CHAR (bs)
3. WM_KEYUP (bs)
4. 1-3  several times (the number of last converted strings).
5. WM_IME_STARTCOMPOSITION preedit string.
but ATOK for win send
1. WM_KEYDOWN (bs) several times (the number of last converted strings).
(not delete last concerted strings yet)
2. WM_IME_STARTCOMPOSITION preedit string.
3. WM_CHAR (bs) several times (the number of last converted strings).
(cause EndComposition and we delete strings of '2.' not last converted strings!)
4. WM_KEYUP (bs) once.
5. next WM_IME_COMPOSITION ...

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 9/19-9/27)]

Attachment: Patch 1

This patch is following functions.

1. Backup the information of IME Composing.
2. Cancel the Current IME session.
3. Create the New IME session that is makes from backup data.

Now, if it is composing when OnChar Event is run,
the IME session is end.
but, then <Ctrl+BS>, the IME session should be none.
because, the VK_BACKSPACE is need for the text that must be deleted.
And the text should come back after KEY_PRESS event.

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 9/19-9/27)]

Comment on attachment 144109 [details] [diff] [review]
Patch 1

crashed on win9x

Comment 5

[Ere Maijala (slow)]

I'm trying to get the patch in bug 197474 in as soon as possible when the trunk
is opened for 1.8a. It seems both bugs touch some same code. Would it be
possible to do the patch here against the nsWindow version patched with the
changes from 197474?

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 9/19-9/27)]

Ere:

This patch don't works fine on Win98(Mozilla crashed),
and some minor problems exists on WinNT.
The cause of a problem isn't known, and I don't have win9x system.
So, this patch's problem cannot immediately be solved.

If all problem is solved, I will demand review again.
At that time, It will rewrite, if there is necessity.
# This bug of may be unable to be fixed...

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 9/19-9/27)]

I will attach the new patch soon.
But new patch having minor problem that is on ATOK only.
The problem is impossible to fix.
When using ImmSetCompositionString with parameter as GCS_COMPSTR, ATOK crash
or doesn't operate well.
That is bug of ATOK.
Because, On WXG, first patch works fine.

For fixing this bug completely, we need to rewrite large code in nsEditor.
But it is not realistic.
So, I tryed to create the patch that is "ad hoc".
The basic concept of new patch is same as comment 3.
But I separate the code as following.
1. The code is used by ATOK.
2. Other code is used by Other IME(However, all IME that this bug doesn't 
   reproduce, that isn't included: e.x., MS-IME).

It isn't good patch, but I don't have better idea.
However, this bug should be fixed.
Because ATOK and WXG is major IME in Japanese people.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 9/19-9/27)]

Attachment: patch v.2

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 9/19-9/27)]

Comment on attachment 144626 [details] [diff] [review]
patch v.2

Please review this patch before rewrite.

# I will rewrite the patch after bug 197474 checking in.

Comment 10

[Ere Maijala (slow)]

Comment on attachment 144626 [details] [diff] [review]
patch v.2

I'm not sure if I'm good in reviewing IME stuff, but I can try. With a quick
glance, there's one question: could we do without LockWindowUpdate? It can
cause horrible window flashing in some circumstances. The locked window behaves
well, but all the others flash. This is most evident when Mozilla is not
running full-screen.

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 9/19-9/27)]

When LockWindowUpdate was removed, the text will flash.
LockWindowUpdate is necessary.

SDK said,
"If the function fails, the return value is zero, indicating that an error
occurred or another window was already locked. "

If we need another way, we need to lock window mechanism when WM_PAINT.

Comment 12

[Ere Maijala (slow)]

Comment on attachment 144626 [details] [diff] [review]
patch v.2

Ok, I don't know the IME so I just have to take your word for it. Technically,
there are just a couple of things:

I really don't like LockWindowUpdate. I wouldn't be seeing its effect, but I
don't want it there. If the flicker is too bad, suppress WM_PAINTs and repaint
when you're done. 

> +        if (!memicmp(imeFileName, "ATOK", 4))

I think memicmp is MSVC++ specific. If so, MinGW doesn't support it and this
would break compiling with it.

One nit:

> +  if (mIMEIsComposing && virtualKeyCode == 0x8) { // BackSpase

Should be BackSpace.

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 9/19-9/27)]

>> +  if (mIMEIsComposing && virtualKeyCode == 0x8) { // BackSpase
> 
> Should be BackSpace.

Why?
If mIMEIsComposing is removed, we will have performance issue.
Because, unnecessary API will run.

Comment 14

[Hidehiro Kozawa]

to Masayuki Nakano
Ere seems to say your 'Typo' problem.
you must check 'nit' with dictionary.

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 9/19-9/27)]

Attachment: patch rv.3(Test Ver.)

Ere:

This patch is not using LockUpdateWindow API.
But, When the text is edited, nsEditor don't use WM_PAINT for repainting the
content.
# printf("Skip Paint.") is not run.
So, I can't stop flicker in WM_PAINT...

Do you have good idea?

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 9/19-9/27)]

Attachment: patch rv.4

This patch do _not_ stop flicker.
However, in many case, the flicker is one time.

We try to test this patch.

Comment 17

[[e-mail]]

(In reply to comment #12)
>(From update of attachment 144626 [details] [diff] [review])
>> +  if (mIMEIsComposing && virtualKeyCode == 0x8) { // BackSpase
>Should be BackSpace.
If you used VK_BACK in the first place the comment would be unnecessary :-P

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 9/19-9/27)]

Attachment: patch rv.5

This is new patch for new architecture(by bug 197474).
The attachment 142468 [details] [diff] [review] cannot fixed this problem.
Because the patch is premised on the message que that having
the pair of WM_KEYDOWN and WM_CHAR or WM_KEYDOWN and WM_SYSCHAR,
but ATOK and WXG's "Kakutei-Undo" is not create the message pair.
In these cases, the message que is following:
 WM_KEYDOWN(VK_BACK) * n
 WM_KEYUP(VK_BACK) * 1 (ATOK only)
 WM_IME_STARTCOMPOSITION(wParam = 0x0, lParam = 0x0)
 WM_IME_COMPOSITION(wParam = 0x0, lParam = 0x1BF)
 WM_CHAR(VK_BACK) * n
 WM_KEYUP(VK_BACK) * 1
In the line of 2992th GetMessage API cannot run, because the message is not
WM_CHAR and WM_SYSCHAR.
The message may be WM_KEYDOWN or WM_KEYUP by ATOK or WXG.

We are testing this patch by Japan testers.
The test build is:
http://www.d-toybox.com/mozilla/testbuilds/bug2885.zip

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 9/19-9/27)]

Adding to Cc.

Momoi-san:
This problem should be tested by other IME user; i.e., Chineses people, and
Korean people.
Do you know that who can test on these system?

I hope that this bug is fixed in 1.8a2.
Because the patch should be tested by many and various IME users.

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 9/19-9/27)]

Attachment: patch rv.5(comment rewrote)

Comment 21

[Ere Maijala (slow)]

Comment on attachment 150255 [details] [diff] [review]
patch rv.5(comment rewrote)

Technically it looks good to me. I trust you know it works correctly. r=ere

Comment 22

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 9/19-9/27)]

Please super-review the patch.

This is major bug and the patch should be tested by many IME users.
So this patch should be checked in before 1.8a2.

Comment 23

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 9/19-9/27)]

Comment on attachment 150255 [details] [diff] [review]
patch rv.5(comment rewrote)

I chage super-reviewer to same as Bug 244334.

Comment 24

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 9/19-9/27)]

Please check in before 1.8a2.

Comment 25

[Ere Maijala (slow)]

Fix checked in for Masayuki.