-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Dismissable Message Component inside a Popup closes the Popup itself #1580
Comments
Strange, setting state immediately within the Message setTimeout(() => {
this.setState({ visible: false })
}, 0) This will need some investigation. |
Since I was blocked in my project, I switched to a totally controlled Popup through the "onClose" and "onOpen" props. When i click on an element inside the Popup, and the onClose callback of Popup itself is fired, I check the event target to ensure that if the user is clicking a specific element, the popup should remain opened.
I know it's awful but it works |
I'm having the same issue and it doesn't seem right to me having to rely on setTimeout or having to check the target node of the onClose callback. @levithomason Was there any further investigation on this? Is there anything I can do to help solve this? |
I am also having the same issue. The contents of my popup are another component that has state and gets re-rendered based on that state. Each time the state of the child component changes and the child component renders, the onClose is fired. I guess my question is why is the onClose fired when a child component of the Popup.Content is re-rendered? The setTimeout workaround seems to work, but I wonder how reliable it will be. |
Apparently connected issue. |
@ecnaidar this is due to all event handlers being added to the global scope and executed simultaneously. Pressing ESC will trigger close of all components listening for it. That said, #1733 was just merged which solves this. Events are now handled in a stack, so only the top most Popup, Modal, Dropdown, etc. will close when ESC is pressed. I will be shipped today. |
I've done no investigation here and likely will not for a long time as it is very far down my priority chain. Yes, you can definitely help out! See the CONTRIBUTING.md for info on setting up the project (pretty straight forward). You can run the docs for testing locally. Just update one of the examples to reproduce the issue, then debug the components involved (Popup, Modal). Open a PR when you figure it out :) |
@kevinswarner Definitely agree, please do not rely on the setTimeout hack :) I have a hunch this bug has something to with the Portal which powers the Popup and Modal. The Portal mounts components after it is mounted, it takes two render cycles for the actual component (Popup/Modal) to mount. By waiting one tick, I was testing whether or not this two-step render process was involved. It appears it is. However, it is not meant to be a suggested fix or workaround. |
I ran across this bug today and was trying to figure out what the cause was and I think I have narrowed it down to the portal behavior when processing click events I have recreated the bug here: https://stackblitz.com/edit/react-b5as56 In the example above, If the click event target changes the contents of the Popup such that the target is no longer in the contents, thus not exist within the Popup, the portal click handler issues a close. It seems that Virtual DOM diff-ing goes into play here as well because if the element that initiated the click event is replaced by a different element of exactly the same Markup building blocks (switching a Don't really know what the best solution to this would be, but I figured I would put my findings here in case someone wants to take a crack at it. Note: The |
This could be the problem precisely. I've run into this in the Datetime component #1240 also. It shows a calendar in a popup. We change the calendar (Table) on click of a Table Cell. This would force the Popup to close, even when the Popup was set to open/close The bug is exactly the race condition you describe. The Table Cell click event replaces the Popup contents, so when the Popup asks "Does the Popup contain the clicked node?" the answer is always no. Because the Table Cell that was clicked is no longer in the DOM and therefore no longer contained in the Popup. It was replaced with a new Table. A Fix ?I've not tested a solution for this yet, but I proposed an idea in this Datetime PR comment: // TODO: Fix close on trigger blur, it closes when clicking inside the calendar.
// DatetimeCalendar contents are changed on click, so Popup cannot find the clicked node within the calendar.
// If the clicked node is not within the Portal, it is considered a "blur" and closes.
// Enable close on trigger blur after this is fixed.
// Portal should be able to identify clicks within the portal even with no e.target, perhaps using x y coords. My final thought there, as you can see, is perhaps using click event x/y coordinates to determine if the click was within the bounds of the Popup. This way, it is not dependant on the child node still existing in the DOM. |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 30 days if no further activity occurs. Thank you for your contributions. |
I upgraded dependencies of @ecnaidar example, and it doesn't work: https://stackblitz.com/edit/react-f6puqr |
There is no dismissable message in the example provided. Please file a new issue and complete the entire report. |
Steps
Open your docs at https://react.semantic-ui.com/modules/popup#popup-example-click
and paste the following code in editor to simulate a dismissable Message Component inside a Popup which is triggered by click on a Button
Expected Result
The message will be dismissed but the Popup remains opened
Actual Result
The Popup will be completely destroyed
Version
0.67.2
The text was updated successfully, but these errors were encountered: