From 95bf5f2c55654a9ce272036fdbf589d11cf23028 Mon Sep 17 00:00:00 2001 From: CHIIMYEN Date: Mon, 14 Sep 2026 13:33:39 +0800 Subject: [PATCH] Fix keyboard shortcuts being ignored while the map has focus (#2157) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Launch Checklist - [x] Briefly describe the changes in this PR. - [x] Link to related issues. - [ ] Include before/after visuals or gifs if this PR includes visual changes. — not applicable, nothing visual changes. - [x] Write tests for all new functionality. - [x] Add an entry to `CHANGELOG.md` under the `## main` section. ### What changed Keyboard shortcuts went dead as soon as the map had focus. The global `keyup` handler in `App.tsx` gated shortcuts on `document.activeElement === document.body`. That reads like "the user is not typing", but it actually asks "is nothing focused at all". Clicking the map makes `.maplibregl-canvas` the active element, so every shortcut stopped working until the map was blurred. The `m` shortcut demonstrates the problem nicely: its entire job is to focus that canvas, so pressing `m` silently disabled all shortcuts until `Esc`. That path is covered by an existing test (`'m' should focus map`), which is why the regression was easy to miss. ### How `isTextEntryElement()` now answers the question the guard was reaching for, so shortcuts are suppressed only for text entry targets: `input`, `textarea`, `select`, and `contenteditable` — the last one covering the CodeMirror editor, which is a `contenteditable` `.cm-content`. The check stays independent of either renderer's DOM. MapLibre's `.maplibregl-canvas` and the OpenLayers viewport live in different structures, so an allow-list of canvas class names would have fixed one renderer and left the other broken. ### Behaviour change Single-letter shortcuts now also fire while focus is on a button or a panel, where previously they did not. That is inherent to making the map case work — the old predicate simply excluded everything that was not `document.body`. Per @HarelM, this can be reverted if anyone complains. ### Tests `e2e/keyboard.spec.ts` gains a `while the map has focus` block. Its `beforeEach` focuses the canvas through the `m` shortcut and asserts the canvas really is focused, then two independent tests check that `!` and `s` still open their modals. Verified the new tests fail without the fix: ``` Error: expect(locator).toBeVisible() failed Locator: locator('[data-wd-key="modal:debug"]').first() Expected: visible Error: element(s) not found ``` and pass with it. Local results: | Check | Result | |---|---| | `npm run lint` | clean | | `npx vitest run` | 9 files, 50 tests passed | | `npx vite build --mode=production` | clean | | `npx playwright test` | 170 passed, 3 failed | The 3 failures — `modals › open › upload via drag and drop`, `modals › global state › remove variable` and `modals › global state › edit variable key` — also fail on unmodified `main`, so they are pre-existing and unrelated to this change. Fixes #940 --- CHANGELOG.md | 1 + e2e/keyboard.spec.ts | 17 +++++++++++++++++ src/components/App.tsx | 18 +++++++++++++++++- 3 files changed, 35 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 70826293..6ba88776 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,7 @@ - Preserve expanded layer groups when deleting layers, including the first layer of a group - The map's data listener now fires on tile loads again, so source and vector layer field autocompletion is populated - The `maputnik` desktop binary now opens the default browser automatically on startup (opt out with `--no-browser`) +- Keyboard shortcuts now keep working while the map has focus, instead of going dead until the map is blurred - _...Add new stuff here..._ ## 3.1.0 diff --git a/e2e/keyboard.spec.ts b/e2e/keyboard.spec.ts index 8b67f12f..7ac6bd6f 100644 --- a/e2e/keyboard.spec.ts +++ b/e2e/keyboard.spec.ts @@ -62,5 +62,22 @@ describe("keyboard", () => { await when.typeKeys("!"); await then(get.elementByTestId("modal:debug")).shouldBeVisible(); }); + + describe("while the map has focus", () => { + beforeEach(async () => { + await when.typeKeys("m"); + await then(get.canvas()).shouldBeFocused(); + }); + + test("'!' should show debug modal", async () => { + await when.typeKeys("!"); + await then(get.elementByTestId("modal:debug")).shouldBeVisible(); + }); + + test("'s' should show settings modal", async () => { + await when.typeKeys("s"); + await then(get.elementByTestId("modal:settings")).shouldBeVisible(); + }); + }); }); }); diff --git a/src/components/App.tsx b/src/components/App.tsx index 09bc13a5..2cbcdc50 100644 --- a/src/components/App.tsx +++ b/src/components/App.tsx @@ -83,6 +83,22 @@ function updateRootSpec(spec: any, fieldName: string, newValues: any) { }; } +/** + * Whether the given element consumes keystrokes as text. + * + * Shortcuts have to stay out of the way while the user is typing, but asking + * whether the focus is on `document.body` answers a different question: the map + * canvas is not a text field, yet focusing it used to disable every shortcut. + */ +function isTextEntryElement(element: Element | null): boolean { + if (!element) return false; + const node = element as HTMLElement; + return node.isContentEditable || + node.tagName === "INPUT" || + node.tagName === "TEXTAREA" || + node.tagName === "SELECT"; +} + type AppState = { errors: MappedError[], infos: string[], @@ -242,7 +258,7 @@ export class App extends React.Component { (e.target as HTMLElement).blur(); document.body.focus(); } - else if(this.state.isOpen.shortcuts || document.activeElement === document.body) { + else if(this.state.isOpen.shortcuts || !isTextEntryElement(document.activeElement)) { const shortcut = shortcuts.find((shortcut) => { return (shortcut.key === e.key); });