fix: the page menu must not claim a modified key (ChatGPT Codex, PR #123)
The menu's early return sat ABOVE `app.js`'s `metaKey` guard, so with the menu open `Cmd-R` reached `menuKey`, was answered `ignore`, and was then defaulted away by `if (!e.ctrlKey && !e.altKey) e.preventDefault()` — true for Meta. Reload was blocked. `Ctrl-j`/`Alt-Home` had the mirror fault and MOVED the menu, two lines below a comment saying Ctrl and Alt stay the browser's. Ordering plus one bail: the `metaKey` guard goes back above the menu branch, `pageMenuKeydown` returns on `ctrlKey || altKey`, and what remains claims unconditionally — every unmodified key, `ignore` included, which is the rule the menu exists to keep. The harness assertion installs its `defaultPrevented` probe AFTER load so it runs second and sees what the page decided, and ends on an unmodified `j` asserting `["j", true]` — without which "nothing was prevented" is satisfied by a page that claims nothing. Proved by restoring the reviewed code: it fails on `Control+j` moving the active row. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH
This commit is contained in:
@@ -3458,3 +3458,32 @@ improvise-wiew's product call, executed. The overlay is open, `]` is pressed,
|
||||
and the page bar still reads `Quarter = Q1 (1/4)`. Its sibling — a panel open,
|
||||
`]` pressed, bar moves to Q2 *and the panel is still open* — is what stops the
|
||||
first one passing because the key does nothing anywhere.
|
||||
|
||||
### a modified key is the browser's, and the menu neither acts on it nor claims it
|
||||
|
||||
Found by ChatGPT Codex on PR #123, and correct. The page menu's early return
|
||||
sat **above** `app.js`'s `metaKey` guard, so with the menu open `Cmd-R` reached
|
||||
`menuKey`, was answered `ignore`, and then hit
|
||||
`if (!e.ctrlKey && !e.altKey) e.preventDefault()` — which is true for a Meta
|
||||
combination. Reload was blocked. `Ctrl-j` and `Alt-Home` had the mirror problem
|
||||
in the other direction: they *moved* the menu, although the comment two lines
|
||||
below said Ctrl and Alt combinations stay the browser's.
|
||||
|
||||
The repair is ordering plus one bail: the `metaKey` guard moves back above the
|
||||
menu branch (so the menu is not even consulted), `pageMenuKeydown` returns
|
||||
immediately on `ctrlKey || altKey`, and what remains then claims
|
||||
**unconditionally** — every unmodified key including `ignore`, which is the rule
|
||||
the menu exists to keep.
|
||||
|
||||
**Why the assertion is shaped the way it is.** The listener that records
|
||||
`defaultPrevented` is installed by `page.evaluate` *after* the page has loaded,
|
||||
so it runs **second** on the same target and observes what `app.js` decided. One
|
||||
registered first would read `false` for every key and pass against any code at
|
||||
all. And the test ends by pressing an **unmodified** `j` and asserting
|
||||
`["j", true]`: without that row, "nothing was defaulted away" is equally
|
||||
satisfied by a page that claims nothing, so the modifier would not be what the
|
||||
test is about.
|
||||
|
||||
Fault injected: the reviewed code restored exactly — menu branch above the
|
||||
`metaKey` guard, no `ctrlKey || altKey` bail, conditional `preventDefault`. The
|
||||
test fails on `Control+j` moving the active row from `✓ Q1` to ` Q2`.
|
||||
|
||||
@@ -83,6 +83,44 @@ test.describe("a page axis can be picked from a menu", () => {
|
||||
await expect(chip(page)).toHaveText(/Quarter = Q2 \(2\/4\)/);
|
||||
});
|
||||
|
||||
// Found by ChatGPT Codex on PR #123: the menu branch sat ABOVE the page's
|
||||
// `metaKey` guard, so Cmd-R was answered `ignore` and then defaulted away.
|
||||
test("a modified key is the browser's, and the menu neither acts on it nor claims it", async ({
|
||||
page,
|
||||
}) => {
|
||||
await theDemo(page);
|
||||
// Registered AFTER `app.js`'s own window listener, so it runs second and
|
||||
// reads what the page did — a listener added first would see `false` always.
|
||||
await page.evaluate(() => {
|
||||
window.__claimed = [];
|
||||
window.addEventListener("keydown", (e) =>
|
||||
window.__claimed.push([e.key, e.defaultPrevented]),
|
||||
);
|
||||
});
|
||||
await chip(page).click();
|
||||
await expect(menu(page)).toBeVisible();
|
||||
await expect(menu(page).locator(".page-item.active")).toHaveText("✓ Q1");
|
||||
|
||||
// `Ctrl-j` is not `j`: it must not move the menu the way the bare key does.
|
||||
await page.keyboard.press("Control+j");
|
||||
await expect(menu(page).locator(".page-item.active")).toHaveText("✓ Q1");
|
||||
await page.keyboard.press("Meta+r");
|
||||
await expect(menu(page)).toBeVisible();
|
||||
|
||||
const claimed = await page.evaluate(() => window.__claimed);
|
||||
expect(claimed, `nothing reached the handler: ${JSON.stringify(claimed)}`).not.toEqual([]);
|
||||
for (const [key, prevented] of claimed) {
|
||||
expect(prevented, `${key} was defaulted away; it is the browser's`).toBe(false);
|
||||
}
|
||||
|
||||
// The precondition the rows above lean on: an UNMODIFIED key still is
|
||||
// claimed, so "nothing was prevented" is a fact about the modifier.
|
||||
await page.keyboard.press("j");
|
||||
await expect(menu(page).locator(".page-item.active")).toHaveText(" Q2");
|
||||
const after = await page.evaluate(() => window.__claimed.at(-1));
|
||||
expect(after).toEqual(["j", true]);
|
||||
});
|
||||
|
||||
test("Escape closes the menu and changes nothing", async ({ page }) => {
|
||||
await theDemo(page);
|
||||
await chip(page).click();
|
||||
|
||||
+11
-7
@@ -724,6 +724,9 @@ el.grid.addEventListener("mousedown", (event) => {
|
||||
/** The menu's own keys, decided in `shell.js`. Every one of them is claimed:
|
||||
* an unhandled key must not reach the grid behind the menu. */
|
||||
function pageMenuKeydown(e) {
|
||||
// Ctrl and Alt combinations are the browser's, exactly as below, so the menu
|
||||
// neither acts on one nor claims it — `Ctrl-j` is not `j` (improvise-rw6l).
|
||||
if (e.ctrlKey || e.altKey) return;
|
||||
const items = pageAxes[pageMenu.axis].items;
|
||||
const action = menuKey(e.key, { count: items.length, active: pageMenu.active });
|
||||
if (action.kind === "close") closePageMenu();
|
||||
@@ -732,23 +735,24 @@ function pageMenuKeydown(e) {
|
||||
pageMenu.active = action.item;
|
||||
paintPageMenu();
|
||||
}
|
||||
// Ctrl and Alt combinations stay the browser's, exactly as below.
|
||||
if (!e.ctrlKey && !e.altKey) e.preventDefault();
|
||||
// Every unmodified key is CLAIMED, `ignore` included: the grid is behind
|
||||
// the menu and must not also act on it.
|
||||
e.preventDefault();
|
||||
}
|
||||
|
||||
window.addEventListener("keydown", (e) => {
|
||||
// The inline editor owns the keyboard while it is open. It stops
|
||||
// propagation, so this is belt and braces for a focus we did not predict.
|
||||
if (editing) return;
|
||||
// …and so does the page menu, which is the third state of that kind: it is
|
||||
// anchored to the grid, so letting `hjkl` through would scroll it away.
|
||||
// BrowserKey drops anything held with the platform meta key precisely so
|
||||
// Cmd-R does not arrive as a bare `r`; do not claim it here either.
|
||||
if (e.metaKey) return;
|
||||
// …and the page menu owns the keyboard too, which is the third state of
|
||||
// that kind: `hjkl` reaching the grid would scroll it out from under it.
|
||||
if (pageMenu) {
|
||||
pageMenuKeydown(e);
|
||||
return;
|
||||
}
|
||||
// BrowserKey drops anything held with the platform meta key precisely so
|
||||
// Cmd-R does not arrive as a bare `r`; do not claim it here either.
|
||||
if (e.metaKey) return;
|
||||
// The notice's <summary> is a real control and owns Enter and Space while
|
||||
// it has focus. Claiming them here would call `preventDefault` on the
|
||||
// browser's own disclosure toggle, so a keyboard visitor could focus it and
|
||||
|
||||
Reference in New Issue
Block a user