Skip to content

useKeyboardShortcut fires inside text inputs/textareas and doesn't exclude extra held modifiers #30

Description

@abayomicornelius

Problem

src/hooks/useKeyboardShortcut.ts, in full:

export function useKeyboardShortcut(key: string, callback: () => void, { modifier, enabled = true }: Options = {}) {
  useEffect(() => {
    if (!enabled) return
    const handler = (e: KeyboardEvent) => {
      const modOk = !modifier ||
        (modifier === "ctrl" && e.ctrlKey) ||
        (modifier === "meta" && e.metaKey) ||
        (modifier === "alt"  && e.altKey)  ||
        (modifier === "shift" && e.shiftKey)
      if (modOk && e.key.toLowerCase() === key.toLowerCase()) {
        e.preventDefault(); callback()
      }
    }
    window.addEventListener("keydown", handler)
    return () => window.removeEventListener("keydown", handler)
  }, [key, callback, modifier, enabled])
}

Two independent, concrete correctness problems:

  1. No check for e.target being a form control. The listener is attached to window and fires regardless of what element has focus. There is no check for document.activeElement/e.target being an <input>, <textarea>, or contenteditable element. Any component using this hook for a single-letter shortcut (e.g. "/" for search-focus, "n" for "new", patterns very common in apps with a contact list / batch-recipient list / memo fields — all present in this codebase) will hijack that keystroke while the user is typing it into a text field elsewhere on the same page.
  2. modOk only checks that the specified modifier is held — it never checks that no other, unspecified modifier is also held. (modifier === "ctrl" && e.ctrlKey) is satisfied by Ctrl+K and by Ctrl+Shift+K and by Ctrl+Alt+Ke.shiftKey/e.altKey are never inspected when modifier === "ctrl". So a shortcut registered as useKeyboardShortcut("k", cb, { modifier: "ctrl" }) also fires for Ctrl+Shift+K, which browsers and OSes very commonly reserve for something else (e.g. many browsers use Ctrl+Shift+K/Cmd+Shift+K for their own devtools/console shortcuts) — this hook's e.preventDefault() will fire and swallow the keystroke regardless, stealing it from the browser or OS shortcut the user actually intended, in addition to also incorrectly triggering this app's own callback for a chord the developer never intended to bind.

Why it matters

  • Point 1 is a direct accessibility/usability regression for any keyboard-driven power-user workflow this hook is used for — exactly the users a keyboard-shortcut feature is built to serve are the ones most likely to be interrupted by it firing while they're mid-typing in an adjacent field.
  • Point 2 means every modifier-gated shortcut registered via this hook is silently broader than its author intended, with no test or type system to catch it — { modifier: "ctrl" } reads like "only Ctrl" but behaves like "Ctrl, optionally with Shift and/or Alt too."

Reproduction

  • Register useKeyboardShortcut("k", cb, { modifier: "ctrl" }), then dispatch a KeyboardEvent with key: "k", ctrlKey: true, shiftKey: truecb fires, even though only Ctrl+K (no Shift) was intended to be bound.
  • Render any component using this hook for an un-modified single-key shortcut (e.g. useKeyboardShortcut("n", cb)), focus a text <input> elsewhere on the page, and type the letter "n" — the shortcut's callback fires and e.preventDefault() blocks the character from appearing in the input.

Suggested fix

  • Add a guard at the top of handler: skip (return early) when e.target is an HTMLInputElement, HTMLTextAreaElement, HTMLSelectElement, or an element with isContentEditable, unless the shortcut explicitly opts into firing during text entry (e.g. an allowInInputs option for the rare shortcut that should still work, like Escape-to-close).
  • Fix modOk to check that all four modifier flags match an explicit expected combination rather than just the one named modifier — e.g. build an expected { ctrl, meta, alt, shift } tuple from Options (defaulting unset ones to false) and compare all four against the event's flags exactly.

Edge cases

  • Some shortcuts legitimately want to allow Shift as a "don't care" (e.g. Ctrl+K and Ctrl+Shift+K are sometimes intentionally aliased to the same action) — the fix should keep that possible via an explicit opt-in (e.g. allow specifying modifier: ["ctrl"] as "these must be held" separately from others being "don't care"), not hardcode strict exact-match as the only mode.

Testing strategy

  • Unit test: register with modifier: "ctrl", dispatch Ctrl+Shift+<key>, assert the callback does not fire (regression test for point 2).
  • Unit test: register a shortcut, render a focused <input>, simulate the bound key being typed into the input, assert the callback does not fire and the character is not prevented from being entered (regression test for point 1).

Metadata

Metadata

Assignees

No one assigned

    Labels

    GrantFox OSSIssue tracked in GrantFox OSSMaybe RewardedIssue may be eligible for a GrantFox rewardOfficial Campaign | FWC26Campaign: Official Campaign | FWC26accessibilityAccessibility (a11y)bugSomething isn't workingvery hardVery difficult / senior-level bounty issue

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions