winit icon indicating copy to clipboard operation
winit copied to clipboard

Tracking Issue: Sync key press/release with window focus

Open goddessfreya opened this issue 6 years ago • 22 comments

  • [ ] Implemented on all platforms
    • [x] Windows - #1307
    • [x] ~~macOS~~ (https://github.com/rust-windowing/winit/issues/1272#issuecomment-609384601)
    • [ ] iOS
    • [x] Linux X11 - #1296
    • [ ] Linux Wayland
    • [ ] Web

As mentioned in https://github.com/rust-windowing/winit/pull/1262#pullrequestreview-314593736, however over the window, hold any key, move off the window release the key. You won't get the released key event. Ever.

https://gist.github.com/goddessfreya/39ae1f768cde5bdbc8c9aefa25984824 https://gist.github.com/goddessfreya/0192aa8d9920dba7c933c98190c42d90

cc @murarth

Related: https://github.com/rust-windowing/winit/issues/1077

goddessfreya avatar Nov 10 '19 07:11 goddessfreya

This is a problem on Windows as well. I wouldn't be surprised if we had this issue everywhere.

IMO the simplest solution is to automatically send a KeyUp event for all pressed keys whenever the active window loses focus.

Osspial avatar Nov 14 '19 03:11 Osspial

Is this really an issue winit needs to solve? I feel like applications should be expected to handle this, if appropriate, by responding to Focused events. If we start generating artificial key release events, there's no way to opt out of it.

murarth avatar Nov 14 '19 05:11 murarth

It could be opt in perhaps, by setting a flag or so, or opt-out?

OvermindDL1 avatar Nov 14 '19 16:11 OvermindDL1

Automatically sending KeyUp events just shifts the problem back to KeyDown events. What if the window loses focus, the user releases the key, and then presses it again before refocusing the window? Then you have a KeyUp event without a preceding KeyDown event.

Rua avatar Nov 21 '19 16:11 Rua

@Rua IMO, when a window loses focus, we should send a KeyUp for all keys currently held down. When the window regains focus, all keys currently held down should send a KeyDown. I'm not sure how feasible this is on other platforms, but it should be achievable on X11. This should make it so that every KeyUp has a matching KeyDown and vice versa.

@murarth If we are expecting users to listen to the Focused events and track the keys via DeviceEvent::Key, then what use is WidnowEvent::KeyboardInput?

goddessfreya avatar Nov 22 '19 20:11 goddessfreya

@goddessfreya I'm not suggesting that application developers should use DeviceEvent::Key for all key press responses. I just think that, if an application's behavior is sensitive to focus loss while a key is held, it may be better handled by the application than by winit generating artificial events. I just feel that, if we go down the road of generating artificial events, we may encounter unforeseen complications.

On the other hand, I can't really think of a use case where an application would want the current behavior. And if there's only one "correct" way for applications to workaround this issue, it would make sense for winit to exhibit that behavior itself.

Anyway, if the consensus is in favor of fixing this, I concede the point and will implement this for X11.

murarth avatar Nov 22 '19 21:11 murarth

@murarth I also can't think of a use case for the current behavior. If some applications don't want these artificial events and wish to maintain existing functionality, I think they should use DeviceEvent::Key and just ignore them when focus is gone.

I'm honestly not sure which applications would want the existing behavior, tho. I think most people assume, or at least I did, that when a window looses focus all the keys held will be released, and vice versa for when focus is regained. At the bare minimum, I imagine most people believe that for every key up they should receive a key down, and for every key down there will be a key up (if the user releases the key).

I'm not sure what complications might result from us injecting the occasional key up/down here and there, but if that's a concern, we can mark some WindowEvents as "artificial" with a bool and users can just filter them out.

EDIT: As an alternative implementation, when a window regains focus, we could just issue all the necessary key ups/downs. This strikes me as slightly harder to implement on winit's side and less useful than my other proposed solution.

goddessfreya avatar Nov 22 '19 21:11 goddessfreya

At the bare minimum, I imagine most people believe that for every key up they should receive a key down, and for every key down there will be a key up (if the user releases the key).

Oh this is a big falsehood right there.

Let's take key repeat, like holding a, you'll get a lot of keydown's but only a single key up (if even then).

OvermindDL1 avatar Nov 22 '19 21:11 OvermindDL1

At the bare minimum, I imagine most people believe that for every key up they should receive a key down, and for every key down there will be a key up (if the user releases the key).

Oh this is a big falsehood right there.

Let's take key repeat, like holding a, you'll get a lot of keydown's but only a single key up (if even then).

@OvermindDL1 Maybe I was not being clear, but this is consistent with what I said. If they held down a, there is still a key up for the key downs if the key is released. And the key up will have at least one key down before it.

goddessfreya avatar Nov 22 '19 21:11 goddessfreya

And the key up will have at least one key down before it.

Eh, that's not always the case either. The events from the hardware don't work that way, and thus neither do any window manager I've seen as well, there are always unexpected things. I personally do not want magical keydowns to be sent if focus is lost, I can handle that if I so wish, and in some cases I do, and in some cases I don't. I don't want fake input events. Opt-in maybe, but not opt-out. It should match the expected OS events by default, no fake events by default.

OvermindDL1 avatar Nov 22 '19 23:11 OvermindDL1

I've submitted PR #1296 to address this issue on X11.

The PR adds a field is_synthetic: bool to WindowEvent::KeyboardInput, which is true if and only if winit has generated the event synthetically. I hope this addresses the concerns raised by @OvermindDL1.

murarth avatar Nov 30 '19 20:11 murarth

I've had some time to think on this and I think the three things most common things applications want with regards to keyboard input are the following:

1- I want the user to type in some text into something => use ReceivedCharacter 2- I want to know if a user just pressed a key => use WindowEvent::KeyboardInput as it functions right now. 3- I want to know if a key is currently held => the fact that all key events are ignored if a window is unfocused makes WindowEvent::KeyboardInput unusable for this purpose.

If we inject some artificial events into WindowEvent::KeyboardInput, but explicitly mark them as synthetic, we can make it so that WindowEvent::KeyboardInput easily accommodate both use case 2 and 3.

However, I think this still leaves a major question to be answered: should all keys be marked as released when a window looses focus or should the keys be updated when the window regains focus?

Now, originally I was leaning towards the former, however, now that I've thought about it some more, I propose the following:

1- A is_synthetic field should be added to WindowEvent::KeyboardInput. 2- After issuing a (re)gained focus event, winit should generate an synthetic key up/down for every key that was released/held when the window was not in focus.

Applications only interested in use case 2 can ignore all the synthetic events and maintain the current functionality. Applications interested in use case 3 that only want to update the key state when focus is regained don't have to do anything. Applications that assume when a window loses focus it also loses all the key downs can trivially track the window's focus.

goddessfreya avatar Dec 01 '19 07:12 goddessfreya

I don't see any significant advantages to reporting synthetic key events only on window focus and only for keys that have changed state since loss of focus. Plus, it would mean added complexity of tracking keyboard state on a per-window basis. Persistent keyboard state tracking, if it is done at all, I think, should be done on a global basis, as this is how the platform understands this state (at least on X11).

murarth avatar Dec 01 '19 18:12 murarth

@murarth I like the synthetic field, easy to match on so we can only get false version. That seems good :-)

3 from @goddessfreya seems impossible on some if not most systems (mobile, web, wayland I gather, etc...), you just won't be able to get key state like that, plus that's not how most interfaces work where you get events on change, not current 'state' (if that even makes sense in many contexts). Most OS's make it very very difficult to get keyboard information when not focused to prevent things like reading passwords (though that does bring to mention that many desktop OS's makes this particular security vulnerability very easy, but that's not the case everywhere). Events are the only thing that are guaranteed to work everywhere and that's what should be done as it is most capable as well (key polling is always a bad idea in my opinion).

OvermindDL1 avatar Dec 02 '19 15:12 OvermindDL1

3 from @goddessfreya seems impossible on some if not most systems (mobile, web, wayland I gather, etc...), you just won't be able to get key state like that, plus that's not how most interfaces work where you get events on change, not current 'state' (if that even makes sense in many contexts).

I don't know about other platforms, but on Wayland when you gain focus you are also notified of a list of keys that are currently pressed, so the typical behavior of a wayland app equate the loss of focus with an implicit release of all keys, and reload the pressed keys when it gains focus again.

elinorbgr avatar Dec 17 '19 14:12 elinorbgr

On MacOS this appears to be a non-issue.

If a key is pressed, the window loses focus, and then the key is released, then the OS still sends a keyUp event. The OS must be doing something like tracking which view last received the corresponding keyDown event.

In the opposite case, the OS also doesn't report "keyUp" events if the key was pressed outside the view.

This appears to be true for mouse events as well.

kettle11 avatar Apr 05 '20 09:04 kettle11

@kettle11 That's good to know, thanks! I've marked off macOS on the list.

Osspial avatar Apr 20 '20 22:04 Osspial

Be aware that I've removed this code from the X11 backend in #1932 for the following reasons:

  • This patch removes support for synthetic keyboard events. This feature cannot be implemented correctly:

    • XKB is a complex state machine where key presses and releases can perform a rich set of actions. For example:

      • Switching modifiers on and off.
      • Switching between keyboard layouts.
      • Moving the mouse cursor.

      These actions depend on the order the key are pressed and released. For example, if a key that switches layouts is released before a regular key, then the release of the regular key will produce different events than it would otherwise.

    • The winit API does not permit synthetic ModifierChanged events. As such, an application cannot distinguish between the user deliberately changing the active modifiers and synthetic changes. For example, consider an application that performs a drag-and-drop operation as long as the Shift modifier is active.

mahkoh avatar Dec 02 '21 20:12 mahkoh

For your info, I've just stumbled into this again with #2349: synthetic Pressed events were handled but clearly shouldn't have been in this example.

For what it's worth, I'd like to add my own experience on this issue, mostly from KAS:

  1. Text input is of course handled through ReceivedCharacter
  2. KeyboardInput may be used for shortcut keys which trigger an action when pressed. Normally only the Pressed action does anything, but the Released action may be linked to visual feedback: e.g. a calculator whose buttons are visually depressed so long as the corresponding keyboard key is held. This implies that we only want non-synthetic Pressed events, but any Released event may be used (tested against a map of known depressed key states, keyed by scancode)
  3. Persistent key state might want synthetic Pressed events, but this is probably not important (it's not normal to switch to a window with a key held, perhaps with the exception of Alt used in Alt+Tab).

Which brings up another issue: ModifiersChanged may be used to detect keys like Alt being pressed, ~~but is not generated (on X11) when switching to a window with Alt held~~ (well... this appears to be fixed in the code, but is not according to my testing... no, when switching using the mouse while Alt is held this works as expected, but modifiers are reported as being empty when switching with Alt+Tab).

dhardy avatar Jul 03 '22 15:07 dhardy

I still don't quite understand the original issue and why we need anything like that in winit(synthetic keys). Though in general we can probably approach it in a different way, like having KeyboardFocus or something like that? Not sure how it maps to other platforms, but that would mean that window will receive keyboard input and when KeyboardFocus(false) it would mean that all the keys must not be assumed as depressed?

We already have something like that for pointer, but more in a nature PointerEnter, PointerLeaved.

kchibisov avatar Jul 03 '22 19:07 kchibisov

Well, uniform behaviour across platforms would be a reason: see above. But in that case, we shouldn't generate synthetic key-press events, and should not emit (non-synthetic) key-release events if no corresponding press-event was omitted (the latter is less important from my point of view).

The principle of least surprise may be another reason: naively, people expect a key-release event after a key-press event. But if it's well documented that such events may be missed if the window loses focus then apps can also find their own way of handling this.

On the whole, I don't find the idea of generating synthetic events bad, though synthetic key-down events can be a little unexpected, as seen in #2349.

But this behaviour does comes with certain assumptions, namely that key-press events may be used to trigger actions while the state of a key and key-release actions are less important.

dhardy avatar Jul 04 '22 07:07 dhardy

~~I marked Web as fixed, which was done in #2817.~~ ~~As far as I know all the other backends were addressed as well in #2817.~~

~~I think this can be closed?~~

~~Cc #2662.~~

EDIT: I didn't read this properly, this isn't about modifiers, it's about any key. As far as I know that's not even possible on Web as there isn't an API to query individual keys. We would have to track all keys in Winit to simulate that.

daxpedda avatar Jun 02 '23 18:06 daxpedda