203671 - Keyboard direction is never detected on GTK

Status: REOPENED

(Core :: Layout: Text and Fonts, defect)

Description

[Lina Kemmel]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.4a) Gecko/20030304
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.4a) Gecko/20030304

"Keyboard direction" stays for the writing direction of the current keyboard
language.
Tracking it is required for bidi processing. Keyboard direction is RTL in Hebrew
or Arabic layer, otherwise its direction is LTR. 
So detecting keyboard direction would be achieved through detecting keyboard layer.
It is not implemented for gtk: see
http://lxr.mozilla.org/seamonkey/source/widget/src/gtk/nsBidiKeyboard.cpp#38

Does anyone aware of any xlib or glib (or even pango?) APIs to query keyboard
layer? If not, perhaps there is some workaround solution.

Reproducible: Always

Steps to Reproduce:

Comment 1

[Lina Kemmel]

Maybe it possible to use XmbLookupString with some predefined event.keycode and
then compare result keysym with ones expected in Hebrew and Arabic keyboard layer?

Comment 2

[Philip K. Warren]

For GTK2, it looks like the following function should be used:

http://developer.gnome.org/doc/API/2.0/gdk/gdk-Keyboard-Handling.html#gdk-keymap-get-direction

Comment 3

[Philip K. Warren]

It looks like the GTK2 code currently uses the X Keyboard Extension to determine
the direction:

http://cvs.gnome.org/bonsai/cvsblame.cgi?file=gtk%2B%2Fgdk%2Fx11/gdkkeys-x11.c&rev=1.34&root=/cvs/gnome&mark=362-390#361

It should be possible to do something similar with Mozilla's GTK1 widget code as
well (but it would only work for people using the X Keyboard Extension).

Comment 4

[Lina Kemmel]

philip: I believe that your proposal is acceptable in linux environment.

[AIX 4.3 with gtk 1 specific]

1. XKB based solution
So far I haven't been successful in configuring xkb on AIX 4.3 - except for 
only one step: adding +kb extension to /usr/lpp/X11/defaults/xserverrc (which 
allowed using xkb functions, but didn't make the gdk code (you refer to) work). 
Is anyone aware where I can RTFM on setting up xkb for AIX?

2. non-XKB
2.1. XmbLookupString does produce some outcome (returned in char buffer as 
opposed to keysym), but this doesn't seem to be the right way to go.
2.2. I believe IM server should be aware of the current keyboard layout. But 
how to query it?..

And what about other unixes?

Comment 5

[Lina Kemmel]

P.S. I can keep configuring xkb on AIX as described in XFree-related 
documentation, but I think that it would be insufficient in the absence of 
proper configuring the system to use xkb.

Comment 6

[Lina Kemmel]

Possible usage of XmbLookupString - though it's probably not good solution:

It appears that for core X protocol, only up to 2 language groups (one of which 
is derived from the current locale) can be supported. So steps to get direction 
could be as follows:
1. Retrieve the current locale, e.g. by gtk_set_locale or XLocaleOfIM.
2. If not Hebrew or Arabic, return LTR.
3. Call XmbLookupString.
4. If a return status is XBufferOverflow or XLookupNone, return LTR.
5. Return RTL.

Any comments?

Comment 7

[Lina Kemmel]

Attachment: Proposed patch for GTK2 based build

Detect the keyboard language:

1. On AIX, using AIX IM:
a. For each shell widget, create an instance of IM object and associate it with
that widget (AIX IMObject is usually associated with a shell, and not with a
whole display).
b. All key press events are filtered through the IM object of the focused
widget.
c. As a result, the IM object (and the shell owning it) is aware of its
language layer, and is queried for it whenever needed.

2. If defined HAVE_XKB: using GTK2 API.

3. Otherwise, the keyboard direction is derived from the direction of the first
alpha entry of the current keyboard map.

Comment 8

[Ilya Konstantinov]

Whoo. Looks cool. Didn't know gtk2 has gdk_keymap_get_direction; solves us a lot
of custom Xlib code. I'll beta-test it.

Comment 9

[Ilya Konstantinov]

In general, looks fine. Few comments:  
  
1. gdk_keymap_get_for_display is available even without XKB. GTK gracefully  
falls back, if it wasn't compiled with XKB. (And we don't have any XKB checks  
in our autoconf anyway.) This means that the XDisplayKeycodes-based option  
should be removed from the gtk2 version.  
If we're worried about gdk_keymap_get_for_display always returning  
PANGO_DIRECTION_LTR on non-XKB GTK2 builds, we should push a fix into GTK. (I'd  
happily do it.)  
  
About reusing the XDisplayKeycodes-version anywhere else:  
  
1. Change  
    guint keycode = 0;  
   to  
    int keycode = 0;  
   XDisplayKeycodes wants an int*.  
  
2. Watch for KeyboardMapping X events to catch keyboard remapping and mark the  
cache as dirty.  
  

Comment 10

[Lina Kemmel]

Thanks very much for reviewing the patch.

> gdk_keymap_get_for_display is available even without XKB.

Not really: it (gdk_keymap_get_direction) just becomes a stub without XKB. My 
purpose was to ensure certain functionality in mozilla. Since that 
functionality is not provided by gtk, I suggested a workaround for it in the 
application code.

Here are the cases (at least I'm aware of), when the API won't work, even if 
GTK is compiled with XKB:
a) XKB is not supported in Xlib.
b) The used keymap is non an XKB one.

> GTK gracefully falls back, if it wasn't compiled with XKB. (And we don't
> have any XKB checks in our autoconf anyway.) This means that the
> XDisplayKeycodes-based option should be removed from the gtk2 version.

If I'm not wrong, GTK is compiled with XKB if either it's asked explicitly, or 
X supports XKB (checked in runtime). But, sorry, I anyway don't understand why 
this means that the XDisplayKeycodes-based option should be removed..

> If we're worried about gdk_keymap_get_for_display always returning
> PANGO_DIRECTION_LTR on non-XKB GTK2 builds, we should push a fix into GTK.
> (I'd  happily do it.)

The fact that without XKB it always returns PANGO_DIRECTION_LTR (btw, it would 
better return some undefined direction instead), was the one and only reason I 
use the XDisplayKeycodes-based workaround. Of course, fixing this on the gtk 
side is the right thing (though I think it was not done because there is no 
good way to proceed - I wish I were wrong). It would be great if you could fix 
it :) 

However, why not to put some workaround in the meantime? Especially, given that 
majority of people don't seem to regularly upgrade their gtks.

> Change  
>     guint keycode = 0;  
>    to  
>     int keycode = 0;  
>    XDisplayKeycodes wants an int*.  

Sorry, of course it should be int. (I changed it, experimenting with gtk, which 
needs guint, I forgot to bring it back.)

> Watch for KeyboardMapping X events to catch keyboard remapping and mark the  
> cache as dirty.

The cache may still remain valid, even if the keymap gets changed. At present, 
XDisplayKeycodes (with iterating over keycodes) is called twice per 
session "normally" (given a keymap isn't changed programmatically). But if we 
invalidate the cache on each KeyboardMapping event, it may be executed as many 
times as the keyboard language is switched (plus 1 time on initialization of 
one of the keycodes).

Or, did you mean listening for KeyboardMapping events to refresh the keyboard 
language (instead of doing it each time keyboard direction is asked)? If so, it 
seems we can't rely that MappingNotify is always generated when a keymap 
changes (at least, in AIX it doesn't happen).

Thanks!

Comment 11

[Lina Kemmel]

> we can't rely that MappingNotify is always generated when a keymap 
> changes
For the time being, the same about the "keys_changed" signal.

Hopefully, if one day gtk fully supports both XKB and non XKB keymaps, it will 
be possible just to listen for the "direction_changed" signal to refresh a 
keyboard direction.

Comment 12

[Lina Kemmel]

Attachment: patch up to date

Slight changes, including replacing |guint keycode| with |int keycode|, as Ilya
pointed out.

Comment 13

[Lina Kemmel]

In the meantime, I kept the XDisplayKeycodes-based section as is. Ilya, do you 
still think it should be removed?

Comment 14

[Ilya Konstantinov]

Until we have autoconf code to check for XKB, the fallback code is irrelevant anyway.
I anticipate that I'll add fallback code to gtk2 itself before anyone modifies Mozilla's autoconf.
So meanwhile, since the USE_XKB check doesn't work, simply always use gtk's keyboard direction function.

Comment 15

[Lina Kemmel]

Can you elaborate please? I can't understand why it's irrelevant, if it covers 
both XKB and "Xmodmap" keymaps, while gtk's API (at present) doesn't work in 
not few cases?

Comment 16

[Lina Kemmel]

Sorry, I got it as though you anticipate you WON'T "add fallback code to gtk2 
itself before anyone modifies Mozilla's autoconf".

OK, I'll remove that piece and resend the patch.

Comment 17

[Lina Kemmel]

gdk_keymap_get_direction() doesn't work also in KDE, when setting layout 
switching policy to "Window" with the KDE keyboard tool.
Ilya, could it be fixed in gtk?

Comment 18

[Ilya Konstantinov]

Attachment: Test utility for gdk_keymap_get_direction

This utility proves that gdk_keymap_get_direction works with KDE's (kxkb's)
"Window" switching policy.

I've tried it and, regardless of the keymap/group I've had in other windows,
the tester window retained its keymap and group. In particular, the characters
that were entered in the input box were as expected and
gdk_keymap_get_direction returned the correct value.

This is with Gtk+ 2.4.3.

Comment 19

[Ilya Konstantinov]

So, for starters, could you please post a patch stripped of all non-XKB sutff? 
Then we could request a review+sr for it (should be quick, I hope). In 
parallel, I'll try to get next version of GTK+ 2.4 to support the non-XKB 
method. 

Comment 20

[Lina Kemmel]

(In reply to comment 18)

The problem occurs when top level windows with different keyboard layer belong 
to the same app.

Comment 21

[Lina Kemmel]

We should get keymap for top level window's display:

gdk_keymap_get_for_display(gtk_widget_get_display(widget));

Then kxkb seems to be working with "Window" switching policy.

Comment 22

[Lina Kemmel]

> Then kxkb seems to be working with "Window" switching policy.

Sorry, I meant then *gdk_keyboard_get_direction()* works when kxkb is 
configured that way.

Comment 23

[Lina Kemmel]

However, I suggest to keep the non-XKB stuff, but condition it with gtk version 
not greater than 2.4.4.

Comment 24

[Lina Kemmel]

Attachment: patch

Detect KBD direction:

1. On AIX: using AIX IM.

2. For GTK version <= 2.4.4: using GTK2 API. (GTK version number is supposed to
match the up-to-date status of the API's non-XKB compatibility. For the time
being, the last version not supporting non-XKB, is 2.4.4.)

3. For the rest of cases, keyboard direction is derived from the direction of
the first alpha keymap entry.

The patch was verified on AIX 4.3 and RedHat 9 Linux - in GNOME and KDE desktop
environments, with different KBD configuration options.

Comment 25

[Lina Kemmel]

Sorry, of course the GTK2 API to be employed with GTK version > 2.4.4 (for the 
time being).

Comment 26

[Simon Montagu :smontagu]

Comment on attachment 152935 [details] [diff] [review]
patch

I don't feel qualified to review all this gtk stuff. Passing off to jshin.

Comment 27

[Asaf Romano (gone)]

jshin?

Comment 28

[Jungshik Shin]

Comment on attachment 152935 [details] [diff] [review]
patch

I'm sorry. I should have deferred to pkwarren or bryner a long time ago

Comment 29

[Eyal Rozenberg]

I have no AIX IM experience either, but that comparison after getenv("lang")
seems a bit suspicious... I thought the Hebrew lang value is he_IL, not iw_IL.
Plus, for Arabic, aren't there also ar_JO, ar_SA, etc? And isn't there a
function already implemented somewhere in the Mozilla codebase which checks the
lang environment variable?

Comment 30

[Christopher Blizzard (:blizzard)]

Comment on attachment 152935 [details] [diff] [review]
patch

Not doing reviews at the moment, over to bryner

Comment 31

[Ilya Konstantinov]

Why do we let this patch bitrot so long?

Eyal:
1. On AIX, it might be called iw_IL. Note that this weirdness is only used on
AIX and I assume the IBM people know their system.
2. The only such function I found is at:
http://lxr.mozilla.org/mozilla1.8/source/browser/base/content/utilityOverlay.js#372
and it seems to miss the old-fashioned "iw_IL" locale (as seems to be still used
by AIX).
See any point in rewriting it in C++?

Also, now that GTK 2.8 is out, maybe we should drop the laborous detection code
completely and just rely on gdk_keymap_get_direction ?

Comment 32

[Brian Ryner (not reading)]

Comment on attachment 152935 [details] [diff] [review]
patch

I'm not familiar with the internals of keyboard detection on AIX, so I'll leave that to pkwarren or another competent reviewer, and just address a couple of other problems:

>--- widget/src/gtk2/nsBidiKeyboard.cpp	17 Jan 2003 11:40:06 -0000	1.6
>+++ widget/src/gtk2/nsBidiKeyboard.cpp	12 Jul 2004 10:14:08 -0000
>@@ -21,6 +21,8 @@
>  * are Copyright (C) 2001 the Initial Developer. All Rights Reserved.
>  *
>  * Contributor(s):
>+ *                  ...............
>+ *                  IBM Corporation
>  *
>  * Alternatively, the contents of this file may be used under the terms of
>  * either the GNU General Public License Version 2 or later (the "GPL"), or
>@@ -38,6 +40,28 @@
> 
> #include "nsBidiKeyboard.h"
> 
>+#ifdef USE_BIM
>+#include "nsWindow.h"

Move the nsWindow.h include up out of the #ifdef, since you also included it in the #else.

>@@ -50,8 +74,47 @@
> 
> NS_IMETHODIMP nsBidiKeyboard::IsLangRTL(PRBool *aIsRTL)
> {
>+#ifndef USE_X11_MAP

If possible, please make #ifdef's positive instead of negative if you're handling both cases.  It's easier to read.  So,

#ifdef USE_X11_MAP
...
#else
...
#endif

Is "BIM" completely AIX-specific?  If not, you might want to rename aix_* to bim_*.

Comment 33

[Behnam Esfahbod [:zwnj]]

We almost did it on the following bugs.  Here are the documentation of what we are going to do, and the problems exist on the GTK2 backend: http://developer.mozilla.org/en/docs/nsIBidiKeyboard .

Duplicate of:
Bug 348712 - RTL Caret for GTK2 on 1.8 Branch
Bug 348724 - nsBidiKeyboard for GTK2 backend

I looked at last patch.  Seems it has a lot of duplication with GDK code.  Unfortunately I didn't understand the IM parts, so someone please help me on that.

*** This bug has been marked as a duplicate of 348724 ***

Comment 34

[Lina Kemmel]

(In reply to comment #32)
Sorry about the late reply and thank you for the valuable comments. Unfortunately, I have no AIX machine available to verify any updates/patches... Hopefully, will be able to get back to this in 2 months.

Comment 35

[Lina Kemmel]

(In reply to comment #33)
I could not see any reference to AIX in your proposal.

Perhaps it makes sense to split the bug, and have one dedicated to AIX and another - to other platforms.

And please observe that GDK code duplicates the last patch, but not the contrary.

Comment 36

[Behnam Esfahbod [:zwnj]]

Doesn't GDK's function work on AIX?  If not, can you explain why?  Doesn't AIX's X server has XKB module?

Comment 37

[Lina Kemmel]

(In reply to Behnam Esfahbod [:zwnj] from comment #33)
> I looked at last patch.  Seems it has a lot of duplication with GDK code. 

Getting on this a bit late ;)
Just to make it clear: I do NOT duplicate any (even a single line) of any code or idea. Also, at the time I uploaded my patch to bugzilla, GDK hasn't yet contained any code for determining KBD direction.