mirror of
https://github.com/maputnik/editor.git
synced 2026-09-15 00:37:31 +00:00
Fix keyboard shortcuts being ignored while the map has focus (#2157)
## 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
This commit is contained in:
@@ -13,6 +13,7 @@
|
|||||||
- Preserve expanded layer groups when deleting layers, including the first layer of a group
|
- 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 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`)
|
- 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..._
|
- _...Add new stuff here..._
|
||||||
|
|
||||||
## 3.1.0
|
## 3.1.0
|
||||||
|
|||||||
@@ -62,5 +62,22 @@ describe("keyboard", () => {
|
|||||||
await when.typeKeys("!");
|
await when.typeKeys("!");
|
||||||
await then(get.elementByTestId("modal:debug")).shouldBeVisible();
|
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();
|
||||||
|
});
|
||||||
|
});
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|||||||
+17
-1
@@ -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 = {
|
type AppState = {
|
||||||
errors: MappedError[],
|
errors: MappedError[],
|
||||||
infos: string[],
|
infos: string[],
|
||||||
@@ -242,7 +258,7 @@ export class App extends React.Component<any, AppState> {
|
|||||||
(e.target as HTMLElement).blur();
|
(e.target as HTMLElement).blur();
|
||||||
document.body.focus();
|
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) => {
|
const shortcut = shortcuts.find((shortcut) => {
|
||||||
return (shortcut.key === e.key);
|
return (shortcut.key === e.key);
|
||||||
});
|
});
|
||||||
|
|||||||
Reference in New Issue
Block a user