feat: Add Shadow DOM support for useOverlay and improve event handling#7751
feat: Add Shadow DOM support for useOverlay and improve event handling#7751ritz078 wants to merge 8 commits intoadobe:mainfrom
Conversation
|
@snowystinger Anything I can do to get this one through ? |
snowystinger
left a comment
There was a problem hiding this comment.
Thanks for your patience, we're in the midst of getting a release out.
|
|
||
| let onInteractOutside = (e: PointerEvent) => { | ||
| if (!shouldCloseOnInteractOutside || shouldCloseOnInteractOutside(e.target as Element)) { | ||
| if (!shouldCloseOnInteractOutside || shouldCloseOnInteractOutside(getEventTarget(e))) { |
There was a problem hiding this comment.
there are some 'relatedTarget' checks below, can we dive into a shadowDOM for those as well? i don't know if there is a composedPath for them
I could see this being an issue if getEventTarget is contained in e.relatedTarget or if the e.relatedTarget is a shadowroot and contains both targets that should be ignored as well as ones which shouldn't.
For example, if someone made a Toast Container inside a shadow dom, then clicking the toasts probably shouldn't count as outside but maybe clicking between individual toasts should for some reason.
There was a problem hiding this comment.
that isChildOfActiveScope might be problematic as well if the activescope is within a shadowroot
| }); | ||
| }); | ||
|
|
||
| describe('useOverlay with shadow dom', () => { |
There was a problem hiding this comment.
I don't think these tests are going to be enough, usually overlays portal out to be a direct child of the body element, which would actually take them out of a shadowroot potentially
This can also be changed with UNSTABLE_PortalProvider
We'll need tests using the RAC Popover at a minimum
snowystinger
left a comment
There was a problem hiding this comment.
noticed that isElementInChildOfActiveScope calls node.contains in FocusScope
Does this need to use the shadowDOM safe query we added in our utils?
What other tests might we need?
- the non-shadowDOM "firefox bug" describe needs to be checked either in an app or as a test in a shadowDOM environment
- If focus is moving into a child focus scope (e.g. menu inside a dialog)
- Can the underlay be outside the shadowDOM? what other setups might need coverage?
shouldCloseOnInteractOutside={(target) => target !== underlay}what if we don't have access to underlay because the shadowroot is just the component instead of the entire app?
It may be easier to write some of these tests in the React Aria Component file
|
|
||
| let onInteractOutside = (e: PointerEvent) => { | ||
| if (!shouldCloseOnInteractOutside || shouldCloseOnInteractOutside(e.target as Element)) { | ||
| if (!shouldCloseOnInteractOutside || shouldCloseOnInteractOutside(getEventTarget(e))) { |
There was a problem hiding this comment.
Is it weird that we call an external API with something that should be inside a shadowroot, it'd unintentionally leak shadow dom internals through our API
We don't do this in the pressEvents, though the use case is obviously a little different
Followup of #6046
This PR fixes an issue in
useOverlaywhereshouldCloseOnInteractOutsidealways received the shadow root parent element as the argument whenever an element inside the shadow root was clicked.✅ Pull Request Checklist:
📝 Test Instructions:
🧢 Your Project:
Nutrient (formerly PSPDFKit)