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

fix(Dialog): track mousedown event to prevent accidental closing #4986

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/eighty-houses-beg.md
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
20 changes: 19 additions & 1 deletion packages/react/src/Dialog/Dialog.test.tsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import React from 'react'
import {render, waitFor} from '@testing-library/react'
import {fireEvent, render, waitFor} from '@testing-library/react'
import userEvent from '@testing-library/user-event'
import {Dialog} from './Dialog'
import MatchMediaMock from 'jest-matchmedia-mock'
Expand Down Expand Up @@ -85,6 +85,24 @@ describe('Dialog', () => {
expect(onClose).toHaveBeenCalledWith('escape')
})

it('does not call `onClose` when click was not originated from backdrop', async () => {
const onClose = jest.fn()

const {getByRole} = render(<Dialog onClose={onClose}>Pay attention to me</Dialog>)

expect(onClose).not.toHaveBeenCalled()

const dialog = getByRole('dialog')
const backdrop = dialog.parentElement!

fireEvent.mouseDown(dialog)
fireEvent.mouseUp(backdrop)
// trigger the click on the backdrop, mouseUp doesn't do it for us
fireEvent.click(backdrop)

expect(onClose).not.toHaveBeenCalled()
})

it('calls `onClose` when keying "Escape"', async () => {
const user = userEvent.setup()
const onClose = jest.fn()
Expand Down
14 changes: 11 additions & 3 deletions packages/react/src/Dialog/Dialog.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Contributor Author

@francinelucca francinelucca Sep 18, 2024

Choose a reason for hiding this comment

The 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)
Expand Down Expand Up @@ -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}
Expand Down
Loading