Skip to content

Commit

Permalink
Reduce usage of legacy event.keyCode. NFC
Browse files Browse the repository at this point in the history
This change replaced the usage of the legacy `keyCode` attribute
with the preferred `key` attribute, which also has the advantage of
removing some hardcoded constant numbers.

In order to test this change I fixed the `test_glfw_get_key_stuck`
test and added `test_sdl_key_test` to the interactive tests.

In doing so I noticed and fixed a crash bug that was introduced in #22874.
  • Loading branch information
sbc100 committed Nov 7, 2024
1 parent 4b3bace commit 9fd11aa
Show file tree
Hide file tree
Showing 6 changed files with 20 additions and 15 deletions.
2 changes: 1 addition & 1 deletion src/library_glfw.js
Original file line number Diff line number Diff line change
Expand Up @@ -419,7 +419,7 @@ var LibraryGLFW = {
// This logic comes directly from the sdl implementation. We cannot
// call preventDefault on all keydown events otherwise onKeyPress will
// not get called
if (event.keyCode === 8 /* backspace */ || event.keyCode === 9 /* tab */) {
if (event.key == 'Backspace' || event.key == 'Tab') {
event.preventDefault();
}
},
Expand Down
8 changes: 5 additions & 3 deletions src/library_sdl.js
Original file line number Diff line number Diff line change
Expand Up @@ -553,7 +553,7 @@ var LibrarySDL = {
receiveEvent(event) {
function unpressAllPressedKeys() {
// Un-press all pressed keys: TODO
for (var keyCode of SDL.keyboardMap) {
for (var keyCode of Object.values(SDL.keyboardMap)) {
SDL.events.push({
type: 'keyup',
keyCode,
Expand Down Expand Up @@ -696,7 +696,7 @@ var LibrarySDL = {
// won't fire. However, it's fine (and in some cases necessary) to
// preventDefault for keys that don't generate a character. Otherwise,
// preventDefault is the right thing to do in general.
if (event.type !== 'keydown' || (!SDL.unicode && !SDL.textInput) || (event.keyCode === 8 /* backspace */ || event.keyCode === 9 /* tab */)) {
if (event.type !== 'keydown' || (!SDL.unicode && !SDL.textInput) || (event.key == 'Backspace' || event.key == 'Tab')) {
event.preventDefault();
}

Expand Down Expand Up @@ -930,7 +930,9 @@ var LibrarySDL = {
switch (event.type) {
case 'keydown': case 'keyup': {
var down = event.type === 'keydown';
//dbg('Received key event: ' + event.keyCode);
#if RUNTIME_DEBUG
dbg('Received key event: ' + event.keyCode);
#endif
var key = SDL.lookupKeyCodeForEvent(event);
var scan;
if (key >= 1024) {
Expand Down
2 changes: 1 addition & 1 deletion src/proxyClient.js
Original file line number Diff line number Diff line change
Expand Up @@ -313,7 +313,7 @@ if (!ENVIRONMENT_IS_NODE) {
// Only prevent default on backspace/tab because we don't want unexpected navigation.
// Do not prevent default on the rest as we need the keypress event.
function shouldPreventDefault(event) {
if (event.type === 'keydown' && event.keyCode !== 8 /* backspace */ && event.keyCode !== 9 /* tab */) {
if (event.type === 'keydown' && event.key != 'Backspace' && event.key != 'Tab') {
return false; // keypress, back navigation
} else {
return true; // NO keypress, NO back navigation
Expand Down
8 changes: 4 additions & 4 deletions test/interactive/test_glfw_get_key_stuck.c
Original file line number Diff line number Diff line change
Expand Up @@ -98,10 +98,10 @@ int main() {
printf("%d. Press and hold spacebar\n", step);

#ifdef __EMSCRIPTEN__
emscripten_set_blur_callback(NULL, NULL, true, on_focuspocus);
emscripten_set_focus_callback(NULL, NULL, true, on_focuspocus);
emscripten_set_focusin_callback(NULL, NULL, true, on_focuspocus);
emscripten_set_focusout_callback(NULL, NULL, true, on_focuspocus);
emscripten_set_blur_callback(EMSCRIPTEN_EVENT_TARGET_WINDOW, NULL, true, on_focuspocus);
emscripten_set_focus_callback(EMSCRIPTEN_EVENT_TARGET_WINDOW, NULL, true, on_focuspocus);
emscripten_set_focusin_callback(EMSCRIPTEN_EVENT_TARGET_WINDOW, NULL, true, on_focuspocus);
emscripten_set_focusout_callback(EMSCRIPTEN_EVENT_TARGET_WINDOW, NULL, true, on_focuspocus);

emscripten_set_main_loop(render, 0, 1);
__builtin_trap();
Expand Down
12 changes: 6 additions & 6 deletions test/test_browser.py
Original file line number Diff line number Diff line change
Expand Up @@ -1013,15 +1013,15 @@ def post():
<script src='fake_events.js'></script>
<script>
// Send 'A'. The corresonding keypress event will not be prevented.
simulateKeyDown(65);
simulateKeyUp(65);
simulateKeyDown(65, 'KeyA);
simulateKeyUp(65, 'KeyA');
// Send backspace. The corresonding keypress event *will* be prevented due to proxyClient.js.
simulateKeyDown(8);
simulateKeyUp(8);
simulateKeyDown(8, 'Backspace');
simulateKeyUp(8, 'Backspace');
simulateKeyDown(100);
simulateKeyUp(100);
simulateKeyDown(100, 'Numpad4');
simulateKeyUp(100, 'Numpad4');
</script>
</body>''')

Expand Down
3 changes: 3 additions & 0 deletions test/test_interactive.py
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,9 @@ def test_sdl_touch(self):
def test_sdl_wm_togglefullscreen(self):
self.btest_exit('test_sdl_wm_togglefullscreen.c')

def test_sdl_key_test(self):
self.btest_exit('test_sdl_key_test.c')

def test_sdl_fullscreen_samecanvassize(self):
self.btest_exit('test_sdl_fullscreen_samecanvassize.c')

Expand Down

0 comments on commit 9fd11aa

Please sign in to comment.