Skip to content

Shadow root fix#97

Closed
csorfab wants to merge 2 commits intotheKashey:masterfrom
csorfab:shadow-root-fix
Closed

Shadow root fix#97
csorfab wants to merge 2 commits intotheKashey:masterfrom
csorfab:shadow-root-fix

Conversation

@csorfab
Copy link
Copy Markdown
Contributor

@csorfab csorfab commented Sep 21, 2023

This PR fixes issue #45.

The problem appeared to be that when a wheel event originates from within a ShadowRoot, the event listener attached to document sees the ShadowRoot as the event target. It seems that events originating from within a ShadowRoot makes event targets opaque, and they are all reported as being the ShadowRoot itself. This made the event target equality check at SideEffect.tsx@117:39 fail, causing the event to be mistakenly prevented on line 143. This PR fixes this by subscribing shouldPrevent to all ShadowRoots found in the document and checking if we're trying to prevent an event whose target isn't contained within element we're attached to (document doesn't .contains any elements within a ShadowRoot), and returning if it isn't, leaving it for the event handlers attached to the ShadowRoots to take care of. The reasoning is that it seems that event listeners attached to a matching ShadowRoot, which will actualy .contains the particulary event target, will indeed recieve the exact target elements as that event's targets, unlike listeners attached simply to document. I couldn't find anything in the W3 specifications to back up these findings, but they appear to hold nonetheless.

I didn't do exhaustive testing. It solved the issue in the repro codepen linked in #45 as well as in my use case, and it appears to not break normal use cases, but I didn't test it on mobile and with all use cases, so any help with testing or matching my finding with actual W3 specs will be appreciated.

@theKashey
Copy link
Copy Markdown
Owner

👍 for the investigation, now the problem is known and that is the most important.

Not fully agree with implementation, but without spike cannot tell if something else will work better. Just for reference - focus-lock faces a similar problem and solves it by hoisting event source to the nearest document, or diving deeper into targets

@theKashey theKashey self-requested a review September 23, 2023 10:42
@csorfab
Copy link
Copy Markdown
Contributor Author

csorfab commented Sep 23, 2023

Thanks for the input, I looked at the linked functions, but I don't really see how any of them could be used in solving this issue. The core of the problem is that ShadowRoots hide their event target from document subscriber, so it seems to me that save for a major reworking of how the library works, it's only possible to solve this by subscribing to all ShadowRoots as well to get access to the specific event targets. If you'll have the time to look a bit more into this and give me some pointers about what you don't like about this particular implementation, I'd be glad to refactor it. I know that traversing the entire DOM tree to look for shadow roots is costly, but the effect runs only on mount, so it seems like a viable compromise to me.

But if you still have concerns, I can refactor it in a feature-flag-esque way so that the ShadowRoot specific changes only apply when a boolean prop (like supportShadowDom) is true, otherwise the exististing behavior is kept. What do you think?

Also, sorry about the whitespace changes, I left them in the PR unintentionally, will fix them

@csorfab
Copy link
Copy Markdown
Contributor Author

csorfab commented Sep 23, 2023

@theKashey after giving it a bit more thought, I realized what you meant, and came up with a much simpler solution that appears to work just as well, I submitted it as a seperate PR: #98

@theKashey
Copy link
Copy Markdown
Owner

Awesome, you are reading my mind. Let's keep this one open until problem is resolved.

@theKashey
Copy link
Copy Markdown
Owner

Fix released as a part of version 2.5.7

@theKashey theKashey closed this Oct 10, 2023
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