In appropriate locales on AIX 4.3, switching keyboard layer is usually done by pressing and releasing Alt+Shift keys. This works for different applications, for example, aixterm or Netscape 4.7. In mozilla, however, switching keyboard layer has no effect; ASCII characters are always produced. Experienced in the iw_IL (hebrew) or ar_AA (arabic) locales, but most likely should happen in other national locales.

nsConvertCharCodeToUnicode(..) - http://lxr.mozilla.org/mozilla/source/widget/src/gtk/nsGtkEventHandler.cpp#329 - is used to convert keysym to Unicode. The problem on AIX 4.3 is that it doesn't receive correct keysym. According to the GDK documentation - http://developer.gnome.org/doc/API/gdk/gdk-event-structures.html#GDKEVENTKEY - the gchar *string member of GdkEventKey* is: "a null-terminated multi-byte string containing the composed characters resulting from the key press. When text is being input, in a GtkEntry for example, it is these characters which should be added to the input buffer. When using Input Methods to support internationalized text input, the composed characters appear here after the pre-editing has been completed. " So my suggestion is to convert to Unicode from the *string* (native encoding) instead of *keysym*.

The suggested fix causes all shortcut keys to stop functioning. (Tested under LANG=C and LANG=en_US - most likely occurs in other locales as well) If you apply the patch, then start the browser and try to issue commands like Ctrl+Q to quit, Ctrl+N to create a new browser window, or Ctrl+T to create a new tab, nothing seems to have an effect.

Hopefully the problem with shortcuts is fixed. I'm going to attach 2 patches: 1) for the code already patched with the previous patch. 2) full patch.

simon: i see some #ifdef bidi in the patch. Can you take a look at it? cc shanjian for gtk code

removing "branchOEM+", till patch gets r=/sr= for trunk

Is anyone available to review the latest patch? We would like to get this fixed on the Netscape 7 OEM branch, but this can't happen until it gets accepted for the trunk.

In Bug 161581, we are seeing similar behavior with gdk receiving invalid keysyms from the x-server. This causes users who start the browser in the ru_RU or zh_CN locales (probably others as well) to not be able to input characters which require the shift key. This is because the GdkEventKey->keyval does not correspond to the GdkEventKey->string. I have looked at the gdk source, and the keyval and string members are initialized by a call to XmbLookupString. The manpage for XmbLookupString (http://www.x.org/consortium/R6doc/man/X11/XmbLookupString) specifically states that "If both a string and a KeySym are returned, the KeySym value does not necessarily correspond to the string returned." To solve both of these problems, I think we need to extend this patch to use the GdkEventKey->string value when no control/alt/meta characters are used. For example, we currently use GdkEventKey->string when GdkEventKey->state is not defined. We should change the behavior to use GdkEventKey->string when !(GdkEventKey->state & GDK_CONTROL_MASK || GdkEventKey->state & GDK_MOD1_MASK || GdkEventKey->state & GDK_MOD4_MASK). In addition, I don't believe this bug is BIDI specific, so all references to IBMBIDI can be removed.

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

This patch cleans up the previous one, and also fixed the problems discussed in Bug 161581. I have confirmed that mode switch functionality works in ar_AA and he_IL, and also that typing uppercase characters in ru_RU and zh_CN works properly.

This patch is identical to the last one, except GetUnichars() can now be defined const.

Comment on attachment 100016 [details] [diff] [review] Hopefully final patch. /r=yokoyama Though I am not familiar with AIX, the patch looks good.

Comment on attachment 100016 [details] [diff] [review] Hopefully final patch. ==== Use C++ style comments "//" like the rest of the file does: + /* On AIX, GDK doesn't get correct keysyms from XIM. Follow GDK + reference recommending to use the 'string' member of GdkEventKey + instead. See: + + developer.gnome.org/doc/API/gdk/gdk-event-structures.html#GDKEVENTKEY + */ + /* + Use 'string' as opposed to 'keyval' when control, alt, or meta is + not pressed. This allows keyboard shortcuts to continue to function + properly, while fixing input problems and mode switching in certain + locales where the 'keyval' does not correspond to the actual input. + See Bug #157397 and Bug #161581 for more details. + */ ==== So if |MultiByteToUnicode()| destroys the buffer |mUnichars| points to, and it reallocates a new buffer, what resets |mUnichars| to point to the new buffer? Shouldn't there be a call to something like |IMEHelper->SetUnichars(unichars)| here? + if (unichar_size > 0) { + IMEHelper->SetUnicharsSize(unilen); + return (long)*unichars; + }

==== Use C++ style comments "//" like the rest of the file does: Sure. ==== So if |MultiByteToUnicode()| destroys the buffer |mUnichars| points to, and it reallocates a new buffer, what resets |mUnichars| to point to the new buffer? Shouldn't there be a call to something like |IMEHelper->SetUnichars(unichars)| here? You are correct. It looks as if MultiByteToUnicode deletes previous memory, and allocates a new buffer if the buffer passed as a parameter is not large enough to hold the converted characters. I will add a new SetUnichars function as you suggest to work around any potential problems.

Adds SetUnichars() function, fixes misspelling in comment.

Comment on attachment 103342 [details] [diff] [review] Address kin's comments sr=kin@netscape.com ==== Turn the empty comment into a blank line: + // + PRBool controlChar = (aGEK->state & GDK_CONTROL_MASK || ==== Put a blank line after the comment so that it's more readable: + // See Bug #157397 and Bug #161581 for more details. + if (!controlChar && gdk_im_ready() && aGEK->length > 0 ==== Just add a comment to this method saying that it is the responsibility of the caller to free any data in |mUnichars| prior to calling |SetUnichars()|. + void SetUnichars(PRUnichar *aUnichars) { mUnichars = aUnichars; }

Comment on attachment 103342 [details] [diff] [review] Address kin's comments /r=YOKOYAMA

Comment on attachment 103342 [details] [diff] [review] Address kin's comments a=asa for checkin to 1.2 (on behalf of drivers).

Checked in: Checking in nsGtkEventHandler.cpp; /cvsroot/mozilla/widget/src/gtk/nsGtkEventHandler.cpp,v <-- nsGtkEventHandler.cpp new revision: 1.173; previous revision: 1.172 done Checking in nsGtkIMEHelper.cpp; /cvsroot/mozilla/widget/src/gtk/nsGtkIMEHelper.cpp,v <-- nsGtkIMEHelper.cpp new revision: 1.45; previous revision: 1.44 done Checking in nsGtkIMEHelper.h; /cvsroot/mozilla/widget/src/gtk/nsGtkIMEHelper.h,v <-- nsGtkIMEHelper.h new revision: 1.14; previous revision: 1.13 done

Verified in today's build.