Skip to content

Commit b2ff43f

Browse files
committed
PR kahme247#255: fix(ask): keep the keyboard up while an option is pressed during Other typing
Squashed from origin/fix/ask-option-tap-keeps-keyboard (1734bdd) onto upstream/main.
1 parent 0ff9ab2 commit b2ff43f

4 files changed

Lines changed: 327 additions & 55 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ All notable changes to **omp-web** (`@kahme247/ompweb`) are documented in this f
1616

1717
### Fixed
1818

19-
- On a phone, the first tap on **Submit** (or **Cancel**) in a question panel now acts on it while you are typing in **Other**. It used to only close the keyboard and resize the panel, which moved the button out from under the tap.
19+
- On a phone, the first tap on an option, **Submit** or **Cancel** in a question panel now acts on it. It used to only close the keyboard or resize the panel, which moved the target out from under the tap. The message box's keyboard now closes as soon as the question appears (the message box is hidden then anyway), the panel no longer resizes when an option or button takes focus, and while you type in **Other** a touch or pen tap elsewhere in the panel no longer moves focus first. Choosing an option then closes the keyboard; tapping back into the kept **Other** text chooses it again. **Submit** closes the keyboard at once instead of after the server confirms the answer, and collapsing the keyboard while typing in **Other** no longer makes the next tap raise it again for a moment. A mouse, including one on a touchscreen laptop, keeps its usual behavior.
2020

2121
---
2222

‎components/ChatWindow.tsx‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1008,7 +1008,15 @@ export function ChatWindow({ session, newSessionCwd, newSessionWorkspace, toolCa
10081008
return null;
10091009
}, [bashRunning, pendingBash, isCompacting, agentRunning, streamState.streamingMessage, pendingToolHeaders.length, agentPhase, activeSubagentCount, currentTodoPhase, t, tn]);
10101010

1011-
1011+
const askOpen = extensionDialog?.method === "ask";
1012+
const hiddenComposerRef = useRef<HTMLDivElement>(null);
1013+
useEffect(() => {
1014+
// A hidden composer that keeps focus keeps the phone keyboard up: the
1015+
// first tap in the ask panel would then close it, resize the panel and
1016+
// miss its target. Let the keyboard go when the composer hides.
1017+
const active = document.activeElement;
1018+
if (askOpen && active instanceof HTMLElement && hiddenComposerRef.current?.contains(active)) active.blur();
1019+
}, [askOpen]);
10121020
const chatInputElement = (
10131021
<ChatInput
10141022
ref={chatInputRef}
@@ -1436,7 +1444,7 @@ export function ChatWindow({ session, newSessionCwd, newSessionWorkspace, toolCa
14361444
{/* An open ask is answered (or cancelled) in its panel; hiding the
14371445
composer leaves no doubt where to type. Kept mounted so the
14381446
draft, attachments and a recording survive. */}
1439-
<div style={{ display: extensionDialog?.method === "ask" ? "none" : "contents" }}>{chatInputElement}</div>
1447+
<div ref={hiddenComposerRef} style={{ display: askOpen ? "none" : "contents" }}>{chatInputElement}</div>
14401448
</div>
14411449
</>
14421450
)}

‎components/ExtensionDialog.mobile.test.mjs‎

Lines changed: 210 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ import test, { afterEach, beforeEach } from "node:test";
44
import React from "react";
55
import { readFile } from "node:fs/promises";
66
import { renderToStaticMarkup } from "react-dom/server";
7-
import { cleanup, fireEvent, render, screen } from "@testing-library/react/pure.js";
7+
import { act, cleanup, fireEvent, render, screen } from "@testing-library/react/pure.js";
88
import { createJiti } from "jiti";
99

1010
const jiti = createJiti(import.meta.url, { jsx: { runtime: "automatic" }, tsconfigPaths: true });
@@ -194,23 +194,144 @@ test("an open keyboard caps the panel to the visible area", () => {
194194
}
195195
});
196196

197-
test("the keyboard-only footer cap applies to every attached method", () => {
197+
/** Resizes the stubbed viewport like a browser does; `layout` also shrinks
198+
* `innerHeight`, as `interactive-widget: resizes-content` (Android Chrome)
199+
* and a desktop window or docked dev tools do. */
200+
function stubResizableViewport() {
201+
const saved = ["innerHeight", "innerWidth"].map((key) => [key, Object.getOwnPropertyDescriptor(window, key)]);
202+
const restoreViewport = stubVisualViewport(window.innerHeight);
203+
const set = (key, value) => Object.defineProperty(window, key, { configurable: true, value });
204+
return {
205+
full: window.innerHeight,
206+
resize(height, { layout = true, width } = {}) {
207+
if (width !== undefined) set("innerWidth", width);
208+
window.visualViewport.height = height;
209+
if (layout) set("innerHeight", height);
210+
act(() => { window.dispatchEvent(new window.Event("resize")); });
211+
},
212+
restore() {
213+
for (const [key, descriptor] of saved) {
214+
if (descriptor) Object.defineProperty(window, key, descriptor);
215+
else delete window[key];
216+
}
217+
restoreViewport();
218+
},
219+
};
220+
}
221+
222+
async function focusOther() {
223+
renderAttached();
224+
await new Promise((resolve) => requestAnimationFrame(resolve));
225+
const other = screen.getByRole("group", { name: /Pick a color/ }).querySelector("textarea");
226+
act(() => other.focus());
227+
return other;
228+
}
229+
230+
for (const [mode, layout] of [["resizes-content", true], ["overlay", false]]) {
231+
test(`collapsing the keyboard while typing in Other lets the text box go (${mode})`, async () => {
232+
const restoreMedia = stubMedia({ coarse: true });
233+
const viewport = stubResizableViewport();
234+
try {
235+
const other = await focusOther();
236+
viewport.resize(Math.round(viewport.full * 0.5), { layout });
237+
assert.ok(document.activeElement === other, "opening the keyboard keeps the text box");
238+
// Back button / swipe: focus would stay put, and the next tap (Submit)
239+
// would raise the keyboard again for a moment.
240+
viewport.resize(viewport.full, { layout });
241+
assert.ok(document.activeElement !== other, "a collapsed keyboard must not keep the text box focused");
242+
} finally {
243+
viewport.restore();
244+
restoreMedia();
245+
}
246+
});
247+
}
248+
249+
test("rotating with the keyboard up is no collapse, and the later collapse still lets the text box go", async () => {
250+
const restoreMedia = stubMedia({ coarse: true });
251+
const viewport = stubResizableViewport();
252+
try {
253+
const other = await focusOther();
254+
viewport.resize(Math.round(viewport.full * 0.5));
255+
// Landscape, keyboard still up: a new width, a shorter visible area.
256+
viewport.resize(Math.round(viewport.full * 0.3), { width: window.innerWidth + 300 });
257+
assert.ok(document.activeElement === other, "rotation must not take the text box away");
258+
// Back button in landscape: the visible area grows by the keyboard.
259+
viewport.resize(Math.round(viewport.full * 0.6));
260+
assert.ok(document.activeElement !== other, "the collapse after a rotation must still let the text box go");
261+
} finally {
262+
viewport.restore();
263+
restoreMedia();
264+
}
265+
});
266+
267+
test("on a desktop, shrinking and regrowing the window keeps the text box", async () => {
268+
// Docked dev tools opening and closing, or a window resized vertically.
269+
const viewport = stubResizableViewport();
270+
try {
271+
const other = await focusOther();
272+
fireEvent.pointerDown(other, { pointerType: "mouse" });
273+
viewport.resize(Math.round(viewport.full * 0.5));
274+
viewport.resize(viewport.full);
275+
assert.ok(document.activeElement === other, "no soft keyboard was involved");
276+
} finally {
277+
viewport.restore();
278+
}
279+
});
280+
281+
test("on a touchscreen laptop, a tap's keyboard is tracked though the mouse is the primary pointer", async () => {
282+
const viewport = stubResizableViewport();
283+
try {
284+
renderAttached();
285+
await new Promise((resolve) => requestAnimationFrame(resolve));
286+
const other = screen.getByRole("group", { name: /Pick a color/ }).querySelector("textarea");
287+
fireEvent.pointerDown(other, { pointerType: "touch" });
288+
act(() => other.focus());
289+
viewport.resize(Math.round(viewport.full * 0.5));
290+
assert.equal(screen.getByRole("dialog").style.maxHeight, `${Math.round(viewport.full * 0.5) - 12}px`, "the footer stays above the keyboard");
291+
viewport.resize(viewport.full);
292+
assert.ok(document.activeElement !== other, "a collapsed keyboard must not keep the text box focused");
293+
} finally {
294+
viewport.restore();
295+
}
296+
});
297+
298+
test("the keyboard cap applies to every attached method with a text field", () => {
198299
const viewportHeight = Math.round(window.innerHeight * 0.5);
199300
const restore = stubVisualViewport(viewportHeight);
200301
const restoreMedia = stubMedia({ coarse: true });
201302
try {
202-
const { responses } = renderAttached({ type: "extension_ui_request", id: "select-1", method: "select", title: "Which?", options: ["First", "Second"] });
203-
const button = screen.getByRole("button", { name: "First" });
204-
fireEvent.click(button);
205-
fireEvent.focus(button);
303+
renderAttached({ type: "extension_ui_request", id: "input-1", method: "input", title: "Name?" });
304+
fireEvent.focus(screen.getByRole("textbox", { name: "Name?" }));
206305
assert.equal(screen.getByRole("dialog").style.maxHeight, `${viewportHeight - 12}px`);
207-
assert.deepEqual(responses, [], "select still submits from the footer, not from focus");
208306
} finally {
209307
restoreMedia();
210308
restore();
211309
}
212310
});
213311

312+
test("on touch, focusing an option or button keeps the panel's height", () => {
313+
const restoreMedia = stubMedia({ coarse: true });
314+
try {
315+
const { view } = renderAttached();
316+
const panel = screen.getByRole("dialog");
317+
// No keyboard comes up for them, and a resize under the finger made the
318+
// tap that focused them miss.
319+
for (const control of [screen.getByRole("radio", { name: "Red" }), screen.getByRole("button", { name: "Submit" })]) {
320+
fireEvent.focus(control);
321+
assert.equal(panel.style.maxHeight, "min(420px, 60dvh)");
322+
}
323+
view.rerender(React.createElement(ExtensionDialog, {
324+
request: { type: "extension_ui_request", id: "select-1", method: "select", title: "Which?", options: ["First", "Second"] },
325+
onRespond: () => {},
326+
attached: true,
327+
}));
328+
fireEvent.focus(screen.getByRole("button", { name: "First" }));
329+
assert.equal(screen.getByRole("dialog").style.maxHeight, "min(420px, 60dvh)");
330+
} finally {
331+
restoreMedia();
332+
}
333+
});
334+
214335
test("without a visible-area API the panel keeps its CSS cap", () => {
215336
const { responses } = renderAttached();
216337
const panel = screen.getByRole("dialog");
@@ -223,15 +344,91 @@ test("without a visible-area API the panel keeps its CSS cap", () => {
223344
assert.deepEqual(responses, [{ answers: [{ id: "color", selectedOptions: [], customInput: "Teal" }] }]);
224345
});
225346

226-
test("a footer press leaves focus in the Other text box", () => {
227-
renderAttached();
228-
// A focus change on press closes the phone keyboard and drops the panel's
229-
// keyboard cap before the click, so the click misses the moved button.
230-
for (const name of ["Cancel", "Submit"]) {
231-
assert.equal(fireEvent.mouseDown(screen.getByRole("button", { name })), false, `${name} must not take focus`);
347+
/** A press as the browser sends it: pointerdown, then the compat mousedown.
348+
* Returns whether mousedown's default (moving focus) still runs. */
349+
function press(target, pointerType) {
350+
fireEvent.pointerDown(target, { pointerType });
351+
return fireEvent.mouseDown(target);
352+
}
353+
354+
test("on touch, while Other is being typed, a press elsewhere in the panel keeps focus there", async () => {
355+
// A phone: the panel leaves focus alone until tapped (#235).
356+
const restore = stubMedia({ coarse: true });
357+
const composer = document.body.appendChild(document.createElement("textarea"));
358+
try {
359+
renderAttached();
360+
await new Promise((resolve) => requestAnimationFrame(resolve));
361+
const other = screen.getByRole("group", { name: /Pick a color/ }).querySelector("textarea");
362+
const red = screen.getByRole("radio", { name: "Red" });
363+
const targets = [red, red.closest("label"), screen.getByRole("button", { name: "Cancel" }), screen.getByRole("button", { name: "Submit" })];
364+
// No text box of the panel focused: no keyboard of ours to protect.
365+
for (const target of targets) assert.equal(press(target, "touch"), true);
366+
act(() => composer.focus());
367+
for (const target of targets) assert.equal(press(target, "touch"), true, "a field outside the panel is not ours to keep");
368+
act(() => red.focus());
369+
assert.equal(press(screen.getByRole("button", { name: "Submit" }), "touch"), true, "a focused radio raises no keyboard");
370+
371+
act(() => other.focus());
372+
// A focus change on press closes the phone keyboard and resizes the panel
373+
// before the click, so the click misses the option or button it aimed at.
374+
for (const pointerType of ["touch", "pen"]) {
375+
for (const target of targets) assert.equal(press(target, pointerType), false, `${target.textContent} must not take focus (${pointerType})`);
376+
}
377+
assert.equal(press(other, "touch"), true, "the text box itself still takes the press");
378+
} finally {
379+
composer.remove();
380+
restore();
381+
}
382+
});
383+
384+
test("a mouse press in the panel moves focus as usual, even on a touch-first device", () => {
385+
// The pointer media query describes the primary pointer only; a hybrid
386+
// device's mouse must keep text selection and radio arrow keys.
387+
const restore = stubMedia({ coarse: true });
388+
try {
389+
renderAttached();
390+
act(() => screen.getByRole("group", { name: /Pick a color/ }).querySelector("textarea").focus());
391+
assert.equal(press(screen.getByText("Red"), "mouse"), true);
392+
} finally {
393+
restore();
232394
}
233395
});
234396

397+
test("choosing an option takes focus from Other, so later typing cannot pick Other", () => {
398+
renderAttached();
399+
const other = screen.getByRole("group", { name: /Pick a color/ }).querySelector("textarea");
400+
const red = screen.getByRole("radio", { name: "Red" });
401+
act(() => other.focus());
402+
// A touch press on the radio itself keeps focus in Other; the change must move it.
403+
assert.equal(press(red, "touch"), false);
404+
fireEvent.click(red);
405+
// Compare identities: a failing element diff walks the whole jsdom graph.
406+
assert.ok(document.activeElement === red, "the chosen radio has focus");
407+
});
408+
409+
test("answering releases the Other text box at once, so the keyboard does not wait for the server", () => {
410+
const { responses } = renderAttached();
411+
const other = screen.getByRole("group", { name: /Pick a color/ }).querySelector("textarea");
412+
act(() => other.focus());
413+
fireEvent.change(other, { target: { value: "Teal" } });
414+
// The panel stays mounted until omp acknowledges the answer (plus a hand-off delay).
415+
fireEvent.click(screen.getByRole("button", { name: "Submit" }));
416+
assert.equal(responses.length, 1);
417+
assert.ok(document.activeElement !== other, "the Other text box must not keep focus");
418+
});
419+
420+
test("tapping back into kept Other text chooses it even when focus never left", () => {
421+
renderAttached();
422+
const other = screen.getByRole("group", { name: /Pick a color/ }).querySelector("textarea");
423+
fireEvent.change(other, { target: { value: "Teal" } });
424+
fireEvent.click(screen.getByRole("radio", { name: "Red" }));
425+
assert.equal(screen.getByRole("radio", { name: /Other/ }).checked, false);
426+
// A tap that finds focus already in the box fires no focus event.
427+
fireEvent.click(other);
428+
assert.equal(screen.getByRole("radio", { name: /Other/ }).checked, true);
429+
assert.equal(screen.getByRole("radio", { name: "Red" }).checked, false);
430+
});
431+
235432
// ---- touch targets (globals.css) ----------------------------------------------
236433

237434
function ruleBody(selector) {

0 commit comments

Comments
 (0)