-
Notifications
You must be signed in to change notification settings - Fork 83
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
Propagate document event to window #88
Conversation
Capture is not interesting here , and it would complicate a lot the events implantation, not sure it’ll ever land, as there are zero use cases on the server . Bubbling was already a stretch, and I don’t want LinkeDOM to became a test oriented library only, there’s JSDOM already for that. |
composed: event.composed, | ||
}; | ||
// in Node 16.5 the same event can't be used for another dispatch | ||
return view.dispatchEvent(new event.constructor(event.type, options)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A note about bubbling as I'm still working on it:
After working on #84 I noticed there was still issues there.
For a page having a button
, a div#parent
and a div#container
, dispatching a click Event on the button should keep "button" as target during all bubbling while "currentTarget" would be the current element the bubbling is on.
I see the current PR is also missing that, as the original target is lost.
Proper bubbling with correct target could be difficult to achieve while keeping the ungap/event-target or Node EventTarget extends, if I have a reasonable proposition I'll do a PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that instead of adding dispatchEvent there, all you need could be to define _getParent
for document that would return this.defaultView
.
This should work as document also extends EventTarget
.
dispatchEvent
is already defined there and use that _getParent
method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
currentTarget is always where the listener was attached, never nodes in between. target is where the initial dispatch happened.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Kal-Aster I opened a PR for solving the target issue but also with an update for the window propagation based on what I suggested: #89
This should be more in line with existing code than adding the dispatchEvent logic to document
, maybe you can check it to see if that works for you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, thank you for taking care of it, I couldn't find time for it these weeks but I think I couldn't make a better implementation!
It seems working for me, haven't test it but as long as the event propagates to the window, like it does, it works.
If @WebReflection agrees with it, we can also close this PR
Should this land before, after, or it replaces, #89 ??? |
It should replace it:
here: #88 (comment) |
@mikemadest apologies for my confusion ... do we still want this in and, if that's the case, do you mind rebasing or be sure everything is fine in here? Thank you! |
I think this PR can be closed now as this issue should be fixed. |
Done, please let me know if this should be re-opened, thank you! |
Hi!
A code of mine broke, beacause it depends on an event propagated to the window from the document.
This PR fixes this behavior.
I also noticed that the capture and bubbling phase are not handled in dispatchEvent, I think that would be helpful to implement that logic, having some code depends on the order of the events.
Since I already done something like that, I could add it to this PR or maybe another one, to make things cleaner.