fix(Popover): unnecesary call of onOpen and missing call of onClose (#4230)

Co-authored-by: Tobias Barsnes <tobias.barsnes@digdir.no>

Author: unekinn

.changeset/stupid-birds-read.md

@@ -0,0 +1,8 @@
+---
+"@digdir/designsystemet-react": patch
+---
+
+**Popover**: Fix unnecesary call of `onOpen` and missing call of `onClose`
+
+  - Don't call `onOpen` when clicking `Popover.Trigger` when `Popover` is already open.
+  - Call `onClose` when a controlled `Popover` is closed by clicking on `Popover.Trigger`.

packages/react/src/components/popover/popover.stories.tsx

@@ -188,8 +188,13 @@ export const Controlled: StoryFn<typeof Popover> = () => {
 
   return (
     <Popover.TriggerContext>
-      <Popover.Trigger onClick={() => setOpen(!open)}>Slett</Popover.Trigger>
-      <Popover open={open} onClose={() => setOpen(false)} data-color='neutral'>
+      <Popover.Trigger>Slett</Popover.Trigger>
+      <Popover
+        open={open}
+        onOpen={() => setOpen(true)}
+        onClose={() => setOpen(false)}
+        data-color='neutral'
+      >
         <Paragraph>Er du sikker på at du vil slette?</Paragraph>
         <div
           style={{

packages/react/src/components/popover/popover.test.tsx

@@ -23,6 +23,38 @@ const CompControlled = () => {
   );
 };
 
+type ControlledWithTriggerProps = {
+  isInitiallyOpen: boolean;
+  onOpen?: () => void;
+  onClose?: () => void;
+};
+
+const CompControlledWithTrigger = (props: ControlledWithTriggerProps) => {
+  const [open, setOpen] = useState(props.isInitiallyOpen);
+
+  return (
+    <Popover.TriggerContext>
+      <Popover.Trigger>
+        Dropdown
+        {open ? <ChevronDownIcon aria-hidden /> : <ChevronUpIcon aria-hidden />}
+      </Popover.Trigger>
+      <Popover
+        open={open}
+        onOpen={() => {
+          props.onOpen?.();
+          setOpen(true);
+        }}
+        onClose={() => {
+          props.onClose?.();
+          setOpen(false);
+        }}
+      >
+        {contentText}
+      </Popover>
+    </Popover.TriggerContext>
+  );
+};
+
 const contentText = 'popover content';
 
 const render = async (props: PopoverProps = {}) => {
@@ -41,6 +73,19 @@ const render = async (props: PopoverProps = {}) => {
   };
 };
 
+const renderControlledWithTrigger = async (
+  props: ControlledWithTriggerProps,
+) => {
+  /* Flush microtasks */
+  await act(async () => {});
+  const user = userEvent.setup();
+
+  return {
+    user,
+    ...renderRtl(<CompControlledWithTrigger {...props} />),
+  };
+};
+
 describe('Popover', () => {
   it('should render popover on trigger-click when closed', async () => {
     const { user } = await render();
@@ -177,4 +222,48 @@ describe('Popover', () => {
     await act(async () => await user.click(document.body));
     expect(onClose).toHaveBeenCalledTimes(1);
   });
+
+  it('should not call onOpen when the popover is open', async () => {
+    const onOpen = vi.fn();
+    const { user } = await render({ onOpen });
+    const popoverTrigger = screen.getByRole('button');
+
+    await act(async () => await user.click(document.body));
+    expect(onOpen).toHaveBeenCalledTimes(0);
+
+    await act(async () => await user.click(popoverTrigger));
+    expect(screen.queryByText(contentText)).toBeVisible();
+    expect(onOpen).toHaveBeenCalledTimes(1);
+    await act(async () => await user.click(popoverTrigger));
+    expect(onOpen).toHaveBeenCalledTimes(1);
+  });
+
+  describe('with controlled state', () => {
+    it('should not call onClose when the popover is closed', async () => {
+      const onClose = vi.fn();
+      const { user } = await renderControlledWithTrigger({
+        isInitiallyOpen: false,
+        onClose,
+      });
+      const popoverTrigger = screen.getByRole('button');
+
+      await act(async () => await user.click(popoverTrigger));
+      expect(onClose).toHaveBeenCalledTimes(0);
+      await act(async () => await user.click(popoverTrigger));
+      expect(onClose).toHaveBeenCalledTimes(1);
+    });
+
+    it('should not call onOpen when the popover is open', async () => {
+      const onOpen = vi.fn();
+      const { user } = await renderControlledWithTrigger({
+        isInitiallyOpen: true,
+        onOpen,
+      });
+      const popoverTrigger = screen.getByRole('button');
+
+      expect(screen.queryByText(contentText)).toBeVisible();
+      await act(async () => await user.click(popoverTrigger));
+      expect(onOpen).toHaveBeenCalledTimes(0);
+    });
+  });
 });

packages/react/src/components/popover/popover.tsx

@@ -131,12 +131,13 @@ export const Popover = forwardRef<HTMLDivElement, PopoverProps>(
 
         if (isTrigger) {
           event.preventDefault(); // Prevent native Popover API
-          setInternalOpen((open) => !open);
-          onOpen?.();
         }
-        if (isOutside && internalOpen) {
+        if (controlledOpen && (isTrigger || isOutside)) {
           setInternalOpen(false);
           onClose?.();
+        } else if (isTrigger) {
+          setInternalOpen(true);
+          onOpen?.();
         }
       };