Default behavior for mouseUp event

[austintackaberry]

v1.28.1 issue:

const onMouseUp = event => {
        this.isMouseDown = false
        if (
          (event.target === this._rootNode ||
            !this._rootNode.contains(event.target)) &&
          this.getState().isOpen
        ) {
          this.reset({type: Downshift.stateChangeTypes.mouseUp}, () =>
            this.props.onOuterClick(this.getStateAndHelpers()),
          )
        }
      }

v1.28.3 solution:

const onMouseUp = event => {
        const {document} = this.props.environment
        this.isMouseDown = false
        if (
          (event.target === this._rootNode ||
            !this._rootNode.contains(event.target)) &&
          this.getState().isOpen &&
          (!this.inputId || document.activeElement.id !== this.inputId)
        ) {
          this.reset({type: Downshift.stateChangeTypes.mouseUp}, () =>
            this.props.onOuterClick(this.getStateAndHelpers()),
          )
        }
      }

What you did and what happened:

v1.28.1 Issue

Problem description:

Previously, downshift would reset if your mouseUp event.target was outside of downshift even though mouseDown happened on the input element. Thanks to @jduthon, in v1.28.3, it only resets if event.target is outside of downshift AND the active element is not the input element. This solution, however, does not support multiple inputs.

Suggested solution:

An alternative solution could be to only reset if event.target is NOT within the downshift root node AND the active element is in the root downshift node. This option would support multiple input elements. However, it would also mean that clicking on the label of a downshift input would NOT cause a reset.

I opened this issue hoping to get additional opinions on this as a result of #354 (comment). Thoughts??

[jduthon]

I believe we could also check if the document.activeElement is an input? That would cover the case with multiple inputs. It would though leave the button case aside.

[kentcdodds]

Thanks for opening this @austintackaberry!

That would cover the case with multiple inputs.

As I mentioned, that was just an example use case. I'm sure there are more situations. What we need to decide is generally: "If the user clicks on something within the downshift component that isn't an item, should we reset the component?" If it's an item, then a whole different code path will follow so we don't need to worry about those, but who knows what else someone could render within the downshift component. If we did this and someone decided they do want a reset on the label, then they simply add: onClick: reset to their getLabelProps call and forget about the workaround because it was so straightforward.

The reason we're considering the label is because it's a pretty common use case to render a label within downshift. As I was thinking about it, I considered the browser's default behavior with a label and its associated form input:

Notice how clicking on the label actually interacts with the checkbox and radio button? I didn't show it, but when you click on a label for an <input /> the input is focused. (demo on bootstrap's site)

So it makes sense to me to not reset when clicking on the label. Perhaps it makes sense to not reset when clicking anywhere within the downshift component as well...

Except.... Now that I think about it, using document.activeElement wont be enough for this because the label is not (by itself) focusable anyway, so it can never be the active element 🤔 Maybe I'm overthinking this... Actually, perhaps document.activeElement is the right approach because it will only prevent a reset when the element is something that's focusable and within downshift which should probably cover our bases pretty well...

I'm thinking this wont be nearly as impactful as I initially assumed.