events: add stop propagation flag to Event.stopImmediatePropagation by mikemadest · Pull Request #39463 · nodejs/node · GitHub

Spec mention stopImmediatePropagation should set both flags:
"stop propagation" and "stop immediate propagation".

So the second is not supported by Node as there is no hierarchy and bubbling,
but the flags are both present as well as stopPropagation.

Would it make sense to follow specs on that?
Refs: https://dom.spec.whatwg.org/#dom-event-stopimmediatepropagation

I didn't want to make a description too long, but feel like a bit more context could be interesting:
As part of contributing to linkedom (https://github.com/WebReflection/linkedom), I was adding event bubbling there.

When Event and EventTarget are available in Node js, the choice was to extend them to provide what was needed (bubbling in dispatchEvent).
Since node don't need the bubbling part it was expected
(I had a good read at #33556 (comment)).

But since Event had everything needed I didn't think it would be required there, except for that tiny missing line in stopImmediatePropagation and it felt like maybe this should be here?
Hoping I don't waste anybody's time here, I understand it's not game changer.

Should this be closed?
This was approved but now is quite old and I'm not sure if there is any interest in merging it. I solved the conflicts just in case...

No this fix looks correct it was just missed - apologies this is our bad.

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

No this fix looks correct it was just missed - apologies this is our bad.

No worries, thank you for the answer!
There seems to be errors on the tests, I'll check that and will be active on this so if you don't see any changes, I'm probably working on them or trying to understand what's wrong.

Pull request updated:
solved conflicts, rebased, tested (everything passed) and pushed again. Everything should be fine now.

Thank you for your patience we truly don't deserve it and apologies for the contribution experience.

Let's run CI and land this as soon as it's green.

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

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

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

Commit Queue failed

- Loading data for nodejs/node/pull/39463
✔  Done loading data for nodejs/node/pull/39463
----------------------------------- PR info ------------------------------------
Title      events: add stop propagation flag to Event.stopImmediatePropagation (#39463)
Author     Mickael Meausoone  (@mikemadest, first-time contributor)
Branch     mikemadest:stop-immediate-propagation-flags -> nodejs:main
Labels     needs-ci
Commits    1
 - events: add stop propagation flag to Event.stopImmediatePropagation
Committers 1
 - Mickael Meausoone 
PR-URL: https://github.com/nodejs/node/pull/39463
Refs: https://dom.spec.whatwg.org/#dom-event-stopimmediatepropagation
Reviewed-By: James M Snell 
Reviewed-By: Benjamin Gruenbaum 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/39463
Refs: https://dom.spec.whatwg.org/#dom-event-stopimmediatepropagation
Reviewed-By: James M Snell 
Reviewed-By: Benjamin Gruenbaum 
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last approving review:
   ⚠  - events: add stop propagation flag to Event.stopImmediatePropagation
   ℹ  This PR was created on Tue, 20 Jul 2021 08:02:57 GMT
   ✔  Approvals: 2
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/39463#pullrequestreview-710658397
   ✔  - Benjamin Gruenbaum (@benjamingr) (TSC): https://github.com/nodejs/node/pull/39463#pullrequestreview-1569362103
   ✘  Last GitHub CI failed
   ℹ  Last Full PR CI on 2023-08-14T09:35:27Z: https://ci.nodejs.org/job/node-test-pull-request/53301/
- Querying data for job/node-test-pull-request/53301/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/5859369572

Spec mention stopImmediatePropagation should set both flags: "stop propagation" and "stop immediate propagation". So the second is not supported by Node as there is no hierarchy and bubbling, but the flags are both present as well as stopPropagation. It would makes sense to follow specs on that. Refs: https://dom.spec.whatwg.org/#dom-event-stopimmediatepropagation

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

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

Landed in 04cf8c2

Spec mention stopImmediatePropagation should set both flags: "stop propagation" and "stop immediate propagation". So the second is not supported by Node.js as there is no hierarchy and bubbling, but the flags are both present as well as stopPropagation. It would makes sense to follow specs on that. Refs: https://dom.spec.whatwg.org/#dom-event-stopimmediatepropagation PR-URL: #39463 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

Spec mention stopImmediatePropagation should set both flags: "stop propagation" and "stop immediate propagation". So the second is not supported by Node.js as there is no hierarchy and bubbling, but the flags are both present as well as stopPropagation. It would makes sense to follow specs on that. Refs: https://dom.spec.whatwg.org/#dom-event-stopimmediatepropagation PR-URL: nodejs#39463 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

nodejs-github-bot added the needs-ci PRs that need a full CI run. label Jul 20, 2021

mikemadest force-pushed the stop-immediate-propagation-flags branch from 33c4ebc to 23744d2 Compare July 20, 2021 08:15

mikemadest changed the title Events: add stop propagation flag to Event.stopImmediatePropagation events: add stop propagation flag to Event.stopImmediatePropagation Jul 20, 2021

jasnell approved these changes Jul 20, 2021

View reviewed changes

mikemadest mentioned this pull request Jul 20, 2021

Extend EventTarget to support event bubbling WebReflection/linkedom#84

Merged

Trott force-pushed the main branch from 2d76238 to ca3ed36 Compare November 12, 2022 01:49

benjamingr approved these changes Jun 26, 2023

View reviewed changes

benjamingr added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 26, 2023

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 26, 2023

mikemadest force-pushed the stop-immediate-propagation-flags branch from 527ff4d to 53f073d Compare June 27, 2023 11:00

benjamingr added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 27, 2023

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 27, 2023

mikemadest force-pushed the stop-immediate-propagation-flags branch from 53f073d to d7de14d Compare July 2, 2023 07:55

This was referenced Jul 3, 2023

CI Reliability 2023-07-03 nodejs/reliability#606

Open

CI Reliability 2023-07-04 nodejs/reliability#607

Open

benjamingr added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 4, 2023

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 4, 2023

mikemadest force-pushed the stop-immediate-propagation-flags branch from c4739fa to d2b2e2c Compare August 8, 2023 12:58

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

benjamingr approved these changes Aug 9, 2023

View reviewed changes

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 10, 2023

mikemadest force-pushed the stop-immediate-propagation-flags branch from d2b2e2c to d712a39 Compare August 14, 2023 07:53

benjamingr added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 14, 2023

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 14, 2023

benjamingr added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Aug 14, 2023

nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Aug 14, 2023

aduh95 force-pushed the stop-immediate-propagation-flags branch from d712a39 to 991ae2e Compare May 11, 2024 17:54

aduh95 added author ready PRs that have at least one approval, no outstanding review comments, and a CI started. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels May 11, 2024

aduh95 merged commit 04cf8c2 into nodejs:main May 12, 2024

targos mentioned this pull request May 13, 2024

v22.2.0 release proposal #52971

Merged

marco-ippolito mentioned this pull request Jun 17, 2024

v20.15.0 proposal #53486

Merged

events: add stop propagation flag to Event.stopImmediatePropagation by mikemadest · Pull Request #39463 · nodejs/node · GitHub

Uh oh!

Uh oh!

events: add stop propagation flag to Event.stopImmediatePropagation - #39463

events: add stop propagation flag to Event.stopImmediatePropagation#39463aduh95 merged 1 commit intonodejs:mainnodejs/node:mainfrom mikemadest:stop-immediate-propagation-flagsmikemadest/node:stop-immediate-propagation-flagsCopy head branch name to clipboard

Conversation

mikemadest commented Jul 20, 2021

Uh oh!

mikemadest commented Jul 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mikemadest commented Jun 26, 2023

Uh oh!

benjamingr commented Jun 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Jun 26, 2023

Uh oh!

mikemadest commented Jun 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mikemadest commented Jun 27, 2023

Uh oh!

benjamingr commented Jun 27, 2023

Uh oh!

nodejs-github-bot commented Jun 27, 2023

Uh oh!

nodejs-github-bot commented Aug 10, 2023

Uh oh!

nodejs-github-bot commented Aug 14, 2023

Uh oh!

nodejs-github-bot commented Aug 14, 2023

Uh oh!

nodejs-github-bot commented May 11, 2024

Uh oh!

nodejs-github-bot commented May 12, 2024

Uh oh!

aduh95 commented May 12, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

events: add stop propagation flag to Event.stopImmediatePropagation#39463
aduh95 merged 1 commit into
nodejs:mainfrom
mikemadest:stop-immediate-propagation-flags

mikemadest commented Jul 20, 2021 •
edited
Loading

benjamingr commented Jun 26, 2023 •
edited
Loading

mikemadest commented Jun 26, 2023 •
edited
Loading