[BUGFIX beta] Fix no-jQuery EventDispatcher to prevent events bubbling up when event.stopPropagation() was called

[simonihmig]

Fixes #17840

[chancancode]

👏

[chancancode]

Does return false work?

[chancancode]

This is probably not the correct nesting for this. I believe we want to check the flag after the event handler has run (currently the check runs when if there is no event handler). Further, we probably need the same treatment for the action branch too. So I think you probably want to do it before setting the target to the parent.

I feel like this should have failed the test tho?

[chancancode]

Also, we should make sure to mirror the jquery/dom behavior that stop propagation doesn’t prevent handlers on the same element from firing

[chancancode]

On that note, we probably have another bug here. It seems like the code assumes you can either have a component element handler, or action handlers, but not both. I believe with the possibility of forwarding action modifiers through splattributes, this is no longer a correct assumption.

[simonihmig]

I believe we want to check the flag after the event handler has run

In what scenario would that not be the case? viewHandler(target, event) will always be called before our cancelBubble check, right?

Further, we probably need the same treatment for the action branch too.

I tried to add a (failing) test for that, until I realized that the action handler will not receive a reference to the event, so cannot call stopPropagation(), or do I miss something?

stop propagation doesn’t prevent handlers on the same element from firing

How would there be two different handlers (component event handlers) on the same element? You mean the case you mentioned in your following comment? 👇

It seems like the code assumes you can either have a component element handler, or action handlers, but not both. I believe with the possibility of forwarding action modifiers through splattributes, this is no longer a correct assumption.

Hm, I see... 🤔Good catch! But we should tackle this in a separate PR I think?

[chancancode]

viewHandler(target, event) will always be called before our cancelBubble check, right?

You're right!

action handler will not receive a reference to the event

Don't we pass it here?

How would there be two different handlers (component event handlers) on the same element? You mean the case you mentioned in your following comment? 👇

Yep that's one case I could think of. I think you could also do <div {{action "foo"}} {{action "bar"}} />? Does that work? (Looking at the code, I'm not sure where this is handled.)

But we should tackle this in a separate PR I think?

Seems fine to me!

[simonihmig]

Don't we pass it here?

Yes, but that goes to

ember.js/packages/@ember/-internals/glimmer/lib/modifiers/action.ts

Lines 125 to 181 in 8326372

	handler(event: Event): boolean {
	let { actionName, namedArgs } = this;
	let bubbles = namedArgs.get('bubbles');
	let preventDefault = namedArgs.get('preventDefault');
	let allowedKeys = namedArgs.get('allowedKeys');
	let target = this.getTarget();
	let shouldBubble = bubbles.value() !== false;
	if (!isAllowedEvent(event, allowedKeys.value())) {
	return true;
	}
	if (preventDefault.value() !== false) {
	event.preventDefault();
	}
	if (!shouldBubble) {
	event.stopPropagation();
	}
	join(() => {
	let args = this.getActionArgs();
	let payload = {
	args,
	target,
	name: null,
	};
	if (typeof actionName[INVOKE] === 'function') {
	flaggedInstrument('interaction.ember-action', payload, () => {
	actionName[INVOKE].apply(actionName, args);
	});
	return;
	}
	if (typeof actionName === 'function') {
	flaggedInstrument('interaction.ember-action', payload, () => {
	actionName.apply(target, args);
	});
	return;
	}
	payload.name = actionName;
	if (target.send) {
	flaggedInstrument('interaction.ember-action', payload, () => {
	target.send.apply(target, [actionName, ...args]);
	});
	} else {
	assert(
	`The action '${actionName}' did not exist on ${target}`,
	typeof target[actionName] === 'function'
	);
	flaggedInstrument('interaction.ember-action', payload, () => {
	target[actionName].apply(target, args);
	});
	}
	});
	return shouldBubble;
	}

, and there the event is not passed to the action handler. I believe this is how the action helper API works: the (element space(!), not closure) action helper passes only the custom args, not the event itself. That's why we have things like bubbles=false.

I think you could also do <div {{action "foo"}} {{action "bar"}} />? Does that work? (Looking at the code, I'm not sure where this is handled.)

We have that loop here:

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

Line 288 in 561759e

for (let index = 0; index < actions.length; index++) {

And a related test here, though for different events:

ember.js/packages/@ember/-internals/glimmer/tests/integration/helpers/element-action-test.js

Lines 1323 to 1360 in 561759e

	['@test allows multiple actions on a single element']() {
	let clickActionWasCalled = false;
	let doubleClickActionWasCalled = false;
	let ExampleComponent = Component.extend({
	actions: {
	clicked() {
	clickActionWasCalled = true;
	},
	doubleClicked() {
	doubleClickActionWasCalled = true;
	},
	},
	});
	this.registerComponent('example-component', {
	ComponentClass: ExampleComponent,
	template: strip`
	<a href="#"
	{{action "clicked" on="click"}}
	{{action "doubleClicked" on="doubleClick"}}
	>click me</a>`,
	});
	this.render('{{example-component}}');
	runTask(() => {
	this.$('a').trigger('click');
	});
	this.assert.ok(clickActionWasCalled, 'the clicked action was called');
	runTask(() => {
	this.$('a').trigger('dblclick');
	});
	this.assert.ok(doubleClickActionWasCalled, 'the doubleClicked action was called');
	}

I tried to add a similar test with the same event type, but that fails, as the first handled action handler returns early here:

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

Line 292 in 561759e

return action.handler(event);

So this seems to be not supported at this point. I guess it should be possible, by always looping over all action handlers, and return false if at least one returned false!?

[simonihmig]

Oh, wait a second. Actually that test with multiple {{action}} does pass w/ jQuery, so it seems it should actually work, but doesn't in no-jQuery mode!

I'll bring up another PR to fix this specific issue. As for the issue in this PR, I think this PR should be ok as it is?

[simonihmig]

Here we go: #17876

[simonihmig]

Does return false work?

Yes, I think so! This was already covered in a test:

ember.js/packages/@ember/-internals/glimmer/tests/integration/event-dispatcher-test.js

Line 109 in 561759e

['@test events bubbling up can be prevented by returning false'](assert) {