From 590fdbb0d79f95c65250c3fac088925b676ccfb5 Mon Sep 17 00:00:00 2001 From: Braden Wong <13159333+braden-w@users.noreply.github.com> Date: Thu, 19 Jun 2025 07:05:15 -0700 Subject: [PATCH] feat: use exact matching for PressedKeys --- .../pressed-keys/pressed-keys.svelte.ts | 3 +- .../pressed-keys/pressed-keys.test.ts | 188 ++++++++++++++++++ .../src/content/utilities/pressed-keys.md | 36 +++- spec.md | 74 +++++++ 4 files changed, 298 insertions(+), 3 deletions(-) create mode 100644 packages/runed/src/lib/utilities/pressed-keys/pressed-keys.test.ts create mode 100644 spec.md diff --git a/packages/runed/src/lib/utilities/pressed-keys/pressed-keys.svelte.ts b/packages/runed/src/lib/utilities/pressed-keys/pressed-keys.svelte.ts index 6862012b..f835fe84 100644 --- a/packages/runed/src/lib/utilities/pressed-keys/pressed-keys.svelte.ts +++ b/packages/runed/src/lib/utilities/pressed-keys/pressed-keys.svelte.ts @@ -76,7 +76,8 @@ export class PressedKeys { has(...keys: string[]): boolean { this.#subscribe?.(); const normalizedKeys = keys.map((key) => key.toLowerCase()); - return normalizedKeys.every((key) => this.#pressedKeys.includes(key)); + return normalizedKeys.length === this.#pressedKeys.length && + normalizedKeys.every((key) => this.#pressedKeys.includes(key)); } get all(): string[] { diff --git a/packages/runed/src/lib/utilities/pressed-keys/pressed-keys.test.ts b/packages/runed/src/lib/utilities/pressed-keys/pressed-keys.test.ts new file mode 100644 index 00000000..1c487304 --- /dev/null +++ b/packages/runed/src/lib/utilities/pressed-keys/pressed-keys.test.ts @@ -0,0 +1,188 @@ +import { describe, expect, it, vi } from "vitest"; +import { PressedKeys } from "./pressed-keys.svelte.js"; +import { testWithEffect } from "$lib/internal/test-utils.svelte.js"; + +describe("PressedKeys", () => { + describe("has method", () => { + it("should use exact matching by default", async () => { + await testWithEffect(async () => { + const keys = new PressedKeys(); + const target = document.body; + + // Press only 'k' + target.dispatchEvent(new KeyboardEvent("keydown", { key: "k" })); + expect(keys.has("k")).toBe(true); + expect(keys.has("K")).toBe(true); // case insensitive + + // Now press 'meta' while 'k' is still down + target.dispatchEvent(new KeyboardEvent("keydown", { key: "Meta" })); + expect(keys.has("k")).toBe(false); // exact match fails + expect(keys.has("meta", "k")).toBe(true); // exact match for both keys + expect(keys.has("k", "meta")).toBe(true); // order doesn't matter + + // Release 'meta' + target.dispatchEvent(new KeyboardEvent("keyup", { key: "Meta" })); + expect(keys.has("k")).toBe(true); // back to just 'k' + + // Release 'k' + target.dispatchEvent(new KeyboardEvent("keyup", { key: "k" })); + expect(keys.has("k")).toBe(false); + }); + }); + + + it("should handle modifier keys correctly", async () => { + await testWithEffect(async () => { + const keys = new PressedKeys(); + const target = document.body; + + // Test each modifier + const modifiers = ["Control", "Alt", "Shift", "Meta"]; + + for (const modifier of modifiers) { + target.dispatchEvent(new KeyboardEvent("keydown", { key: modifier })); + expect(keys.has(modifier.toLowerCase())).toBe(true); + target.dispatchEvent(new KeyboardEvent("keyup", { key: modifier })); + } + }); + }); + + it("should clear keys on blur", async () => { + await testWithEffect(async () => { + const keys = new PressedKeys(); + const target = document.body; + + target.dispatchEvent(new KeyboardEvent("keydown", { key: "a" })); + target.dispatchEvent(new KeyboardEvent("keydown", { key: "b" })); + expect(keys.has("a", "b")).toBe(true); + + window.dispatchEvent(new Event("blur")); + expect(keys.all).toEqual([]); + expect(keys.has("a")).toBe(false); + expect(keys.has("b")).toBe(false); + }); + }); + + it("should clear keys on visibility change", async () => { + await testWithEffect(async () => { + const keys = new PressedKeys(); + const target = document.body; + + target.dispatchEvent(new KeyboardEvent("keydown", { key: "x" })); + expect(keys.has("x")).toBe(true); + + // Mock hidden visibility + Object.defineProperty(document, "visibilityState", { + value: "hidden", + writable: true, + }); + document.dispatchEvent(new Event("visibilitychange")); + + expect(keys.all).toEqual([]); + expect(keys.has("x")).toBe(false); + }); + }); + }); + + describe("onKeys method", () => { + it("should use exact matching by default", async () => { + await testWithEffect(async () => { + const keys = new PressedKeys(); + const target = document.body; + const callback = vi.fn(); + + keys.onKeys("k", callback); + + // Press only 'k' - should trigger + target.dispatchEvent(new KeyboardEvent("keydown", { key: "k" })); + expect(callback).toHaveBeenCalledTimes(1); + + // Press 'meta' while 'k' is down - should not trigger again + target.dispatchEvent(new KeyboardEvent("keydown", { key: "Meta" })); + expect(callback).toHaveBeenCalledTimes(1); // still 1 + + // Release 'meta' - should trigger again as we're back to just 'k' + target.dispatchEvent(new KeyboardEvent("keyup", { key: "Meta" })); + expect(callback).toHaveBeenCalledTimes(2); + }); + }); + + + it("should handle multiple keys with exact matching", async () => { + await testWithEffect(async () => { + const keys = new PressedKeys(); + const target = document.body; + const callback = vi.fn(); + + keys.onKeys(["meta", "k"], callback); + + // Press only 'k' - should not trigger + target.dispatchEvent(new KeyboardEvent("keydown", { key: "k" })); + expect(callback).toHaveBeenCalledTimes(0); + + // Press 'meta' - should trigger + target.dispatchEvent(new KeyboardEvent("keydown", { key: "Meta" })); + expect(callback).toHaveBeenCalledTimes(1); + + // Press 'shift' - should not trigger again (exact match fails) + target.dispatchEvent(new KeyboardEvent("keydown", { key: "Shift" })); + expect(callback).toHaveBeenCalledTimes(1); + }); + }); + }); + + describe("all property", () => { + it("should return all pressed keys", async () => { + await testWithEffect(async () => { + const keys = new PressedKeys(); + const target = document.body; + + expect(keys.all).toEqual([]); + + target.dispatchEvent(new KeyboardEvent("keydown", { key: "a" })); + expect(keys.all).toEqual(["a"]); + + target.dispatchEvent(new KeyboardEvent("keydown", { key: "B" })); + expect(keys.all).toEqual(["a", "b"]); + + target.dispatchEvent(new KeyboardEvent("keyup", { key: "a" })); + expect(keys.all).toEqual(["b"]); + }); + }); + }); + + describe("edge cases", () => { + it("should handle repeated keydown events for same key", async () => { + await testWithEffect(async () => { + const keys = new PressedKeys(); + const target = document.body; + + target.dispatchEvent(new KeyboardEvent("keydown", { key: "a" })); + target.dispatchEvent(new KeyboardEvent("keydown", { key: "a" })); + target.dispatchEvent(new KeyboardEvent("keydown", { key: "a" })); + + expect(keys.all).toEqual(["a"]); + expect(keys.has("a")).toBe(true); + }); + }); + + it("should handle modifier release clearing non-modifier keys", async () => { + await testWithEffect(async () => { + const keys = new PressedKeys(); + const target = document.body; + + // Press several keys including modifiers + target.dispatchEvent(new KeyboardEvent("keydown", { key: "a" })); + target.dispatchEvent(new KeyboardEvent("keydown", { key: "Meta" })); + target.dispatchEvent(new KeyboardEvent("keydown", { key: "b" })); + target.dispatchEvent(new KeyboardEvent("keydown", { key: "Shift" })); + + expect(keys.all.sort()).toEqual(["a", "b", "meta", "shift"]); + + // Release one modifier - should clear non-modifiers + target.dispatchEvent(new KeyboardEvent("keyup", { key: "Meta" })); + expect(keys.all).toEqual(["shift"]); + }); + }); + }); +}); \ No newline at end of file diff --git a/sites/docs/src/content/utilities/pressed-keys.md b/sites/docs/src/content/utilities/pressed-keys.md index 0711b36c..a4eb1e6e 100644 --- a/sites/docs/src/content/utilities/pressed-keys.md +++ b/sites/docs/src/content/utilities/pressed-keys.md @@ -23,18 +23,50 @@ const isArrowDownPressed = $derived(keys.has("ArrowDown")); const isCtrlAPressed = $derived(keys.has("Control", "a")); ``` -Or get all of the currently pressed keys: +`has` uses **exact matching** - it only returns `true` when exactly the specified keys are pressed: + +```ts +const keys = new PressedKeys(); + +// If only 'k' is pressed: +keys.has("k"); // true +keys.has("k", "meta"); // false + +// If 'meta' + 'k' are pressed: +keys.has("k"); // false (exact match requires only 'k') +keys.has("k", "meta"); // true +``` + +If you need to check whether a specific key is pressed regardless of modifiers, use the `all` property: + +```ts +// If 'meta' + 'k' are pressed: +keys.all.includes("k"); // true +keys.all.includes("meta"); // true +``` + +### Getting All Pressed Keys + +Get all of the currently pressed keys: ```ts const keys = new PressedKeys(); console.log(keys.all); ``` -Or register a callback to execute when specified key combination is pressed: +### Monitoring Key Combinations + +Register a callback to execute when specified key combination is pressed: ```ts const keys = new PressedKeys(); +// Triggers only when exactly meta+k are pressed keys.onKeys(["meta", "k"], () => { console.log("open command palette"); }); + +// Triggers only when 'k' is pressed alone +keys.onKeys("k", () => { + console.log("k pressed without modifiers"); +}); ``` diff --git a/spec.md b/spec.md new file mode 100644 index 00000000..657e3c78 --- /dev/null +++ b/spec.md @@ -0,0 +1,74 @@ +# PressedKeys Ergonomics Improvement Plan + +## Problem Statement +The current `PressedKeys` implementation has an ergonomics issue where checking for a single key (e.g., `has("k")`) also matches when modifiers are pressed (e.g., Cmd+K). This doesn't align with user expectations or industry best practices. + +## Todo List +- [x] Update the `has` method to use exact matching only +- [x] Update the `onKeys` method to use exact matching only +- [x] Remove inclusive option entirely for simpler API +- [x] Add comprehensive tests for exact matching behavior +- [x] Update documentation with examples of exact matching +- [x] Write PR description explaining the breaking change + +## Implementation Details + +### Option 1: Add options parameter to existing methods +```typescript +// Default behavior (backward compatible - inclusive match) +keys.has("k") // true for both "k" and "Cmd+k" + +// New exact match option +keys.has("k", { exact: true }) // true only for "k" alone +keys.has(["meta", "k"], { exact: true }) // true only for exactly Cmd+k + +// For onKeys +keys.onKeys("k", callback, { exact: true }) // fires only for "k" alone +``` + +### Option 2: Add new methods for exact matching +```typescript +// Existing methods remain unchanged (inclusive) +keys.has("k") // true for both "k" and "Cmd+k" + +// New exact methods +keys.hasExact("k") // true only for "k" alone +keys.onKeysExact("k", callback) // fires only for "k" alone +``` + +### Option 3: Change default behavior (breaking change) +Make exact matching the default and add an option for inclusive matching. This would be more aligned with user expectations but would be a breaking change. + +## Recommendation +I recommend **Option 1** as it: +- Maintains backward compatibility +- Provides clear, explicit control over matching behavior +- Aligns with patterns used in other parts of the codebase +- Makes the API more flexible for different use cases + +## Test Cases to Add +1. `has("k")` returns true when only "k" is pressed +2. `has("k")` returns true when "Cmd+k" is pressed (backward compatibility) +3. `has("k", { exact: true })` returns true only when "k" is pressed alone +4. `has("k", { exact: true })` returns false when "Cmd+k" is pressed +5. `has(["meta", "k"], { exact: true })` returns true only for exactly Cmd+k +6. `onKeys` with exact option fires appropriately +7. Edge cases with multiple modifiers + +## Review + +### Summary of Changes +1. **Simplified the API** - Removed the `KeyMatchOptions` type and `inclusive` option entirely +2. **Made exact matching the only behavior** - `has()` and `onKeys()` now only match when exactly the specified keys are pressed +3. **Updated tests** - Removed tests for inclusive matching and kept only exact matching tests +4. **Breaking change** - Users who relied on the previous inclusive behavior will need to update their code + +### Key Decisions +- Chose simplicity over backward compatibility by removing the inclusive option +- This makes the API more predictable and aligns with user expectations +- Users who need inclusive behavior can use `keys.all.includes("k")` directly + +### Migration Impact +- Any code using `has("k")` that expected it to match "Cmd+K" will break +- The migration path is clear: either specify all keys or use `keys.all.includes()` +- Documentation has been updated to explain the new behavior \ No newline at end of file