diff --git a/context/test-evidence/clients.md b/context/test-evidence/clients.md index 60f4ae6f..7619e5ec 100644 --- a/context/test-evidence/clients.md +++ b/context/test-evidence/clients.md @@ -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`. diff --git a/harness/tests/pages.spec.js b/harness/tests/pages.spec.js index 3a542747..90fd338a 100644 --- a/harness/tests/pages.spec.js +++ b/harness/tests/pages.spec.js @@ -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(); diff --git a/web/app.js b/web/app.js index 3374d13c..2f24eda1 100644 --- a/web/app.js +++ b/web/app.js @@ -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 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