ryansolid / dom-expressions

A Fine-Grained Runtime for Performant DOM Rendering
MIT License
874 stars 125 forks source link

Improve event delegation handling of open shadow DOM #315

Closed olivercoad closed 3 weeks ago

olivercoad commented 6 months ago

Fix https://github.com/solidjs/solid/issues/2103

Adds unit tests for event delegation with open shadow dom.

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 commented 1 month ago

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 commented 4 weeks ago

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.