mirror of
https://github.com/zulip/zulip.git
synced 2025-11-02 21:13:36 +00:00
hotkey: Fix Caps Lock incorrectly affecting Shift state in shortcuts.
Earlier, with Caps Lock enabled, pressing letters like 'a', 'i', etc resulted in the keyboard shortcuts for 'Shift+A', 'Shift+I', etc getting executed. Ideally, they should work only when 'Shift + Key' is pressed. We used to check capitalized letters, which can result either due to `key` pressed with Caps Lock enabled or `Shift + key` pressed. This commit fixes the bug by using the event modifiers states to construct a fully descriptive string like 'Cmd+Ctrl+Alt+Shift+A', and then look up that string in a single mapping for further execution. We treat a-z letters case-insensitively. We distinguish between 'a' and 'A' as 'A' vs 'Shift+A'. This descriptive string approach also resolves a bug where `Ctrl + Meta + C/K/S/.` was working. We define the valid hotkey as `Meta + C/K/S/.` for macOS and `Ctrl + C/K/S/.` for others.
This commit is contained in:
Prakhar Pratyush
committed by
Tim Abbott
Tim Abbott
parent
c2d0002b4f
commit
f506ccc5f4
@@ -89,6 +89,23 @@ const color_picker_hotkeys = new Set([
|
||||
"enter",
|
||||
]);
|
||||
|
||||
// A set of characters that, on many keyboard layouts, require 'Shift' modifier key
|
||||
// to type, or where pressing the key with a 'Shift' modifier is expected to produce
|
||||
// a different character. We ignore the 'Shift' modifier when processing these keys,
|
||||
// and disallow assigning hotkeys that involve 'Shift' modifier combined with them.
|
||||
const KNOWN_IGNORE_SHIFT_MODIFIER_KEYS = new Set([
|
||||
"*",
|
||||
"+",
|
||||
"=",
|
||||
"-",
|
||||
"/",
|
||||
":",
|
||||
"<",
|
||||
">",
|
||||
"?",
|
||||
"@",
|
||||
]);
|
||||
|
||||
// Note that multiple keys can map to the same event_name, which
|
||||
// we'll do in cases where they have the exact same semantics.
|
||||
// DON'T FORGET: update keyboard_shortcuts.html
|
||||
@@ -99,25 +116,68 @@ const color_picker_hotkeys = new Set([
|
||||
// in the main message view with a selected message.
|
||||
// `message_view_only` hotkeys, as a group, are not processed if any
|
||||
// overlays are open (e.g. settings, streams, etc.).
|
||||
|
||||
const keydown_shift_mappings = {
|
||||
// these can be triggered by Shift + key only
|
||||
Tab: {name: "shift_tab", message_view_only: false},
|
||||
" ": {name: "shift_spacebar", message_view_only: true},
|
||||
ArrowLeft: {name: "left_arrow", message_view_only: false},
|
||||
ArrowRight: {name: "right_arrow", message_view_only: false},
|
||||
ArrowUp: {name: "up_arrow", message_view_only: false},
|
||||
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: [
|
||||
const KEYDOWN_MAPPINGS = {
|
||||
"Alt+P": {name: "toggle_compose_preview", message_view_only: true},
|
||||
"Ctrl+[": {name: "escape", message_view_only: false},
|
||||
"Cmd+Enter": {name: "action_with_enter", message_view_only: true},
|
||||
"Cmd+C": {name: "copy_with_c", message_view_only: false},
|
||||
"Cmd+K": {name: "search_with_k", message_view_only: false},
|
||||
"Cmd+S": {name: "star_message", message_view_only: true},
|
||||
"Cmd+.": {name: "narrow_to_compose_target", message_view_only: true},
|
||||
"Cmd+'": {name: "open_saved_snippet_dropdown", message_view_only: true},
|
||||
"Ctrl+Enter": {name: "action_with_enter", message_view_only: true},
|
||||
"Ctrl+C": {name: "copy_with_c", message_view_only: false},
|
||||
"Ctrl+K": {name: "search_with_k", message_view_only: false},
|
||||
"Ctrl+S": {name: "star_message", message_view_only: true},
|
||||
"Ctrl+.": {name: "narrow_to_compose_target", message_view_only: true},
|
||||
"Ctrl+'": {name: "open_saved_snippet_dropdown", message_view_only: true},
|
||||
"Shift+A": {name: "stream_cycle_backward", message_view_only: true},
|
||||
"Shift+C": {name: "C_deprecated", message_view_only: true},
|
||||
"Shift+D": {name: "stream_cycle_forward", message_view_only: true},
|
||||
"Shift+G": {name: "G_end", message_view_only: true},
|
||||
"Shift+H": {name: "view_edit_history", message_view_only: true},
|
||||
"Shift+I": {name: "open_inbox", message_view_only: true},
|
||||
"Shift+J": {name: "vim_page_down", message_view_only: true},
|
||||
"Shift+K": {name: "vim_page_up", message_view_only: true},
|
||||
"Shift+M": {name: "toggle_topic_visibility_policy", message_view_only: true},
|
||||
"Shift+N": {name: "narrow_to_next_unread_followed_topic", message_view_only: false},
|
||||
"Shift+P": {name: "narrow_private", message_view_only: true},
|
||||
"Shift+R": {name: "respond_to_author", message_view_only: true},
|
||||
"Shift+S": {name: "toggle_stream_subscription", message_view_only: true},
|
||||
"Shift+U": {name: "mark_unread", message_view_only: true},
|
||||
"Shift+V": [
|
||||
{name: "view_selected_stream", message_view_only: false},
|
||||
{name: "toggle_read_receipts", message_view_only: true},
|
||||
],
|
||||
};
|
||||
|
||||
const keydown_unshift_mappings = {
|
||||
// these can be triggered by key only (without Shift)
|
||||
"Shift+Tab": {name: "shift_tab", message_view_only: false},
|
||||
"Shift+ ": {name: "shift_spacebar", message_view_only: true},
|
||||
"Shift+ArrowLeft": {name: "left_arrow", message_view_only: false},
|
||||
"Shift+ArrowRight": {name: "right_arrow", message_view_only: false},
|
||||
"Shift+ArrowUp": {name: "up_arrow", message_view_only: false},
|
||||
"Shift+ArrowDown": {name: "down_arrow", 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},
|
||||
D: {name: "open_drafts", message_view_only: true},
|
||||
E: {name: "edit_message", message_view_only: true},
|
||||
G: {name: "gear_menu", message_view_only: true},
|
||||
H: {name: "vim_left", message_view_only: true},
|
||||
I: {name: "message_actions", message_view_only: true},
|
||||
J: {name: "vim_down", message_view_only: true},
|
||||
K: {name: "vim_up", message_view_only: true},
|
||||
L: {name: "vim_right", message_view_only: true},
|
||||
M: {name: "move_message", message_view_only: true},
|
||||
N: {name: "n_key", message_view_only: false},
|
||||
P: {name: "p_key", message_view_only: false},
|
||||
Q: {name: "query_streams", message_view_only: true},
|
||||
R: {name: "reply_message", message_view_only: true},
|
||||
S: {name: "toggle_conversation_view", message_view_only: true},
|
||||
T: {name: "open_recent_view", message_view_only: true},
|
||||
U: {name: "toggle_sender_info", message_view_only: true},
|
||||
V: {name: "show_lightbox", message_view_only: true},
|
||||
W: {name: "query_users", message_view_only: true},
|
||||
X: {name: "compose_private_message", message_view_only: true},
|
||||
Z: {name: "zoom_to_message_near", message_view_only: true},
|
||||
Tab: {name: "tab", message_view_only: false},
|
||||
Escape: {name: "escape", message_view_only: false},
|
||||
" ": {name: "spacebar", message_view_only: true},
|
||||
@@ -129,43 +189,6 @@ const keydown_unshift_mappings = {
|
||||
ArrowRight: {name: "right_arrow", message_view_only: false},
|
||||
ArrowUp: {name: "up_arrow", message_view_only: false},
|
||||
ArrowDown: {name: "down_arrow", message_view_only: false},
|
||||
};
|
||||
|
||||
const keydown_ctrl_mappings = {
|
||||
"[": {name: "escape", message_view_only: false},
|
||||
};
|
||||
|
||||
const keydown_cmd_or_ctrl_mappings = {
|
||||
Enter: {name: "action_with_enter", message_view_only: true},
|
||||
c: {name: "copy_with_c", message_view_only: false},
|
||||
k: {name: "search_with_k", message_view_only: false},
|
||||
s: {name: "star_message", message_view_only: true},
|
||||
".": {name: "narrow_to_compose_target", message_view_only: true},
|
||||
"'": {name: "open_saved_snippet_dropdown", message_view_only: true},
|
||||
};
|
||||
|
||||
const keydown_alt_mappings = {
|
||||
p: {name: "toggle_compose_preview", message_view_only: true},
|
||||
};
|
||||
|
||||
const keydown_either_mappings = {
|
||||
// these can be triggered by key or Shift + key
|
||||
// Note that codes for letters are still case sensitive!
|
||||
//
|
||||
// We may want to revisit both of these. For Backspace, we don't
|
||||
// have any specific mapping behavior; we are just trying to disable
|
||||
// the normal browser features for certain OSes when we are in the
|
||||
// compose box, and the little bit of Backspace-related code here is
|
||||
// dubious, but may apply to Shift-Backspace.
|
||||
// For Enter, there is some possibly that Shift-Enter is intended to
|
||||
// have special behavior for folks that are used to Shift-Enter behavior
|
||||
// in other apps, but that's also slightly dubious.
|
||||
Backspace: {name: "backspace", message_view_only: true},
|
||||
Enter: {name: "enter", message_view_only: false},
|
||||
Delete: {name: "delete", message_view_only: false},
|
||||
};
|
||||
|
||||
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},
|
||||
@@ -176,94 +199,55 @@ const character_mappings = {
|
||||
">": {name: "compose_quote_message", message_view_only: true},
|
||||
"?": {name: "show_shortcuts", message_view_only: false},
|
||||
"@": {name: "compose_reply_with_mention", message_view_only: true},
|
||||
A: {name: "stream_cycle_backward", message_view_only: true},
|
||||
C: {name: "C_deprecated", message_view_only: true},
|
||||
D: {name: "stream_cycle_forward", message_view_only: true},
|
||||
G: {name: "G_end", message_view_only: true},
|
||||
I: {name: "open_inbox", message_view_only: true},
|
||||
J: {name: "vim_page_down", message_view_only: true},
|
||||
K: {name: "vim_page_up", message_view_only: true},
|
||||
M: {name: "toggle_topic_visibility_policy", message_view_only: true},
|
||||
P: {name: "narrow_private", message_view_only: true},
|
||||
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},
|
||||
// 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},
|
||||
d: {name: "open_drafts", message_view_only: true},
|
||||
e: {name: "edit_message", message_view_only: true},
|
||||
g: {name: "gear_menu", message_view_only: true},
|
||||
h: {name: "vim_left", message_view_only: true},
|
||||
i: {name: "message_actions", message_view_only: true},
|
||||
j: {name: "vim_down", message_view_only: true},
|
||||
k: {name: "vim_up", message_view_only: true},
|
||||
l: {name: "vim_right", message_view_only: true},
|
||||
m: {name: "move_message", message_view_only: true},
|
||||
n: {name: "n_key", message_view_only: false},
|
||||
p: {name: "p_key", message_view_only: false},
|
||||
q: {name: "query_streams", message_view_only: true},
|
||||
r: {name: "reply_message", message_view_only: true},
|
||||
s: {name: "toggle_conversation_view", message_view_only: true},
|
||||
t: {name: "open_recent_view", message_view_only: true},
|
||||
u: {name: "toggle_sender_info", message_view_only: true},
|
||||
v: {name: "show_lightbox", message_view_only: true},
|
||||
w: {name: "query_users", message_view_only: true},
|
||||
x: {name: "compose_private_message", message_view_only: true},
|
||||
z: {name: "zoom_to_message_near", message_view_only: true},
|
||||
// these can be triggered by key or Shift + key
|
||||
//
|
||||
// We may want to revisit both of these. For Backspace, we don't
|
||||
// have any specific mapping behavior; we are just trying to disable
|
||||
// the normal browser features for certain OSes when we are in the
|
||||
// compose box, and the little bit of Backspace-related code here is
|
||||
// dubious, but may apply to Shift-Backspace.
|
||||
// For Enter, there is some possibly that Shift-Enter is intended to
|
||||
// have special behavior for folks that are used to Shift-Enter behavior
|
||||
// in other apps, but that's also slightly dubious.
|
||||
Backspace: {name: "backspace", message_view_only: true},
|
||||
Enter: {name: "enter", message_view_only: false},
|
||||
Delete: {name: "delete", message_view_only: false},
|
||||
"Shift+Backspace": {name: "backspace", message_view_only: true},
|
||||
"Shift+Enter": {name: "enter", message_view_only: false},
|
||||
"Shift+Delete": {name: "delete", message_view_only: false},
|
||||
};
|
||||
|
||||
// Keyboard Event Viewer tool:
|
||||
// https://w3c.github.io/uievents/tools/key-event-viewer.html
|
||||
export function get_keydown_hotkey(e) {
|
||||
let hotkey;
|
||||
if (common.has_mac_keyboard() && e.ctrlKey && e.key !== "[") {
|
||||
// On macOS, Cmd is used instead of Ctrl. Except 'Ctrl + ['.
|
||||
return undefined;
|
||||
}
|
||||
if (!common.has_mac_keyboard() && e.metaKey) {
|
||||
return undefined;
|
||||
}
|
||||
|
||||
// Make lowercase a-z uppercase; leave others unchanged.
|
||||
// We distinguish between 'a' and 'A' as 'A' vs 'Shift+A'.
|
||||
// This helps to avoid Caps Lock affecting shortcuts.
|
||||
let key = /^[a-z]$/.test(e.key) ? e.key.toUpperCase() : e.key;
|
||||
|
||||
if (e.shiftKey && !KNOWN_IGNORE_SHIFT_MODIFIER_KEYS.has(key)) {
|
||||
key = "Shift+" + key;
|
||||
}
|
||||
if (e.altKey) {
|
||||
hotkey = keydown_alt_mappings[e.key];
|
||||
if (hotkey) {
|
||||
return hotkey;
|
||||
}
|
||||
return undefined;
|
||||
key = "Alt+" + key;
|
||||
}
|
||||
if (e.ctrlKey) {
|
||||
key = "Ctrl+" + key;
|
||||
}
|
||||
if (e.metaKey) {
|
||||
assert(common.has_mac_keyboard());
|
||||
key = "Cmd+" + key;
|
||||
}
|
||||
|
||||
if (e.ctrlKey && !e.shiftKey) {
|
||||
hotkey = keydown_ctrl_mappings[e.key];
|
||||
if (hotkey) {
|
||||
return hotkey;
|
||||
}
|
||||
}
|
||||
|
||||
const isCmdOrCtrl = common.has_mac_keyboard() ? e.metaKey : e.ctrlKey;
|
||||
if (isCmdOrCtrl && !e.shiftKey) {
|
||||
hotkey = keydown_cmd_or_ctrl_mappings[e.key];
|
||||
if (hotkey) {
|
||||
return hotkey;
|
||||
}
|
||||
return undefined;
|
||||
} else if (e.metaKey || e.ctrlKey) {
|
||||
return undefined;
|
||||
}
|
||||
|
||||
if (e.shiftKey) {
|
||||
hotkey = keydown_shift_mappings[e.key];
|
||||
if (hotkey) {
|
||||
return hotkey;
|
||||
}
|
||||
}
|
||||
|
||||
if (!e.shiftKey) {
|
||||
hotkey = keydown_unshift_mappings[e.key];
|
||||
if (hotkey) {
|
||||
return hotkey;
|
||||
}
|
||||
}
|
||||
|
||||
hotkey = keydown_either_mappings[e.key];
|
||||
if (hotkey) {
|
||||
return hotkey;
|
||||
}
|
||||
|
||||
return character_mappings[e.key];
|
||||
return KEYDOWN_MAPPINGS[key];
|
||||
}
|
||||
|
||||
export let processing_text = () => {
|
||||
|
||||
@@ -195,6 +195,8 @@ run_test("mappings", () => {
|
||||
assert.equal(map_down(".", false, true).name, "narrow_to_compose_target");
|
||||
|
||||
assert.equal(map_down("p", false, false, false, true).name, "toggle_compose_preview"); // Alt + P
|
||||
assert.equal(map_down("+", false).name, "thumbs_up_emoji");
|
||||
assert.equal(map_down("+", true).name, "thumbs_up_emoji");
|
||||
|
||||
// More negative tests.
|
||||
assert.equal(map_down("Escape", true), undefined);
|
||||
@@ -217,12 +219,14 @@ run_test("mappings", () => {
|
||||
assert.equal(map_down("S", true, true), undefined);
|
||||
assert.equal(map_down("[", true, true, false), undefined);
|
||||
assert.equal(map_down("P", true, false, false, true), undefined);
|
||||
assert.equal(map_down("+", false, true), undefined);
|
||||
|
||||
// Cmd tests for MacOS
|
||||
navigator.platform = "MacIntel";
|
||||
assert.equal(map_down("[", false, true, false).name, "escape");
|
||||
assert.equal(map_down("[", false, false, true), undefined);
|
||||
assert.equal(map_down("c", false, true, true).name, "copy_with_c");
|
||||
assert.equal(map_down("c", false, false, true).name, "copy_with_c");
|
||||
assert.equal(map_down("c", false, true, true), undefined);
|
||||
assert.equal(map_down("c", false, true, false), undefined);
|
||||
assert.equal(map_down("k", false, false, true).name, "search_with_k");
|
||||
assert.equal(map_down("k", false, true, false), undefined);
|
||||
@@ -232,6 +236,12 @@ run_test("mappings", () => {
|
||||
assert.equal(map_down(".", false, true, false), undefined);
|
||||
// Reset platform
|
||||
navigator.platform = "";
|
||||
|
||||
// Caps Lock doesn't interfere with shortcuts.
|
||||
assert.equal(map_down("A").name, "open_combined_feed");
|
||||
assert.equal(map_down("A", true).name, "stream_cycle_backward");
|
||||
assert.equal(map_down("C", false, true).name, "copy_with_c");
|
||||
assert.equal(map_down("P", false, false, false, true).name, "toggle_compose_preview");
|
||||
});
|
||||
|
||||
function process(s, shiftKey) {
|
||||
@@ -260,7 +270,8 @@ function assert_mapping(c, module, func_name, shiftKey) {
|
||||
|
||||
function assert_unmapped(s) {
|
||||
for (const c of s) {
|
||||
assert.equal(process(c), false);
|
||||
const shiftKey = /^[A-Z]$/.test(c);
|
||||
assert.equal(process(c, shiftKey), false);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -308,7 +319,7 @@ test_while_not_editing_text("streams", ({override}) => {
|
||||
delete settings_data.user_can_create_web_public_streams;
|
||||
override(overlays, "streams_open", () => true);
|
||||
override(overlays, "any_active", () => true);
|
||||
assert_mapping("S", stream_settings_ui, "keyboard_sub");
|
||||
assert_mapping("S", stream_settings_ui, "keyboard_sub", true);
|
||||
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;
|
||||
@@ -323,12 +334,12 @@ test_while_not_editing_text("basic mappings", () => {
|
||||
assert_mapping("w", activity_ui, "initiate_search");
|
||||
assert_mapping("q", stream_list, "initiate_search");
|
||||
|
||||
assert_mapping("A", message_view, "stream_cycle_backward");
|
||||
assert_mapping("D", message_view, "stream_cycle_forward");
|
||||
assert_mapping("A", message_view, "stream_cycle_backward", true);
|
||||
assert_mapping("D", message_view, "stream_cycle_forward", true);
|
||||
|
||||
assert_mapping("c", compose_actions, "start");
|
||||
assert_mapping("x", compose_actions, "start");
|
||||
assert_mapping("P", message_view, "show");
|
||||
assert_mapping("P", message_view, "show", true);
|
||||
assert_mapping("g", gear_menu, "toggle");
|
||||
});
|
||||
|
||||
@@ -381,9 +392,9 @@ test_while_not_editing_text("misc", ({override}) => {
|
||||
assert_mapping("r", compose_reply, "respond_to_message");
|
||||
assert_mapping("R", compose_reply, "respond_to_message", true);
|
||||
assert_mapping("j", navigate, "down");
|
||||
assert_mapping("J", navigate, "page_down");
|
||||
assert_mapping("J", navigate, "page_down", true);
|
||||
assert_mapping("k", navigate, "up");
|
||||
assert_mapping("K", navigate, "page_up");
|
||||
assert_mapping("K", navigate, "page_up", true);
|
||||
assert_mapping("u", popovers, "toggle_sender_info");
|
||||
assert_mapping("i", message_actions_popover, "toggle_message_actions_menu");
|
||||
assert_mapping(":", emoji_picker, "toggle_emoji_popover", true);
|
||||
@@ -446,8 +457,8 @@ run_test("emoji picker", ({override}) => {
|
||||
|
||||
test_while_not_editing_text("G/M keys", () => {
|
||||
// TODO: move
|
||||
assert_mapping("G", navigate, "to_end");
|
||||
assert_mapping("M", user_topics_ui, "toggle_topic_visibility_policy");
|
||||
assert_mapping("G", navigate, "to_end", true);
|
||||
assert_mapping("M", user_topics_ui, "toggle_topic_visibility_policy", true);
|
||||
});
|
||||
|
||||
test_while_not_editing_text("n/p keys", () => {
|
||||
|
||||
Reference in New Issue
Block a user