Downshift: Change how to prevent default events

[kentcdodds]

Right now if you add a custom event handler (like onKeyDown to the getInputProps for example), you can prevent the default behavior with event.preventDefault(). This has worked pretty well, but honestly it's overloading the feature of preventing the default from events so we should come up with another way to signal preventing the default behavior.

What if instead you simply add a property to the event like: event.preventDownshiftDefault = true and then downshift references that rather than event.defaultPrevented?

I think that'd be pretty simple and straightforward and the upgrade for this breaking change would be pretty smooth. In fact, we could support both for a little while without much trouble.

Thoughts?

[ericedem]

Ok so just to clarify the particular case mentioned in #330. The new workaround will be:

getInputProps({
  onKeyDown(event) {
    if (event.key === 'Enter') {
      // If nothing is highlighted, let the normal input event through.
      if (!isOpen || highlightedIndex === null) {
        event.preventDownshiftDefault = true;
      }
    }
  }
})

Which overrides the default behavior of calling event.preventDefault() when there is no item highlighted, but otherwise let Downshift behave normally.

[kentcdodds]

Thanks for the example @ericedem. That's exactly what I would do :+1:

[ericedem]

If there is a consensus, happy to take this one on.

[kentcdodds]

So I want this to be implemented. I'd be happy to implement it myself, but I'm happy to accept a PR from anyone who gets to it before me.

For this first iteration we should implement it as a non-breaking change and support both this and the current mechanism. It shouldn't be too much work. Probably just documentation, tests, and an update in the utils file for composeEventHandlers:

https://github.com/paypal/downshift/blob/10d2cf6128bc008bd7984d537edd812dd2e4306e/src/utils.js#L132-L146

Should probably just be:

// TODO: remove everything after the || in the next breaking change
return event.preventDownshiftDefault || event.defaultPrevented

[kentcdodds]

Perfect @ericedem :+1: Thanks!

[ericedem]

@kentcdodds I have an implementation working for this, where do you recommend putting docs, Under getInputProps perhaps? Or maybe a new section related to handlers?

[kentcdodds]

Let's add a new section for it. It applies to several prop getters

[k-ode]

Just wanted to add that this would be great. I was very confused by the current behavior. We use Downshift together with Draft.js and have to use this ugly thing to make them play nice together.

getNewInputProps(downshiftProps) {
    const inputProps = downshiftProps.getInputProps();
    const inputOnKeyDown = inputProps.onKeyDown;
    inputProps.onKeyDown = ev => {
         ev.defaultPrevented = false;

         inputOnKeyDown(ev);

         ev.preventDefault();

         this.onKeyDown(ev);
     };
     return inputProps;
}

[kentcdodds]

Yeah, it was kind of a last minute feature and in retrospect wasn't thought through very well 😅

[pcattori]

@kimgronqvist any chance you can provide a link to a more complete example?

I'm also trying to integrate draftjs with downshift, but it seems that draftjs is eating the up/down/escape events and not letting downshift see them.

[k-ode]

Sure! This example conveys the general idea: https://codesandbox.io/s/zx9z9275xm

[pcattori]

@kimgronqvist you missed a pair of {} to destructure onKeyDown from getInputProps 😛 :

const { onKeyDown } = getInputProps();

link with the fix

thanks!

[TLadd]

The correct way to do this now is to set the field on the native event rather than the synthetic event, since React reuses synthetic events. The README has a correct example