Downshift: Enter event on form wrapped input should not have e.preventDefault()

[hzhu]

First of all thanks @kentcdodds for authoring this library! Also thanks to everyone else who's been helping Kent maintain this project ❤️.

• downshift version: 1.28.0
• node version: v8.1.3
• yarn version: 1.2.1

What you did:

I type the string "black" (lowercase) into the form example in Downshift's storybook - then hit ENTER.

What happened:

The form submission handler does not get called. However, my expectation is that the form submit handler gets called even though lowercase "black" isn't a part of the list. This is important when building autocomplete for search services. When a user hits ENTER, I want to still be able to execute a search on lowercase "black", even though it is not part of the list in the combobox:

Reproduction repository:

Reproducible in Downshift Storybook in the form example.

Problem description:

I think that a <form> submission handler should always be called on ENTER, even if the combo box list is open. I believe in the past, a <form> submission was not possible entirely, see #161.

Pull request #161 made it possible to submit the <form>, but not when the combobox list isOpen.

Suggested solution:

I am not sure I have an elegant solution. I am seeking the help of the community on this one. Please let me know if I got this all wrong and I'm simply not understanding the correct usage of Downshift.

A solution could be for Downshift to accept a prop isForm to Downshift. Then we will not call e.preventDefault when a user presses ENTER at all if the consumer of the component tells Downshift that a <form> wraps its <input>.

[kentcdodds]

Hey @hzhu,

Thanks for the issue! I'm not sure of the best solution for this to be honest. I can think of several things you could do, but I'm not super fond of them.

I don't think I want to add a prop especially for this case. I think we can come up with something else. I'd love people to give their input to solving this problem. Anyone have ideas?

[newyork-anthonyng]

@hzhu I was working on this commit that seems to address the issue.

I'm not sure if it's the most elegant. I had to control the isOpen and selectedItem of the Downshift component. I also had to pass the onSubmit function to the input element.

Something else that I tried playing with but couldn't make it work:
The Enter keydown event is prevented only when the menu is open (see code here). I had an idea to close the menu first, and then redispatch the Enter keydown event.

<Input
    {...getInputProps({
        placeholder: 'Enter color here',
        onKeyDown: e => {
            e.persist();
            // Use closeMenu action function that Downshift provides
            closeMenu(() => {
                // somehow redispatch the Synthetic event so that Downshift could take care of it
                // https://stackoverflow.com/questions/11974262/how-to-clone-or-re-dispatch-dom-events
                // this is where I failed 😟
            });      
        }
     })}
/>

[austintackaberry]

It seems like there's only way to let downshift handle the event correctly as the code stands right now. You have to find a way to highlight the item before the Enter keyDownHandler is run.

@newyork-anthonyng for my app, I ended up doing something similar to your commit. I can't think of a better way to do it.

@kentcdodds what do you think about highlighting an item if it matches inputValue? That way hitting enter would select the correct item without the user having to click on it or navigate to it with the keyboard.

[austintackaberry]

The implementation below allows the user to let downshift handle the event while still getting the functionality that @hzhu wanted.

https://codesandbox.io/s/x229880xjo

The general idea is:

if (item === inputValue && highlightedIndex !== index) {
   // take control of highlightedIndex and force it to equal index
}
else {
   // give control of highlightedIndex back to Downshift
}

I'm sure there are many reasons this is a terrible idea...it feels wrong to only control highlightedIndex some of the time.

My first thought was to do something with onInputValueChange, but the issue there is that you would have to repeat the map and filter code in order to know what the highlightedIndex should be.

You could have it highlight the item if it is the only item in the list, but that wouldn't work if you had apple and say apple juice in the same list.

[newyork-anthonyng]

@austintackaberry I forked your CodeSandbox example and wrapped it with a <form />. It looks like the onSubmit isn't firing.

Could you take a look at see if I'm missing something?

[kentcdodds]

What if we allowed you to do this:

const handlers = {
  Enter(event) {
    if (this.getState().isOpen) {
      this.selectHighlightedItem({
        type: Downshift.stateChangeTypes.keyDownEnter,
      })
    }
  }
}

const ui = <Downshift keyDownHandlers={handlers}>
 {() => <div>your ui</div>}
</Downshift>

This would be fairly trivial to implement. We'd just need to add a little bit of logic here. Probably change it to this:

input_handleKeyDown = event => {
  const handler = this.props.keyDownHandlers[event.key] || this.keyDownHandlers[event.key]
  if (handler) {
    handler.call(this, event)
  }
}

It would be an advanced feature and we should mention that in the docs to avoid using it because it exposes the downshift instance in a way that _could_ lead to your code breaking if we decide to change a method you're relying on...

Alternatively we could do: handler.call(this.getStateAndHelpers(), event) which would only allow you to access the same things you can access in the render prop... I think I like that better. It would require us to expose moveHighlightedIndex, but I'm cool with that.

Thoughts? Anyone wanna try to take this on? We'll need at least one or two tests and the keyDownHandlers prop will need prop types and a default value.

The more I think about this the more I like it as a solution. It's not the best, but it seems like a good way to get around this problem without complicating the codebase or the usage too much.

[austintackaberry]

Oops, I misread the original concern. Yeah, my codesandbox doesn't submit the form (especially considering there is no form). All it does is select the item that equals inputValue when hitting Enter.

Why do you need to actually submit the form though? Why not just make your form a controlled component and let React handle the state of each element in the form?

[hzhu]

Why do you need to actually submit the form though? Why not just make your form a controlled component and let React handle the state of each element in the form?

Hi Austin, just to preface, I could be wrong as I'm pretty new to working with autocomplete UIs 🤐 . Some apps like Amazon.com implement their search autocomplete as a form like the <form> example in Downshift's storybook. I think it would be standard behavior when a user hits ENTER that the <form>'s onSubmit handler is invoked. Note, the form doesn't need to be submitted to a server. The page doesn't need to refresh.

Thoughts? Anyone wanna try to take this on? We'll need at least one or two tests and the keyDownHandlers prop will need prop types and a default value.

Hi Kent, I'd love to take this on as my first open source contribution to Downshift! I just tested your suggestion and it works. It seems non-invasive to the existing codebase. My teammate (who is helping me implement autocomplete at work using Downshift) has been working on a solution over the weekend. I'd love to see what he has in mind also!

[indiesquidge]

Potential Solution (Change in Downshift Internals)

⚠️ NOTE: this will not work for all cases. See below for more details (https://github.com/paypal/downshift/issues/330#issuecomment-367307526)

I had a similar thought as @austintackaberry, but instead of taking control of anything, I thought about intercepting the event and passing along the target value to selectHighlightedItem.

Enter(event) {
  if (this.getState().isOpen) {
    event.preventDefault()
-  this.selectHighlightedItem({
+  this.selectHighlightedItem(event.target.value, {
      type: Downshift.stateChangeTypes.keyDownEnter,
    })
  }
}

Then in selectHighlightedItem we could check if any item was actually highlighted. If it was, then we just keep the same behavior that already exists; if there is no highlightedIndex (i.e. the user did not highlight anything from the dropdown list), then we instead make a call to select the current input value.

selectHighlightedItem = (inputValue, otherStateToSet, cb) => {
  const itemIndex = this.getState().highlightedIndex

  // if a user has not selected an item from the list, "select" the current input value
  if (itemIndex == null) {
    return this.selectItem(inputValue, otherStateToSet, cb)
  }

  // otherwise, carry on as usual
  return this.selectItemAtIndex(itemIndex, otherStateToSet, cb)
}

This has the implication that if a user does not select an item from the list upon hitting enter, Downshift will default to "selecting" the input field's value. This seems okay to me, but I do not have enough context around all of the nuanced problems Downshift is attempting to solve for different use-cases, so I'm not confident that there isn't a situation in which this would "break".

Solution without Changes

https://github.com/paypal/downshift/issues/330#issuecomment-366538491 is a useful solution that requires no changes to the current Downshift codebase; simply add a key down event handler to the getInputProps props getter, check for the correct key and whether or not there is a highlighted index, and pass the event to the submission event handler based on those predicates.

<input
  {...getInputProps({
    onKeyDown (e) {
      if (e.key === 'Enter' && highlightedIndex === null) {
        return onSubmit(query)
      }
    }
  })}
/>

This works as expected, solves the problem posted in this issue, and requires no changes to the current behavior of Downshift. The downside is that it's pretty imperative code, and I don't think it solves the deeper issue here (see below).

Overarching Problem

IMO, the overarching issue here is that the user's input is not included as a type of suggestion. Not only does this affect the issue @hzhu brought up here, but also issues around how a user navigates through a list of suggestions.

If you check out how many comboboxes are implemented on popular sites (e.g. YouTube, Google, Amazon, etc.) you'll notice that using arrow keys to navigate up and down the list _include the user's original query as part of the cycle_, i.e. they preserve the user's input as a "suggestion" the user can choose to search.

This feels like a better UX overall to me. I am not sure what a solution would look like to fix this "pending" issue (could be that I'm just mistaken on how this library is meant to be behave and in fact it was intentional to not treat user input the same as any other suggestion), I'm merely bringing it up because I think it is umbrella problem over what @hzhu is dealing with in this issue.

[kentcdodds]

Thanks for that thorough overview @indiesquidge.

I think a lot of people forget that there are a _TON_ of use cases with user experience for components that use downshift. Sure YouTube, Google, and Amazon may have autocompletes that behave similarly (and differently from downshift's default), but that's because they actually have similar "search" kinds of use cases. What about an autocomplete where you can only select one of a given set of options? That was the original use case for downshift which is why that's the default. That said, you can support the experience that YouTube, Google, and Amazon give you with downshift without weird use-case-specific props or hacks and that's one of the things that I think makes downshift so powerful.

So are we going to change the default behavior? No, I don't think that we will. If we did, the people on the other side of the use case spectrum would be frustrated instead. There have to be defaults in downshift, otherwise it wouldn't be very useful. So instead we'll just make changing the defaults easier which is what I think my keyDownHandlers prop idea does :)

@hzhu, I'm glad that the solution seems to work for you! I'd love a PR. If you'd prefer to wait for your co-worker to weigh in that's fine with me. I'm not waiting on this issue personally. But I do think the solution is a good one.

[kentcdodds]

Ha! I knew that making the solution more generic was a good idea! This solution will also make the arrow key behavior easier to control as well (#336)

[kentcdodds]

Oh and @indiesquidge, I forgot to address your "Potential Solution (Change in Downshift Internals)" solution. That wont work because the value of the input and the value of the item is not necessarily the same because the item can be an object and the itemToString is responsible for serializing the items to a string for the input value. We don't have a mechanism to deserialize a string value to an item I'm afraid. Consider a list of contacts where two contacts have the same name.

This keyDownHandlers idea is what I want to go with. Any takers??? :)

[austintackaberry]

I believe @hzhu was taking this one on

[indiesquidge]

Hi, @kentcdodds. Those are all super valid points, and I agree that it is not necessarily Downshift's responsibility to implement (or even help provide a way to implement) all of the features for all of the different types of autocompletes, typeaheads, combo boxes, etc.—it could be a never-ending list of requests 😅

I knew I was missing some caveat with my one-off solution, haha. Thanks for explaining why it wouldn't work. Your more generic solution of exposing the keydown handlers seems like a great idea for solving the form submission problem; I can't wait to see it added to this library by @hzhu.

I am very grateful for how much is provided _for free_ for all of us using OSS like this, and it's even more pleasant that you (and other maintainers) are willing to actively maintain and engage in a discussion around the tools you release. Thanks again 💛

[hzhu]

I'm glad that the solution seems to work for you! I'd love a PR. If you'd prefer to wait for your co-worker to weigh in that's fine with me.

Hi @kentcdodds, my schedule's been hectic but I'll try to submit a PR for review this weekend 😸 !

[kentcdodds]

Folks who care, I'd appreciate feedback on the PR: https://github.com/paypal/downshift/pull/346#discussion_r170763621

I'm starting to rethink the decision we've made about this.

[kentcdodds]

Alright! So [email protected] now supports event.preventDownshiftDefault = true. Much better IMO and will help us avoid this issue entirely :)

Thanks everyone! Especially @ericedem for implementing it and those who contributed to the discussion in #353 :+1: :tada:

[kentcdodds]

Hmmm.. I'm not certain what went wrong with the auto-release, but I'm kicking off another one. Hopefully we'll have the actual new version soon.

[kentcdodds]

Ok, great! It's out and it's actually [email protected] (because this is a new feature 😅)

Thanks again!