CODAP-1508: toggle legend category selection on a modifier click

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", () => {

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")
   },

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])
 

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))

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])

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

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)
+    })
+  })
+})

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
 }