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

[Bug]: Popover with custom target counts clicking on trigger as "clickOutside" #33862

Closed
2 tasks done
hoovercj opened this issue Feb 18, 2025 · 2 comments
Closed
2 tasks done

Comments

@hoovercj
Copy link
Member

Component

Popover

Package version

9.59.0

React version

18.2.0

Environment

System:
    OS: Windows 11 10.0.26100
    CPU: (16) x64 AMD EPYC 7763 64-Core Processor
    Memory: 25.68 GB / 63.95 GB
  Browsers:
    Edge: Chromium (131.0.2903.86)
    Internet Explorer: 11.0.26100.1882
  npmPackages:
    @fluentui/react: 8.111.3 => 8.111.3
    @fluentui/react-calendar-compat: 0.1.1 => 0.1.1
    @fluentui/react-components: 9.56.3 => 9.56.3
    @fluentui/react-experiments: 8.14.114 => 8.14.114
    @fluentui/react-icons: 2.0.271 => 2.0.271
    @fluentui/react-icons-mdl2: 1.3.69 => 1.3.69
    @fluentui/react-list-preview: 0.3.3 => 0.3.3
    @fluentui/react-motion-components-preview: 0.2.0 => 0.2.0
    @fluentui/react-theme: 9.1.14 => 9.1.14
    @fluentui/react-theme-sass: 9.0.0-alpha.18 => 9.0.0-alpha.18
    @fluentui/scheme-utilities: 8.0.1 => 8.0.1
    @types/react: 18.2.33 => 18.2.33
    @types/react-dom: 18.2.14 => 18.2.14
    react: 18.2.0 => 18.2.0
    react-dom: 18.2.0 => 18.2.0

Current Behavior

When you click on a custom trigger that launches a Popover, it is treated as a "click outside" and closes the popover, even when the target is passed as a positioning prop.

The root cause is in usePostioning.ts where targetRef and setTarget are not initialized to point to the passed in target.

I believe the fix is to update these two lines:

-const targetRef = React.useRef<TargetElement | null>(null);
+const targetRef = React.useRef<TargetElement | null>(options.target);

...

- const setTarget = useCallbackRef<TargetElement>(null, target => {
+ const setTarget = useCallbackRef<TargetElement>(targetRef.current, target => {
    if (targetRef.current !== target) {
      targetRef.current = target;
      updatePositionManager();
    }
  });

Expected Behavior

When passing a target to the positioning prop to the popover component, clicking on the target shouldn't count as clicking outside.

Reproduction

https://stackblitz.com/edit/u5pgjqno

Steps to reproduce

  1. Open the stackblitz
  2. Click on the button to open the popover
  3. Click on the button again
  4. Expected: The popover doesn't close and then reopen
  5. Actual: The popover closes and then reopens

Are you reporting an Accessibility issue?

None

Suggested severity

High - No workaround

Products/sites affected

No response

Are you willing to submit a PR to fix?

no

Validations

  • Check that there isn't already an issue that reports the same bug to avoid creating a duplicate.
  • The provided reproduction is a minimal reproducible example of the bug.
@layershifter
Copy link
Member

In offline chat with @edwardUL99 I mentioned that this issue feels like a deja-vu. We have discussed with the team the possibility of new API, but @ling1726 pointed out that state of our components is controlled, so nothing new is needed and it should be done the same way as in other components.

Basically, you can add checks to onOpenChange to ignore some of state updates:

<Popover
  onOpenChange={(e, data) => {
    if (!data.open && e.target === buttonRef.current) {
      // Ignore state update
      return;
    }

    setOpen(data.open);
  }}
/>

Here is a complete example: https://stackblitz.com/edit/u5pgjqno-uxjnn16r?file=src%2Fexample.tsx

@hoovercj
Copy link
Member Author

@layershifter thanks for the example and for the PR to update the docs to help others going forward!

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