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

Popover cannot be controlled programmatically consistently #3496

Open
Benrajalu opened this issue Feb 21, 2024 · 6 comments
Open

Popover cannot be controlled programmatically consistently #3496

Benrajalu opened this issue Feb 21, 2024 · 6 comments

Comments

@Benrajalu
Copy link

Benrajalu commented Feb 21, 2024

Current behavior

When providing open and setOpen on a Popover, the expectation is that the element can be fully controlled by an external state.
However, when updating open programmatically through external events, the popover store seems out of sync: clicking on the Disclosure closes then immediately re-opens the popover. Clicking back on the node responsible for the outside event does not close the Popover, in spite of `hideOnIntereactOutside.

Steps to reproduce the bug

  • Open this codesandbox
  • Try typing in the input (triggering onChange={() => setOpen(true)})
  • Try clicking on the button, a Disclosure for the Popover

The Popover will flash from closed to open. It should close.

  • Repeat the first two steps, then click on the input again

The Popover will remain open, when it should close.

Expected behavior

  • When a Popover is controlled, on interacting outside or clicking back on an interactive element, the "outside" state should still be driving the display of the Popover.
  • When opening or closing a Popover through an external state, the Disclosure state should be updated.

Workaround

#3496 (comment)

Possible solutions

No response

@diegohaz

This comment was marked as outdated.

@diegohaz
Copy link
Member

Steps to reproduce the bug

  • Open this codesandbox
  • Try typing in the input (triggering onChange={() => setOpen(true)})
  • Try clicking on the button, a Disclosure for the Popover

The Popover will flash from closed to open. It should close.

  • Repeat the first two steps, then click on the input again

The Popover will remain open, when it should close.

Actually, after going through these steps, things seem to be working as expected.

As of v0.3.6, popovers support multiple disclosure elements, which are automatically assigned upon interaction. When you trigger the popover externally as the user interacts with the input, the input element becomes the popover's disclosure element. Consequently, the PopoverDisclosure element serves as an outside element that hides the popover when it gains focus (on mouse down) and re-opens the popover on click.

I understand that this behavior might not be suitable for this particular scenario, and we might consider adding an option to turn off this automatic disclosure detection in the future.

In the meantime, you can ensure that the popover's disclosure element is always assigned to the PopoverDisclosure element using the code below:

const popover = usePopoverStore();
const disclosureRef = useRef<HTMLButtonElement>(null);
const disclosureElement = popover.useState("disclosureElement");

useEffect(() => {
  popover.setDisclosureElement(disclosureRef.current);
}, [popover, disclosureElement]);

<PopoverDisclosure ref={disclosureRef} />

@Benrajalu
Copy link
Author

Benrajalu commented Feb 22, 2024

That is indeed a working workaround, thanks a lot.
It wasn't clear at all to us that we were effectively creating that second disclosure by programatically opening the popover through an apparently unrelated event. An option would indeed be very welcome, if only as a pointer that something automatic may happen if we don't disable it.

@ArthurGoupil
Copy link

ArthurGoupil commented Feb 28, 2024

I think it partly fixes the issue.
I still have weird behaviours with controlled popovers :/

Enregistrement.de.l.ecran.2024-02-28.a.17.56.37.mov

@diegohaz
Copy link
Member

@ArthurGoupil Could you please provide a repro?

@ArthurGoupil
Copy link

False alarm @diegohaz , problem was on my side.
Sorry for that!

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

No branches or pull requests

3 participants