search

emberjs / ember.js

Ember.js - A JavaScript framework for creating ambitious web applications
https://emberjs.com
MIT License
22.45k stars 4.21k forks source link

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

Open chancancode opened 5 years ago

chancancode commented 5 years ago

See https://github.com/emberjs/ember.js/pull/17874#discussion_r272809665

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 commented 5 years ago

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

chancancode commented 5 years ago

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

/cc @cibernox

simonihmig commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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.