feat(plotly): Escape to clear selection and sync greyout across traces#9311
Open
axsseldz wants to merge 3 commits intomarimo-team:mainfrom
Open
feat(plotly): Escape to clear selection and sync greyout across traces#9311axsseldz wants to merge 3 commits intomarimo-team:mainfrom
axsseldz wants to merge 3 commits intomarimo-team:mainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
There was a problem hiding this comment.
Pull request overview
Adds improved selection UX for mo.ui.plotly by enabling Escape-to-clear, ensuring non-selected points are consistently greyed out across all traces, and preventing stale box/lasso overlays after click selections.
Changes:
- Forward selection state into the Plot component and reconcile per-trace
selectedpointsviaPlotly.updatefor consistent greyout. - Add Escape key handling to clear selections (in addition to Plotly’s double-click deselect).
- Adjust
plotly_selectedhandling to treat click-style selection events as overlay-clearing updates.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| frontend/src/plugins/impl/plotly/tests/PlotlyPlugin.test.tsx | Adds frontend tests covering forwarded selection props, overlay clearing, click-vs-box selection behavior, and deselect state reset. |
| frontend/src/plugins/impl/plotly/PlotlyPlugin.tsx | Computes hasSelection, forwards selection props to Plot, centralizes clearing behavior, and refines onSelected handling for clickmode edge cases. |
| frontend/src/plugins/impl/plotly/Plot.tsx | Implements Escape-to-clear and a selection-visual sync pass that updates selectedpoints across traces and clears stale layout.selections. |
Comment on lines
+165
to
+183
| // Don't hijack Escape from text editors elsewhere on the page | ||
| // (e.g. CodeMirror notebook cells, inputs, search boxes). | ||
| const active = document.activeElement; | ||
| if (active instanceof HTMLElement) { | ||
| const tag = active.tagName; | ||
| if ( | ||
| tag === "INPUT" || | ||
| tag === "TEXTAREA" || | ||
| tag === "SELECT" || | ||
| active.isContentEditable | ||
| ) { | ||
| return; | ||
| } | ||
| // With multiple plots on the page, only clear the one whose | ||
| // container holds focus. A bare-body activeElement means nothing | ||
| // in particular is focused, in which case it's fine to clear. | ||
| if (active !== document.body && !el.contains(active)) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
The Escape handler is effectively global: Plot’s container
isn’t focusable, so after clicking a chart
document.activeElement is typically document.body. With multiple plots that have selections, all of them will satisfy active === document.body and clear on a single Escape press, which doesn’t match the “only the focused chart clears” behavior.
Consider making the plot container focusable (e.g., tabIndex=0) and explicitly focusing it on pointer/click, then require el.contains(document.activeElement) (or a tracked “last interacted” plot id) before clearing. Alternatively, don’t treat document.body as a valid target for clearing.
Collaborator
There was a problem hiding this comment.
You can maybe use the onKeyDown on the element. Or use useKeydownOnElement
return (
<div
id={divId}
className={className}
style={style}
ref={containerRef}
tabIndex={-1} // focusable programmatically, not in tab order
onMouseDown={() => {
// Plotly doesn't focus its container on click; do it ourselves so
// Escape-to-clear routes to the chart the user just interacted with.
containerRef.current?.focus({ preventScroll: true });
}}
onKeyDown={(e) => {
if (e.key !== "Escape" || e.defaultPrevented) return;
if (!hasSelection || !onDeselect) return;
const el = containerRef.current;
if (!el) return;
Plotly.restyle(el, "selectedpoints", null).catch(() => {});
Plotly.relayout(el, "selections", null).catch(() => {});
onDeselect();
e.stopPropagation();
}}
/>
);
Comment on lines
+209
to
+261
| useEffect(() => { | ||
| const el = containerRef.current; | ||
| if (!el) { | ||
| return; | ||
| } | ||
| if (!data.length) { | ||
| return; | ||
| } | ||
| if (selectedPoints === undefined && layoutSelections === undefined) { | ||
| return; | ||
| } | ||
|
|
||
| const traceIndices = data.map((_, i) => i); | ||
| const traceUpdate: Partial<PlotlyTypes.Data> = {}; | ||
| if (selectedPoints !== undefined) { | ||
| const byTrace = new Map<number, number[]>(); | ||
| for (const point of selectedPoints) { | ||
| const curve = | ||
| typeof point.curveNumber === "number" ? point.curveNumber : undefined; | ||
| if (curve === undefined) { | ||
| continue; | ||
| } | ||
| const pointIdx = | ||
| typeof point.pointIndex === "number" | ||
| ? point.pointIndex | ||
| : typeof point.pointNumber === "number" | ||
| ? point.pointNumber | ||
| : undefined; | ||
| if (pointIdx === undefined) { | ||
| continue; | ||
| } | ||
| const indices = byTrace.get(curve) ?? []; | ||
| indices.push(pointIdx); | ||
| byTrace.set(curve, indices); | ||
| } | ||
| const anySelection = byTrace.size > 0; | ||
| const emptyFill: number[] | null = anySelection ? [] : null; | ||
| (traceUpdate as { selectedpoints: (number[] | null)[] }).selectedpoints = | ||
| traceIndices.map((i) => byTrace.get(i) ?? emptyFill); | ||
| } | ||
|
|
||
| const layoutUpdate: Partial<PlotlyTypes.Layout> = {}; | ||
| if (layoutSelections !== undefined) { | ||
| const hasActiveOverlay = | ||
| Array.isArray(layoutSelections) && layoutSelections.length > 0; | ||
| if (!hasActiveOverlay) { | ||
| // `null` removes the attribute; cast because Layout's type omits it. | ||
| (layoutUpdate as { selections: null }).selections = null; | ||
| } | ||
| } | ||
|
|
||
| Plotly.update(el, traceUpdate, layoutUpdate, traceIndices).catch(() => {}); | ||
| }, [selectedPoints, layoutSelections, data]); |
There was a problem hiding this comment.
Plotly.update(...) here can run before the preceding Plotly.react(...) render finishes (and also before/while Plotly.react is re-running when data/layout change). Because errors are swallowed, this can silently fail and leave initial/restored selections un-synced (e.g., no greyout applied on mount when value.points is pre-populated).
Consider gating this effect on a “plot ready” flag set after Plotly.react resolves, or chaining the selection sync off the Plotly.react promise, so the update is guaranteed to apply after the plot is initialized.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
📝 Summary
Followup (#9011 (review))
Escapewith a chart focused to clear any box, lasso, or click selection (complements Plotly's double-click).clickmode="event+select"now clears any previous box/lasso overlay instead of leaving a stale shape behind.📋 Pre-Review Checklist
✅ Merge Checklist
CC: @Light2Dark @mscolnick