Take the message you were reading into the selection
Ctrl-clicking a second message selected only the second. The first stayed highlighted, because it was the one open -- which is a different state wearing a similar colour -- and was never actually selected. So both looked picked, one was, and every action that followed applied to half of what the screen showed, silently. The cause is visible once the two rules sit together: shift-click took the whole run *including* the row it started from, and ctrl-click took only the row clicked. Two branches of one handler that had drifted apart, with nothing asserting they agreed. So they are one function now, and tested. Ctrl-click brings the current row with it while nothing is selected yet; once there is a selection it toggles exactly one row, which is what it is for. Shift-click is unchanged, and keeps its anchor where it is so extending a range twice grows it from the same place rather than from wherever it last reached. The last test asserts the property that failed rather than the branches: whichever modifier begins a selection, the anchor is in it.
This commit is contained in:
@@ -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");
|
||||
}
|
||||
});
|
||||
});
|
||||
@@ -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" };
|
||||
}
|
||||
@@ -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(
|
||||
|
||||
Reference in New Issue
Block a user