From 24215dc3168fce8bfdd829a32ea3f80568dea68c Mon Sep 17 00:00:00 2001
From: Alejandro Celaya <alejandro.celaya@hypothes.is>
Date: Fri, 29 Nov 2024 10:29:37 +0100
Subject: [PATCH] Avoid popover click-away to trigger when clicking its anchor

---
 src/components/feedback/Popover.tsx          | 25 ++++++++++++++------
 src/components/feedback/test/Popover-test.js | 10 ++++++--
 2 files changed, 26 insertions(+), 9 deletions(-)

diff --git a/src/components/feedback/Popover.tsx b/src/components/feedback/Popover.tsx
index b0b4c959..9c5ea3a8 100644
--- a/src/components/feedback/Popover.tsx
+++ b/src/components/feedback/Popover.tsx
@@ -167,6 +167,7 @@ function usePopoverPositioning(
  */
 function useOnClose(
   popoverRef: RefObject<HTMLElement | undefined>,
+  anchorElementRef: RefObject<HTMLElement | undefined>,
   onClose: () => void,
   popoverOpen: boolean,
   asNativePopover: boolean,
@@ -195,12 +196,22 @@ function useOnClose(
   // Disable these while the popover is closed, otherwise trying to open it
   // by interacting with some other element will trigger a click-away and
   // immediately close it
-  useClickAway(popoverRef, onClose, {
-    enabled: popoverOpen && !asNativePopover,
-  });
-  useKeyPress(['Escape'], onClose, {
-    enabled: popoverOpen && !asNativePopover,
-  });
+  const enabled = popoverOpen && !asNativePopover;
+  useClickAway(
+    popoverRef,
+    e => {
+      // Ignore clicking "away" when the target is the anchor element.
+      // In most cases, popovers will be anchored to a "toggle" which is
+      // supposed to open/close the popover on click, so closing-on-click-away
+      // when they are the target will cause the popover to close and
+      // immediately open again.
+      if (!e.composedPath().includes(anchorElementRef.current!)) {
+        onClose();
+      }
+    },
+    { enabled },
+  );
+  useKeyPress(['Escape'], onClose, { enabled });
 }
 
 export type PopoverProps = {
@@ -263,7 +274,7 @@ export default function Popover({
     align === 'right',
   );
 
-  useOnClose(popoverRef, onClose, open, asNativePopover);
+  useOnClose(popoverRef, anchorElementRef, onClose, open, asNativePopover);
 
   useLayoutEffect(() => {
     const restoreFocusTo = open
diff --git a/src/components/feedback/test/Popover-test.js b/src/components/feedback/test/Popover-test.js
index a3ccd751..fdfe031b 100644
--- a/src/components/feedback/test/Popover-test.js
+++ b/src/components/feedback/test/Popover-test.js
@@ -223,7 +223,10 @@ describe('Popover', () => {
   context('when popover is not supported', () => {
     it('closes popover when Escape is pressed', () => {
       const onClose = sinon.stub();
-      createComponent({ onClose, asNativePopover: false });
+      const wrapper = createComponent({ onClose, asNativePopover: false });
+
+      // The popover needs to be open for the event handler to be attached
+      togglePopover(wrapper);
 
       document.body.dispatchEvent(
         new KeyboardEvent('keydown', { key: 'Escape' }),
@@ -234,7 +237,10 @@ describe('Popover', () => {
 
     it('closes popover when clicking away', () => {
       const onClose = sinon.stub();
-      createComponent({ onClose, asNativePopover: false });
+      const wrapper = createComponent({ onClose, asNativePopover: false });
+
+      // The popover needs to be open for the event handler to be attached
+      togglePopover(wrapper);
 
       const externalButton = document.createElement('button');
       document.body.append(externalButton);