-
Notifications
You must be signed in to change notification settings - Fork 671
fix(Dialog): track mousedown event to prevent accidental closing #4986
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
Changes from 5 commits
b79a346
8230a36
d831be3
86bded5
1057cdb
159567c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| "@primer/react": patch | ||
| --- | ||
|
|
||
| fix(Dialog): track mousedown event to prevent accidental closing |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -419,14 +419,15 @@ const _Dialog = React.forwardRef<HTMLDivElement, React.PropsWithChildren<DialogP | |
| footerButton.ref = autoFocusedFooterButtonRef | ||
| } | ||
| } | ||
| const [lastMouseDownIsBackdrop, setLastMouseDownIsBackdrop] = useState<boolean>(false) | ||
| const defaultedProps = {...props, title, subtitle, role, dialogLabelId, dialogDescriptionId} | ||
| const onBackdropClick = useCallback( | ||
| (e: SyntheticEvent) => { | ||
| if (e.target === e.currentTarget) { | ||
| if (e.target === e.currentTarget && lastMouseDownIsBackdrop) { | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. for the current test case (mouseDown on dialog, drag to backdrop, mouseUp), the e.target and e.currentTarget are both the same (the backdrop) since the click is generated at the backdrop on mouseUp. Circumventing this by explicitly checking that the mouseDown event that triggered the click was originated at the backdrop, but open to other ides on how to handle this if any recs! |
||
| onClose('escape') | ||
| } | ||
| }, | ||
| [onClose], | ||
| [onClose, lastMouseDownIsBackdrop], | ||
| ) | ||
|
|
||
| const dialogRef = useRef<HTMLDivElement>(null) | ||
|
|
@@ -479,7 +480,14 @@ const _Dialog = React.forwardRef<HTMLDivElement, React.PropsWithChildren<DialogP | |
| return ( | ||
| <> | ||
| <Portal> | ||
| <Backdrop ref={backdropRef} {...positionDataAttributes} onClick={onBackdropClick}> | ||
| <Backdrop | ||
| ref={backdropRef} | ||
| {...positionDataAttributes} | ||
| onClick={onBackdropClick} | ||
| onMouseDown={e => { | ||
| setLastMouseDownIsBackdrop(e.target === e.currentTarget) | ||
| }} | ||
| > | ||
| <StyledDialog | ||
| width={width} | ||
| height={height} | ||
|
|
||
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.
how do you feel about this @keithamus ? I do have to manually trigger the
clickevent but it will fail if the current implementation were to be refactored out 🤔