fix(popper): flickering (#1434)

* fix(popper): flickering * changelog * added comment and small fix * flipVariationsByContent should be added by default to Popper to improve positioning algorithm * addressed last comments * fix(popper): flickering * changelog * added comment and small fix * flipVariationsByContent should be added by default to Popper to improve positioning algorithm * addressed last comments * comment Popper dependency to review
microsoft · May 31, 2019 · 882f293 · 882f293
1 parent e077ecf
commit 882f293
Show file tree

Hide file tree

Showing 5 changed files with 50 additions and 18 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -30,6 +30,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 - Call `getDerivedStateFromProps()` from `AutoControlledComponent` in `Dropdown` ([#1416](https://github.com/stardust-ui/react/pull/1416))
 - Fix `backgroundHover1` color in the Teams dark theme `colorScheme` @mnajdova ([1437](https://github.com/stardust-ui/react/pull/1437))
 - Revert changes with different roots in `Icon` component @layershifter ([#1435](https://github.com/stardust-ui/react/pull/1435))
+- Fix flickering issues with `Dropdown` and `Popup` components @Bugaa92 ([#1434](https://github.com/stardust-ui/react/pull/1434))
 
 ### Features
 - Add keyboard navigation and screen reader support for `Accordion` @silviuavram ([#1322](https://github.com/stardust-ui/react/pull/1322))

diff --git a/packages/react/src/components/Dropdown/Dropdown.tsx b/packages/react/src/components/Dropdown/Dropdown.tsx
@@ -660,7 +660,7 @@ class Dropdown extends AutoControlledComponent<WithAsProp<DropdownProps>, Dropdo
           position={position}
           offset={offset}
           rtl={rtl}
-          eventsEnabled={open}
+          enabled={open}
           targetRef={this.containerRef}
           positioningDependencies={[items.length]}
         >

diff --git a/packages/react/src/components/Menu/MenuItem.tsx b/packages/react/src/components/Menu/MenuItem.tsx
@@ -247,7 +247,6 @@ class MenuItem extends AutoControlledComponent<WithAsProp<MenuItemProps>, MenuIt
               align={vertical ? 'top' : 'start'}
               position={vertical ? 'after' : 'below'}
               targetRef={this.itemRef}
-              modifiers={{ flip: { flipVariationsByContent: true } }}
             >
               {Menu.create(menu, {
                 defaultProps: {

diff --git a/packages/react/src/lib/positioner/Popper.tsx b/packages/react/src/lib/positioner/Popper.tsx
@@ -22,7 +22,7 @@ const Popper: React.FunctionComponent<PopperProps> = props => {
   const {
     align,
     children,
-    eventsEnabled,
+    enabled,
     modifiers: userModifiers,
     offset,
     pointerTargetRef,
@@ -51,14 +51,33 @@ const Popper: React.FunctionComponent<PopperProps> = props => {
     [rtl, offset, position],
   )
 
-  React.useEffect(
+  const scheduleUpdate = React.useCallback(() => {
+    if (popperRef.current) {
+      popperRef.current.scheduleUpdate()
+    }
+  }, [])
+
+  const destroyInstance = React.useCallback(() => {
+    if (popperRef.current) {
+      popperRef.current.destroy()
+      popperRef.current = null
+    }
+  }, [])
+
+  const createInstance = React.useCallback(
     () => {
+      destroyInstance()
+
+      if (!enabled || !targetRef.current || !contentRef.current) {
+        return
+      }
+
       const pointerTargetRefElement = pointerTargetRef && pointerTargetRef.current
       const popperHasScrollableParent = getScrollParent(contentRef.current) !== document.body
 
       const modifiers: PopperJS.Modifiers = _.merge(
         { preventOverflow: { padding: 0 } },
-        { flip: { padding: 0 } },
+        { flip: { padding: 0, flipVariationsByContent: true } },
         /**
          * When the popper box is placed in the context of a scrollable element, we need to set
          * preventOverflow.escapeWithReference to true and flip.boundariesElement to 'scrollParent' (default is 'viewport')
@@ -92,39 +111,51 @@ const Popper: React.FunctionComponent<PopperProps> = props => {
 
       const options: PopperJS.PopperOptions = {
         placement: proposedPlacement,
-        eventsEnabled,
         positionFixed,
         modifiers,
         onCreate: handleUpdate,
         onUpdate: handleUpdate,
       }
 
       popperRef.current = createPopper(targetRef.current, contentRef.current, options)
-      return () => popperRef.current.destroy()
     },
-    [computedModifiers, eventsEnabled, userModifiers, positionFixed, proposedPlacement],
+    // TODO review dependencies for popperHasScrollableParent
+    [computedModifiers, enabled, userModifiers, positionFixed, proposedPlacement],
   )
 
   React.useEffect(
     () => {
-      popperRef.current.scheduleUpdate()
+      createInstance()
+      return destroyInstance
     },
-    [...positioningDependencies, computedPlacement],
+    [createInstance],
   )
 
+  React.useEffect(scheduleUpdate, [...positioningDependencies, computedPlacement])
+
   const child =
     typeof children === 'function'
       ? (children as PopperChildrenFn)({
           placement: computedPlacement,
-          scheduleUpdate: () => popperRef.current && popperRef.current.scheduleUpdate(),
+          scheduleUpdate,
         })
-      : React.Children.only(children)
-
-  return <Ref innerRef={contentRef}>{child as React.ReactElement}</Ref>
+      : children
+
+  return (
+    <Ref
+      innerRef={contentElement => {
+        contentRef.current = contentElement
+        // for correct positioning we need to create the PopperJS instance immediately after we get a ref to the popper box
+        createInstance()
+      }}
+    >
+      {React.Children.only(child) as React.ReactElement}
+    </Ref>
+  )
 }
 
 Popper.defaultProps = {
-  eventsEnabled: true,
+  enabled: true,
   positionFixed: false,
   positioningDependencies: [],
 }

diff --git a/packages/react/src/lib/positioner/types.ts b/packages/react/src/lib/positioner/types.ts
@@ -1,3 +1,4 @@
+import * as React from 'react'
 import PopperJS from 'popper.js'
 
 export type Position = 'above' | 'below' | 'before' | 'after'
@@ -42,9 +43,9 @@ export interface PopperProps extends PositioningProps {
 
   /**
    * Enables events (resize, scroll).
-   * @prop {Boolean} eventsEnabled=true
+   * @prop {Boolean} enabled=true
    */
-  eventsEnabled?: boolean
+  enabled?: boolean
 
   /**
    * List of modifiers used to modify the offsets before they are applied to the Popper box.
@@ -66,7 +67,7 @@ export interface PopperProps extends PositioningProps {
   /**
    * Ref object containing the target node (the element that we're using as reference for Popper box).
    */
-  targetRef?: React.RefObject<Element>
+  targetRef: React.RefObject<Element>
 
   /**
    * Rtl attribute for the component.