onChange called with null when `esc` pressed

[TLadd]

• downshift version: 3.2.10
• node version: 10.16.0
• npm (or yarn) version: 1.16.0

Relevant code or config

import React from "react";
import ReactDOM from "react-dom";
import Downshift from "downshift";

import "./styles.css";

function App() {
  const [value, setValue] = React.useState("great");
  const items = [
    { label: "great", value: "great" },
    { label: "bad", value: "bad" }
  ];
  const selectedItem = items.find(item => item.value === value);
  return (
    <div className="App">
      <h1>Hello CodeSandbox</h1>
      <h2>Start editing to see some magic happen!</h2>
      <Downshift
        items={items}
        itemToString={i => (!i || i.label == null ? "" : String(i.label))}
        selectedItem={selectedItem}
        onChange={item => {
          console.log(item.value);
          setValue(item.value);
        }}
      >
        {({
          getToggleButtonProps,
          getMenuProps,
          getItemProps,
          isOpen,
          selectedItem,
          highlightedIndex
        }) => (
          <div>
            <button {...getToggleButtonProps({})}>
              {(selectedItem && selectedItem.label) || "Select An Item"}
            </button>
            <ul hidden={!isOpen} {...getMenuProps()}>
              {items.map((item, index) => (
                <li
                  {...getItemProps({
                    item,
                    key: item.value,
                    style: {
                      textDecoration:
                        highlightedIndex === index ? "underline" : undefined
                    }
                  })}
                >
                  {item.label}
                </li>
              ))}
            </ul>
          </div>
        )}
      </Downshift>
    </div>
  );
}

const rootElement = document.getElementById("root");
ReactDOM.render(<App />, rootElement);

What you did: I created a Downshift dropdown that behaves as a simple select with a button to open/close. I click on the button to open the menu and then press esc key.

What happened: The onChange prop is called with null, and the above code crashes because onChange is expecting to receive an object.

Reproduction repository: https://codesandbox.io/s/musing-austin-g1bt1

Problem description: It was unexpected for me that onChange was triggered at all in this case. I would expect esc to close the menu, but keep the value the same as it was.

Suggested solution: Do not trigger onChange in this case.

[silviuaavram]

Just as a heads up, I am planning to create React hooks for dropdowns. The solution you are looking for is my first hook, here but it's not an npm package yet. Still work in progress, just as a heads up.

As for this usage, Downshift only works by design with search dropdowns (comboboxes), not simple dropdowns such as your.

What you can do is to use the stateReducer and tweak it in however way you want for your case. I think that we treat Escape as a clear function, probably that is why you get the null.

In stardust-ui we did just that so we can use Downshift and also add other stateful logic to cover all our cases. Stardust Dropdown

You can also wait for the hooks library to get published and use that directly, since I want to have it 100% ARIA compliant as far as a dropdown is concerned.

Let me know how I can assist you further.

[klis87]

I believe this behaviour was added recently. Previously on ESC just options were closed and item text was set to selected itemToString. Now, additionally selected item is reset to null

Personally I find this behaviour really user unfriendly. As a user, on ESC I would not expected a selected item to be unset. This is not even done in native select!

Remember, Downshift can be used also to build not searchable dropdowns, also with multiple options allowed to be selected. Imagine a client to select 10 items and his surprised face that those 10 items are gone! This person would probably leave the site.

I think that 3.2.10 release was a mistake, it was not even a bug fix. Obviously it should not be released as minor update, as this is breaking change.

please reconsider and at least give a prop to downshift to revert to a previous behaviour, like resetOnEsc or sth. This update broke many apps including mine and it stops me from even upgrading.

[TLadd]

Yeah it does appear it was added recently in https://github.com/downshift-js/downshift/pull/689 to match the spec for comboboxes.

The problem actually wasn't the fact that the component was a simple dropdown. I have another component that utilizes Downshift and failed in the exact same way, since it also had an onChange prop that assumed it would only be called with a valid item. Now obviously, this isn't compliant with the combobox spec linked to in the above PR, but it was a safe assumption for how Downshift worked before this change. So yeah, I think a major release would have been nice for this change.

Anyways, it's easy enough to override the behavior when needed. It's just a lot easier now to run into a situation where this crashes than it was before. And anyone who was making the same assumption I was (that onChange always passed an item) will have their dropdowns break.

For instance, the basic autocomplete example linked to in the README fails when pressing ESC after first selecting an item because of this: https://codesandbox.io/s/github/kentcdodds/downshift-examples/tree/master/?module=%2Fsrc%2Fordered-examples%2F02-complete-autocomplete.js&moduleview=1

onChange={selection => alert(`You selected ${selection.value}`)}

because selection now is null if you press Escape.

So I would guess I'm not alone in making the assumption that onChange is only called with an item. Assuming the behavior is remaining this way, we probably should call this out in the docs. Currently, I think they would lead you to believe that it will always be an item.

[klis87]

The problem is that Downshift can be used not only for combobox, but as select, multiselect too, as mentioned in readme. For select input obviously clearing selected item is wrong. For combobox, well, I know it is w3, but for me this is illogical. Escape usually is used for closing/hiding, not to update state. Standards are changed too, they are not axiom, they are based on someone elses opinion. But I will not argue with it, just problem with using Downshift for select type widget is imho enough to justify at least making this behaviour configurable as a prop.

[silviuaavram]

Thank you all for you input, let's try to work out a solution out of this. This is still a versioned library, no need to update to 3.2.10, nobody wants to break apps and all the effort is to actually make both the library and the apps that use it better.

With that in mind:

• I know that you can use Downshift to create other forms of dropdowns, I did it myself for stardust-ui. But it requires customised logic. By default, this library worked only for single selection comboboxes. If you look at the example with multi select, you will see that selectedItem is passed as a controlled null. https://codesandbox.io/s/github/kentcdodds/downshift-examples 04-multi-select.
• I added that change to make the behavior match the aria combobox. We need to match something! We cannot have a library that does 50% combobox, 25% dropdown, 10% multiselect etc.
• If I would just clear the input text on Escape, it would be useless, the selectedItem will be back as the string equivalent at some point. Not really the best idea.
• Deciding between major / minor was not easy, but I considered that I am fixing a behavior so decided to go with minor.

I like @TLadd docs and examples change for now. I don't think not calling onChange is the solution. The selectedItem did change, it was removed.

@klis87 I am not for blindly following docs. But those docs are also followed by screen readers, users with disabilities, accessibility trainers, accessibility app developers etc. We should be in line with that. What do you think about the docs/example edit?

[klis87]

@silviuavram I hope community will not complain about it as long as there will be an easy way to revert it to previous behaviour which would also explained in docs.

What would you recommend to make not painful for people like me who prefer the previous behaviour?

[eldargab]

Agree, that this behaviour from W3C is weird, raised an issue about that - https://github.com/w3c/aria-practices/issues/1066

[silviuaavram]

@klis87 if you want to have Downshift as a <select> then why not try my experiment and give me feedback? :)

[klis87]

@silviuavram thx for the link, I will try it after my holidays. Personally I cannot wait for official Downshift hooks support though :)

[klis87]

Please reopen this, they adjusted standards so that escape doesnt clear value if options are opened

https://github.com/w3c/aria-practices/commit/315a4ddc6b2161d9d8ad6c8bc3558d2976f2bef2

common sense won after all ;) this behaviour was veery weird, I am glad they fixed that.

Now, we should revert default behaviour in Downshift too

[silviuaavram]

Downshift will follow ARIA patterns and suggestions. We are not looking to follow 'common sense', but common patterns. Once the 1.2 version is done and released we will apply the needed changes to Downshift and the hooks. Thanks for keeping us in the loop!

[tony]

I am a bit confused, even on 1.1:

On the ARIA examples page: https://www.w3.org/TR/wai-aria-practices/examples/combobox/aria1.1pattern/listbox-combo.html

On the repo, though, it says clearing the value is optional:

... Dismisses the popup if it is visible. Optionally, if the popup is hidden before <kbd>Escape</kbd> is pressed, clears the textbox.

Link: https://github.com/w3c/aria-practices/blob/apg-1.1/aria-practices.html#L812

Which one is true?

Because is 1.1 clearing states that clearing is optional, shouldn't Downshift support not clearing on esc with an option right now?

[silviuaavram]

The new ARIA 1.2 example is only closing the menu on Escape, and it will remove the text only if the menu is closed. Also deque suggests the same behavior.

We will include this in the v6 release, for both useCombobox and Downshift. Escape will close the menu if open, and clear the text if closed. Documentation and tests might need changing as well, so it may take a bit more time, but the v6 branch is ready apart from this and https://github.com/downshift-js/downshift/issues/1010.

[silviuaavram]

:tada: This issue has been resolved in version 6.0.0 :tada:

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot :package::rocket:

[haleybarlar]

@silviuaavram is it possible to prevent the text from being cleared if the menu is closed?

[silviuaavram]

Hi @haleybarlar! Yes, stateReducer is your friend, you should be able to customise the behaviour in any way you see fit.

https://www.downshift-js.com/use-combobox#state-reducer https://github.com/downshift-js/downshift/tree/master/src/hooks/useCombobox#statereducer

In your case you probably need to look for InputKeyDownEscape state change and return the new state you want. Good luck!

[ajsharp]

I ran into this as well, and fixed it with the state reducer:

  const stateReducer = (state, changes) => {
    switch (changes.type) {
      case Downshift.stateChangeTypes.keyDownEscape:
        return {
          ...changes,
          ...state,
          isOpen: false,
          inputValue: state.selectedItem.label,
        }
      default:
        return changes
    }
  }