157397 - Unable to enter national language data on AIX 4.3

Status: VERIFIED FIXED

(Core :: Internationalization, defect)

Description

[Lina Kemmel]

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.

Comment 1

[Lina Kemmel]

Attachment: suggested fix for this bug

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*.

Comment 2

[Philip K. Warren]

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.

Comment 3

[Lina Kemmel]

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.

Comment 4

[Lina Kemmel]

Attachment: Patch for the code patched by attachment 91277

Comment 5

[Lina Kemmel]

Attachment: Full patch against mozilla cvs branch

Comment 6

[Roy Yokoyama]

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

Comment 7

[Lina Kemmel]

Attachment: Patch with some cleanup

Comment 8

[Jim Dunn]

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

Comment 9

[Philip K. Warren]

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.

Comment 10

[Philip K. Warren]

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.

Comment 11

[Philip K. Warren]

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

Comment 12

[Philip K. Warren]

Attachment: Updated patch

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.

Comment 13

[Philip K. Warren]

Attachment: Hopefully final patch.

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

Comment 14

[Roy Yokoyama]

Comment on attachment 100016 [details] [diff] [review]
Hopefully final patch.

/r=yokoyama
Though I am not familiar 
with AIX, the patch looks
good.

Comment 15

[kinmoz]

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;
+      }

Comment 16

[Philip K. Warren]

==== 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.

Comment 17

[Philip K. Warren]

Attachment: Address kin's comments

Adds SetUnichars() function, fixes misspelling in comment.

Comment 18

[kinmoz]

Comment on attachment 103342 [details] [diff] [review]
Address kin's comments

sr=[e-mail]

==== 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 19

[Roy Yokoyama]

Comment on attachment 103342 [details] [diff] [review]
Address kin's comments

/r=YOKOYAMA

Comment 20

[Asa Dotzler [:asa]]

Comment on attachment 103342 [details] [diff] [review]
Address kin's comments

a=asa for checkin to 1.2 (on behalf of drivers).

Comment 21

[Philip K. Warren]

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

Comment 22

[Philip K. Warren]

Verified in today's build.