libsdl-org / SDL

Simple Directmedia Layer
https://libsdl.org
zlib License
9.52k stars 1.77k forks source link

SDL3/android: "Return" software key press no longer synthesises a `SDL_EVENT_KEY_DOWN` with `SDL_SCANCODE_RETURN` + `SDLK_RETURN` #10679

Open bdach opened 3 weeks ago

bdach commented 3 weeks ago

Original issue: https://github.com/ppy/osu/issues/28966

TL;DR: At some point after migrating to SDL3, our users started reporting that their enter keys stopped committing text in text boxes. For implementing text commit in textboxes, we rely on key down events with SDL_SCANCODE_RETURN/SDLK_RETURN. In my investigation into the above issue, it appears that at some point (within the last few months for sure, I cannot pinpoint further) SDL stopped returning such events when the software enter key was pressed, instead returning this:

2024-09-02 11:12:42 [verbose]: Unknown SDL key. type=SDL_EVENT_KEY_DOWN scancode=SDL_SCANCODE_RESERVED keycode=SDLK_UNKNOWN raw=0 mod=SDL_KMOD_NONE state=1
2024-09-02 11:12:42 [verbose]: Unknown SDL key. type=SDL_EVENT_KEY_UP scancode=SDL_SCANCODE_RESERVED keycode=SDLK_UNKNOWN raw=0 mod=SDL_KMOD_NONE state=0

In typical Android fashion, this only seems to affect selected software keyboards (as corroborated by @Susko3, another contributor), but notably this affects Gboard, which will be the de-facto standard keyboard on most android devices.

What follows will be a rather terse explanation of what was going on, and some pieces are probably missing (e.g. where this broke and how to fix it), which is why I'm opening this as an issue to discuss further. I apologise for that, but due to the nature of how our project is structured (it being a C# -> C -> Java chimera) even getting a barebones root cause was a question of hours spent.

The pathway through which this goes in the android code is here:

https://github.com/libsdl-org/SDL/blob/deb313dd99ecf4aa4cdbb236bc1f7eae9e7f4087/android-project/app/src/main/java/org/libsdl/app/SDLInputConnection.java#L115-L125

When the soft return key is pressed, the codePoint produced is \n. onNativeSoftReturnKey() will return false unless a specific hint is set:

https://github.com/libsdl-org/SDL/blob/deb313dd99ecf4aa4cdbb236bc1f7eae9e7f4087/src/core/android/SDL_android.c#L1321-L1330

and we're not setting this hint in our project, so this can be excluded from further consideration. Thus, the only remaining pathway is the generic nativeGenerateScancodeForUnichar(), which goes through

https://github.com/libsdl-org/SDL/blob/deb313dd99ecf4aa4cdbb236bc1f7eae9e7f4087/src/core/android/SDL_android.c#L1491

then

https://github.com/libsdl-org/SDL/blob/deb313dd99ecf4aa4cdbb236bc1f7eae9e7f4087/src/events/SDL_keyboard.c#L648

which is

https://github.com/libsdl-org/SDL/blob/deb313dd99ecf4aa4cdbb236bc1f7eae9e7f4087/src/events/SDL_keymap.c#L106-L120

I believe the keycode_to_scancode hashtable is not present / not populated on android, so the next relevant place is the fallback:

https://github.com/libsdl-org/SDL/blob/deb313dd99ecf4aa4cdbb236bc1f7eae9e7f4087/src/events/SDL_keymap.c#L594-L639

which is where the issue probably is clearest - this fallback path does not handle \n, thus coercing it to SDL_SCANCODE_UNKNOWN, and the rest is history.

To possibly add insult to injury, SDLK_RETURN cannot be used in this code, since it is defined to be \r:

https://github.com/libsdl-org/SDL/blob/deb313dd99ecf4aa4cdbb236bc1f7eae9e7f4087/include/SDL3/SDL_keycode.h#L52

so the crude patch I applied to "fix" this looks like so:

diff --git a/src/events/SDL_keymap.c b/src/events/SDL_keymap.c
index dc1b31177..e997bacc6 100644
--- a/src/events/SDL_keymap.c
+++ b/src/events/SDL_keymap.c
@@ -635,6 +635,10 @@ static SDL_Scancode SDL_GetDefaultScancodeFromKey(SDL_Keycode key, SDL_Keymod *m
         return SDL_SCANCODE_DELETE;
     }

+    if (key == (SDL_Keycode)0x0000000a) {
+        return SDL_SCANCODE_RETURN;
+    }
+
     return SDL_SCANCODE_UNKNOWN;
 }

but is also probably very wrong, so please advise as to how this fix should be layered, should you be wanting to see it go in.

AntTheAlchemist commented 1 week ago

Just want to confirm I'm seeing the same.

I wonder when this broke. There were some changes to text input mode a few months ago.

slouken commented 1 week ago

@AntTheAlchemist, are you able to git bisect to see where it changed?

AntTheAlchemist commented 1 week ago

@AntTheAlchemist, are you able to git bisect to see where it changed?

@slouken not very easily, to be honest (because I do it by hand, and C / Java ABI changes will make it fiddly). I'll poke around and find the problem later if no one else has tried.

slouken commented 1 week ago

@AntTheAlchemist, are you able to git bisect to see where it changed?

@slouken not very easily, to be honest (because I do it by hand, and C / Java ABI changes will make it fiddly). I'll poke around and find the problem later if no one else has tried.

Ah, makes sense. I'll take a look when I get a chance. Thanks!