Missing key release events on winit backend results in permanently pressed modifiers

[sodiboo]

When running in the winit backend, we don't always get release events from the outer compositor/window manager.

You can reproduce this issue reliably like so:

1. Open niri nested in niri
   • i use niri myself, and tested this on niri. it might work just as well in other compositors
2. Focus nested niri
3. Press any modifier key, e.g. "Super" (and do not release)
   • nested niri gets a KeyState::Pressed event
4. Click with the mouse cursor on another window in the top-level compositor
5. Release the modifier key
   • nested niri gets no event
6. Focus nested niri again

Now, xkbcommon will be in an annoying state where it reports that modifier as being permanently pressed.

These faulty modifiers are observed by Smithay here:

smithay/src/input/keyboard/modifiers_state.rs

Lines 30 to 41 in 8287457

	impl ModifiersState {
	/// Update the modifiers state from an xkb state
	pub fn update_with(&mut self, state: &xkb::State) {
	self.ctrl = state.mod_name_is_active(&xkb::MOD_NAME_CTRL, xkb::STATE_MODS_EFFECTIVE);
	self.alt = state.mod_name_is_active(&xkb::MOD_NAME_ALT, xkb::STATE_MODS_EFFECTIVE);
	self.shift = state.mod_name_is_active(&xkb::MOD_NAME_SHIFT, xkb::STATE_MODS_EFFECTIVE);
	self.caps_lock = state.mod_name_is_active(&xkb::MOD_NAME_CAPS, xkb::STATE_MODS_EFFECTIVE);
	self.logo = state.mod_name_is_active(&xkb::MOD_NAME_LOGO, xkb::STATE_MODS_EFFECTIVE);
	self.num_lock = state.mod_name_is_active(&xkb::MOD_NAME_NUM, xkb::STATE_MODS_EFFECTIVE);
	self.serialized = serialize_modifiers(state);
	}
	}

I implemented a workaround on compositor side:

• Workaround winit modifiers issue YaLTeR/niri#249

[sodiboo]

Okay, so i don't know how i missed this at first:

xkbcommon is actually counting the number of keys it thinks has that physical key pressed. It needs to receive enough release events to reset the counter to 0 for it to count as unpressed. As such, you can work around this as a user without any changes to the code with the follow process:

0. Is the modifier counted as pressed (> 0 keys pressed) when it isn't really?
   • No: Great! We're done.
   • Yes: Continue
1. Focus outside of the nested compositor with the mouse
2. Press the bugged modifier, and keep it held
3. Focus the nested compositor with the mouse
   • Note that so far it received no keyboard key events
4. Release the modifier
   • The nested compositor receives a KeyState::Released event
   • Its internal key counter decrements
5. Go to step 0.

I don't know how i missed this when i first reported this issue. I even thought that it might be doing exactly this, but somehow concluded that it doesn't, and that the bugged is irreversible and permanent. My testing was not rigorous enough it seems.

This isn't a bug in xkbcommon like it kind of looked like? Note that the reason for my workaround in niri being implemented in that way is because this happened in xkbcommon, for which i didn't investigate its source code. I treated it as a black box, since it's a native library. I didn't see the true nature of the issue as observed by xkbcommon, so my niri workaround relies on just preventing the bad state from ever being sent to xkbcommon, which is a valid workaround, but not a very reliable one.

It seems that this stems from the InputBackend in niri fundamentally being modeled after libinput. It assumes that it has full control over a given peripheral, and that it receives all events. Notably, the winit backend handling keeps track of this "total keys pressed" counter in smithay code:

smithay/src/backend/winit/mod.rs

Lines 467 to 472 in f364c73

	match event.state {
	ElementState::Pressed => self.inner.key_counter += 1,
	ElementState::Released => {
	self.inner.key_counter = self.inner.key_counter.saturating_sub(1);
	}
	};

This is, in general, a violation of the wl_keyboard interface in the core Wayland protocol. Here's a paragraph from it, annotated with how Smithay handles those events.

In the wl_keyboard logical state,

That's the codeblock shown above

this event adds the key to the keys currently logically down (if the state argument is pressed)

This part is handled correctly

or removes the key from the keys currently logically down (if the state argument is released).

This part is handled entirely wrong. Smithay doesn't set the key to logically down

The compositor must not send this event if the wl_keyboard did not have an active surface immediately before this event. The compositor must not send this event if state is pressed (resp. released) and the key was already logically down (resp. was not logically down) immediately before this event.

This part maps to the key count as such:

• KeyState::Pressed means that key_counter was exactly equal to zero and should now be exactly equal to one
• KeyState::Released means that key_counter was exactly equal to one and should now be exactly equal to one.

Note that we do NOT receive events for any count > 1, nor is it guaranteed we receive an event when the boundary between 0 and 1 is crossed (for example, we get nothing when the window is unfocused). It is legal and expected for the compositor to repeatedly send KeyState::Pressed; and this always means that the window wasn't focused when the key was released; and as such every time it is pressed the compositor informs us that "this key has been released since you last had focused, but now it was pressed again". Smithay currently handles such an event by essentially ignoring it; which is entirely wrong.

---

But wait! That last part isn't entirely true. Not when we think of the whole application as a wayland client, in general. But that's the stuff winit tells us. Wait what? Yeah!

The enter event is sent when we gain keyboard focus. Notably, it contains an array of all the pressed keys.

So, the new, refined model is that:

• When we gain focus, we know exactly what keys are pressed
• While we have focus, we get exactly which new keys are pressed and released
• When we lose focus, we have no idea about the keyboard's state until we gain focus again. The modifiers event is sent without focus, so we maybe don't have "no idea", but for a key in general we cannot know
• When we gain focus again, THE COMPOSITOR TELLS US EVERYTHING

So, woo? How can we use this in the Smithay winit backend? Well, we can't. Because, the winit crate does not tell us anything about the new state upon keyboard enter. In fact, it completely ignores all parameters to that event, besides the surface (to figure out which window has focus, i guess, and it does stuff with that). winit doesn't expose this information.

you can't currently make a client that handles keyboard input correcttly in winit because winit literally doesn't give you the information except when a single key is pressed/released.

so this isn't even really so much Smithay's fault. given the winit API, i genuinely don't think what Smithay does looks unreasonable at all.

[Drakulix]

Yeah that pretty much sums up our problems with winit that lead to the X11 backend being created in the first place (with the idea of implementing a wayland backend as well further down the line).

Later we got aware of plans to make it possible to gain access to more internal types of winit or even implementing our own Window (which apparently even landed on git builds of winit already), which appeared to be less of a maintenance burden than building complete custom backends.

So the state of the nested backends we have right now is stuck in a limbo of "barely working" to "good-enough-for-now" until we decide to go through with either solution and somebody actually takes the time to implement all that.