jquense / react-widgets

Polished, feature rich, accessible form inputs built with React
http://jquense.github.io/react-widgets/
MIT License
2.34k stars 393 forks source link

Typing full item text in combobox removes filter #247

Closed dubrowgn closed 6 years ago

dubrowgn commented 9 years ago

https://jquense.github.io/react-widgets/docs/#/combobox

  1. Uncheck "Suggestions."
  2. Change "Filter" to any setting.
  3. Type "Jim" into one of the top two comboboxes.
  4. Notice the filter is no longer applied to the combobox.
jquense commented 9 years ago

yeah this is a consequence of how the stuff is (needs to be) implemented, otherwise selecting an item would result in the list always just containing the selected item

dubrowgn commented 9 years ago

The behavior is inconsistent because it automatically selects the first full match, regardless of it there are other partial matches. If you were typing in "Jimmy," you get to "Jim" and all the options show up because it automatically selected "Jim". Then you type another "m" and the options disappear again. This all happens without ever leaving the textbox or clicking on anything.

dubrowgn commented 9 years ago

I would expect selection to happen only on explicit user action, such as a click, tab off, or pressing enter. Auto selecting without a user action is confusing in general because you can't tell if it is actually selected or not. If a selection has been made, why is the options menu still open for example?

dubrowgn commented 9 years ago

Sorry for so many comments...

It seems onSelect doesn't fire until the user explicitly selects an item (after the dropdown options are reset in the original example). I would expect the dropdown options to reset after this event, not when the filter text happens to be a full match for any item.

jquense commented 9 years ago

I agree that the behavior is sort of odd, but there isn't really another way to handle it other than only firing onChange when you blur out of the input or allowing the user to select a text value that exactly matches an item in the list but isn't that item. Neither option is appealing.

the reason an item is selected as soon as there is a match because the value of ComboBox changes on each keypress (like a normal react input), It has to be that way otherwise the user could type in "joe" which exactly matches { text: 'joe', id: 1 } but instead of returning that data item it would just set the value to the misleading text "joe", to a user it would look exactly the same, expect they didn't actually select a value. That is generally not acceptable, since you want to prefer the selection of list items to free text entries when using a combobox.

dubrowgn commented 9 years ago

I don't see any problems with keeping the way onChange works, but I do think it would make more sense to remove the filter at the same time you fire the onSelect event instead. The dropdown closes at that point anyway, so the user is much less likely to experience any weirdness.

jquense commented 9 years ago

I think you might be misunderstanding the problem. If I don't remove it when an item is matched (i.e. selected) while typing, it would just eternally show a dropdown list of exactly one item. You can't then "select" the item because its already selected. If we rely on the user hitting enter or clicking the item in the list, if they don't do that the only way to remove the filer would be to delete the value in the dropdown.

Sorry if its still not quite clear, its a bit tough to explain. There probably is a way to get the behavior you want but it involves a lot of state bookkeeping (like clearing when you blur, close, etc), and I am not sure it would ever play nicely with all the controllable props.

I'd be happy to entertain a PR though that fixes this, but I don't have the patience or time to try again myself! :)

dubrowgn commented 9 years ago

I took another look at onSelect. The only reason my proposal wouldn't work is because onSelect doesn't fire when the control loses focus. It fires on click, and on enter key press. Currently, onSelect doesn't seem very useful, because it doesn't always fire when a user selects a new value. If it did always fire, I think clearing the filter at the same time would work better than the current implementation.

jquense commented 9 years ago

onSelect is an event specifically for tracking when a user selects something from the list (a fuzzy concept with the combobox i know), its useful for differentiating a change due to a user picking something and the user typing a value. Right now its consistent with onSelect callback in the Multiselect and dropdown, it may not seem useful but it has uses as an extensibility point.

Right now there isn't a single event that covers waht you want, but to be fair if there was this behavior would already be in here.

dubrowgn commented 9 years ago

What is the use case for not wanting to know an item was selected via the textbox?

It seems to me that onChange is currently conflating value changed and filter text changed. If there was a onFilterChange (or similar) in addition to onChange, would you need onSelect?

jquense commented 9 years ago

onChange isn't conflating them as you type the value is changing. Remember that filtering isn't even the default behavior, its opt in, so in the default (and filter) case the value is actually changing as you type. The point of the combobox being that you can choose a predefined option or type something as you wish.

I think we are getting off track here...is this issue about wanting to change the way onSelect works? because that isn't likely at least not until 4.0.0. In terms of implementing the behavior you are originally asking for, that is not dependent on the onSelect event behavior. Internal logic shouldn't be listening to it's own events.

dubrowgn commented 9 years ago

Sorry, this thread has been wandering a bit as I learn more about the combobox class.

My request is to fix the filter weirdness that happens as a user types. Options shouldn't reappear as a user types a progressively more restrictive filter. How it could be fixed has been the topic since my first post.

You can tell onChange is conflating ideas because it returns two completely different kinds of values. If the "value" of combobox is actually the text from the textbox, this should be the only thing being returned from onChange. If the "value" of combobox is an item from the dropdown, an item should be the only thing being returned from onChange.

I bring this up only because if onSelect or onChange fired only when a user selects an item from the drop down (via any method), it would be trivial to put the filter reset call before or after the call to raise the onChange or onSelect event. This would address the original issue without too much fuss.

I thought onSelect's purpose was to capture exactly that, so that's why I suggested it. I then realized there is no event to capture when a user selects an item via any method, and that's why I asked what the use case was for not having onSelect be that event.

jquense commented 9 years ago

if onSelect or onChange fired only when a user selects an item from the drop down (via any method), it would be trivial to put the filter reset call before or after the call to raise the onChange or onSelect event.

This is exactly the current behavior of the ComboBox. If an item is selected from the list or if the user selects an item via typing, it resets the filter. What you want is the latter case to only reset when the user either hits enter, blurs, or closes the dropdown. I agree with you that is the better behavior, but it was too complex and error prone when I tried to implement it originally. admittedly its been a while and maybe I could give it another shot, but that fastest way to get it in would be a PR ;)

You can tell onChange is conflating ideas because it returns two completely different kinds of values.

The data types might be different (say a string vs an object) but they are not different kinds of values, You could also have a data prop of just strings and then they'd be the same type, or you could have a data prop full of polymophric objects. The whole point of combobox is to enable exactly this behavior. (If you want the filter value to be distinct from the list list value I suggest using a DropdownList with the filter prop).

Lets be done with this for now, since I think we are all on the same page that the current behavior is wonky and less than ideal, I don't particularly mind it enough to find the time to fix it right now (which is not to say I won't in the future). If you'd like to take a shot at it in a PR I'd definitely accept it if its solid!

bent0b0x commented 7 years ago

+1 on this behavior needing adjustment

jquense commented 7 years ago

Patches Welcome