-
Notifications
You must be signed in to change notification settings - Fork 64
Only prevent the default browser event handling when the specific event types aren't disabled by the user #10
Conversation
} | ||
} | ||
|
||
/* if we prevent keydown, we won't get keypress | ||
* also we need to ALWAYS prevent backspace and tab otherwise chrome takes action and does bad navigation UX |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think preventing default backspace and tab handling even if the user ignores SDL_KEYUP / SDL_KEYDOWN events is not a good idea, but this is debatable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change breaks text input, we need to make sure that we don't prevent keydown otherwise we won't get any keypress events.
Something like this should fix text input and also only prevent default if KEYUP/DOWN are enabled.
SDL_bool prevent_default = SDL_GetEventState(eventType == EMSCRIPTEN_EVENT_KEYDOWN ? SDL_KEYDOWN : SDL_KEYUP) == SDL_ENABLE;
if (eventType == EMSCRIPTEN_EVENT_KEYDOWN && SDL_GetEventState(SDL_TEXTINPUT) == SDL_ENABLE && keyEvent->keyCode != 8 /* backspace */ && keyEvent->keyCode != 9 /* tab */)
prevent_default = SDL_FALSE;
return prevent_default;
Some returns of 0 and of 1 are replaced with dynamic checks here - it seems like that could be surprising, we are changing the current behavior? |
Yes, we are changing it like discussed at #4. I didn't notice that one of the returns I changed was a |
To my understanding, the changes here should be good. If an application has set to ignore specific events altogether, I think it should be just safe to run the default browser behavior in that case. |
@@ -420,6 +425,8 @@ Emscripten_HandleTouch(int eventType, const EmscriptenTouchEvent *touchEvent, vo | |||
xscale = window_data->window->w / client_w; | |||
yscale = window_data->window->h / client_h; | |||
|
|||
bool preventDefault = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be SDL_bool
, SDL_TRUE
/SDL_FALSE
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I forgot C doesn't have booleans. Would SDL_bool
really make more sense than int
here though, as we have return type int and return this variable in the end?
Alright, the first test was kind of successful. With
I can allow the mousewheel to function normally, and with
I can allow scrolling and zooming on my smartphone. Interestingly, my smartphone seemed to trigger However, I wasn't able to make emscripten allow the default browser actions for keys – I could never navigate the page with my arrow keys, no matter which keyboard events were ignored. I don't know why. I actually tested this with a game that doesn't use keyboard inputs at all, though I don't care a lot whether they are usable for page navigation. Should we revert the keyboard-related changes as they don't work? Also, there is one other thing I'm unsure about: Desktop tests were done in Firefox, Smartphone tests in the Ubuntu Phone default browser app |
|
@Daft-Freak Okay, so what do you think should be returned in that function? |
|
…nt types aren't disabled by the user
…USEMOTION is enabled
Alright, fixed that and also did a rebase, as my fork had merge conflicts. Which means that the only thing left to look after before this can be merged is the keyboard event stuff. For convenience, here is what I wrote about that earlier:
|
It seems to be working in Chrome, but not FIrefox. |
Okay, so with:
keyboard navigation works in Firefox. |
It does? I'll try that again as well, maybe I just did something wrong before :D |
No, for me it still doesn't work. And |
Hmm, for me it's working. This is what i'm using to test
Compiled version: https://dl.dropboxusercontent.com/u/17360362/tmp/nokeys.html |
Okay, that one works for me too. I don't know what I did wrong when modifying the game I tested this with before, but I guess I'll be able to figure that out :) EDIT: I'm not sure why it didn't work before, but now I managed to do it with my game too. |
Code looks good, but I think we need to add a test for this in emscripten, so it doesn't break. Also we should verify that this doesn't affect current tests. |
Okay. I don't have a lot of time in the coming days (so it might take a while), but unless someone else wants to do this, I'll try to create some kind of test for this. |
Hi. Sorry I didn't answer for along. As far as I can see this pool request is preferred over my #4 ? Did anyone add test for this? |
Hey, sorry I haven't replied in quite some time. I'm not actively using emscripten anymore and don't plan to do so in the near future, so the motivation isn't there anymore. I would be happy to help anyone try to pick up on this, but I don't think it'll be worth it to get into writing a test for this myself. |
This doesn't seem to break any existing tests. I'm not sure what a test for this would look like. |
Fair enough. Merged. Thanks @jplatte! |
This broke TEXTINPUT events. I've pushed a fix for that, but it probably breaks #21. (Don't currently have a working chrome install to test) |
Can you please add a test for TEXTINPUT, so this doesn't regress again? In |
I'll try and get a test done soon. |
Robert Folland When running this little test program with SDL2 on Wayland it often crashes in SDL_Init. From a backtrace it is apparent that there is a race condition in creating a xkb_context_ref. Sometimes it is 0x0. By moving the relevant lines higher up in Wayland_VideoInit (in SDL2-2.0.4/src/video/wayland/SDL_waylandvideo.c:302) this seems to get fixed. I moved the call to WAYLAND_xkb_context_new() up to before the call to WAYLAND_wl_display_connect(). Here is the test program (just a loop of init and quit), and a backtrace from gdb: #include <cstdio> #include <stdlib.h> #include <SDL2/SDL.h> #include <unistd.h> #include <iostream> int main(int argc, char **argv) { int count = atoi(argv[1]); for (int i = 0; i < count; i++) { std::cout << "Init " << i << std::endl; if (SDL_Init(SDL_INIT_VIDEO) < 0) { SDL_LogError(SDL_LOG_CATEGORY_APPLICATION, "Couldn't initialize SDL: %s\n", SDL_GetError()); return 1; } std::cout << "Quit" << std::endl; SDL_Quit(); } return 0; } Init 12 Quit Init 13 Program received signal SIGSEGV, Segmentation fault. xkb_context_ref (ctx=ctx@entry=0x0) at src/context.c:156 156 ctx->refcnt++; (gdb) bt #0 xkb_context_ref (ctx=ctx@entry=0x0) at src/context.c:156 #1 0x00007ffff5e1cd4c in xkb_keymap_new (ctx=0x0, format=XKB_KEYMAP_FORMAT_TEXT_V1, flags=flags@entry=XKB_KEYMAP_COMPILE_NO_FLAGS) at src/keymap-priv.c:65 #2 0x00007ffff5e1c6cc in xkb_keymap_new_from_buffer (ctx=<optimized out>, buffer=0x7ffff7fd5000 "xkb_keymap {\nxkb_keycodes \"(unnamed)\" {\n\tminimum = 8;\n\tmaximum = 255;\n\t<ESC>", ' ' <repeats 16 times>, "= 9;\n\t<AE01>", ' ' <re peats 15 times>, "= 10;\n\t<AE02>", ' ' <repeats 15 times>, "= 11;\n\t<AE03>", ' ' <repeats 15 times>, "= 12;\n\t<AE04>", ' ' <repeats 12 times>..., length=48090, format=<optimized out>, flags=<optimized out>) at src/keymap.c:191 #3 0x00007ffff7b8ea4e in keyboard_handle_keymap (data=0x6169b0, keyboard=<optimized out>, format=<optimized out>, fd=5, size=48091) at /home/vlab/abs/sdl2/src/SDL2-2.0.4/src/video/wayland/SDL_waylandevents.c:269 #4 0x00007ffff64501f0 in ffi_call_unix64 () from /usr/lib/libffi.so.6 #5 0x00007ffff644fc58 in ffi_call () from /usr/lib/libffi.so.6 #6 0x00007ffff665be3e in wl_closure_invoke (closure=closure@entry=0x61f000, flags=flags@entry=1, target=<optimized out>, target@entry=0x616d20, opcode=opcode@entry=0, data=<optimized out>) at src/connection.c:949 #7 0x00007ffff6658be0 in dispatch_event (display=<optimized out>, queue=<optimized out>) at src/wayland-client.c:1274 #8 0x00007ffff6659db4 in dispatch_queue (queue=0x617398, display=0x6172d0) at src/wayland-client.c:1420 #9 wl_display_dispatch_queue_pending (display=0x6172d0, queue=0x617398) at src/wayland-client.c:1662 #10 0x00007ffff665a0cf in wl_display_roundtrip_queue (display=0x6172d0, queue=0x617398) at src/wayland-client.c:1085 #11 0x00007ffff7b8faa0 in Wayland_VideoInit (_this=<optimized out>) at /home/vlab/abs/sdl2/src/SDL2-2.0.4/src/video/wayland/SDL_waylandvideo.c:302 #12 0x00007ffff7b7aed6 in SDL_VideoInit_REAL (driver_name=<optimized out>, driver_name@entry=0x0) at /home/vlab/abs/sdl2/src/SDL2-2.0.4/src/video/SDL_video.c:513 #13 0x00007ffff7ae0ee7 in SDL_InitSubSystem_REAL (flags=16416) at /home/vlab/abs/sdl2/src/SDL2-2.0.4/src/SDL.c:173 #14 0x0000000000400b24 in main (argc=2, argv=0x7fffffffebb8) at vplay-init.cpp:13 (gdb)
Extended version of / resolves #4