search

Semantic-Org / Semantic-UI-React

The official Semantic-UI-React integration
https://react.semantic-ui.com
MIT License
13.23k stars 4.05k forks source link

Popup's do not work with modal's when dimmers are enabled. #1803

Open Vino4 opened 7 years ago

Vino4 commented 7 years ago

This is a follow up to this issue: https://github.com/Semantic-Org/Semantic-UI-React/issues/1545

The submitter was forwarded to /Semantic-UI I followed up in this post: https://github.com/Semantic-Org/Semantic-UI/issues/5440

It turns out that the issue has to do with the order the the popups/modals are created in. /Semantic-UI responded that pre-existing modals should be used for this. As far as i know, there's no way to achieve that in the current react variant of the UI.

Thank you :)

Steps

Expected Result

Hovering over trigger displays the popup on top of the modal

Actual Result

Popup displays behind the modal, even behind the dimmer actually!

Version

0.70.0

Testcase

https://codepen.io/eGust/pen/zZXomK

katpas commented 7 years ago

@Vino4 thanks for creating this issue. I've had the same problem. We're using rc-time-picker and have just added a Modal around that component.

The pop-up on the time picker now only shows if the dimmer on the modal is set to false.

Update for anyone following: I've figured out this bug only happens for us when the dimmer is set to "blurring". If it's just left as the default dimmer it works as expected.

eli8levit commented 7 years ago

@Vino4 @katpas There is a way that it could work now with blurring dimmer?

levithomason commented 7 years ago

Oh boy, this is a tricky issue. In short, the CSS for a blurring dimmer will blur anything outside of the ui page modals dimmer transition visible active but the Portal renders first the Dimmer, then the Popup both to the body as siblings.

The Popup is, therefore, outside of the dimmer and is blurred by the dimmable dimmed blurring classes on the `body.

In this case, the Popup needs to be rendered into the Modal, with the Modal as its mountNode. However, there is a catch 22 here. The Modal creates the Popup.

In order to do this currently, you'd have to have some ugly logic. First, the Modal needs to be rendered, with some unique id so you can find the DOM node. Then, onMount, you need to query the DOM for the Modal's DOM node so you can set the Popup's mountNode to the Modal. Case in point: https://codepen.io/levithomason/pen/WZJpLN

However, this also is insufficient as the Popup currently doesn't calculate its position based on the mountNode, but assumes the body to be the mountNode.

Workaround

Use a <Modal dimmer="inverted" /> as it is closer to blurring than the default and works. The default Dimmer also works.

Fix

The best path here is likely to update the CSS for blurring dimmers. Right now, it blurs anything that is not a dimmer:

.blurring.dimmed.dimmable > :not(.dimmer) {
    filter: blur(5px) grayscale(0.7);
}

Simply allowing a className like not blurred to escape this rule would allow us to ensure Popup portals are not blurred.

I would suggest posting a bug on SUI core and linking this suggestion to it.

stale[bot] commented 6 years ago

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.

pdfowler commented 6 years ago

Ugh - just found a link to this issue going through my code. Anyone up for taking a look at this? It would be pretty awesome 😎 . Posting mainly to de-stale this issue ... I'd take it on but I think the css is a little out of my depth.

aryalprakash commented 6 years ago

I found a way to make it work. Set z-index of the popup the largest.

brianespinosa commented 6 years ago

@pdfowler @aryalprakash per the comment above, either of you might want to try doing this:

I would suggest posting a bug on SUI core and linking this suggestion to it.

Because of the chicken or the egg situation with how the popup is rendered, this is something that will have to be fixed in CSS.

pdfowler commented 6 years ago

According to SUI 761, you can set the popup to display inline, rather than attached to the body tag. However, inline is not a valid property on <Popup ...> element, resulting in warning.js?6327:33 Warning: Unknown propinlineon <div> tag ...

I swore I had seen a way to attach the popup to an element, rather than to the body, but maybe that was on another component. Anyone have thoughts on this path?

Also, @aryalprakash, I'm not sure how you got z-index to work since as @levithomason mentioned, even if the popup is at the "top" of the z-index stack, it will still be blurred per the css implementation of dimmer. Inspecting the elements, my modal's dimmer has a default z-index: 1005, while the popup has a default z-index: 1900, so a manual override should not make any difference. (But clearly, YMMV)

fracmak commented 6 years ago

Inline popups are something I've been wanting as well because if you have a pop-up in something like a scrolling sidebar, the pop-up doesn't stick to the element

pdfowler commented 6 years ago

Looking at the history of this, it looks like inlining was an issue from the original PR

inline (popup as a sibling of trigger, will be implemented later)

... with the discussion continuing around a possible mountNode param, along with the difficulties of implementation, @debrice noted his concerns:

My concern is offering a mountNode props might not be a feature we want to keep as I would rather have an boolean inline option in the long run. The inline option would make the popup a sibling element of the the trigger and require a bit more kung-fu for positioning but would also be more intuitive and closer to the spirit of SUI. I'm not sure how long it would take me to build a clean inline feature so I would rather deliver the popup without it initially.

While other discussions address injecting the Portal adjacent to the trigger. @debrice asked:

can you think of an easy way to have the portal injected as a sibling to the trigger? That's might be the last thing that prevents me from implementing the inline feature

Question with those more familiar with the internals, engineering wise, is this an issue that the Portal component can now support? I assume it is not as simple as adding the {inline: true} per SUI 761, as it involves JS implementation in the SUI-React package. Is this correct?

edit: It appears that Portal does support the mountNode property, could this be as simple as passing the trigger node to mountNode of the Portal, if inline === true in the Popup props?

edit2: After reviewing @levithomason comments above, sounds like there's some additional Fu involved. Hoping to find some time this weekend to take a look, but any further guidance would be appreciated 😎

ForumT commented 6 years ago

Hi,

Anyone found a solution yet? I have been struggling with this issue as well..

redlive commented 6 years ago

I have the same issue... Could it be fixed, please?

stale[bot] commented 5 years ago

There has been no activity in this thread for 180 days. While we care about every issue and we’d love to see this fixed, the core team’s time is limited so we have to focus our attention on the issues that are most pressing. Therefore, we will likely not be able to get to this one.

However, PRs for this issue will of course be accepted and welcome!

If there is no more activity in the next 180 days, this issue will be closed automatically for housekeeping. To prevent this, simply leave a reply here. Thanks!

ritvikgautam commented 5 years ago

Anyone who's still on this, a possible workaround could be to set filter: none css property on the popup.

levithomason commented 5 years ago

Not sure when this was resolved, but it is resolved in the latest release 0.85.0:

http://g.recordit.co/2QUytUKWGn.gif

levithomason commented 5 years ago

Spoke a bit too soon. Default dimmers do work, as shown above.

inverted dimmers work:

http://g.recordit.co/25SBzmlrSa.gif

However, blurring dimmers still have an issue:

http://g.recordit.co/wCZMmroFnU.gif

My original diagnosis and proposed solution still seems relevant: https://github.com/Semantic-Org/Semantic-UI-React/issues/1803#issuecomment-335371082

lubber-de commented 5 years ago

We fixed this for Fomantic-UI by adding :not(.popup) to the existing selector

.blurring.dimmed.dimmable > :not(.dimmer):not(.popup) {
    filter: blur(5px) grayscale(0.7);
}

See https://jsfiddle.net/s48j9b5v

More explanation in the related PR https://github.com/fomantic/Fomantic-UI/pull/616

semireg commented 5 years ago

This worked for me: 

.blurring.dimmed.dimmable > :not(.dimmer).popup {
  filter: none;
}
stale[bot] commented 5 years ago

There has been no activity in this thread for 180 days. While we care about every issue and we’d love to see this fixed, the core team’s time is limited so we have to focus our attention on the issues that are most pressing. Therefore, we will likely not be able to get to this one.

However, PRs for this issue will of course be accepted and welcome!

If there is no more activity in the next 180 days, this issue will be closed automatically for housekeeping. To prevent this, simply leave a reply here. Thanks!

motiwalam commented 2 years ago

The css fix from @semireg didn't work for me, but adding a custom wrapper with float: 'none' through the popper prop did.

<Popup
   ...
   popper={<div style={{ filter: 'none' }}></div>}
   ...
/>
amol-vaditake commented 1 year ago

I encountered the same issue, but I was able to resolve it by using some external CSS:

.ui.popup.transition.visible { z-index: 9999; }

This CSS code snippet fixed the problem for me.