RFC: Combobox improvements

[benja]

Problem

The Combobox is a complex component with lots of functionality. To build one that fits our needs we have look at a wide variety of use-cases and attempt to find a solution that solves a multitude of problems, whether it is internally in the component or through component customisability.

During the lifespan of the current component, we've realised that the use-cases vary quite a bit from team to team, making it a bigger challenge to create the "perfect" component. However, it is not impossible, it just requires an open discussion so we can make compromises if needed and agree on a solution.

Where are we now?

The current version of the Comobox is entirely custom built with no external dependencies. We have tried to imitate Reach's component in functionality to the best of our ability, but there are a few quirks here and there. It might currently function a bit different to what we would expect to use in FINN products.

Where are we heading?

In updating the current version of the component, we are trying to follow the ARIA spec more closely: https://www.w3.org/TR/wai-aria-practices/examples/combobox/aria1.1pattern/listbox-combo.html (Example 3) & https://www.w3.org/TR/wai-aria-practices-1.2/examples/combobox/combobox-autocomplete-list.html. Please do let us know if you have any immediate thoughts, comments or further sources on this.

Properties

There have been discussions back and forth regarding properties and when/how to pass certain events. How we do this is entirely up to us, as long as we remain consistent in how we choose to do it.

E.g. fire

• onSelect with the option value if the user has navigated to one of the options without pressing Enter or clicking it
• onBlur with either the input value, or the navigation option value if the user has navigated to an option

...etc

See some of the improvements we have already made here.

It would be nice to have an open discussion around how you would expect the component to behave. How are you currently using the component within your team and what problems have you stumbled upon using the current version? If you have any questions, concerns or feedback, do feel free to leave them below!

[vsandvold]

Leaning a bit on @reach/combobox, what do you think about separating props declared on the component itself, from props inherited by native HTML elements?

Props declared on the Combobox component could be:

• onSelect?(value: string): void
  • Called with the selection value when the user makes a selection from the list (clicking, Enter, blur)
• selectOnBlur: boolean
  • If true, onSelect is called when the text box looses focus. Default true.
• openOnFocus: boolean
  • If true, the popover opens when focus is on the text box. Default false.

Props inherited from input element could be:

• onChange(event: React.ChangeEvent<HTMLInputElement>): void
  • Called when the input value changes. Use event.target.value to retrieve the current input value.
  • Question: will change events be fired when whitespace is added at the end of the input value?
• onBlur(event: React.FocusEvent <HTMLInputElement>): void
  • Called when the input looses focus. Use event.target.value to retrieve the current input value.
• onFocus(event: React.FocusEvent <HTMLInputElement>): void
  • Called when the input acquires focus.

[benja]

Do you think we should pass the native event forward on onChange, onBlur and onFocus instead of only passing forward the e.target.value? It goes a bit against the consistency of other components we have that parse the event and pass the value, but at least it would maybe allow for some more in-depth / custom solutions if people need access to the event? Alternatively, we could pass the value as the first parameter and the event as the second, where the second param is optional. For example onBlur(value: string, event?: React.FocusEvent<HTMLInputElement>): void.

[benja]

I think the latter would make more sense, as most people would probably only subscribe to the value and the onBlur also needs to be able to pass forward the navigation value (if set), which doesn't always correspond with the raw input value.

[benja]

Another question I have is whether we should only pass the value back on these events or if we should pass the option object back, so the user have to set setValue(option.value). This gives the consumer more freedom and potentially makes it easier if they wish to store more data on the option than the options' label and value props.

Alternatively, the consumer can map over their object to the correct structure and keep a copy of the whole object elsewhere and check against it when the user selects a value. E.g. user selects Luke Skywalker, you can check this value against the copy of the object to read more data regarding this option, e.g. height, movies played in, ++, keeping the option passed to the Combobox to only a value and possibly a label.

[vsandvold]

Thoughts on the component behavior for keyboard navigation:

• If no option is selected, should Enter be propagated up to the parent component, in order to trigger submit on an enclosing form element?

Thoughts on maintaining component state:

• Does the component maintain it's own input value state, or does it rely on the developer passing the value forward from eg. the onChange callback?
• Does the component maintain it's own open/closed state, or does it rely on the developer sending in eg. empty value and options props to close the popover?

[benja]

Regarding your first point. It could, but since we already pass onSelect maybe it would make more sense for the consumer to take care of this? Seems like it would be a simple check whether the value matches any of the option values, if not submit form onSelect or onBlur? Not sure if including it within the component would be a bit too use-case specific, unless maybe it's a boolean prop.

We had a discussion regarding keeping component state internally towards the end of last year and I think we landed on not having states internally for input values, etc. The consumer must pass this to the component, which gives the consumer more control over the values too if they wanted to change it.

The last point, not so sure. Considering the consumer passes a list of options down, I think it's safe to assume when the option list should be visible and not. This is something we're looking at now because it has been quite a bit of a mess in the current component. I think the component should take care of when it is displayed as the consumer may handle it correctly.

@digitalsadhu and I sat down yesterday speaking of the different states the Combobox can have/be in, just to take a step back and look at the overall picture.

Rough illustration. The events are temporary and may not reflect the end result

We figured that the component in theory can have these four states. Unselected is when the component is either idle (not yet interacted with) and/or doesn't have a valid value, e.g. asdasoijdsai matches none of the options. Querying is the state of typing something into the input for the options to render, navigating is when you navigate between the options suggested and selected when a valid option has been selected and is set as the input value.

Breaking it down to these few states makes the component a bit more understandable and allows for us to see the overall picture. For example, when the combobox is querying we want to display the options list to the user, when the user navigates and tabs away or clicks away it would fire the onBlur with the value the user had navigated to before this event happened.

[vsandvold]

Great idea with the state diagram! I tried to elaborate a bit by adding some more details (and a few more states that may not be that important). Hope it's ok without a diagram 🙂

Unfocused

• State:
  • Input unfocused
  • Popover closed
  • Input value provided by developer
• Actions:
  • onBlur called if transitioning from any other state
  • onSelect called with selected option value (or with current input value?) if selectOnBlur and transitioning from any other state
• Transitions:
  • User clicks or tabs into input field -> Focused

Focused

• State:
  • Input focused
  • Popover open if openOnFocus
• Actions:
  • onFocus called if transitioning from Unselected
• Transitions:
  • User types printable character, Backspace or Delete -> Querying
  • User navigates with keyboard -> Navigating
  • User hits Enter -> ???
  • User hits Esc -> Cleared
  • User clicks or tabs out of input field -> Unfocused

Querying

• State:
  • Input focused
  • Popover open
  • Input value provided by user
  • Options provided by developer
• Actions:
  • onChange called with current input value
• Transitions:
  • User types printable character, Backspace or Delete -> Querying
  • User navigates with keyboard -> Navigating if input value is not empty
  • User clicks on option -> Selected
  • User hits Enter -> ???
  • User hits Esc -> Focused
  • User clicks or tabs out of input field -> Unfocused

Navigating

• State:
  • Input focused
  • Popover open
  • Input value provided by option user navigates to
• Transitions:
  • User types printable character, Backspace or Delete -> Querying
  • User navigates with keyboard -> Navigating
  • User clicks on option -> Selected
  • User hits Enter -> Selected if user has navigated to option, otherwise ???
  • User hits Esc -> Focused
  • User clicks or tabs out of input field -> Unfocused

Selected

• State:
  • Input focused
  • Popover closed
• Actions:
  • onSelect called with selected option value (or with current input value?)
• Transitions:
  • User types printable character, Backspace or Delete -> Querying
  • User navigates with keyboard -> Navigating
  • User hits Enter -> ???
  • User hits Esc -> Cleared
  • User clicks or tabs out of input field -> Unfocused

Cleared

• State:
  • Input focused
  • Popover closed
  • Input value is empty
• Transitions:
  • User types printable character, Backspace or Delete -> Querying
  • User hits Enter -> ???
  • User clicks or tabs out of input field -> Unfocused

[vsandvold]

And just a few suggestions for general requirements:

• It must be possible to edit the input value in every way, ie. both at the beginning, in the middle, and at the end of the input value.
• The combobox does not show an outdated/stale list of options when opening and closing the popover.
• Highlighting of input value matches must only change when the options are updated (either via async API calls or fixed list filtering).

[benja]

I agree, great feedback.

Second point refers to the options list being visible with old values for a very short time before the new options have resolved. This is hard to predict considering the options are sent asynchronously from the consumer. This wouldn't be a problem if the list was static. Can't think of any immediate solutions for what would fix this.. hmm. The options list should at all times reflect the input value, so when you have selected an option and the dropdown opens again, you should see the same option list you saw before you selected an option, whether the option was the only one in the list or if there were more.

[vsandvold]

When the dropdown closes, doesn't the selected option become the new input value. So when the dropdown open again, the old options list will not correspond to the new input value anymore? Perhaps the expected behavior is different for fixed lists and API responses?

I suggest we look at the behavior implemented by @reach/combobox and go with that, instead of agonizing over these issues. They have probably had the same discussion already 🙂

[benja]

That is correct. When the dropdown closes and you have either a navigation option selected and blur the input field or select it, that will become the new input value. What we can do is clear the list on select so that when the value is updated the options are also cleared and updated. The asynchronous option fetching makes the option list rendering a bit more difficult than static options as far as I can tell, but that's not a big worry.

[vsandvold]

@benja I think you have done a great job with the flow chart and mapping out the different states of the combobox. I find the complexity a bit intimidating, and the number of different requirements and design choices is taxing and time-consuming to work through. In order to keep the momentum up, should we perhaps try to simplify some things? I would at least try to adopt as many things as possible from a similar component in another framework, or to keep the scope as small as possible.

Just my two cents, since I also feel responsible for this RFC and would hate to become a bottleneck and an obstacle for changes that may be useful for more people.

[benja]

I agree that it may be time-consuming and intimidating. However, after a bit more digging into the component I think we can do with just sorting out how async fetching of options is handled. Then it goes back to the events we fire off, such as blur, select, etc.. but you listed your input on this above, so no need to worry about this component anymore unless I let you know. I'm going off the ARIA spec and Reach combobox from here. Appreciate your concern! 💯