-
Notifications
You must be signed in to change notification settings - Fork 1.5k
fix: preserve selection anchor for multi-Select range selection #10150
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -29,7 +29,7 @@ import {FormValidationState, useFormValidationState} from '../form/useFormValida | |
| import {ListState, useListState} from '../list/useListState'; | ||
| import {OverlayTriggerState, useOverlayTriggerState} from '../overlays/useOverlayTriggerState'; | ||
| import {useControlledState} from '../utils/useControlledState'; | ||
| import {useMemo, useState} from 'react'; | ||
| import {useMemo, useRef, useState} from 'react'; | ||
|
|
||
| export type SelectionMode = 'single' | 'multiple'; | ||
| export type ValueType<M extends SelectionMode> = M extends 'single' ? Key | null : readonly Key[]; | ||
|
|
@@ -196,12 +196,27 @@ export function useSelectState<T, M extends SelectionMode = 'single'>( | |
| } | ||
| }; | ||
|
|
||
| // Preserve the selection's anchor (anchorKey/currentKey) across renders. The | ||
| // multiple-selection `value` is a plain Key[], so without this the listbox | ||
| // would rebuild an anchorless Selection on every render and range selection | ||
| // (shift+click / shift+arrow) would collapse to just the clicked item. We keep | ||
| // the last Selection produced internally and feed it back while its membership | ||
| // still matches `value`. | ||
| let lastSelection = useRef<Set<Key> | null>(null); | ||
|
|
||
| let listState = useListState({ | ||
| ...props, | ||
| selectionMode, | ||
| disallowEmptySelection: selectionMode === 'single', | ||
| allowDuplicateSelectionEvents: true, | ||
| selectedKeys: useMemo(() => convertValue(displayValue), [displayValue]), | ||
| selectedKeys: useMemo(() => { | ||
| let selectedKeys = convertValue(displayValue); | ||
| let last = lastSelection.current; | ||
| if (last != null && Array.isArray(selectedKeys) && isSameSelection(last, selectedKeys)) { | ||
| return last; | ||
| } | ||
| return selectedKeys; | ||
|
Comment on lines
+213
to
+218
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was thinking maybe we could preserve the anchorKey and currentKey from the Selection instead and preserve them instead of walking and replicating the Selection, but I suppose we would still need to guard against if the set of selected keys changed outside of this flow still and thus walk the set anyways |
||
| }, [displayValue]), | ||
| onSelectionChange: (keys: Selection) => { | ||
| // impossible, but TS doesn't know that | ||
| if (keys === 'all') { | ||
|
|
@@ -212,6 +227,9 @@ export function useSelectState<T, M extends SelectionMode = 'single'>( | |
| let key = keys.values().next().value ?? null; | ||
| setValue(key); | ||
| } else { | ||
| // Remember the Selection (with its anchor) so it survives the round-trip | ||
| // through the plain `value` array on the next render. | ||
| lastSelection.current = keys; | ||
| setValue([...keys]); | ||
| } | ||
| if (shouldCloseOnSelect) { | ||
|
|
@@ -278,3 +296,15 @@ function convertValue(value: Key | Key[] | null | undefined) { | |
| } | ||
| return Array.isArray(value) ? value : [value]; | ||
| } | ||
|
|
||
| function isSameSelection(selection: Set<Key>, keys: Key[]): boolean { | ||
| if (selection.size !== keys.length) { | ||
| return false; | ||
| } | ||
| for (let key of keys) { | ||
| if (!selection.has(key)) { | ||
| return false; | ||
| } | ||
| } | ||
| return true; | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this technically breaks the rules of react because it reads from a ref during render.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
alternative suggestion: we could associate the extra info directly with the value via a WeakMap in onChange, and read it here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did a little looking into it, the current implementation does indeed fail the compiler, though it isn't being caught in lint at the moment even when updated, not sure why. You can see more info here #10174