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

Tracking Issue: Sync key press/release with window focus #1272

Open
3 of 7 tasks
goddessfreya opened this issue Nov 10, 2019 · 22 comments
Open
3 of 7 tasks

Tracking Issue: Sync key press/release with window focus #1272

goddessfreya opened this issue Nov 10, 2019 · 22 comments
Labels

Comments

@goddessfreya
Copy link
Contributor

goddessfreya commented Nov 10, 2019

As mentioned in #1262 (review), 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: #1077

@goddessfreya goddessfreya added B - bug Dang, that shouldn't have happened DS - x11 C - needs investigation Issue must be confirmed and researched labels Nov 10, 2019
@Osspial Osspial changed the title WindowEvent does not register Key release if looses focus. WindowEvent does not register Key release if loses focus. Nov 14, 2019
@Osspial
Copy link
Contributor

Osspial commented Nov 14, 2019

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.

@murarth
Copy link
Contributor

murarth commented Nov 14, 2019

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.

@OvermindDL1
Copy link

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

@Rua
Copy link

Rua commented Nov 21, 2019

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.

@goddessfreya
Copy link
Contributor Author

goddessfreya commented Nov 22, 2019

@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?

@murarth
Copy link
Contributor

murarth commented Nov 22, 2019

@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.

@goddessfreya
Copy link
Contributor Author

goddessfreya commented Nov 22, 2019

@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.

@OvermindDL1
Copy link

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).

@goddessfreya
Copy link
Contributor Author

goddessfreya commented Nov 22, 2019

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.

@OvermindDL1
Copy link

OvermindDL1 commented Nov 22, 2019

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.

@murarth
Copy link
Contributor

murarth commented Nov 30, 2019

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.

@goddessfreya
Copy link
Contributor Author

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.

@murarth
Copy link
Contributor

murarth commented Dec 1, 2019

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).

@OvermindDL1
Copy link

@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).

@elinorbgr
Copy link
Contributor

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.

@Osspial Osspial changed the title WindowEvent does not register Key release if loses focus. Tracking Issue: Sync key press/release with window focus Dec 29, 2019
@kettle11
Copy link

kettle11 commented Apr 5, 2020

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.

@Osspial
Copy link
Contributor

Osspial commented Apr 20, 2020

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

@mahkoh
Copy link
Contributor

mahkoh commented Dec 2, 2021

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.

@dhardy
Copy link
Contributor

dhardy commented Jul 3, 2022

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).

@kchibisov
Copy link
Member

kchibisov commented Jul 3, 2022

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.

@dhardy
Copy link
Contributor

dhardy commented Jul 4, 2022

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.

@daxpedda
Copy link
Member

daxpedda commented Jun 2, 2023

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.

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