Flip Modifiers Not Applied

[athielen2]

Describe the bug

When passing popperConfig into an Overlay component, the flip modifiers are not applied.

To Reproduce

Steps to reproduce the behavior:

1. Create an Overlay component with a popperConfig that has flip modifiers set
2. Observe that those modifiers are not applied

Expected behavior

Expected that the modifiers passed in (ex: behavior) would be applied to the underlying popper instance

Screenshots

If applicable, add screenshots to help explain your problem.

Environment (please complete the following information)

• Operating System: [e.g. macOS]
  MacOS
• Browser, Version [e.g. Chrome 74]
  Chrome 78.0.3904.108
• react-overlays Version [e.g. 2.0.0]
  2.1.0

Additional context

I see in Overlay.js that the preventOverflow modifiers are spread onto the popper config. Is this intentional?

const { styles, arrowStyles, ...popper } = usePopper(target, rootElement, {
    ...popperConfig,
    placement: placement || 'bottom',
    enableEvents: props.show,
    modifiers: {
      ...modifiers,
      preventOverflow: {
        padding: containerPadding || 5,
        ...modifiers.preventOverflow,
      },
      arrow: {
        ...modifiers.arrow,
        enabled: !!arrowElement,
        element: arrowElement,
      },
      flip: {
        enabled: !!flip,
        ...modifiers.preventOverflow,
      },
    },
  });

[taion]

@jquense should explicitly-specified popperConfig take precedence over our inferred options here?

[juliusv]

I've been running into something similar, where the positionFixed: true / strategy: 'fixed' popper config option is ignored, and thus I cannot get a dropdown menu to break out of its scroll container. The reason is that not all popper options are passed on, but are computed like this from just the modifiers property of the provided popper options:

react-overlays/src/DropdownMenu.js

Lines 42 to 65 in 26c1f4f

	const modifiers = toModifierMap(popperConfig.modifiers);
	const popper = usePopper(toggleElement, menuElement, {
	placement,
	enabled: !!(shouldUsePopper && show),
	modifiers: {
	...modifiers,
	eventListeners: {
	enabled: !!show,
	},
	arrow: {
	...modifiers.arrow,
	enabled: !!arrowElement,
	options: {
	...modifiers.arrow?.options,
	element: arrowElement,
	},
	},
	flip: {
	enabled: !!flip,
	...modifiers.flip,
	},
	},
	});

Although this docstring falsely claims that all provided options are passed on:

react-overlays/src/DropdownMenu.js

Lines 153 to 156 in 26c1f4f

	/**
	* A set of popper options and props passed directly to react-popper's Popper component.
	*/
	popperConfig: PropTypes.object,

[jquense]

which options aren't being passed?

[juliusv]

@jquense Anything outside of the modifiers aren't passed to Popper, but there might be relevant non-modifier options too, like in my case the positionFixed: true (which is parallel to modifiers as a top-level key in popperConfig, not nested inside it). If I manually hack this file to add positionFixed: true in the object passed to usePopper(), my dropdown breaks out correctly.

[jquense]

Happy to take a PR fixing this if you want to send one over

[juliusv]

@jquense I'm afraid I'm lacking a bit of JS testing expertise (naively getting the computed style of the menu and checking for the value of the position attribute yielded me an empty value, so I couldn't get the test to work), but the actual non-code fix itself should just be:

diff --git a/src/DropdownMenu.js b/src/DropdownMenu.js
index ae98e2e..8c2dfbe 100644
--- a/src/DropdownMenu.js
+++ b/src/DropdownMenu.js
@@ -42,6 +42,7 @@ export function useDropdownMenu(options = {}) {
   const modifiers = toModifierMap(popperConfig.modifiers);
 
   const popper = usePopper(toggleElement, menuElement, {
+    ...popperConfig,
     placement,
     enabled: !!(shouldUsePopper && show),
     modifiers: {

For now I switched back to Reactstrap (where this behavior can be made to work), but maybe someone else feels qualified to also add working tests here.

[jquense]

that change looks fine, i don't think you need to add any tests for it

[juliusv]

@jquense Ok great, I filed #779, but didn't mark it as fixing this specific issue here, as the flip modifier still gets overridden after the spread of the user-provided options. Or maybe the spreading order should be inverted, so that for both modifiers and the rest of the popper config, the user-provided values always win? But for now I only provided the obvious (least risky) fix in the PR.