Winit: X11 reporting modifiers after release

[chrisduerr]

It looks like in some cases the state of modifiers is not released correctly, leading to key events having incorrect modifiers attached.

More detail can be found in https://github.com/jwilm/alacritty/issues/2906.

[murarth]

I'm unable to reproduce, but looking at the log linked in the Alacritty issue, I've determined the likely cause of the issue:

• On X11, winit tracks modifier state manually in order to correctly indicate modifiers state when a modifier key is pressed or released.
• To correctly report modifiers state for DeviceEvent::Key and WindowEvent::KeyboardInput, device modifier state and window modifier state must be tracked separately.
• Because winit cannot track window modifier state without window focus, when window focus is lost, winit clears the window modifier state and issues a ModifiersChanged event with an empty modifier set.
• In an attempt to restore correct window modifier state, when focus is gained, winit copies the device modifier state to the window modifier state and reports any modifiers set.
• The log shows the following series of events: DeviceEvent::Key(logo key pressed), Focused(true), DeviceEvent::Key(logo key released), WindowEvent::KeyboardInput(up arrow key pressed, modifier state: { logo: true }). What's happening is that winit adopts the device modifier state (set to logo: true) as window modifier state, but never receives a WindowEvent indicating that the logo key has been released.

If we can't rely on every DeviceEvent::Key having a corresponding WindowEvent::KeyboardInput, it would suggest that we should no longer copy device modifier state to window modifier state when focus is gained. This would mean that, when a window gains focus, its modifier state will always be considered empty, even if the user is holding some modifier keys.

An alternative solution would be, when a window key event is received for a key that is not a modifier, winit could assign the window modifier state to the set of modifiers indicated in the event data. This would correct the issue, but it would mean that any modifier keys pressed before a non-modifier key would incorrectly report logo: true.

Either solution would solve the specific issue reported, but each carries some possibility of incorrectly reporting modifier state in other scenarios. Whether either scenario sounds like something that could actually impact users, I don't know.

[chrisduerr]

This would mean that, when a window gains focus, its modifier state will always be considered empty, even if the user is holding some modifier keys.

I don't think that's an option.

This would correct the issue, but it would mean that any modifier keys pressed before a non-modifier key would incorrectly report logo: true.

Having modifiers incorrectly reported is also going to lead to a lot of headaches I think. Especially because identical behavior on all platforms is desired for sanity.

Whether either scenario sounds like something that could actually impact users, I don't know.

A scenario similar to the first one has already caused bug reports in Alacritty, since then we do our own tracking in addition to winit's. People will run into this constantly.

The second scenario will also cause significant annoyances because it does not allow to correctly track modifier state changes. Alacritty's URL highlighting for example could depend on modifiers pressed, so we need to be able to update the highlighting accurately when modifiers change.

If we can't rely on every DeviceEvent::Key having a corresponding WindowEvent::KeyboardInput, it would suggest that we should no longer copy device modifier state to window modifier state when focus is gained.

I don't quite understand this. Wouldn't it be sufficient to track modifiers by DeviceEvents? Also if the appropriate events aren't sent by X, that seems like it would be an X bug, which likely means this would be a known bug. If there is none, I'm skeptical if the reporting of events is at fault here.

Additionally, winit can always query the active modifiers at all times, so that would always be an option.

[murarth]

I don't quite understand this. Wouldn't it be sufficient to track modifiers by DeviceEvents?

If modifier state is only tracked by DeviceEvents, WindowEvents will have the wrong modifier state, as XI_RawKeyPress (the X11 event that generates DeviceEvent::Key) is received before KeyPress (the event that generates WindowEvent::KeyboardInput).

Additionally, winit can always query the active modifiers at all times, so that would always be an option.

Are you referring to XQueryKeymap? This function performs a request to the X server and waits for a reply. That would introduce more complications into this situation: When would winit send this request? Upon Focused(true)? A modifier key may still be set at that time. And when will the response come? If a KeyPress event is buffered while Xlib waits for the QueryKeymap reply, then what's the modifier state for that KeyPress? If a modifier KeyPress is received, how do we know whether it happened before or after the QueryKeymap reply?

[chrisduerr]

Are you referring to XQueryKeymap? This function performs a request to the X server and waits for a reply. That would introduce more complications into this situation: When would winit send this request? Upon Focused(true)? A modifier key may still be set at that time. And when will the response come? If a KeyPress event is buffered while Xlib waits for the QueryKeymap reply, then what's the modifier state for that KeyPress? If a modifier KeyPress is received, how do we know whether it happened before or after the QueryKeymap reply?

I'm referring to a different request, but that does go through X11 too. And yeah this would not be ideal but it would likely produce much better results than the other things mentioned which always come with direct consequences that severely affect accuracy of reported modifiers. In practice I'd assume this would perform significantly better if used as a sanity check in special cases.

If modifier state is only tracked by DeviceEvents, WindowEvents will have the wrong modifier state, as XI_RawKeyPress (the X11 event that generates DeviceEvent::Key) is received before KeyPress (the event that generates WindowEvent::KeyboardInput).

I'm not entirely sure what you're saying here. If we track through DeviceEvent and DeviceEvent is received first, then everything should be good, right? The only problem would be if we track DeviceEvent and it's received after the KeyPress.

Unless you're referring to things like Shift having modifiers shift set, in which case other platforms already exhibit that behavior. So I don't think that would be too much of an issue either. And it would always be possible to use the modifiers of the previous DeviceEvent in the current KeyPress, using a tiny buffer basically.

But I'd assume I've misunderstood you here?

[murarth]

In practice I'd assume this would perform significantly better if used as a sanity check in special cases.

Performance is not my primary concern about using an X request to determine modifier state. My primary concern is that it will be inaccurate. If a request is made to obtain the modifier state when the Focused(true) event is issued, the current modifier state may be logo: true (as the logs from the Alacritty issue appear to indicate).

I'm not entirely sure what you're saying here. If we track through DeviceEvent and DeviceEvent is received first, then everything should be good, right? The only problem would be if we track DeviceEvent and it's received after the KeyPress.

Huh. I guess you're right about this. In which case, I'm not sure why I ever bothered to track window modifier state as separate from device modifier state. I'll put together a PR to make this change.

[murarth]

I've submitted PR #1262 to address this issue.

[kchibisov]

It was reported to alacritty, that the same issue exists on Wayland. I personally can't repro it, but it's worth checking its modifiers handling too.

[kchibisov]

It turned out that there still a problem under XWayland and the way winit tracks modifiers on X11. So if I start glutin window example on XWayland(WAYLAND_DISPLAY= cargo run --example window, then focus some Wayland client with Win + J on sway, then release Win, and focus window with mouse, winit will report events with logo: true, instead of logo: false.

[chrisduerr]

@kchibisov Might want to open a separate issue, that's usually cleaner than reopening again and again, especially since these two issues are unrelated (platform code is different).

[kchibisov]

@chrisduerr Xwayland uses winit X11 backend iirc, so its again X11 modifiers state problem, so It's the same code.

But anyway I'll open a new one.

[chrisduerr]

Oh, I've just read Wayland and my brain kinda stopped parsing at that point, thanks for the clarification.

I'd assume in XWayland winit is using the Wayland code but might not get DeviceEvents for keyboard interactions that happened on the Wayland side? That sounds like it might be impossible to solve.

@kchibisov Have you tried running Firefox in XWayland and doing something like selecting stuff -> pressing shift -> leaving XWayland window -> entering -> clicking to expand/shrink selection?

[kchibisov]

@chrisduerr Firefox and glfw works fine here.

[chrisduerr]

Then the events are either reported to the application, or they manually request the modifiers at some point.

[kchibisov]

Just clarification on how it works on XWayland + Wayland combo.

When you're pressing Win + J you're getting your DeviceEvent and update modifers.
Then when you focus a Wayland client you wont get any its events, so if you release a Win key in it you also wont get it, this is by Wayland design.
So, without this event, winit fails to track modifiers properly.

The right way to solve this problem would be either to request modifiers manually at some point of time or , I guess (not familiar with X11), get them from Key events(KeyPress), since they're sending modifiers info in state field all the time (man 3 xkeyevent).

[murarth]

I have a fix in mind for this that is not quite perfect, but I think will make this issue unnoticeable in most cases: X11 sends modifier state in a number of events, including key press/release, mouse button press/release, and mouse motion. If winit checks modifier state on these events, it can correct lingering modifier keys. e.g., if we get a KeyPress/ButtonPress/MotionNotify event with logo: false, we know that the logo key was released and can send a delayed ModifiersChanged event to correct the application state.

I'll get started on implementing this in a short while.

[chrisduerr]

if we get a KeyPress/ButtonPress/MotionNotify event with logo: false, we know that the logo key was released and can send a delayed ModifiersChanged event to correct the application state.

KeyPress/ButtonPress likely wouldn't provide any useful results, however MotionNotify likely would. Do we get the modifiers from X on every mouse motion? I'd assume we only want to publish a ModifiersChanged if they aren't the same as the previous events, right?

[kchibisov]

KeyPress/ButtonPress likely wouldn't provide any useful results

Why? I think we should check all the thing for its state and report proper events. Checking only mouse motions wouldn't solve the problem, because you can focus without sending mouse and keyboard event for X11. So, on the next keypress you'll still have a wrong modifier, unless mouse motion. So checking modifiers state and updating in time is the right way to do it, the problem is performance and sending ModifiersChanged in time.

[chrisduerr]

Because it's too late. We need to track it on all key presses ourselves anyways, since ModifiersChanged isn't supported on all platforms yet. So it doesn't make any difference at all to us if a ModifiersChanged event is emitted when a modifier change is detected on key press, so it will not help with resolving any Alacritty problems.

[murarth]

The modifier set from KeyPress events is not entirely useful and winit would need to continue tracking modifier state manually, but the modifier set of KeyPress/KeyRelease events can be used to augment manual modifier tracking. e.g., if the XWayland issue causes winit to miss a KeyRelease for the logo key, but winit then receives a KeyPress with an empty modifier set, it can correct the current modifier state, issuing a ModifiersChanged event and providing the correct ModifiersState values in Key events.

[chrisduerr]

the modifier set of KeyPress/KeyRelease events can be used to augment manual modifier tracking. e.g., if the XWayland issue causes winit to miss a KeyRelease for the logo key, but winit then receives a KeyPress with an empty modifier set, it can correct the current modifier state, issuing a ModifiersChanged event and providing the correct ModifiersState values in Key events.

Of course that makes sense in the long run and winit can do that. I'm just stating that it will not have any effect on Alacritty and doesn't help at resolving the problem, since Alacritty already does this internally.

While winit also sends the modifiers over on mouse motion, Alacritty does not handle them, so that's worth a shot. However this could also be tested in Alacritty first before creating a winit fix by simply overwriting modifiers on mouse motion.

[murarth]

@kchibisov I've submitted PR #1279 with my above described fix. Let me know if resolves the issue for you.