Disabled items can be selected by pressing enter

[aknaus]

• downshift version: 1.7.2
• node version: 8.8.1
• npm (or yarn) version: 1.2.1

Problem description:

At the moment it is possible to select a disabled item by pressing enter although selection by click is disabled. Personally, I think it should not be possible to select a disabled item (at least not for the user).

Suggested solution:

Since the event handler is not registered on the button and not on the item (like the onClick handler), we cannot just disable it. The easiest solution I can think of is checking the disabled prop before selecting an item. However, I do not have the required knowledge of your code to predict possible side effects of this change.

selectItem = (item, otherStateToSet, cb) => {
  if (!item.disabled) {
    otherStateToSet = pickState(otherStateToSet)
    this.internalSetState(
      {
        isOpen: false,
        highlightedIndex: this.props.defaultHighlightedIndex,
        selectedItem: item,
        inputValue:
          this.isControlledProp('selectedItem') &&
          this.props.breakingChanges.resetInputOnSelection
            ? this.props.defaultInputValue
            : this.props.itemToString(item),
        ...otherStateToSet,
      },
      cb,
    )
  }
}

[kentcdodds]

Thanks for the report @aknaus!

So your suggested solution wont work I'm afraid because we have no control over the item's properties. It could be a string, object, function, anything. So we can't check the disabled prop. But we do have a mechanism for finding the DOM node by it's index and the DOM node will have a disabled prop.

So we could change this:

https://github.com/paypal/downshift/blob/703fd05819957914d1fb5d9cff65d278d664088c/src/downshift.js#L278-L284

To this:

  selectItemAtIndex = (itemIndex, otherStateToSet, cb) => {
    const item = this.items[itemIndex]
    const itemNode = getItemNodeFromIndex(itemIndex)
    if (item == null || (itemNode && itemNode.hasAttribute('disabled')) {
      return
    }
    this.selectItem(item, otherStateToSet, cb)
  }

I'm pretty sure that'll work. If someone would like to make a PR for that with tests that'd be super! Thanks a bunch!

[kentcdodds]

Actually, this could preclude someone who wanted to force selection of a disabled item programatically... Perhaps we should put that logic right in the event handler here:

https://github.com/paypal/downshift/blob/703fd05819957914d1fb5d9cff65d278d664088c/src/downshift.js#L514-L521

---

Another thought... How is the highlighted item getting set to a disabled item? That shouldn't be possible. If it is then that's probably a bug. The highlighted index should probably skip disabled items don't you think?

[aknaus]

You are right @kentcdodds it should be handled in the event handler. However, I think it should be possible to focus disabled elements in order to aid people who are using screen readers. If I may, I would create a PR to resolve it?

[kentcdodds]

That sounds fine. Thanks!