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

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

Open
Vino4 opened this issue Jun 27, 2017 · 21 comments
Open

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

Vino4 opened this issue Jun 27, 2017 · 21 comments

Comments

@Vino4
Copy link

Vino4 commented Jun 27, 2017

This is a follow up to this issue:
#1545

The submitter was forwarded to /Semantic-UI
I followed up in this post:
Semantic-Org/Semantic-UI#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

@katpas
Copy link

katpas commented Jul 22, 2017

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

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

@levithomason
Copy link
Member

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

stale bot commented Feb 4, 2018

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 stale bot added the stale label Feb 4, 2018
@pdfowler
Copy link

pdfowler commented Feb 14, 2018

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.

@stale stale bot removed the stale label Feb 14, 2018
@aryalprakash
Copy link

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

@brianespinosa
Copy link
Member

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

pdfowler commented Apr 5, 2018

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)

@fracmak
Copy link
Member

fracmak commented Apr 6, 2018

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

pdfowler commented Apr 6, 2018

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

ForumT commented Apr 16, 2018

Hi,

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

@redlive
Copy link

redlive commented Jun 18, 2018

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

@stale
Copy link

stale bot commented Dec 15, 2018

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 stale bot added the stale label Dec 15, 2018
@ritvikgautam
Copy link

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

@stale stale bot removed the stale label Feb 23, 2019
@levithomason
Copy link
Member

levithomason commented Feb 24, 2019

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

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

@levithomason
Copy link
Member

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: #1803 (comment)

@levithomason levithomason reopened this Feb 24, 2019
@lubber-de

This comment was marked as spam.

@semireg
Copy link

semireg commented May 30, 2019

This worked for me:

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

@stale
Copy link

stale bot commented Nov 26, 2019

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

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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests