Feedback dialog: keep the draft on a stray release, lock it while sending, and cancel on close

src/web/__tests__/fake-react.ts

@@ -111,10 +111,11 @@ export function mount<P>(
   let tree: unknown = null;
   let shown = props;
   let running = false;
+  let gone = false;
   const inst: Instance = {
     slots: [], effects: [], queued: [], cursor: 0, effectCursor: 0, dirty: false,
     update() {
-      if (running) return;
+      if (running || gone) return;
       running = true;
       try {
         let passes = 0;
@@ -144,6 +145,12 @@ export function mount<P>(
   return {
     get tree() { return tree; },
     rerender(next: P = shown) { shown = next; inst.update(); },
+    /** Takes the component away, the way a dialog closing does: every
+     *  effect's cleanup runs, and a state set afterwards draws nothing. */
+    unmount() {
+      gone = true;
+      for (const e of inst.effects) if (typeof e?.cleanup === "function") e.cleanup();
+    },
   };
 }
 

src/web/__tests__/feedback-dialog-form.test.ts

@@ -297,7 +297,7 @@ describe("Send feedback is never greyed out, and never drops focus (#518)", () =
   });
 
   it("says aria-busy while the request is out, and refuses a second press itself", () => {
-    expect(flatDialog).toMatch(/<button type="submit" className="btn primary fb-send"[^>]*\{\.\.\.selfPressProps\(sending\)\}/);
+    expect(flatDialog).toMatch(/<button ref=\{sendRef\} type="submit" className="btn primary fb-send"[^>]*\{\.\.\.selfPressProps\(sending\)\}/);
     expect(sendHook).toMatch(/if \(!selfPressAccepted\(sendingRef\.current\)\) return;/);
     expect(flatDialog).toMatch(/<span className="fb-send-busy">Sending…<\/span>/);
   });

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(/<div className="modal-backdrop" onClick=\{onClose\} role="presentation" onDragOver=\{refuseBesideDialog\} onDrop=\{dropBesideDialog\} data-leaving=\{leaving \|\| undefined\}>/);
+    expect(flatDialog).toMatch(/<div className="modal-backdrop" \{\.\.\.scrimPress\} role="presentation" onDragOver=\{refuseBesideDialog\} onDrop=\{dropBesideDialog\} data-leaving=\{leaving \|\| undefined\}>/);
     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\(\); \}/);
   });

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> = {}): 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);
+    }
+  });
+});

src/web/__tests__/feedback-refused-while-sending.test.ts

@@ -0,0 +1,113 @@
+// An image refused while Send waited for it was dropped, and the report went
+// without it. Send waits for every image still being redrawn to fit; one that
+// then turns out not to decode, or not to shrink far enough, is taken off the
+// strip and refused beside it — and the send posted what was left, said
+// "Thanks — feedback sent." over the refusal and closed the dialog, so the
+// person never learnt their screenshot had not gone. A refusal said while Send
+// waits now stops the send, and the dialog stays open saying why.
+//
+// Run on fake-react.ts's React: the images hook and the send hook together,
+// as the dialog holds them, with the fit of each image answered by the test.
+import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
+
+import { flush, mount } from "./fake-react";
+import type { FeedbackDraft } from "../feedback";
+import type { Prepared } from "../feedback-images";
+
+vi.mock("react", async () => (await import("./fake-react")).react);
+
+/** Each fit under way, answered when the test says. The decoder is called
+ *  first, which is the moment the hook shows the image as resizing. */
+let fits: Array<(prepared: Prepared) => void>;
+vi.mock("../feedback-images", async orig => ({
+  ...(await orig<typeof import("../feedback-images")>()),
+  browserDecoder: async () => null,
+  prepareImage: async (file: Blob, _budget: number, decode: (file: Blob) => Promise<unknown>) => {
+    await decode(file);
+    return new Promise<Prepared>(resolve => fits.push(resolve));
+  },
+}));
+
+const { useFeedbackImages } = await import("../use-feedback-images");
+const { useFeedbackSend } = await import("../use-feedback-send");
+
+const DRAFT: FeedbackDraft = { kind: "bug", body: "Here is what the canvas looked like", contact: "" };
+const png = (name: string) => new File([new Uint8Array(64)], name, { type: "image/png" });
+const FITTED: Prepared = { ok: true, blob: new Blob([new Uint8Array(32)], { type: "image/png" }), resized: { width: 8000, height: 6000 } };
+const DAMAGED: Prepared = { ok: false, problem: "unreadable" };
+
+let posts: Array<RequestInit>;
+
+beforeEach(() => {
+  fits = [];
+  posts = [];
+  vi.stubGlobal("fetch", vi.fn(async (_url: string, init: RequestInit) => {
+    posts.push(init);
+    return { ok: true, status: 200, json: async () => ({ ok: true }) };
+  }));
+});
+afterEach(() => { vi.unstubAllGlobals(); });
+
+function dialog() {
+  return mount(() => {
+    const images = useFeedbackImages();
+    return { images, ...useFeedbackSend(images) };
+  }, {});
+}
+
+/** One image pasted, and its fit begun: shown, still resizing. */
+async function pasteBig(view: ReturnType<typeof dialog>, name: string) {
+  view.tree.images.add([png(name)]);
+  await flush();
+}
+
+describe("an image refused while Send waits for it", () => {
+  it("stops the send, and says why, with the refusal still beside the images", async () => {
+    const view = dialog();
+    await pasteBig(view, "damaged.png");
+    expect(view.tree.images.shots).toHaveLength(1);
+    const sent = view.tree.send(DRAFT);
+    fits[0](DAMAGED);
+    await sent;
+    expect(posts).toHaveLength(0);
+    expect(view.tree.outcome.state).toBe("failed");
+    expect(view.tree.outcome.state === "failed" && view.tree.outcome.message).toMatch(/image/i);
+    expect(view.tree.images.problem).toMatch(/“damaged\.png” could not be read as an image/);
+    expect(view.tree.images.shots).toHaveLength(0);
+  });
+
+  it("sends once it is pressed again, with what is there now", async () => {
+    const view = dialog();
+    await pasteBig(view, "damaged.png");
+    const first = view.tree.send(DRAFT);
+    fits[0](DAMAGED);
+    await first;
+    await view.tree.send(DRAFT);
+    expect(posts).toHaveLength(1);
+    expect(view.tree.outcome).toEqual({ state: "sent" });
+  });
+
+  it("does not stop for a refusal already said before Send was pressed", async () => {
+    // That one was on screen when the person chose to send.
+    const view = dialog();
+    await pasteBig(view, "damaged.png");
+    fits[0](DAMAGED);
+    await flush();
+    expect(view.tree.images.problem).not.toBe("");
+    await view.tree.send(DRAFT);
+    expect(posts).toHaveLength(1);
+    expect(view.tree.outcome).toEqual({ state: "sent" });
+  });
+
+  it("sends an image that fits while Send waits for it", async () => {
+    const view = dialog();
+    await pasteBig(view, "canvas.png");
+    const sent = view.tree.send(DRAFT);
+    fits[0](FITTED);
+    await sent;
+    expect(posts).toHaveLength(1);
+    expect(posts[0].body).toBeInstanceOf(FormData);
+    expect((posts[0].body as FormData).getAll("images")).toHaveLength(1);
+    expect(view.tree.outcome).toEqual({ state: "sent" });
+  });
+});

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<typeof import("../components/use-modal-dismiss")>()),
+  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);
+  });
+});

src/web/__tests__/feedback-send-cancel.test.ts

@@ -0,0 +1,84 @@
+// Cancel, Esc or the × pressed while a report said "Sending…" did not cancel
+// it. Closing only took the dialog away; the send went on awaiting the images
+// and then posted, so a report the person had just cancelled — screenshots
+// and all — could start uploading after the dialog was gone, and a failure of
+// that post was told to nobody. Closed now, a send not yet posted is never
+// posted and one under way is aborted.
+//
+// Run on fake-react.ts's React: the hook is mounted, the images it waits for
+// and the fetch it makes are held by the test, and unmount is the dialog
+// closing.
+import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
+
+import { flush, mount } from "./fake-react";
+import type { FeedbackImages } from "../use-feedback-images";
+import type { FeedbackDraft } from "../feedback";
+
+vi.mock("react", async () => (await import("./fake-react")).react);
+
+const { useFeedbackSend } = await import("../use-feedback-send");
+
+const DRAFT: FeedbackDraft = { kind: "bug", body: "The usage chart is empty after a restart", contact: "" };
+
+/** Images whose fits finish when the test says. */
+let finishFits: (blobs: Blob[]) => void;
+const images = { ready: () => new Promise<Blob[]>(resolve => { finishFits = resolve; }) } as unknown as FeedbackImages;
+
+/** Every post made, with what it was handed. Each one is answered only when
+ *  the test says, and fails the way fetch does when its signal is aborted. */
+let posts: Array<{ init: RequestInit; answer: (ok: boolean) => void }>;
+
+beforeEach(() => {
+  posts = [];
+  vi.stubGlobal("fetch", vi.fn((_url: string, init: RequestInit) => new Promise((resolve, reject) => {
+    init.signal?.addEventListener("abort", () => reject(new DOMException("The operation was aborted.", "AbortError")));
+    posts.push({ init, answer: ok => resolve({ ok, status: ok ? 200 : 502, json: async () => ({ ok }) }) });
+  })));
+});
+afterEach(() => { vi.unstubAllGlobals(); });
+
+function open() {
+  return mount(() => useFeedbackSend(images), {});
+}
+
+describe("closing the feedback dialog while a report is sending", () => {
+  it("posts nothing when it closes while the images are still being made ready", async () => {
+    const view = open();
+    void view.tree.send(DRAFT);
+    view.unmount();
+    finishFits([]);
+    await flush();
+    expect(posts).toHaveLength(0);
+  });
+
+  it("aborts the post when it closes while the post is out", async () => {
+    const view = open();
+    void view.tree.send(DRAFT);
+    finishFits([]);
+    await flush();
+    expect(posts).toHaveLength(1);
+    view.unmount();
+    expect(posts[0].init.signal?.aborted).toBe(true);
+  });
+
+  it("still sends, and thanks, when nobody closes it", async () => {
+    const view = open();
+    const sent = view.tree.send(DRAFT);
+    finishFits([]);
+    await flush();
+    expect(posts).toHaveLength(1);
+    posts[0].answer(true);
+    await sent;
+    expect(view.tree.outcome).toEqual({ state: "sent" });
+  });
+
+  it("still says a failure that was not a close", async () => {
+    const view = open();
+    const sent = view.tree.send(DRAFT);
+    finishFits([]);
+    await flush();
+    posts[0].answer(false);
+    await sent;
+    expect(view.tree.outcome.state).toBe("failed");
+  });
+});