Keep shortcuts working after a checkbox is clicked
Ticking "select all" disabled every keyboard shortcut until the reader clicked somewhere else (#260). Same for the per-message checkboxes, so selecting a few messages and pressing e to archive them did nothing. The guard that stops "a" archiving while you are typing into the search box tested `tagName === "INPUT"`. That is also true of a checkbox, and a checkbox keeps focus after a click -- correctly, since space should toggle it again. So the guard was suppressing shortcuts for an element that swallows no keystroke: space is handled by the browser before this listener runs. The question is not "is this an input" but "does this input take text", which is what isTextEntry now asks. A <select> counts, in the sense that matters here: typing a letter jumps to the option starting with it, and a shortcut would steal that. Thirteen checkboxes and seven file inputs across the app were affected, not just the one reported. The regression test was checked against the old guard first: it fails there and passes here, which is the only thing that makes it a regression test.
This commit is contained in:
@@ -0,0 +1,94 @@
|
|||||||
|
import { afterEach, describe, expect, it, vi } from "vitest";
|
||||||
|
import { isTextEntry, keyboard } from "@/lib/keyboard";
|
||||||
|
|
||||||
|
/*
|
||||||
|
* Shortcuts after a click on a checkbox (#260).
|
||||||
|
*
|
||||||
|
* The guard that stops "a" archiving while you are typing into the search box
|
||||||
|
* tested `tagName === "INPUT"`, which is also true of a checkbox. A checkbox
|
||||||
|
* keeps focus after a click, so ticking "select all" disabled every shortcut
|
||||||
|
* until the reader clicked somewhere else — and nothing about a checkbox
|
||||||
|
* swallows a keystroke in the first place.
|
||||||
|
*/
|
||||||
|
|
||||||
|
const pressFrom = (el: Element, key: string) => {
|
||||||
|
const e = new KeyboardEvent("keydown", { key, bubbles: true, cancelable: true });
|
||||||
|
el.dispatchEvent(e);
|
||||||
|
return e;
|
||||||
|
};
|
||||||
|
|
||||||
|
let pop: (() => void) | null = null;
|
||||||
|
afterEach(() => {
|
||||||
|
pop?.();
|
||||||
|
pop = null;
|
||||||
|
document.body.innerHTML = "";
|
||||||
|
vi.restoreAllMocks();
|
||||||
|
});
|
||||||
|
|
||||||
|
describe("isTextEntry", () => {
|
||||||
|
const input = (type?: string) => {
|
||||||
|
const el = document.createElement("input");
|
||||||
|
if (type) el.setAttribute("type", type);
|
||||||
|
return el;
|
||||||
|
};
|
||||||
|
|
||||||
|
it("is false for the inputs you cannot type into", () => {
|
||||||
|
for (const type of ["checkbox", "radio", "button", "submit", "reset", "file", "color", "range"]) {
|
||||||
|
expect(isTextEntry(input(type)), type).toBe(false);
|
||||||
|
}
|
||||||
|
});
|
||||||
|
|
||||||
|
it("is true for the ones you can", () => {
|
||||||
|
for (const type of ["text", "search", "email", "url", "tel", "password", "number", "date", "time"]) {
|
||||||
|
expect(isTextEntry(input(type)), type).toBe(true);
|
||||||
|
}
|
||||||
|
});
|
||||||
|
|
||||||
|
it("treats an input with no type as text, which is what the browser does", () => {
|
||||||
|
expect(isTextEntry(input())).toBe(true);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("covers textarea, select and contenteditable", () => {
|
||||||
|
expect(isTextEntry(document.createElement("textarea"))).toBe(true);
|
||||||
|
// A select takes letters too: typing jumps to the matching option, and a
|
||||||
|
// shortcut would steal that.
|
||||||
|
expect(isTextEntry(document.createElement("select"))).toBe(true);
|
||||||
|
const div = document.createElement("div");
|
||||||
|
div.contentEditable = "true";
|
||||||
|
Object.defineProperty(div, "isContentEditable", { value: true });
|
||||||
|
expect(isTextEntry(div)).toBe(true);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("is false for a button and for nothing at all", () => {
|
||||||
|
expect(isTextEntry(document.createElement("button"))).toBe(false);
|
||||||
|
expect(isTextEntry(null)).toBe(false);
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
|
describe("shortcuts with a checkbox focused", () => {
|
||||||
|
it("still fire — the reported bug", () => {
|
||||||
|
const handler = vi.fn();
|
||||||
|
pop = keyboard.pushScope("test", [{ keys: "e", description: "Archive", group: "Mail", handler }]);
|
||||||
|
|
||||||
|
const box = document.createElement("input");
|
||||||
|
box.type = "checkbox";
|
||||||
|
document.body.appendChild(box);
|
||||||
|
box.focus();
|
||||||
|
|
||||||
|
pressFrom(box, "e");
|
||||||
|
expect(handler).toHaveBeenCalledTimes(1);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("still do not fire from a text field", () => {
|
||||||
|
const handler = vi.fn();
|
||||||
|
pop = keyboard.pushScope("test", [{ keys: "e", description: "Archive", group: "Mail", handler }]);
|
||||||
|
|
||||||
|
const field = document.createElement("input");
|
||||||
|
field.type = "search";
|
||||||
|
document.body.appendChild(field);
|
||||||
|
field.focus();
|
||||||
|
|
||||||
|
pressFrom(field, "e");
|
||||||
|
expect(handler).not.toHaveBeenCalled();
|
||||||
|
});
|
||||||
|
});
|
||||||
+33
-3
@@ -54,9 +54,7 @@ class Keyboard {
|
|||||||
// Let modal dialogs and popovers handle their own keys (Escape, arrows, ...).
|
// Let modal dialogs and popovers handle their own keys (Escape, arrows, ...).
|
||||||
if (document.querySelector(".dialog-backdrop, .popover")) return;
|
if (document.querySelector(".dialog-backdrop, .popover")) return;
|
||||||
const target = e.target as HTMLElement | null;
|
const target = e.target as HTMLElement | null;
|
||||||
const inInput =
|
const inInput = isTextEntry(target);
|
||||||
!!target &&
|
|
||||||
(target.tagName === "INPUT" || target.tagName === "TEXTAREA" || target.tagName === "SELECT" || target.isContentEditable);
|
|
||||||
const combo = comboOf(e);
|
const combo = comboOf(e);
|
||||||
if (!combo) return;
|
if (!combo) return;
|
||||||
|
|
||||||
@@ -102,6 +100,38 @@ class Keyboard {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Is the focused element somewhere the reader is typing?
|
||||||
|
*
|
||||||
|
* This guard exists so that pressing "a" in the search box searches for "a"
|
||||||
|
* rather than archiving the message behind it. The test used to be
|
||||||
|
* `tagName === "INPUT"`, which is true of a checkbox — and a checkbox keeps
|
||||||
|
* focus after you click it, so ticking "select all" silently disabled every
|
||||||
|
* shortcut until the reader clicked somewhere else (#260). Nothing about a
|
||||||
|
* checkbox swallows a keystroke: space toggles it and the browser handles
|
||||||
|
* that before this listener ever runs.
|
||||||
|
*
|
||||||
|
* So the question is not "is this an input" but "does this input take text".
|
||||||
|
* A `<select>` does, in the sense that matters here: typing a letter jumps to
|
||||||
|
* the option beginning with it, which a shortcut would steal.
|
||||||
|
*/
|
||||||
|
const TEXT_ENTRY_TYPES = new Set([
|
||||||
|
"text", "search", "email", "url", "tel", "password", "number",
|
||||||
|
"date", "datetime-local", "month", "time", "week",
|
||||||
|
]);
|
||||||
|
|
||||||
|
export function isTextEntry(el: Element | null): boolean {
|
||||||
|
if (!el) return false;
|
||||||
|
const node = el as HTMLElement;
|
||||||
|
if (node.isContentEditable) return true;
|
||||||
|
const tag = node.tagName;
|
||||||
|
if (tag === "TEXTAREA" || tag === "SELECT") return true;
|
||||||
|
if (tag !== "INPUT") return false;
|
||||||
|
// An <input> with no type attribute is a text field.
|
||||||
|
const type = (node as HTMLInputElement).type?.toLowerCase() || "text";
|
||||||
|
return TEXT_ENTRY_TYPES.has(type);
|
||||||
|
}
|
||||||
|
|
||||||
const isMac = typeof navigator !== "undefined" && /Mac|iPhone|iPad/.test(navigator.platform);
|
const isMac = typeof navigator !== "undefined" && /Mac|iPhone|iPad/.test(navigator.platform);
|
||||||
|
|
||||||
export function comboOf(e: KeyboardEvent): string | null {
|
export function comboOf(e: KeyboardEvent): string | null {
|
||||||
|
|||||||
Reference in New Issue
Block a user