Skip to content

Reduce usage of legacy event.keyCode. NFC #22881

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Reduce usage of legacy event.keyCode. NFC #22881

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/library_glfw.js
View file
Open in desktop
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
6 changes: 4 additions & 2 deletions src/library_sdl.js
View file
Open in desktop
Original file line number Diff line number Diff line change
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
View file
Open in desktop
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') {
Copy link
Collaborator Author

@sbc100 sbc100 Nov 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW the logic here seems to be the inverse of what the comment says. Shouldn't all the clauses above be ===?

Copy link
Member

@kripken kripken Nov 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks right to me? Comment above says "prevent default on backspace/tab", the if checks "not backspace, not tab" and returns false if so. So it returns true - prevents default - on backspace/tab as described.

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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
Loading