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

Author: layershifter

cypress/integration/Popup/Popup.visual.js

@@ -8,6 +8,7 @@ describe('Popup: visual', () => {
     cy.get('[data-tid="button-popup"]').click()
     cy.get('[data-tid="popup-content"]').should('be.visible')
 
+    // This screenshot contains invalid position of an unknown reason
     cy.percySnapshot('Popup: inside a Modal')
   })
 
@@ -19,4 +20,22 @@ describe('Popup: visual', () => {
 
     cy.percySnapshot('Popup: floating Button')
   })
+
+  it('flowing', () => {
+    cy.visit('/maximize/popup-example-flowing')
+
+    cy.get('.ui.button').click()
+    cy.get('.ui.popup').should('be.visible')
+
+    cy.percySnapshot('Popup: flowing')
+  })
+
+  it('positionFixed', () => {
+    cy.visit('/maximize/popup-example-position-fixed')
+
+    cy.get('.ui.button').click()
+    cy.get('.ui.popup').should('be.visible')
+
+    cy.percySnapshot('Popup: positionFixed')
+  })
 })

docs/src/examples/modules/Popup/Usage/PopupExamplePopper.js

@@ -0,0 +1,18 @@
+import React from 'react'
+import { Button, Popup } from 'semantic-ui-react'
+
+const PopupExamplePopper = () => (
+  <Popup
+    content={
+      <>
+        A wrapping element in this Popup will have custom <code>id</code> &{' '}
+        <code>zIndex</code>.
+      </>
+    }
+    on='click'
+    popper={{ id: 'popper-container', style: { zIndex: 2000 } }}
+    trigger={<Button>Open a popup</Button>}
+  />
+)
+
+export default PopupExamplePopper

docs/src/examples/modules/Popup/Usage/PopupExamplePositionFixed.js

@@ -0,0 +1,17 @@
+import React from 'react'
+import { Button, Popup } from 'semantic-ui-react'
+
+const PopupExamplePositionFixed = () => (
+  <Popup
+    content={
+      <>
+        This Popup is positioned with <code>position: fixed</code>
+      </>
+    }
+    on='click'
+    positionFixed
+    trigger={<Button>Open a popup</Button>}
+  />
+)
+
+export default PopupExamplePositionFixed

docs/src/examples/modules/Popup/Usage/index.js

@@ -94,6 +94,40 @@ const PopupUsageExamples = () => (
       examplePath='modules/Popup/Usage/PopupExamplePopperDependencies'
       renderHtml={false}
     />
+    <ComponentExample
+      title={
+        <>
+          <code>popper</code> element
+        </>
+      }
+      description={
+        <>
+          From <code>semantic-ui-react@2.0.0</code> we are using an additional
+          wrapping element around <code>Popup</code> for positioning, see{' '}
+          <a href='https://github.com/Semantic-Org/Semantic-UI-React/pull/3947'>
+            Semantic-Org/Semantic-UI-React#3947
+          </a>{' '}
+          for more details. To pass props to this element <code>popper</code>{' '}
+          shorthand can be used.
+        </>
+      }
+      examplePath='modules/Popup/Usage/PopupExamplePopper'
+    />
+    <ComponentExample
+      title={
+        <>
+          Positioning via <code>position: fixed</code>
+        </>
+      }
+      description={
+        <>
+          If your reference element is in a <code>fixed</code> container, use{' '}
+          <code>positionFixed</code>. This will prevent any jumpiness since no
+          repositioning is needed.
+        </>
+      }
+      examplePath='modules/Popup/Usage/PopupExamplePositionFixed'
+    />
     <ComponentExample
       title='Actions'
       description='A popup can be triggered on hover, click, focus or multiple actions.'

docs/src/pages/MigrationGuide.mdx

@@ -32,6 +32,12 @@ The `popperModifiers` prop should be defined as an array with changed format (se
 +<Popup popperModifiers={[{ name: 'preventOverflow', options: { padding: 0 } }]} />
 ```
 
+### a wrapping element around `Popup`
+
+We started to use an additional wrapping element around `Popup` for positioning, see [Semantic-Org/Semantic-UI-React#3947](https://github.com/Semantic-Org/Semantic-UI-React/pull/3947) for more details. To pass props to this element `popper` shorthand can be used, see [an example](/modules/popup/#usage-position-fixed).
+
+⚠️We also made a fix in [Semantic-Org/Semantic-UI-React#4094](https://github.com/Semantic-Org/Semantic-UI-React/pull/4094) to transfer `zIndex` value to avoid any breaks.
+
 ## `Responsive` component was removed
 
 `Responsive` component was deprecated in 1.1.0. There are two main reasons for the removal: there is no connection between breakpoints and pure SSR (server side rendering) support.

src/modules/Popup/Popup.d.ts

@@ -119,6 +119,9 @@ export interface StrictPopupProps extends StrictPortalProps {
   /** Tells `Popper.js` to use the `position: fixed` strategy to position the popover. */
   positionFixed?: boolean
 
+  /** A wrapping element for an actual content that will be used for positioning. */
+  popper?: SemanticShorthandItem<React.HTMLAttributes<HTMLDivElement>>
+
   /** An array containing custom settings for the Popper.js modifiers. */
   popperModifiers?: any[]
 

src/modules/Popup/Popup.js

@@ -2,13 +2,14 @@ import EventStack from '@semantic-ui-react/event-stack'
 import cx from 'clsx'
 import _ from 'lodash'
 import PropTypes from 'prop-types'
-import React, { Component, createRef } from 'react'
+import React, { Component } from 'react'
 import { Popper } from 'react-popper'
 import shallowEqual from 'shallowequal'
 
 import {
   eventStack,
   childrenUtils,
+  createHTMLDivision,
   customPropTypes,
   getElementType,
   getUnhandledProps,
@@ -32,7 +33,9 @@ export default class Popup extends Component {
   state = {}
 
   open = false
-  triggerRef = createRef()
+  zIndexWasSynced = false
+
+  triggerRef = React.createRef()
 
   static getDerivedStateFromProps(props, state) {
     if (state.closed || state.disabled) return {}
@@ -145,6 +148,7 @@ export default class Popup extends Component {
       flowing,
       header,
       inverted,
+      popper,
       size,
       style,
       wide,
@@ -174,25 +178,36 @@ export default class Popup extends Component {
       ...style,
     }
 
-    return (
-      // 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}>
-        <ElementType {...contentRestProps} className={classes} style={styles}>
-          {childrenUtils.isNil(children) ? (
-            <>
-              {PopupHeader.create(header, { autoGenerateKey: false })}
-              {PopupContent.create(content, { autoGenerateKey: false })}
-            </>
-          ) : (
-            children
-          )}
-          {hideOnScroll && <EventStack on={this.hideOnScroll} name='scroll' target='window' />}
-        </ElementType>
-      </div>
+    const innerElement = (
+      <ElementType {...contentRestProps} className={classes} style={styles}>
+        {childrenUtils.isNil(children) ? (
+          <>
+            {PopupHeader.create(header, { autoGenerateKey: false })}
+            {PopupContent.create(content, { autoGenerateKey: false })}
+          </>
+        ) : (
+          children
+        )}
+        {hideOnScroll && <EventStack on={this.hideOnScroll} name='scroll' target='window' />}
+      </ElementType>
     )
+
+    // 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`.
+    return createHTMLDivision(popper || {}, {
+      overrideProps: {
+        children: innerElement,
+        ref: popperRef,
+        style: {
+          // Fixes layout for floated elements
+          // https://github.com/Semantic-Org/Semantic-UI-React/issues/4092
+          display: 'flex',
+          ...popperStyle,
+        },
+      },
+    })
   }
 
   render() {
@@ -202,6 +217,7 @@ export default class Popup extends Component {
       eventsEnabled,
       offset,
       pinned,
+      popper,
       popperModifiers,
       position,
       positionFixed,
@@ -220,6 +236,37 @@ export default class Popup extends Component {
       { name: 'preventOverflow', enabled: !!offset },
       { name: 'offset', enabled: !!offset, options: { offset } },
       ...popperModifiers,
+
+      // We are syncing zIndex from `.ui.popup.content` to avoid layering issues as in SUIR we are using an additional
+      // `div` for Popper.js
+      // https://github.com/Semantic-Org/Semantic-UI-React/issues/4083
+      {
+        name: 'syncZIndex',
+        enabled: true,
+        phase: 'beforeRead',
+        fn: ({ state }) => {
+          if (this.zIndexWasSynced) {
+            return
+          }
+
+          // if zIndex defined in <Popup popper={{ style: {} }} /> there is no sense to override it
+          const definedZIndex = popper?.style?.zIndex
+
+          if (_.isUndefined(definedZIndex)) {
+            // eslint-disable-next-line no-param-reassign
+            state.elements.popper.style.zIndex = window.getComputedStyle(
+              state.elements.popper.firstChild,
+            ).zIndex
+          }
+
+          this.zIndexWasSynced = true
+        },
+        effect: () => {
+          return () => {
+            this.zIndexWasSynced = false
+          }
+        },
+      },
     ]
     debug('popper modifiers:', modifiers)
 
@@ -352,6 +399,9 @@ Popup.propTypes = {
   /** Tells `Popper.js` to use the `position: fixed` strategy to position the popover. */
   positionFixed: PropTypes.bool,
 
+  /** A wrapping element for an actual content that will be used for positioning. */
+  popper: customPropTypes.itemShorthand,
+
   /** An array containing custom settings for the Popper.js modifiers. */
   popperModifiers: PropTypes.array,
 

test/specs/modules/Popup/Popup-test.js

@@ -343,6 +343,50 @@ describe('Popup', () => {
     })
   })
 
+  describe('popper', () => {
+    it('passes a zIndex value from .popup', (done) => {
+      wrapperMount(<Popup open style={{ zIndex: 5000 }} />)
+
+      const popperElement = wrapper.find('Popper').childAt(0)
+      const popperNode = popperElement.getDOMNode()
+
+      setTimeout(() => {
+        // zIndex transfer is done in a Popper modifier which will be executed in next frame
+        popperNode.style.zIndex.should.be.equal('5000')
+        done()
+      }, 0)
+    })
+
+    it('zIndex passed to a shorthand wins', (done) => {
+      wrapperMount(<Popup open popper={{ style: { zIndex: 100 } }} style={{ zIndex: 5000 }} />)
+
+      const popperElement = wrapper.find('Popper').childAt(0)
+      const popperNode = popperElement.getDOMNode()
+
+      setTimeout(() => {
+        // zIndex transfer is done in a Popper modifier which will be executed in next frame
+        popperNode.style.zIndex.should.be.equal('100')
+        done()
+      }, 0)
+    })
+
+    it('additional props can be passed via shorthand', () => {
+      wrapperMount(<Popup open popper={{ className: 'foo', id: 'bar' }} />)
+      const popperElement = wrapper.find('Popper').childAt(0)
+
+      popperElement.should.have.prop('className', 'foo')
+      popperElement.should.have.prop('id', 'bar')
+    })
+
+    it('"style" prop is merged', () => {
+      wrapperMount(<Popup open popper={{ style: { color: 'red', display: 'block' } }} />)
+      const popperElement = wrapper.find('Popper').childAt(0)
+
+      popperElement.should.have.style('color', 'red')
+      popperElement.should.have.style('display', 'flex')
+    })
+  })
+
   describe('popperModifiers', () => {
     it('are passed to Popper', () => {
       const modifierOffset = { name: 'offset', options: { offset: [0, 10] } }