Fix Esc key closing Sheets

src/app/dim-ui/Sheet.m.scss

@@ -111,6 +111,7 @@ $control-color: rgba(255, 255, 255, 0.5);
   inset: 0;
   display: none;
   transition: opacity 300ms linear;
+  z-index: 1;
   .sheetDisabled & {
     display: block;
     opacity: 0.6;

src/app/dim-ui/Sheet.tsx

@@ -65,6 +65,12 @@ const animationVariants = {
 const dragConstraints = { top: 0, bottom: window.innerHeight } as const;
 
 const stopPropagation = (e: React.SyntheticEvent) => e.stopPropagation();
+const handleKeyDown = (e: React.KeyboardEvent) => {
+  // Allow "esc" to propagate which lets you escape focus on inputs.
+  if (e.key !== 'Escape') {
+    e.stopPropagation();
+  }
+};
 
 /**
  * Automatically disable the parent sheet while this sheet is shown. You must
@@ -157,18 +163,15 @@ export default function Sheet({
 
   /**
    * Triggering close starts the animation. The onClose prop is called by the callback
-   * passed to the onAnimationComplete motion prop
+   * passed to the onAnimationComplete motion prop.
    */
   const triggerClose = useCallback(
     (e?: React.MouseEvent | KeyboardEvent) => {
-      if (disabled) {
-        return;
-      }
       e?.preventDefault();
       // Animate offscreen
       animationControls.start('close');
     },
-    [disabled, animationControls]
+    [animationControls]
   );
 
   // Handle global escape key
@@ -262,7 +265,7 @@ export default function Sheet({
       ref={sheet}
       role="dialog"
       aria-modal="false"
-      onKeyDown={stopPropagation}
+      onKeyDown={handleKeyDown}
       onKeyUp={stopPropagation}
       onKeyPress={stopPropagation}
       onClick={allowClickThrough ? undefined : stopPropagation}
@@ -298,7 +301,11 @@ export default function Sheet({
           </div>
         )}
       </div>
-      <div className={styles.disabledScreen} />
+      <div
+        className={styles.disabledScreen}
+        onClick={stopPropagation}
+        onPointerDown={stopPropagation}
+      />
     </motion.div>
   );
 

src/app/hotkeys/hotkeys.test.ts

@@ -0,0 +1,45 @@
+import { clearAllHotkeysForTest, registerHotkeys, removeHotkeysById } from './hotkeys';
+
+beforeEach(clearAllHotkeysForTest);
+
+it('stacks hotkeys', () => {
+  let p1 = 0;
+  const handleP1 = () => {
+    p1++;
+  };
+  let p2 = 0;
+  const handleP2 = () => {
+    p2++;
+  };
+  let p3 = 0;
+  const handleP3 = () => {
+    p3++;
+  };
+
+  registerHotkeys('1', [{ combo: 'p', callback: handleP1, description: 'p' }]);
+  document.dispatchEvent(new KeyboardEvent('keydown', { key: 'p' }));
+
+  expect(p1).toBe(1);
+
+  registerHotkeys('2', [{ combo: 'p', callback: handleP2, description: 'p' }]);
+  document.dispatchEvent(new KeyboardEvent('keydown', { key: 'p' }));
+
+  expect(p1).toBe(1);
+  expect(p2).toBe(1);
+
+  // Now, re-register 1 with a different handler
+  registerHotkeys('1', [{ combo: 'p', callback: handleP3, description: 'p' }]);
+  document.dispatchEvent(new KeyboardEvent('keydown', { key: 'p' }));
+
+  // It should still trigger p2!
+  expect(p1).toBe(1);
+  expect(p2).toBe(2);
+  expect(p3).toBe(0);
+
+  removeHotkeysById('2', 'p');
+  document.dispatchEvent(new KeyboardEvent('keydown', { key: 'p' }));
+
+  expect(p1).toBe(1);
+  expect(p2).toBe(2);
+  expect(p3).toBe(1);
+});

src/app/hotkeys/hotkeys.ts

@@ -41,31 +41,52 @@ export function symbolize(combo: string) {
 }
 
 export interface Hotkey {
+  /** The actual hotkey combo, like "shift+p" */
   combo: string;
+  /** A description that'll be shown on the hotkey help screen. */
   description: string;
+  /** What to do when the hotkey is triggered. */
   callback: (event: KeyboardEvent) => void;
 }
 
 // Each key combo can have many hotkey implementations bound to it, but only the
 // last one in the array gets triggered.
-const keyMap: { [combo: string]: Hotkey[] } = {};
+const keyMap: { [combo: string]: (Hotkey & { id: string })[] } = {};
+
+export function clearAllHotkeysForTest() {
+  for (const key of Object.keys(keyMap)) {
+    delete keyMap[key];
+  }
+}
 
 /**
- * Add a new set of hotkeys. Returns an unregister function that can be used to
- * remove these bindings.
+ * Add a new set of hotkeys. The id parameter allows us to preserve this hotkey
+ * in the stack of bindings for the hotkey even when repeatedly registered - use
+ * the useId hook to generate a stable ID for a component to use for this. Call
+ * removeHotkeysById when a component is unmounted or the hotkey is disabled.
  */
-export function registerHotkeys(hotkeys: Hotkey[]) {
+export function registerHotkeys(id: string, hotkeys: Hotkey[]) {
   if (!hotkeys?.length) {
     return noop;
   }
   for (const hotkey of hotkeys) {
-    bind(hotkey);
+    bind(id, hotkey);
   }
-  return () => {
-    for (const hotkey of hotkeys) {
-      unbind(hotkey);
+}
+
+/**
+ * Remove bound hotkeys from the stack by id. This should be the same ID the
+ * hotkey was registered under. Pass combo if you know it to speed up removal.
+ */
+export function removeHotkeysById(id: string, combo?: string) {
+  if (combo) {
+    unbind(id, combo);
+  } else {
+    // Look for the ID in every combo
+    for (const combo of Object.keys(keyMap)) {
+      unbind(id, combo);
     }
-  };
+  }
 }
 
 export function getAllHotkeys() {
@@ -87,14 +108,22 @@ function normalizeCombo(combo: string) {
     .join('+');
 }
 
-function bind(hotkey: Hotkey) {
-  (keyMap[normalizeCombo(hotkey.combo)] ??= []).push(hotkey);
+function bind(id: string, hotkey: Hotkey) {
+  const keys = (keyMap[normalizeCombo(hotkey.combo)] ??= []);
+  // Replace existing hotkeys in the same place in the stack, so re-renders
+  // don't pop the hotkey to the top.
+  const existingIndex = keys.findIndex((h) => h.id === id);
+  if (existingIndex >= 0) {
+    keys[existingIndex] = { ...hotkey, id };
+  } else {
+    keys.push({ ...hotkey, id });
+  }
 }
 
-function unbind(hotkey: Hotkey) {
-  const normalizedCombo = normalizeCombo(hotkey.combo);
+function unbind(id: string, combo: string) {
+  const normalizedCombo = normalizeCombo(combo);
   const hotkeysForCombo = keyMap[normalizedCombo];
-  const existingIndex = hotkeysForCombo.indexOf(hotkey);
+  const existingIndex = hotkeysForCombo.findIndex((h) => h.id === id);
   if (existingIndex >= 0) {
     hotkeysForCombo.splice(existingIndex, 1);
   }

src/app/hotkeys/useHotkey.ts

@@ -1,5 +1,5 @@
-import { useEffect } from 'react';
-import { Hotkey, registerHotkeys } from './hotkeys';
+import { useEffect, useId } from 'react';
+import { Hotkey, registerHotkeys, removeHotkeysById } from './hotkeys';
 
 /**
  * A hook for registering a single global hotkey that will appear in the hotkey
@@ -20,19 +20,23 @@ export function useHotkey(
   callback: (event: KeyboardEvent) => void,
   disabled?: boolean
 ) {
+  const id = useId();
   useEffect(() => {
     if (disabled) {
+      removeHotkeysById(id, combo);
       return;
     }
-    const keys: Hotkey[] = [
+    registerHotkeys(id, [
       {
         combo,
         description,
         callback,
       },
-    ];
-    return registerHotkeys(keys);
-  }, [combo, description, callback, disabled]);
+    ]);
+  }, [id, combo, description, callback, disabled]);
+
+  // Remove the hotkey only once the component unmounts
+  useEffect(() => () => removeHotkeysById(id, combo), [combo, id]);
 }
 
 /**
@@ -43,5 +47,9 @@ export function useHotkey(
  * @see {@link useHotkey}
  */
 export function useHotkeys(hotkeyDefs: Hotkey[]) {
-  useEffect(() => registerHotkeys(hotkeyDefs), [hotkeyDefs]);
+  const id = useId();
+  useEffect(() => registerHotkeys(id, hotkeyDefs), [hotkeyDefs, id]);
+
+  // Remove the hotkeys only once the component unmounts
+  useEffect(() => () => removeHotkeysById(id), [id]);
 }