Popup: zIndex is not applied in v2 #4083

JMS-1 · 2020-10-02T18:08:39Z

Bug Report

Steps

Originally we have a Popup inside a Modal. On hover the Popup will not show up.

Expected Result

Should behave as in 1.3.1.

Actual Result

Popup is behind the Modal.

Version

2.0.0

Testcase => Reason

The

created as a popup container no longer has ANY classes, esp. not .ui.popup. Especially the z-index is not set.

The class is set one level below the outer div and the z-Index (1900) is no longer able to move the Popop contents above the modal.

welcome · 2020-10-02T18:08:41Z

👋 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 · 2020-10-03T09:22:29Z

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

Semantic-UI-React/src/modules/Popup/Popup.js

Lines 178 to 182 in 1fbeed9

    
           // /~https://github.com/popperjs/popper-core/blob/f1f9d1ab75b6b0e962f90a5b2a50f6cfd307d794/src/createPopper.js#L136-L137 
        
           // Heads up! 
        
           // A wrapping `div` there is a pure magic, it's required as Popper warns on margins that are 
        
           // defined by SUI CSS. It also means that this `div` will be positioned instead of `content`. 
        
           <div ref={popperRef} style={popperStyle}>

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.

I will upgrade the migration guide and PR description to mention this issue.
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 · 2020-10-03T09:32:39Z

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 · 2020-10-04T01:03:33Z

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 · 2020-10-04T07:02:06Z

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 · 2020-10-04T10:49:36Z

I've patched locally my semantic-ui with

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

In here:

Semantic-UI-React/src/modules/Popup/Popup.js

Line 177 in 1fbeed9

return (

And it solved the issue.

JMS-1 · 2020-10-04T13:09:55Z

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 · 2020-10-04T13:50:41Z

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

felixmosh · 2020-10-04T13:51:58Z

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 · 2020-10-04T13:52:45Z

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

I think that the comment here

Semantic-UI-React/src/modules/Popup/Popup.js

Lines 178 to 182 in 1fbeed9

    
           // /~https://github.com/popperjs/popper-core/blob/f1f9d1ab75b6b0e962f90a5b2a50f6cfd307d794/src/createPopper.js#L136-L137 
        
           // Heads up! 
        
           // A wrapping `div` there is a pure magic, it's required as Popper warns on margins that are 
        
           // defined by SUI CSS. It also means that this `div` will be positioned instead of `content`. 
        
           <div ref={popperRef} style={popperStyle}>

is the reason why this won't work.

gshilin-sdb · 2020-10-04T13:56:03Z

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

gshilin-sdb · 2020-10-04T13:56:31Z

and popperClassName

layershifter · 2020-10-05T07:39:39Z

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 · 2020-10-05T08:09:22Z

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 · 2020-10-05T08:28:34Z

@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 · 2020-10-05T09:17:23Z

Agree with syncing it in componentDidMount

sergeushenecz · 2020-10-08T16:22:37Z

Hello everyone. Have workaround?

felixmosh · 2020-10-08T16:23:47Z

Hello everyone. Have workaround?

#4083 (comment)

sergeushenecz · 2020-10-08T16:33:56Z

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

felixmosh · 2020-10-08T16:39:35Z

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

sergeushenecz · 2020-10-08T16:43:24Z

@felixmosh Thanks. I see you created pull request.

maxfi · 2020-10-12T05:42:10Z

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

Setup patch-package

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

Run npx patch-package semantic-ui-react
Run git commit -m 'Fix popup z-index'

This worked for me in a meteor project.

Blapi · 2020-10-13T23:48:15Z

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
                    },
                  },
                ]}

sergeushenecz · 2020-10-17T06:40:39Z

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

JMS-1 · 2020-10-17T09:42:07Z

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 · 2020-10-17T10:10:55Z

@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 · 2020-10-17T15:11:25Z

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 · 2020-10-25T09:51:16Z

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 · 2020-10-26T16:50:46Z

Released in semantic-ui-react@2.0.1 🚢 Please check if everything works as expected.

iampeter · 2020-10-27T03:43:12Z

@layershifter just in time for me, thx :)

felixmosh added a commit to felixmosh/Semantic-UI-React that referenced this issue Oct 4, 2020

Popup: Add z-index to popper container, fixes Semantic-Org#4083

eccf6f8

felixmosh mentioned this issue Oct 4, 2020

Popup: Add z-index to popper container #4084

Closed

layershifter changed the title ~~popup broken in 2.0.0 - class list not set~~ Popup: zIndex is not applied in v2 Oct 5, 2020

layershifter added the needs engineering label Oct 5, 2020

mysteriiet mentioned this issue Oct 15, 2020

DateTimeInput popups behind modal arfedulov/semantic-ui-calendar-react#211

Closed

meltyshev mentioned this issue Oct 21, 2020

Add User popup incorrectly displayed on Chrome 86 plankanban/planka#46

Closed

layershifter self-assigned this Oct 22, 2020

layershifter added bug and removed needs engineering labels Oct 22, 2020

This was referenced Oct 22, 2020

chore: add visual tests #4093

Merged

fix(Popup): transfer zIndex & fix floated elements #4094

Merged

layershifter closed this as completed in #4094 Oct 25, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Popup: zIndex is not applied in v2 #4083

Popup: zIndex is not applied in v2 #4083

JMS-1 commented Oct 2, 2020

welcome bot commented Oct 2, 2020

layershifter commented Oct 3, 2020

JMS-1 commented Oct 3, 2020

gshilin commented Oct 4, 2020

felixmosh commented Oct 4, 2020 •

edited

Loading

felixmosh commented Oct 4, 2020

JMS-1 commented Oct 4, 2020

gshilin-sdb commented Oct 4, 2020

felixmosh commented Oct 4, 2020

felixmosh commented Oct 4, 2020 •

edited

Loading

gshilin-sdb commented Oct 4, 2020

gshilin-sdb commented Oct 4, 2020

layershifter commented Oct 5, 2020

felixmosh commented Oct 5, 2020

layershifter commented Oct 5, 2020

felixmosh commented Oct 5, 2020

sergeushenecz commented Oct 8, 2020

felixmosh commented Oct 8, 2020

sergeushenecz commented Oct 8, 2020

felixmosh commented Oct 8, 2020

sergeushenecz commented Oct 8, 2020

maxfi commented Oct 12, 2020 •

edited

Loading

Blapi commented Oct 13, 2020

sergeushenecz commented Oct 17, 2020

JMS-1 commented Oct 17, 2020

Blapi commented Oct 17, 2020

layershifter commented Oct 17, 2020 •

edited

Loading

layershifter commented Oct 25, 2020

layershifter commented Oct 26, 2020

iampeter commented Oct 27, 2020

Popup: zIndex is not applied in v2 #4083

Popup: zIndex is not applied in v2 #4083

Comments

JMS-1 commented Oct 2, 2020

Bug Report

Steps

Expected Result

Actual Result

Version

Testcase => Reason

welcome bot commented Oct 2, 2020

layershifter commented Oct 3, 2020

JMS-1 commented Oct 3, 2020

gshilin commented Oct 4, 2020

felixmosh commented Oct 4, 2020 • edited Loading

felixmosh commented Oct 4, 2020

JMS-1 commented Oct 4, 2020

gshilin-sdb commented Oct 4, 2020

felixmosh commented Oct 4, 2020

felixmosh commented Oct 4, 2020 • edited Loading

gshilin-sdb commented Oct 4, 2020

gshilin-sdb commented Oct 4, 2020

layershifter commented Oct 5, 2020

Why additional div is required?

felixmosh commented Oct 5, 2020

layershifter commented Oct 5, 2020

felixmosh commented Oct 5, 2020

sergeushenecz commented Oct 8, 2020

felixmosh commented Oct 8, 2020

sergeushenecz commented Oct 8, 2020

felixmosh commented Oct 8, 2020

sergeushenecz commented Oct 8, 2020

maxfi commented Oct 12, 2020 • edited Loading

Blapi commented Oct 13, 2020

sergeushenecz commented Oct 17, 2020

JMS-1 commented Oct 17, 2020

Blapi commented Oct 17, 2020

layershifter commented Oct 17, 2020 • edited Loading

layershifter commented Oct 25, 2020

layershifter commented Oct 26, 2020

iampeter commented Oct 27, 2020

felixmosh commented Oct 4, 2020 •

edited

Loading

felixmosh commented Oct 4, 2020 •

edited

Loading

Why additional `div` is required?

maxfi commented Oct 12, 2020 •

edited

Loading

layershifter commented Oct 17, 2020 •

edited

Loading