events: setting event cancelBubble calls stopPropagation#50405

mertcanaltin · 2023-10-26T08:33:10Z

In this issue, we addressed the problem where setting cancelBubble to true unintentionally called the stopPropagation method. The solution ensures that stopPropagation is only called when cancelBubble is explicitly set to false, as it should be. This change prevents the method from being called when cancelBubble is true or not explicitly set

lib/internal/event_target.js

Co-authored-by: Deokjin Kim <[email protected]>

anonrig · 2023-10-26T17:22:01Z

Can you add a test?

benjamingr

Please add a test, changes lgtm

mertcanaltin · 2023-11-12T13:34:16Z

ı added a test

jeanbern · 2023-11-16T04:12:15Z

lib/internal/event_target.js

 thrownewERR_INVALID_THIS('Event');
 if(value){
-this.stopPropagation();
+this.#propagationStopped =true;


Is setting cancelBubble supposed to be one-way? The proposed change doesn't modify this.#propagationStopped when value is false
nvm: For anyone else wondering the same thing see here: whatwg/dom#211

nodejs-github-bot · 2023-12-09T03:59:01Z

CI: https://ci.nodejs.org/job/node-test-pull-request/56173/

nodejs-github-bot · 2023-12-25T03:59:12Z

CI: https://ci.nodejs.org/job/node-test-pull-request/56522/

mertcanaltin · 2023-12-30T13:14:55Z

I wonder if I should do an update here?

nodejs-github-bot · 2023-12-31T02:15:38Z

CI: https://ci.nodejs.org/job/node-test-pull-request/56603/

nodejs-github-bot · 2024-01-01T21:49:24Z

CI: https://ci.nodejs.org/job/node-test-pull-request/56621/

nodejs-github-bot · 2024-01-02T02:05:41Z

CI: https://ci.nodejs.org/job/node-test-pull-request/56624/

mertcanaltin · 2024-01-06T17:18:41Z

I guess flakky was the test 🤔 💭

nodejs-github-bot · 2024-01-06T20:22:19Z

CI: https://ci.nodejs.org/job/node-test-pull-request/56696/

nodejs-github-bot · 2024-01-09T22:39:22Z

CI: https://ci.nodejs.org/job/node-test-pull-request/56723/

nodejs-github-bot · 2024-01-17T12:50:08Z

CI: https://ci.nodejs.org/job/node-test-pull-request/56821/

nodejs-github-bot · 2024-01-21T02:01:17Z

CI: https://ci.nodejs.org/job/node-test-pull-request/56862/

mertcanaltin · 2024-01-21T08:36:16Z

passed 🎉🚀

mertcanaltin · 2024-01-25T17:56:08Z

@benjamingr I would be very happy if you can make an update here, now it seems to have passed all the tests

mhdawson

LGTM

mhdawson · 2024-02-26T21:55:29Z

@mertcanaltin I'm getting this warnings when landing -
GitHub cannot link the author of 'events: setting event cancelBubble calls stopPropagation' to their GitHub account.
⚠ Please suggest them to take a look at https://github.com/nodejs/node/blob/99b1ada/doc/guides/contributing/pull-requests.md#step-1-fork

Can you take a look? I'll land anyway but would be good to get that fixed up.

PR-URL: #50405 Reviewed-By: Matthew Aitken <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Deokjin Kim <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

mhdawson · 2024-02-26T22:02:40Z

Landed in 399654f

mertcanaltin · 2024-02-27T06:17:46Z

I am very sorry for the late response, thank you very much for correcting me @mhdawson ❤️ 🚀

PR-URL: #50405 Reviewed-By: Matthew Aitken <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Deokjin Kim <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

PR-URL: nodejs#50405 Reviewed-By: Matthew Aitken <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Deokjin Kim <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

events: setting event cancelBubble calls stopPropagation
0473c3e

nodejs-github-bot added the needs-ci PRs that need a full CI run. label Oct 26, 2023

mertcanaltin requested a review from KhafraDev October 26, 2023 10:37

mertcanaltin added events Issues and PRs related to the events subsystem / EventEmitter. eventtarget Issues and PRs related to the EventTarget implementation. labels Oct 26, 2023

deokjinkim reviewed Oct 26, 2023
View reviewed changes

lib/internal/event_target.js Outdated Show resolvedHide resolved

Update lib/internal/event_target.js
b1fc248
Co-authored-by: Deokjin Kim <[email protected]>

KhafraDev approved these changes Oct 26, 2023
View reviewed changes

benjamingr requested changes Oct 26, 2023
View reviewed changes

added test
9742ede

mertcanaltin requested a review from benjamingr November 13, 2023 09:59

jeanbern reviewed Nov 16, 2023
View reviewed changes

Scout2012 mentioned this pull request Dec 6, 2023
events: setting Event.cancelBubble calls stopPropagation #50401
Closed

mertcanaltin requested review from KhafraDev and deokjinkim December 8, 2023 18:03

KhafraDev approved these changes Dec 8, 2023
View reviewed changes

deokjinkim approved these changes Dec 9, 2023
View reviewed changes

deokjinkim added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 9, 2023

github-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 9, 2023

jasnell approved these changes Dec 9, 2023
View reviewed changes

benjamingr approved these changes Jan 1, 2024
View reviewed changes

mertcanaltin added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 3, 2024

mertcanaltin removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 3, 2024

benjamingr added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 6, 2024

github-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 6, 2024

mertcanaltin added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 17, 2024

github-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 17, 2024

mertcanaltin added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 18, 2024

mhdawson approved these changes Feb 26, 2024
View reviewed changes

mhdawson closed this Feb 26, 2024

marco-ippolito mentioned this pull request Mar 1, 2024
v21.7.0 proposal #51932
Merged

richardlau mentioned this pull request Mar 25, 2024
v20.12.0 proposal #52212
Merged

Uh oh!

events: setting event cancelBubble calls stopPropagation#50405

events: setting event cancelBubble calls stopPropagation #50405

Uh oh!

Conversation

mertcanaltin commented Oct 26, 2023

Uh oh!

Uh oh!

anonrig commented Oct 26, 2023

Uh oh!

benjamingr left a comment

Choose a reason for hiding this comment

Uh oh!

mertcanaltin commented Nov 12, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jeanbernNov 16, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Dec 9, 2023

Uh oh!

nodejs-github-bot commented Dec 25, 2023

Uh oh!

mertcanaltin commented Dec 30, 2023

Uh oh!

nodejs-github-bot commented Dec 31, 2023

Uh oh!

nodejs-github-bot commented Jan 1, 2024

Uh oh!

nodejs-github-bot commented Jan 2, 2024

Uh oh!

mertcanaltin commented Jan 6, 2024

Uh oh!

nodejs-github-bot commented Jan 6, 2024

Uh oh!

nodejs-github-bot commented Jan 9, 2024

Uh oh!

nodejs-github-bot commented Jan 17, 2024

Uh oh!

nodejs-github-bot commented Jan 21, 2024

Uh oh!

mertcanaltin commented Jan 21, 2024

Uh oh!

mertcanaltin commented Jan 25, 2024

Uh oh!

mhdawson left a comment

Choose a reason for hiding this comment

Uh oh!

mhdawson commented Feb 26, 2024

Uh oh!

mhdawson commented Feb 26, 2024

Uh oh!

mertcanaltin commented Feb 27, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

mertcanaltin commented Nov 12, 2023•
edited
Loading

jeanbernNov 16, 2023•
edited
Loading