From c2d0002b4f20e9fa9b23cf11facc6e4ae85ee2a5 Mon Sep 17 00:00:00 2001 From: Prakhar Pratyush Date: Wed, 7 May 2025 18:48:48 +0530 Subject: [PATCH] hotkey: Remove the usage of deprecated `keypress` event. This commit removes the deprecated `keypress` event. Now, we use the `keydown` event to handle all the cases. Earlier, we used `keypress` because it allowed us to distinguish between lowercase and uppercase characters using `event.which`. For example: * For 'r': * keypress.which = 114 * keydown.which = 82 * For 'R': * keypress.which = 82 * keydown.which = 82 As shown, `keydown.which` cannot distinguish between 'r' and 'R', which is why `keypress` was helpful. Now, modern browsers support `event.key` on `keydown`, which directly gives 'r' or 'R' as needed. This makes `keypress` unnecessary, and we can safely rely on `keydown` with `event.key` to get the exact character. An earlier commit in this PR #34570, in which we replaced the `event.which` with `event.key` plays a major role as a prep commit to help removing `keypress` event in this commit. --- web/src/hotkey.js | 51 ++++++++++++--------------------------- web/tests/hotkey.test.cjs | 46 +++++++++++++++-------------------- 2 files changed, 35 insertions(+), 62 deletions(-) diff --git a/web/src/hotkey.js b/web/src/hotkey.js index 03f5ae3f12..0070babdc0 100644 --- a/web/src/hotkey.js +++ b/web/src/hotkey.js @@ -110,7 +110,10 @@ const keydown_shift_mappings = { ArrowDown: {name: "down_arrow", message_view_only: false}, H: {name: "view_edit_history", message_view_only: true}, N: {name: "narrow_to_next_unread_followed_topic", message_view_only: false}, - V: {name: "toggle_read_receipts", message_view_only: true}, + V: [ + {name: "view_selected_stream", message_view_only: false}, + {name: "toggle_read_receipts", message_view_only: true}, + ], }; const keydown_unshift_mappings = { @@ -162,7 +165,7 @@ const keydown_either_mappings = { Delete: {name: "delete", message_view_only: false}, }; -const keypress_mappings = { +const character_mappings = { "*": {name: "open_starred_message_view", message_view_only: true}, "+": {name: "thumbs_up_emoji", message_view_only: true}, "=": {name: "upvote_first_emoji", message_view_only: true}, @@ -185,7 +188,6 @@ const keypress_mappings = { R: {name: "respond_to_author", message_view_only: true}, S: {name: "toggle_stream_subscription", message_view_only: true}, U: {name: "mark_unread", message_view_only: true}, - V: {name: "view_selected_stream", message_view_only: false}, // The shortcut "a" dates from when this was called "All messages". a: {name: "open_combined_feed", message_view_only: true}, c: {name: "compose", message_view_only: true}, @@ -256,15 +258,12 @@ export function get_keydown_hotkey(e) { } } - return keydown_either_mappings[e.key]; -} - -export function get_keypress_hotkey(e) { - if (e.metaKey || e.ctrlKey || e.altKey) { - return undefined; + hotkey = keydown_either_mappings[e.key]; + if (hotkey) { + return hotkey; } - return keypress_mappings[e.key]; + return character_mappings[e.key]; } export let processing_text = () => { @@ -681,7 +680,7 @@ export function process_shift_tab_key() { return false; } -// Process a keydown or keypress event. +// Process a keydown event. // // Returns true if we handled it, false if the browser should. export function process_hotkey(e, hotkey) { @@ -1318,31 +1317,17 @@ export function process_hotkey(e, hotkey) { return false; } -/* We register both a keydown and a keypress function because - we want to intercept PgUp/PgDn, Esc, etc, and process them - as they happen on the keyboard. However, if we processed - letters/numbers in keydown, we wouldn't know what the case of - the letters were. - - We want case-sensitive hotkeys (such as in the case of r vs R) - so we bail in .keydown if the event is a letter or number and - instead just let keypress go for it. */ - export function process_keydown(e) { activity.set_new_user_input(true); - const hotkey = get_keydown_hotkey(e); - if (!hotkey) { + const result = get_keydown_hotkey(e); + if (!result) { return false; } - return process_hotkey(e, hotkey); -} -export function process_keypress(e) { - const hotkey = get_keypress_hotkey(e); - if (!hotkey) { - return false; + if (Array.isArray(result)) { + return result.some((hotkey) => process_hotkey(e, hotkey)); } - return process_hotkey(e, hotkey); + return process_hotkey(e, result); } export function initialize() { @@ -1351,10 +1336,4 @@ export function initialize() { e.preventDefault(); } }); - - $(document).on("keypress", (e) => { - if (process_keypress(e)) { - e.preventDefault(); - } - }); } diff --git a/web/tests/hotkey.test.cjs b/web/tests/hotkey.test.cjs index 00857b5da4..b83a1c2948 100644 --- a/web/tests/hotkey.test.cjs +++ b/web/tests/hotkey.test.cjs @@ -153,13 +153,6 @@ function test_while_not_editing_text(label, f) { } run_test("mappings", () => { - function map_press(key, shiftKey) { - return hotkey.get_keypress_hotkey({ - key, - shiftKey, - }); - } - function map_down(key, shiftKey, ctrlKey, metaKey, altKey) { return hotkey.get_keydown_hotkey({ key, @@ -172,7 +165,7 @@ run_test("mappings", () => { // The next assertion protects against an iOS bug where we // treat "!" as a hotkey, because iOS sends the wrong code. - assert.equal(map_press("!"), undefined); + assert.equal(map_down("!"), undefined); // Test page-up does work. assert.equal(map_down("PageUp").name, "page_up"); @@ -187,10 +180,13 @@ run_test("mappings", () => { assert.equal(map_down("Enter", true).name, "enter"); assert.equal(map_down("H", true).name, "view_edit_history"); assert.equal(map_down("N", true).name, "narrow_to_next_unread_followed_topic"); - assert.equal(map_down("V", true).name, "toggle_read_receipts"); + assert.deepEqual( + map_down("V", true).map((item) => item.name), + ["view_selected_stream", "toggle_read_receipts"], + ); - assert.equal(map_press("/").name, "search"); - assert.equal(map_press("j").name, "vim_down"); + assert.equal(map_down("/").name, "search"); + assert.equal(map_down("j").name, "vim_down"); assert.equal(map_down("[", false, true).name, "escape"); assert.equal(map_down("c", false, true).name, "copy_with_c"); @@ -201,8 +197,6 @@ run_test("mappings", () => { assert.equal(map_down("p", false, false, false, true).name, "toggle_compose_preview"); // Alt + P // More negative tests. - assert.equal(map_down("/"), undefined); - assert.equal(map_press("Escape"), undefined); assert.equal(map_down("Escape", true), undefined); assert.equal(map_down("v", false, true), undefined); assert.equal(map_down("z", false, true), undefined); @@ -240,16 +234,13 @@ run_test("mappings", () => { navigator.platform = ""; }); -function process(s, shiftKey, keydown = false) { +function process(s, shiftKey) { const e = { key: s, shiftKey, }; try { - if (keydown) { - return hotkey.process_keydown(e); - } - return hotkey.process_keypress(e); + return hotkey.process_keydown(e); } catch (error) /* istanbul ignore next */ { // An exception will be thrown here if a different // function is called than the one declared. Try to @@ -260,9 +251,9 @@ function process(s, shiftKey, keydown = false) { } } -function assert_mapping(c, module, func_name, shiftKey, keydown) { +function assert_mapping(c, module, func_name, shiftKey) { stubbing(module, func_name, (stub) => { - assert.ok(process(c, shiftKey, keydown)); + assert.ok(process(c, shiftKey)); assert.equal(stub.num_calls, 1); }); } @@ -318,7 +309,7 @@ test_while_not_editing_text("streams", ({override}) => { override(overlays, "streams_open", () => true); override(overlays, "any_active", () => true); assert_mapping("S", stream_settings_ui, "keyboard_sub"); - assert_mapping("V", stream_settings_ui, "view_stream"); + assert_mapping("V", stream_settings_ui, "view_stream", true); assert_mapping("n", stream_settings_ui, "open_create_stream"); settings_data.user_can_create_private_streams = () => false; settings_data.user_can_create_public_streams = () => false; @@ -424,11 +415,11 @@ test_while_not_editing_text("misc", ({override}) => { override(message_edit, "can_move_message", () => false); assert_unmapped("m"); - assert_mapping("V", read_receipts, "show_user_list", true, true); + assert_mapping("V", read_receipts, "show_user_list", true); override(modals, "any_active", () => true); override(modals, "active_modal", () => "#read_receipts_modal"); - assert_mapping("V", read_receipts, "hide_user_list", true, true); + assert_mapping("V", read_receipts, "hide_user_list", true); }); test_while_not_editing_text("lightbox overlay open", ({override}) => { @@ -468,7 +459,7 @@ test_while_not_editing_text("n/p keys", () => { }); test_while_not_editing_text("narrow next unread followed topic", () => { - assert_mapping("N", message_view, "narrow_to_next_topic", true, true); + assert_mapping("N", message_view, "narrow_to_next_topic", true); }); test_while_not_editing_text("motion_keys", () => { @@ -581,7 +572,10 @@ run_test("test new user input hook called", () => { hook_called = true; }); - hotkey.process_keydown({key: "S"}); + // Currently, "b" is not a valid hotkey. + // But it serves our purpose here to verify + // `hook_called` on keydown. + hotkey.process_keydown({key: "b"}); assert.ok(hook_called); }); @@ -598,7 +592,7 @@ test_while_not_editing_text("e shortcut works for anonymous users", ({override_r }; stubbing(message_edit, "start", (stub) => { - hotkey.process_keypress(e); + hotkey.process_keydown(e); assert.equal(stub.num_calls, 1); }); assert.equal(stub.num_calls, 0, "login_to_access should not be called for 'e' shortcut");