Semantic-ui-react: Popup: zIndex is not applied in v2

[welcome[bot]]

👋 Thanks for opening your first issue here! If you're reporting a 🐞 bug, please make sure you've completed all the fields in the issue template so we can best help.

We get a lot of issues on this repo, so please be patient and we will get back to you as soon as we can.

[layershifter]

@JMS-1 thanks for reporting this, this is a definitely valid issue. This change is a part of #3947:

https://github.com/Semantic-Org/Semantic-UI-React/blob/1fbeed912c80037b07201f37973f2d96fcd933b9/src/modules/Popup/Popup.js#L178-L182

It was done to avoid positioning issues with Popper.js as margins on containers are not handled anymore. As we don't control styles on our side we cannot simply remove margins (it also related to arrow positioning) 💥

Sorry for this unobvious change, I didn't take that it can affect apps... However, I tried different options before and I don't have a better solution there.

1. I will upgrade the migration guide and PR description to mention this issue.
2. I will added a shorthand prop (probably unstable_container) to allow customize classes and styles on that top level div:

<Popup unstable_container={{ className: 'foo', data-id: 'container', style: { zIndex: 10000 } }} />

Will it work for you?

[JMS-1]

Actually on first thought using Popup in Modal seems not be so rare - Modal for some Input Form, Popup for errors therein - so the solution seems to be a bit strange.

But indeed I think we have only few places and could live with it. I would have done something like this in CSS IF the container div hat at lease ANY class attached to ist but it hasn't. In F12 developer mode z-index: 1000 did the job.

So to summarize: yes, this will work for us. But it feels wrong (sorry) and personally I would prefer to have some class on the container IF this is possible (even data- would do) but as I understand this isn't.

Thanks

Jochen

PS: Thought by the side: why not putting an ADDITIONAL own div (absolute/0/0) around the container?

[gshilin]

Any way to supply a class/style for parent will do. But don't give it a name of unstable as I'm not supposed to know about it's real nature.

[felixmosh]

The issue reproduces in many examples in my app which are not related to Modals.

Tooltip in a menu dropdown

Page inside a Pusher (part of SideBar component) which has z-index by default.

Looks like in previous popup version, the popup (it has a large z-index, 1900) class was the root of the popup portal, and not it is a child of some container.

Simple solution is to add the z-index to the wrapper it self.

[felixmosh]

I've patched locally my semantic-ui with

popperStyle = _extends({zIndex: 1900}, popperStyle);

In here: https://github.com/Semantic-Org/Semantic-UI-React/blob/1fbeed912c80037b07201f37973f2d96fcd933b9/src/modules/Popup/Popup.js#L177

And it solved the issue.

[JMS-1]

Do you think it's a good idea to have the z-Index in code? Wouldn't it be better to add some (new?) class and style the z-Index in?

[gshilin-sdb]

It may be good enough to add .ui.popup to this div, it will definitely solve the problem

[felixmosh]

Do you think it's a good idea to have the z-Index in code? Wouldn't it be better to add some (new?) class and style the z-Index in?

The problem is that the this lib (React version of SUI) can't make changes in the CSS... it just relays on the original styles.

[felixmosh]

It may be good enough to add .ui.popup to this div, it will definitely solve the problem

I think that the comment here https://github.com/Semantic-Org/Semantic-UI-React/blob/1fbeed912c80037b07201f37973f2d96fcd933b9/src/modules/Popup/Popup.js#L178-L182 is the reason why this won't work.

[gshilin-sdb]

Popup has properties popperDependencies and popperModifiers. Maybe there is some way to add popperStyle?

[gshilin-sdb]

and popperClassName

[layershifter]

Just to clarify, this change is a part of 2.0.0 and there is no any bugfixes or features that can force you to upgrade.

---

Why additional div is required?

Popper.js v2 requires to not have margins, but SUI defines them on .popup and they are also used to position a pointing beak:

I tried few options that came to my mind, but all of them were overcomplicated. Having an additional wrapping div is the most simple solution and it's used for positioning now.

---

Popup has properties popperDependencies and popperModifiers. Maybe there is some way to add popperStyle?

This will now scale as at some point it may be useful to add a data attribute, for example. Having a slot seems more reasonable for me and it's aligned with our API.

<Popup unstable_container={{ className: 'foo', data-id: 'container', style: { zIndex: 10000 } }} />
<Popup container={{ className: 'foo', data-id: 'container', style: { zIndex: 10000 } }} />
<Popup popper={{ className: 'foo', data-id: 'container', style: { zIndex: 10000 } }} />

---

@felixmosh I am thinking about syncing zIndex value between a container and .popup element:

componentDidMount() {
  this.syncZIndex()
}

componentDidUpdate() {
  this.syncZIndex()
}

syncZIndex() {
  // if it's defined in <Popup popper={{ style: {} }} /> there is no sense to override it
  if (_.isUndefined(this.props.popper.style.zIndex)) {
    this.popperRef.style.zIndex = window.getComputedStyle(this.popupRef).zIndex
  }
}

[felixmosh]

Since the value is hard coded in the css, there is no need to sync it.
Just add it to the proper styles as I suggested in my PR.

[layershifter]

@felixmosh it can be the first iteration, however the issue that zIndex is not hardcoded, it's a variable in LESS so technically someone can have a different value:

z-index: @zIndex;

https://github.com/Semantic-Org/Semantic-UI/blob/master/src/definitions/modules/popup.less#L34

IMO, at least we should sync the initial value in componentDidMount().

[felixmosh]

Agree with syncing it in componentDidMount

[sergeu90]

Hello everyone. Have workaround?

[felixmosh]

Hello everyone. Have workaround?

https://github.com/Semantic-Org/Semantic-UI-React/issues/4083#issuecomment-703237102

[sergeu90]

@felixmosh Can we without fork?. Because i don't want to do fork

[felixmosh]

@sergeu90 I'm using patch-package in order to make a patch for the lib.

It allows you to modify the files in node_modules and save a patch file of this modification. At the end it will apply the patch each time you install dependency.

[sergeu90]

@felixmosh Thanks. I see you created pull request.

[maxfi]

@sergeu90 Expanding upon @felixmosh's workaround, for people unfamiliar with patch-package here are some more detailed instructions:

1. Setup patch-package

2. Create a new file patches/semantic-ui-react+2.0.0.patch and add the following to the file:

   diff --git a/node_modules/semantic-ui-react/dist/es/modules/Popup/Popup.js b/node_modules/semantic-ui-react/dist/es/modules/Popup/Popup.js
   index 9d58fd4..6c1a42c 100644
   --- a/node_modules/semantic-ui-react/dist/es/modules/Popup/Popup.js
   +++ b/node_modules/semantic-ui-react/dist/es/modules/Popup/Popup.js
   @@ -151,7 +151,7 @@ var Popup = /*#__PURE__*/function (_Component) {
           // defined by SUI CSS. It also means that this `div` will be positioned instead of `content`.
           React.createElement("div", {
             ref: popperRef,
   -          style: popperStyle
   +          style: _extends({zIndex: 1900}, popperStyle)
           }, /*#__PURE__*/React.createElement(ElementType, _extends({}, contentRestProps, {
             className: classes,
             style: styles
   

3. Run npx patch-package semantic-ui-react

4. Run git commit -m 'Fix popup z-index'

This worked for me in a meteor project.

[Blapi]

Using popperModifers, you can use this workaround to apply directly on your Popup :

popperModifiers={[
                  {
                    name: 'zIndex',
                    enabled: true,
                    phase: 'write',
                    fn: ({state}) => {
                      state.styles.popper.zIndex = 42
                    },
                  },
                ]}

[sergeu90]

@Blapi It is cool decision. But we have many popup in the code and in each add this workaround not nice

[JMS-1]

actually I prefer the solution where the zIndex is not fixed in code - although up to now we never messed with the zIndex on semantic dialogs but you knows. So as long this isn't fixed we prefer a bit of a manual work-around which seems (only tested on one place) to work.

Step 1 is to provide a global method (TypeScript version)
export function semantic2PopupFix(elem: HTMLElement): void { if (elem) { elem.parentElement.parentElement.style.zIndex = getComputedStyle(elem.parentElement).zIndex } }

Step 2 is to use it on every Popup.Content where it is necessary (in fact not all Popups come on as a problem and perhaps this is not applicable to the DateTimeInput problem - didn't check)
<Ref innerRef={semantic2PopupFix}> <Popup.Content> ... </Popup.Content> </Ref>

In addition I still can't understand why it's so hard to "fix the fix" in suir itself - the outstanding PR uses a hardcoded 1900 and IMHO this is not an option. Ok, so in suir 2 you added an additional div for some good reasons. Why not simply propagate the zIndex from the inside to this new element when it's mounted - e.g. like the syncZIndex approach mentioned earlier?

Regards

Jochen

[Blapi]

@Blapi It is cool decision. But we have many popup in the code and in each add this workaround not nice

@sergeu90 Indeed, this is why we took the decision to create a wrapper for the SUIR Popup

class PoppersPopup extends React.Component {
  static propTypes = {
    popperModifiers: PropTypes.arrayOf(PropTypes.object),
  }

  static defaultProps = {
    popperModifiers: [],
  }

  render() {
    const {popperModifiers, ...additionalProps} = this.props

    return (
      <Popup
        popperModifiers={[
          {
            name: 'zIndex',
            enabled: true,
            phase: 'write',
            fn: ({state}) => {
              state.styles.popper.zIndex = 42 // eslint-disable-line no-param-reassign
            },
          },
          ...popperModifiers,
        ]}
        {...additionalProps}
      />
    )
  }
}

And then replace each SUIR Popup in our codebase with a simple find & replace + a script to adding the imports (~60 files so it was not hectic)

And nothing was broken as we always used the content and trigger props, SUIR Popup was never wrapping anything in our codebase so this fix was, as I said, a - quick - workaround but it didn't take us much times to fix this issue in our app after finding out for popperModifiers while waiting for an official fix :)

But I can understand your point if you are not in my kind of situation, just want to point out that you can do this instead of adding this fix in every Popup that you have !

[layershifter]

Folks please stop fixing this issue on your side, it will be fixed in SUIR on next week. If it creates issues for you just stay few days on 1.x until 2.0.1 will be released.

I have personal issues that's why it's not fixed yet...

[layershifter]

Thanks every for reporting and suggestions ❤️

I fixed the issue in #4094 (visual tests were also added), BTW popperModifiers were the correct option as React lifecycle methods are executed in different frames. I expect that we will release it in 2.0.1 on Monday.

[layershifter]

Released in [email protected] 🚢 Please check if everything works as expected.

[iampeter]

@layershifter just in time for me, thx :)