From 93a9af7b3a2dc51d0c22320e20058ed5369d880f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=A0imon=20Brandner?= Date: Wed, 23 Feb 2022 10:12:04 +0100 Subject: [PATCH] Keybinding code unification #3 (#7850) --- src/BasePlatform.ts | 6 + src/KeyBindingsDefaults.ts | 7 +- src/KeyBindingsManager.ts | 4 + src/accessibility/KeyboardShortcuts.ts | 147 ++++++++++++++---- src/components/structures/LoggedInView.tsx | 32 +++- src/components/views/voip/CallView.tsx | 31 ++-- src/i18n/strings/en_EN.json | 8 +- test/accessibility/KeyboardShortcuts-test.ts | 57 ++++--- .../views/rooms/SendMessageComposer-test.tsx | 3 + 9 files changed, 214 insertions(+), 81 deletions(-) diff --git a/src/BasePlatform.ts b/src/BasePlatform.ts index 54fc6081c9..40ac9d1199 100644 --- a/src/BasePlatform.ts +++ b/src/BasePlatform.ts @@ -309,6 +309,12 @@ export default abstract class BasePlatform { return false; } + public overrideBrowserShortcuts(): boolean { + return false; + } + + public navigateForwardBack(back: boolean): void {} + getAvailableSpellCheckLanguages(): Promise | null { return null; } diff --git a/src/KeyBindingsDefaults.ts b/src/KeyBindingsDefaults.ts index e122b2dff8..65b198d69e 100644 --- a/src/KeyBindingsDefaults.ts +++ b/src/KeyBindingsDefaults.ts @@ -152,7 +152,7 @@ const navigationBindings = (): KeyBinding[] => { const bindings = getBindingsByCategory(CategoryName.NAVIGATION); bindings.push({ - action: "KeyBinding.closeDialogOrContextMenu" as KeyBindingAction, + action: KeyBindingAction.CloseDialogOrContextMenu, keyCombo: { key: Key.ESCAPE, }, @@ -161,6 +161,10 @@ const navigationBindings = (): KeyBinding[] => { return bindings; }; +const callBindings = (): KeyBinding[] => { + return getBindingsByCategory(CategoryName.CALLS); +}; + const labsBindings = (): KeyBinding[] => { if (!SdkConfig.get()['showLabsSettings']) return []; @@ -173,5 +177,6 @@ export const defaultBindingsProvider: IKeyBindingsProvider = { getRoomListBindings: roomListBindings, getRoomBindings: roomBindings, getNavigationBindings: navigationBindings, + getCallBindings: callBindings, getLabsBindings: labsBindings, }; diff --git a/src/KeyBindingsManager.ts b/src/KeyBindingsManager.ts index 995ca3bf32..5f14fbaef1 100644 --- a/src/KeyBindingsManager.ts +++ b/src/KeyBindingsManager.ts @@ -155,6 +155,10 @@ export class KeyBindingsManager { return this.getAction(this.bindingsProviders.map(it => it.getNavigationBindings), ev); } + getCallAction(ev: KeyboardEvent | React.KeyboardEvent): KeyBindingAction | undefined { + return this.getAction(this.bindingsProviders.map(it => it.getCallBindings), ev); + } + getLabsAction(ev: KeyboardEvent | React.KeyboardEvent): KeyBindingAction | undefined { return this.getAction(this.bindingsProviders.map(it => it.getLabsBindings), ev); } diff --git a/src/accessibility/KeyboardShortcuts.ts b/src/accessibility/KeyboardShortcuts.ts index 3d798f47b7..7db10078d9 100644 --- a/src/accessibility/KeyboardShortcuts.ts +++ b/src/accessibility/KeyboardShortcuts.ts @@ -20,6 +20,7 @@ import { isMac, Key } from "../Keyboard"; import { IBaseSetting } from "../settings/Settings"; import SettingsStore from "../settings/SettingsStore"; import IncompatibleController from "../settings/controllers/IncompatibleController"; +import PlatformPeg from "../PlatformPeg"; export enum KeyBindingAction { /** Send a message */ @@ -115,6 +116,25 @@ export enum KeyBindingAction { /** Select next room with unread messages */ SelectNextUnreadRoom = 'KeyBinding.nextUnreadRoom', + /** Switches to a space by number */ + SwitchToSpaceByNumber = "KeyBinding.switchToSpaceByNumber", + /** Opens user settings */ + OpenUserSettings = "KeyBinding.openUserSettings", + /** Navigates backward */ + PreviousVisitedRoomOrCommunity = "KeyBinding.previousVisitedRoomOrCommunity", + /** Navigates forward */ + NextVisitedRoomOrCommunity = "KeyBinding.nextVisitedRoomOrCommunity", + + /** Toggles microphone while on a call */ + ToggleMicInCall = "KeyBinding.toggleMicInCall", + /** Toggles webcam while on a call */ + ToggleWebcamInCall = "KeyBinding.toggleWebcamInCall", + + /** Closes a dialog or a context menu */ + CloseDialogOrContextMenu = "KeyBinding.closeDialogOrContextMenu", + /** Clicks the selected button */ + ActivateSelectedButton = "KeyBinding.activateSelectedButton", + /** Toggle visibility of hidden events */ ToggleHiddenEventVisibility = 'KeyBinding.toggleHiddenEventVisibility', } @@ -132,13 +152,13 @@ type KeyboardShortcutSetting = IBaseSetting; type IKeyboardShortcuts = { // TODO: We should figure out what to do with the keyboard shortcuts that are not handled by KeybindingManager - [k in (KeyBindingAction | string)]: KeyboardShortcutSetting; + [k in (KeyBindingAction)]?: KeyboardShortcutSetting; }; export interface ICategory { categoryLabel: string; // TODO: We should figure out what to do with the keyboard shortcuts that are not handled by KeybindingManager - settingNames: (KeyBindingAction | string)[]; + settingNames: (KeyBindingAction)[]; } export enum CategoryName { @@ -200,8 +220,8 @@ export const CATEGORIES: Record = { }, [CategoryName.CALLS]: { categoryLabel: _td("Calls"), settingNames: [ - "KeyBinding.toggleMicInCall", - "KeyBinding.toggleWebcamInCall", + KeyBindingAction.ToggleMicInCall, + KeyBindingAction.ToggleWebcamInCall, ], }, [CategoryName.ROOM]: { categoryLabel: _td("Room"), @@ -229,8 +249,8 @@ export const CATEGORIES: Record = { categoryLabel: _td("Navigation"), settingNames: [ KeyBindingAction.ToggleUserMenu, - "KeyBinding.closeDialogOrContextMenu", - "KeyBinding.activateSelectedButton", + KeyBindingAction.CloseDialogOrContextMenu, + KeyBindingAction.ActivateSelectedButton, KeyBindingAction.ToggleRoomSidePanel, KeyBindingAction.ToggleSpacePanel, KeyBindingAction.ShowKeyboardSettings, @@ -240,6 +260,10 @@ export const CATEGORIES: Record = { KeyBindingAction.SelectPrevUnreadRoom, KeyBindingAction.SelectNextRoom, KeyBindingAction.SelectPrevRoom, + KeyBindingAction.OpenUserSettings, + KeyBindingAction.SwitchToSpaceByNumber, + KeyBindingAction.PreviousVisitedRoomOrCommunity, + KeyBindingAction.NextVisitedRoomOrCommunity, ], }, [CategoryName.AUTOCOMPLETE]: { categoryLabel: _td("Autocomplete"), @@ -258,6 +282,17 @@ export const CATEGORIES: Record = { }, }; +const DESKTOP_SHORTCUTS = [ + KeyBindingAction.OpenUserSettings, + KeyBindingAction.SwitchToSpaceByNumber, + KeyBindingAction.PreviousVisitedRoomOrCommunity, + KeyBindingAction.NextVisitedRoomOrCommunity, +]; + +const MAC_ONLY_SHORTCUTS = [ + KeyBindingAction.OpenUserSettings, +]; + // This is very intentionally modelled after SETTINGS as it will make it easier // to implement customizable keyboard shortcuts // TODO: TravisR will fix this nightmare when the new version of the SettingsStore becomes a thing @@ -332,14 +367,14 @@ export const KEYBOARD_SHORTCUTS: IKeyboardShortcuts = { }, displayName: _td("Navigate to previous message in composer history"), }, - "KeyBinding.toggleMicInCall": { + [KeyBindingAction.ToggleMicInCall]: { default: { ctrlOrCmdKey: true, key: Key.D, }, displayName: _td("Toggle microphone mute"), }, - "KeyBinding.toggleWebcamInCall": { + [KeyBindingAction.ToggleWebcamInCall]: { default: { ctrlOrCmdKey: true, key: Key.E, @@ -538,13 +573,51 @@ export const KEYBOARD_SHORTCUTS: IKeyboardShortcuts = { }, displayName: _td("Undo edit"), }, + [KeyBindingAction.EditRedo]: { + default: { + key: isMac ? Key.Z : Key.Y, + ctrlOrCmdKey: true, + shiftKey: isMac, + }, + displayName: _td("Redo edit"), + }, + [KeyBindingAction.PreviousVisitedRoomOrCommunity]: { + default: { + metaKey: isMac, + altKey: !isMac, + key: isMac ? Key.SQUARE_BRACKET_LEFT : Key.ARROW_LEFT, + }, + displayName: _td("Previous recently visited room or community"), + }, + [KeyBindingAction.NextVisitedRoomOrCommunity]: { + default: { + metaKey: isMac, + altKey: !isMac, + key: isMac ? Key.SQUARE_BRACKET_RIGHT : Key.ARROW_RIGHT, + }, + displayName: _td("Next recently visited room or community"), + }, + [KeyBindingAction.SwitchToSpaceByNumber]: { + default: { + ctrlOrCmdKey: true, + key: DIGITS, + }, + displayName: _td("Switch to space by number"), + }, + [KeyBindingAction.OpenUserSettings]: { + default: { + metaKey: true, + key: Key.COMMA, + }, + displayName: _td("Open user settings"), + }, }; // XXX: These have to be manually mirrored in KeyBindingDefaults const getNonCustomizableShortcuts = (): IKeyboardShortcuts => { const ctrlEnterToSend = SettingsStore.getValue('MessageComposerInput.ctrlEnterToSend'); - return { + const keyboardShortcuts: IKeyboardShortcuts = { [KeyBindingAction.SendMessage]: { default: { key: Key.ENTER, @@ -578,39 +651,46 @@ const getNonCustomizableShortcuts = (): IKeyboardShortcuts => { }, displayName: _td("Search (must be enabled)"), }, - "KeyBinding.closeDialogOrContextMenu": { + [KeyBindingAction.CloseDialogOrContextMenu]: { default: { key: Key.ESCAPE, }, displayName: _td("Close dialog or context menu"), }, - "KeyBinding.activateSelectedButton": { + [KeyBindingAction.ActivateSelectedButton]: { default: { key: Key.ENTER, }, displayName: _td("Activate selected button"), }, }; + + if (PlatformPeg.get().overrideBrowserShortcuts()) { + keyboardShortcuts[KeyBindingAction.SwitchToSpaceByNumber] = { + default: { + ctrlOrCmdKey: true, + key: DIGITS, + }, + displayName: _td("Switch to space by number"), + }; + } + + return keyboardShortcuts; }; export const getCustomizableShortcuts = (): IKeyboardShortcuts => { - const keyboardShortcuts = Object.assign({}, KEYBOARD_SHORTCUTS); + const overrideBrowserShortcuts = PlatformPeg.get().overrideBrowserShortcuts(); - keyboardShortcuts[KeyBindingAction.EditRedo] = { - default: { - key: isMac ? Key.Z : Key.Y, - ctrlOrCmdKey: true, - shiftKey: isMac, - }, - displayName: _td("Redo edit"), - }; + return Object.keys(KEYBOARD_SHORTCUTS).filter((k: KeyBindingAction) => { + if (KEYBOARD_SHORTCUTS[k]?.controller?.settingDisabled) return false; + if (MAC_ONLY_SHORTCUTS.includes(k) && !isMac) return false; + if (DESKTOP_SHORTCUTS.includes(k) && !overrideBrowserShortcuts) return false; - return Object.keys(keyboardShortcuts).filter(k => { - return !keyboardShortcuts[k].controller?.settingDisabled; + return true; }).reduce((o, key) => { - o[key] = keyboardShortcuts[key]; + o[key] = KEYBOARD_SHORTCUTS[key]; return o; - }, {}); + }, {} as IKeyboardShortcuts); }; export const getKeyboardShortcuts = (): IKeyboardShortcuts => { @@ -622,14 +702,15 @@ export const getKeyboardShortcuts = (): IKeyboardShortcuts => { return entries.reduce((acc, [key, value]) => { acc[key] = value; return acc; - }, {}); + }, {} as IKeyboardShortcuts); }; -export const registerShortcut = ( - shortcutName: string, - categoryName: CategoryName, - shortcut: KeyboardShortcutSetting, -): void => { - KEYBOARD_SHORTCUTS[shortcutName] = shortcut; - CATEGORIES[categoryName].settingNames.push(shortcutName); -}; +// For tests +export function mock({ keyboardShortcuts, macOnlyShortcuts, desktopShortcuts }): void { + Object.keys(KEYBOARD_SHORTCUTS).forEach((k) => delete KEYBOARD_SHORTCUTS[k]); + if (keyboardShortcuts) Object.assign(KEYBOARD_SHORTCUTS, keyboardShortcuts); + MAC_ONLY_SHORTCUTS.splice(0, MAC_ONLY_SHORTCUTS.length); + if (macOnlyShortcuts) macOnlyShortcuts.forEach((e) => MAC_ONLY_SHORTCUTS.push(e)); + DESKTOP_SHORTCUTS.splice(0, DESKTOP_SHORTCUTS.length); + if (desktopShortcuts) desktopShortcuts.forEach((e) => DESKTOP_SHORTCUTS.push(e)); +} diff --git a/src/components/structures/LoggedInView.tsx b/src/components/structures/LoggedInView.tsx index 46dee05b21..b8e3c80095 100644 --- a/src/components/structures/LoggedInView.tsx +++ b/src/components/structures/LoggedInView.tsx @@ -23,7 +23,7 @@ import { ISyncStateData, SyncState } from 'matrix-js-sdk/src/sync'; import { IUsageLimit } from 'matrix-js-sdk/src/@types/partials'; import { RoomStateEvent } from "matrix-js-sdk/src/models/room-state"; -import { Key } from '../../Keyboard'; +import { isOnlyCtrlOrCmdKeyEvent, Key } from '../../Keyboard'; import PageTypes from '../../PageTypes'; import MediaDeviceHandler from '../../MediaDeviceHandler'; import { fixupColorFonts } from '../../utils/FontManager'; @@ -73,6 +73,7 @@ import { OpenToTabPayload } from "../../dispatcher/payloads/OpenToTabPayload"; import RightPanelStore from '../../stores/right-panel/RightPanelStore'; import { TimelineRenderingType } from "../../contexts/RoomContext"; import { KeyBindingAction } from "../../accessibility/KeyboardShortcuts"; +import { SwitchSpacePayload } from "../../dispatcher/payloads/SwitchSpacePayload"; // We need to fetch each pinned message individually (if we don't already have it) // so each pinned message may trigger a request. Limit the number per room for sanity. @@ -535,9 +536,14 @@ class LoggedInView extends React.Component { unread: true, }); break; - default: - // if we do not have a handler for it, pass it to the platform which might - handled = PlatformPeg.get().onKeyDown(ev); + case KeyBindingAction.PreviousVisitedRoomOrCommunity: + PlatformPeg.get().navigateForwardBack(true); + handled = true; + break; + case KeyBindingAction.NextVisitedRoomOrCommunity: + PlatformPeg.get().navigateForwardBack(false); + handled = true; + break; } // Handle labs actions here, as they apply within the same scope @@ -560,12 +566,24 @@ class LoggedInView extends React.Component { handled = true; break; } - default: - // if we do not have a handler for it, pass it to the platform which might - handled = PlatformPeg.get().onKeyDown(ev); } } + if ( + !handled && + PlatformPeg.get().overrideBrowserShortcuts() && + SpaceStore.spacesEnabled && + ev.code.startsWith("Digit") && + ev.code !== "Digit0" && // this is the shortcut for reset zoom, don't override it + isOnlyCtrlOrCmdKeyEvent(ev) + ) { + dis.dispatch({ + action: Action.SwitchSpace, + num: ev.code.slice(5), // Cut off the first 5 characters - "Digit" + }); + handled = true; + } + if (handled) { ev.stopPropagation(); ev.preventDefault(); diff --git a/src/components/views/voip/CallView.tsx b/src/components/views/voip/CallView.tsx index e0a8220e1f..a809adcf41 100644 --- a/src/components/views/voip/CallView.tsx +++ b/src/components/views/voip/CallView.tsx @@ -29,7 +29,6 @@ import { _t, _td } from '../../../languageHandler'; import VideoFeed from './VideoFeed'; import RoomAvatar from "../avatars/RoomAvatar"; import AccessibleButton from '../elements/AccessibleButton'; -import { isOnlyCtrlOrCmdKeyEvent, Key } from '../../../Keyboard'; import { avatarUrlForMember } from '../../../Avatar'; import { replaceableComponent } from "../../../utils/replaceableComponent"; import DesktopCapturerSourcePicker from "../elements/DesktopCapturerSourcePicker"; @@ -38,6 +37,8 @@ import CallViewSidebar from './CallViewSidebar'; import CallViewHeader from './CallView/CallViewHeader'; import CallViewButtons from "./CallView/CallViewButtons"; import PlatformPeg from "../../../PlatformPeg"; +import { getKeyBindingsManager } from "../../../KeyBindingsManager"; +import { KeyBindingAction } from "../../../accessibility/KeyboardShortcuts"; interface IProps { // The call for us to display @@ -293,25 +294,21 @@ export default class CallView extends React.Component { // CallHandler would probably be a better place for this private onNativeKeyDown = (ev): void => { let handled = false; - const ctrlCmdOnly = isOnlyCtrlOrCmdKeyEvent(ev); - switch (ev.key) { - case Key.D: - if (ctrlCmdOnly) { - this.onMicMuteClick(); - // show the controls to give feedback - this.buttonsRef.current?.showControls(); - handled = true; - } + const callAction = getKeyBindingsManager().getCallAction(ev); + switch (callAction) { + case KeyBindingAction.ToggleMicInCall: + this.onMicMuteClick(); + // show the controls to give feedback + this.buttonsRef.current?.showControls(); + handled = true; break; - case Key.E: - if (ctrlCmdOnly) { - this.onVidMuteClick(); - // show the controls to give feedback - this.buttonsRef.current?.showControls(); - handled = true; - } + case KeyBindingAction.ToggleWebcamInCall: + this.onVidMuteClick(); + // show the controls to give feedback + this.buttonsRef.current?.showControls(); + handled = true; break; } diff --git a/src/i18n/strings/en_EN.json b/src/i18n/strings/en_EN.json index f26df700ed..3345e96794 100644 --- a/src/i18n/strings/en_EN.json +++ b/src/i18n/strings/en_EN.json @@ -3442,10 +3442,14 @@ "Jump to first message": "Jump to first message", "Jump to last message": "Jump to last message", "Undo edit": "Undo edit", + "Redo edit": "Redo edit", + "Previous recently visited room or community": "Previous recently visited room or community", + "Next recently visited room or community": "Next recently visited room or community", + "Switch to space by number": "Switch to space by number", + "Open user settings": "Open user settings", "New line": "New line", "Force complete": "Force complete", "Search (must be enabled)": "Search (must be enabled)", "Close dialog or context menu": "Close dialog or context menu", - "Activate selected button": "Activate selected button", - "Redo edit": "Redo edit" + "Activate selected button": "Activate selected button" } diff --git a/test/accessibility/KeyboardShortcuts-test.ts b/test/accessibility/KeyboardShortcuts-test.ts index 88be362af9..f0d45d7959 100644 --- a/test/accessibility/KeyboardShortcuts-test.ts +++ b/test/accessibility/KeyboardShortcuts-test.ts @@ -15,18 +15,24 @@ limitations under the License. */ import { - CATEGORIES, - CategoryName, getCustomizableShortcuts, getKeyboardShortcuts, KEYBOARD_SHORTCUTS, - registerShortcut, + mock, } from "../../src/accessibility/KeyboardShortcuts"; -import { Key } from "../../src/Keyboard"; -import { ISetting } from "../../src/settings/Settings"; +import PlatformPeg from "../../src/PlatformPeg"; describe("KeyboardShortcuts", () => { - it("doesn't change KEYBOARD_SHORTCUTS when getting shortcuts", () => { + it("doesn't change KEYBOARD_SHORTCUTS when getting shortcuts", async () => { + mock({ + keyboardShortcuts: { + "Keybind1": {}, + "Keybind2": {}, + }, + macOnlyShortcuts: ["Keybind1"], + desktopShortcuts: ["Keybind2"], + }); + PlatformPeg.get = () => ({ overrideBrowserShortcuts: () => false }); const copyKeyboardShortcuts = Object.assign({}, KEYBOARD_SHORTCUTS); getCustomizableShortcuts(); @@ -35,22 +41,31 @@ describe("KeyboardShortcuts", () => { expect(KEYBOARD_SHORTCUTS).toEqual(copyKeyboardShortcuts); }); - describe("registerShortcut()", () => { - it("correctly registers shortcut", () => { - const shortcutName = "Keybinding.definitelyARealShortcut"; - const shortcutCategory = CategoryName.NAVIGATION; - const shortcut: ISetting = { - displayName: "A real shortcut", - default: { - ctrlKey: true, - key: Key.A, - }, - }; + it("correctly filters shortcuts", async () => { + mock({ + keyboardShortcuts: { + "Keybind1": {}, + "Keybind2": {}, + "Keybind3": { "controller": { settingDisabled: true } }, + "Keybind4": {}, + }, + macOnlyShortcuts: ["Keybind1"], + desktopShortcuts: ["Keybind2"], - registerShortcut(shortcutName, shortcutCategory, shortcut); - - expect(getKeyboardShortcuts()[shortcutName]).toBe(shortcut); - expect(CATEGORIES[shortcutCategory].settingNames.includes(shortcutName)).toBeTruthy(); }); + PlatformPeg.get = () => ({ overrideBrowserShortcuts: () => false }); + expect(getCustomizableShortcuts()).toEqual({ "Keybind4": {} }); + + mock({ + keyboardShortcuts: { + "Keybind1": {}, + "Keybind2": {}, + }, + macOnlyShortcuts: undefined, + desktopShortcuts: ["Keybind2"], + }); + PlatformPeg.get = () => ({ overrideBrowserShortcuts: () => true }); + expect(getCustomizableShortcuts()).toEqual({ "Keybind1": {}, "Keybind2": {} }); + jest.resetModules(); }); }); diff --git a/test/components/views/rooms/SendMessageComposer-test.tsx b/test/components/views/rooms/SendMessageComposer-test.tsx index 52d6743d0f..dfd58b9e69 100644 --- a/test/components/views/rooms/SendMessageComposer-test.tsx +++ b/test/components/views/rooms/SendMessageComposer-test.tsx @@ -37,6 +37,7 @@ import MatrixToPermalinkConstructor from "../../../../src/utils/permalinks/Matri import defaultDispatcher from "../../../../src/dispatcher/dispatcher"; import DocumentOffset from '../../../../src/editor/offset'; import { Layout } from '../../../../src/settings/enums/Layout'; +import PlatformPeg from "../../../../src/PlatformPeg"; describe('', () => { const roomContext = { @@ -253,6 +254,8 @@ describe('', () => { }); it("persists to session history upon sending", async () => { + PlatformPeg.get = () => ({ overrideBrowserShortcuts: () => false }); + const wrapper = mount(