Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unable to use Popper with Popover, wrong positions #985

Closed
Venipa opened this issue Dec 10, 2021 · 25 comments · Fixed by #1820
Closed

Unable to use Popper with Popover, wrong positions #985

Venipa opened this issue Dec 10, 2021 · 25 comments · Fixed by #1820
Assignees

Comments

@Venipa
Copy link

Venipa commented Dec 10, 2021

What package within Headless UI are you using?

@headlessui/react@^1.4.2

What version of that package are you using?

v1.4.2

What browser are you using?

Chrome / Electron

Reproduction URL

Reproduction: https://codesandbox.io/s/tailwind-react-headlessui-popper-5c15b?file=/src/components/Homepage.js

Describe your issue

Popper always renders Popover.Panel to top left (both strategies "fixed" & "absolute")

@jakekaminski
Copy link

jakekaminski commented Jan 6, 2022

Having the same issue. This appears to be a bug with the Transition component. I tried removing it and using the default management and it positions correctly.

export default function Popover({ button, children }: PopoverProps) {
    const [
        referenceElement,
        setReferenceElement,
    ] = useState<HTMLButtonElement | null>(null)
    const [popperElement, setPopperElement] = useState<HTMLDivElement | null>(
        null
    )
    const { styles, attributes } = usePopper(referenceElement, popperElement, {
        strategy: 'fixed',
    })

    return (
        <Popover as="div" className="relative">
            <Popover.Button
                ref={setReferenceElement}
                className="inline-flex"
            >
                {button}
            </Popover.Button>

            <Popover.Panel
                ref={setPopperElement}
                style={styles.popper}
                {...attributes.popper}
                className="z-10 w-64 mt-2 origin-top bg-white divide-y divide-gray-100 rounded-md shadow-lg ring-1 ring-black ring-opacity-5 focus:outline-none"
            >
                <div className="px-1 py-1 ">{children}</div>
            </Popover.Panel>
        </Popover>
    )
}

However, I really like the animations provided with Transition. It'd be nice if Popover, Transition, and Popper worked together seamlessly.

@atomiks
Copy link

atomiks commented Jan 20, 2022

Does https://floating-ui.com/docs/react-dom work for you? (this is the next generation of the Popper project)

@RobinMalfait is it possible to update the docs with Floating UI usage instead?

@Venipa
Copy link
Author

Venipa commented Jan 22, 2022

Does https://floating-ui.com/docs/react-dom work for you? (this is the next generation of the Popper project)

@RobinMalfait is it possible to update the docs with Floating UI usage instead?

https://codesandbox.io/s/tailwind-react-headlessui-floating-ui-sfl8n?file=/src/components/Homepage.js
seems to work 👌

@atomiks
Copy link

atomiks commented Jan 22, 2022

@Venipa that's not how the API is used, the placement is not working at all 😅

The problem is the ref (ref={floating}) is not being called on the Popover.Panel element, see line 41: https://codesandbox.io/s/tailwind-react-headlessui-floating-ui-forked-skor3?file=/src/components/Homepage.js

@frontshift
Copy link

Bummer!
Stumbled upon the same issue and was unable to use floating-ui due to some ESM related bundling issue in of the middleware I need.
One ugly workaround using popper is to set unmount to false

<Transition show={open} unmount={false}>

Which will essentially initially mount all popovers and then only leave the wrapper div in the DOM. You will get an initial flicker of the popovers once they get mounted but that's fixable by hiding the transition and setting it to display: block' in a setTimeout`.
Works but I have quite a few popovers, so I'll probably end up sacrificing the transition

@frontshift
Copy link

Slightly less ugly version: call update() on popper as soon as it exists (when the popover gets mounted)

 const { styles, attributes, update } = usePopper(buttonRef.current, popperElement, {.....

  React.useEffect(() => {
    if (update) {
      update();
    }
  }, [update]);

Works well without flicker and you don't need to set unmount to false.

@grefrit
Copy link

grefrit commented Feb 23, 2022

Hello. I have similar issue with Menu.Items component. I would like to render it in the portal with floating-ui, but example provided in floating ui docs here doesn't update component. It seems that refs.floating doesn't change. Also it doesn't work with transition component completely probably because of that line.


It seems to transition component doesn't merge refs provided to child component. Any workarounds?

@RobinMalfait RobinMalfait self-assigned this Feb 25, 2022
@RobinMalfait
Copy link
Member

Hey! Thank you for your bug report!
Much appreciated! 🙏

All components will forward the ref now so the popper examples should work.

You can already try it using npm install @headlessui/react@insiders or yarn add @headlessui/react@insiders.

@nkrmr
Copy link

nkrmr commented Feb 28, 2022

Sorry but it doesn't work for me the popover still render to top left

@ycs77
Copy link

ycs77 commented Mar 15, 2022

Hi! I create a package Headless UI Float, that can be easy to position floating Headless UI elements using Floating UI, support Transition & Portal.

If you need to float Headless UI element can try it out 😊

@aarontwf
Copy link

aarontwf commented Apr 7, 2022

I managed to get it to work by creating a wrapping <div> and attaching the floating-ui floating ref and styles to it instead along with autoUpdate.

const { x, y, reference, floating, strategy, update, refs } = useFloating({
  placement: 'bottom-end',
  strategy: 'fixed',
  middleware: [offset(8), flip()],
});

useEffect(() => {
  if (!refs.reference.current || !refs.floating.current) {
    return;
  }

  return autoUpdate(
    refs.reference.current,
    refs.floating.current,
    update,
  );
}, [refs.reference, refs.floating, update]);

return (
  <div
    ref={floating}
    style={{
        position: strategy,
        top: y ?? '',
        left: x ?? '',
    }}
  >
    <Transition as={Fragment} ...>
      <Popover.Panel>
        ....
      </Popover.Panel>
    </Transition>
  </div>
)

@atomiks
Copy link

atomiks commented Apr 8, 2022

@aarontwf note that the autoUpdate listeners are on all the time in that scenario and will be very expensive, rather they should only be on when the Popover is mounted to the DOM. I'm not sure if there's an open boolean for you to check?

@atomiks
Copy link

atomiks commented Apr 15, 2022

@RobinMalfait the issue here is not ref forwarding but ref stealing. You need to preserve the consumer's ref: facebook/react#8873 (comment)

Note that TypeScript thinks ref doesn't exist on JSX.Element / children, idk why but I think it's wrong.

@JacobMuchow
Copy link

JacobMuchow commented Jun 30, 2022

I am having the same issue. @RobinMalfait I think this issue needs to be reopened.

Specifically this is the problem I seem to be having:

const MyPopover = () => {
  const [popoverRef, setPopoverRef] = useState()

  return (
    <Transition as={Fragment} ... >
      <Popover.Panel as="div" ref={setPopoverRef}>
        /* children... */
      </Popover.Panel>
    </Transition>
  )
}

setPopoverRef() is not called. If I remove the Transition, it is called.

Noteworthy: this happens using either react-popper or @floating-ui/react-dom.

@tozz
Copy link

tozz commented Sep 5, 2022

Would also like to see this reopened (sorry for resurrecting a very old issue), not being able to use floating ui and transitions makes the ui just feel a little bit less nice.

RobinMalfait added a commit that referenced this issue Sep 5, 2022
When a higher-level component (like `Transition`) provides a `ref` to
its child component, then it will override the `ref` that was
potentially already on the child.

This will make sure that these are merged together correctly.

Fixes: #985
RobinMalfait added a commit that referenced this issue Sep 5, 2022
* fix ref stealing

When a higher-level component (like `Transition`) provides a `ref` to
its child component, then it will override the `ref` that was
potentially already on the child.

This will make sure that these are merged together correctly.

Fixes: #985

* update changelog
@RobinMalfait
Copy link
Member

I had this on my todo list to revisit and this this should be fixed by #1820, and will be available in the next release.

You can already try it using npm install @headlessui/react@insiders.

@tozz
Copy link

tozz commented Sep 6, 2022

I had this on my todo list to revisit and this this should be fixed by #1820, and will be available in the next release.

You can already try it using npm install @headlessui/react@insiders.

Seems the positioning is working now, nice! The enter transition is however not working unfortunately, no matter what duration you have (the exit transition works fine though). I know floating ui has changed a bit from how popper used to work, but I assume the general idea should still apply, wrapping the <Popover.Panel> with a <Transition>

@caleb
Copy link

caleb commented Sep 9, 2022

Try calling floating-ui's update method from the beforeEnter property of Transition. That's what I needed to do to get it working properly.

The transition component will force the position of the popup to be corrected and the element will be animated in. As long as you're not transitioning the top or left it should work correctly. I used a transition all for debugging purposes and the tooltip animated across the screen the first time it was shown.

EDIT: I see that you're having trouble with the animation, not the positioning... Here is my transition code. It's Clojurescript, but you can see the element nesting ($ is react.createElement and <> is react.Fragment)

The Transition wraps the floating element like you say. I have a tailwind prefix, but you can see the transition classes that I'm using.

(<>
      children
      ($ Transition {:show        open
                     :as          react/Fragment
                     :beforeEnter update
                     :enter       "tw-ease-in-out tw-transition-[transform,opacity] tw-transform-gpu tw-duration-200"
                     :enterFrom   "tw-opacity-0 tw-scale-95"
                     :enterTo     "tw-opacity-100 tw-scale-100"
                     :leave       "tw-ease-in-out tw-transition-[transform,opacity] tw-transform-gpu tw-duration-200"
                     :leaveFrom   "tw-opacity-100 tw-scale-100"
                     :leaveTo     "tw-opacity-0 tw-scale-95"}
         ($ :div {:ref floating & floating-props :className "tw-relative"}
            ($ :div {:className classes}
               tooltip)
            ($ :div {:ref       arrow-ref
                     :style     arrow-style
                     :className arrow-classes}))))

@tozz
Copy link

tozz commented Sep 11, 2022

Thanks for trying to help @caleb , unfortunately it's still the same issue. I can see the transition element starting out correct, but it looks like it's the re-rendered (as in the child nodes being recreated in the DOM) at the floating ui destination position almost immediately. Very strange.

@caleb
Copy link

caleb commented Sep 22, 2022

@tozz

One more idea for you that I found worked in a case with floating and transition: try adding a hidden attribute to your popup (I think this JSX syntax is right):

<Transition...>
  <div hidden={true}>
    ...
  </div>
</Transition>

This fixed a similar case I had where a popup wasn't animating in properly. I don't know why this fixed my case, but it did.

@tozz
Copy link

tozz commented Sep 23, 2022

@caleb Thanks! That actually makes the transition work but break positioning calculations somehow, so negative to negative translations doesn't use the correct offsets 😅 I will try to get a small repo together showcasing the issue.

@caleb
Copy link

caleb commented Sep 23, 2022

I think I've figured out at least part or maybe all of the problem.

The floating and reference "ref" functions (they aren't references, but are functions that set references) of useFloating call the update function of useFloating. update calls floating-ui's computePosition function, and then updates the react state of the useFloating hook with the new coordinates.

This causes a re-render of your component where you set the new top and left and position of your floating element like the manual says.

Because the headlessui Transition has already started, changing the style of the floating element causes the css transition to cancel.

I tested this by hooking into the transitioncancel event on a parent element and sure enough the first time the popup was rendered the transitioncancel event was fired:

function trancancel(e) {
  console.log("transition cancelled")
}
...

  React.useEffect(function () {
    const el = ref.current;
    if (el) {
      el.addEventListener("transitioncancel", trancancel)
    }

    return function () {
      if (el) {
        el.removeEventListener(trancancel)
      }
    }

  }, [])

Subsequent times the popup is shown, as long as the position hasn't changed, your component won't be re-rendered since useFloating does a deep comparison of the data returned by computePosition and only updates the state if the position is different. This allows the animation to happen

The solution I came up with for this is to make my own useFloating hook that doesn't trigger re-renders of my react component when the position changes or when references are set, but instead lets me call update as needed (in beforeEnter in the case of headlessui Transition) which updates the top and left of my floating element directly.

I'm hoping this will allow the element to be positioned before the transition even starts and allows it to run.

I'll post back with a hook if I get it working.

@caleb
Copy link

caleb commented Sep 23, 2022

I've come up with a solution that I think works. It's rudimentary, but you can tweak it to your needs:

https://gist.github.com/caleb/6e20d9c1ec218d05d917795822d9540c

Basically, I created a hook called useFloatingTransition that you can use like this:

let floatingState = useFloatingTransition(isShowing, {placement: "bottom-start"})
const {floating, reference} = floatingState

This replaces the useFloating hook. Just pass the open state, and any options to floating-ui's computePosition. Then I created a wrapper component for Transition called FloatingTransition that you can use like this:

{floatingState.visible ?
  <FloatingTransition state={floatingState}
                      enter="transform transition duration-[400ms]"
                      enterFrom="opacity-0 rotate-[-120deg] scale-50"
                      enterTo="opacity-100 rotate-0 scale-100"
                      leave="transform duration-200 transition ease-in-out"
                      leaveFrom="opacity-100 rotate-0 scale-100 "
                      leaveTo="opacity-0 scale-95 ">
    <div ref={floating} className="absolute h-16 w-16 rounded-md bg-white shadow-lg" />
  </FloatingTransition> : null}

<button ref={reference}
        onClick={(e) => { e.preventDefault(); setIsShowing((s) => !s) }}
        className="backface-visibility-hidden mt-8 flex items-center rounded-full bg-black bg-opacity-20 px-3 py-2 text-sm font-medium text-white">
  <svg viewBox="0 0 20 20" fill="none" className="h-5 w-5 opacity-70">
    <path
      d="M14.9497 14.9498C12.2161 17.6835 7.78392 17.6835 5.05025 14.9498C2.31658 12.2162 2.31658 7.784 5.05025 5.05033C7.78392 2.31666 12.2161 2.31666 14.9497 5.05033C15.5333 5.63385 15.9922 6.29475 16.3266 7M16.9497 2L17 7H16.3266M12 7L16.3266 7"
      stroke="currentColor"
      strokeWidth="1.5"/>
  </svg>

  <span className="ml-3">Click to transition</span>
</button>

You use reference and floating passed back from useFloatingTransition like you would from useFloating. The difference is that they are just plain refs and won't trigger a re-render when an element is assigned to them. Then you pass the whole return from useFloatingTransition to the FloatingTransition's state prop.

You can inspect the visible boolean of the floating state to determine whether you should render the popup. This was important to me since I will be rendering a lot of these on a page and only want to render them when they are showing.

Simply change the boolean passed in as the first argument to useFloatingTransition and your popup should be animated in and out appropriately.

The way it works is by using two state values to determine whether the transition is rendered and whether it's "shown" (in Transition's sense). This allows us to first render the transition in a closed state, but have it's contents put in the dom (because I use unmount = false in the Transition), allowing us to update the position and set up autoUpdate from floating-ui.

Then after the popup is added to the DOM, it updates the show flag, which triggers the animation, and since the element is already in the dom and positioned correctly, the enter animation isn't cancelled.

@maxratajczak
Copy link

maxratajczak commented Mar 16, 2023

Hi! I create a package Headless UI Float, that can be easy to position floating Headless UI elements using Floating UI, support Transition & Portal.

If you need to float Headless UI element can try it out 😊

YOU ARE A LIFESAVER. Everyone should check this package out it is so easy to use and fixed all my problems.

@BilalShah3
Copy link

i am using floating ui vue package.

the example they are provided is works fine when page is not scroll if we scroll the page the floating element is not anchored with refrence element. i have a requirement where i use float element inside teleport. i dont know how to handle this. if anyone helps me.

example link: https://github.com/tailwindlabs/headlessui/blob/main/packages/playground-vue/src/components/menu/menu-with-floating-ui.vue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.