You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Modal doesn't close on the first click on the overlay after click on elements containing event.stopPropagation() inside modal #1015
When opening a second modal from within a first modal, closing the second modal results in the first modal not closing on the first click on the overlay. Instead, it requires a second click to close. This issue is similar to issue #958. However, the solution provided there is not really suitable for our cross-platform project, which uses components from react-native, and I would like to avoid creating separate button components for the web modal, in which event.stopPropagation() would be removed, in order to achieve the correct behavior.
This issue also occurs when closing a modal by clicking on the close button, then reopening it and attempting to close it by clicking on the overlay. The modal only closes after the second click, not the first. This is a kind of standard behavior, so you can reproduce the problem for both the first and second modals. Below are the steps to reproduce the problem for the first modal.
I would like to propose a simple solution to this issue:
In the handleOverlayOnMouseDown method (src/components/ModalPortal.js:321), add this.shouldClose = null; to allow the modal to close in cases where the mouse press started on the overlay. Then, in the handleContentOnMouseDown method (src/components/ModalPortal.js:331), add event.stopPropagation(); as otherwise, there's an issue where the modal would close if OnMouseDown was called within the modal content, but OnMouseUp was called in the layout. I wanted to create a PR with these changes, but I don't have the necessary permissions.
Both of these fixes seem logical to me – if a user clicks on the layout, they likely want to close the modal. Propagation through the content to the layout doesn't seem to provide any benefits, and it appears reasonable not to trigger layout events when clicking on the content.
I would also like to note that I have already applied these changes to our project via a patch, and after testing, our QA reported that the issue was resolved, and no new issues were discovered.
@diasbruno Ok, his no react 5 days already, what should I do to make this small fix? It's already third issue with same problem, I really want to solve it and move on
I have the same problem. In my case I don't have a second modal but I have a drag handler inside my modal content and when I trigger a drag event using the mouse it then requires two clicks on the overlay to close the modal.
I haven't looked into it deeply but I guess it's the same handleOverlayOnMouseDown / handleContentOnMouseDown issue reported by @ErikDeviashin above.
From what I saw, the issue actually is that either (or both) components - overlay and content, are receiving an event that is not theirs and they are setting up the flags incorrectly.
The ideal solution is to only act if the event has the event.currentTarget = overlay or content.
The error does't occur , just an observation , i'am trying to pin point the issue here without any react-native components , would appreciate any insights.
Activity
ErikDeviashin commentedon Apr 9, 2023
I would like to propose a simple solution to this issue:
In the
handleOverlayOnMouseDownmethod (src/components/ModalPortal.js:321), addthis.shouldClose = null;to allow the modal to close in cases where the mouse press started on the overlay. Then, in thehandleContentOnMouseDownmethod (src/components/ModalPortal.js:331), addevent.stopPropagation();as otherwise, there's an issue where the modal would close ifOnMouseDownwas called within the modal content, butOnMouseUpwas called in the layout. I wanted to create a PR with these changes, but I don't have the necessary permissions.Both of these fixes seem logical to me – if a user clicks on the layout, they likely want to close the modal. Propagation through the content to the layout doesn't seem to provide any benefits, and it appears reasonable not to trigger layout events when clicking on the content.
I would also like to note that I have already applied these changes to our project via a patch, and after testing, our QA reported that the issue was resolved, and no new issues were discovered.
msubham193 commentedon Apr 9, 2023
@ErikDeviashin can i solve this issue ?
ErikDeviashin commentedon Apr 12, 2023
@msubham193 If you have a right to push in repo, of course you can.
@diasbruno Can you have a look to this issue please?
diasbruno commentedon Apr 12, 2023
@msubham193 go for it! Let me know if you need anything.
ErikDeviashin commentedon Apr 16, 2023
@msubham193 Good day! Have you seen my comments in the PR? Can you please correct what is indicated in the comments?
ErikDeviashin commentedon Apr 18, 2023
@diasbruno Ok, his no react 5 days already, what should I do to make this small fix? It's already third issue with same problem, I really want to solve it and move on
khmelevskii commentedon Jun 17, 2023
I faced with the same problem. Do we have any workaround before fixing it?
evoyy commentedon Jul 7, 2023
I have the same problem. In my case I don't have a second modal but I have a drag handler inside my modal content and when I trigger a drag event using the mouse it then requires two clicks on the overlay to close the modal.
I haven't looked into it deeply but I guess it's the same
handleOverlayOnMouseDown/handleContentOnMouseDownissue reported by @ErikDeviashin above.diasbruno commentedon Jul 7, 2023
From what I saw, the issue actually is that either (or both) components - overlay and content, are receiving an event that is not theirs and they are setting up the flags incorrectly.
The ideal solution is to only act if the event has the
event.currentTarget = overlay or content.diasbruno commentedon Jul 7, 2023
I've assigned to me to test the lateral buttons...but now I can't remove.
This issue is free to anyone who want to work on it.
gautamz07 commentedon Jul 20, 2023
@diasbruno @ErikDeviashin if i replace your App.js code with :-
The error does't occur , just an observation , i'am trying to pin point the issue here without any react-native components , would appreciate any insights.