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
[Dialog] Create new component and hook #372
base: master
Are you sure you want to change the base?
Conversation
if (modal) { | ||
// When the dialog is modal, clicking on the backdrop is recognized as clicking on the dialog itself. | ||
// We need to check whether the click was outside the dialog's bounding box. | ||
// We also don't want to close the dialog when clicking on any descendant of it (such as an open select). | ||
if ( | ||
(event.target === popupElement && hasClickedOutsideBoundingBox(event, popupElement)) || | ||
!popupElement.contains(event.target as Node) | ||
) { | ||
onOpenChange?.(false); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Several issues with "outside click" detection popped up in Floating UI:
- The dialog shouldn't close if you clicked on the scrollbar if present, since they may be trying to scroll.
- The
contains
check needs to handle portaled elements that aren't a direct descendant of the dialog in the DOM and also shadow DOM elements. - It shouldn't close if clicking a third-party widget (e.g. extension popup or toast) that appeared up after the dialog was rendered over the top. (This actually may bring about another problem I saw a few months ago - third-party widgets that don't use top-layer elements can't ever appear over the dialog itself? This includes Grammarly popups, translation popups, etc.)
- "Click outside" should use
mousedown
notpointerdown
since touch events can fire on elements beneath the backdrop with touch input unintentionally otherwise when dimissing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can solve almost all of these problems with our ClickAwayListener. The only issue that remains is the top-layer thing. I've investigated this for a while, but I don't think we can work around it, unfortunately.
Netlify deploy preview |
* Determines whether the dialog should close when clicking outside of it or pressing the escape key. | ||
*/ | ||
softClose: SoftCloseOptions; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MDN docs call this light dismiss
instead. I also think it should be the default if it isn't here
Preview: https://deploy-preview-372--base-ui.netlify.app/base-ui/react-dialog/
Closes #215