Update modifiers before propagating key press events

[chrisduerr]

Currently, when receiving WindowEvent::KeyboardInput events, it is not possible to detect when all shift keys have been released unless manually tracking the shift key state. This is the case for all modifiers.

The cause of this is that the modifiers get updated after the events are sent, which means that shift press key events do not have the modifiers shift set, but shift release events always do have them set. As a result, it's impossible to know if RShift is still held when the released event for LShift comes in.

My proposal to solve this would be to just update the shift state before sending the key events. This would mean that when LShift comes in and the shift modifier is still set, RShift must still be held.

Of course this is a breaking change and I'm not sure what inspired the initial design, which is why I haven't looked into the code changes myself yet. But are there any specific reasons why this would be a bad idea?

[elinorbgr]

First of all, I think we could try to asses the exact current situation, because it is likely that not all platforms behave the same way regarding that... Which platforms have you observed this on?

[chrisduerr]

This was tested on Linux/X11 (i3), but I also checked Linux/Wayland (sway) and it behaves exactly the same way. So I kinda just assumed that this would be consistent across all platforms.

It seems like there's mostly macOS/Windows left only, I can take a quick look at macOS if that's of interest.

[chrisduerr]

Okay, testing macOS was a good idea. Turns out macOS behaves exactly the way I have proposed here,s it seems like this is a platform inconsistency issue.

Is there a process for standardizing things across platforms if there are inconsistencies detected between them? I'd selfishly say that if it hasn't been reported as a bug, the macOS way doesn't seem to cause any trouble and there's obviously benefits to it for me.

[ghost]

The current behaviour on Linux doesn't sound right to me. The proposed behaviour sounds more reasonable. I would suggest checking Windows as well. Without putting too much thought into it, I'm for standardising the macOS behaviour across all backends.

[Osspial]

I can confirm that the proposed behavior is what happens on Windows.

[goddessfreya]

cc @murarth

[murarth]

I'm looking into this now.

[murarth]

So, the reason that winit reports modifier state the way that it does on X11 is simply because that's how X11 reports it (source; "The state member is set to indicate the logical state of the pointer buttons and modifier keys just prior to the event").

However, the X server does provide information to clients indicating which keycodes correspond to which modifiers (source), so it should be possible for winit to reliably track the state of each modifier key and report modifier state in the proposed manner.

I'll get started on a PR for this soon.

[chrisduerr]

I've got a working implementation using XkbGetState to query the modifiers after a key was pressed whenever the key is pressed/released. I thought this would be a nicer way to track all modifiers without having to store left/right state independently.

I can create a PR in a bit to show how it works, though it requires a patch to x11_dl first.

[murarth]

@chrisduerr Tracking modifier and keypress state may be a bit more work, but it means that winit has the modifier state when it receives a KeyPress/KeyRelease event, without having to send a request to the X server and wait for a reply, as XkbGetState does (source).

[murarth]

I've submitted a PR that fixes this issue for X11, but not Wayland (#1129).

A fix for Wayland should be able to reuse the ModifierKeymap and ModifierKeyState structs added in that PR. However, I'm at a loss for how to access keycode-to-modifier mappings using the Wayland client API.

[elinorbgr]

Well, that's going to be odd for Wayland: they way the protocol is designed is that we basically have to trust the server for the state of the modifiers, and I suppose whether the modifiers are updated before or after the key press/release event may be compositor dependent.

However, I'd like to point that tracking the state of LShift / RShift by using the Shift modifier seems like a fragile way to do it, and a misuse of the feature.

It would for example completely break in the presence of accessibility features some desktop environment may have: Sticky Keys.

So, in general, I think one cannot assume that the state of the modifiers is directly related to the physical state of the associated keys on the keyboard. If you need to know if RShift / LShift is still pressed, the correct way would likely be to actually track it.

[chrisduerr]

whether the modifiers are updated before or after the key press/release event may be compositor dependent.

This sounds really strange. It seems like something so important, it should definitely be part of the specification.

However, I'd like to point that tracking the state of LShift / RShift by using the Shift modifier seems like a fragile way to do it, and a misuse of the feature.

I don't see how this would break things like sticky keys. The point of tracking L/RShift in Alacritty is to get a definite answer about modifier state when the modifier is released. Even with stick keys that should be possible.

So, in general, I think one cannot assume that the state of the modifiers is directly related to the physical state of the associated keys on the keyboard.

This is precisely not the case though. When shift is released, on X11 and in Sway, the modifiers currently still list shift as active. So the modifier state does not track the latest state of the keyboard at the moment. All we need in Alacritty is some way to know about the modifier state as soon as the last modifier key is released.

[elinorbgr]

So the modifier state does not track the latest state of the keyboard at the moment.

Well, that is the point I'm trying to make: the modifier states does not track the state of the keyboard. If Sticky Keys are active the Shift modifier would remain active even after the last Shift key is released.

Arguably, the most correct way to handle it would be winit to have a ModifiersChanged event, rather than plugging modifiers as a property of other input events.

[chrisduerr]

Well, that is the point I'm trying to make: the modifier states does not track the state of the keyboard. If Sticky Keys are active the Shift modifier would remain active even after the last Shift key is released.

This is perfectly fine. We just need to know when the shift modifier is active based on key events. Otherwise you'll always have to wait for the next mouse or keyboard event to know about modifier updates. As a result, it's impossible for us to change the mouse cursor when temporarily disabling mouse mode by holding the shift key.

[Osspial]

Perhaps the best solution would be to split ModifiersChanged into a separate event?

[chrisduerr]

That would solve our usecase. Though so would the solution proposed in this issue.

There's not actually any problem in this proposal with sticky keys or similar, so there might not be a necessity for a separate event.

However, a separate event might solve some usecases that Alacritty does not have at the moment. I obviously can only speak for our needs, but I can totally see some usecases that might benefit from a separate event.

[elinorbgr]

A separate event would be the only way to make it reliable on Wayland though: we don't have any other source of truth for the modifier status than what the server tells us.

[chrisduerr]

A separate event would be the only way to make it reliable on Wayland though: we don't have any other source of truth for the modifier status than what the server tells us.

How would you generate this event? If the key press of shift/ctrl/alt/... does not generate a modifier change, how would winit ever know to generate such an event?

[elinorbgr]

For Wayland, the server already sends a "modifiers changed" events, which is not directly correlated to key presses. That's why I'd expect some compositors to send it just before or just after the keyboard event that changed the modifiers.

For the other platforms, I don't know them. If making such an event is not possible in a cross-platform way, I'm not sure how we could unify all that tbh.

[Osspial]

On platforms that don't send modifier events, we could just hook into the keypress events and send them then.

[murarth]

I did some testing with Sticky Keys on X11 and it seems that it doesn't affect the ModifiersState value at all. Pressing Shift, followed by a correctly produces a ReceivedCharacter('A'), but also produces a Key event with shift: false. The same input with the changes in my PR produces the same result.

[murarth]

I agree that adding a ModifiersChanged event may be the right way to go. It can be implemented more easily on Wayland, certainly. But on X11, it would be implemented in the same way I've implemented the solution in my PR, by tracking press/release events for modifier keys.

EDIT: Oh, this is kind of exactly what Osspial just said. Sorry. Don't mind me.

[chrisduerr]

Just as a reference, these are all the changes required for the approach I've investigated:
chrisduerr@fcb9d1d

I thought it might be worth exploring if the latency introduced by this would be worth the reduced complexity by this approach. Especially as a separate event this might be worthwhile since latency is less relevant.

Generally I think that @murarth's approach is very reasonable, though it does create some significant code for managing and tracking the modifier state outside of what X11 does already.

Might be worthwhile to move this to the PR though, but I just thought I'd link the commit here for evaluation, I'm personally not really invested in the actual solution as long as it is resolved.

[murarth]

I've run into an interesting conundrum in implementing a ModifiersChanged event: What kind of event is it?

It's not specific to a window, so it's not a WindowEvent. At least in X11, it's not specific to a device, either. There seems to be a shared global modifier state. If you have two keyboards plugged into one computer, holding Shift on one keyboard influences input from either keyboard. So, it's not a DeviceEvent, either.

So, would it just be an Event? Seems odd, but I'm not aware of a more sensible alternative.

EDIT: On second thought, maybe it does work as a WindowEvent. Any edge cases involving modifier keys also apply to any other keyboard input and we seem to have accepted that possible weirdness.

[murarth]

I've submitted a PR (#1132) implementing the ModifiersChanged event for X11 and Wayland.

[Osspial]

One more thing to add - if we're adding ModifiersChanged, should we remove the modifiers fields from the other WindowEvents that expose it? I'm leaning towards yes, since the new event makes those fields redundant.

[murarth]

I don't think the modifiers field should be removed from other WindowEvent variants. I think it's still a nice convenience for applications that aren't interested in the ModifiersChanged event itself.

[Osspial]

I'd like to argue for removing the modifiers field from non-ModifiersChanged events again, since the recent discussion in #833 has brought this sort of topic to the forefront of my mind. I outlined my reasoning for wanting to avoid redundant sources of information in events, such as modifiers when ModifiersChanged exists, in #883 (comment), and for the sake of readability I've copied the relevant section of the comment down below.

I think the main issue with this idea is that it forces users to deal with multiple sources of truth.

I agree with this sentiment.

In my mind, there are two ways Winit can expose device input state:

1. Expose everything through events, and require the user to derive the device state through those events.
2. Have Winit keep track of device state, and allow the user to query the current device state at any time.

Either of those options are acceptable. However, not committing to either option in particular and internally keeping track of some device state for some events is far, far worse than going all-in on either option, since that makes the API harder to predict. It forces users to reference the documentation to check and see if a value is exposed in an event, rather than assuming that a particular event will only store information related to that event.

That reasoning absolutely applies in this situation. If we're exposing modifiers in multiple locations, we're forcing the user to deal with multiple sources of truth and adding additional internal and external complexity to our API that simply does not need to be there.

[chrisduerr]

As a client, I don't see any problem with not having modifiers on other functions. Though Alacritty is complex enough that we store the modifiers ourselves anyways, a valid concern might be for "simpler" applications which will need to store modifiers across events.

Fundamentally I think the original design without a dedicated event is probably better for simple non-wayland platforms when you're able to report the modifiers on the mouse down of the modifier key event itself.

However since that doesn't seem to be possible based on my Wayland understanding, I think a dedicated event without any other modifiers anywhere else is the next best thing and likely the best way forwards for winit. Though obviously this would need to be implemented on all platforms and I can't predict potential issues with platforms I'm not familiar with.