Semantic-UI-React icon indicating copy to clipboard operation
Semantic-UI-React copied to clipboard

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

Open Vino4 opened this issue 7 years ago • 21 comments

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

  • Create a modal with a dimmer setting that's not false.
  • Place a pop up inside the modal

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

Vino4 avatar Jun 27 '17 22:06 Vino4

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

katpas avatar Jul 22 '17 14:07 katpas

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

eli8levit avatar Oct 03 '17 13:10 eli8levit

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.

levithomason avatar Oct 10 '17 06:10 levithomason

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.

stale[bot] avatar Feb 04 '18 19:02 stale[bot]

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.

pdfowler avatar Feb 14 '18 04:02 pdfowler

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

aryalprakash avatar Apr 05 '18 14:04 aryalprakash

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

brianespinosa avatar Apr 05 '18 15:04 brianespinosa

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 prop inline on <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)

pdfowler avatar Apr 05 '18 23:04 pdfowler

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

fracmak avatar Apr 06 '18 01:04 fracmak

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 😎

pdfowler avatar Apr 06 '18 16:04 pdfowler

Hi,

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

ForumT avatar Apr 16 '18 09:04 ForumT

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

redlive avatar Jun 18 '18 21:06 redlive

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!

stale[bot] avatar Dec 15 '18 22:12 stale[bot]

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

ritvikgautam avatar Feb 23 '19 01:02 ritvikgautam

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

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

levithomason avatar Feb 24 '19 18:02 levithomason

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

levithomason avatar Feb 24 '19 18:02 levithomason

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

lubber-de avatar Apr 03 '19 22:04 lubber-de

This worked for me:

.blurring.dimmed.dimmable > :not(.dimmer).popup {
  filter: none;
}

semireg avatar May 30 '19 18:05 semireg

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!

stale[bot] avatar Nov 26 '19 18:11 stale[bot]

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>}
   ...
/>

motiwalam avatar Aug 18 '22 19:08 motiwalam

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.

amol-vaditake avatar Sep 21 '23 13:09 amol-vaditake