Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

events: cancelBubble is a property #33613

Closed

Conversation

benjamingr
Copy link
Member

Event#cancelBubble is property (and not a function). Change Event#cancelBubble to a property and add a test.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@benjamingr benjamingr requested a review from jasnell May 28, 2020 14:35
@benjamingr benjamingr added the events Issues and PRs related to the events subsystem / EventEmitter. label May 28, 2020
@benjamingr benjamingr changed the title event: cancelBubble is a property events: cancelBubble is a property May 28, 2020
@targos
Copy link
Member

targos commented May 28, 2020

I'd be in favor of implementing these properties by following the spec: https://dom.spec.whatwg.org/#stop-propagation-flag
That would mean that cancelBubble should use a private "stop propagation flag"

@jasnell
Copy link
Member

jasnell commented May 28, 2020

Yeah, even tho we're not making use of this at all, we should at least preserve the value of the flag.

@benjamingr
Copy link
Member Author

@targos ok, I'll update this to reflect the flag.

@benjamingr
Copy link
Member Author

@targos I pushed a property and added some tests to check the behavior. Still definitely not perfect, I would eventually like to run better tests on this (following the guide), I am starting by doing a shallow run now to get familiarized with the code - and so there are a bunch of small PRs rather than a large-ish one for compatibility.

@nodejs-github-bot
Copy link
Collaborator

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 28, 2020
@jasnell
Copy link
Member

jasnell commented May 28, 2020

@benjamingr ... thank you for going through these, btw... it would have been at least a week before I would have been able to get back to it.

benjamingr added a commit that referenced this pull request May 31, 2020
Event#cancelBubble is property (and not a function). Change
Event#cancelBubble to a property and add a test.

PR-URL: #33613
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
@benjamingr
Copy link
Member Author

benjamingr commented May 31, 2020

Landed in f643cf4

@benjamingr benjamingr closed this May 31, 2020
@benjamingr benjamingr deleted the event-target-cancel-bubble branch May 31, 2020 10:22
@benjamingr benjamingr restored the event-target-cancel-bubble branch May 31, 2020 12:11
@BridgeAR BridgeAR reopened this May 31, 2020
@BridgeAR BridgeAR force-pushed the event-target-cancel-bubble branch from d65c8d1 to b1ba21f Compare May 31, 2020 13:31
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@jasnell
Copy link
Member

jasnell commented Jun 3, 2020

@benjamingr ... this needs a rebase

@benjamingr
Copy link
Member Author

rebased

Event#cancelBubble is property (and not a function). Change
Event#cancelBubble to a property and add a test.
@nodejs-github-bot
Copy link
Collaborator

jasnell pushed a commit that referenced this pull request Jun 4, 2020
Event#cancelBubble is property (and not a function). Change
Event#cancelBubble to a property and add a test.

PR-URL: #33613
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
@jasnell
Copy link
Member

jasnell commented Jun 4, 2020

Landed in 3128915

@jasnell jasnell closed this Jun 4, 2020
codebytere pushed a commit that referenced this pull request Jun 18, 2020
Event#cancelBubble is property (and not a function). Change
Event#cancelBubble to a property and add a test.

PR-URL: #33613
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
benjamingr added a commit to benjamingr/io.js that referenced this pull request Jun 21, 2020
Event#cancelBubble is property (and not a function). Change
Event#cancelBubble to a property and add a test.

PR-URL: nodejs#33613
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
jasnell pushed a commit to jasnell/node that referenced this pull request Jun 22, 2020
Event#cancelBubble is property (and not a function). Change
Event#cancelBubble to a property and add a test.

PR-URL: nodejs#33613
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
jasnell pushed a commit that referenced this pull request Jun 24, 2020
Event#cancelBubble is property (and not a function). Change
Event#cancelBubble to a property and add a test.

PR-URL: #33613
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>

PR-URL: #34015
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
codebytere pushed a commit that referenced this pull request Jun 27, 2020
Event#cancelBubble is property (and not a function). Change
Event#cancelBubble to a property and add a test.

PR-URL: #33613
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>

PR-URL: #34015
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@codebytere codebytere mentioned this pull request Jun 28, 2020
codebytere pushed a commit that referenced this pull request Jun 30, 2020
Event#cancelBubble is property (and not a function). Change
Event#cancelBubble to a property and add a test.

PR-URL: #33613
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
codebytere pushed a commit that referenced this pull request Jun 30, 2020
Event#cancelBubble is property (and not a function). Change
Event#cancelBubble to a property and add a test.

PR-URL: #33613
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>

PR-URL: #34015
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. events Issues and PRs related to the events subsystem / EventEmitter.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants