Skip to content

Commit 6655bad

Browse files
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
1 parent c818eec commit 6655bad

File tree

11 files changed

+412
-405
lines changed

11 files changed

+412
-405
lines changed

.changeset/funny-pens-vanish.md

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
---
2+
'@qwik-ui/headless': patch
3+
---
4+
5+
# Combobox Improvements
6+
7+
## 🔄 Reactive Improvements
8+
9+
- Better handling of array-based values
10+
- Improved handling of initial and reactive values
11+
12+
## 🐛 Key Bug Fixes
13+
14+
- Fixed highlight jumping issues
15+
- Enhanced empty input handling
16+
- Better filtered item highlighting
17+
18+
## 🖱️ Interaction Enhancements
19+
20+
- Smoother scrolling experience
21+
- Improved keyboard and mouse coordination
22+
23+
## 🚀 Performance Optimizations
24+
25+
- More efficient item filtering
26+
27+
## 🧪 Reliability
28+
29+
- Added tests for reactivity handling, item unselection, and mouse-to-pointer interaction switching
30+
31+
- Improved handling of edge cases in user interactions

packages/kit-headless/src/components/combobox/combobox-context.tsx

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -18,14 +18,17 @@ export type ComboboxContext = {
1818
labelRef: Signal<HTMLDivElement | undefined>;
1919
controlRef: Signal<HTMLDivElement | undefined>;
2020
selectedValueSetSig: Signal<Set<string>>;
21+
selectedValuesSig: Signal<string | string[]>;
22+
filteredIndexSetSig: Signal<Set<number>>;
2123
highlightedIndexSig: Signal<number | null>;
22-
currDisplayValueSig: Signal<string | string[] | undefined>;
24+
displayValuesSig: Signal<string | string[] | undefined>;
2325
isMouseOverPopupSig: Signal<boolean>;
26+
isKeyboardFocusSig: Signal<boolean>;
2427
disabledIndexSetSig: Signal<Set<number>>;
2528
removeOnBackspace: boolean;
26-
hasVisibleItemsSig: Signal<boolean>;
29+
isNoItemsSig: Signal<boolean>;
2730
initialLoadSig: Signal<boolean>;
28-
_value: string | undefined;
31+
initialValue: string | undefined;
2932

3033
loop: boolean;
3134
multiple: boolean | undefined;

packages/kit-headless/src/components/combobox/combobox-empty.tsx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ export const HComboboxEmpty = component$((props: PropsOf<'div'>) => {
77
return (
88
<div
99
data-qui-combobox-empty
10-
data-empty={context.hasVisibleItemsSig.value ? undefined : ''}
10+
data-empty={context.isNoItemsSig.value ? '' : undefined}
1111
{...props}
1212
>
1313
<Slot />

packages/kit-headless/src/components/combobox/combobox-hidden-option.tsx

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ export const ComboboxHiddenSelectOption = component$(
2222
const context = useContext(comboboxContextId);
2323

2424
useTask$(async function modularFormsValidation({ track }) {
25-
track(() => context.selectedValueSetSig.value);
25+
track(() => context.selectedValuesSig.value);
2626

2727
if (isServer || !nativeSelectRef.value || !optionRef.value) return;
2828

@@ -37,7 +37,11 @@ export const ComboboxHiddenSelectOption = component$(
3737
'Qwik UI: value not found when trying to select or unselect an item.',
3838
);
3939
}
40-
optionRef.value.selected = context.selectedValueSetSig.value.has(value);
40+
41+
const selectedValues = context.selectedValuesSig.value;
42+
optionRef.value.selected = Array.isArray(selectedValues)
43+
? selectedValues.includes(value)
44+
: selectedValues === value;
4145
});
4246

4347
return (
Lines changed: 66 additions & 99 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,10 @@
1-
import { type JSXNode, Component } from '@builder.io/qwik';
1+
import { Component, JSXChildren } from '@builder.io/qwik';
22
import { HComboboxRootImpl, HComboboxRootImplProps } from './combobox-root';
33
import { HComboboxItem as InternalComboboxItem } from './combobox-item';
44
import { HComboboxItemLabel as InternalComboboxItemLabel } from './combobox-item-label';
55
import { HComboboxEmpty as InternalComboboxEmpty } from './combobox-empty';
66
import { HComboboxErrorMessage } from './combobox-error-message';
7+
import { findComponent, processChildren } from '../../utils/inline-component';
78

89
export type TItemsMap = Map<
910
number,
@@ -13,8 +14,8 @@ export type TItemsMap = Map<
1314
export type InternalComboboxProps = {
1415
/** When a value is passed, we check if it's an actual item value, and get its index at pre-render time.
1516
**/
16-
_valuePropIndex?: number | null;
17-
_value?: string;
17+
initialIndex?: number | null;
18+
initialValue?: string;
1819

1920
/** Checks if the consumer added the label in their JSX */
2021
_label?: boolean;
@@ -35,7 +36,7 @@ export const HComboboxRoot: Component<InternalComboboxProps & HComboboxRootImplP
3536
props: InternalComboboxProps & HComboboxRootImplProps,
3637
) => {
3738
const {
38-
children: myChildren,
39+
children,
3940
comboboxItemComponent: UserItem,
4041
comboboxItemLabelComponent: UserItemLabel,
4142
...rest
@@ -47,121 +48,87 @@ export const HComboboxRoot: Component<InternalComboboxProps & HComboboxRootImplP
4748

4849
// source of truth
4950
const itemsMap = new Map();
51+
5052
let currItemIndex = 0;
53+
let initialIndex = null;
54+
let initialValue;
55+
5156
let isItemDisabled = false;
52-
let givenItemValue = null;
53-
let valuePropIndex = null;
54-
let _value;
57+
58+
// user adds value prop on Item component
59+
let givenItemValue: string | null = null;
60+
5561
let hasEmptyComp = false;
5662
let hasErrorComp = false;
5763

58-
const childrenToProcess = (
59-
Array.isArray(myChildren) ? [...myChildren] : [myChildren]
60-
) as Array<JSXNode>;
64+
findComponent(HComboboxItem, (itemProps) => {
65+
itemProps._index = currItemIndex;
66+
67+
isItemDisabled = itemProps.disabled === true;
68+
69+
if (itemProps.value) {
70+
givenItemValue = itemProps.value as string;
71+
}
72+
73+
// the default case isn't handled here, so we need to process the children to get to the label component
74+
if (itemProps.children) {
75+
return processChildren(itemProps.children as JSXChildren);
76+
}
77+
});
78+
79+
findComponent(HComboboxItemLabel, (labelProps) => {
80+
const displayValue = labelProps.children as string;
6181

62-
while (childrenToProcess.length) {
63-
const child = childrenToProcess.shift();
82+
// distinct value, or the display value is the same as the value
83+
const value = (givenItemValue !== null ? givenItemValue : displayValue) as string;
6484

65-
if (!child) {
66-
continue;
85+
itemsMap.set(currItemIndex, { value, displayValue, disabled: isItemDisabled });
86+
87+
if (props.value && props.multiple) {
88+
throw new Error(
89+
`Qwik UI: When in multiple selection mode, the value prop is disabled. Use the bind:value prop's initial signal value instead.`,
90+
);
6791
}
6892

69-
if (Array.isArray(child)) {
70-
childrenToProcess.unshift(...child);
71-
continue;
93+
// if the current option value is equal to the initial value
94+
if (value === props.value) {
95+
// minus one because it is incremented already in SelectOption
96+
initialIndex = currItemIndex;
97+
initialValue = value;
7298
}
7399

74-
switch (child.type) {
75-
case HComboboxItem: {
76-
// get the index of the current option
77-
child.props._index = currItemIndex;
78-
79-
isItemDisabled = child.props.disabled === true;
80-
81-
if (child.props.value) {
82-
givenItemValue = child.props.value;
83-
}
84-
85-
// the default case isn't handled here, so we need to process the children to get to the label component
86-
if (child.props.children) {
87-
const childChildren = Array.isArray(child.props.children)
88-
? [...child.props.children]
89-
: [child.props.children];
90-
childrenToProcess.unshift(...childChildren);
91-
}
92-
93-
break;
94-
}
95-
96-
case HComboboxItemLabel: {
97-
const displayValue = child.props.children as string;
98-
99-
// distinct value, or the display value is the same as the value
100-
const value = (givenItemValue !== null ? givenItemValue : displayValue) as string;
101-
102-
itemsMap.set(currItemIndex, { value, displayValue, disabled: isItemDisabled });
103-
104-
if (props.value && props.multiple) {
105-
throw new Error(
106-
`Qwik UI: When in multiple selection mode, the value prop is disabled. Use the bind:value prop's initial signal value instead.`,
107-
);
108-
}
109-
110-
// if the current option value is equal to the initial value
111-
if (value === props.value) {
112-
// minus one because it is incremented already in SelectOption
113-
valuePropIndex = currItemIndex;
114-
_value = value;
115-
}
116-
117-
const isString = typeof child.props.children === 'string';
118-
119-
if (!isString) {
120-
throw new Error(
121-
`Qwik UI: select item label passed was not a string. It was a ${typeof child
122-
.props.children}.`,
123-
);
124-
}
125-
126-
// increment after processing children
127-
currItemIndex++;
128-
129-
break;
130-
}
131-
132-
case HComboboxEmpty: {
133-
hasEmptyComp = true;
134-
break;
135-
}
136-
137-
case HComboboxErrorMessage: {
138-
hasErrorComp = true;
139-
break;
140-
}
141-
142-
default: {
143-
if (child) {
144-
const anyChildren = Array.isArray(child.children)
145-
? [...child.children]
146-
: [child.children];
147-
childrenToProcess.unshift(...(anyChildren as JSXNode[]));
148-
}
149-
150-
break;
151-
}
100+
const isString = typeof labelProps.children === 'string';
101+
102+
if (!isString) {
103+
throw new Error(
104+
`Qwik UI: select item label passed was not a string. It was a ${typeof labelProps.children}.`,
105+
);
152106
}
153-
}
107+
108+
// increment after processing children
109+
currItemIndex++;
110+
});
111+
112+
findComponent(HComboboxEmpty, () => {
113+
hasEmptyComp = true;
114+
});
115+
116+
findComponent(HComboboxErrorMessage, () => {
117+
hasErrorComp = true;
118+
});
119+
120+
processChildren(children);
154121

155122
return (
156123
<HComboboxRootImpl
157124
{...rest}
158-
_valuePropIndex={valuePropIndex}
159-
_value={_value}
125+
initialIndex={initialIndex}
126+
initialValue={initialValue}
160127
_itemsMap={itemsMap}
161128
hasEmptyComp={hasEmptyComp}
162129
hasErrorComp={hasErrorComp}
163130
>
164-
{props.children}
131+
{children}
165132
</HComboboxRootImpl>
166133
);
167134
};

0 commit comments

Comments
 (0)