Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove synthetic key events? #3543

Open
madsmtm opened this issue Mar 1, 2024 · 10 comments
Open

Remove synthetic key events? #3543

madsmtm opened this issue Mar 1, 2024 · 10 comments
Labels

Comments

@madsmtm
Copy link
Member

madsmtm commented Mar 1, 2024

Raised in #3538 (comment).

Should we remove synthetic keyboard events (those that contain is_synthetic)?

What are the benefits and the drawbacks?

@kchibisov
Copy link
Member

I'd rather move them out of normal events, because folks handle them but these events shouldn't trigger general input.

@dhardy
Copy link
Contributor

dhardy commented Mar 1, 2024

Synthetic press events: I don't have a use for them.

Synthetic release events (i.e. matching a real press): these are useful to notify that a press-and-hold sequence should be terminated.

@kchibisov
Copy link
Member

Yeah, but you generally should assume that every key is lifted upon focus lost, since that's mostly the only case where it can happen iirc. But yeah, releases make more sense than presses, since people may think that they should input from synthetic presses, while in really it's not the case.

@dhardy
Copy link
Contributor

dhardy commented Mar 1, 2024

Sure, I could handle the key-releases on my end.

I guess the only real thing is whether keys held while the window does not have focus are handled correctly.

I can trick my text editor like this, but it's unimportant: focus another window, press and hold Alt, click to focus the editor, press F. The File menu is opened but the access keys are not underlined like usual.

More important might be something like Blender where modifier keys affect what click+drag does. But to handle that correctly the synthetic key press has to arrive before the mouse-click event, and that's still not good enough to handle cursors (in case the modifier key should change the mouse cursor seen on hover).

It is also possible to handle this stuff without synthetic key presses; they just make that easier (assuming they do the right thing).

@kchibisov
Copy link
Member

You shouldn't handle represses when you had something latched upon focus though, the general recommendation to never do so, because it's always not intended. That's the general advice by core wayland protocol as well and the same with x11, I think.

I can trick my text editor like this, but it's unimportant: focus another window, press and hold Alt, click to focus the editor, press F. The File menu is opened but the access keys are not underlined like usual.

You generally shouldn't replay button presses, because user could use this modifier as a part of e.g. Alt+Tab so you end up releasing and showing the menu on focus from such switcher, this is most of the time not desired from the UX point of view, because user haven't asked for that.

It is also possible to handle this stuff without synthetic key presses; they just make that easier (assuming they do the right thing).

ModifiersChanged is not synthetic and always broadcasted to you and rebroadcasted on focus change, so if you want e.g. shift+mouse drag it should just work, with the exception of unfocused clients where the modifiers are considered empty now, though it'll likely change. Keep in mind that some Wayland compositors don't send modifiers when the application is not focused even when you have mouse pointer above it, though wayland-core allows that. We have #2768 for that and it'll be addressed.

@hut
Copy link

hut commented May 9, 2024

For reference, here is an incomplete list of UI frameworks that mistakenly interpret synthetic key presses as real key presses:

I'm guessing every UI framework will make this mistake, until, after years of subtle customer confusion/annoyance, they realize what is going on and fix the bug on their end.

Perhaps it would be best to hide the synthetic key press events by default.

hut added a commit to hut/bevy that referenced this issue Jun 5, 2024
On Linux/X11, changing focus into a winit window will produce winit
KeyboardInput events with a "is_synthetic=true" flag that are not
intended to be used. Bevy erroneously passes them on to the user,
resulting in phantom key presses.

This patch properly filters them out.

For example, pressing Alt+Tab to focus a bevy winit window results in a
permanently stuck Tab key until the user presses Tab once again to
produce a winit KeyboardInput release event.

The Tab key press event that causes this problem is "synthetic", should
not be used according to the winit devs, and simply ignoring it fixes
this problem.

Reference: https://docs.rs/winit/0.30.0/winit/event/enum.WindowEvent.html#variant.KeyboardInput.field.is_synthetic
Relevant discussion: rust-windowing/winit#3543

Synthetic key **releases** are still evaluated though, as they are
essential for correct release key handling. For example, if the user
binds the key combination Alt+1 to the action "move the window to
workspace 1", places the bevy game in workspace 2, focuses the game and
presses Alt+1, then the key release event for the "1" key will be
synthetic. If we would filter out all synthetic keys, the bevy game
would think that the 1 key remains pressed forever, until the user
manually presses+releases the key again inside bevy.
hut added a commit to hut/bevy that referenced this issue Jun 5, 2024
On Linux/X11, changing focus into a winit window will produce winit
KeyboardInput events with a "is_synthetic=true" flag that are not
intended to be used. Bevy erroneously passes them on to the user,
resulting in phantom key presses.

This patch properly filters them out.

For example, pressing Alt+Tab to focus a bevy winit window results in a
permanently stuck Tab key until the user presses Tab once again to
produce a winit KeyboardInput release event.

The Tab key press event that causes this problem is "synthetic", should
not be used according to the winit devs, and simply ignoring it fixes
this problem.

Reference: https://docs.rs/winit/0.30.0/winit/event/enum.WindowEvent.html#variant.KeyboardInput.field.is_synthetic
Relevant discussion: rust-windowing/winit#3543

Synthetic key **releases** are still evaluated though, as they are
essential for correct release key handling. For example, if the user
binds the key combination Alt+1 to the action "move the window to
workspace 1", places the bevy game in workspace 2, focuses the game and
presses Alt+1, then the key release event for the "1" key will be
synthetic. If we would filter out all synthetic keys, the bevy game
would think that the 1 key remains pressed forever, until the user
manually presses+releases the key again inside bevy.
@alice-i-cecile
Copy link

Can these be emitted as a different event, rather than variants of a keyboard event? That would allow them to be passed through unchanged, but avoid the current footgun.

@kchibisov
Copy link
Member

That what I was saying, we should just route them separately, it's just at the current state of things we can not do much about them for 0.30.

We could still add a warning in docs, but IIRC the presence of such events was desired before #1272.

@dhardy
Copy link
Contributor

dhardy commented Jun 7, 2024

Is a simpler solution possible: never emit synthetic key-press events but do emit synthetic key-release events?

Or make synthetic events opt-in?

github-merge-queue bot pushed a commit to bevyengine/bevy that referenced this issue Jun 17, 2024
# Objective

Fixes #13299

On Linux/X11, changing focus into a winit window will produce winit
KeyboardInput events with a "is_synthetic=true" flag that are not
intended to be used. Bevy erroneously passes them on to the user,
resulting in phantom key presses.

## Solution

This patch properly filters out winit KeyboardInput events with
"is_synthetic=true".

For example, pressing Alt+Tab to focus a bevy winit window results in a
permanently stuck Tab key until the user presses Tab once again to
produce a winit KeyboardInput release event. The Tab key press event
that causes this problem is "synthetic", should not be used according to
the winit devs, and simply ignoring it fixes this problem.

Synthetic key **releases** are still evaluated though, as they are
essential for correct release key handling. For example, if the user
binds the key combination Alt+1 to the action "move the window to
workspace 1", places the bevy game in workspace 2, focuses the game and
presses Alt+1, then the key release event for the "1" key will be
synthetic. If we would filter out all synthetic keys, the bevy game
would think that the 1 key remains pressed forever, until the user
manually presses+releases the key again inside bevy.

Reference:
https://docs.rs/winit/0.30.0/winit/event/enum.WindowEvent.html#variant.KeyboardInput.field.is_synthetic
Relevant discussion: rust-windowing/winit#3543

## Testing

Tested with the "keyboard_input_events" example. Entering/exiting the
window with various keys, as well as changing its workspace, produces
the correct press/release events.
mockersf pushed a commit to bevyengine/bevy that referenced this issue Jun 19, 2024
# Objective

Fixes #13299

On Linux/X11, changing focus into a winit window will produce winit
KeyboardInput events with a "is_synthetic=true" flag that are not
intended to be used. Bevy erroneously passes them on to the user,
resulting in phantom key presses.

## Solution

This patch properly filters out winit KeyboardInput events with
"is_synthetic=true".

For example, pressing Alt+Tab to focus a bevy winit window results in a
permanently stuck Tab key until the user presses Tab once again to
produce a winit KeyboardInput release event. The Tab key press event
that causes this problem is "synthetic", should not be used according to
the winit devs, and simply ignoring it fixes this problem.

Synthetic key **releases** are still evaluated though, as they are
essential for correct release key handling. For example, if the user
binds the key combination Alt+1 to the action "move the window to
workspace 1", places the bevy game in workspace 2, focuses the game and
presses Alt+1, then the key release event for the "1" key will be
synthetic. If we would filter out all synthetic keys, the bevy game
would think that the 1 key remains pressed forever, until the user
manually presses+releases the key again inside bevy.

Reference:
https://docs.rs/winit/0.30.0/winit/event/enum.WindowEvent.html#variant.KeyboardInput.field.is_synthetic
Relevant discussion: rust-windowing/winit#3543

## Testing

Tested with the "keyboard_input_events" example. Entering/exiting the
window with various keys, as well as changing its workspace, produces
the correct press/release events.
@NEON725
Copy link

NEON725 commented Jul 22, 2024

Can these be emitted as a different event, rather than variants of a keyboard event? That would allow them to be passed through unchanged, but avoid the current footgun.

This solution has my vote. Also reduces the boiler plate in the typical case, without limiting functionality for any devs that have a legitimate use case for interpreting these events.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

No branches or pull requests

6 participants