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 from inside web components with shadow root mode open have wrong target #2103

Open
olivercoad opened this issue Mar 19, 2024 · 3 comments · May be fixed by ryansolid/dom-expressions#315
Labels
enhancement New feature or request

Comments

@olivercoad
Copy link

Describe the bug

I started investigating this issue when I noticed that adding a SUID button to a page caused all ninja-keys hotkeys (including up/down arrows to select commands) to stop working while the ninja-keys menu is open. After stepping through the debugger, the reason is that ninja-keys is a web component and relies on keyboard events from within the menu having a target of the ninja-keys component instead of the actual text input element inside the menu. However, the SUID button uses solid onKeyDown and onKeyUp, causing these events to use solid's event delegation from the document, which overrides the event target.

Ryan explains in this video about "reverse shadow DOM retargetting". There's not really an explaination for why solid deviates from the web standards here, but I'd assume it's so that events have the expected target when using solid inside web components, such as with solid-element. When listening to events from outside a web component though, I think it should not do reverse shadow DOM retargetting, especially when using an event listener that's not using solid's event delegation (whether it's an external library like ninja-keys using hotkeys-js, or with on: directives in solid).

It's worth noting that the order the event listeners on document are added matters; if it was only modifying the event "once everyone else is done with it" then that would be fine, but when event listeners are added after solid has already initialised it's the event delegation, then it gets the mutated event.

A simple fix could be to just keep hold of the original target and put it back after solid's event delegation is done with it. That wouldn't make event handlers that do use event delegation get the "correct" target, but it would at least fix it for event handlers that aren't using event delegation.

Your Example Website or App

https://stackblitz.com/edit/solidjs-templates-zz63ns?file=src%2FApp.tsx

Steps to Reproduce the Bug or Issue

  1. Go to the provided repro on stackblitz
  2. Type in the text input
  3. Observe that keyDown target B shows INPUT

Expected behavior

All event listeners should show the same thing, MY-INPUT-COMPONENT, because the events originate from inside the web component and the listeners are on document, outside the web component.

Screenshots or Videos

No response

Platform

  • OS: Linux
  • Browser: Tested in Chrome and Firefox
  • Version: Chrome 122.0.6261.111, Firefox 123.0.1

Additional context

No response

@olivercoad
Copy link
Author

I've made some unit tests for dom-expressions and have an idea about how to fix it. I'm thinking perhaps in eventHandler where it walks up the tree, every time it uses host to walk up a shadowRoot, it can swap out target to that node. I'm just not sure if slots would complicate things or if they would work seamlessly, so would have to test how they work.

Should I go ahead with attempting the fix and make a PR?

@ryansolid
Copy link
Member

Yeah I'm no longer deep in webcomponents so I haven't played with this for a while. I did retargetting due to yes having SolidJS in components. I built apps that had nested levels of Solid web components. So when I extracted a component out like say a wrapper around input I found it odd that I didn't have the input element anymore. I found a React package that did similar retargetting and used that as my solution.

Resetting on host seems reasonable. Basically it is only reversing the event target for the deepest level where the event orginates. I would need a second opinion from some of the web component people in our community like @trusktr. That being said I'm not sure any of them use event delegation anyway because of other issues.

I see now a lot of my webcomponent considerations when building Solid were fighting against them. I didn't really want web components but I wanted to use the platform. I should have just left them to do their own thing earlier and built what I ultimately wanted to.

@ryansolid ryansolid added the enhancement New feature or request label Mar 19, 2024
@olivercoad
Copy link
Author

That being said I'm not sure any of them use event delegation anyway because of other issues

I'm not deep in web components either, just consuming a few from external libraries. What kind of other issues are there? Searching old issues on github, the only things I can see are for non-composed events (but it looks like delegation is whitelisted now anyway) and shadow with mode closed.
I don't want to disable event delegation completely because for this particular project it's actually a massive performance win.

Retargetting on host works well and makes the target correct for things inside as well as outside the component, as well as being correct for non-delegated handlers that run afterwards. However, the unit tests revealed another issue which is that events originating inside a slot don't get passed to delegated handlers inside the shadow DOM. While I'm in the headspace I'll try to find an easy fix for that too, otherwise will make a PR with just the original fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants