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

Extend EventTarget to support event bubbling #84

Merged
merged 29 commits into from
Jul 21, 2021

Conversation

mikemadest
Copy link
Contributor

This solves: ungap/event-target#8
As discussed there, EventTarget is extended to add bubbling and parent node knowledge only in the DOM context.

- add getParent method to Node returning parentNode
- on dispatch trigger parent dispatchEvent
- unit tests
@WebReflection
Copy link
Owner

Couple of changes needed, and I'd love to have stopImmediatePropagation() tested too, otherwise, awesome job!

@mikemadest
Copy link
Contributor Author

Couple of changes needed, and I'd love to have stopImmediatePropagation() tested too, otherwise, awesome job!

Thanks!
I can't really test the full use case without updating "ungap/event-target" (I could do a PR for that later if that's working for you), but I'll add more tests to cover stopImmediatePropagation here.

@WebReflection
Copy link
Owner

Couple of changes needed, and I'd love to have stopImmediatePropagation() tested too, otherwise, awesome job!

Thanks!
I can't really test the full use case without updating "ungap/event-target" (I could do a PR for that later if that's working for you), but I'll add more tests to cover stopImmediatePropagation here.

Oh ... I see. Sorry, busy days in here ... I'll try to update/change that later on but if you are, somehow, in a rush, please go ahead with the PR (ungap is a bit weird to work with though, no modern JS and stuff) ... as you prefer.

- method renamed to "_getParent"
- dispatchEvent return value now take EventTarget.dispatchEvent return into account as well as parent.dispatchEvent
- "stopImmediatePropagation" now tested in unit tests.

Todo: when ungap/event-target support "_stopImmediatePropagationFlag" more event listeners could be added to `buttonTarget` to ensure full behaviour is working.
@mikemadest
Copy link
Contributor Author

Oh ... I see. Sorry, busy days in here ... I'll try to update/change that later on but if you are, somehow, in a rush, please go ahead with the PR (ungap is a bit weird to work with though, no modern JS and stuff) ... as you prefer.

No worries, I didn't mean to rush you :)
I can't say I am in a particular hurry, but I can understand you would be busy and I don't mind at all doing a PR there (especially since I already pulled the code and played with it... so it's mostly ready).
To be honest the only thing I was confused about were the unit tests (in ungap) and for my local tests I used your assert there (but I'll refactor that to follow existing code).

used "protected" here instead of "private", because Node extend EventTarget and needs to overwrite "_getParent" while still allowing "dispatchEvent" to access it.
- Node 16.5 will throw if we try to dispatch the current event to the parent. So instead we need to create a new event with the same options.

- replaced "_stopPropagationFlag" by "cancelBubble" to be consistent with Node implementation of Event

- currently Node don't handle bubbling, but also don't properly set "_stopPropagationFlag" when calling "stopImmediatePropagation" creating a problem and inconsistency for us.
=> should we extend the Node Event object to ensure the proper behaviour?
@WebReflection
Copy link
Owner

So ... what are we missing here? is that still ungap for the event target? Thanks!

@mikemadest
Copy link
Contributor Author

So ... what are we missing here? is that still ungap for the event target? Thanks!

yes, I can make a PR for that next, it should be way simpler.

Mickael Meausoone added 5 commits July 20, 2021 20:59
- add getParent method to Node returning parentNode
- on dispatch trigger parent dispatchEvent
- unit tests
- method renamed to "_getParent"
- dispatchEvent return value now take EventTarget.dispatchEvent return into account as well as parent.dispatchEvent
- "stopImmediatePropagation" now tested in unit tests.

Todo: when ungap/event-target support "_stopImmediatePropagationFlag" more event listeners could be added to `buttonTarget` to ensure full behaviour is working.
used "protected" here instead of "private", because Node extend EventTarget and needs to overwrite "_getParent" while still allowing "dispatchEvent" to access it.
- Node 16.5 will throw if we try to dispatch the current event to the parent. So instead we need to create a new event with the same options.

- replaced "_stopPropagationFlag" by "cancelBubble" to be consistent with Node implementation of Event

- currently Node don't handle bubbling, but also don't properly set "_stopPropagationFlag" when calling "stopImmediatePropagation" creating a problem and inconsistency for us.
=> should we extend the Node Event object to ensure the proper behaviour?
@WebReflection
Copy link
Owner

@mikemadest this looks good to me, but I don't understand why there are unrelated changes in this MR ... by any chance you can remove those and pull rebase from main instead?

git pull --rebase origin main

I'd rather keep changes per PR/MR, thanks. Once you do that, I'll squash merge and publish.

@mikemadest
Copy link
Contributor Author

@mikemadest this looks good to me, but I don't understand why there are unrelated changes in this MR ... by any chance you can remove those and pull rebase from main instead?

git pull --rebase origin main

I'd rather keep changes per PR/MR, thanks. Once you do that, I'll squash merge and publish.

Sorry I'm not sure what happened there, will cleanup

Mickael Meausoone and others added 15 commits July 21, 2021 11:20
- add getParent method to Node returning parentNode
- on dispatch trigger parent dispatchEvent
- unit tests
* Adding NamedNodeMap to global export
- method renamed to "_getParent"
- dispatchEvent return value now take EventTarget.dispatchEvent return into account as well as parent.dispatchEvent
- "stopImmediatePropagation" now tested in unit tests.

Todo: when ungap/event-target support "_stopImmediatePropagationFlag" more event listeners could be added to `buttonTarget` to ensure full behaviour is working.
used "protected" here instead of "private", because Node extend EventTarget and needs to overwrite "_getParent" while still allowing "dispatchEvent" to access it.
- Node 16.5 will throw if we try to dispatch the current event to the parent. So instead we need to create a new event with the same options.

- replaced "_stopPropagationFlag" by "cancelBubble" to be consistent with Node implementation of Event

- currently Node don't handle bubbling, but also don't properly set "_stopPropagationFlag" when calling "stopImmediatePropagation" creating a problem and inconsistency for us.
=> should we extend the Node Event object to ensure the proper behaviour?
- add getParent method to Node returning parentNode
- on dispatch trigger parent dispatchEvent
- unit tests
- method renamed to "_getParent"
- dispatchEvent return value now take EventTarget.dispatchEvent return into account as well as parent.dispatchEvent
- "stopImmediatePropagation" now tested in unit tests.

Todo: when ungap/event-target support "_stopImmediatePropagationFlag" more event listeners could be added to `buttonTarget` to ensure full behaviour is working.
used "protected" here instead of "private", because Node extend EventTarget and needs to overwrite "_getParent" while still allowing "dispatchEvent" to access it.
- Node 16.5 will throw if we try to dispatch the current event to the parent. So instead we need to create a new event with the same options.

- replaced "_stopPropagationFlag" by "cancelBubble" to be consistent with Node implementation of Event

- currently Node don't handle bubbling, but also don't properly set "_stopPropagationFlag" when calling "stopImmediatePropagation" creating a problem and inconsistency for us.
=> should we extend the Node Event object to ensure the proper behaviour?
@mikemadest
Copy link
Contributor Author

@mikemadest this looks good to me, but I don't understand why there are unrelated changes in this MR ... by any chance you can remove those and pull rebase from main instead?

git pull --rebase origin main

I'd rather keep changes per PR/MR, thanks. Once you do that, I'll squash merge and publish.

ok so I was just behind main because of other merge, this looks all good now

@WebReflection WebReflection merged commit 12c7235 into WebReflection:main Jul 21, 2021
@WebReflection
Copy link
Owner

v0.11.1 is up and running 🍻

Thanks a lot for your help ❤️

@mikemadest
Copy link
Contributor Author

v0.11.1 is up and running 🍻

Thanks a lot for your help ❤️

thank you for bearing with me :)

@mikemadest mikemadest deleted the event-bubbling branch July 22, 2021 17:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants