From 2b9dc1f3988e33f47f06a70cdb5cf4200051010b Mon Sep 17 00:00:00 2001 From: Aman Agrawal Date: Sat, 21 Oct 2023 09:38:49 +0000 Subject: [PATCH] hotkey: Avoid inconsistent hotkey handling. Pressing `r` to open compose box, or search via hotkey didn't work for gear menu while it worked for other popovers. So, this is an attempt to unify that behavior so that if the hotkeys are not handles the navbar_menus popovers, then can be handled elsewhere. --- tools/test-js-with-node | 1 + web/src/hotkey.js | 34 +++++++------------------ web/src/message_actions_popover.js | 12 +++++++++ web/src/navbar_menus.js | 40 ++++++++++++++++++++++++++++++ web/src/personal_menu_popover.js | 7 ++++++ web/src/popover_menus.js | 4 +++ 6 files changed, 73 insertions(+), 25 deletions(-) create mode 100644 web/src/navbar_menus.js diff --git a/tools/test-js-with-node b/tools/test-js-with-node index ebc35a3110..77f780908e 100755 --- a/tools/test-js-with-node +++ b/tools/test-js-with-node @@ -152,6 +152,7 @@ EXEMPT_FILES = make_set( "web/src/narrow_history.js", "web/src/narrow_title.js", "web/src/navbar_alerts.js", + "web/src/navbar_menus.js", "web/src/navigate.js", "web/src/overlay_util.ts", "web/src/overlays.ts", diff --git a/web/src/hotkey.js b/web/src/hotkey.js index ac7f91e2c4..9d139a0599 100644 --- a/web/src/hotkey.js +++ b/web/src/hotkey.js @@ -34,6 +34,7 @@ import * as message_scroll_state from "./message_scroll_state"; import * as modals from "./modals"; import * as narrow from "./narrow"; import * as narrow_state from "./narrow_state"; +import * as navbar_menus from "./navbar_menus"; import * as navigate from "./navigate"; import * as overlays from "./overlays"; import {page_params} from "./page_params"; @@ -733,31 +734,6 @@ export function process_hotkey(e, hotkey) { return false; } - // We don't need to process arrow keys in navbar menus for spectators - // since they only have gear menu present. - if ( - popover_menus.is_personal_menu_popover_displayed() && - event_name === "left_arrow" && - !page_params.is_spectator - ) { - // Open gear menu popover on left arrow. - $("#personal-menu").trigger("click"); - gear_menu.toggle(); - return true; - } - - if (popover_menus.is_gear_menu_popover_displayed()) { - if (event_name === "gear_menu") { - gear_menu.toggle(); - return true; - } else if (event_name === "right_arrow" && !page_params.is_spectator) { - // Open personal menu popover on g + right arrow. - gear_menu.toggle(); - $("#personal-menu").trigger("click"); - return true; - } - } - if (overlays.settings_open() && !user_card_popover.user_card.is_open()) { return false; } @@ -792,6 +768,14 @@ export function process_hotkey(e, hotkey) { return true; } + // Handle hotkeys for active popovers here which can handle keys other than `menu_dropdown_hotkeys`. + if ( + navbar_menus.is_navbar_menus_displayed() && + navbar_menus.handle_keyboard_events(event_name) + ) { + return true; + } + // The next two sections date back to 00445c84 and are Mac/Chrome-specific, // and they should possibly be eliminated in favor of keeping standard // browser behavior. diff --git a/web/src/message_actions_popover.js b/web/src/message_actions_popover.js index e0573534d4..ccd2573771 100644 --- a/web/src/message_actions_popover.js +++ b/web/src/message_actions_popover.js @@ -13,6 +13,7 @@ import * as message_lists from "./message_lists"; import * as message_viewport from "./message_viewport"; import * as popover_menus from "./popover_menus"; import * as popover_menus_data from "./popover_menus_data"; +import * as popovers from "./popovers"; import * as read_receipts from "./read_receipts"; import * as rows from "./rows"; import * as stream_popover from "./stream_popover"; @@ -39,6 +40,11 @@ function focus_first_action_popover_item() { } export function toggle_message_actions_menu(message) { + if (popover_menus.is_message_actions_popover_displayed()) { + popovers.hide_all(); + return true; + } + if (message.locally_echoed || message_edit.is_editing(message.id)) { // Don't open the popup for locally echoed messages for now. // It creates bugs with things like keyboard handlers when @@ -49,6 +55,12 @@ export function toggle_message_actions_menu(message) { return true; } + // Since this can be called via hotkey, we need to + // hide any other popovers that may be open before. + if (popovers.any_active()) { + popovers.hide_all(); + } + message_viewport.maybe_scroll_to_show_message_top(); const $popover_reference = $(".selected_message .actions_hover .message-actions-menu-button"); message_actions_popover_keyboard_toggle = true; diff --git a/web/src/navbar_menus.js b/web/src/navbar_menus.js new file mode 100644 index 0000000000..4494b5497b --- /dev/null +++ b/web/src/navbar_menus.js @@ -0,0 +1,40 @@ +import * as gear_menu from "./gear_menu"; +import {page_params} from "./page_params"; +import * as personal_menu_popover from "./personal_menu_popover"; +import * as popover_menus from "./popover_menus"; + +export function is_navbar_menus_displayed() { + return ( + popover_menus.is_personal_menu_popover_displayed() || + popover_menus.is_gear_menu_popover_displayed() + ); +} + +export function handle_keyboard_events(event_name) { + // We don't need to process arrow keys in navbar menus for spectators + // since they only have gear menu present. + if ( + popover_menus.is_personal_menu_popover_displayed() && + (event_name === "left_arrow" || event_name === "gear_menu") && + !page_params.is_spectator + ) { + // Open gear menu popover on left arrow. + personal_menu_popover.toggle(); + gear_menu.toggle(); + return true; + } + + if (popover_menus.is_gear_menu_popover_displayed()) { + if (event_name === "gear_menu") { + gear_menu.toggle(); + return true; + } else if (event_name === "right_arrow" && !page_params.is_spectator) { + // Open personal menu popover on g + right arrow. + gear_menu.toggle(); + personal_menu_popover.toggle(); + return true; + } + } + + return false; +} diff --git a/web/src/personal_menu_popover.js b/web/src/personal_menu_popover.js index 2bf6981c9a..3fd3adcff8 100644 --- a/web/src/personal_menu_popover.js +++ b/web/src/personal_menu_popover.js @@ -107,3 +107,10 @@ export function initialize() { }, }); } + +export function toggle() { + // NOTE: Since to open personal menu, you need to click on your avatar (which calls + // tippyjs.hideAll()), or go via gear menu if using hotkeys, we don't need to + // call tippyjs.hideAll() for it. + $("#personal-menu").trigger("click"); +} diff --git a/web/src/popover_menus.js b/web/src/popover_menus.js index f4f19ede40..fc5025f3d9 100644 --- a/web/src/popover_menus.js +++ b/web/src/popover_menus.js @@ -111,6 +111,10 @@ export function get_gear_menu_instance() { return popover_instances.gear_menu; } +export function is_message_actions_popover_displayed() { + return popover_instances.message_actions?.state.isVisible; +} + function get_popover_items_for_instance(instance) { const $current_elem = $(instance.popper); const class_name = $current_elem.attr("class");