[Combobox] "Input props" support

[nathanmmiller]

Area

UI Components

The problem

The use case is basically as follows: We have a multi-select, free-form-allowed combobox that we want to support the following behavior:

• When a user types in the input, it should filter the combobox (trivially already supported)
• When a user selects an item (via keyboard or mouse), the input should remain as it was and the combobox should remain open and filtered
• When a user presses the Esc key, the input should empty and the combobox should remain open but become unfiltered
• When the combobox loses focus (user clicks outside or tabs to another field), the input should NOT add another item with the remaining text.

So for example, there could be the following scenario. Items in Combobox: Red, Blue, Baby Blue, Green, Mauve User types: "b" Combobox shows Blue, Baby Blue User arrows down to Blue and selects it "Blue" is selected, input still shows "b" and Combobox still shows Blue, Baby Blue User hits [Esc] Combobox shows all options, input is blank User types M Combobox shows "Mauve" User clicks outside the combobox Combobox input is cleared, only Blue is selected.

The solution

Currently, in the older design system we currently use, there's a concept of "InputProps" that control the Input of the combobox. We use something like this (not real code):

InputProps={{

    disableAddOnBlur: true, //This allows us to ensure that we don't add the typed text as an item when we lose focus

    onInputChange: a function that allows us to control the input text (we use a useState, call it [input, setInput] for this),

    onInputBlur: we both control the popper and reset the input text on this (since we override this we need to control the open state of the combobox) (to reset the input text, we would do for example setInput("").),

    onInputFocus: we open the combobox with this, only necessary because of the note in onInputBlur,

    onKeyDown: we hijack the Esc key to clear the input text here (by doing, for example, setInput("") on event.key = Esc)

}}

Additionally, the combobox itself also has an "inputValue" prop that we then can feed with the "input" state from above. And there's also an "allowFreeForm" that this all ties into, allowing us to actually "select" options that aren't there when the text controlled by "input" is [Enter]'d.

Alternatives and examples

I can provide examples internal to JPMC if someone pings me.

Are you a JPMorgan Chase & Co. employee?

• [X] I am an employee of JPMorgan Chase & Co.

[origami-z]

allowFreeForm is in #2409

Currently Combobox has inputProps which supports some of these

[nathanmmiller]

allowFreeForm is in #2409

Currently Combobox has inputProps which supports some of these

Thank you @origami-z - I've subscribed to #2409 and will patiently await that. The rest of these - are inputProps documented, or rather, is there a list of which of the "some" are supported and which of the others should be part of this feature request?

[nathanmmiller]

OK I've played around with inputProps and also with some of the events and I'm able to do most of this, though there are still some missing pieces. I think this is the list of what's missing to be able to support this use case:

• ability to not close the floating ui on "Esc" - I think a change to ComboBox::handleOpenChange could work here - I would propose a prop like, "ignoreEsc" that would then do something like: if (ignoreEsc && reason === "escape-key") return; [I will happily raise a PR for this if y'all agree with this approach.]
• clicking an item in the floating ui should not trigger the onBlur for the combobox itself or the event that it triggers with should have an indication that it was the same overall component that was clicked. I have a workaround for this (I'm using onBlur to setInput("")) using !event.relatedTarget.classList.contains("saltOptionList") but I imagine that's fragile for the future as it's clearly a workaround.
• allowFreeForm (already covered by #2409)

[nathanmmiller]

• clicking an item in the floating ui should not trigger the onBlur for the combobox itself or the event that it triggers with should have an indication that it was the same overall component that was clicked. I have a workaround for this (I'm using onBlur to setInput("")) using !event.relatedTarget.classList.contains("saltOptionList") but I imagine that's fragile for the future as it's clearly a workaround.

4012 looks to solve this part of this issue.

[origami-z]

ability to not close the floating ui on "Esc" - I think a change to ComboBox::handleOpenChange could work here - I would propose a prop like, "ignoreEsc" that would then do something like: if (ignoreEsc && reason === "escape-key") return; [I will happily raise a PR for this if y'all agree with this approach.]

It's unlikely we will include this, given it's part of component accessibility spec (and shares across components). Can you tap into controlling open for this ?

[nathanmmiller]

ability to not close the floating ui on "Esc" - I think a change to ComboBox::handleOpenChange could work here - I would propose a prop like, "ignoreEsc" that would then do something like: if (ignoreEsc && reason === "escape-key") return; [I will happily raise a PR for this if y'all agree with this approach.]

It's unlikely we will include this, given it's part of component accessibility spec (and shares across components). Can you tap into controlling open for this ?

I don't think I can - the problem is that handleOpenChange is called on Escape, no matter what, and that's what I'm trying to avoid.

Or are you saying if I'm completely controlling open that setOpen's internal state will be ignored? If so, wouldn't I lose all the other benefits of the way the component controls open state (like on clicks and arrow keys etc.)? And if that's the case... wouldn't I be essentially just rewriting this entire component for one minor feature? This seems like a very simple change (one which I'd be happy to submit a PR for!) that would allow us to get slightly closer to unblocking our migration to Salt.

[origami-z]

Will discuss what's best way to support your use case:

@nathanmmiller Can you confirm whether this example does what you need? https://stackblitz.com/edit/salt-template-wqbpxa?file=App.tsx

[joshwooding]

I think all of your requirements are possible with the recent change to onBlur. Would this implementation work for the above use case, @nathanmmiller?

https://codesandbox.io/p/sandbox/agitated-sunset-wqg8ql

However, as mentioned above, I would check that this doesn't break any a11y guidelines.

[nathanmmiller]

@origami-z's example does most of what I need, except for delimiter support and freetext and multiselect (though the latter seems as simple as just adding multiselect=true).

Unfortunately, I can't comment on @joshwooding's change because that link doesn't work for me, either on PC or phone...

[joshwooding]

@origami-z's example does most of what I need, except for delimiter support and freetext and multiselect (though the latter seems as simple as just adding multiselect=true).

Unfortunately, I can't comment on @joshwooding's change because that link doesn't work for me, either on PC or phone...

When you say the link doesn't work as in codesandbox doesn't load at all?

[nathanmmiller]

@origami-z's example does most of what I need, except for delimiter support and freetext and multiselect (though the latter seems as simple as just adding multiselect=true). Unfortunately, I can't comment on @joshwooding's change because that link doesn't work for me, either on PC or phone...

When you say the link doesn't work as in codesandbox doesn't load at all?

On mobile, the page crashes over and over and over. On my (work) desktop, it loads but never gets past "loading react-refresh 1/8."

That said, I just tried in an incognito window and it loaded. I think this might be perfect. Looks like it works with the delimiter and freetext too. Only thing I'd change in my implementation is removing the .trim() in the filters, because we care about spacing. But yeah I think this is everything I need (though I cannot use it until my issue in #3294 is addressed as well, unfortunately).

[nathanmmiller]

Oh I guess one other thing is free-text shouldn't duplicate values when the same thing is typed in again, though I suspect that's a simple thing to change from this review.