From 95b3e3fd80ed8dc7f129fb3bce5334c244c4bbbc Mon Sep 17 00:00:00 2001 From: Bargan Constantin Date: Sun, 4 Oct 2026 22:37:54 +0300 Subject: [PATCH 1/5] fix(feedback): a selection let go over the scrim keeps the dialog open A click goes to the nearest element its press and its release share, so dragging a selection out of the message and letting go over the scrim reached the backdrop as a click of its own and closed the dialog, with the message, the contact and the screenshots. The backdrop now closes only on a press that went down on the scrim and came up there, through a useScrimDismiss helper any modal can spread. --- src/web/__tests__/feedback-images.test.ts | 2 +- .../__tests__/feedback-scrim-dismiss.test.ts | 83 +++++++++++++++++++ src/web/components/FeedbackDialog.tsx | 7 +- src/web/components/use-modal-dismiss.ts | 30 +++++++ 4 files changed, 119 insertions(+), 3 deletions(-) create mode 100644 src/web/__tests__/feedback-scrim-dismiss.test.ts diff --git a/src/web/__tests__/feedback-images.test.ts b/src/web/__tests__/feedback-images.test.ts index 762cc7baf..fbb10585b 100644 --- a/src/web/__tests__/feedback-images.test.ts +++ b/src/web/__tests__/feedback-images.test.ts @@ -437,7 +437,7 @@ describe("the dialog takes an image three ways", () => { it("keeps a file dropped beside the dialog from opening in place of the deck", () => { // The browser's own answer to a file dropped on a page is to open it, // which would throw away everything typed. - expect(flatDialog).toMatch(/
/); + expect(flatDialog).toMatch(/
/); expect(flatDialog).toMatch(/function refuseBesideDialog\(e: DragEvent\) \{ if \(e\.target !== e\.currentTarget \|\| !fileDrag\(e\)\) return; e\.preventDefault\(\); e\.dataTransfer\.dropEffect = "none"; \}/); expect(flatDialog).toMatch(/function dropBesideDialog\(e: DragEvent\) \{ if \(fileDrag\(e\)\) e\.preventDefault\(\); \}/); }); diff --git a/src/web/__tests__/feedback-scrim-dismiss.test.ts b/src/web/__tests__/feedback-scrim-dismiss.test.ts new file mode 100644 index 000000000..74a5c892a --- /dev/null +++ b/src/web/__tests__/feedback-scrim-dismiss.test.ts @@ -0,0 +1,83 @@ +// The feedback dialog closed under a text selection and took the report with +// it. A click goes to the nearest element its press and its release share, so +// a drag that selects part of the message and is let go past the dialog's +// edge, over the scrim, arrives as a click whose target is the backdrop — the +// dialog's own stopPropagation is not on that path — and the backdrop closed +// on any click. The message, the contact and every screenshot were state in +// the dialog, and went with it. +// +// Only a press that starts on the scrim and ends there closes it now. Run on +// fake-react.ts's React: the backdrop's handlers are called with the targets +// the browser would hand them. +import { beforeEach, afterEach, describe, expect, it, vi } from "vitest"; + +import { mount, one, type Drawn } from "./fake-react"; + +vi.mock("react", async () => (await import("./fake-react")).react); +vi.mock("../components/use-modal-dismiss", async orig => ({ + ...(await orig()), + useModalDismiss: () => ({ current: null }), +})); +// Built with forwardRef when it loads; the thanks is not what is under test. +vi.mock("../components/SuccessMark", () => ({ default: () => null })); + +const { default: FeedbackDialog } = await import("../components/FeedbackDialog"); + +beforeEach(() => { + // The facts the dialog asks for on open; never answered, which it allows. + vi.stubGlobal("fetch", vi.fn(() => new Promise(() => {}))); +}); +afterEach(() => { vi.unstubAllGlobals(); }); + +const scrim = (tree: unknown) => one(tree, el => el.props.className === "modal-backdrop") as Drawn; + +/** Stand-ins for the nodes a press can land on: the scrim itself, and the + * message field inside the dialog. */ +const SCRIM = { node: "scrim" }; +const FIELD = { node: "message field" }; + +/** One mouse press, as the backdrop sees it: down on one node, up on + * another, and the click the browser then sends to the nearest node the two + * share — the scrim whenever either end was on it, since the dialog is + * inside it. Handlers the backdrop does not have are skipped. */ +function press(tree: unknown, down: object, up: object) { + const call = (name: string, target: object) => + (scrim(tree).props[name] as ((e: unknown) => void) | undefined)?.({ + target, currentTarget: SCRIM, stopPropagation() {}, preventDefault() {}, + }); + call("onPointerDown", down); + call("onPointerUp", up); + call("onClick", down === FIELD && up === FIELD ? FIELD : SCRIM); +} + +describe("the feedback dialog's scrim", () => { + it("stays open when a selection begun in the message is let go over the scrim", () => { + const onClose = vi.fn(); + const view = mount(FeedbackDialog, { onClose }); + press(view.tree, FIELD, SCRIM); + expect(onClose).not.toHaveBeenCalled(); + }); + + it("stays open when a press begun on the scrim is let go inside the dialog", () => { + // Moving off a control before letting go is how a press is taken back. + const onClose = vi.fn(); + const view = mount(FeedbackDialog, { onClose }); + press(view.tree, SCRIM, FIELD); + expect(onClose).not.toHaveBeenCalled(); + }); + + it("closes on a press that starts and ends on the scrim", () => { + const onClose = vi.fn(); + const view = mount(FeedbackDialog, { onClose }); + press(view.tree, SCRIM, SCRIM); + expect(onClose).toHaveBeenCalledTimes(1); + }); + + it("closes on the next scrim press after a selection that did not", () => { + const onClose = vi.fn(); + const view = mount(FeedbackDialog, { onClose }); + press(view.tree, FIELD, SCRIM); + press(view.tree, SCRIM, SCRIM); + expect(onClose).toHaveBeenCalledTimes(1); + }); +}); diff --git a/src/web/components/FeedbackDialog.tsx b/src/web/components/FeedbackDialog.tsx index 9729cc844..34543ed2d 100644 --- a/src/web/components/FeedbackDialog.tsx +++ b/src/web/components/FeedbackDialog.tsx @@ -31,7 +31,7 @@ // through the picker. What is checked and redrawn before it goes is // feedback-images.ts's. With no image, Send posts the JSON it always did. import { useEffect, useRef, useState, type ClipboardEvent, type DragEvent, type FormEvent } from "react"; -import { useModalDismiss } from "./use-modal-dismiss"; +import { useModalDismiss, useScrimDismiss } from "./use-modal-dismiss"; import { focusDropped, selfPressProps } from "../panel-press"; import SuccessMark from "./SuccessMark"; import FeedbackKinds from "./FeedbackKinds"; @@ -63,6 +63,9 @@ export default function FeedbackDialog({ onClose, initialKind, initialBody }: Pr const formRef = useRef(null); const dragDepth = useRef(0); const dialogRef = useModalDismiss(onClose, { focusRef: bodyRef }); + // Only a press of the scrim itself: a selection dragged out of the message + // and let go over it would otherwise close the dialog and lose the report. + const scrimPress = useScrimDismiss(onClose); const [kind, setKind] = useState(initialKind ?? "bug"); const [body, setBody] = useState(initialBody ?? ""); const [contact, setContact] = useState(""); @@ -152,7 +155,7 @@ export default function FeedbackDialog({ onClose, initialKind, initialBody }: Pr } return ( -
( return dialogRef; } + +/** Where a press on a backdrop landed, read structurally so a test can hand + * in plain objects; React's pointer and mouse events both satisfy it. */ +interface ScrimPress { + target: EventTarget | null; + currentTarget: EventTarget | null; +} + +/** The handlers a backdrop spreads to close its dialog on a press of the + * scrim — one that goes down on the scrim itself and comes up there. + * + * A click goes to the nearest element its press and its release share. A + * selection begun in a field and let go past the dialog's edge therefore + * arrives as a click whose target is the backdrop, and the dialog's own + * stopPropagation is not on that path: a backdrop closing on any click + * closed the feedback dialog under a drag-select and threw away the message + * and its screenshots. So the press is followed from the pointer going down + * to it coming up, and the click closes only when both were on the scrim. */ +export function useScrimDismiss(onDismiss: () => void) { + const fromScrim = useRef(false); + return { + onPointerDown: (e: ScrimPress) => { fromScrim.current = e.target === e.currentTarget; }, + onPointerUp: (e: ScrimPress) => { if (e.target !== e.currentTarget) fromScrim.current = false; }, + onClick: (e: ScrimPress) => { + const close = fromScrim.current && e.target === e.currentTarget; + fromScrim.current = false; + if (close) onDismiss(); + }, + }; +} From afad7a72c6e4f2b057e42aa78280b4cee1d6d9cf Mon Sep 17 00:00:00 2001 From: Bargan Constantin Date: Sun, 4 Oct 2026 22:38:26 +0300 Subject: [PATCH 2/5] fix(feedback): Enter on Bug, Idea or Other no longer sends the report The kinds are radios inside the form, and a browser submits a form on a bare Enter in a radio, so choosing a kind with the arrows and pressing Enter sent the report before a screenshot or a contact could be added. A bare Enter on a kind now does nothing, as in the contact field; Send and the shortcut stay the only ways a report leaves. --- src/web/__tests__/feedback-kind-enter.test.ts | 47 +++++++++++++++++++ src/web/components/FeedbackKinds.tsx | 8 +++- 2 files changed, 53 insertions(+), 2 deletions(-) create mode 100644 src/web/__tests__/feedback-kind-enter.test.ts diff --git a/src/web/__tests__/feedback-kind-enter.test.ts b/src/web/__tests__/feedback-kind-enter.test.ts new file mode 100644 index 000000000..4ee4b521c --- /dev/null +++ b/src/web/__tests__/feedback-kind-enter.test.ts @@ -0,0 +1,47 @@ +// Enter on Bug, Idea or Other sent the report. The kinds are native radios +// inside the form, and a browser submits a form implicitly on Enter in a +// radio, so picking a kind with the arrows and pressing Enter to settle on it +// posted the message there and then, before a screenshot or a contact could +// be added. Send and the shortcut are the only two ways a report leaves; the +// contact field already kept its bare Enter for that reason, and the kinds now +// do the same. +// +// FeedbackKinds has no hooks, so it is called as it is and its radios' key +// handlers are pressed with the keystrokes a browser would hand them. +import { describe, expect, it, vi } from "vitest"; + +import { all, type Drawn } from "./fake-react"; +import FeedbackKinds from "../components/FeedbackKinds"; +import { KINDS, type EnterKey } from "../feedback"; + +const radios = () => all(FeedbackKinds({ kind: "bug", onChange: () => {} }), el => el.type === "input" && el.props.type === "radio"); + +const key = (over: Partial = {}): EnterKey => + ({ key: "Enter", metaKey: false, ctrlKey: false, altKey: false, shiftKey: false, ...over }); + +/** Presses `k` on `radio`; answers whether the browser's own answer — the + * implicit submit — was stopped. */ +function press(radio: Drawn, k: EnterKey): boolean { + const preventDefault = vi.fn(); + (radio.props.onKeyDown as ((e: unknown) => void) | undefined)?.({ ...k, nativeEvent: k, preventDefault }); + return preventDefault.mock.calls.length > 0; +} + +describe("Enter on a kind of feedback", () => { + it("draws one radio per kind", () => { + expect(radios().map(r => r.props.value)).toEqual(KINDS.map(k => k.value)); + }); + + it("does not send the report from any of them", () => { + for (const radio of radios()) expect(press(radio, key()), String(radio.props.value)).toBe(true); + }); + + it("leaves the send shortcut to the form, and an input method's Enter alone", () => { + for (const radio of radios()) { + expect(press(radio, key({ ctrlKey: true }))).toBe(false); + expect(press(radio, key({ metaKey: true }))).toBe(false); + expect(press(radio, key({ isComposing: true }))).toBe(false); + expect(press(radio, key({ key: "ArrowRight" }))).toBe(false); + } + }); +}); diff --git a/src/web/components/FeedbackKinds.tsx b/src/web/components/FeedbackKinds.tsx index f8f145a91..0f2f19bea 100644 --- a/src/web/components/FeedbackKinds.tsx +++ b/src/web/components/FeedbackKinds.tsx @@ -6,8 +6,11 @@ // difference matters. // // Native radios under the paint, so the arrows walk them, Tab stops once and a -// screen reader hears a group of three; each