diff --git a/.changeset/funny-pens-vanish.md b/.changeset/funny-pens-vanish.md new file mode 100644 index 000000000..bd99ffd15 --- /dev/null +++ b/.changeset/funny-pens-vanish.md @@ -0,0 +1,31 @@ +--- +'@qwik-ui/headless': patch +--- + +# Combobox Improvements + +## ๐Ÿ”„ Reactive Improvements + +- Better handling of array-based values +- Improved handling of initial and reactive values + +## ๐Ÿ› Key Bug Fixes + +- Fixed highlight jumping issues +- Enhanced empty input handling +- Better filtered item highlighting + +## ๐Ÿ–ฑ๏ธ Interaction Enhancements + +- Smoother scrolling experience +- Improved keyboard and mouse coordination + +## ๐Ÿš€ Performance Optimizations + +- More efficient item filtering + +## ๐Ÿงช Reliability + +- Added tests for reactivity handling, item unselection, and mouse-to-pointer interaction switching + +- Improved handling of edge cases in user interactions diff --git a/packages/kit-headless/src/components/combobox/combobox-context.tsx b/packages/kit-headless/src/components/combobox/combobox-context.tsx index aff81076c..a22bce646 100644 --- a/packages/kit-headless/src/components/combobox/combobox-context.tsx +++ b/packages/kit-headless/src/components/combobox/combobox-context.tsx @@ -18,14 +18,17 @@ export type ComboboxContext = { labelRef: Signal; controlRef: Signal; selectedValueSetSig: Signal>; + selectedValuesSig: Signal; + filteredIndexSetSig: Signal>; highlightedIndexSig: Signal; - currDisplayValueSig: Signal; + displayValuesSig: Signal; isMouseOverPopupSig: Signal; + isKeyboardFocusSig: Signal; disabledIndexSetSig: Signal>; removeOnBackspace: boolean; - hasVisibleItemsSig: Signal; + isNoItemsSig: Signal; initialLoadSig: Signal; - _value: string | undefined; + initialValue: string | undefined; loop: boolean; multiple: boolean | undefined; diff --git a/packages/kit-headless/src/components/combobox/combobox-empty.tsx b/packages/kit-headless/src/components/combobox/combobox-empty.tsx index d2eac1d7b..6c9833d12 100644 --- a/packages/kit-headless/src/components/combobox/combobox-empty.tsx +++ b/packages/kit-headless/src/components/combobox/combobox-empty.tsx @@ -7,7 +7,7 @@ export const HComboboxEmpty = component$((props: PropsOf<'div'>) => { return (
diff --git a/packages/kit-headless/src/components/combobox/combobox-hidden-option.tsx b/packages/kit-headless/src/components/combobox/combobox-hidden-option.tsx index 1ee0bc7a9..017cc2e5f 100644 --- a/packages/kit-headless/src/components/combobox/combobox-hidden-option.tsx +++ b/packages/kit-headless/src/components/combobox/combobox-hidden-option.tsx @@ -22,7 +22,7 @@ export const ComboboxHiddenSelectOption = component$( const context = useContext(comboboxContextId); useTask$(async function modularFormsValidation({ track }) { - track(() => context.selectedValueSetSig.value); + track(() => context.selectedValuesSig.value); if (isServer || !nativeSelectRef.value || !optionRef.value) return; @@ -37,7 +37,11 @@ export const ComboboxHiddenSelectOption = component$( 'Qwik UI: value not found when trying to select or unselect an item.', ); } - optionRef.value.selected = context.selectedValueSetSig.value.has(value); + + const selectedValues = context.selectedValuesSig.value; + optionRef.value.selected = Array.isArray(selectedValues) + ? selectedValues.includes(value) + : selectedValues === value; }); return ( diff --git a/packages/kit-headless/src/components/combobox/combobox-inline.tsx b/packages/kit-headless/src/components/combobox/combobox-inline.tsx index ac8e604d4..2ca5a5c7f 100644 --- a/packages/kit-headless/src/components/combobox/combobox-inline.tsx +++ b/packages/kit-headless/src/components/combobox/combobox-inline.tsx @@ -1,9 +1,10 @@ -import { type JSXNode, Component } from '@builder.io/qwik'; +import { Component, JSXChildren } from '@builder.io/qwik'; import { HComboboxRootImpl, HComboboxRootImplProps } from './combobox-root'; import { HComboboxItem as InternalComboboxItem } from './combobox-item'; import { HComboboxItemLabel as InternalComboboxItemLabel } from './combobox-item-label'; import { HComboboxEmpty as InternalComboboxEmpty } from './combobox-empty'; import { HComboboxErrorMessage } from './combobox-error-message'; +import { findComponent, processChildren } from '../../utils/inline-component'; export type TItemsMap = Map< number, @@ -13,8 +14,8 @@ export type TItemsMap = Map< export type InternalComboboxProps = { /** When a value is passed, we check if it's an actual item value, and get its index at pre-render time. **/ - _valuePropIndex?: number | null; - _value?: string; + initialIndex?: number | null; + initialValue?: string; /** Checks if the consumer added the label in their JSX */ _label?: boolean; @@ -35,7 +36,7 @@ export const HComboboxRoot: Component { const { - children: myChildren, + children, comboboxItemComponent: UserItem, comboboxItemLabelComponent: UserItemLabel, ...rest @@ -47,121 +48,87 @@ export const HComboboxRoot: Component; + findComponent(HComboboxItem, (itemProps) => { + itemProps._index = currItemIndex; + + isItemDisabled = itemProps.disabled === true; + + if (itemProps.value) { + givenItemValue = itemProps.value as string; + } + + // the default case isn't handled here, so we need to process the children to get to the label component + if (itemProps.children) { + return processChildren(itemProps.children as JSXChildren); + } + }); + + findComponent(HComboboxItemLabel, (labelProps) => { + const displayValue = labelProps.children as string; - while (childrenToProcess.length) { - const child = childrenToProcess.shift(); + // distinct value, or the display value is the same as the value + const value = (givenItemValue !== null ? givenItemValue : displayValue) as string; - if (!child) { - continue; + itemsMap.set(currItemIndex, { value, displayValue, disabled: isItemDisabled }); + + if (props.value && props.multiple) { + throw new Error( + `Qwik UI: When in multiple selection mode, the value prop is disabled. Use the bind:value prop's initial signal value instead.`, + ); } - if (Array.isArray(child)) { - childrenToProcess.unshift(...child); - continue; + // if the current option value is equal to the initial value + if (value === props.value) { + // minus one because it is incremented already in SelectOption + initialIndex = currItemIndex; + initialValue = value; } - switch (child.type) { - case HComboboxItem: { - // get the index of the current option - child.props._index = currItemIndex; - - isItemDisabled = child.props.disabled === true; - - if (child.props.value) { - givenItemValue = child.props.value; - } - - // the default case isn't handled here, so we need to process the children to get to the label component - if (child.props.children) { - const childChildren = Array.isArray(child.props.children) - ? [...child.props.children] - : [child.props.children]; - childrenToProcess.unshift(...childChildren); - } - - break; - } - - case HComboboxItemLabel: { - const displayValue = child.props.children as string; - - // distinct value, or the display value is the same as the value - const value = (givenItemValue !== null ? givenItemValue : displayValue) as string; - - itemsMap.set(currItemIndex, { value, displayValue, disabled: isItemDisabled }); - - if (props.value && props.multiple) { - throw new Error( - `Qwik UI: When in multiple selection mode, the value prop is disabled. Use the bind:value prop's initial signal value instead.`, - ); - } - - // if the current option value is equal to the initial value - if (value === props.value) { - // minus one because it is incremented already in SelectOption - valuePropIndex = currItemIndex; - _value = value; - } - - const isString = typeof child.props.children === 'string'; - - if (!isString) { - throw new Error( - `Qwik UI: select item label passed was not a string. It was a ${typeof child - .props.children}.`, - ); - } - - // increment after processing children - currItemIndex++; - - break; - } - - case HComboboxEmpty: { - hasEmptyComp = true; - break; - } - - case HComboboxErrorMessage: { - hasErrorComp = true; - break; - } - - default: { - if (child) { - const anyChildren = Array.isArray(child.children) - ? [...child.children] - : [child.children]; - childrenToProcess.unshift(...(anyChildren as JSXNode[])); - } - - break; - } + const isString = typeof labelProps.children === 'string'; + + if (!isString) { + throw new Error( + `Qwik UI: select item label passed was not a string. It was a ${typeof labelProps.children}.`, + ); } - } + + // increment after processing children + currItemIndex++; + }); + + findComponent(HComboboxEmpty, () => { + hasEmptyComp = true; + }); + + findComponent(HComboboxErrorMessage, () => { + hasErrorComp = true; + }); + + processChildren(children); return ( - {props.children} + {children} ); }; diff --git a/packages/kit-headless/src/components/combobox/combobox-input.tsx b/packages/kit-headless/src/components/combobox/combobox-input.tsx index e4bb482f7..c0c5bd7e5 100644 --- a/packages/kit-headless/src/components/combobox/combobox-input.tsx +++ b/packages/kit-headless/src/components/combobox/combobox-input.tsx @@ -15,7 +15,7 @@ import { useCombinedRef } from '../../hooks/combined-refs'; type HComboboxInputProps = PropsOf<'input'>; export const HComboboxInput = component$( - ({ 'bind:value': inputValueSig, ...props }: HComboboxInputProps) => { + ({ 'bind:value': givenInputValueSig, ...props }: HComboboxInputProps) => { const context = useContext(comboboxContextId); const contextRefOpts = { context, givenContextRef: context.inputRef }; const inputRef = useCombinedRef(props.ref, contextRefOpts); @@ -25,7 +25,7 @@ export const HComboboxInput = component$( const labelId = `${context.localId}-label`; const descriptionId = `${context.localId}-description`; const errorMessageId = `${context.localId}-error-message`; - const initialValueSig = useSignal(); + const initialValueSig = useSignal(); const wasEmptyBeforeBackspaceSig = useSignal(false); const isInputResetSig = useSignal(false); @@ -54,6 +54,8 @@ export const HComboboxInput = component$( const handleKeyDown$ = $(async (e: KeyboardEvent) => { if (!context.itemsMapSig.value) return; + context.isKeyboardFocusSig.value = true; + if (e.key === 'Backspace') { // check if input was empty before backspace wasEmptyBeforeBackspaceSig.value = context.inputValueSig.value.length === 0; @@ -107,7 +109,7 @@ export const HComboboxInput = component$( if (!context.isListboxOpenSig.value) break; await selectionManager$(context.highlightedIndexSig.value, 'toggle'); - if (context.selectedValueSetSig.value.size <= 0) break; + if (context.selectedValuesSig.value.length <= 0) break; if (!context.multiple) { context.isListboxOpenSig.value = false; @@ -142,9 +144,15 @@ export const HComboboxInput = component$( context.highlightedIndexSig.value = null; isInputResetSig.value = false; + if (target.value === '' && !context.multiple) { + context.selectedValuesSig.value = ''; + } else { + context.isListboxOpenSig.value = true; + } + // bind:value on the input - if (inputValueSig) { - inputValueSig.value = el.value; + if (givenInputValueSig) { + givenInputValueSig.value = el.value; context.inputValueSig.value = el.value; } }); @@ -153,64 +161,51 @@ export const HComboboxInput = component$( if (e.key === 'Backspace') { // removeOnBackspace if (!context.multiple) return; - if (context.selectedValueSetSig.value.size === 0) return; + if (context.selectedValuesSig.value.length === 0) return; if (!context.removeOnBackspace) return; if ( (wasEmptyBeforeBackspaceSig.value || isInputResetSig.value) && context.inputValueSig.value.length === 0 ) { - const selectedValuesArray = [...context.selectedValueSetSig.value]; - selectedValuesArray.pop(); // Remove the last element - context.selectedValueSetSig.value = new Set(selectedValuesArray); + const selectedValuesArray = [...context.selectedValuesSig.value]; + selectedValuesArray.pop(); + + context.selectedValuesSig.value = selectedValuesArray; } } }); /** Users may pass an initial value to bind:value on the input, use the value, or bind:value props on the root. */ - useTask$(function initialState() { - const selectedValue = - context.selectedValueSetSig.value.size > 0 - ? context.selectedValueSetSig.value.values().next().value - : ''; - - let initialValue = ''; - let matchingItemValue = null; - let matchingItemIndex = -1; - - context.itemsMapSig.value.forEach((item, index) => { - if (item.value === selectedValue) { - initialValue = item.displayValue; - } - if (inputValueSig?.value && item.displayValue === inputValueSig.value) { - matchingItemValue = item.value; - matchingItemIndex = index; + useTask$(function getInitialValues() { + const { selectedValuesSig, multiple, itemsMapSig, highlightedIndexSig } = context; + const selectedValues = selectedValuesSig.value; + const initialValue: string[] = []; + + for (const [index, item] of itemsMapSig.value.entries()) { + const isSelected = multiple + ? Array.isArray(selectedValues) && selectedValues.includes(item.value) + : item.value === selectedValues; + + if (isSelected) { + initialValue.push(item.displayValue); + highlightedIndexSig.value = index; + // end the loop when we've found our selected value in single mode + if (!multiple) break; } - }); - - initialValueSig.value = initialValue; - - if (matchingItemValue !== null) { - context.selectedValueSetSig.value.add(matchingItemValue); - context.highlightedIndexSig.value = matchingItemIndex; } - }); - const computedInputValueSig = useComputed$(() => { - if (initialValueSig.value) { - return initialValueSig.value; - } else { - if (inputValueSig?.value) { - return inputValueSig.value; - } - return ''; - } + initialValueSig.value = multiple ? initialValue : initialValue[0] || ''; }); + const inputValueSig = useComputed$( + () => givenInputValueSig?.value ?? initialValueSig.value ?? '', + ); + return ( & { }; export const HComboboxItem = component$((props: HComboboxItemProps) => { + const context = useContext(comboboxContextId); + const debounceTimeoutSig = useSignal(); + if (props._index === undefined) { throw new Error('Qwik UI: Combobox component item cannot find its proper index.'); } - const context = useContext(comboboxContextId); + const localIndexSig = useComputed$(() => props._index ?? -1); const itemRef = useCombinedRef(props.ref); - const itemLabelId = `${context.localId}-${props._index ?? -1}-item-label`; const { selectionManager$, filterManager$ } = useCombobox(); + + const itemLabelId = `${context.localId}-${localIndexSig.value}-item-label`; + const isDisabledSig = useComputed$(() => - context.disabledIndexSetSig.value.has(props._index ?? -1), + context.disabledIndexSetSig.value.has(localIndexSig.value), ); const isSelectedSig = useComputed$(() => { - const index = props._index ?? -1; + const index = localIndexSig.value; const selectedValue = context.itemsMapSig.value.get(index)?.value; - return !isDisabledSig.value && context.selectedValueSetSig.value.has(selectedValue!); + if (!selectedValue || isDisabledSig.value) return false; + + if (Array.isArray(context.selectedValuesSig.value)) { + return context.selectedValuesSig.value.includes(selectedValue); + } else { + return context.selectedValuesSig.value === selectedValue; + } }); const isHighlightedSig = useComputed$(() => { if (isDisabledSig.value) return; - if (context.highlightedIndexSig.value === props._index ?? -1) { + if (context.highlightedIndexSig.value === localIndexSig.value) { return true; } else { return false; @@ -64,7 +76,7 @@ export const HComboboxItem = component$((props: HComboboxItemProps) => { const containerRect = context.panelRef.value?.getBoundingClientRect(); const itemRect = itemRef.value?.getBoundingClientRect(); - if (!containerRect || !itemRect || context.isMouseOverPopupSig.value) return; + if (!containerRect || !itemRect || !context.isKeyboardFocusSig.value) return; // Calculates the offset to center the item within the container const offset = @@ -78,9 +90,9 @@ export const HComboboxItem = component$((props: HComboboxItemProps) => { }); const handleClick$ = $(async () => { - if (isDisabledSig.value || (props._index ?? -1) === null) return; + if (isDisabledSig.value || localIndexSig.value === null) return; - await selectionManager$(props._index ?? -1, 'toggle'); + await selectionManager$(localIndexSig.value, 'toggle'); if (!isSelectedSig.value && !context.multiple) { context.isListboxOpenSig.value = false; @@ -88,10 +100,10 @@ export const HComboboxItem = component$((props: HComboboxItemProps) => { }); const handlePointerOver$ = $(() => { - if (isDisabledSig.value) return; + if (isDisabledSig.value || context.isKeyboardFocusSig.value) return; if (props._index !== undefined && context.isMouseOverPopupSig.value) { - context.highlightedIndexSig.value = props._index ?? -1; + context.highlightedIndexSig.value = localIndexSig.value; } }); @@ -107,50 +119,56 @@ export const HComboboxItem = component$((props: HComboboxItemProps) => { useContextProvider(comboboxItemContextId, itemContext); - useTask$(async function navigationTask({ track, cleanup }) { + useTask$(function handleScrolling({ track, cleanup }) { track(() => context.highlightedIndexSig.value); if (isServer || !context.panelRef.value) return; - if (props._index !== context.highlightedIndexSig.value) return; const hasScrollbar = context.panelRef.value.scrollHeight > context.panelRef.value.clientHeight; - if (!hasScrollbar) { - return; + if (!hasScrollbar) return; + + if (debounceTimeoutSig.value !== undefined) { + clearTimeout(debounceTimeoutSig.value); } - const observer = new IntersectionObserver(checkVisibility$, { - root: context.panelRef.value, - threshold: 1.0, - }); + debounceTimeoutSig.value = setTimeout(() => { + if (props._index !== context.highlightedIndexSig.value) return; - cleanup(() => observer?.disconnect()); + const observer = new IntersectionObserver(checkVisibility$, { + root: context.panelRef.value, + threshold: 1.0, + }); - if (itemRef.value) { - observer.observe(itemRef.value); - } + cleanup(() => observer?.disconnect()); + + if (itemRef.value) { + observer.observe(itemRef.value); + } + }, 100); + + cleanup(() => { + if (debounceTimeoutSig.value !== undefined) { + clearTimeout(debounceTimeoutSig.value); + } + }); }); - useTask$(async ({ track }) => { + useTask$(async function defaultFilter({ track }) { track(() => context.inputValueSig.value); if (isServer || !itemRef.value) return; - if (context.inputValueSig.value === '' && !context.multiple) { - context.selectedValueSetSig.value = new Set(); - } else { - context.isListboxOpenSig.value = true; - } + const displayValue = context.itemsMapSig.value.get(localIndexSig.value)?.displayValue; let isVisible; - const displayValue = context.itemsMapSig.value.get(props._index ?? -1)?.displayValue; if (!displayValue) return; if (context.filter) { const lowerCaseDisplayValue = displayValue?.toLowerCase(); const lowerCaseInputValue = context.inputValueSig.value.toLowerCase(); isVisible = lowerCaseDisplayValue?.includes(lowerCaseInputValue); - filterManager$(!!isVisible, itemRef, props._index ?? -1); + filterManager$(!!isVisible, itemRef, localIndexSig.value); } }); @@ -167,7 +185,7 @@ export const HComboboxItem = component$((props: HComboboxItemProps) => { role="option" ref={itemRef} tabIndex={-1} - id={`${context.localId}-${props._index ?? -1}`} + id={`${context.localId}-${localIndexSig.value}`} aria-selected={isSelectedSig.value} aria-disabled={isDisabledSig.value === true ? 'true' : 'false'} data-disabled={isDisabledSig.value ? '' : undefined} @@ -177,7 +195,7 @@ export const HComboboxItem = component$((props: HComboboxItemProps) => { onFocus$={[handleFocus$, props.onFocus$]} aria-labelledby={itemLabelId} data-item - onClick$={handleClick$} + onClick$={[handleClick$, props.onClick$]} {...props} > diff --git a/packages/kit-headless/src/components/combobox/combobox-popover.tsx b/packages/kit-headless/src/components/combobox/combobox-popover.tsx index b935f42f2..aafd115ca 100644 --- a/packages/kit-headless/src/components/combobox/combobox-popover.tsx +++ b/packages/kit-headless/src/components/combobox/combobox-popover.tsx @@ -14,7 +14,6 @@ import { comboboxContextId } from './combobox-context'; import { HPopoverRoot } from '../popover/popover-root'; import { isServer } from '@builder.io/qwik/build'; import { useCombinedRef } from '../../hooks/combined-refs'; -import { useDebouncer } from '../../hooks/use-debouncer'; export const HComboboxPopover = component$>((props) => { const context = useContext(comboboxContextId); @@ -85,12 +84,10 @@ export const HComboboxPopover = component$>((props) initialLoadSig.value = false; }); - const resetScrollMove = useDebouncer( - $(() => { - context.isMouseOverPopupSig.value = false; - }), - 650, - ); + const handleMouseEnter$ = $(() => { + context.isKeyboardFocusSig.value = false; + context.isMouseOverPopupSig.value = true; + }); return ( >((props) role="listbox" aria-expanded={context.isListboxOpenSig.value ? 'true' : 'false'} aria-multiselectable={context.multiple ? 'true' : undefined} - onMouseMove$={async () => { - context.isMouseOverPopupSig.value = true; - - await resetScrollMove(); - }} - onMouseOut$={() => (context.isMouseOverPopupSig.value = false)} - onKeyDown$={() => (context.isMouseOverPopupSig.value = true)} + onMouseEnter$={[handleMouseEnter$, props.onMouseEnter$]} {...rest} > diff --git a/packages/kit-headless/src/components/combobox/combobox-root.tsx b/packages/kit-headless/src/components/combobox/combobox-root.tsx index 6e8a03477..b901f662f 100644 --- a/packages/kit-headless/src/components/combobox/combobox-root.tsx +++ b/packages/kit-headless/src/components/combobox/combobox-root.tsx @@ -12,8 +12,8 @@ import { } from '@builder.io/qwik'; import { ComboboxContext, comboboxContextId } from './combobox-context'; import { InternalComboboxProps } from './combobox-inline'; -import { useCombobox } from './use-combobox'; import { useCombinedRef } from '../../hooks/combined-refs'; +import { useBoundSignal } from '../../utils/bound-signal'; export type TMultiple = M extends true ? string[] : string; @@ -118,13 +118,12 @@ export type HComboboxRootImplProps = Omit< export const HComboboxRootImpl = component$< HComboboxRootImplProps & InternalComboboxProps >((props: HComboboxRootImplProps & InternalComboboxProps) => { - const isListboxOpenSig = useSignal(false); const { onInput$, onChange$, onOpenChange$, - _valuePropIndex: givenValuePropIndex, - _value, + initialIndex, + initialValue, loop: givenLoop, scrollOptions: givenScrollOptions, multiple = false, @@ -132,15 +131,23 @@ export const HComboboxRootImpl = component$< filter = true, _itemsMap, hasEmptyComp, - hasErrorComp, removeOnBackspace = false, name, required, - disabled, + 'bind:value': givenValueSig, + 'bind:open': givenOpenSig, + 'bind:displayValue': givenDisplayValueSig, ...rest } = props; // source of truth + const isListboxOpenSig = useBoundSignal(givenOpenSig, false); + const displayValuesSig = useBoundSignal(givenDisplayValueSig, []); + const selectedValuesSig = useBoundSignal( + givenValueSig, + multiple ? (initialValue ? [initialValue] : []) : initialValue || '', + ); + const itemsMapSig = useComputed$(() => { return props._itemsMap ?? new Map(); }); @@ -154,20 +161,24 @@ export const HComboboxRootImpl = component$< const loop = givenLoop ?? false; const localId = useId(); /** We use selected values to preserve the item state when the consumer filters the items. */ - const selectedValueSetSig = useSignal>(new Set(_value ? [_value] : [])); + const selectedValueSetSig = useSignal>( + new Set(initialValue ? [initialValue] : []), + ); + const disabledIndexSetSig = useSignal>(new Set()); + const filteredIndexSetSig = useSignal>(new Set()); const isMouseOverPopupSig = useSignal(false); - const isDisabledSig = useSignal(disabled ?? false); - const highlightedIndexSig = useSignal(givenValuePropIndex ?? null); + const highlightedIndexSig = useSignal(initialIndex ?? null); const initialLoadSig = useSignal(true); - const currDisplayValueSig = useSignal(); const scrollOptions = givenScrollOptions ?? { behavior: 'smooth', block: 'center', inline: 'nearest', }; const inputValueSig = useSignal(inputRef.value?.value ?? ''); - const isInvalidSig = useSignal(hasErrorComp ?? false); + const isDisabledSig = useComputed$(() => props.disabled ?? false); + const isInvalidSig = useComputed$(() => props.hasErrorComp ?? false); + const isKeyboardFocusSig = useSignal(false); // check any initial disabled items before the computed read below useTask$(() => { @@ -181,19 +192,19 @@ export const HComboboxRootImpl = component$< disabledIndexSetSig.value = disabledIndices; }); - useTask$(({ track }) => { - isInvalidSig.value = track(() => props.hasErrorComp ?? false); - }); - - const hasVisibleItemsSig = useComputed$(() => { - return itemsMapSig.value.size !== disabledIndexSetSig.value.size; + const isNoItemsSig = useComputed$(() => { + return ( + itemsMapSig.value.size === filteredIndexSetSig.value.size || + itemsMapSig.value.size === 0 + ); }); useTask$(function closeIfEmptyComp({ track }) { - track(() => disabledIndexSetSig.value); + track(() => itemsMapSig.value); + track(() => filteredIndexSetSig.value); // automatically closes the listbox if there are no visible items AND the combobox does not have an empty component - if (!hasEmptyComp && !hasVisibleItemsSig.value) { + if (!hasEmptyComp && isNoItemsSig.value) { isListboxOpenSig.value = false; } }); @@ -210,19 +221,22 @@ export const HComboboxRootImpl = component$< controlRef, localId, highlightedIndexSig, + selectedValuesSig, selectedValueSetSig, disabledIndexSetSig, - currDisplayValueSig, + filteredIndexSetSig, + displayValuesSig, isMouseOverPopupSig, + isKeyboardFocusSig, initialLoadSig, removeOnBackspace, filter, loop, multiple, - _value, + initialValue, scrollOptions, placeholder, - hasVisibleItemsSig, + isNoItemsSig, name, required, isDisabledSig, @@ -231,104 +245,50 @@ export const HComboboxRootImpl = component$< useContextProvider(comboboxContextId, context); - const { selectionManager$ } = useCombobox(); - - useTask$(async function reactiveUserValue({ track }) { - const bindValueSig = props['bind:value']; - if (!bindValueSig) return; - track(() => bindValueSig.value); - - for (const [index, item] of itemsMapSig.value) { - if (bindValueSig.value?.includes(item.value)) { - await selectionManager$(index, 'add'); - - if (initialLoadSig.value) { - // for both SSR and CSR, we need to set the initial index - context.highlightedIndexSig.value = index; - } - } else { - await selectionManager$(index, 'remove'); - } - } - }); - - useTask$(function reactiveUserOpen({ track }) { - const bindOpenSig = props['bind:open']; - if (!bindOpenSig) return; - track(() => bindOpenSig.value); - - isListboxOpenSig.value = bindOpenSig.value ?? isListboxOpenSig.value; - }); - - useTask$(function onOpenChangeTask({ track }) { - const bindOpenSig = props['bind:open']; + useTask$(async function handleOpenChange({ track }) { track(() => isListboxOpenSig.value); if (!initialLoadSig.value) { - onOpenChange$?.(isListboxOpenSig.value); - } - - // sync the user's given signal for the open state - if (bindOpenSig && bindOpenSig.value !== isListboxOpenSig.value) { - bindOpenSig.value = isListboxOpenSig.value; + await onOpenChange$?.(isListboxOpenSig.value); } }); - useTask$(function onInputTask({ track }) { + useTask$(async function handleInput({ track }) { track(() => context.inputValueSig.value); if (!initialLoadSig.value) { - onInput$?.(context.inputValueSig.value); + await onInput$?.(context.inputValueSig.value); } }); - useTask$(async function updateConsumerProps({ track }) { - const bindValueSig = props['bind:value']; - const bindDisplayTextSig = props['bind:displayValue']; - track(() => selectedValueSetSig.value); + useTask$(async function handleChange({ track }) { + track(() => selectedValuesSig.value); - const values = Array.from(selectedValueSetSig.value); - const displayValues = []; - - for (const value of values) { - for (const item of context.itemsMapSig.value.values()) { - if (item.value === value) { - displayValues.push(item.displayValue); - break; - } - } + if (!initialLoadSig.value && selectedValuesSig.value.length > 0) { + await onChange$?.(selectedValuesSig.value); } - if (onChange$ && selectedValueSetSig.value.size > 0) { - await onChange$(context.multiple ? values : values[0]); - } + const selectedValues = Array.isArray(selectedValuesSig.value) + ? selectedValuesSig.value + : [selectedValuesSig.value]; - // sync the user's given signal when an option is selected - if (bindValueSig && bindValueSig.value) { - const currUserSigValues = JSON.stringify(bindValueSig.value); - const newUserSigValues = JSON.stringify(values); + const displayValues: string[] = []; - if (currUserSigValues !== newUserSigValues) { - if (context.multiple) { - bindValueSig.value = values; - } else { - bindValueSig.value = values[0]; + for (const [index, item] of itemsMapSig.value.entries()) { + if (selectedValues.includes(item.value)) { + displayValues.push(item.displayValue); + + /* avoid "highlight jumping" on multiple selections */ + if (context.isListboxOpenSig.value === false) { + context.highlightedIndexSig.value = index; } } } - context.currDisplayValueSig.value = displayValues; - - // sync the user's given signal for the display value - if (bindDisplayTextSig && context.currDisplayValueSig.value) { - bindDisplayTextSig.value = context.multiple - ? context.currDisplayValueSig.value - : context.currDisplayValueSig.value[0]; - } - }); + displayValuesSig.value = displayValues; - useTask$(({ track }) => { - isDisabledSig.value = track(() => disabled ?? false); + if (multiple || !context.inputRef.value || !displayValues[0]) return; + context.inputRef.value.value = displayValues[0]; }); useTask$(() => { diff --git a/packages/kit-headless/src/components/combobox/combobox.test.ts b/packages/kit-headless/src/components/combobox/combobox.test.ts index 80cee0898..012bc4a1a 100644 --- a/packages/kit-headless/src/components/combobox/combobox.test.ts +++ b/packages/kit-headless/src/components/combobox/combobox.test.ts @@ -238,6 +238,32 @@ test.describe('Keyboard Behavior', () => { await d.getInput().press('ArrowUp'); await expect(d.getItemAt(1)).toHaveAttribute('data-highlighted'); }); + + test(`GIVEN a combobox with a selected value via mouse + WHEN using the arrow keys to select another option + THEN the latest selected option should be highlighted when the menu is opened again`, async ({ + page, + }) => { + const { driver: d } = await setup(page, 'hero'); + + // initial mouse selection + await d.openListbox('click'); + await d.getItemAt(2).click(); + + await d.openListbox('ArrowDown'); + await expect(d.getItemAt(2)).toHaveAttribute('data-highlighted'); + + // selection via arrow keys + await d.getInput().press('ArrowDown'); + await expect(d.getItemAt(3)).toHaveAttribute('data-highlighted'); + + await d.getInput().press('Enter'); + await expect(d.getItemAt(3)).toHaveAttribute('data-selected'); + + // latest selected option should be highlighted when the menu is opened again + await d.openListbox('ArrowDown'); + await expect(d.getItemAt(3)).toHaveAttribute('data-highlighted'); + }); }); test.describe('selecting options', () => { @@ -589,49 +615,39 @@ test.describe('Props', () => { }); test(`GIVEN a reactive combobox - WHEN the user empties the input with selecting all text and backspace - THEN there should be no selected value`, async ({ page }) => { + WHEN the user empties the input with selecting all text and backspace + THEN there should be no selected value`, async ({ page }) => { const { driver: d } = await setup(page, 'reactive'); - const apricot = (await d.getItemAt(1).textContent()) ?? ''; - - await expect(d.getInput()).toHaveValue(apricot); + await expect(d.getInput()).toHaveValue('Apricot'); + await expect(page.getByRole('paragraph')).toContainText('Apricot'); await d.getInput().clear(); await expect(d.getInput()).toHaveValue(''); - await expect( - page.getByRole('paragraph', { name: 'Your favorite fruit is:' }), - ).toBeVisible(); + await expect(page.getByRole('paragraph')).not.toContainText('Apricot'); }); test(`GIVEN a reactive combobox - WHEN the user empties the input and selects a new value - THEN that should update the given signal`, async ({ page }) => { + WHEN the user empties the input and selects a new value + THEN that should update the given signal`, async ({ page }) => { const { driver: d } = await setup(page, 'reactive'); - const apricot = (await d.getItemAt(1).textContent()) ?? ''; - - await expect(d.getInput()).toHaveValue(apricot); + await expect(d.getInput()).toHaveValue('Apricot'); + await expect(page.getByRole('paragraph')).toContainText('Apricot'); await d.getInput().clear(); await expect(d.getInput()).toHaveValue(''); - await expect( - page.getByRole('paragraph', { name: 'Your favorite fruit is:' }), - ).toBeVisible(); + await expect(page.getByRole('paragraph')).not.toContainText('Apricot'); await d.openListbox('click'); + await d.getItemAt(0).click(); - await d.getItemAt(1).click(); - - await expect(d.getInput()).toHaveValue('Apricot'); - - await expect( - page.getByRole('paragraph', { name: 'Your favorite fruit is: Apricot' }), - ).toBeVisible(); + await expect(d.getInput()).toHaveValue('Apple'); + await expect(page.getByRole('paragraph')).toContainText('Apple'); }); }); @@ -892,19 +908,6 @@ test.describe('Filtering options', () => { expect(await d.getVisibleItemsLength()).toBe(8); }); - test(`GIVEN a combobox - WHEN an option is selected - AND the typed string does not match the filter function - THEN the option should be unselected`, async ({ page }) => { - const { driver: d } = await setup(page, 'hero'); - await d.openListbox('ArrowDown'); - await d.getInput().press('Enter'); - await expect(d.getItemAt(0)).toHaveAttribute('aria-selected', 'true'); - - await d.getInput().press('z'); - await expect(d.getItemAt(0)).toHaveAttribute('aria-selected', 'false'); - }); - test(`GIVEN a combobox WHEN a custom filter is set THEN it should respect the filter function`, async ({ page }) => { diff --git a/packages/kit-headless/src/components/combobox/use-combobox.tsx b/packages/kit-headless/src/components/combobox/use-combobox.tsx index 3a970b7eb..a1462a1e3 100644 --- a/packages/kit-headless/src/components/combobox/use-combobox.tsx +++ b/packages/kit-headless/src/components/combobox/use-combobox.tsx @@ -1,13 +1,46 @@ import { useContext, $, Signal } from '@builder.io/qwik'; import { comboboxContextId } from './combobox-context'; +class ValueManager { + constructor( + private isMultiple: boolean, + private initialValue: T | T[], + ) {} + + add(value: T): T | T[] { + if (this.isMultiple) { + const currentArray = Array.isArray(this.initialValue) ? [...this.initialValue] : []; + return [...currentArray, value]; + } + return value; + } + + remove(value: T): T | T[] { + if (this.isMultiple && Array.isArray(this.initialValue)) { + return this.initialValue.filter((v) => v !== value); + } + return '' as T; + } + + toggle(value: T): T | T[] { + if (this.isMultiple) { + const currentArray = Array.isArray(this.initialValue) ? [...this.initialValue] : []; + return currentArray.includes(value) + ? currentArray.filter((v) => v !== value) + : [...currentArray, value]; + } + return this.initialValue === value ? ('' as T) : value; + } +} + export function useCombobox() { const context = useContext(comboboxContextId); + const selectionManager$ = $( async (index: number | null, action: 'add' | 'toggle' | 'remove') => { if (index === null) return; - const selectedDisplayValue = context.itemsMapSig.value.get(index)?.displayValue; + const selectedDisplayValue = context.itemsMapSig.value.get(index)?.displayValue; const value = context.itemsMapSig.value.get(index)?.value; if (!value) { @@ -16,49 +49,26 @@ export function useCombobox() { ); } - if (action === 'add') { - if (context.multiple) { - context.selectedValueSetSig.value = new Set([ - ...context.selectedValueSetSig.value, - value, - ]); - } else { - context.selectedValueSetSig.value = new Set([value]); - } - } - if (action === 'toggle') { - if (context.selectedValueSetSig.value.has(value)) { - context.selectedValueSetSig.value = new Set( - [...context.selectedValueSetSig.value].filter( - (selectedValue) => selectedValue !== value, - ), - ); - } else { - if (context.multiple) { - context.selectedValueSetSig.value = new Set([ - ...context.selectedValueSetSig.value, - value, - ]); - } else { - context.selectedValueSetSig.value = new Set([value]); - } - } - } - if (action === 'remove') { - context.selectedValueSetSig.value = new Set( - [...context.selectedValueSetSig.value].filter( - (selectedValue) => selectedValue !== value, - ), - ); - } + const valueManager = new ValueManager( + context.multiple ?? false, + context.selectedValuesSig.value, + ); - if (action === 'add' || action === 'toggle') { - if (!context.inputRef.value) return; - if (!selectedDisplayValue) return; + switch (action) { + case 'add': + context.selectedValuesSig.value = valueManager.add(value); + break; + case 'remove': + context.selectedValuesSig.value = valueManager.remove(value); + return; // Early return for 'remove' action + case 'toggle': + context.selectedValuesSig.value = valueManager.toggle(value); + break; + } - if (!context.multiple && context.selectedValueSetSig.value.has(value)) { - context.inputRef.value.value = selectedDisplayValue; - } + // Update input value for single-select combobox + if (!context.multiple && context.inputRef.value && selectedDisplayValue) { + context.inputRef.value.value = selectedDisplayValue; } }, ); @@ -66,56 +76,81 @@ export function useCombobox() { const filterManager$ = $((isVisible: boolean, itemRef: Signal, index: number) => { if (!itemRef.value) return; + const isDisabled = context.itemsMapSig.value.get(index)?.disabled; + itemRef.value.style.display = isVisible ? '' : 'none'; - context.disabledIndexSetSig.value = new Set( + + context.filteredIndexSetSig.value = new Set( isVisible - ? [...context.disabledIndexSetSig.value].filter( - (selectedIndex) => selectedIndex !== index, + ? [...context.filteredIndexSetSig.value].filter( + (filteredIndex) => filteredIndex !== index, ) - : [...context.disabledIndexSetSig.value, index], + : [...context.filteredIndexSetSig.value, index], ); + + // Update disabledIndexSetSig + if (isDisabled) { + context.disabledIndexSetSig.value = new Set([ + ...context.disabledIndexSetSig.value, + index, + ]); + } else { + context.disabledIndexSetSig.value = new Set( + [...context.disabledIndexSetSig.value].filter( + (disabledIndex) => disabledIndex !== index, + ), + ); + } }); const getNextEnabledItemIndex$ = $((index: number) => { const len = context.itemsMapSig.value.size; - if (len === 1) { - return context.disabledIndexSetSig.value.has(0) ? -1 : 0; - } + if (len === 0) return -1; - let offset = 1; - if (!context.loop && index + 1 >= len) { - return index; - } - while (offset < len) { - const nextIndex = (index + offset) % len; - if (!context.disabledIndexSetSig.value.has(nextIndex)) { - return nextIndex; - } - offset++; - if (!context.loop && index + offset >= len) { - break; + const findNextEnabled = (start: number) => { + for (let i = 0; i < len; i++) { + const nextIndex = (start + i) % len; + if ( + !context.disabledIndexSetSig.value.has(nextIndex) && + !context.filteredIndexSetSig.value.has(nextIndex) + ) { + return nextIndex; + } } + return -1; + }; + + if (index === -1 || len === 1) { + return findNextEnabled(0); } - return index; + + const nextIndex = findNextEnabled(index + 1); + return context.loop || nextIndex > index ? nextIndex : index; }); const getPrevEnabledItemIndex$ = $((index: number) => { - let offset = 1; const len = context.itemsMapSig.value.size; - if (!context.loop && index - 1 < 0) { - return index; - } - while (offset <= len) { - const prevIndex = (index - offset + len) % len; - if (!context.disabledIndexSetSig.value.has(prevIndex)) { - return prevIndex; - } - offset++; - if (!context.loop && index - offset < 0) { - break; + if (len === 0) return -1; + + const findPrevEnabled = (start: number) => { + for (let i = 0; i < len; i++) { + const prevIndex = (start - i + len) % len; + if ( + !context.disabledIndexSetSig.value.has(prevIndex) && + !context.filteredIndexSetSig.value.has(prevIndex) + ) { + return prevIndex; + } } + return -1; + }; + + if (index === -1 || len === 1) { + return findPrevEnabled(len - 1); } - return index; + + const prevIndex = findPrevEnabled(index - 1); + return context.loop || prevIndex < index ? prevIndex : index; }); const getActiveDescendant$ = $((index: number) => {