Preserve modified keys when modifier and key are sent simultaneously on Wayland

[JeffDess]

This is an attempt to fix #7258, in which shifted keys sent by a programmable keyboard are dropped half of the time on Wayland/Fcitx.

Required setup to reproduce:

1. Wayland (I am using Hyprland)
2. ZMK keyboard over bluetooth, with a keybinding that sends &RPAR
3. Fcitx with "Forward Key event instead of committing text if it is not handled" unchecked in "Wayland Input Interface Method" settings

Behavior

When opening a Kitty terminal, typing ")" once does nothing, typing it again displays the ")" character. Repeating this key sequence invariably yields the same result.

Let's analyze just those two first keypresses.

Observations

wev

Here's the wev output for the first keypress:

[14:     wl_keyboard] key: serial: 9411; time: 34465426; key: 50; state: 1 (pressed)
                      sym: Shift_L      (65505), utf8: ''
[14:     wl_keyboard] modifiers: serial: 0; group: 0
                      depressed: 00000001: Shift
                      latched: 00000000
                      locked: 00000000
[14:     wl_keyboard] key: serial: 9413; time: 34465426; key: 19; state: 1 (pressed)
                      sym: parenright   (41), utf8: ')'
[14:     wl_keyboard] key: serial: 9414; time: 34465426; key: 50; state: 0 (released)
                      sym: Shift_L      (65505), utf8: ''
[14:     wl_keyboard] modifiers: serial: 0; group: 0
                      depressed: 00000000
                      latched: 00000000
                      locked: 00000000
[14:     wl_keyboard] key: serial: 9416; time: 34465426; key: 19; state: 0 (released)
                      sym: 0            (48), utf8: ''

And the second keypress:

[14:     wl_keyboard] key: serial: 9417; time: 34467136; key: 50; state: 1 (pressed)
                      sym: Shift_L      (65505), utf8: ''
[14:     wl_keyboard] modifiers: serial: 0; group: 0
                      depressed: 00000001: Shift
                      latched: 00000000
                      locked: 00000000
[14:     wl_keyboard] key: serial: 9419; time: 34467136; key: 19; state: 1 (pressed)
                      sym: parenright   (41), utf8: ')'
[14:     wl_keyboard] key: serial: 9420; time: 34467136; key: 50; state: 0 (released)
                      sym: Shift_L      (65505), utf8: ''
[14:     wl_keyboard] key: serial: 9422; time: 34467136; key: 19; state: 0 (released)
                      sym: 0            (48), utf8: ''

Serials:

• 1st: 9411, 9413, 9414, 9416 (+2, +1, +2)
• 2nd: 9417, 9419, 9420, 9422 (+2, +1, +2)

Events:

• 1st: Press Shift, Press ), Release Shift, Release 0
• 2nd: Press Shift, Press ), Release Shift, Release 0

Timestamps:

• 1st: 34465426, 34465426, 34465426, 34465426
• 2nd: 34467136, 34467136, 34467136, 34467136

Everything looks in order, but let's take note that all of the events of a keypress are sent with the same timestamp, which is not possible with a regular keyboard (this is an optimization for bluetooth communication, since the shifted key is triggered with a single keypress instead of two).

Kitty

In Kitty, here's how it goes:

First keypress:

Press xkb_keycode: 0x2a clean_sym: Shift_L composed_sym: Shift_L mods: none glfw_key: 57441 (LEFT_SHIFT) xkb_key: 65505 (Shift_L)
on_key_input: glfw key: 0xe061 native_code: 0xffe1 action: PRESS mods: shift text: '' state: 0
text-input: updating cursor position: left=44 top=458 width=10 height=21
ignoring as keyboard mode does not support encoding this event
text-input: commit_string event: text: )
text-input: done event: serial: 2 current_commit_serial: 3
Release xkb_keycode: 0x2a clean_sym: Shift_L mods: none glfw_key: 57441 (LEFT_SHIFT) xkb_key: 65505 (Shift_L)
on_key_input: glfw key: 0xe061 native_code: 0xffe1 action: RELEASE mods: none text: '' state: 0 ignoring as keyboard mode does not support encoding this event
Release xkb_keycode: 0xb clean_sym: 0 mods: none glfw_key: 48 (0) xkb_key: 48 (0)

Second keypress:

Press xkb_keycode: 0x2a clean_sym: Shift_L composed_sym: Shift_L mods: none glfw_key: 57441 (LEFT_SHIFT) xkb_key: 65505 (Shift_L)
on_key_input: glfw key: 0xe061 native_code: 0xffe1 action: PRESS mods: shift text: '' state: 0 ignoring as keyboard mode does not support encoding this event
text-input: commit_string event: text: )
text-input: done event: serial: 3 current_commit_serial: 3
on_key_input: glfw key: 0x0 native_code: 0x0 action: PRESS mods: none text: ')' state: 2 committed pre-edit text: )
Release xkb_keycode: 0x2a clean_sym: Shift_L mods: shift glfw_key: 57441 (LEFT_SHIFT) xkb_key: 65505 (Shift_L)
on_key_input: glfw key: 0xe061 native_code: 0xffe1 action: RELEASE mods: none text: '' state: 0
text-input: updating cursor position: left=54 top=458 width=10 height=21
ignoring as keyboard mode does not support encoding this event
Release xkb_keycode: 0xb clean_sym: 0 mods: none glfw_key: 48 (0) xkb_key: 48 (0)

Events

• 1st: Press Shift, Update cursor position, Commit string ), Release, Release
• 2nd: Press Shift, Commit string ), Release, Update cursor position, Release

Serials

• 1st: Serial 2, Commit serial 3
• 2nd: Serial 3, Commit serial 3

Something's not right. For some reason, the first and second keypress events are not handled the same way. I'm no expert at this and I have not found the site of this issue yet. This could come from Kitty but this could likely be in the Compositor/IME chain. Guidance or enlightment would be quite welcomed here.

What is happening?

1. In the first keypress, the cursor position is updated before committing, but it's the opposite in the second keypress.
2. Upon committing, the commit_serial counter is incremented.
3. When serial and commit_serial doesn't match Kitty drops the pre-edit text.

So that explains it, but I do not know why this was implemented. From what I deducted, it was to mute warnings on a specific contexts?

Solutions

There are multiple ways to fix this:

1. What I'm suggesting in this PR is probably the less involved solution. Let's render the pre-edit text anyway, even if in case of a serial count mismatch. Would it have any adverse effect? In which situation wouldn't we want to render text that the user has typed?

2. My second option would be not to commit on cursor position updates. In my tests, it was unecessary to commit at this point and it's the source of the mismatch, but surely I am missing some use cases where it is useful. Perhaps skipping the commit step on some condition would be less drastic, which leads me to the next idea.

3. In keys.c, the case GLFW_IME_NONE updates the IME position for MacOS and Fig (RIP). If a condition could be met to skip this action for other contexts, the bug would be prevented as the cursor position would not be updated.

I'm sure there's more ways to fix it, but for now I'd be interested to hear your take on this.

Both fixes work equally well from a user perspective. Codewise, option 1 is only a patch for a mishandling that has already happened and the log will still be mixed up. Option 3 yields a more cleaned up events order (identical log for each keypress).

Thanks for taking the time to review this and provide some feedback!

[kovidgoyal]

OK I will look at it when I have a moment.

[kovidgoyal]

1. See https://wayland.app/protocols/text-input-unstable-v3#zwp_text_input_v3:event:done best I can tell, the spec says serial must be same as commit_serial, otherwise dont commit. But I cant be sure the language in that spec, like most wayland specs, is unclear.
2. We must commit on cursor position changes otherwise how is the IME supposed to move the input box alongwith the cursor?
3. This is a good to have for a future Fig. In general, keeping the system informed of the cursor position seems potentially useful.

So I am not really convinced by your proposed fixes, except maybe 1 which needs more investigation. It would be good if we can get some clarity on the spec from whoever maintains it. Or at least check what other toolkits like Qt, GTK, winint, etc. do so we can "follow the herd".

[kovidgoyal]

Pinging @wengxt they may have some useful comments.

[JeffDess]

Thanks a lot for the review. I'll make sure to read those docs. I don't feel there's anything wrong about the implementation in wl_text_input. If the input was consistent, it would work just fine, but it's not and the spec doesn't account for that. Option 1 is a workaround to patch a symptom, if it does no harm then it just works. But I'd feel better about preventing the bug at the source, it is not normal that this part receives different inputs for a same keypress. I'll continue investigating higher up the chain, maybe I'll find something useful.

[JeffDess]

I agree the wording in the docs is unclear. But if I understand correctly, option 1 might be on the right track:

When the client receives a done event with a serial different than the number of past commit requests, it must proceed with evaluating and applying the changes as normal, except it should not change the current state of the zwp_text_input_v3 object.

And they go on refining the concept of state:

All pending state requests (set_surrounding_text, set_content_type and set_cursor_rectangle)[...]

They also state that commit is 'applying changes' without changing the state:

Atomically applies state changes recently sent to the compositor.
Neither current nor pending state are modified unless noted otherwise.

Since in text_input_done none of these setters are called, in order to "evaluate and apply the changes as normal", I guess the early return in the serial check shouldn't be there and the entire function should be allowed to run even in case of a mismatch? Does any of this make sense?

I've done a preliminary check of a few terminals and toolkits, and it doesn't seems there's a clear standard to follow. I've not digged into the entire code bases but it looks like GTK does what I just stated but Qt would stop processing in case of a mismatch. 🤷🏼‍♂️

I also figured out why the second keypress works and no the first (impression of different inputs for same keypress). It's the actually the same data: The first keypress is blocked because of the serial mismatch, the second keypress sends the same input and that unblocks the first one:

1st: text-input: done event: serial: 2 current_commit_serial: 3
2nd: text-input: done event: serial: 3 current_commit_serial: 3

So I think it comes down to either sending the text in case of a mismatch, or finding the root cause of the serial difference.

[kovidgoyal]

I fixed it slightly differently from your proposal, please have a look and let me know if you feel there are any remaining issues.

[JeffDess]

I've tested it and it works. The logs are outputting the same events sequence for a same keypress. LGTM, thanks a lot for taking the time to look into this. This has been bugging me for months but it seems like it's finally over.

[kovidgoyal]

thanks for doing the legwork!