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

Splattributes, {{action}} modifier and component event handlers #17877

Open
chancancode opened this issue Apr 7, 2019 · 5 comments
Open

Splattributes, {{action}} modifier and component event handlers #17877

chancancode opened this issue Apr 7, 2019 · 5 comments

Comments

@chancancode
Copy link
Member

See #17874 (comment)

As part of the fix we need to decide which one fires first, since it was not specified in the RFC. My gut feeling is that the component’s handler should fire first and given the option to cancel it.

@chancancode
Copy link
Member Author

@simonihmig do you want to work on this one too?

@chancancode
Copy link
Member Author

This one is [BUGFIX canary] only, the feature was added after the beta branch point.

/cc @cibernox

@simonihmig
Copy link
Contributor

do you want to work on this one too?

I can do it, but probably only more towards the end of the week...

My gut feeling is that the component’s handler should fire first and given the option to cancel it.

Agree on the order. Not sure about the cancelation.

Currently returning false means we do event.stopPropagation() (or look into event.cancelBubble with the recent changes to see if the user called event.stopPropagation()), and prevent the event to bubble up within our own custom bubbling up implementation.

But here we are dealing with the case that we have multiple listeners on the same element, which event.stopPropagation() does not cancel. event.stopImmediatePropagation() would do that. But not sure if returning false should have stopImmediatePropagation() semantics? And when the user has called stopImmediatePropagation(), we have no way to find that out, as there seems no equivalent of cancelBubble for that case, AFAICT.

So not really sure if we can/should allow event cancelation for all the listeners of the same element here? Which is really ironic, as this was exactly the example I brought up as a "dangerous" thing during the RFC process! 🤷‍♂️😂

@chancancode
Copy link
Member Author

But not sure if returning false should have stopImmediatePropagation() semantics?

I think it should not. return false is modeled after the jQuery semantics, which I believe is event.stopPropagation() not event.stopImmediatePropagation().

And when the user has called stopImmediatePropagation(), we have no way to find that out, as there seems no equivalent of cancelBubble for that case, AFAICT.

That's a good point 🤔

So not really sure if we can/should allow event cancelation for all the listeners of the same element here?

I don't "not allowing" it is not really an option, I think the main decision is whether the action handlers get run before or after the component handler. If it's after, then stopPropagation === stopImmediatePropagation, so that's a bit easier for us. Maybe that's our only option anyway?

@chancancode
Copy link
Member Author

The {{on ...}} thing your brought up in the RFC actually has an even worse problem – because it uses addEventListener on the underlying element, it will not go through the EventDispatcher and so it will always fire first. But that is a general problem anyway, you can observe that difference by passing {{on ...}} and {{action ...}} on a regular element.

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

No branches or pull requests

2 participants