Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Honor event.stopPropagation #20

Closed
pashasadri opened this issue Nov 23, 2015 · 8 comments
Closed

Honor event.stopPropagation #20

pashasadri opened this issue Nov 23, 2015 · 8 comments
Milestone

Comments

@pashasadri
Copy link

DOM (and React) events bubble up the DOM unless a handler in the chain calls e.stopPropagation(). This behavior gives developers control over how events are handled.

HotKeys used to have this behavior but it changed some time since 0.5.0. Now, if there is a child handler the keyboard event stops processing regardless of whether the child handler handled the event or not. It is no longer possible to write code like this:

...
handleUpKey: (event) => {
    if (this.x) {
        event.stopPropagation();
        this.doX(); 
    } else {
        // let event bubble and be handled by parent
    }
},
render: () => {
    return <HotKeys handlers={{'up': this.handleUpKey}}>...</HotKeys>;
}

It would be great to have the DOM like behavior back.

@chrisui
Copy link
Contributor

chrisui commented Nov 23, 2015

Hmm, interesting. Cheers for the report!

This may end up bubbling to the underlying Mousetrap library but may also be related to "did the child handler handle the event" logic.

Just to confirm your exact use-case, are you saying you are calling event.stopPropagation() but its not stopping or that the event is not propagating from a child handler no matter what?

(Both cases mentioned there of course should be fixed I'm just trying to find your exact repo!)

@pashasadri
Copy link
Author

My use case is something like this:

Main component:

focusSearch: function(event) {
    this.refs.search.focus();
},
render: function() {
return 
<HK handlers={{up: this.focusSearch}}>
   <Search ref="search"/>
   <List ref="list" ... />
</HK>;
}

List Component:

prev: function() {
   if (this.state.selected === this.props.items[0]) {
     // do nothing.  let the event bubble and be handled by Main
     return;
   } else {
     // move selection up
   }
},
render: function() {
return
<HK handlers={{up: this.prev, down: this.next}}>
   <Item />
   <Item />
   ...
</HK>;
}

List handles up/down arrows to move the selection up/down the items in the list. I want a behavior where if selected item === first item in the list, pressing up focuses the Search field. In this case, I want the 'up' event to be left un-handled by List's handler and bubble to Main's focusSearch.

One way could be to simply not register a List 'up' handler when the above condition is true. However, for performance reasons, I avoid re-rendering the List. And even if we did want to re-render, it still begs the question of how event.stopPropagation should play into HotKeys?

@chrisui
Copy link
Contributor

chrisui commented Nov 24, 2015

Gotcha! (I think)

So what's happening here is the following internal logic: "If a child has handled an event of the same sequence (in this case the sequence is simply up) then any parent handlers shouldn't be called".

I am not sure we can ride off of the standard event api here because stopPropagation would have some nasty side effects. Ie. imagine you had a parent key sequence of up down left right and your child up sequence prevented the first up event propagating - you'd break the parent sequence!

What we need is a way to say "I have (or have not) handled this sequence so keep going" rather than the fact a child handler is registered be the overriding factor. Two solutions off the bat could be a return false from handler indicating we didn't do anything or handler accepting an extra parameter which resembles something like the event api but is actually some sort of "hotkey event api".

Going to have a mull over this now. Feel free to spitball any thoughts or ideas you have!

@pashasadri
Copy link
Author

True. I didn't think of the case where the child handles a subset of a parent's sequence.

@chrisui
Copy link
Contributor

chrisui commented Nov 24, 2015

Leaning towards a "hotkey event" api to be more synonymous with standard events. Also returning, essentially "magic", values is not as clear from a reader POV or as flexible.

@chrisui chrisui mentioned this issue Jan 13, 2016
8 tasks
@chrisui
Copy link
Contributor

chrisui commented Jul 22, 2016

This is documented for a v1 refactor as it has some broader api implications. Best handling prevention of this behaviour manually currently by passing an explicit callback prop down.

Closing for now.

@chrisui chrisui closed this as completed Jul 22, 2016
@greena13
Copy link
Owner

Reopening to better get up to speed on the context and justification for this issue, tracked separately.

@greena13 greena13 reopened this Nov 19, 2017
@greena13 greena13 added this to the v1.0.0 milestone Nov 19, 2017
@greena13
Copy link
Owner

This will be fixed in v2.0.0-pre1 when it's released, soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants