Fix phantom key presses in winit on focus change (#13299)

[hut]

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.

[github-actions]

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

[SpecificProtagonist]

Nice. This behavior is more useful than using synthetic key presses. This doc comment needs to be updated/removed though.

On Linux/X11

According to the winit doc this is the case on Windows as well.

[alice-i-cecile]

I think we should do this! Those docs should be clearer though; lemme make a PR. There seems to be a minor redundancy in your code, but once that's cleared up it LGTM.

[hut]

I added a call to is_pressed() instead of comparing to ElementState::Pressed, is shorter and avoids an import.

And the ifs do seem redundant, but I stared long at the code and can't see a way to eliminate the duplicate check for is_pressed() though, because statement A uses an AND operation on the two conditions, and statement B uses an OR operation, see below:

// if !synthetic || !pressed
if !(is_synthetic && event.state.is_pressed()) {
    // if !synthetic && pressed
    if event.state.is_pressed() { // "Statement A"
        if let Some(char) = &event.text {
            let char = char.clone();
            #[allow(deprecated)]
            self.winit_events.send(ReceivedCharacter { window, char });
        }
    }
    // if !synthetic || !pressed
    self.winit_events // "Statement B"
        .send(converters::convert_keyboard_input(event, window));
}

The best I can think of is to store the result of event.state.is_pressed() in a variable to avoid computing it twice. Is that what you mean? Though I figure the rust compiler would optimize that for us anyway.

Open to suggestions.

[hut]

Also I want to say: Thanks so much for working on this! It's been a major pain in the neck for me for months ♥

[IceSentry]

How about this? To me that's easier to follow

if !is_synthetic {
    if event.state.is_pressed() {
        if let Some(char) = &event.text {
            let char = char.clone();
            #[allow(deprecated)]
            self.winit_events.send(ReceivedCharacter { window, char });
        }
    } else {
        self.winit_events.send(converters::convert_keyboard_input(event, window));
    }
}

I have absolutely 0 context but you could also flip is_synthetic to early return if that's in a function.

Nevermind, that ignores a case

[IceSentry]

I think this would work?

match (is_synthetic, event.state.is_pressed()) {
    (false, true) => {
        if let Some(char) = &event.text {
            let char = char.clone();
            #[allow(deprecated)]
            self.winit_events.send(ReceivedCharacter { window, char });
        }
    }
    (false, _) | (_, false) => self
        .winit_events
        .send(converters::convert_keyboard_input(event, window)),
    _ => {}
}

[alice-i-cecile]

I think I like the match solution better, but this is fine to merge and a good fix.

[IceSentry]

And final one

if !is_synthetic && event.state.is_pressed()) {
    if let Some(char) = &event.text {
        let char = char.clone();
        #[allow(deprecated)]
        self.winit_events.send(ReceivedCharacter { window, char });
    }
} else if !synthetic || !event.state.is_pressed() {
    self.winit_events.send(converters::convert_keyboard_input(event, window)); 
}

This might be abit clearer in this case, and the is_pressed is cheap enough to not matter.

Also, I'm not sure why there's an allow(deprecated) in there. If you remove that and do the char.clone() in the send() it becomes one line

[alice-i-cecile]

Also, I'm not sure why there's an allow(deprecated) in there. If you remove that and do the char.clone() in the send() it becomes one line

ReceivedCharacter is currently deprecated.

[hut]

@IceSentry careful, your variants change the logic. Note that there is no else before statement B in the original code. The code using match, as well as the "final one" with the else if will only run EITHER statement A OR statement B.

But there is a case where both statements should be run: When !is_synthetic && event.state.is_pressed().

Your "final" variant can easily be fixed by removing the else. But I think match won't work here.

---

But let me outline the reasoning behind my own variant. I think there's value in separating the "quirky" handling of synthetic key press events from the original, "normal" code that handles key presses. The first line:

if !(is_synthetic && event.state.is_pressed()) {

is basically saying: "If it's a synthetic key press, don't do anything at all."

This is the main idea of the patch. To discard synthetic key presses. IMO it should be made clear that we are working around something quirky, rather than mixing the quirk-handling with our "normal" logic.

Everything inside this "if" is just the same code as before the patch.

I have just added some comments, perhaps this makes it more clear:

// Winit sends "synthetic" key press events when the window gains focus. These
// should not be handled, so we only process key events if they are not synthetic
// key presses. "synthetic" key release events should still be handled though, for
// properly releasing keys when the window loses focus.
if !(is_synthetic && event.state.is_pressed()) {
    // Process the keyboard input event, as long as it's not a synthetic key press.
    if event.state.is_pressed() {
        if let Some(char) = &event.text {
            let char = char.clone();
            #[allow(deprecated)]
            self.winit_events.send(ReceivedCharacter { window, char });
        }
    }
    self.winit_events
        .send(converters::convert_keyboard_input(event, window));
}

Eventually, winit will probably remove this quirkiness and we can simply delete the if !(is_synthetic && event.state.is_pressed()) { line to restore normality.

[hut]

Let's merge this? @IceSentry @alice-i-cecile

[alice-i-cecile]

Needs a second review; I'll ask around :)

[kristoff3r]

LGTM