Handling Events, prevent default, stop propagation

[johnjesse]

This is a question, and this may not be the correct forum for it as it's more about events themselves than Downshift.

Downshift handles the keyDown event and calls preventDefault on the event https://github.com/paypal/downshift/blob/27a19a31fb3f3f9475d29080817c2ad866c6eed6/src/downshift.js#L556

It doesn't call stopPropagation - I'm assuming that's by design.
This causes an issue for me as I use a downshift select input inside another element that is also listening to keyDown events.

My question is why does it only call preventDefault and not stopPropagation too? Downshift has handled the event so why does it allow it to bubble up and be potentially handled by someone else. I'm really interested in the rationale for only using preventDefault in this case and how this applies generally to handling events.

[kentcdodds]

It's mostly because I'm not that smart and I wasn't sure what made the most sense. I can be convinced to use stopPropagation as well 😅

[johnjesse]

haha, fair enough,

so I'd argue that it should be using stopPropagation too, it's handled the event and no one else should be. By not calling stopPropagation anything further up the Dom tree would have to check the event to see if it's already been handled.

I'd be happy to submit a PR

[kentcdodds]

That sounds good to me, so long as there's a way to prevent that (like we have with preventDefault). Thanks for your help and understanding <3

Also, your username is the same name as my father-in-law....

Dad?

[timvracer]

NOOOOO This is wrong and has broken my app in 3.8, with NO WORKAROUND. If you want to stop propagation, just create a key handler and stop stop it for the key events you wish to not propagate. Some of us badly need to know when ESCAPE is hit, and the element that "needs to know" is not the immediate parent element. Things were fine as they were, but now broken in 3.8. PLEASE remove the stopPropagation on 'ESCAPE', and for those who want the event to stop, it is very easily handled with a keyEvent handler per the documentation.

[kentcdodds]

Sorry to break your app. I agree. I think it would be better to propagate. Feel free to open a pull request to fix this.

[timvracer]

Thanks Kent, I submitted a new issue for this... is that good enough, or do I need to go and make the change in the source? (I did not inspect the source, I just saw this issue and realized what had happened).

Happy to try and figure it out in the source and create a PR if that is what you would like.

[johnjesse]

@timvracer sorry that this broke your app, can't you use preventDownshiftDefault to stop this behaviour if you don't want it?

[johnjesse]

@timvracer One of the reasons I didn't like the workaround you suggested is it ties my implementation to knowing the state of the downshift element to decide whether to stop the propagation of the key event.

e.g. If I put the downshift element in a dialog and hit escape when the downshift element is focused I need to know if downshift has actually processed the key - which it may or may not have done based on the state of the control, to then decide whether the event can be propagated... cos it will also cause the dialog to close...

[johnjesse]

I see you opened a new issue, I'll comment there

[linzhaoken]

Hi @johnjesse Thanks for opening this ticket, I am facing the exact same issue that I need the stoppropagation() to isolate event in downshift from surrounding component. So do you mind share how do you resolve your issue, since your pr is reverted in the downshift?

[johnjesse]

my Select component that uses downshift also adds an eventhandler that handles the keydown event - where I explicitly stop propagation on the event IF it's an event that downshift has handled - in my case if it's an up/down arrow key or if it's an escape key and my dropdown is still open

[linzhaoken]

@johnjesse thanks a lot for your quick reply, that gives me some direction, my situation is more complicated, I am using a component library that is using downshift to create a custom dropdown. I might need to talk to the author of that library to make the change, or I could folk a version just for a quick solution.
Thanks again!!!

[silviuaavram]

If we can help from downshift feel free to create a separate issue. Thank you!

[linzhaoken]

@silviuaavram thanks for the suggestion, I think @johnjesse make a very valid point in this ticket about propagation, any event outside of downshift should stay outside, shouldn't affect event handling inside downshift. I am sad the previous PR is reverted. From my current project (it is a very large old kind of industrial level application), it is highly impacted. below is one of the case that blocks me.

• I use dropdown inside the form, if I click enter key while selecting an option, it will call form submit function also. I could not make change to other form elements because I am integrating React component to a dojo js code. try to isolate the new implementation from the old code.

[johnjesse]

@linzhaoken I'm still of that opinion - if you've handled an event - you've handled it - and no one else should have to care or check. But I certainly understand the issue with stopping event propagation in libraries like this one - you're denying people the chance to do something different - and whose to say when you really know that an event has been "handled" - if down shift is used as a controlled component and handles the event to raise a state change event - that is then ignored - has it really "handled" the event...

anyway we ended up reverting because the alternative was

1. Making downshift do the right thing by default (see above for problems with defining the right thing)
2. We'd still want to provide a work around for people who didn't want it to handle events in this way

In the end it seemed easier to just work around the issue myself - I'm the consumer of 3rd party code and need to build a seam in my app where I consume it - that seam is always going to be somewhat coupled to the 3rd party code - in this case I happen to know exactly how downshift is handling the keydown event, it's not ideal and means I'm coupled somewhat to downshifts internals when I upgrade - But really, I think I'd class the events it handles as being part of the public API - it's how the user interacts with the drop down - so it's not too bad.

And as no one else was complaining it didn't seem like a major issue either.

[joemun]

+1 @johnjesse

I've encountered the same issue. We're using downshift within a React modal. As an example, pressing escape on the downshift combobox should just close the combobox itself, but due to the event propagating up the tree, it closes the entire enclosing modal instead.

react-modal is an example of another library that does implement the proposed behavior (ref). It'll stop propagation of the event after it's been handled by the library itself.