Skip to content

Commit

Permalink
[dagit] Allow disabling keyboard shortcuts (#7515)
Browse files Browse the repository at this point in the history
## Summary

We received a report that users with QWERTZ keyboard layouts were inadvertently launching runs from Launchpad by typing <kbd>Alt</kbd>+<kbd>L</kbd>, which is `@` in that keyboard layout and therefore a perfectly reasonable thing to type. Our current assumption in shortcut handling is that `Alt` is not expected to produce characters, which is inaccurate for this keyboard layout.

Users may also find that their preferred task manager has keybinding conflicts with Dagit generally.

To that end, allow users to disable keyboard shortcuts in Dagit entirely. This PR adds a localStorage setting and a switch in "User Settings" to allow this customization. By default, shortcuts are enabled. If the user turns them off, they are all disabled and holding down a modifier key no longer displays shortcuts on the app.

Additionally:

- Replace all uses of deprecated `keyCode` with `code`.
- Update the modifier key list used to determine whether to display shortcuts on the page.
- Fix a bug where control keys weren't displaying shortcuts.

## Test Plan

Load Dagit, verify that shortcuts are enabled by default. Go to User Settings, toggle them off. After reload, verify that modifier keys don't show shortcuts on the page and that no shortcuts are enabled. Toggle them back on, verify that they are restored.

With shortcuts disabled, switch to a German keyboard layout (QWERTZ), view Launchpad. Type an `@` into the editor, verify that a run isn't inadvertently launched.
  • Loading branch information
hellendag committed Apr 21, 2022
1 parent f5bdb5e commit 4181eba
Show file tree
Hide file tree
Showing 7 changed files with 63 additions and 15 deletions.
31 changes: 30 additions & 1 deletion js_modules/dagit/packages/core/src/app/SettingsRoot.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -11,15 +11,21 @@ import {
import * as React from 'react';

import {useDocumentTitle} from '../hooks/useDocumentTitle';
import {useStateWithStorage} from '../hooks/useStateWithStorage';

import {FeatureFlag, getFeatureFlags, setFeatureFlags} from './Flags';
import {SHORTCUTS_STORAGE_KEY} from './ShortcutHandler';
import {TimezoneSelect} from './time/TimezoneSelect';
import {automaticLabel} from './time/browserTimezone';

const SettingsRoot = () => {
useDocumentTitle('User settings');

const [flags, setFlags] = React.useState<FeatureFlag[]>(() => getFeatureFlags());
const [shortcutsEnabled, setShortcutsEnabled] = useStateWithStorage(
SHORTCUTS_STORAGE_KEY,
(value: any) => (typeof value === 'boolean' ? value : true),
);

React.useEffect(() => {
setFeatureFlags(flags);
Expand All @@ -37,6 +43,15 @@ const SettingsRoot = () => {
[],
);

const toggleKeyboardShortcuts = (e: React.ChangeEvent<HTMLInputElement>) => {
const {checked} = e.target;
setShortcutsEnabled(checked);
// Delay slightly so that the switch visibly changes first.
setTimeout(() => {
window.location.reload();
}, 1000);
};

return (
<div style={{height: '100vh', overflowY: 'auto'}}>
<PageHeader title={<Heading>User settings</Heading>} />
Expand All @@ -48,7 +63,21 @@ const SettingsRoot = () => {
rows={[
{
key: 'Timezone',
value: <TimezoneSelect trigger={trigger} />,
value: (
<Box margin={{bottom: 4}}>
<TimezoneSelect trigger={trigger} />
</Box>
),
},
{
key: 'Enable keyboard shortcuts',
value: (
<Checkbox
checked={shortcutsEnabled}
format="switch"
onChange={toggleKeyboardShortcuts}
/>
),
},
]}
/>
Expand Down
28 changes: 21 additions & 7 deletions js_modules/dagit/packages/core/src/app/ShortcutHandler.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,18 @@ import * as React from 'react';
import ReactDOM from 'react-dom';
import styled from 'styled-components/macro';

const MODIFIER_KEYCODES = [17, 18, 91, 224];
import {getJSONForKey} from '../hooks/useStateWithStorage';

export const SHORTCUTS_STORAGE_KEY = 'keyboard-shortcuts-enabled';

const MODIFIER_KEYS = [
'ControlLeft',
'ControlRight',
'AltLeft',
'AltRight',
'MetaLeft',
'MetaRight',
];
const SHORTCUT_VISIBILITY_EVENT_TYPE = 'shortcut-visibility';
const SHORTCUT_VISIBLITY_DELAY = 800;

Expand Down Expand Up @@ -41,13 +52,13 @@ const otherModifiersUsed = (event: KeyboardEvent) => {
return (
event.shiftKey ||
(key !== 'Alt' && event.altKey) ||
(key !== 'Ctrl' && event.ctrlKey) ||
(key !== 'Control' && event.ctrlKey) ||
(key !== 'Meta' && event.metaKey)
);
};

window.addEventListener('keydown', (event) => {
const isModifier = MODIFIER_KEYCODES.includes(event.keyCode);
const isModifier = MODIFIER_KEYS.includes(event.code);
if (!isModifier || otherModifiersUsed(event)) {
// If any non-modifiers are pressed or if multiple modifiers are in use, kill the timeout
// and hide the shortcuts.
Expand All @@ -57,7 +68,7 @@ window.addEventListener('keydown', (event) => {
}
});
window.addEventListener('keyup', (event) => {
if (MODIFIER_KEYCODES.includes(event.keyCode)) {
if (MODIFIER_KEYS.includes(event.code)) {
hideShortcuts();
}
});
Expand Down Expand Up @@ -89,9 +100,12 @@ export class ShortcutHandler extends React.Component<ShortcutHandlerProps, Short
};

componentDidMount() {
window.addEventListener('keydown', this.onGlobalKeydown);
window.addEventListener(SHORTCUT_VISIBILITY_EVENT_TYPE, this.onShortcutVisiblityChange);
this.onShortcutVisiblityChange();
const shortcutsEnabled = getJSONForKey(SHORTCUTS_STORAGE_KEY);
if (shortcutsEnabled || shortcutsEnabled === undefined) {
window.addEventListener('keydown', this.onGlobalKeydown);
window.addEventListener(SHORTCUT_VISIBILITY_EVENT_TYPE, this.onShortcutVisiblityChange);
this.onShortcutVisiblityChange();
}
}

componentWillUnmount() {
Expand Down
7 changes: 6 additions & 1 deletion js_modules/dagit/packages/core/src/graph/SVGViewport.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -401,7 +401,12 @@ export class SVGViewport extends React.Component<SVGViewportProps, SVGViewportSt
return;
}

const dir = ({37: 'left', 38: 'up', 39: 'right', 40: 'down'} as const)[e.keyCode];
const dir = ({
ArrowLeft: 'left',
ArrowUp: 'up',
ArrowRight: 'right',
ArrowDown: 'down',
} as const)[e.code];
if (!dir) {
return;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -272,7 +272,7 @@ const ConfigEditorConfigGeneratorPicker: React.FC<ConfigEditorConfigGeneratorPic
<div>
<ShortcutHandler
shortcutLabel="⌥E"
shortcutFilter={(e) => e.keyCode === 69 && e.altKey}
shortcutFilter={(e) => e.code === 'KeyE' && e.altKey}
onShortcut={() => button.current?.click()}
>
<Select<ConfigGenerator>
Expand Down
4 changes: 2 additions & 2 deletions js_modules/dagit/packages/core/src/launchpad/LaunchButton.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ export const LaunchButton = ({config, runCount}: LaunchButtonProps) => {
<ShortcutHandler
onShortcut={onClick}
shortcutLabel="⌥L"
shortcutFilter={(e) => e.keyCode === 76 && e.altKey}
shortcutFilter={(e) => e.code === 'KeyL' && e.altKey}
>
<ButtonWithConfiguration
status={status}
Expand Down Expand Up @@ -118,7 +118,7 @@ export const LaunchButtonDropdown = ({
<ShortcutHandler
onShortcut={() => onConfigSelected(primary)}
shortcutLabel="⌥L"
shortcutFilter={(e) => e.keyCode === 76 && e.altKey}
shortcutFilter={(e) => e.code === 'KeyL' && e.altKey}
>
<ButtonWithConfiguration
status={status}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -591,7 +591,7 @@ const LaunchpadSessionContainer: React.FC<LaunchpadSessionContainerProps> = (pro
<>
<ShortcutHandler
shortcutLabel="⌥T"
shortcutFilter={(e) => e.keyCode === 84 && e.altKey}
shortcutFilter={(e) => e.code === 'KeyT' && e.altKey}
onShortcut={openTagEditor}
>
<Button onClick={openTagEditor} icon={<Icon name="add_circle" />}>
Expand Down
4 changes: 2 additions & 2 deletions js_modules/dagit/packages/core/src/launchpad/TagEditor.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,7 @@ export const TagEditor: React.FC<ITagEditorProps> = ({
<Button onClick={onRequestClose}>Cancel</Button>
<ShortcutHandler
shortcutLabel="⌥Enter"
shortcutFilter={(e) => e.keyCode === 13 && e.altKey}
shortcutFilter={(e) => e.code === 'Enter' && e.altKey}
onShortcut={onSave}
>
<Button intent="primary" onClick={onSave} disabled={disabled}>
Expand Down Expand Up @@ -227,7 +227,7 @@ interface ITagEditorLinkProps {
const TagEditorLink = ({onRequestOpen, children}: ITagEditorLinkProps) => (
<ShortcutHandler
shortcutLabel="⌥T"
shortcutFilter={(e) => e.keyCode === 84 && e.altKey}
shortcutFilter={(e) => e.code === 'KeyT' && e.altKey}
onShortcut={onRequestOpen}
>
<Link onClick={onRequestOpen}>{children}</Link>
Expand Down

0 comments on commit 4181eba

Please sign in to comment.