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

feat: Add Shadow DOM support for useOverlay and improve event handling #7751

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

ritz078
Copy link
Contributor

@ritz078 ritz078 commented Feb 9, 2025

Followup of #6046

This PR fixes an issue in useOverlay where shouldCloseOnInteractOutside always received the shadow root parent element as the argument whenever an element inside the shadow root was clicked.

✅ Pull Request Checklist:

  • Included link to corresponding React Spectrum GitHub Issue.
  • Added/updated unit tests and storybook for this change (for new code or code which already has tests).
  • Filled out test instructions.
  • Updated documentation (if it already exists for this component).
  • Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

🧢 Your Project:

Nutrient (formerly PSPDFKit)

@ritz078 ritz078 marked this pull request as ready for review February 9, 2025 23:37
@ritz078
Copy link
Contributor Author

ritz078 commented Feb 22, 2025

@snowystinger Anything I can do to get this one through ?

Copy link
Member

@snowystinger snowystinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your patience, we're in the midst of getting a release out.

@@ -101,7 +102,7 @@ export function useOverlay(props: AriaOverlayProps, ref: RefObject<Element | nul
};

let onInteractOutside = (e: PointerEvent) => {
if (!shouldCloseOnInteractOutside || shouldCloseOnInteractOutside(e.target as Element)) {
if (!shouldCloseOnInteractOutside || shouldCloseOnInteractOutside(getEventTarget(e))) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that isChildOfActiveScope might be problematic as well if the activescope is within a shadowroot

fireEvent.pointerUp(document.body);
expect(isPrevented).toBeFalsy(); // meaning the event had preventDefault called
});
});
});

describe('useOverlay with shadow dom', () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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