downshift icon indicating copy to clipboard operation
downshift copied to clipboard

Question: Downshift Component not announcing selectedItem on change - is this by design?

Open JoshuaAYoung opened this issue 4 years ago • 4 comments

PLEASE SEE LATEST COMMENT

  • downshift version: 4.1.0
  • node version: 10.24.0
  • npm (or yarn) version: yarn 1.22.10

Relevant code or config

   _proto.componentDidUpdate = function componentDidUpdate(prevProps, prevState) {
      if (process.env.NODE_ENV !== 'production') {
        validateControlledUnchanged(prevProps, this.props);
        /* istanbul ignore if (react-native) */

        if ( this.getMenuProps.called && !this.getMenuProps.suppressRefError) {
          validateGetMenuPropsCalledCorrectly(this._menuNode, this.getMenuProps);
        }
      }

      if (isControlledProp(this.props, 'selectedItem') && this.props.selectedItemChanged(prevProps.selectedItem, this.props.selectedItem)) {
        this.internalSetState({
          type: controlledPropUpdatedSelectedItem,
          inputValue: this.props.itemToString(this.props.selectedItem)
        });
      }

      if (!this.avoidScrolling && this.shouldScroll(prevState, prevProps)) {
        this.scrollHighlightedItemIntoView();
      }
      /* istanbul ignore else (react-native) */


      this.updateStatus();
    };

What you did: Used multiple instances of a dropdown component with Downshift in a form. On this particular page, several of the dropdowns have default values selected on load.

What happened: When tabbing away from one of the downshift dropdown components on the page, the componentDidUpdate hook (shown above from downshift.js) will fire for every instance of the dropdown. Since each of these instances has a selectedItem by default, setStatus tries to populate the a11y-status-message div with the selectedItem string from every instance of the dropdown component, even though none of the values had changed. In effect, screen reader users (without interacting with anything mind you) will have these default selection strings (in our case "sale", "alabama" and a few others) announced to them 6 or 7 times tabbing through the whole page.

Problem description: I haven't tracked down what exactly is causing the componentDidUpdate to fire on every instance of the component, but it doesn't appear to be a change in any of the variables that getA11yStatusMessage uses to create the status string (isOpen, highlightedIndex, or resultCount). I just updated from Downshift 3.x to 4.1, hoping that it would fix the issue without much luck. I'm hesitant to upgrade to the latest version as it looks like updateStatus is still called from componentDidUpdate without any conditions.

Suggested solution: Would it make sense to check if isOpen, highlightedIndex or resultCount had changed before calling updateStatus in the componentDidUpdate hook? It seems to me that if one of those three had not changed on the page, then the status message has no business populating, right?

if (_this.getItemCount() !== _this.previousResultCount || this.props.isOpen !== prevProps.isOpen || this.props.highlightedIndex !== prevProps.highlightedIndex) {
this.updateStatus();
}

JoshuaAYoung avatar Jul 21 '21 16:07 JoshuaAYoung

Just wanted to provide a little more context on this issue, in case the original issue description wasn't clear. I'm working as a contractor for a company and sharing code is out of the question, and I don't have time to sandbox this this thing.

Anyway, we have a form with about 20 inputs, 4 of them Downshift dropdowns with the rest of them text inputs. We use setState in the parent component to handle the touched values of the text inputs for error handling purposes. But every time setState is called, React re-renders the entire component (as well as child components).

Because updateStatus() is called inside of a componentDidUpdate, and because we have default values passed in via selectedItem, every time the component re-renders (which happens every time you tab away from a text input), Downshift tries to pass all 4 of our selectedItem values into the a11y-status-message div at the same time.

Luckily, only 1 of the 4 values makes it into the status div at a time (seems random which value makes it in), but that still means that every time a user tabs away from a text input (again, there are about 16 on the page), their screen reader announces said value.

JoshuaAYoung avatar Aug 02 '21 18:08 JoshuaAYoung

So I finally bit the bullet and upgraded to 6.x from 3.x. Less breaking changes than upgrading to 4.x somehow (not looking this gift horse in the mouth).

At any rate, 6.x seems to have solved our problem with one caveat. It looks like Downshift is no longer announcing the selectedItem at all, even when changing the selectedItem. I saw that the selectedItem was removed from the getA11yStatusMessage in V5. There's a new getA11ySelectionMessage method that seems to be the go-to for handling the selection message, but it appears to only be called in useSelect, useCombobox, and useMultipleSelection hooks, not in the Downshift component (which we are using). Can someone confirm that this is correct? That the Downshift component no longer announces selectedItem changes at all?

JoshuaAYoung avatar Aug 12 '21 22:08 JoshuaAYoung

default values passed in via selectedItem

@JoshuaAYoung it sounds like you are using a controlled Downshift component by passing the selectedItem controlled prop: https://github.com/downshift-js/downshift#control-props. If so, I think we may be seeing the same behavior.

I need to use a controlled prop here because I want to dynamically update the selected item from the parent component's state. When I was using initialSelectedItem previously, I would see onSelectedItemChange callback fired when the user changes the dropdown selection.

Now with the selectedItem control prop, I don't see this firing anymore, and I think I need to use the onStateChange callback instead to wire up the changes I want to see, that's mentioned in the Control Props docs I linked.

I'm also curious if I can use the stateReducer instead, but I don't think my use case is that advanced yet.

akrawchyk avatar Oct 01 '21 16:10 akrawchyk

@JoshuaAYoung @silviuaavram related to https://github.com/downshift-js/downshift/issues/1092 maybe?

akrawchyk avatar Oct 01 '21 19:10 akrawchyk

The default implementation of getA11yStatusMessage does not announce the selected item anymore, indeed. You can override the behaviour to announce it again. We are planning to do a change for the status update in v8, but it won't happen for Downshift. Only the hooks.

silviuaavram avatar Dec 19 '22 16:12 silviuaavram