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

francinelucca · 2024-09-18T18:33:27Z

Closes https://github.com/github/primer/issues/3477

Keeps a state of whether the last mousedown event originated on the backdrop to prevent closing the dialog when the backdrop is clicked accidentally.

Changelog

New

lastMouseDownIsBackdrop state variable to keep track of whether the last mousedown event originated on the backdrop.

Changed

Modify onBackdropClick logic to only trigger the onClose callback if the mousedown event originates on the backdrop (i.e., lastMouseDownIsBackdrop is true).

Rollout strategy

Patch release
Minor release
Major release; if selected, include a written rollout or migration plan
None; if selected, include a brief description as to why

Testing & Reviewing

Follow repro instructions from issue on preview deployment and validate behavior is no longer experienced.

Note: I tried to add a unit test for this but couldn't get a combination of mouseDown/mouseUp to trigger a click on the backdrop so wasn't able to simulate this particular scenario. Open to any suggestions on how to do that.

Merge checklist

Added/updated tests
Added/updated documentation
Added/updated previews (Storybook)
Changes are SSR compatible
Tested in Chrome
Tested in Firefox
Tested in Safari
Tested in Edge
(GitHub staff only) Integration tests pass at github/github (Learn more about how to run integration tests)

changeset-bot · 2024-09-18T18:33:31Z

🦋 Changeset detected

Latest commit: 159567c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@primer/react	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

francinelucca · 2024-09-18T18:35:19Z

packages/react/src/Dialog/Dialog.tsx

  const defaultedProps = {...props, title, subtitle, role, dialogLabelId, dialogDescriptionId}
  const onBackdropClick = useCallback(
    (e: SyntheticEvent) => {
-      if (e.target === e.currentTarget) {
+      if (e.target === e.currentTarget && lastMouseDownIsBackdrop) {


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!

github-actions · 2024-09-18T18:36:28Z

size-limit report 📦

Path	Size
packages/react/dist/browser.esm.js	97.48 KB (-0.06% 🔽)
packages/react/dist/browser.umd.js	97.75 KB (+0.05% 🔺)

primer-integration · 2024-09-18T19:06:31Z

👋 Hi from github/github! Your integration PR is ready: https://github.com/github/github/pull/342878

…revent-dialog-from-closing-on-accidental-mouse-release

keithamus · 2024-09-19T15:50:23Z

Do you think it's viable to write a test for this?

francinelucca · 2024-09-19T16:02:10Z

Do you think it's viable to write a test for this?

So I tried using fireEvent.mouseUp/mouseDown but couldn't get a combination of mouseDown/mouseUp to trigger a click on the backdrop so wasn't able to simulate this scenario. Any suggestions? I need to "start" the click at the dialog and "end" it in the backdrop 😅 @keithamus

keithamus · 2024-09-19T16:07:05Z

Yes I guess any test that manually fired events would be too contrived. I would just like to find a way to preserve the functionality against refactors.

…og-from-closing-on-accidental-mouse-release' of github.com:primer/react into francinelucca/3477-prcdialog-best-practice-prevent-dialog-from-closing-on-accidental-mouse-release

francinelucca · 2024-09-19T16:40:46Z

packages/react/src/Dialog/Dialog.test.tsx

+  it('does not call `onClose` when click was not originated from backdrop', async () => {
+    const user = userEvent.setup()
+    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()
+  })
+


how do you feel about this @keithamus ? I do have to manually trigger the click event but it will fail if the current implementation were to be refactored out 🤔

keithamus

LGTM. Thanks for adding the test.

* fix(Dialog): track mousedown event to prevent accidental closing * Create eighty-houses-beg.md * test(Dialog): add test for accidental closure behavior * test(Dialog): remove unused var

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

b79a346

francinelucca temporarily deployed to github-pages September 18, 2024 18:35 — with GitHub Actions Inactive

francinelucca commented Sep 18, 2024

View reviewed changes

github-actions bot temporarily deployed to storybook-preview-4986 September 18, 2024 18:35 Inactive

Create eighty-houses-beg.md

8230a36

francinelucca temporarily deployed to github-pages September 18, 2024 18:44 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-4986 September 18, 2024 18:44 Inactive

francinelucca changed the title ~~WIP: fix(Dialog): track mousedown event to prevent accidental closing~~ fix(Dialog): track mousedown event to prevent accidental closing Sep 19, 2024

francinelucca added staff Author is a staff member status: review needed labels Sep 19, 2024

francinelucca marked this pull request as ready for review September 19, 2024 13:20

francinelucca requested a review from a team as a code owner September 19, 2024 13:20

francinelucca requested a review from jonrohan September 19, 2024 13:20

Merge branch 'main' into francinelucca/3477-prcdialog-best-practice-p…

d831be3

…revent-dialog-from-closing-on-accidental-mouse-release

francinelucca temporarily deployed to github-pages September 19, 2024 13:22 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-4986 September 19, 2024 13:23 Inactive

keithamus approved these changes Sep 19, 2024

View reviewed changes

francinelucca added 2 commits September 19, 2024 12:38

test(Dialog): add test for accidental closure behavior

86bded5

Merge branch 'francinelucca/3477-prcdialog-best-practice-prevent-dial…

1057cdb

…og-from-closing-on-accidental-mouse-release' of github.com:primer/react into francinelucca/3477-prcdialog-best-practice-prevent-dialog-from-closing-on-accidental-mouse-release

francinelucca temporarily deployed to github-pages September 19, 2024 16:40 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-4986 September 19, 2024 16:40 Inactive

francinelucca commented Sep 19, 2024

View reviewed changes

francinelucca requested a review from keithamus September 19, 2024 16:40

test(Dialog): remove unused var

159567c

francinelucca deployed to github-pages September 19, 2024 16:44 — with GitHub Actions View deployment

github-actions bot deployed to storybook-preview-4986 September 19, 2024 16:45 View deployment

keithamus approved these changes Sep 19, 2024

View reviewed changes

francinelucca added this pull request to the merge queue Sep 19, 2024

Merged via the queue into main with commit d6471aa Sep 19, 2024
32 checks passed

francinelucca deleted the francinelucca/3477-prcdialog-best-practice-prevent-dialog-from-closing-on-accidental-mouse-release branch September 19, 2024 20:18

primer bot mentioned this pull request Sep 19, 2024

Release tracking (rc.6) #4980

Merged

primer bot mentioned this pull request Oct 18, 2024

Release tracking (37.0.0) #5143

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

francinelucca commented Sep 18, 2024 •

edited

Loading

changeset-bot bot commented Sep 18, 2024 •

edited

Loading

francinelucca Sep 18, 2024 •

edited

Loading

github-actions bot commented Sep 18, 2024 •

edited

Loading

primer-integration bot commented Sep 18, 2024

keithamus commented Sep 19, 2024

francinelucca commented Sep 19, 2024 •

edited

Loading

keithamus commented Sep 19, 2024

francinelucca Sep 19, 2024

keithamus left a comment

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

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

Conversation

francinelucca commented Sep 18, 2024 • edited Loading

Changelog

New

Changed

Rollout strategy

Testing & Reviewing

Merge checklist

changeset-bot bot commented Sep 18, 2024 • edited Loading

🦋 Changeset detected

francinelucca Sep 18, 2024 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Sep 18, 2024 • edited Loading

size-limit report 📦

primer-integration bot commented Sep 18, 2024

keithamus commented Sep 19, 2024

francinelucca commented Sep 19, 2024 • edited Loading

keithamus commented Sep 19, 2024

francinelucca Sep 19, 2024

Choose a reason for hiding this comment

keithamus left a comment

Choose a reason for hiding this comment

francinelucca commented Sep 18, 2024 •

edited

Loading

changeset-bot bot commented Sep 18, 2024 •

edited

Loading

francinelucca Sep 18, 2024 •

edited

Loading

github-actions bot commented Sep 18, 2024 •

edited

Loading

francinelucca commented Sep 19, 2024 •

edited

Loading