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(Portal): click inside detection #2384

Merged
merged 3 commits into from
Jan 10, 2018

Conversation

levithomason
Copy link
Member

@levithomason levithomason commented Dec 14, 2017

Fixes #1831

Problem

We've had a cryptic bug for a while where clicks inside of a popup cause it to close. This is because clicks inside are detected by checking if the popup .contains() the clicked e.target node.

This fails if the result of the click also removes the node. Example, a popup containing a Dropdown with removable Labels. When the x is clicked on the label, the label is removed from the Dropdown. The Popup then checks to see if the label is within the Popup node. It has been removed, so the Popup does not .contain() the e.target. The Popup then assumes the click originated outside of the Popup (body click) and closes.

Test case: https://codesandbox.io/s/53ykvz7rk4

Solution

Instead of checking for nodes containing other nodes, we now check that the coordinates of the click event are within the bounds of the Portal. This way, we're shielded from DOM updates within the Portal as a result of click handlers.

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

ghost commented Dec 26, 2017

Can you please review my PR (#2397) on the fix with tiny changes before submitting this?

Dealing with ClientX/ClientY is bad when another solutions is available

@levithomason
Copy link
Member Author

@Attrash-Islam I reviewed the other PR and it looks like there's a browser compatibility issue with that event property and method. That would have been a nice approach!

One shortcoming here may be that this implementation doesn't consider whether or not the node is in the topmost layer. I can imagine a rare edge case where a click on an open Modal could be considered "inside" of an underlying Portal if they were aligned. That said, this is probably a very rare case if an issue at all.

@codecov-io
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (master@723f347). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #2384   +/-   ##
=========================================
  Coverage          ?   99.73%           
=========================================
  Files             ?      152           
  Lines             ?     2664           
  Branches          ?        0           
=========================================
  Hits              ?     2657           
  Misses            ?        7           
  Partials          ?        0
Impacted Files Coverage Δ
src/addons/Portal/Portal.js 100% <100%> (ø)
src/modules/Dimmer/Dimmer.js 100% <100%> (ø)
src/modules/Dropdown/Dropdown.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 723f347...cac9b68. Read the comment docs.

@levithomason levithomason merged commit 9f195bb into master Jan 10, 2018
@levithomason levithomason deleted the fix/portal-click-inside-detection branch January 10, 2018 07:45
@levithomason
Copy link
Member Author

Released in semantic-ui-react@0.77.2

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.

Popup: Popup in a Popup closes when you interact with the inner Popup
2 participants