Unable to enter national language data on AIX 4.3

VERIFIED FIXED

Status

()

VERIFIED FIXED
17 years ago
16 years ago

People

(Reporter: lkemmel, Assigned: tetsuroy)

Tracking

({intl})

Trunk
Other
AIX
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

(Reporter)

Description

17 years ago
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.
(Reporter)

Updated

17 years ago
Depends on: 157394
(Reporter)

Updated

17 years ago
Blocks: 18688
(Reporter)

Comment 1

17 years ago
Created attachment 91277 [details] [diff] [review]
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*.

Updated

17 years ago
Keywords: intl

Comment 2

17 years ago
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.
(Reporter)

Comment 3

17 years ago
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.
(Reporter)

Comment 4

17 years ago
Created attachment 92565 [details] [diff] [review]
Patch for the code patched by attachment 91277 [details] [diff] [review]
(Reporter)

Comment 5

17 years ago
Created attachment 92569 [details] [diff] [review]
Full patch against mozilla cvs branch
(Assignee)

Comment 6

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

Comment 7

17 years ago
Created attachment 93148 [details] [diff] [review]
Patch with some cleanup
Attachment #91277 - Attachment is obsolete: true
Attachment #92565 - Attachment is obsolete: true
Attachment #92569 - Attachment is obsolete: true

Updated

16 years ago
Whiteboard: branchOEM

Updated

16 years ago
Whiteboard: branchOEM → branchOEM+

Updated

16 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 8

16 years ago
removing "branchOEM+", till patch gets r=/sr= for trunk
Whiteboard: branchOEM+ → branchOEM

Comment 9

16 years ago
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

16 years ago
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

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

Comment 12

16 years ago
Created attachment 96344 [details] [diff] [review]
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.

Updated

16 years ago
Attachment #93148 - Attachment is obsolete: true

Updated

16 years ago
URL: Any
Keywords: branchOEM
Whiteboard: branchOEM

Comment 13

16 years ago
Created attachment 100016 [details] [diff] [review]
Hopefully final patch.

This patch is identical to the last one, except GetUnichars() can now be
defined const.
Attachment #96344 - Attachment is obsolete: true
(Assignee)

Comment 14

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

/r=yokoyama
Though I am not familiar 
with AIX, the patch looks
good.
Attachment #100016 - Flags: review+

Comment 15

16 years ago
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

16 years ago
==== 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

16 years ago
Created attachment 103342 [details] [diff] [review]
Address kin's comments

Adds SetUnichars() function, fixes misspelling in comment.
Attachment #100016 - Attachment is obsolete: true

Updated

16 years ago
Attachment #103342 - Flags: superreview+

Comment 18

16 years ago
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; }
(Assignee)

Comment 19

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

/r=YOKOYAMA
Attachment #103342 - Flags: review+

Comment 20

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

a=asa for checkin to 1.2 (on behalf of drivers).
Attachment #103342 - Flags: approval+

Comment 21

16 years ago
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
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED

Comment 22

16 years ago
Verified in today's build.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.