diff --git a/web/src/lib/__tests__/listSelection.test.ts b/web/src/lib/__tests__/listSelection.test.ts new file mode 100644 index 0000000..203d8eb --- /dev/null +++ b/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[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"); + } + }); +}); diff --git a/web/src/lib/listSelection.ts b/web/src/lib/listSelection.ts new file mode 100644 index 0000000..5244612 --- /dev/null +++ b/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; + 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" }; +} diff --git a/web/src/views/mail/MessageList.tsx b/web/src/views/mail/MessageList.tsx index c4328f0..38e8b60 100644 --- a/web/src/views/mail/MessageList.tsx +++ b/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; + onOpen(rowId); return; } - lastClick.current = rowId; - if (selCount > 0 && isMobile) { - select([rowId], !selected[rowId]); - return; - } - onOpen(rowId); + 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(