640884 - Can't delete Korean characters in any input text area

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Tejun Heo]

User-Agent:       Mozilla/5.0 (X11; Linux i686 on x86_64; rv:2.0) Gecko/20100101 Firefox/4.0
Build Identifier: Mozilla/5.0 (X11; Linux i686 on x86_64; rv:2.0) Gecko/20100101 Firefox/4.0

This happens with all the text input areas including the address bar, search bar and any text input areas in the web content.  If I type in, say, "하나둘" and then press backspace, I can delete the character which were still being combined but past that I can't delete anything. If I switch input mode to english and back; then I can delete them.


Reproducible: Always

Steps to Reproduce:
1. With keyboard layout set to 2-bul in Korean, typing "gkskenf" in qwerty layout should give you "하나둘" in Korean with the last glyph '둘' still under contruction.
2. If I then press backspace three times '둘' is deconstructed and deleted
3. On the fourth backspace, '나' should be deleted as a whole but nothing happens.  All further backspaces are ignored.
4. If I switch to English input mode by pressing 'shift+space' and then press backspace again, then it works.
Actual Results:  
Korean characters aren't deleted as expected.

Expected Results:  
They should be deleted.

I'm on openSUSE 11.3.  I have scim installed but am using GTK input module by setting $GTK_IM_MODULE environment variable to "hangul2".  Here are some version information.

libhangul-32bit-0.0.10-3.1.x86_64
imhangul-0.9.14-6.1.x86_64
libhangul-0.0.10-3.1.x86_64
imhangul-32bit-0.9.14-6.1.x86_64
scim-m17n-0.2.2-235.1.x86_64
scim-1.4.7-174.1.x86_64
scim-qtimm-0.9.4-278.1.x86_64
scim-hangul-0.3.2-113.1.x86_64
scim-32bit-1.4.7-174.1.x86_64

Comment 1

[(no longer active)]

Masayuki, this seems IME specific.  Do you have any idea what's going on here?

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Yes, this is an issue of nsGtkIMModule.

Tejun, I have a question. Is this a new regression of Fx4? Or Fx3.6 also has this problem?

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I suspect that your IM doesn't send preedit_end signal to GtkIMContext.

Therefore, we think we're in composition even if the composition string has been empty already.

I guess that if the composition string is empty, gtk_im_context_filter_keypress() returns false but we return true due to composing.
http://mxr.mozilla.org/mozilla-central/source/widget/src/gtk2/nsGtkIMModule.cpp#410

Tejun, would you attach a log of nsGtkIMModule? See bug 636131 comment 10 for getting the log. And please do *only* followings:

1. Launch Fx4
2. Focus searchbar (upper-right in toolbar) by click
3. Switch to Korean IM if needed (if you can, please switch by mouse click on IM toolbar)
4. Type 2 Hangul characters (1st is committed automatically and 2nd is in composition)
5. Type backspace key until you can see this bug
6. Type shift+space key
7. Close the window of Fx4 by clicking X button of the window

Then, I'll get enough hints.

Comment 4

[Tejun Heo]

Attachment: fx.log

Yes, this is new to firefox 4.  I've never experienced the bug in previous versions.

Attaching fx.log.  I had to switch to Korean IME using shift+space.  I don't know how to do that with mouse with gtk IM.  I've never used it.

Thank you.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Tejun:

Thank you the log!

The root cause is the IM doesn't send the preedit_end as I expected. I have another question. I guess that if you pressed an arrow key or a PageUp/PageDown key or a Home/End key or Tab key during composition, then, caret (or focus for Tab key) should have moved after the composition was committed immediately. If so and you could see such behavior on another GTK applications but you couldn't see it on Fx4, please let me know. I think that current our key event handling code is wrong for some Korean and Chinese IMs.

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v1.0

test builds will be here:
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-2eaea1183f3d

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v1.0

Oops, this is the right patch.

Comment 8

[Tejun Heo]

Unfortunately, I can't input any Korean characters with the test build. I can input English fine but once the input is switched to Korean, inputs are just ignored.

As for the caret movement during composition, I can't see any difference between ff4 and other gtk applications.  Any specific scenario you want me to test?

Thanks.

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #8)
> Unfortunately, I can't input any Korean characters with the test build. I can
> input English fine but once the input is switched to Korean, inputs are just
> ignored.

Thank you for your testing. I'll try to look forward another approach...

> As for the caret movement during composition, I can't see any difference
> between ff4 and other gtk applications.  Any specific scenario you want me to
> test?

For example, type a Korean character and press right arrow key. Then, is the composition string committed and move the caret to right? (If other applications work so)

Anyway, would you report this bug to the IM community too? I think that when composition string becomes empty or IM doesn't filter the key event with empty composition string, they should notify the composition end by preedit_end signal. Current behavior is very odd for applications.

# Of course, I continue to look for the workaround.

Comment 10

[Tejun Heo]

(In reply to comment #9)
> For example, type a Korean character and press right arrow key. Then, is the
> composition string committed and move the caret to right? (If other
> applications work so)

Yes, it does and the behavior in that case isn't different from other gtk applications.

> Anyway, would you report this bug to the IM community too? I think that when
> composition string becomes empty or IM doesn't filter the key event with empty
> composition string, they should notify the composition end by preedit_end
> signal. Current behavior is very odd for applications.

Not quite sure where to report.  I'll file a bug report against gtk, I guess.

Thanks.

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #10)
> (In reply to comment #9)
> > For example, type a Korean character and press right arrow key. Then, is the
> > composition string committed and move the caret to right? (If other
> > applications work so)
> 
> Yes, it does and the behavior in that case isn't different from other gtk
> applications.

Good. For risk management, I'll remove the block for this from next patch.

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v2.0

http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-f6beb0868a0c

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v2.0

oops, sorry for the spam.

Comment 14

[Tejun Heo]

The behavior has changed but it still is somewhat buggy.  So, here's what happens now.

* The deletion works.  I can compose two Korean characters and while the focus is on the second character, backspace de-constructs it and when done the first character as a whole is deleted.  Hooray!

* However, after doing that, I can't input further Korean characters unless I switch to English mode and back.  ie. If I type 'dndhkd^H^H^H^H^Hdn' where 'dn' constructs '우' and dhkd '왕', the result should be '우'; however, after the initial '우왕' is deleted, further inputs are ignored until input method is switched.

Thanks.

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v3.0

http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-73a05beba1e7

I think that this patch works fine...

Comment 16

[Tejun Heo]

Yes, it now works as expected.  AFAICS, input now behaves perfectly.  Thanks a lot for your effort.  Much appreciated.

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 519859 [details] [diff] [review]
Patch v3.0

Thank you, Tejun.

Karl, would you review this patch?

The Korean input engine for SCIM doesn't emit preedit_end signal to the context when the composition string becomes empty by backspace key. Then, we don't dispatch compositionend event to Gecko and we are filtering all key events during the composition. Therefore, backspace key events are never dispatched to editor even if the composition has already finished visually.

We should honor the result of gtk_im_context_filter_keypress() even if we're in composition. But it has very big impact, we should land a patch with such approach.

This patch commits the empty composition *only* when the composition string is empty and gtk_im_context_filter_keypress() returns FALSE. This is smaller impact than the ideal approach.

By this, editor finishes the IME transaction with empty string. And the IM module doesn't consume the native key event. Then, the native key event causes dispatching our key event from nsWindow.

I think that this bug is critical for Korean SCIM users. However, this patch may cause some regressions for other IM. So, I think that we should land this patch on trunk, first. And if there is no regression reports, we should land this patch to Gecko2.0 branch too.

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

> But it has very big impact, we should land a patch with such approach.

we should not, I meant.

Comment 19

[Karl Tomlinson (:karlt)]

Comment on attachment 519859 [details] [diff] [review]
Patch v3.0

> nsGtkIMModule::OnCommitCompositionNative(GtkIMContext *aContext,
>                                          const gchar *aUTF8Char)

>+    // If we are not in composition and committing with empty string,
>+    // we need to do nothing.
>+    if (!mIsComposing && (!aUTF8Char || !aUTF8Char[0])) {
>+        return;
>+    }
>+

I assume this is to avoid beginning composition when dispatching an empty text event?
If so, please add a comment stating that.
If not, is there a need for this?

The !aUTF8Char check doesn't make much sense to me because it is only performed when !mIsComposing.
I'm guessing the check is unnecessary, but if it is necessary when !mIsComposing, then it could also be necessary when mIsComposing.

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #19)
> Comment on attachment 519859 [details] [diff] [review]
> Patch v3.0
> 
> > nsGtkIMModule::OnCommitCompositionNative(GtkIMContext *aContext,
> >                                          const gchar *aUTF8Char)
> 
> >+    // If we are not in composition and committing with empty string,
> >+    // we need to do nothing.
> >+    if (!mIsComposing && (!aUTF8Char || !aUTF8Char[0])) {
> >+        return;
> >+    }
> >+
> 
> I assume this is to avoid beginning composition when dispatching an empty text
> event?
> If so, please add a comment stating that.
> If not, is there a need for this?

Not for avoiding beginning composition. The signal is a text input event. Without the added if block, such empty string commit event will causes compositionstart, texttext, compositionend events.

> The !aUTF8Char check doesn't make much sense to me because it is only performed
> when !mIsComposing.
> I'm guessing the check is unnecessary, but if it is necessary when
> !mIsComposing, then it could also be necessary when mIsComposing.

!aUTF8Char is for safety. Anybody can emit the signal to IM context. Then, somebody can send the null pointer to this signal. We shouldn't trust any callers including all IM engines.

If we're in composition, the commit signal with empty string means canceling the composition. So, when mIsComposing && !aUTF8Char, we should dispatch texttext event and compositionend event.

Looks like NS_ConvertUTF8toUTF16(NULL) isn't safe for !aUTF8Char case. So, I think that we should care it.

Comment 21

[Karl Tomlinson (:karlt)]

(In reply to comment #20)
> Without the added if block, such empty string commit event will causes
> compositionstart, texttext, compositionend events.

If you can add a comment stating that, I think it would be helpful.

> !aUTF8Char is for safety. Anybody can emit the signal to IM context. Then,
> somebody can send the null pointer to this signal. We shouldn't trust any
> callers including all IM engines.

Makes sense.

> Looks like NS_ConvertUTF8toUTF16(NULL) isn't safe for !aUTF8Char case. So, I
> think that we should care it.

Yes, please touch up that while you are here.

Comment 22

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v3.1

Comment 23

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

> if (!mIsComposing && (!commitString[0])) {

oops, there is a pair of unnecessary ().

Comment 24

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v3.1.1

carrying over r=karlt.

Comment 25

[(no longer active)]

http://hg.mozilla.org/projects/cedar/rev/139cb01d3a63

Comment 26

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/139cb01d3a63

Comment 27

[Choe Hwanjin]

I've added some code to send "preedit-start" and "preedit-end" signal in imhangul.
http://kldp.net/scm/viewvc.php/?root=imhangul&view=rev&revision=407