Bug 16145

Summary: Simulated Caps lock key press via XTestFakeKeyEvent does not toggle the LED indicator
Product: xorg Reporter: Stefan Dirsch <sndirsch>
Component: Server/GeneralAssignee: Peter Hutterer <peter.hutterer>
Status: RESOLVED FIXED QA Contact: Xorg Project Team <xorg-team>
Severity: normal    
Priority: medium CC: ametzler, cffs-fdo, freedesktop.zen.ssokolow, jeromeg, l.lunak, mcepl, mloiseleur, nucleo, ossi, peter.hutterer
Version: 7.3 (2007.09)   
Hardware: Other   
OS: All   
Whiteboard:
i915 platform: i915 features:
Attachments:
Description Flags
CapsLockToggle.c
none
numlockx-1.1.tar.gz none

Description Stefan Dirsch 2008-05-29 00:37:10 UTC
A simulated Caps Lock key press makes the logical and the physical states of
the Caps Lock out of sync.  This causes a problem as one of OpenOffice's
functionality - correct accidental use of cAPS LOCK key - relies on this
feature of X.

The attached test program toggles the logical state of the Caps Lock key, but
the LED indicator does not get toggled.

The "correct accidental use of Caps Lock" feature in OOo is based on this
test program. Explanation of this feature:

Let's say you open Writer, with your Caps Lock key turned on (but you don't
know it's on).  Start typing a sentence e.g. "Here comes the Sun" but it comes
up as hERE because of the Caps Lock.  As soon as you hit space after the first
word, Writer will then correct that to 'Here' and turn off the Caps Lock key. 
Calc, Impress and Draw all do this.

This LED indicator still worked with xserver 1.3.0.0, but not any longer with
1.4.0.90.
Comment 1 Stefan Dirsch 2008-05-29 00:38:58 UTC
Created attachment 16798 [details]
CapsLockToggle.c

Compile it with 'gcc -Wall -o CapsToggle CapsToggle.c -l Xtst'.
Comment 2 Peter Hutterer 2008-05-29 01:28:53 UTC
On Thu, May 29, 2008 at 12:37:12AM -0700, bugzilla-daemon@freedesktop.org wrote:
> A simulated Caps Lock key press makes the logical and the physical states of
> the Caps Lock out of sync.  This causes a problem as one of OpenOffice's
> functionality - correct accidental use of cAPS LOCK key - relies on this
> feature of X.

confirmed with git master.

Now, with 1.4 and master having similar subsystems I think I found what the
issue is.
1.3 and before had one physical device as the "core keyboard". 1.4 and beyond
however feature a "virtual core keyboard" (VCK), all physical input devices
are extension keyboards.

If you hit a key on the keyboard, the core event is generated by the VCK, and
an XI event by the actual keyboard. The LED change then happens when the XI
event is passed through xkb (the LEDS on the virtual keyboard change too,
but, well...).

If you use xtest for core events, the server picks the VCK and processes it
accordingly - but, because it is the VCK you don't see the LED change.
To fix this, you essentially have to pick which devices need the LED state
updated and run it past them as well. Which gets a bit iffy, if you have
multiple keyboards, some of which have CL on and some of which have it off
(which AFAIK should work now). Which ones do you pick? Do you switch CL on? or
off?

Comments?
Comment 3 Stefan Dirsch 2008-05-29 02:36:37 UTC
As you can see in the test program

a) I didn't pick any special keyboard at all
b) it's just a toggle (no explicit turn on/off)

I attach a more complex test program for turning numlock on/off or toggle it,
which still works with xserver 1.4.0.90. Not sure how to adjust it to get also Capslock working. I'm afraid numlock and Capslock needs to be handled slightly different.
Comment 4 Stefan Dirsch 2008-05-29 02:39:19 UTC
Created attachment 16806 [details]
numlockx-1.1.tar.gz
Comment 5 Martin-Éric Racine 2008-05-29 03:00:59 UTC
Upstream hasn't maintained numlockx code in ages. Already, Debian/Ubuntu had to autoreconf to accommodate the modular X.org structure. If anybody feels like taking over upstream maintenance and issuing a patched source with proper #ifdef to compile this on recent X. please do so and inform me of the new upstream tarball location, so that I update the debian package. Thanks!
Comment 6 Daniel Stone 2008-05-29 03:20:01 UTC
On Thu, May 29, 2008 at 01:28:54AM -0700, bugzilla-daemon@freedesktop.org wrote:
> If you hit a key on the keyboard, the core event is generated by the VCK, and
> an XI event by the actual keyboard. The LED change then happens when the XI
> event is passed through xkb (the LEDS on the virtual keyboard change too,
> but, well...).
> 
> If you use xtest for core events, the server picks the VCK and processes it
> accordingly - but, because it is the VCK you don't see the LED change.
> To fix this, you essentially have to pick which devices need the LED state
> updated and run it past them as well. Which gets a bit iffy, if you have
> multiple keyboards, some of which have CL on and some of which have it off
> (which AFAIK should work now). Which ones do you pick? Do you switch CL on? or
> off?
> 
> Comments?

I think the only thing that makes sense is for XTest to use GKE to
generate events for the keyboard that last sent core events.  Principle
of least surprise and all -- why mangle other keyboards?
Comment 7 Stefan Dirsch 2008-05-29 03:21:38 UTC
(In reply to comment #5)
> Upstream hasn't maintained numlockx code in ages. Already, Debian/Ubuntu had
> to autoreconf to accommodate the modular X.org structure. If anybody feels
> like taking over upstream maintenance and issuing a patched source with
> proper #ifdef to compile this on recent X. please do so and inform me of the
> new upstream tarball location, so that I update the debian package. Thanks!

This is completely off-topic. Please discuss this issue with Lubos Lunak via private mail, IRC or whatever. numlockx is *not* the problem here at all!
Comment 8 Peter Hutterer 2008-05-29 04:04:28 UTC
> --- Comment #6 from Daniel Stone <daniel@fooishbar.org>  2008-05-29 03:20:01 PST ---
> I think the only thing that makes sense is for XTest to use GKE to
> generate events for the keyboard that last sent core events.  Principle
> of least surprise and all -- why mangle other keyboards?

that would actually be easy now. MDs keep dev->u.lastSlave around, so we could
just route through that. For 1.4, this would be slightly more complicated.
First thought is to just add two fields to inputInfo.
Comment 9 Daniel Stone 2008-05-29 06:07:11 UTC
(In reply to comment #8)
> > --- Comment #6 from Daniel Stone <daniel@fooishbar.org>  2008-05-29 03:20:01 PST ---
> > I think the only thing that makes sense is for XTest to use GKE to
> > generate events for the keyboard that last sent core events.  Principle
> > of least surprise and all -- why mangle other keyboards?
> 
> that would actually be easy now. MDs keep dev->u.lastSlave around, so we could
> just route through that. For 1.4, this would be slightly more complicated.
> First thought is to just add two fields to inputInfo.

possibly not so complicated -- what about the following patch:
diff --git a/dix/getevents.c b/dix/getevents.c
index 2fd4e54..79366f8 100644
--- a/dix/getevents.c
+++ b/dix/getevents.c
@@ -414,6 +414,10 @@ GetKeyboardValuatorEvents(xEvent *events, DeviceIntPtr pDev, int type,
     CARD32 ms = 0;
     deviceKeyButtonPointer *kbp = NULL;
 
+    if (pDev == inputInfo.keyboard)
+        pDev = dixLookupPrivate(&inputInfo.keyboard->devPrivates,
+                                CoreDevicePrivateKey);
+
     if (!events)
         return 0;
 

(assuming all callers of GK{,V}E are happy with this.)
Comment 10 Stefan Dirsch 2008-05-29 06:50:33 UTC
Daniel, is the patch for 1.4 or git master? I would be happy to try it with 1.4.0.90.
Comment 11 Daniel Stone 2008-05-29 07:08:33 UTC
On Thu, May 29, 2008 at 06:50:35AM -0700, bugzilla-daemon@freedesktop.org wrote:
> Daniel, is the patch for 1.4 or git master? I would be happy to try it with
> 1.4.0.90.

Should work with both, modulo merge detritus (although MPX may get rid
of the private and replace it with ->lastSlave).  The point is that, if
GetKeyboardEvents is called with inputInfo.keyboard as the device, to
change the device to the last device which used it to send events.  It's
definitely not suitable for applying or distributions though, as I
haven't checked all callers of GK(V)E; it may have bad side effects.
Comment 12 Julien Cristau 2008-05-29 07:24:17 UTC
On Thu, May 29, 2008 at 07:08:33 -0700, bugzilla-daemon@freedesktop.org wrote:

> --- Comment #11 from Daniel Stone <daniel@fooishbar.org>  2008-05-29 07:08:33 PST ---
> On Thu, May 29, 2008 at 06:50:35AM -0700, bugzilla-daemon@freedesktop.org
> wrote:
> > Daniel, is the patch for 1.4 or git master? I would be happy to try it with
> > 1.4.0.90.
> 
> Should work with both, modulo merge detritus

Modulo devPrivates rework, too, as 1.4 doesn't have dixLookupPrivate().
You'll need pDev->devPrivates[CoreDevicePrivatesIndex].ptr I think.

Cheers,
Julien
Comment 13 Peter Hutterer 2008-05-29 23:19:07 UTC
> possibly not so complicated -- what about the following patch:
> diff --git a/dix/getevents.c b/dix/getevents.c
> index 2fd4e54..79366f8 100644
> --- a/dix/getevents.c
> +++ b/dix/getevents.c
> @@ -414,6 +414,10 @@ GetKeyboardValuatorEvents(xEvent *events, DeviceIntPtr
> pDev, int type,
>      CARD32 ms = 0;
>      deviceKeyButtonPointer *kbp = NULL;
> 
> +    if (pDev == inputInfo.keyboard)
> +        pDev = dixLookupPrivate(&inputInfo.keyboard->devPrivates,
> +                                CoreDevicePrivateKey);
> +
>      if (!events)
>          return 0;
> 
> 
> (assuming all callers of GK{,V}E are happy with this.)

won't work, ProcXTestFakeInput doesn't use GPE/GKVE right now. I have a patch
for master in the pipe here, but it still needs some special handling for
xinerama. once it is in, it can probably be backported to 1.4.
Comment 14 Peter Hutterer 2008-05-31 01:17:01 UTC
I just pushed a fix to master [1] that fixes this issue. Unfortunately it turned
out rather invasive, so it needs active backporting to 1.4.

The main part is to switch ProcXTestFakeInput to use GPE/GKVE, and flip the
device to the last-used slave device.

[1] 105d28652d1fb80dd8ce8511e2605dccc8812e99
Comment 15 Brice Goglin 2008-06-21 11:16:22 UTC
Is all this related to bug#15359 and bug#9964 ? I am having a hard time trying to sort out all our LED-related bugs.
Comment 16 Peter Hutterer 2008-06-26 03:25:17 UTC
> --- Comment #15 from Brice Goglin <brice.goglin@ens-lyon.org>  2008-06-21 11:16:22 PST ---
> Is all this related to bug#15359 and bug#9964 ? I am having a hard time trying
> to sort out all our LED-related bugs.

I don't think so. This bug is a special case as it modifies the state of the
VCP without going through the physical device (using XTest).
#15359 and #9964 are caused by wrong processing of the device.
Comment 17 Stefan Dirsch 2008-11-22 14:43:10 UTC
CLosing as fixed.
Comment 18 Michel Loiseleur 2010-03-01 08:34:29 UTC
Hi *,

It seems it's broken again on current 7.5 version. If you take and launch the sample program (CapsLockToggle.c), it won't light the LED as it should.

Can someone please re-open the bug ?

Thanks,
Comment 19 Jérôme Guelfucci 2010-07-02 10:58:45 UTC
I can confirm Michel Loiseleur's comment, the original issue is present again with 7.5. I'm reopening the bug.

Thanks!
Comment 20 Stefan Dirsch 2010-09-11 08:03:52 UTC
I can confirm that the behaviour is broken again (LED doesn't lighten up/down) with xorg-server 1.9.0 when running the CapsLockToggle sample.
Comment 21 Stefan Dirsch 2010-09-11 08:06:14 UTC
numlockx works as expected (including LED status) on xorg-server 1.9.0.
Comment 22 Peter Hutterer 2010-10-21 17:36:11 UTC
right, so this situation is actually quite complicated.

First - any XTEST event will be executed on the virtual XTEST device, which will have the LED on. Being virtual, that's of limited use. Since the same event travels through the matching master device (virtual core keyboard (VCK), in most cases), this one will also have the LED on. Of limited, use, it's virtual too.

Using XTEST to switch a physical keyboard's LEDs on or off becomes complicated because each keyboard may have different indicators (and modifiers, etc.) so it's not clear-cut which indicators to switch on or off and under which conditions to do so. Furthermore, because we handle keyboards separately (and the event handling is quite frankly a disaster) there is a disconnect between the actual keyboard devices and the merged master device, the VCK. Hitting CapsLock on a physical keyboard will set the lock on this keyboard and thus on the master device. Sending a CapsLock through XTEST will set CapsLock on the XTEST device but unlock it on the merged device. So now you have two keyboard devices with CapsLock on, but the master device has it off. For indicators, the same is true of course.

I have yet to find a correct solution for the server to handle this case, especially when there are multiple keyboard layouts present. However, there's a simple solution for clients to deal with this: send XTEST events through the matching physical device that forced the CapsLock on. This will unset CapsLock on the physical keyboard and thus also extinguish the LED.
Comment 23 Stephan Sokolow 2013-04-18 23:16:29 UTC
(In reply to comment #22)
> Using XTEST to switch a physical keyboard's LEDs on or off becomes
> complicated because each keyboard may have different indicators (and
> modifiers, etc.) so it's not clear-cut which indicators to switch on or off
> and under which conditions to do so. Furthermore, because we handle
> keyboards separately (and the event handling is quite frankly a disaster)
> there is a disconnect between the actual keyboard devices and the merged
> master device, the VCK. Hitting CapsLock on a physical keyboard will set the
> lock on this keyboard and thus on the master device. Sending a CapsLock
> through XTEST will set CapsLock on the XTEST device but unlock it on the
> merged device. So now you have two keyboard devices with CapsLock on, but
> the master device has it off. For indicators, the same is true of course.

You have my sincere thanks. Now that I finally know what causes the LED to go out of sync, I can trigger it intentionally by running this on login:

numlockx off; xdotool key Num_Lock

I still don't know how to break the modifier state and the LED apart so I can flash the LED to indicate something like unread mail (see Bug 49276), but at least it can warn me if I accidentally disable NumLock (which I only do intentionally in certain games), rather than requiring me to put a piece of tape over it.

(My keyboard is a Rosewill RK-9000I. The LED is one of those ultra-bright, water-clear lens'd blue ones meant more for illumination than use as indicators and the surrounding plastic is white as snow. It'd be bothersome to find pure white electrical tape, ugly to apply enough layers to block the LED, and I don't want void the warranty by desoldering it.)
Comment 24 Pierre Ossman 2017-09-12 14:32:35 UTC
From what I can tell this got fixed some time ago:

https://cgit.freedesktop.org/xorg/xserver/commit/?id=45fb3a934dc0db51584aba37c2f9d73deff9191d

That patch seems to solve the use cases I'm seeing with XTest at least.
Comment 25 Peter Hutterer 2017-09-12 22:19:06 UTC
indeed, that one should've fixed this issue. Thanks for the follow-up.

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.