Fixing Double Tap Enter Bug

[lab596]

Closes Issue #102

Hey please conduct a code review and let me know if this works for you. I ensured that it worked in my local server, however, it may be over-engineered because there seemed to be a problem where 2 identical searches were placed on the Google search heap. I solved it by preventing 2 identical searches within a few milliseconds.

[mfish33]

Thanks @lab596 for your quick PR. I appreciate it. CI is not passing because of an issue on our side. Don't worry about it.

As for the solution provided while it works, I think it could be simplified a bit. Downshift provides a way to override the default handlers for components https://github.com/downshift-js/downshift/tree/master/src/hooks/useCombobox#customizing-handlers. Using this I think we can just prevent the default behavior of downshift on the input when there is nothing currently highlighted. This will cause the enter event to be bubbled and submit the form leading to the correct behavior

[lab596]

Hey, I spent some time looking into it however I wasn't able to figure it out, can you provide more instruction, please?

[mfish33]

Hopefully, I can explain this coherently over text. Let me know if you have any questions and would like to discuss over VC.

Background - Dom event bubbling

Any time an event such as a key press or click happens on a web page that same action gets triggered on the dom ancestor until it reaches the root element. This is only stopped by calling event.stopPropagation

Douple enter bug

The reason for the double enter bug is in the Downshift library. An oversimplified view of the rendered html is

<form onSubmit={onFormSubmit}>
  <Autocomplete />
  <button type="submit">Search</button>
</form>

The first time the enter button is pressed Downshift prevents the propagation of the event and instead closes the autocomplete options. The second time it is pressed the logic in Downshift does not stop the propagation. Because it does not stop the propagation the event bubbles and gets to the form which runs the onSubmit handler.

  const onFormSubmit = (e: React.FormEvent<HTMLFormElement>) => {
      e.preventDefault();
      navigate(`/search/${searchType}?term=${encodeURIComponent(searchValue)}`);
  };

The solution to this bug is to prevent the default Downshift behavior, in order to let this already existing logic run.

Hints of a solution

We are not the only ones to have run into this issue https://github.com/downshift-js/downshift/issues/330. As a solution, the event override feature was implemented https://github.com/downshift-js/downshift/tree/master/src/hooks/useCombobox#customizing-handlers. This allows the ability to override Downshift defaults for events (the exact thing we are looking to do!).

The question now becomes where exactly we want to override the default Downshift behavior. Since the input element is focused when we press enter it is the most likely culprit.

Autocomplete.tsx~89

<input
    aria-label={label}
    className={`p-2 w-full h-full outline-none ${inputClassName}`}
    type="text"
    placeholder={placeholder}
    {...getInputProps({
        onKeyDown: (event) => {
           // Some code here
        }
    })}
/>

I will leave what replaces the comment for you to solve

[lab596]

Thanks for all the guidance, in this version, I have used downshift to prevent the default enter operation and instead called the search function which creates an instance of onFormSubmit redirecting the search.

Your explanation was very thorough and helpful. Thanks again :)

[AddisonTustin]

Hey Rohan,

We can actually take your new changes further, and purely rely on event propagation (bubbling) without having to try and pass any hooks or explicitly making changes to state in order to solve the bug.

Like Max said, the AutoComplete component was already capable of properly triggering a form submission on Enter press, but the default behavior of downshift prevented the event from propagating on the first input event.

It might be useful to try and identify what the most minimal changes need to be implemented to accomplish the intended behavior.

[lab596]

Hey Rohan,

We can actually take your new changes further, and purely rely on event propagation (bubbling) without having to try and pass any hooks or explicitly making changes to state in order to solve the bug.

Like Max said, the AutoComplete component was already capable of properly triggering a form submission on Enter press, but the default behavior of downshift prevented the event from propagating on the first input event.

It might be useful to try and identify what the most minimal changes need to be implemented to accomplish the intended behavior.

Hey, yeah I was trying to play around with that as well, however, when I removed the Search call and just had the preventDefault call it prevented any search with the Enter button. That makes sense however I'm not sure how to push the call towards onFormSubmit without the helper function. Would I have to look into where explicitly the Enter functionality is?

[lab596]

Hey, I looked into ways to conduct the search without the help of the additional hook. One quite cheeky way to accomplish it is by having a single Enter press force another using:

const simulatedEvent = new KeyboardEvent("keydown", {
     key: "Enter",
 });
event.currentTarget.dispatchEvent(simulatedEvent);

When looking specifically for event propagation I wasn't quite sure how to push forward the enter press to skip over the closing of the autocomplete suggestion box. Trying to close the suggestion box manually didn't work. Also, I didn't find any function call that simply allows you to skip over an event call propagating the enter call further.

[mfish33]

event.nativeEvent.preventDownshiftDefault = true

does exactly what you are looking to do. This line prevents downshift from handling the event thus allowing it to propagate up to the form component. Using this method you do not need to change anything in the SearchBar component

[lab596]

event.nativeEvent.preventDownshiftDefault = true

does exactly what you are looking to do. This line prevents downshift from handling the event thus allowing it to propagate up to the form component. Using this method you do not need to change anything in the SearchBar component

I see, I had actually seen that before but didn't know I had to implement a global. Thanks for the guidance once again.

• Simply adding the event.nativeEvent.preventDownshiftDefault = true; prevented the functionality to search the highlighted teacher in the suggestion box as mentioned above by @AddisonTustin
• In this commit I added the functionality that conducts the direct search of the professor's page as well as the search term (I thought it would be helpful for a back operation)

Thanks for all the help, as you may have been able to tell I'm super new to TypeScript.

[lab596]

Running npm t looks to pass everything.

[mfish33]

@lab596 Thank you for your contribution 🎉 It will be included in the next release (which will be very soon)