diff --git a/v3/cypress/e2e/graph-legend.spec.ts b/v3/cypress/e2e/graph-legend.spec.ts index 8c68088e90..d0fd1d9035 100644 --- a/v3/cypress/e2e/graph-legend.spec.ts +++ b/v3/cypress/e2e/graph-legend.spec.ts @@ -526,7 +526,7 @@ context("Test drawing legend on existing legend", () => { }) }) -context("Test selecting and selecting categories in legend", () => { +context("Test selecting and unselecting categories in legend", () => { beforeEach(function () { const queryParams = "?sample=mammals&dashboard&mouseSensor&suppressUnsavedWarning" const url = `${Cypress.config("index")}${queryParams}` @@ -549,10 +549,8 @@ context("Test selecting and selecting categories in legend", () => { glh.selectCategoryColorForCategoricalLegend(arrayOfValues[7].values[2]) glh.verifyCategoricalLegendKeySelected(arrayOfValues[7].values[2]) - // For some reason clicking on the background to unselect the legend - // is not working - // glh.unselectLegendCategory() - // glh.verifyNoLegendCategorySelectedForCategoricalLegend() + glh.unselectLegendCategory() + glh.verifyNoLegendCategorySelectedForCategoricalLegend() // glh.openLegendMenu() // glh.removeAttributeFromLegend(arrayOfAttributes[7]) // ah.openAxisAttributeMenu("bottom") @@ -566,10 +564,8 @@ context("Test selecting and selecting categories in legend", () => { glh.selectNumericLegendCategory(1) glh.verifyNumericLegendKeySelected() - // For some reason clicking on the background to unselect the legend - // is not working - // glh.unselectLegendCategory() - // glh.verifyNoLegendCategorySelectedForNumericLegend() + glh.unselectLegendCategory() + glh.verifyNoLegendCategorySelectedForNumericLegend() // glh.openLegendMenu() // glh.removeAttributeFromLegend(arrayOfAttributes[3]) @@ -599,6 +595,58 @@ context("Test selecting and selecting categories in legend", () => { glh.selectCategoryKeyCornerForCategoricalLegend(arrayOfValues[7].values[0]) glh.verifyCategoricalLegendKeySelected(arrayOfValues[7].values[0]) }) + + /* + * A legend key selects cases elsewhere rather than being selected itself, so a plain click + * replaces the selection and a modifier click toggles one category in or out. The command key is + * platform-exclusive -- cmd on a Mac, ctrl everywhere else -- so the two are tested apart: the + * platform's own key toggles, and the other platform's key carries no selection meaning. + */ + const cmdModifier = Cypress.platform === "darwin" ? "metaKey" : "ctrlKey" + const foreignModifier = Cypress.platform === "darwin" ? "ctrlKey" : "metaKey" + + const plotHabitatAgainstDiet = () => { + cy.dragAttributeToTarget("table", arrayOfAttributes[8], "bottom") // Diet => x-axis + glh.dragAttributeToPlot(arrayOfAttributes[7]) // Habitat => plot area + } + + it("will keep a category selected when its key is clicked a second time", () => { + plotHabitatAgainstDiet() + glh.selectCategoryNameForCategoricalLegend(arrayOfValues[7].values[0]) + glh.verifyCategoricalLegendKeySelected(arrayOfValues[7].values[0]) + glh.selectCategoryNameForCategoricalLegend(arrayOfValues[7].values[0]) + glh.verifyCategoricalLegendKeySelected(arrayOfValues[7].values[0]) + }) + + it("will add and remove a category with a shift click", () => { + plotHabitatAgainstDiet() + glh.selectCategoryNameForCategoricalLegend(arrayOfValues[7].values[0]) + glh.modifierClickCategoryForCategoricalLegend(arrayOfValues[7].values[1], "shiftKey") + glh.verifyCategoricalLegendKeySelected(arrayOfValues[7].values[0]) + glh.verifyCategoricalLegendKeySelected(arrayOfValues[7].values[1]) + glh.modifierClickCategoryForCategoricalLegend(arrayOfValues[7].values[1], "shiftKey") + glh.verifyCategoricalLegendKeySelected(arrayOfValues[7].values[0]) + glh.verifyCategoricalLegendKeyNotSelected(arrayOfValues[7].values[1]) + }) + + it("will add and remove a category with the platform command key", () => { + plotHabitatAgainstDiet() + glh.selectCategoryNameForCategoricalLegend(arrayOfValues[7].values[0]) + glh.modifierClickCategoryForCategoricalLegend(arrayOfValues[7].values[1], cmdModifier) + glh.verifyCategoricalLegendKeySelected(arrayOfValues[7].values[0]) + glh.verifyCategoricalLegendKeySelected(arrayOfValues[7].values[1]) + glh.modifierClickCategoryForCategoricalLegend(arrayOfValues[7].values[1], cmdModifier) + glh.verifyCategoricalLegendKeySelected(arrayOfValues[7].values[0]) + glh.verifyCategoricalLegendKeyNotSelected(arrayOfValues[7].values[1]) + }) + + it("will replace the selection when the other platform's command key is held", () => { + plotHabitatAgainstDiet() + glh.selectCategoryNameForCategoricalLegend(arrayOfValues[7].values[0]) + glh.modifierClickCategoryForCategoricalLegend(arrayOfValues[7].values[1], foreignModifier) + glh.verifyCategoricalLegendKeyNotSelected(arrayOfValues[7].values[0]) + glh.verifyCategoricalLegendKeySelected(arrayOfValues[7].values[1]) + }) }) context("Test changing legend colors", () => { describe("Test changing legend colors for categorical legend", () => { diff --git a/v3/cypress/support/helpers/graph-legend-helper.ts b/v3/cypress/support/helpers/graph-legend-helper.ts index 6c71375612..49b5afe09c 100644 --- a/v3/cypress/support/helpers/graph-legend-helper.ts +++ b/v3/cypress/support/helpers/graph-legend-helper.ts @@ -23,6 +23,15 @@ export const GraphLegendHelper = { selectCategoryNameForCategoricalLegend(name: string) { gle.getCategoricalLegendCategory(name).click() }, + /* + * Clicks a key with a modifier held. Legend categories are nominal, so shift, cmd, and ctrl all + * mean the same thing -- toggle this category -- and each needs its own coverage. cmd and ctrl + * are platform-exclusive, so only the one matching the running platform toggles; see + * hasSelectionModifier in platform-utils. + */ + modifierClickCategoryForCategoricalLegend(name: string, modifier: "shiftKey" | "metaKey" | "ctrlKey") { + gle.getCategoricalLegendCategory(name).click({ [modifier]: true }) + }, selectCategoryColorForCategoricalLegend(name: string) { gle.getCategoricalLegendCategory(name).parent().find(".legend-key-shape").click() }, @@ -46,8 +55,15 @@ export const GraphLegendHelper = { cy.get("body").click(x, y) }) }, + /* + * Clears the selection by clicking empty plot background. The click has to land on the renderer's + * canvas: useRendererPointerDown only deselects when event.target is the canvas itself, and the + * canvas is layered over the SVG plot background, so a real pointer never reaches the rect + * underneath. Clicking that rect instead -- which needs `force` precisely because the canvas + * covers it -- simulates a click no user can perform, and silently does nothing. + */ unselectLegendCategory() { - gle.getGraphTile().find(".plot-cell-background").eq(0).click({force:true}) + gle.getGraphTile().find("canvas").first().click("topLeft") }, // A circle is the only key drawn as arcs, so the arc command tells the two apart without pinning // down the exact path data. @@ -62,6 +78,10 @@ export const GraphLegendHelper = { gle.getCategoricalLegendCategory(name).parent().find(".legend-key-shape") .should("have.class", "legend-rect-selected") }, + verifyCategoricalLegendKeyNotSelected(name: string) { + gle.getCategoricalLegendCategory(name).parent().find(".legend-key-shape") + .should("not.have.class", "legend-rect-selected") + }, verifyNumericLegendKeySelected() { gle.getNumericLegendCategories().should("have.class", "legend-rect-selected") }, diff --git a/v3/src/components/data-display/components/legend/categorical-legend.tsx b/v3/src/components/data-display/components/legend/categorical-legend.tsx index 384a2a97c6..0dcb3a707d 100644 --- a/v3/src/components/data-display/components/legend/categorical-legend.tsx +++ b/v3/src/components/data-display/components/legend/categorical-legend.tsx @@ -4,6 +4,7 @@ import {mstReaction} from "../../../../utilities/mst-reaction" import { mstAutorun } from "../../../../utilities/mst-autorun" import { kDefaultPointShape } from "../../../../utilities/point-shape-utils" import { setSelectedCases, selectCases } from "../../../../models/data/data-set-utils" +import { hasSelectionModifier } from "../../../../utilities/platform-utils" import { getTileModel } from "../../../../models/tiles/tile-model" import {axisGap} from "../../../axis/axis-types" import { swapCategoriesNotification } from "../../data-display-notifications" @@ -70,11 +71,16 @@ export const CategoricalLegend = const handleLegendKeyClick = useCallback((event: any, d: Key) => { const caseIds = dataConfiguration?.getCasesForLegendValue(d.category) - if (caseIds) { - // This is breaking the graph-legend cypress test - // setOrExtendSelection(caseIds, dataConfiguration?.dataset, event.shiftKey) - if (event.shiftKey) selectCases(caseIds, dataConfiguration?.dataset) - else setSelectedCases(caseIds, dataConfiguration?.dataset) + if (!caseIds) return + if (hasSelectionModifier(event)) { + // Reuses the predicate behind the key's selected styling, so the gesture always matches what + // the key shows. getCasesForLegendValue returns parent cases when the legend attribute lives + // in a parent collection; selectCases expands those to their child items in both directions, + // so deselecting reaches the same cases selecting did. + const isSelected = dataConfiguration?.allCasesForCategoryAreSelected(d.category) + selectCases(caseIds, dataConfiguration?.dataset, !isSelected) + } else { + setSelectedCases(caseIds, dataConfiguration?.dataset) } }, [dataConfiguration]) diff --git a/v3/src/components/data-display/hooks/use-renderer-pointer-down-deselect.ts b/v3/src/components/data-display/hooks/use-renderer-pointer-down-deselect.ts index f9783cd197..32d89b49d0 100644 --- a/v3/src/components/data-display/hooks/use-renderer-pointer-down-deselect.ts +++ b/v3/src/components/data-display/hooks/use-renderer-pointer-down-deselect.ts @@ -1,4 +1,5 @@ import { selectAllCases } from "../../../models/data/data-set-utils" +import { preservesSelection } from "../../../utilities/platform-utils" import { IDataDisplayContentModel } from "../models/data-display-content-model" import { PointRendererArray, PointRendererBase } from "../renderer" import { useRendererPointerDown } from "./use-renderer-pointer-down" @@ -8,7 +9,7 @@ export function useRendererPointerDownDeselect( model?: IDataDisplayContentModel ) { useRendererPointerDown(rendererArray, (event, renderer: PointRendererBase) => { - if (!event.shiftKey && !event.metaKey && !event.ctrlKey) { + if (!preservesSelection(event)) { renderer.requestAnimationFrame("deselectAll", () => { const datasetsArray = model?.datasetsArray ?? [] datasetsArray.forEach(data => selectAllCases(data, false)) diff --git a/v3/src/components/graph/plots/scatter-plot/use-residual-marquee.ts b/v3/src/components/graph/plots/scatter-plot/use-residual-marquee.ts index 12b408b932..167f0010c8 100644 --- a/v3/src/components/graph/plots/scatter-plot/use-residual-marquee.ts +++ b/v3/src/components/graph/plots/scatter-plot/use-residual-marquee.ts @@ -4,6 +4,7 @@ import RTreeLib from "rtree" import { Logger } from "../../../../lib/logger" import { IDataSet } from "../../../../models/data/data-set" import { selectAllCases, selectAndDeselectCases } from "../../../../models/data/data-set-utils" +import { preservesSelection } from "../../../../utilities/platform-utils" import { rTreeRect } from "../../../data-display/data-display-types" import { getCasesForDelta, rectNormalize } from "../../../data-display/data-display-utils" import { MarqueeState } from "../../../data-display/models/marquee-state" @@ -120,7 +121,7 @@ export function useResidualMarquee(props: IUseResidualMarquee) { suppressClickRef.current = false return } - if (!event.shiftKey && !event.metaKey && !event.ctrlKey && dataset) { + if (!preservesSelection(event) && dataset) { selectAllCases(dataset, false) } }, [dataset]) diff --git a/v3/src/components/map/hooks/use-map-click-deselect.ts b/v3/src/components/map/hooks/use-map-click-deselect.ts index 7a2b10b9d8..5c6949036f 100644 --- a/v3/src/components/map/hooks/use-map-click-deselect.ts +++ b/v3/src/components/map/hooks/use-map-click-deselect.ts @@ -1,6 +1,7 @@ import { useCallback, useEffect, useRef } from "react" import { selectAllCases } from "../../../models/data/data-set-utils" import { useTileSelectionContext } from "../../../hooks/use-tile-selection-context" +import { preservesSelection } from "../../../utilities/platform-utils" import { kDoubleClickDelay } from "../../constants" import { IMapContentModel } from "../models/map-content-model" @@ -37,7 +38,7 @@ export function useMapClickDeselect(mapModel: IMapContentModel) { const handleMapClick = useCallback((event: MouseEvent) => { if (!wasTileSelectedRef.current) return if (mapModel._ignoreLeafletClicks) return - if (event.shiftKey || event.metaKey || event.ctrlKey) return + if (preservesSelection(event)) return // Delay deselection so double-clicks (which trigger zoom) don't // inadvertently clear the selection. The pending deselect is cancelled diff --git a/v3/src/utilities/platform-utils.test.ts b/v3/src/utilities/platform-utils.test.ts new file mode 100644 index 0000000000..fbd9a3f216 --- /dev/null +++ b/v3/src/utilities/platform-utils.test.ts @@ -0,0 +1,103 @@ +/* + * isMac is evaluated once at import, so each platform needs its own module instance rather than a + * mock that can be swapped between assertions. + */ +function loadForPlatform(platform: string) { + Object.defineProperty(window.navigator, "platform", { value: platform, configurable: true }) + let loaded: typeof import("./platform-utils") | undefined + jest.isolateModules(() => { + loaded = require("./platform-utils") + }) + return loaded! +} + +const mac = () => loadForPlatform("MacIntel") +const win = () => loadForPlatform("Win32") + +describe("platform-utils", () => { + const originalPlatform = Object.getOwnPropertyDescriptor(window.navigator, "platform") + + afterEach(() => { + if (originalPlatform) Object.defineProperty(window.navigator, "platform", originalPlatform) + }) + + describe("isMac / cmdKey", () => { + it("detects a Mac", () => { + expect(mac().isMac).toBe(true) + expect(mac().cmdKey).toBe("Meta") + }) + + it("detects a non-Mac", () => { + expect(win().isMac).toBe(false) + expect(win().cmdKey).toBe("Control") + }) + }) + + describe("isCommandKeyDown", () => { + it("reads meta on a Mac and ignores ctrl", () => { + const { isCommandKeyDown } = mac() + expect(isCommandKeyDown({ metaKey: true })).toBe(true) + expect(isCommandKeyDown({ ctrlKey: true })).toBe(false) + }) + + it("reads ctrl off the Mac and ignores meta", () => { + const { isCommandKeyDown } = win() + expect(isCommandKeyDown({ ctrlKey: true })).toBe(true) + expect(isCommandKeyDown({ metaKey: true })).toBe(false) + }) + }) + + describe("hasSelectionModifier", () => { + it("treats shift as a selection modifier on either platform", () => { + expect(mac().hasSelectionModifier({ shiftKey: true })).toBe(true) + expect(win().hasSelectionModifier({ shiftKey: true })).toBe(true) + }) + + // ctrl is the secondary-click gesture on a Mac, so it must not toggle a selection there. + it("accepts cmd but not ctrl on a Mac", () => { + const { hasSelectionModifier } = mac() + expect(hasSelectionModifier({ metaKey: true })).toBe(true) + expect(hasSelectionModifier({ ctrlKey: true })).toBe(false) + }) + + it("accepts ctrl but not cmd off the Mac", () => { + const { hasSelectionModifier } = win() + expect(hasSelectionModifier({ ctrlKey: true })).toBe(true) + expect(hasSelectionModifier({ metaKey: true })).toBe(false) + }) + + // alt already means option-click zoom on the plot background and rescale on an axis. + it("ignores alt", () => { + expect(mac().hasSelectionModifier({ altKey: true })).toBe(false) + expect(win().hasSelectionModifier({ altKey: true })).toBe(false) + }) + + it("is false for an unmodified click", () => { + expect(mac().hasSelectionModifier({})).toBe(false) + expect(win().hasSelectionModifier({})).toBe(false) + }) + }) + + describe("preservesSelection", () => { + // Deliberately more permissive than hasSelectionModifier: any modifier at all means the click + // should not destroy the existing selection, including ctrl on a Mac. + it("accepts shift, cmd, and ctrl on both platforms", () => { + for (const load of [mac, win]) { + const { preservesSelection } = load() + expect(preservesSelection({ shiftKey: true })).toBe(true) + expect(preservesSelection({ metaKey: true })).toBe(true) + expect(preservesSelection({ ctrlKey: true })).toBe(true) + } + }) + + it("ignores alt", () => { + expect(mac().preservesSelection({ altKey: true })).toBe(false) + expect(win().preservesSelection({ altKey: true })).toBe(false) + }) + + it("is false for an unmodified click", () => { + expect(mac().preservesSelection({})).toBe(false) + expect(win().preservesSelection({})).toBe(false) + }) + }) +}) diff --git a/v3/src/utilities/platform-utils.ts b/v3/src/utilities/platform-utils.ts index 93f89000ec..8ad89a018e 100644 --- a/v3/src/utilities/platform-utils.ts +++ b/v3/src/utilities/platform-utils.ts @@ -2,6 +2,48 @@ export const isMac = navigator.platform.toLowerCase().includes("mac") export const cmdKey = isMac ? "Meta" : "Control" -export function isCommandKeyDown(event: KeyboardEvent | React.KeyboardEvent) { - return (isMac && event.metaKey) || (!isMac && event.ctrlKey) +/* + * The modifier flags shared by React, DOM, d3, and Leaflet events, so every caller can pass its own + * event without converting it. Leaflet callers pass `event.originalEvent`. + */ +export interface IModifierKeys { + altKey?: boolean + ctrlKey?: boolean + metaKey?: boolean + shiftKey?: boolean +} + +export function isCommandKeyDown(event: IModifierKeys) { + return (isMac && !!event.metaKey) || (!isMac && !!event.ctrlKey) +} + +/* + * Whether a click should adjust the selection instead of replacing it. + * + * Selection targets in the graph, map, and legend are nominal: there is no meaningful span between + * two categories or two plotted points, so there is nothing for shift to extend that cmd would not + * do. Every modifier therefore means one thing -- toggle this target in or out. Ordered targets + * like case table rows are the exception; they read shift separately to extend an anchored range. + * + * ctrl counts only off the Mac, where it is not the secondary-click gesture. alt is excluded + * because it is already spoken for: option-click zooms the plot background and rescales axes. + * + * New callers should route through here rather than reading modifier flags directly, so that + * supporting context menus, or deciding ctrl should toggle on the Mac after all, is a single edit. + * Several selection handlers in the graph, map, and case table still read the flags themselves and + * have yet to be migrated. + */ +export function hasSelectionModifier(event: IModifierKeys) { + return !!event.shiftKey || isCommandKeyDown(event) +} + +/* + * Whether a click should leave an existing selection alone rather than clearing it. Deliberately + * more permissive than hasSelectionModifier: acting on a selection should be precise, but + * destroying one should not happen while the user holds shift, cmd, or ctrl -- including ctrl on a + * Mac, where the click is opening a context menu rather than asking us to clear. alt is excluded + * here too, since it means option-click zoom rather than anything about selection. + */ +export function preservesSelection(event: IModifierKeys) { + return !!event.shiftKey || !!event.metaKey || !!event.ctrlKey }