-
Notifications
You must be signed in to change notification settings - Fork 4k
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 Issue #2388 #2397
Fix Issue #2388 #2397
Conversation
Details: The issue was related to portals and there management of outside-clicks, where Portal support dismissing the Portal children component once a click outside the Portal is recongnize. The issue can be reproduce on any element (Not just the Select component) by clicking on element that will be deleted from the DOM, In this case the click on the icon-delete button (x) was doing some internal logic of the Select component and then the icon-delete will be removed from the DOM, after that the click will be bubbled to the Portal and the portal will decide if the click is in/out the Portal to handle auto-close feature. The Portal will search for icon-delete and see if it in the this.rootNode of the Portal, and because has already deleted the removal icon from the DOM tree it will not find it in the rootNode and it will decide that it's was a click outside the Portal! Also, the event.target won't keep a full reference to the top body element in case we want to search the icon-delete parents! so the way is to use event.path / event.composedPath to have the full snapshot of the bubbled event
Codecov Report
@@ Coverage Diff @@
## master #2397 +/- ##
=======================================
Coverage 99.73% 99.73%
=======================================
Files 152 152
Lines 2664 2664
=======================================
Hits 2657 2657
Misses 7 7
Continue to review full report at Codecov.
|
Does this PR propose better changes that #2384? |
@layershifter This PR is better than the one referenced absolutely. I don't know why to propose such a difficult implementation with ClientX and ClientY when you can fix it with a tiny fix like the proposed here in this PR! Also, Compare the doesNodeContainClick.js code with my isNodeInEventBubblePath.js code! Which is easiest to maintain? and note that the issue in Portal and not in Popup and I've already tested the case and it's working as expected (Hopefully not affecting other scenarios) |
There's a problem in FF, I'm checking more things |
The cross-browser incompatibility dropped this PR in my face 😞 You can amend it or close it if you think about something else |
Details: The issue was related to portals and their management of outside-clicks, where Portal
support dismissing the Portal children component once a click outside the Portal is recongnize. The
issue can be reproduce on any element (Not just the Select component) by clicking on element that will be deleted from the DOM, In this case the click on the icon-delete button (x) was doing some internal logic of the Select component and then the icon-delete will be removed from the DOM, after that the click will be bubbled to the Portal and the portal will decide if the click is in/out the Portal to handle auto-close feature.
The Portal will search for icon-delete and see if it in the this.rootNode of the Portal, and because it has already deleted the icon from the DOM tree it will not find it in the rootNode and it will decide that it's was a click outside the Portal! Also, the event.target won't keep a full reference to the top body element in case we want to search the icon-delete parents! so the way is to use event.path / event.composedPath to have the full snapshot of the bubbled event