[BUGFIX beta] Fix no-jQuery EventDispatcher to allow handling multiple element-space actions

[simonihmig]

Related to #17874 (comment)

[chancancode]

Looking good!

ember.js/packages/@ember/-internals/views/lib/system/event_dispatcher.js

Lines 361 to 371 in 18d9087

	if (getElementView(target)) {
	if (viewHandler(target, event) === false) {
	event.preventDefault();
	event.stopPropagation();
	break;
	}
	} else if (target.hasAttribute('data-ember-action')) {
	if (actionHandler(target, event) === false) {
	break;
	}
	}

It bothers mean that these two branches are quite diverged. You would really expect this to be something like:

let rtn = false;

if (getElementView(target)) {
  rtn = viewHandler(target, event);
} else if (target.hasAttribute('data-ember-action')) {
  rtn = viewHandler(target, event);
}

if (rtn === false) {
  event.preventDefault();
  event.stopPropagation();
  break;
}

But it's not. I suspect this is probably a long standing, or maybe it was introduced during Glimmer 2 migration, but it seems wrong that returning false in component handlers vs action handlers mean different things. Specifically, in components it means preventDefault + stopPropagation (which is what it means to return false from an event handler in jQuery), in the other it does something else. And it looks like you can't even return false in your own action handlers. Maybe we should try to track and fix that separately.

[chancancode]

I think this is not quite right: if the first handler returned false, that will prevent the remaining handlers (on the same elements) from running. That would be event.stopImmediatePropagation() semantics in jQuery, and I don't think that's what we want here.

[simonihmig]

Ah yes, excellent catch! I switched the order of those && operands, and added another test to cover this!

[chancancode]

By the way, is this right?

ember.js/packages/@ember/-internals/views/lib/system/event_dispatcher.js

Line 342 in 18d9087

// mouseEnter/Leave don't bubble, so there is no logic to prevent it as with other events

I understand that the native mouseEnter/Leave events does not themselves bubble, however, since we are synthetically bubbling them, if you do return false or call stopPropagation, should we not stop the synthetic bubbling (i.e. breaking out of the loop)? Also the if you can preventDefault on the synthetic event (or return false in the component handler), are we supposed to call preventDefault on the underlying mapped event?

(The answer to those are whatever jQuery does, but I don't happen to know the answer to that. Anyway, it's not related to this patch so we can track/fix it separately if needed.)

[simonihmig]

It bothers mean that these two branches are quite diverged. You would really expect this to be something like:

Not sure about this, but @rwjblue might know? This was already present here: https://github.com/rwjblue/ember-native-dom-event-dispatcher/blob/master/addon/index.js#L124-L133, which the no-jquery code was based on AFAIK.

if you do return false or call stopPropagation, should we not stop the synthetic bubbling (i.e. breaking out of the loop)?

First, this loop is a bit different from the other (doing the synthetic bubbling), in that it tries only to mimic how mouseenter events are triggered, i.e. it dispatches separate events, and does not bubble up all the way to the root element. And by dispatching separate events, calling stopPropagation on one of them should not affect the others I think, as this would mean we would introduce different semantics for the mouseenter event, wouldn't it? (with normal event handlers stopPropagation has no effect for mouseenter, so why should it have here?)

Btw, this is related: emberjs/rfcs#474. Have been thinking about this a while ago, but just today submitted this issue. Maybe you have some thoughts?

The answer to those are whatever jQuery does, but I don't happen to know the answer to that.

The implementation of this mouseenter support was largely based on what jQuery does, so I think it should do more or less the same. But would have to check again to make sure...

[chancancode]

All sounds reasonable to me! Thanks for working on this!

I'm thinking we would back port these to as far back as LTS, assuming these two bug exists those branches, what do you think? (also cc @rwjblue)

[simonihmig]

I'm thinking we would back port these to as far back as LTS, assuming these two bug exists those branches, what do you think?

Yes, sounds reasonable. Pretty sure those bugs must have existed for any other version as well!

[rwjblue]

I'm thinking we would back port these to as far back as LTS, assuming these two bug exists those branches, what do you think? (also cc @rwjblue)

agree

[chriskrycho]

A question about this (as @nlfurniss and I are working on upgrading an app): this PR says it is fixing the no-jQuery eventDispatcher, but this test appears (and perhaps I'm simply misunderstanding this?) to be adding tests here and below for jQuery-based event trigger. Is this using jQuery only for event triggering, but not for dispatch? Thanks!

[simonihmig]

@chriskrycho You mean because of that $() call looking like jQuery?

If so, it is actually not jQuery here. Was also confused when I first saw this. But there is some test infrastructure that adds this without actually using jQuery. Probably helped migrating tests away from jQuery back then (my guess). See

• https://github.com/simonihmig/ember.js/blob/52b6657d1984a2b524d980be7a290de416053bc0/packages/internal-test-helpers/lib/test-cases/abstract.js#L88 and
• https://github.com/simonihmig/ember.js/blob/52b6657d1984a2b524d980be7a290de416053bc0/packages/internal-test-helpers/lib/node-query.js

[chriskrycho]

We thought that might be the case but wanted to double check. Thank you very much for clarifying!