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 outside click listeners (from click to mousedown+mouseup events) #5160

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

S3v3Nice
Copy link

@S3v3Nice S3v3Nice commented Jan 27, 2024

Fixes #2597.

The problem is related to the fact that the click event is used to recognize a click on the outside of an element, so that a planned action (e.g. closing a modal) is triggered by a full click, but only when the click starts OR ends on the outside (because the target of the click event is the nearest element, containing both the target of the mousedown event and the target of the mouseup event). Because of this, for example, selecting text inside a dialog and then moving the cursor to outside (while holding down the mouse button) causes the dialog to close.

There may be two solutions to this problem:

  1. Implement proper full click handling: capture mousedown and mouseup events, and check the targets of mousedown and mouseup events (both targets should not be the current component)
  2. Simply change the click event to mousedown, so the modal will close immediately when the user mouse down outside the modal, and will not close when the user starts clicking inside the modal and finishes outside.

I chose the second solution because it's simpler, and it's standard practice (if the user starts clicking outside the modal, it's more likely that he wants to close it, and why not do it immediately, without waiting for the click to complete). By the way, this is how it was already implemented in the Calendar component.

UPD:
For some reasons listed in the comment below, it was decided to use the first way of solving the problem - mousedown + mouseup events - in almost all components.

Copy link

vercel bot commented Jan 27, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

2 Ignored Deployments
Name Status Preview Comments Updated (UTC)
primevue ⬜️ Ignored (Inspect) Visit Preview Apr 6, 2024 7:43pm
primevue-v4 ⬜️ Ignored (Inspect) Visit Preview Apr 6, 2024 7:43pm

@mertsincan
Copy link
Member

Hi,

Thanks a lot for your contribution! We didn't do mousedown for some reason but I can't remember. I will discuss this with the team and get back to you.

@mertsincan mertsincan added the Status: Pending Review Issue or pull request is being reviewed by Core Team label Jan 28, 2024
@mertsincan mertsincan added this to the 3.50.0 milestone Jan 28, 2024
@matthew-dean
Copy link

matthew-dean commented Mar 7, 2024

@S3v3Nice Are you certain this won't cause issues with actions like the following?:

  1. Say someone implements a drag-drop component.
  2. They click down outside a dialog, and drag into the dialog.

Logically, the dialog should not close because the click did not start / finish outside the dialog. It sounds like what you are describing would immediately dismiss the dialog if a mousedown started outside the dialog and finished within it (or possibly would close the dialog before they could finish dragging).

(I'm not saying someone should do this pattern, and ideally they would be disabling "click outside to close", but I'm just pointing out that switching from click to mousedown might be a breaking change for some scenarios. I personally interpret "click outside to close" as inferring BOTH mousedown AND mouseup outside the dialog, not one or the other. IMO it's important to not be over-eager in closing dialogs.)

@tugcekucukoglu tugcekucukoglu modified the milestones: 3.50.0, 3.52.0 Mar 15, 2024
@S3v3Nice S3v3Nice changed the title Fix outside click listeners (from click to mousedown event) Fix outside click listeners (from click to mousedown+mouseup events) Apr 6, 2024
@tugcekucukoglu tugcekucukoglu modified the milestones: 3.52.0, 3.54.0 Apr 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Pending Review Issue or pull request is being reviewed by Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Selecting Text in Dialog / Modal closes Modal
4 participants