Some key combinations with control generate incomplete events

[ZyX-II]

While working with neovide/neovide#1899 it was found out that neovide from this PR still cannot recognize keyboard shortcuts like <C-]> on some layouts.

Tracing the issue it was found out that for offending shortcuts neovide receives events from winint looking like

KeyEvent {
  physical_key: Digit0,
  logical_key: Character("]"),
  text: None,
  location: Standard,
  state: Pressed,
  repeat: false,
  platform_specific: KeyEventExtra {
    text_with_all_modifers: None,
    key_without_modifiers: Character("]") } }

: note lack of any indication of <C- being pressed and text_with_all_modifiers being absent. (Keyboard layout in question is programming dvorak, with some modifications.) Some more digging led me to finding out that earlier in winint::platform_impl::windows::keyboard::PartialKeyEventInfo::finalize function receives self with utf16parts missing, looking like this:

PartialKeyEventInfo {
  vkey: 48,
  key_state: Released,
  is_repeat: false,
  code: Digit0,
  location: Standard,
  logical_key: This(Character("]")),
  key_without_modifiers: Character("]"),
  utf16parts: [],
  text: System([]) }

and also utterly lacking any indication that this key was pressed while <C- was pressed.

I can reproduce the issue on standard Windows US layout with <C-->: https://gist.github.com/ZyX-II/0ae83a6c1935cdd7ccd64df7ccc683d2. (Log was generated with modifications to neovide from neovide/neovide#1899 (comment), and winit f7a400d with

diff --git a/src/platform_impl/windows/event_loop.rs b/src/platform_impl/windows/event_loop.rs
index 7ed5b820..e9de3ffa 100644
--- a/src/platform_impl/windows/event_loop.rs
+++ b/src/platform_impl/windows/event_loop.rs
@@ -163,7 +163,7 @@ impl<T> ThreadMsgTargetData<T> {
 }
 
 /// The result of a subclass procedure (the message handling callback)
-#[derive(Clone, Copy)]
+#[derive(Clone, Copy, Debug)]
 pub(crate) enum ProcResult {
     DefWindowProc(WPARAM),
     Value(isize),
diff --git a/src/platform_impl/windows/keyboard.rs b/src/platform_impl/windows/keyboard.rs
index 595cd4c5..23a962eb 100644
--- a/src/platform_impl/windows/keyboard.rs
+++ b/src/platform_impl/windows/keyboard.rs
@@ -191,6 +191,7 @@ impl KeyEventBuilder {
                         trace!("Received a CHAR message but no `event_info` was available. The message is probably IME, returning.");
                         return MatchResult::Nothing;
                     }
+                    trace!("w:{wparam:04x} l:{lparam:04x} r:{result:?} ei:{event_info:?}");
                     let pending_token = self.pending.add_pending();
                     *result = ProcResult::Value(0);
                     let is_high_surrogate = (0xD800..=0xDBFF).contains(&wparam);
@@ -490,12 +491,14 @@ impl KeyEventBuilder {
     }
 }
 
+#[derive(Debug)]
 enum PartialText {
     // Unicode
     System(Vec<u16>),
     Text(Option<SmolStr>),
 }
 
+#[derive(Debug)]
 enum PartialLogicalKey {
     /// Use the text provided by the WM_CHAR messages and report that as a `Character` variant. If
     /// the text consists of multiple grapheme clusters (user-precieved characters) that means that
@@ -507,6 +510,7 @@ enum PartialLogicalKey {
     This(Key),
 }
 
+#[derive(Debug)]
 struct PartialKeyEventInfo {
     vkey: VIRTUAL_KEY,
     key_state: ElementState,
@@ -617,6 +621,7 @@ impl PartialKeyEventInfo {
     }
 
     fn finalize(self) -> KeyEvent {
+        trace!("KeyEvent fin: {self:?}");
         let mut char_with_all_modifiers = None;
         if !self.utf16parts.is_empty() {
             let os_string = OsString::from_wide(&self.utf16parts);

)

[ZyX-II]

There are also report by @fredizzimo that similar problem happens on Wayland, but this is probably a different issue.

[ZyX-II]

I am on Windows 10 21H2 build 19044.1526, 64-bit.

[kchibisov]

@ZyX-II you should have ModifiersChanged with Control key before this event. That's how modifiers are indicated. If your keyboard/os doesn't produce ] that's unfortunate.

[ZyX-II]

Looks like offending key combinations come as just WM_KEYDOWN/WM_SYSKEYDOWN which is why utf16parts is missing. Log where I typed <C--><C--><C-->ZZ (<C- was held) with the patch below: https://gist.github.com/ZyX-II/db67748a34f01abe95261ece708832e2.

diff --git a/src/platform_impl/windows/event_loop.rs b/src/platform_impl/windows/event_loop.rs
index 7ed5b820..e9de3ffa 100644
--- a/src/platform_impl/windows/event_loop.rs
+++ b/src/platform_impl/windows/event_loop.rs
@@ -163,7 +163,7 @@ impl<T> ThreadMsgTargetData<T> {
 }
 
 /// The result of a subclass procedure (the message handling callback)
-#[derive(Clone, Copy)]
+#[derive(Clone, Copy, Debug)]
 pub(crate) enum ProcResult {
     DefWindowProc(WPARAM),
     Value(isize),
diff --git a/src/platform_impl/windows/keyboard.rs b/src/platform_impl/windows/keyboard.rs
index 595cd4c5..fe5084dd 100644
--- a/src/platform_impl/windows/keyboard.rs
+++ b/src/platform_impl/windows/keyboard.rs
@@ -124,6 +124,7 @@ impl KeyEventBuilder {
                         return MatchResult::Nothing;
                     }
                     let pending_token = self.pending.add_pending();
+                    trace!("KD w:{wparam:04x} l:{lparam:04x} r:{result:?}");
                     *result = ProcResult::Value(0);
 
                     let next_msg = next_kbd_msg(hwnd);
@@ -136,6 +137,7 @@ impl KeyEventBuilder {
                         &mut layouts,
                     ));
                     let mut event_info = self.event_info.lock().unwrap();
+                    trace!("KD ei:{event_info:?}");
                     *event_info = None;
                     if let Some(next_msg) = next_msg {
                         let next_msg_kind = next_msg.message;
@@ -191,6 +193,7 @@ impl KeyEventBuilder {
                         trace!("Received a CHAR message but no `event_info` was available. The message is probably IME, returning.");
                         return MatchResult::Nothing;
                     }
+                    trace!("w:{wparam:04x} l:{lparam:04x} r:{result:?} ei:{event_info:?}");
                     let pending_token = self.pending.add_pending();
                     *result = ProcResult::Value(0);
                     let is_high_surrogate = (0xD800..=0xDBFF).contains(&wparam);
@@ -490,12 +493,14 @@ impl KeyEventBuilder {
     }
 }
 
+#[derive(Debug)]
 enum PartialText {
     // Unicode
     System(Vec<u16>),
     Text(Option<SmolStr>),
 }
 
+#[derive(Debug)]
 enum PartialLogicalKey {
     /// Use the text provided by the WM_CHAR messages and report that as a `Character` variant. If
     /// the text consists of multiple grapheme clusters (user-precieved characters) that means that
@@ -507,6 +512,7 @@ enum PartialLogicalKey {
     This(Key),
 }
 
+#[derive(Debug)]
 struct PartialKeyEventInfo {
     vkey: VIRTUAL_KEY,
     key_state: ElementState,
@@ -617,6 +623,7 @@ impl PartialKeyEventInfo {
     }
 
     fn finalize(self) -> KeyEvent {
+        trace!("KeyEvent fin: {self:?}");
         let mut char_with_all_modifiers = None;
         if !self.utf16parts.is_empty() {
             let os_string = OsString::from_wide(&self.utf16parts);

[ZyX-II]

@kchibisov So you are saying that this should be fixed on neovide side?

[kchibisov]

@fredizzimo could you reproduce your issue on Wayland with some example in winit with the full log of events?

[kchibisov]

@ZyX-II I'm saying that neovide has all the information, however it would be nice to know why on windows we don't get ^].

[fredizzimo]

I will do later this evening, when I have access to that computer.

But here is what I wrote when I did the testing before, with US layout and the keys <C--> and <C-=>. Note that I also had fcitx enabled when I did it, so the inputs were routed through that. I will test without later.

@ZyX-II, that's very weird. I can repeat the bug with the us layout here as well. But the same mappings work on the Swedish keyboard layout. <C-=> is control+shift+0 on the Swedish layout and still works.

<C-+> also works, which is the same physical keys as on the US layout. And even <C-´>, which is a dead key so I have to press ctrl+´+´ to trigger it.

And I don't see anything in the logs, so somehow Winit must be suppressing the key.

So somehow it seems to be layout dependent.

[ZyX-II]

For Windows it looks like it is just a matter of system choosing to not emit WM_CHAR for some keys when used with modifier and then neovide not knowing how to process keyboard events for keys which are not special and yet have None in text. I checked whether format_key has correct modifiers in self.modifiers and it does. I can easily add something like neovide/neovide#1705 to new code, but again I do not know whether it would not break something - especially with me seeing nothing like self.prev_dead_key which was being checked in that PR and this possible problem being mentioned in neovide/neovide#1899 (comment).

[fredizzimo]

We could check for this in the following order

1. special keys, (ENTER, ESC, F1 and so on) (these are always sent to Neovim with the full modifier state)
2. KeyEvent::text (We always remove shift and in some cases M- on Mac from these)
3. KeyEvent::Logical_key if it's a Key::Character, threat it the same as 2. KeyEvent::Text

But I don't see why we should have to check logical_key if it can be represented by a char.

[kchibisov]

@fredizzimo I think you can check key_without_modifiers and build everything from it? Like you could have internal mappings on how to translate to ^] or something like that.

It's true though, that you might want to have bindings for some weird characters, so you may want text as well to bind weird keys (but I don't see why you'd want to bind anything other than latin in vim anyway (they have keyboard map setting to make it work for non-latin...).

[fredizzimo]

We definitely want the operating system to resolve the keymap specific characters, dor example, to resolve shift and alt-gr together with a key. So I don't think key_without_modifiers is useful at all.

There are plenty of default bindings that would stop working without that on my Swedish keyboard and as far as I know keymap only works for insert mode.

Edit: That makes me wonder what does for example, <C-+> which requires a shift to type produce on an US layout @ZyX-II?

[kchibisov]

keymap only works for insert mode.

That's for normal, so you can navigate on non ascii layouts.

[fredizzimo]

keymap only works for insert mode.

That's for normal, so you can navigate on non ascii layouts.

Yes, but my point was that it solves a different problem and does nothing to help with turning base key + shift state + altgr state to the correct mapping in Vim. So, if we used just key_without_modfiers I would manually have to re-map almost everything. Pretty much all symbols are in different place or use different shift/alt-gr combinations compared to the US layout.

[ZyX-II]

Edit: That makes me wonder what does for example, <C-+> which requires a shift to type produce on an US layout @ZyX-II?

Basically this:

TRACE [neovide::window::keyboard_manager] Format key: KeyEvent { physical_key: ShiftLeft, logical_key: Shift, text: None, location: Left, state: Pressed, repeat: true, platform_specific: KeyEventExtra { text_with_all_modifers: None, key_without_modifiers: Shift } }. Mods: Modifiers { state: ModifiersState(SHIFT | CONTROL), pressed_mods: ModifiersKeys(0x0) }.
TRACE [winit::platform_impl::platform::keyboard] KD w:00bb l:d0001 r:Value(0)
TRACE [winit::platform_impl::platform::keyboard] KD ei:None
TRACE [winit::platform_impl::platform::keyboard] KeyEvent fin: PartialKeyEventInfo { vkey: 187, key_state: Pressed, is_repeat: false, code: Equal, location: Standard, logical_key: This(Character("+")), key_without_modifiers: Character("="), utf16parts: [], text: System([]) }
TRACE [neovide::window::keyboard_manager] Format key: KeyEvent { physical_key: Equal, logical_key: Character("+"), text: None, location: Standard, state: Pressed, repeat: false, platform_specific: KeyEventExtra { text_with_all_modifers: None, key_without_modifiers: Character("=") } }. Mods: Modifiers { state: ModifiersState(SHIFT | CONTROL), pressed_mods: ModifiersKeys(0x0) }.
TRACE [winit::platform_impl::platform::keyboard] KeyEvent fin: PartialKeyEventInfo { vkey: 187, key_state: Released, is_repeat: false, code: Equal, location: Standard, logical_key: This(Character("+")), key_without_modifiers: Character("="), utf16parts: [], text: System([]) }
TRACE [winit::platform_impl::platform::keyboard] KeyEvent fin: PartialKeyEventInfo { vkey: 16, key_state: Released, is_repeat: false, code: ShiftLeft, location: Left, logical_key: This(Shift), key_without_modifiers: Shift, utf16parts: [], text: System([]) }
TRACE [winit::platform_impl::platform::keyboard] KeyEvent fin: PartialKeyEventInfo { vkey: 17, key_state: Released, is_repeat: false, code: ControlLeft, location: Left, logical_key: This(Control), key_without_modifiers: Control, utf16parts: [], text: System([]) }

Full log where I typed <C-+>ZZ

[ZyX-II]

Note: neovide diff changed from previous comment in PR, it is now

diff --git a/src/window/keyboard_manager.rs b/src/window/keyboard_manager.rs
index 3525f08..ddd6f52 100644
--- a/src/window/keyboard_manager.rs
+++ b/src/window/keyboard_manager.rs
@@ -67,6 +67,7 @@ impl KeyboardManager {
     }
 
     fn format_key(&self, key_event: &KeyEvent) -> Option<String> {
+        log::trace!("Format key: {key_event:?}. Mods: {:?}.", &self.modifiers);
         if let Some(text) = get_special_key(&key_event.logical_key) {
             Some(self.format_key_text(text, true))
         } else {

. Basically I just added “Mods: …” part with current modifiers.

[ZyX-II]

Logs are currently collected by running

cargo +stable-x86_64-pc-windows-msvc run --release -- --log -- -u NONE -i NONE -N

with $env:RUST_LOG = 'neovide::window::keyboard_manager, winit'.

[fredizzimo]

That's good news. I was afraid that it would return the base key. But the logical key has the correct information. So using that as a backup could work in Neovide. Still i think that according to the winit documentation it should also include the correct text.

[fredizzimo]

I won't be able to test wayland until after the weekend. But I did some testing on Windows using spy++, I can confirm that WM_CHAR is not sent for the offending characters.

I found that on Windows none of the control + number mappings work with my Swedish keyboard layout. But the alpha keys can be mapped. Then I went to http://www.kbdlayout.info to check the Swedish keymap. I found out that it's actually possible to map Control to a character, and disallow it completely with -1, as can be seen for the numbers here http://www.kbdlayout.info/kbdsw/download/klc. So, I believe this solves the mystery why it works the way it does currently, at least on Windows.

Now, it's more of a winit design decision to choose if text should be valid or not. In Neovide I think we can work both ways, I already did some local changes and can confirm that using virtual_key as a backup fixes the problem in Neovide.

[dhardy]

I think the issue title is inaccurate?

Anyway, I have run into basically the same thing:

• Make a keyboard shortcut matching 1
• Press AltGr+N (maps to "1" on my keyboard; basically a stealth numpad)
• This does not match against key_without_modifiers which returns Character("n")

On the other hand, does use of logical_key for modifiers make sense? Matching Ctrl+Z requires using lower-case z. This is probably workable, though confusing given how common it is to display shortcut keys with upper-case letters.

Using logical_key then, I have no problem matching Ctrl+].

[kchibisov]

I think the original issue was about some special layout on Windows and the fact that some keys were not reaching the users of winit.

@dhardy logical keys is what should be used for mappings. If you want to ignore shift, in the current revision the only way to do so, is to lowercase the character yourself, which is more or less ok...

[fecet]

I think it's relevant to neovide/neovide#1290, and https://github.com/joshgoebel/keyszer can work (with some little bug)

I have the same problem on wayland with xremap