Bug 32290 – [GTK] can't input korean into lower all input box except adress input box in webkit gtk launcher

• Component: WebKitGTK
• Status: RESOLVED FIXED

[sandy paulanskaya]

can`t input korean into lower all input box except for adress input box in webkit gtk launcher after recent svn update.
prior version worked well with my scim-bridge and scim-hangul in korean input.
first i report this bug to midori Bug Tracker(http://www.twotoasts.de/bugs/index.php?do=details&task_id=598).
i`m using gtk 2.14.4 with webkit 1.1.17(recent svn version compiled by me) on ubuntu 8.10.

you can see my problems in screenshots.
midori screenshot(http://www.twotoasts.de/bugs/index.php?getfile=811)
gtk launcher screenshot(http://www.twotoasts.de/bugs/index.php?getfile=826)

what cause this weird korean input failure?
my linux system(google chrom and prior version midori before recent webkit gtk update )
works well with current scim-bridge and scim-hangul.

of course there is no problem in google chrome.
google chrome screenshot(http://www.twotoasts.de/bugs/index.php?getfile=810)

[Duy Nguyen]

I will try to reproduce but I admit I haven't used scim before. Just for the record, how did you do scim setup (any env variable setting, or file config, or it comes from Ubuntu default config)?

[Duy Nguyen]

Confirmed preedit characters do not show up (only shown after pressing space, I guess that would stop preedit mode). Tested with GtkLauncher r52555 and GTK_IM_MODULE=scim-bridge

[Duy Nguyen]

It seems that scim generates preedit and commit signals properly. But for some reason, the code that commits string in EditorClient::handleKeyboardEvent() is only executed when I press whitespace key. As the result, string is queued up in pendingComposition but never gets processed.

Martin, any idea how a keypressEvent is created? Is it possible that scim eats all key press and release so keypressEvent is never created?

[Martin Robinson]

After looking into this, the situation as I understand it is...

For non-SCIM input: "commit" and "preedit-changed" signals are fired during keydown events. The correct behavior in this case is to wait for the keyup event to modify the editable area (the current code).

For SCIM input: SCIM eats keypresses after the preedit starts. After this point, the editable area needs to be updated during signal handling (the old behavior of this code), because no further key events will be passed to WebKit for this composition.

The second case is currently broken. I'll try making a patch for this issue. My approach will be to change the editable area immediately only if we are not in the middle of handling a keydown event, otherwise the code will wait until keyup event to commit changes.

As an aside, it would be nice to establish some tests for this behavior, but I'm unsure of the best way given the intricacies of SCIM configuration.

[Bert Wasbeer]

I'd like to add that the same problem occurs with IBus, which has replaced SCIM as the default input method (for Korean, Chinese etc.) in distributions such as Ubuntu and Fedora. 

I'm not really sure where/how to find the version of Webkit. I did search for it, but couldn't find it unfortunately, so I'll go with the numbering used in the Ubuntu repositories.

With libwebkit 1.02-2 1.1.15 from the default Ubuntu Karmic repositories, both SCIM and IBus work fine. I can type English (or any European language) as well as Chinese (or Korean, Japanese, etc).

With versions 1.1.16, 1.1.17 and 1.1.18 from the Webkit Team PPA for Ubuntu, neither SCIM nor IBus works. I can type English, but not Chinese.

With version 1.1.19 from the Webkit Team PPA however, SCIM is also not usable when I want to write in English, because every letter appears in duplicate. When for instance I type "hello" I get "hheelloo" on the screen. When I press backspace, it also removes two letters at a time. When I switch to Chinese input in SCIM, both the pinyin and Chinese characters appear on the screen. For instance, when I type "nihao" it gives me "nihao你好" where it should only give "你好". IBus works as with versions 1.1.16 through 1.1.18: English is fine, Chinese does not work at all.

With both SCIM and IBus I used the default configurations that come with Ubuntu Karmic.

Hope this can be of help.

[Martin Robinson]

Created attachment 47782 [details]
Proposed patch

I've attached a proposed patch for this issue. It'd be great if those having problems who can compile WebKit give this a shot and let me know how it works. I'll be able to put this up for review once I get a good bunch of unit tests to verify this fix and prevent regressions in the future.

Here is what I've changed.
- Handle the case where the IMContext sends signals when not handling WebKit key events.
- Handle the case where the IMContext sends more than one "committed" signal in a row.
- Properly handle IME in two WebView simultaneously.

[Gustavo Noronha (kov)]

*** Bug 34394 has been marked as a duplicate of this bug. ***

[Gustavo Noronha (kov)]

Comment on attachment 47782 [details]
Proposed patch

 67     gchar* newPreedit = 0;

You may want to use GOwnPtr for these, I guess?

 60         void updatePendingComposition(const gchar*);
 61         void updatePendingPreedit(const gchar*);

Just make these take const char*, there's no good reason to use gchar* unless it's API we're exposing (so keeping consistency with what's used on most APIs is useful). I understand the gchar typedef is required for the GOwnPtr, though.

[Martin Robinson]

Created attachment 48121 [details]
Proposed patch (rev2)

I've attached another patch which incorporates kov's suggestions and also seems to fix the behavior of the xim IM module. While there are still some patches forthcoming to improve IME support, I think this one will fix most issues. After I finish writing some tests to go with this patch, I'll put it up for review.

[sandy paulanskaya]

webkit take too long to compile on my celeron 1.7 Ghz.(almost all day long).
your old patch has been applied to svn revision 54136 and worked on me.
(midori, arora ,gtk launcher) 
screenshot : http://yfrog.com/5escreenshot087gp

thanks for patch.i`ll try new rev2 patch next time :)

$ svn info
Path: .
URL: http://svn.webkit.org/repository/webkit/trunk
Repository Root: http://svn.webkit.org/repository/webkit
Repository UUID: 268f45cc-cd09-0410-ab3c-d52691b4dbfc
Revision: 54136
Node Kind: directory
Schedule: normal
Last Changed Author: [e-mail]
Last Changed Rev: 54136
Last Changed Date: 2010-02-02 01:22:39 +0900 (Tue, 02 Feb 2010)

[Duy Nguyen]

Applied rev2 on top of r54341, ran testkeyevents and got:

/webkit/keyevent/textfield: OK
/webkit/keyevent/buttons: OK
/webkit/keyevent/link: OK
/webkit/keyevent/textfield: OK
/webkit/keyevent/textfield: Segmentation fault

(gdb) bt
#0  0xb5529129 in g_type_check_instance_cast ()
   from /usr/lib/libgobject-2.0.so.0
#1  0x080496d7 in test_scimlike_input_window_object_cleared_cb ()
#2  0xb797fd16 in webkit_marshal_VOID__OBJECT_POINTER_POINTER ()
   from /home/pclouds/w/webkit/.libs/libwebkit-1.0.so.2
#3  0xb55092eb in g_closure_invoke () from /usr/lib/libgobject-2.0.so.0
#4  0xb551a388 in signal_emit_unlocked_R () from /usr/lib/libgobject-2.0.so.0
#5  0xb551b72f in g_signal_emit_valist () from /usr/lib/libgobject-2.0.so.0
#6  0xb551ecdc in g_signal_emit_by_name () from /usr/lib/libgobject-2.0.so.0
#7  0xb79366a8 in WebKit::FrameLoaderClient::dispatchDidClearWindowObjectInWorld () from /home/pclouds/w/webkit/.libs/libwebkit-1.0.so.2
#8  0xb757b39d in WebCore::FrameLoader::dispatchDidClearWindowObjectInWorld ()
   from /home/pclouds/w/webkit/.libs/libwebkit-1.0.so.2

[Duy Nguyen]

*** Bug 30389 has been marked as a duplicate of this bug. ***

[Martin Robinson]

I accidentally included my half-done unit test with the patch, sorry about the crash. :)

[Martin Robinson]

Created attachment 48551 [details]
Patch for this issue (rev3)

I've attached a patch for this issue which streamlines the fixes from the previous iteration, calls focus_in and focus_out at the appropriate times, and properly handles Ctrl+U Unicode composition. After the review cycles are over, I don't mind committing this myself, I'm eager to learn the process. :)

[WebKit Review Bot]

Attachment 48551 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp:96:  Use 0 instead of NULL.  [readability/null] [5]
Ignoring "WebKit/gtk/tests/testkeyevents.c": this file is exempt from the style guide.
Total errors found: 1


If any of these errors are false positives, please file a bug against check-webkit-style.

[Martin Robinson]

I've filed a bug for webkit-style and g_strconcat here: https://bugs.webkit.org/show_bug.cgi?id=34834

[Oliver Hunt]

Comment on attachment 48551 [details]
Patch for this issue (rev3)

You may want to consider adding DRT support for input methods (a la /platform/mac/editing/input/text-control-ime-input.html i think).

The patch looks good to me, but i'm not really a gtk dev -- if xan or some other gtk person thinks it's okay i'm happy to r+ this patch

[Xan Lopez]

Comment on attachment 48551 [details]
Patch for this issue (rev3)

We did a review of this on IRC, Martin will post a new patch soon.

[Martin Robinson]

Created attachment 48709 [details]
Patch for this issue (rev4)

I've attached an updated patch with the following changes:
    - Fixed the case where a page calls preventDefault() on some keypress events.
    - If a key release event is filtered and there is no pending composition, block the event.
    - Added a few comments which describe the timeline of key events using the 'simple' context.
    - Add two more sets of unit tests.

[WebKit Review Bot]

Attachment 48709 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp:96:  Use 0 instead of NULL.  [readability/null] [5]
WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp:557:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Ignoring "WebKit/gtk/tests/testkeyevents.c": this file is exempt from the style guide.
Total errors found: 2


If any of these errors are false positives, please file a bug against check-webkit-style.

[Martin Robinson]

Created attachment 48710 [details]
Patch for this issue (rev5)

The style bot is a harsh mistress. I've updated the patch to fix the style nit.

[WebKit Review Bot]

Attachment 48710 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp:96:  Use 0 instead of NULL.  [readability/null] [5]
Ignoring "WebKit/gtk/tests/testkeyevents.c": this file is exempt from the style guide.
Total errors found: 1


If any of these errors are false positives, please file a bug against check-webkit-style.

[sandy paulanskaya]

(In reply to comment #21)
> Created an attachment (id=48710) [details]
> Patch for this issue (rev5)
> 
> The style bot is a harsh mistress. I've updated the patch to fix the style nit.

thanks for new v5 patch

but i got some error about gtk version 2,14.

 CC     WebKit/gtk/tests/Programs_unittests_testkeyevents-testkeyevents.o
WebKit/gtk/tests/testkeyevents.c: In function ‘test_xim_load_status_cb’:
WebKit/gtk/tests/testkeyevents.c:258: warning: implicit declaration of function ‘gtk_im_multicontext_get_context_id’
WebKit/gtk/tests/testkeyevents.c:258: warning: initialization makes pointer from integer without a cast
WebKit/gtk/tests/testkeyevents.c:259: warning: implicit declaration of function ‘gtk_im_multicontext_set_context_id’
  CCLD   Programs/unittests/testkeyevents
WebKit/gtk/tests/Programs_unittests_testkeyevents-testkeyevents.o: In function `test_xim_load_status_cb':
testkeyevents.c:(.text+0x289): undefined reference to `gtk_im_multicontext_get_context_id'
testkeyevents.c:(.text+0x2ae): undefined reference to `gtk_im_multicontext_set_context_id'
testkeyevents.c:(.text+0x406): undefined reference to `gtk_im_multicontext_set_context_id'
collect2: ld returned 1 exit status
make[1]: *** [Programs/unittests/testkeyevents] error 1

maybe gtk_im_multicontext_get_context_id is gtk 2.16 function?
i want some ifdef statements for my gtk 2.14

. :)

[Martin Robinson]

Created attachment 49120 [details]
Patch for this issue (rev6)

Thanks for the information. I've uploaded a new version of this patch which wraps the calls to gtk_im_multicontext_{g,s}et_context_id in an #if GTK_CHECK_VERSION(2, 16, 0).

[sandy paulanskaya]

Applied rev2 on top of r54959, ran testkeyevents and got:

/webkit/keyevents/event-textinput: OK
/webkit/keyevents/event-buttons: OK
/webkit/keyevents/event-link: OK
/webkit/keyevent/event-div: OK
/webkit/keyevent/ime-textinput: **
ERROR:WebKit/gtk/tests/testkeyevents.c:196:test_ime_load_status_cb: assertion failed: (element_text_equal_to(context, "a"))
Aborted

(gdb) r
Starting program: /media/sdc1/WebKit/Programs/unittests/testkeyevents 
[Thread debugging using libthread_db enabled]
/webkit/keyevents/event-textinput: [New Thread 0xb4f35b90 (LWP 9310)]
[New Thread 0xb460db90 (LWP 9311)]
[New Thread 0xb3e0db90 (LWP 9312)]
[Thread 0xb3e0db90 (LWP 9312) exited]
OK
/webkit/keyevents/event-buttons: OK
/webkit/keyevents/event-link: OK
/webkit/keyevent/event-div: OK
/webkit/keyevent/ime-textinput: **
ERROR:WebKit/gtk/tests/testkeyevents.c:196:test_ime_load_status_cb: assertion failed: (element_text_equal_to(context, "a"))

Program received signal SIGABRT, Aborted.
0xb68a5a16 in *__GI_raise (sig=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:64
64	../nptl/sysdeps/unix/sysv/linux/raise.c: No such file or directory.
	in ../nptl/sysdeps/unix/sysv/linux/raise.c
(gdb) bt full
#0  0xb68a5a16 in *__GI_raise (sig=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:64
        resultvar = <value optimised out>
        pid = -1231314956
        selftid = 9288
#1  0xb68a7318 in *__GI_abort () at abort.c:88
        act = {__sigaction_handler = {sa_handler = 0x1, sa_sigaction = 0x1}, sa_mask = {__val = {136609064, 3065769893, 136075184, 1632, 
              3221214080, 3221214068, 134538008, 3221214036, 4, 3221214144, 3064812272, 3063820273, 134536992, 0, 0, 1, 3221214008, 
              3062794188, 3063656768, 136073136, 136073136, 3064524788, 136073136, 1, 3221214040, 3062792100, 136073136, 135126960, 147, 
              3064524788, 3064524788, 147}}, sa_flags = -1073753112, sa_restorer = 0x806f7e0}
        sigs = {__val = {32, 0 <repeats 31 times>}}
#2  0xb6a38d13 in g_assertion_message () from /usr/lib/libglib-2.0.so.0
No symbol table info available.
#3  0xb6a3933d in g_assertion_message_expr () from /usr/lib/libglib-2.0.so.0
No symbol table info available.
#4  0x080499dc in test_ime_load_status_cb ()
No locals.
#5  0xb6aae55a in g_cclosure_marshal_VOID__PARAM () from /usr/lib/libgobject-2.0.so.0
No symbol table info available.
#6  0xb6a9ff4b in g_closure_invoke () from /usr/lib/libgobject-2.0.so.0
No symbol table info available.
#7  0xb6ab7697 in ?? () from /usr/lib/libgobject-2.0.so.0
No symbol table info available.
#8  0x081c68a0 in ?? ()
No symbol table info available.
#9  0x00000000 in ?? ()
No symbol table info available.
(gdb) i r
eax            0x0	0
ecx            0x2448	9288
edx            0x6	6
ebx            0x2448	9288
esp            0xbfffd2b8	0xbfffd2b8
ebp            0xbfffd2c0	0xbfffd2c0
esi            0x806f7e0	134674400
edi            0xb69b9ff4	-1231314956
eip            0xb68a5a16	0xb68a5a16 <*__GI_raise+70>
eflags         0x200206	[ PF IF ID ]
cs             0x73	115
ss             0x7b	123
ds             0x7b	123
es             0x7b	123
fs             0x0	0
gs             0x33	51

[sandy paulanskaya]

Applied rev6  not "rev2 "on top of r54959. sorry.

[Martin Robinson]

Sandy, what version of GTK+ 2.14 are you on? I'm having trouble reproducing the test failure, even while running on 2.14.

[sandy paulanskaya]

$ more /usr/lib/pkgconfig/glib-2.0.pc
prefix=/usr
exec_prefix=${prefix}
libdir=${exec_prefix}/lib
includedir=${prefix}/include

glib_genmarshal=glib-genmarshal
gobject_query=gobject-query
glib_mkenums=glib-mkenums

Name: GLib
Description: C Utility Library
Version: 2.21.3
Libs: -L${libdir} -lglib-2.0 
Libs.private: 
Cflags: -I${includedir}/glib-2.0 -I${libdir}/glib-2.0/include 

$ more /usr/lib/pkgconfig/gtk+-2.0.pc
prefix=/usr
exec_prefix=${prefix}
libdir=/usr/lib
includedir=${prefix}/include
target=x11

gtk_binary_version=2.10.0
gtk_host=i486-pc-linux-gnu

Name: GTK+
Description: GTK+ Graphical UI Library (${target} target)
Version: 2.14.4
Requires: gdk-${target}-2.0 atk cairo gio-2.0 pangoft2
Libs: -L${libdir} -lgtk-${target}-2.0 
Cflags: -I${includedir}/gtk-2.0

[Benjamin Otte]

Might bug 35847 be related?

[Benjamin Otte]

I just tested the patch against the Compose key issue in bug 35847 with svn 65508 and it does not fix it. Unlike before, it now consumes the keypresses, but it does not output the final composed key. So instead of "<3", I get nothing.

[sandy paulanskaya]

rev 55801 have not now wtf/gtk/GOwnPtr.h.
so compile failed.


CXX    WebKit/gtk/WebCoreSupport/libwebkit_1_0_la-EditorClientGtk.lo
In file included from WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp:25:
WebKit/gtk/WebCoreSupport/EditorClientGtk.h:38:29: error: wtf/gtk/GOwnPtr.h: No such file or directory
In file included from WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp:25:
WebKit/gtk/WebCoreSupport/EditorClientGtk.h:132: error: ISO C++ forbids declaration of ‘GOwnPtr’ with no type
WebKit/gtk/WebCoreSupport/EditorClientGtk.h:132: error: expected ‘;’ before ‘<’ token
WebKit/gtk/WebCoreSupport/EditorClientGtk.h: In member function ‘void WebKit::EditorClient::clearPendingComposition()’:
WebKit/gtk/WebCoreSupport/EditorClientGtk.h:61: error: ‘m_pendingComposition’ was not declared in this scope
WebKit/gtk/WebCoreSupport/EditorClientGtk.h: In member function ‘bool WebKit::EditorClient::hasPendingComposition()’:
WebKit/gtk/WebCoreSupport/EditorClientGtk.h:62: error: ‘m_pendingComposition’ was not declared in this scope
WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp: In member function ‘void WebKit::EditorClient::updatePendingComposition(const gchar
*)’:
WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp:93: error: ‘m_pendingComposition’ was not declared in this scope
WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp: In member function ‘virtual void WebKit::EditorClient::handleKeyboardEvent(WebCore:
:KeyboardEvent*)’:
WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp:499: error: ‘m_pendingComposition’ was not declared in this scope
WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp: In member function ‘virtual void WebKit::EditorClient::handleInputMethodKeydown(Web
Core::KeyboardEvent*)’:
WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp:557: error: ‘m_pendingComposition’ was not declared in this scope
make[1]: *** [WebKit/gtk/WebCoreSupport/libwebkit_1_0_la-EditorClientGtk.lo] error 1
make[1]: Leaving directory `/media/sdc1/WebKit'
make: *** [all] error 2

[Martin Robinson]

Created attachment 50660 [details]
Patch for this issue (rev7)

I've updated this patch against ToT as well as initializing the m_treatContextCommitAsKeyEvent member, which I hope should fix the failure that Sandy is seeing.

[sandy paulanskaya]

applied rev7 patch on svn revision 55967.
works great. have passed all key event tests.
thanks.

[Martin Robinson]

Created attachment 50772 [details]
Patch for this issue (rev8)

I've attached a new patch for this issue which includes the following improvements:
    * Fixes for memory corruption issues in testkeyevents.c
    * In webkit_web_view_focus_out_event, only call gtk_im_context_focus_out if the IM context is still valid.
    * Fixes for entering characters using the Amharic SCIM module and an expanded comment explaining this.

[WebKit Review Bot]

Attachment 50772 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp:558:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp:559:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WARNING: File exempt from style guide. Skipping: "WebKit/gtk/tests/testkeyevents.c"
Total errors found: 2 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Martin Robinson]

Created attachment 50775 [details]
Patch for this issue (rev9)

Sorry for the spam. I have fixed the style issues. I mistakenly ran "check-webkit-style --git-commit=HEAD" instead of "check-webkit-style --git-commit=HEAD^", which I believe is under discussion here: https://bugs.webkit.org/show_bug.cgi?id=35781 .

[Xan Lopez]

Comment on attachment 50775 [details]
Patch for this issue (rev9)

>-static void setPendingComposition(gchar* newComposition)
>+static void imContextCommitted(GtkIMContext* context, const gchar* compositionString, EditorClient* client)
> {
>-    g_free(pendingComposition);
>-    pendingComposition = newComposition;
>-}
>+    // If this signal fires during a keydown event when we are not in the middle
>+    // of a composition, then treat this 'commit' as a normal key event and just
>+    // change the editable area right before the keypress event.
>+    if (client->treatContextCommitAsKeyEvent()) {
>+        client->updatePendingComposition(compositionString);
>+        return;
>+    }
> 
>-static void setPendingPreedit(gchar* newPreedit)
>-{
>-    g_free(pendingPreedit);
>-    pendingPreedit = newPreedit;
>-}
>+    Frame* frame = core(client->webView())->focusController()->focusedOrMainFrame();
>+    if (!frame || !frame->editor()->canEdit())
>+        return;

I thought we had decided to move this check to the beginning of the function? Did you forget or have you found any problems with that?

It looks fine to me other than that, so r=me with that change unless you have a good reason not to do it.

[Martin Robinson]

Created attachment 50799 [details]
Patch for this issue (final)

I've attached the final version of this patch incorporating xan's suggestion.

[Xan Lopez]

Comment on attachment 50799 [details]
Patch for this issue (final)

r=me

[Martin Robinson]

Committed r56072: <http://trac.webkit.org/changeset/56072>