Merge pull request #188 from Coffey-Labs/fix/ctrl-click-selection

Take the message you were reading into the selection

web/src/lib/__tests__/listSelection.test.ts

@@ -0,0 +1,104 @@
+import { describe, expect, it } from "vitest";
+import { rowClick, type RowClick } from "@/lib/listSelection";
+
+const IDS = ["a", "b", "c", "d", "e"];
+const click = (over: Partial<Parameters<typeof rowClick>[0]> = {}): RowClick =>
+  rowClick({
+    rowId: "c", ids: IDS, anchor: null, selected: {},
+    modifiers: { shift: false, ctrl: false }, isMobile: false,
+    ...over,
+  });
+
+describe("a plain click", () => {
+  it("opens the message rather than selecting it", () => {
+    expect(click()).toEqual({ kind: "open" });
+  });
+
+  it("opens it even when another message is already open", () => {
+    expect(click({ anchor: "a" })).toEqual({ kind: "open" });
+  });
+
+  it("goes on selecting on a touchscreen once a selection exists", () => {
+    // There is no modifier to hold on a phone, and opening a message in the
+    // middle of picking several is almost never what the tap meant.
+    expect(click({ isMobile: true, selected: { a: true } })).toEqual({ kind: "select", ids: ["c"], on: true, moveAnchor: true });
+  });
+
+  it("still opens on a touchscreen when nothing is selected", () => {
+    expect(click({ isMobile: true })).toEqual({ kind: "open" });
+  });
+});
+
+describe("ctrl-clicking", () => {
+  it("takes the message that was already current with it", () => {
+    // Issue #186: this used to select only the row clicked, leaving the open
+    // message highlighted but unticked, so actions applied to one of two.
+    expect(click({ anchor: "a", modifiers: { shift: false, ctrl: true } }))
+      .toEqual({ kind: "select", ids: ["a", "c"], on: true, moveAnchor: true });
+  });
+
+  it("toggles one row once there is a selection, and leaves the rest alone", () => {
+    expect(click({ anchor: "a", selected: { a: true, c: true }, modifiers: { shift: false, ctrl: true } }))
+      .toEqual({ kind: "select", ids: ["c"], on: false, moveAnchor: true });
+    expect(click({ anchor: "a", selected: { a: true }, modifiers: { shift: false, ctrl: true } }))
+      .toEqual({ kind: "select", ids: ["c"], on: true, moveAnchor: true });
+  });
+
+  it("selects just the row when there is nothing current to bring along", () => {
+    expect(click({ anchor: null, modifiers: { shift: false, ctrl: true } }))
+      .toEqual({ kind: "select", ids: ["c"], on: true, moveAnchor: true });
+  });
+
+  it("does not bring along a row that has scrolled out of the list", () => {
+    // The anchor can name a message from a folder that is no longer shown.
+    expect(click({ anchor: "gone", modifiers: { shift: false, ctrl: true } }))
+      .toEqual({ kind: "select", ids: ["c"], on: true, moveAnchor: true });
+  });
+
+  it("does not pair a row with itself", () => {
+    expect(click({ rowId: "a", anchor: "a", modifiers: { shift: false, ctrl: true } }))
+      .toEqual({ kind: "select", ids: ["a"], on: true, moveAnchor: true });
+  });
+});
+
+describe("shift-clicking", () => {
+  it("takes the whole run, including the row it started from", () => {
+    expect(click({ rowId: "d", anchor: "b", modifiers: { shift: true, ctrl: false } }))
+      .toEqual({ kind: "select", ids: ["b", "c", "d"], on: true, moveAnchor: false });
+  });
+
+  it("works the same way backwards", () => {
+    expect(click({ rowId: "b", anchor: "d", modifiers: { shift: true, ctrl: false } }))
+      .toEqual({ kind: "select", ids: ["b", "c", "d"], on: true, moveAnchor: false });
+  });
+
+  it("leaves the anchor where it is, so the range grows from one place", () => {
+    const first = click({ rowId: "c", anchor: "a", modifiers: { shift: true, ctrl: false } });
+    expect(first).toMatchObject({ moveAnchor: false });
+    // Extending again still starts at "a" rather than at "c".
+    expect(click({ rowId: "e", anchor: "a", modifiers: { shift: true, ctrl: false } }))
+      .toMatchObject({ ids: ["a", "b", "c", "d", "e"] });
+  });
+
+  it("falls back to opening when there is nothing to extend from", () => {
+    expect(click({ anchor: null, modifiers: { shift: true, ctrl: false } })).toEqual({ kind: "open" });
+  });
+
+  it("falls back when the anchor is no longer in the list", () => {
+    expect(click({ anchor: "gone", modifiers: { shift: true, ctrl: false } })).toEqual({ kind: "open" });
+  });
+});
+
+describe("the two rules agree with each other", () => {
+  it("both include the row the selection started from", () => {
+    // The bug was that only one of them did. Whatever else changes, a modifier
+    // click that begins a selection has to contain the anchor.
+    const withCtrl = click({ rowId: "d", anchor: "b", modifiers: { shift: false, ctrl: true } });
+    const withShift = click({ rowId: "d", anchor: "b", modifiers: { shift: true, ctrl: false } });
+    for (const result of [withCtrl, withShift]) {
+      expect(result.kind, JSON.stringify(result)).toBe("select");
+      expect((result as { ids: string[] }).ids).toContain("b");
+      expect((result as { ids: string[] }).ids).toContain("d");
+    }
+  });
+});

web/src/lib/listSelection.ts

@@ -0,0 +1,70 @@
+import type { Id } from "@/jmap/types";
+
+/**
+ * What a click on a message row means.
+ *
+ * Lifted out of the list so the rules sit together and can be tested. They had
+ * drifted apart while they were two branches of one handler: shift-click
+ * selected the whole range including the row it started from, and ctrl-click
+ * selected only the row clicked, leaving the message you had open highlighted
+ * but unticked. Both looked picked; one was. That is issue #186, and the reason
+ * this is a function rather than a comment asking the next person to be careful.
+ */
+
+export type RowClick =
+  | { kind: "open" }
+  | { kind: "select"; ids: Id[]; on: boolean; moveAnchor: boolean };
+
+export function rowClick(opts: {
+  /** The row clicked. */
+  rowId: Id;
+  /** Every row on screen, in the order they are shown. */
+  ids: Id[];
+  /** The row a range would extend from: the last one clicked without shift. */
+  anchor: Id | null;
+  selected: Record<Id, boolean>;
+  modifiers: { shift: boolean; ctrl: boolean };
+  isMobile: boolean;
+}): RowClick {
+  const { rowId, ids, anchor, selected, modifiers, isMobile } = opts;
+  const selectedCount = Object.keys(selected).length;
+
+  // A range, from the anchor to here, inclusive at both ends.
+  if (modifiers.shift && anchor) {
+    const from = ids.indexOf(anchor);
+    const to = ids.indexOf(rowId);
+    if (from >= 0 && to >= 0) {
+      const [start, end] = from < to ? [from, to] : [to, from];
+      // The anchor stays where it is, so extending the range again grows it
+      // from the same place rather than from wherever it last reached.
+      return { kind: "select", ids: ids.slice(start, end + 1), on: true, moveAnchor: false };
+    }
+  }
+
+  if (modifiers.ctrl) {
+    /*
+     * The row that was already current joins the selection.
+     *
+     * Opening a message does not select it -- it is highlighted because it is
+     * the one being read, which is a different state -- so picking a second one
+     * with ctrl used to select only the second, and every action that followed
+     * quietly applied to half of what the screen showed.
+     *
+     * Only while nothing is selected yet. Once there is a selection, ctrl-click
+     * toggles exactly one row, which is the whole point of it.
+     */
+    if (!selectedCount && anchor && anchor !== rowId && ids.includes(anchor)) {
+      return { kind: "select", ids: [anchor, rowId], on: true, moveAnchor: true };
+    }
+    return { kind: "select", ids: [rowId], on: !selected[rowId], moveAnchor: true };
+  }
+
+  // On a touchscreen, once anything is selected a plain tap goes on selecting:
+  // there is no modifier to hold, and opening a message mid-selection is almost
+  // never what the tap meant.
+  if (selectedCount > 0 && isMobile) {
+    return { kind: "select", ids: [rowId], on: !selected[rowId], moveAnchor: true };
+  }
+
+  return { kind: "open" };
+}

web/src/views/mail/MessageList.tsx

@@ -9,6 +9,7 @@ import { formatListDate } from "@/lib/format";
 import { canEmpty, confirmAndEmpty, emptyLabel } from "@/lib/emptyFolder";
 import { displayName, shortName } from "@/lib/address";
 import { Avatar, Empty, useIsMobile, useIsTouch } from "@/ui/misc";
+import { rowClick } from "@/lib/listSelection";
 import { MenuItem, MenuSep, MenuTitle, Popover, useMenu } from "@/ui/popover";
 import { useCompose } from "@/store/compose";
 import { useCalendar } from "@/store/calendar";
@@ -153,29 +154,23 @@ export function MessageList({ title, list, openThreadId, focusId, setFocusId, on
 
   const onRowClick = useCallback(
     (e: MouseEvent, rowId: Id) => {
-      if (e.shiftKey && lastClick.current) {
-        const a = ids.indexOf(lastClick.current);
-        const b = ids.indexOf(rowId);
-        if (a >= 0 && b >= 0) {
-          const [s, en] = a < b ? [a, b] : [b, a];
-          select(ids.slice(s, en + 1), true);
-          window.getSelection()?.removeAllRanges();
-          return;
-        }
-      }
-      if (e.ctrlKey || e.metaKey) {
-        select([rowId], !selected[rowId]);
+      const action = rowClick({
+        rowId, ids, anchor: lastClick.current, selected,
+        modifiers: { shift: e.shiftKey, ctrl: e.ctrlKey || e.metaKey },
+        isMobile,
+      });
+      if (action.kind === "open") {
         lastClick.current = rowId;
-        return;
-      }
-      lastClick.current = rowId;
-      if (selCount > 0 && isMobile) {
-        select([rowId], !selected[rowId]);
-        return;
-      }
         onOpen(rowId);
+        return;
+      }
+      select(action.ids, action.on);
+      if (action.moveAnchor) lastClick.current = rowId;
+      // Shift-clicking a list also drags a text selection across it, which
+      // leaves the rows looking smeared blue over the selection they meant.
+      else window.getSelection()?.removeAllRanges();
     },
-    [ids, select, selected, selCount, isMobile, onOpen],
+    [ids, select, selected, isMobile, onOpen],
   );
 
   const onContext = useCallback(