events: setting Event.cancelBubble calls stopPropagation

[KhafraDev]

Version

v22

Platform

Linux DESKTOP-L4O1H93 5.10.102.1-microsoft-standard-WSL2 #1 SMP Wed Mar 2 00:30:59 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux

Subsystem

events

What steps will reproduce the bug?

const ev = new Event('')
ev.stopPropagation = () => {
  console.log('???')
}

ev.cancelBubble = true

How often does it reproduce? Is there a required condition?

n/a

What is the expected behavior? Why is that the expected behavior?

nothing to be printed

What do you see instead?

???

Additional information

node/lib/internal/event_target.js

Lines 304 to 310 in 9c714d8

	set cancelBubble(value) {
	if (!isEvent(this))
	throw new ERR_INVALID_THIS('Event');
	if (value) {
	this.stopPropagation();
	}
	}

[KhafraDev]

I have a couple of these but I don't know if they come off as spammy. I'm hoping a new contributor might find them easy to fix.

[BenzeneAlcohol]

Anymore such good first issues? :)

[KhafraDev]

@BenzeneAlcohol #50417

[DylanTet]

Looking for some other good first issues as well!

[KhafraDev]

@DylanTet #50432

[DylanTet]

@KhafraDev thank you!

[MattSilverio]

@KhafraDev , I'm newbie here and if I understood your issue, you are currently changing the behaviour of Event.stopPropagation to just a simple console.log(???)

[MattSilverio]

Here is the interface produced by your code

And this one is produced without changing the stopPropagation function behaviour:

Calling both instances to execute StopPropagation, it looks that you call your function and the stopPropagation, since when I call stopPropagation in my instance it gives undefined since it's the same second output provided from your code

[Scout2012]

@KhafraDev could you please elaborate a bit on what makes this a bug exactly?

I see that the expectation was defined as "nothing to be printed", but that seems like it's just a byproduct of overwriting the stopPropagation method and should be allowed if the user sees fit.

If it's for the fact that cancelBubble calls stopPropagation, the spec that seems to have been used to guide the PR that introduced this behavior states that stopPropagation and cancelBubble have essentially the same responsibility of setting the stop propagation flag, cancelBubble will just only set the flag if the value provided to cancelBubble was true.

The spec doesn't say that they have to call each other, but it makes sense to me at least to only have one definition for what "setting the stop propagation flag" looks like for a given event, and have any other of the event's methods that needs to perform the same task of setting it rely on doing it in the same defined manner. If that user defined the process of setting the flag as including the execution of some logging statements, then that seems reasonable to allow.

[KhafraDev]

The behavior is implied, WHATWG specs typically do it. If we look at addEventListener (as an example), the spec doesn't mention that the first argument must be coerced to a string, but the implied behavior is that it's converted to a DOMString because of its idl definition. Similarly, it's implied that setting cancelBubble won't have any side effects because it's not mentioned in the spec.

If we go deeper into spec, cancelBubble's steps are to "set this’s stop propagation flag if the given value is true; otherwise do nothing". The spec explicitly tells implementations to set the flag directly, rather than calling stopPropagation.

[Scout2012]

Good point, thank you for the elaboration!

So to make sure I understand, the bug here is the unspec'd/implied relationship between stopPropagation and cancelBubble, and the resolve would just be to specifically only set the stop propagation flag (when value is true) in cancelBubble's setter? If so, it definitely does sound like a good first issue indeed!

Thanks for logging it, I'll work on a PR 🙂.

[KhafraDev]

If you have any questions, I can answer them (they might be answered in the contributing guide already). Make sure to add in a test too! The change looks good to me.

[Scout2012]

I somehow missed that there was already a PR open that addresses this issue🤦. Still has been a good exercise. I'll keep my eye out for other issues!

[sedaklnc]

hello #50401 can we close this issue because it has been merged

[mertcanaltin]

as @sedaklnc said #50405 pr was also merge as said I am closing everyone's hands health ❤️