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 Issue #2388 #2397

Closed
wants to merge 1 commit into from
Closed

Fix Issue #2388 #2397

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Dec 25, 2017

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

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-io
Copy link

Codecov Report

Merging #2397 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2397   +/-   ##
=======================================
  Coverage   99.73%   99.73%           
=======================================
  Files         152      152           
  Lines        2664     2664           
=======================================
  Hits         2657     2657           
  Misses          7        7
Impacted Files Coverage Δ
src/addons/Portal/Portal.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7ef9fdb...61e80f0. Read the comment docs.

@layershifter
Copy link
Member

Does this PR propose better changes that #2384?

@ghost
Copy link
Author

ghost commented Dec 26, 2017

@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)

@ghost ghost mentioned this pull request Dec 26, 2017
@ghost
Copy link
Author

ghost commented Dec 26, 2017

There's a problem in FF, I'm checking more things

@ghost
Copy link
Author

ghost commented Dec 26, 2017

The cross-browser incompatibility dropped this PR in my face 😞 You can amend it or close it if you think about something else

@ghost ghost mentioned this pull request Dec 26, 2017
@ghost ghost closed this Dec 31, 2017
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants