fix: bound signal updates on the combobox (#968)

* initial tests * get values with arrays instead * reactively update * make selection manager cleaner * remove unnecessary addition * fix: onChange$ only executes on client & simpler disabled state * feat: simpler invalid check * remove early return * feat: simplify inline component * simpler initial value naming * refactor: better context names * fix: combobox closes on enter key * fix: combobox properly resets when empty * get correct initial value reactively * fix: highlight jumping * multiple shows correct display * fix: undefined does not become part of the input * fix: handle proper empty input * feat: use local index instead * fix: last filtered item now gets highlighted on down key * fix: preserve item disabled state * fix: combobox empty now properly shows for both filter and non-filtered comboboxes * fix: removing items on backspace * respect input's given signal first * refactor: remove all the logs * fix: combobox scrolling * fix: form submissions * fix: pw tests * refactor: remove test since behavior is no longer warranted * fix: mouse should not affect initial keyboard highlight * feat: changeset
qwikifiers · Sep 22, 2024 · 6655bad · 6655bad
1 parent c818eec
commit 6655bad
Show file tree

Hide file tree

Showing 11 changed files with 412 additions and 405 deletions.
diff --git a/.changeset/funny-pens-vanish.md 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
@@ -18,14 +18,17 @@ export type ComboboxContext = {
   labelRef: Signal<HTMLDivElement | undefined>;
   controlRef: Signal<HTMLDivElement | undefined>;
   selectedValueSetSig: Signal<Set<string>>;
+  selectedValuesSig: Signal<string | string[]>;
+  filteredIndexSetSig: Signal<Set<number>>;
   highlightedIndexSig: Signal<number | null>;
-  currDisplayValueSig: Signal<string | string[] | undefined>;
+  displayValuesSig: Signal<string | string[] | undefined>;
   isMouseOverPopupSig: Signal<boolean>;
+  isKeyboardFocusSig: Signal<boolean>;
   disabledIndexSetSig: Signal<Set<number>>;
   removeOnBackspace: boolean;
-  hasVisibleItemsSig: Signal<boolean>;
+  isNoItemsSig: Signal<boolean>;
   initialLoadSig: Signal<boolean>;
-  _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
@@ -7,7 +7,7 @@ export const HComboboxEmpty = component$((props: PropsOf<'div'>) => {
   return (
     <div
       data-qui-combobox-empty
-      data-empty={context.hasVisibleItemsSig.value ? undefined : ''}
+      data-empty={context.isNoItemsSig.value ? '' : undefined}
       {...props}
     >
       <Slot />

diff --git 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
@@ -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<InternalComboboxProps & HComboboxRootImplP
   props: InternalComboboxProps & HComboboxRootImplProps,
 ) => {
   const {
-    children: myChildren,
+    children,
     comboboxItemComponent: UserItem,
     comboboxItemLabelComponent: UserItemLabel,
     ...rest
@@ -47,121 +48,87 @@ export const HComboboxRoot: Component<InternalComboboxProps & HComboboxRootImplP
 
   // source of truth
   const itemsMap = new Map();
+
   let currItemIndex = 0;
+  let initialIndex = null;
+  let initialValue;
+
   let isItemDisabled = false;
-  let givenItemValue = null;
-  let valuePropIndex = null;
-  let _value;
+
+  // user adds value prop on Item component
+  let givenItemValue: string | null = null;
+
   let hasEmptyComp = false;
   let hasErrorComp = false;
 
-  const childrenToProcess = (
-    Array.isArray(myChildren) ? [...myChildren] : [myChildren]
-  ) as Array<JSXNode>;
+  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 (
     <HComboboxRootImpl
       {...rest}
-      _valuePropIndex={valuePropIndex}
-      _value={_value}
+      initialIndex={initialIndex}
+      initialValue={initialValue}
       _itemsMap={itemsMap}
       hasEmptyComp={hasEmptyComp}
       hasErrorComp={hasErrorComp}
     >
-      {props.children}
+      {children}
     </HComboboxRootImpl>
   );
 };