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

Improve event delegation handling of open shadow DOM #315

Merged
merged 4 commits into from
Sep 9, 2024

Conversation

olivercoad
Copy link
Contributor

Fix solidjs/solid#2103

Adds unit tests for event delegation with open shadow dom.

  • Set target when traversing shadow root hosts
  • Uses composedPath so that delegated handlers inside the shadow dom get events from within slots
  • Doesn't retarget when traversing a portal with useShadow

When a portal mount is in shadow dom (and possibly some other cases when mixing portals and shadow dom), it can't naturally end with the original target, so it needs the explicit retarget at the end to handle that edge case.

Mixing portals and shadow dom (especially slots) may have some interesting interactions, I've tried to make it have the most intuitive behaviour even when that doesn't follow the standard because bubbling through portals is already non standard behaviour.

@ryansolid
Copy link
Owner

I have to admit I've probably been dragging my feet on this one because of the increased code size this adds to every SolidJS project (and I haven't done a minor release in a while). Trying to understand the problem I'm more questioning some of the design decisions that we've made in the past.

Looking at the original issue in Solid this appears to be because I modified the event instead of just using our own local copy. That seems trivial to fix but you found a few other issues which you have addressed here I'm gathering.

I guess my question is does this still happen if portals don't have the useShadow option? Reading this I believe it does? This issue only pertains to portals + Shadow DOM because portals do want the original target always.

In general it seems like if I've messed up shadow DOM propagation I probably need to apply a fix like this to fix it properly so I guess I will move forward and see if I can slim this down a bit. @trusktr do you have any opinion on this issue?

@trusktr
Copy link
Contributor

trusktr commented Sep 8, 2024

Hmm, I haven't had issues with this probably because since I typically use refs I typically don't check event.target or event.currentTarget.

What the test cases are saying looks good though. Like if I put onclick on a div inside a custom element but the event.target ended up being the custom instead of the element on which I put onclick, that could be surprising.

Basically I'd expect the delegation feature to be an invisible optimization, as if I'd used .addEventListener('click') directly on the same element that I used onclick on.

@ryansolid ryansolid changed the base branch from main to next September 9, 2024 19:05
@ryansolid ryansolid merged commit ffee20f into ryansolid:next Sep 9, 2024
ryansolid pushed a commit that referenced this pull request Sep 9, 2024
* Add tests for event delegation and shadow dom

* Retarget when walking up shadow DOM

* Use composedPath where possible for bubbling events

* Explicitly reset to original target

---------

Co-authored-by: Oliver Coad <[email protected]>
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.

Events from inside web components with shadow root mode open have wrong target
3 participants