From 5269b19865edc04e75cb453c832cefd8e0cc2152 Mon Sep 17 00:00:00 2001 From: Brian Ingles Date: Tue, 3 Dec 2024 17:30:17 -0600 Subject: [PATCH 1/7] WIP: ComboBox show all items on open (DH-18088) --- packages/components/package.json | 1 + .../src/spectrum/comboBox/ComboBox.tsx | 2 +- .../src/spectrum/ComboBox.tsx | 32 ++++++++++++++++++- 3 files changed, 33 insertions(+), 2 deletions(-) diff --git a/packages/components/package.json b/packages/components/package.json index f872a24ae3..ac6d44cb22 100644 --- a/packages/components/package.json +++ b/packages/components/package.json @@ -35,6 +35,7 @@ "@react-spectrum/theme-default": "^3.5.1", "@react-spectrum/toast": "^3.0.0-beta.16", "@react-spectrum/utils": "^3.11.5", + "@react-types/combobox": "3.13.1", "@react-types/radio": "^3.8.1", "@react-types/shared": "^3.22.1", "@react-types/textfield": "^3.9.1", diff --git a/packages/components/src/spectrum/comboBox/ComboBox.tsx b/packages/components/src/spectrum/comboBox/ComboBox.tsx index 3898d65743..f6a365635b 100644 --- a/packages/components/src/spectrum/comboBox/ComboBox.tsx +++ b/packages/components/src/spectrum/comboBox/ComboBox.tsx @@ -10,7 +10,7 @@ import type { NormalizedItem } from '../utils'; import { type PickerPropsT, usePickerProps } from '../picker'; export type ComboBoxProps = PickerPropsT>; - +export { type MenuTriggerAction } from '@react-types/combobox'; export { SpectrumComboBox }; export const ComboBox = React.forwardRef(function ComboBox( diff --git a/packages/jsapi-components/src/spectrum/ComboBox.tsx b/packages/jsapi-components/src/spectrum/ComboBox.tsx index c41a23b608..f139b8b2cb 100644 --- a/packages/jsapi-components/src/spectrum/ComboBox.tsx +++ b/packages/jsapi-components/src/spectrum/ComboBox.tsx @@ -1,9 +1,10 @@ import { ComboBoxNormalized, + type MenuTriggerAction, type NormalizedItem, type SpectrumComboBoxProps, } from '@deephaven/components'; -import { useCallback } from 'react'; +import { useCallback, useRef } from 'react'; import { type PickerWithTableProps } from './PickerProps'; import { usePickerProps } from './utils'; @@ -18,19 +19,48 @@ export function ComboBox(props: ComboBoxProps): JSX.Element { ...pickerProps } = usePickerProps(props); + const menuTriggerActionRef = useRef(); + const onInputChange = useCallback( (value: string) => { onInputChangeInternal?.(value); + onSearchTextChange(value); + + // const searchText = menuTriggerActionRef.current === 'input' ? value : ''; + + console.log('[TESTING]', menuTriggerActionRef.current, value); + + // We want the ComboBox to show all items whenever it is initially opened, + // so keep search text set to empty string while it is closed. This is + // mostly to handle the intial state, since `onInputChange` gets called + // before the user has interacted. + // onSearchTextChange(isOpenRef.current ? value : ''); }, [onInputChangeInternal, onSearchTextChange] ); + const onOpenChange = useCallback( + (isOpen: boolean, menuTrigger?: MenuTriggerAction) => { + console.log('[TESTING] onOpenChange', isOpen, menuTrigger); + menuTriggerActionRef.current = isOpen ? menuTrigger : undefined; + + pickerProps.onOpenChange?.(isOpen); + + // Clear filtering on close so that all items show on next open + if (!isOpen) { + onSearchTextChange(''); + } + }, + [onSearchTextChange, pickerProps] + ); + return ( ); } From 2b3905bbc1076ba306c53180e8ec600df89c9a10 Mon Sep 17 00:00:00 2001 From: Brian Ingles Date: Thu, 2 Jan 2025 17:45:26 -0600 Subject: [PATCH 2/7] Handle different menu triggers (DH-18088) --- .../spectrum/picker/usePickerScrollOnOpen.ts | 7 +++-- .../src/spectrum/ComboBox.tsx | 29 ++++++++----------- 2 files changed, 16 insertions(+), 20 deletions(-) diff --git a/packages/components/src/spectrum/picker/usePickerScrollOnOpen.ts b/packages/components/src/spectrum/picker/usePickerScrollOnOpen.ts index 6e0c5bc0e6..dd0587efce 100644 --- a/packages/components/src/spectrum/picker/usePickerScrollOnOpen.ts +++ b/packages/components/src/spectrum/picker/usePickerScrollOnOpen.ts @@ -4,11 +4,12 @@ import { findSpectrumPickerScrollArea, usePopoverOnScrollRef, } from '@deephaven/react-hooks'; +import type { MenuTriggerAction } from '../comboBox'; export interface UsePickerScrollOnOpenOptions { getInitialScrollPosition?: () => Promise; onScroll: (event: Event) => void; - onOpenChange?: (isOpen: boolean) => void; + onOpenChange?: (isOpen: boolean, menuTrigger?: MenuTriggerAction) => void; } export interface UsePickerScrollOnOpenResult { @@ -37,11 +38,11 @@ export function usePickerScrollOnOpen({ ); const onOpenChangeInternal = useCallback( - (isOpen: boolean): void => { + (isOpen: boolean, menuTrigger?: MenuTriggerAction): void => { // Attach scroll event handling popoverOnOpenChange(isOpen); - onOpenChange?.(isOpen); + onOpenChange?.(isOpen, menuTrigger); }, [onOpenChange, popoverOnOpenChange] ); diff --git a/packages/jsapi-components/src/spectrum/ComboBox.tsx b/packages/jsapi-components/src/spectrum/ComboBox.tsx index f139b8b2cb..73a3168b18 100644 --- a/packages/jsapi-components/src/spectrum/ComboBox.tsx +++ b/packages/jsapi-components/src/spectrum/ComboBox.tsx @@ -19,37 +19,32 @@ export function ComboBox(props: ComboBoxProps): JSX.Element { ...pickerProps } = usePickerProps(props); - const menuTriggerActionRef = useRef(); + const isOpenRef = useRef(false); + const inputValueRef = useRef(''); const onInputChange = useCallback( (value: string) => { - onInputChangeInternal?.(value); - - onSearchTextChange(value); + inputValueRef.current = value; - // const searchText = menuTriggerActionRef.current === 'input' ? value : ''; - - console.log('[TESTING]', menuTriggerActionRef.current, value); + onInputChangeInternal?.(value); - // We want the ComboBox to show all items whenever it is initially opened, - // so keep search text set to empty string while it is closed. This is - // mostly to handle the intial state, since `onInputChange` gets called - // before the user has interacted. - // onSearchTextChange(isOpenRef.current ? value : ''); + // Clear search text when ComboBox is closed + onSearchTextChange(isOpenRef.current ? value : ''); }, [onInputChangeInternal, onSearchTextChange] ); const onOpenChange = useCallback( (isOpen: boolean, menuTrigger?: MenuTriggerAction) => { - console.log('[TESTING] onOpenChange', isOpen, menuTrigger); - menuTriggerActionRef.current = isOpen ? menuTrigger : undefined; + isOpenRef.current = isOpen; pickerProps.onOpenChange?.(isOpen); - // Clear filtering on close so that all items show on next open - if (!isOpen) { - onSearchTextChange(''); + // Restore search text when ComboBox is being opened if menu trigger was + // from user input. Otherwise we don't want to show all items on initial + // open. + if (isOpen && menuTrigger === 'input') { + onSearchTextChange(inputValueRef.current); } }, [onSearchTextChange, pickerProps] From 92af58b21b82f718df43e52f86ceaee1c2d81171 Mon Sep 17 00:00:00 2001 From: Brian Ingles Date: Fri, 3 Jan 2025 09:36:53 -0600 Subject: [PATCH 3/7] Updated comment (DH-18088) --- packages/jsapi-components/src/spectrum/ComboBox.tsx | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/jsapi-components/src/spectrum/ComboBox.tsx b/packages/jsapi-components/src/spectrum/ComboBox.tsx index 73a3168b18..5fea0a7825 100644 --- a/packages/jsapi-components/src/spectrum/ComboBox.tsx +++ b/packages/jsapi-components/src/spectrum/ComboBox.tsx @@ -41,8 +41,7 @@ export function ComboBox(props: ComboBoxProps): JSX.Element { pickerProps.onOpenChange?.(isOpen); // Restore search text when ComboBox is being opened if menu trigger was - // from user input. Otherwise we don't want to show all items on initial - // open. + // from user input. if (isOpen && menuTrigger === 'input') { onSearchTextChange(inputValueRef.current); } From 56ee44cc822ca3abc4f0e19075b6fc91bc42e2bc Mon Sep 17 00:00:00 2001 From: Brian Ingles Date: Fri, 3 Jan 2025 09:52:43 -0600 Subject: [PATCH 4/7] Updated comments (DH-18088) --- .../src/spectrum/ComboBox.tsx | 23 ++++++++++++------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/packages/jsapi-components/src/spectrum/ComboBox.tsx b/packages/jsapi-components/src/spectrum/ComboBox.tsx index 5fea0a7825..01f5ba0c95 100644 --- a/packages/jsapi-components/src/spectrum/ComboBox.tsx +++ b/packages/jsapi-components/src/spectrum/ComboBox.tsx @@ -24,27 +24,34 @@ export function ComboBox(props: ComboBoxProps): JSX.Element { const onInputChange = useCallback( (value: string) => { - inputValueRef.current = value; - onInputChangeInternal?.(value); - // Clear search text when ComboBox is closed - onSearchTextChange(isOpenRef.current ? value : ''); + // Only apply search text if ComboBox is open. Note that `onInputChange` + // fires before `onOpenChange`, so we have to check `isOpenRef` to see the + // last value set by `onOpenChange`. + if (isOpenRef.current) { + onSearchTextChange(value); + } else { + // If the ComboBox is closed, reset the search text but store the value + // so it can be re-applied if the ComboBox is opened by user input. + onSearchTextChange(''); + inputValueRef.current = value; + } }, [onInputChangeInternal, onSearchTextChange] ); const onOpenChange = useCallback( (isOpen: boolean, menuTrigger?: MenuTriggerAction) => { - isOpenRef.current = isOpen; - pickerProps.onOpenChange?.(isOpen); - // Restore search text when ComboBox is being opened if menu trigger was - // from user input. + // Restore search text when ComboBox has been opened by user input. if (isOpen && menuTrigger === 'input') { onSearchTextChange(inputValueRef.current); } + + // Store the open state so that `onInputChange` has access to it. + isOpenRef.current = isOpen; }, [onSearchTextChange, pickerProps] ); From f460a6228d35ff51a2864d2471ae26ce3ab5dad1 Mon Sep 17 00:00:00 2001 From: Brian Ingles Date: Fri, 3 Jan 2025 11:18:16 -0600 Subject: [PATCH 5/7] Fixed tests (DH-18088) --- package-lock.json | 2 ++ .../spectrum/picker/usePickerScrollOnOpen.test.ts | 15 ++++++++++----- .../src/spectrum/picker/usePickerScrollOnOpen.ts | 2 +- 3 files changed, 13 insertions(+), 6 deletions(-) diff --git a/package-lock.json b/package-lock.json index 97cbe95cc0..6f8ca51c40 100644 --- a/package-lock.json +++ b/package-lock.json @@ -29426,6 +29426,7 @@ "@react-spectrum/theme-default": "^3.5.1", "@react-spectrum/toast": "^3.0.0-beta.16", "@react-spectrum/utils": "^3.11.5", + "@react-types/combobox": "3.13.1", "@react-types/radio": "^3.8.1", "@react-types/shared": "^3.22.1", "@react-types/textfield": "^3.9.1", @@ -31957,6 +31958,7 @@ "@react-spectrum/theme-default": "^3.5.1", "@react-spectrum/toast": "^3.0.0-beta.16", "@react-spectrum/utils": "^3.11.5", + "@react-types/combobox": "3.13.1", "@react-types/radio": "^3.8.1", "@react-types/shared": "^3.22.1", "@react-types/textfield": "^3.9.1", diff --git a/packages/components/src/spectrum/picker/usePickerScrollOnOpen.test.ts b/packages/components/src/spectrum/picker/usePickerScrollOnOpen.test.ts index e09cef195f..e21f49e8d6 100644 --- a/packages/components/src/spectrum/picker/usePickerScrollOnOpen.test.ts +++ b/packages/components/src/spectrum/picker/usePickerScrollOnOpen.test.ts @@ -49,9 +49,14 @@ describe('usePickerScrollOnOpen', () => { expect(result.current.ref).toBe(mockUsePopoverOnScrollRefResult.ref); }); - it.each([true, false])( - 'should return a callback that calls popoverOnOpenChange and onOpenChange: %s', - isOpen => { + it.each([ + [true, undefined], + [false, undefined], + [true, 'input'], + [false, 'input'], + ] as const)( + 'should return a callback that calls popoverOnOpenChange and onOpenChange: %s, %s', + (isOpen, menuTrigger) => { const { result } = renderHook(() => usePickerScrollOnOpen({ getInitialScrollPosition, @@ -60,12 +65,12 @@ describe('usePickerScrollOnOpen', () => { }) ); - result.current.onOpenChange(isOpen); + result.current.onOpenChange(isOpen, menuTrigger); expect(mockUsePopoverOnScrollRefResult.onOpenChange).toHaveBeenCalledWith( isOpen ); - expect(onOpenChange).toHaveBeenCalledWith(isOpen); + expect(onOpenChange).toHaveBeenCalledWith(isOpen, menuTrigger); } ); }); diff --git a/packages/components/src/spectrum/picker/usePickerScrollOnOpen.ts b/packages/components/src/spectrum/picker/usePickerScrollOnOpen.ts index dd0587efce..884923b330 100644 --- a/packages/components/src/spectrum/picker/usePickerScrollOnOpen.ts +++ b/packages/components/src/spectrum/picker/usePickerScrollOnOpen.ts @@ -14,7 +14,7 @@ export interface UsePickerScrollOnOpenOptions { export interface UsePickerScrollOnOpenResult { ref: DOMRef; - onOpenChange: (isOpen: boolean) => void; + onOpenChange: (isOpen: boolean, menuTrigger?: MenuTriggerAction) => void; } /** From d3ae40cb4910c7be41fb718410f91397933f5f9f Mon Sep 17 00:00:00 2001 From: Brian Ingles Date: Fri, 3 Jan 2025 11:56:06 -0600 Subject: [PATCH 6/7] Fixed bug with controlled input_value not clearing search results on open (DH-18088) --- .../src/spectrum/ComboBox.tsx | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/packages/jsapi-components/src/spectrum/ComboBox.tsx b/packages/jsapi-components/src/spectrum/ComboBox.tsx index 01f5ba0c95..e0be8e9c67 100644 --- a/packages/jsapi-components/src/spectrum/ComboBox.tsx +++ b/packages/jsapi-components/src/spectrum/ComboBox.tsx @@ -26,15 +26,18 @@ export function ComboBox(props: ComboBoxProps): JSX.Element { (value: string) => { onInputChangeInternal?.(value); - // Only apply search text if ComboBox is open. Note that `onInputChange` - // fires before `onOpenChange`, so we have to check `isOpenRef` to see the - // last value set by `onOpenChange`. + // Only apply search text if ComboBox is open. if (isOpenRef.current) { onSearchTextChange(value); } else { - // If the ComboBox is closed, reset the search text but store the value - // so it can be re-applied if the ComboBox is opened by user input. + // If the ComboBox is closed, reset the search text. This is needed for + // cases where the input change is not the result of user search input. onSearchTextChange(''); + + // Store the input value so that it can be restored when the ComboBox is + // opened as the result of user search input. Unfortunately, we can't + // determine this here but have to wait to check the `menuTrigger` arg + // passed to `onOpenChange`. inputValueRef.current = value; } }, @@ -45,8 +48,12 @@ export function ComboBox(props: ComboBoxProps): JSX.Element { (isOpen: boolean, menuTrigger?: MenuTriggerAction) => { pickerProps.onOpenChange?.(isOpen); + // Reset the search text when the ComboBox is closed. + if (!isOpen) { + onSearchTextChange(''); + } // Restore search text when ComboBox has been opened by user input. - if (isOpen && menuTrigger === 'input') { + else if (menuTrigger === 'input') { onSearchTextChange(inputValueRef.current); } From 733a315b3e8d02aa7596aef3c68a656c0f013462 Mon Sep 17 00:00:00 2001 From: Brian Ingles Date: Fri, 3 Jan 2025 12:01:39 -0600 Subject: [PATCH 7/7] Comments (DH-18088) --- .../jsapi-components/src/spectrum/ComboBox.tsx | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/packages/jsapi-components/src/spectrum/ComboBox.tsx b/packages/jsapi-components/src/spectrum/ComboBox.tsx index e0be8e9c67..3d1ce2cf53 100644 --- a/packages/jsapi-components/src/spectrum/ComboBox.tsx +++ b/packages/jsapi-components/src/spectrum/ComboBox.tsx @@ -29,15 +29,16 @@ export function ComboBox(props: ComboBoxProps): JSX.Element { // Only apply search text if ComboBox is open. if (isOpenRef.current) { onSearchTextChange(value); - } else { - // If the ComboBox is closed, reset the search text. This is needed for - // cases where the input change is not the result of user search input. + } + // When the ComboBox is closed, `onInputChange` may have been called as a + // result of user search input, ComboBox selection, or by selected key + // prop changes. We can't determine the source here, so we clear the search + // text and store the search value so that the list is unfiltered the next + // time the ComboBox is opened. We also store the search value so we can + // re-apply it in `onOpenChange` if the ComboBox is opened by user search + // input. + else { onSearchTextChange(''); - - // Store the input value so that it can be restored when the ComboBox is - // opened as the result of user search input. Unfortunately, we can't - // determine this here but have to wait to check the `menuTrigger` arg - // passed to `onOpenChange`. inputValueRef.current = value; } },