Repository: mapbox/mr-ui
Pull request #146 (2021-12-13 20:10:56)

<PopoverTrigger> Gate showBecauseClick if element is inside the popover content
# Description

This fixes a race condition where if a button inside the popover content closes the popover on its `onClick` handler (without a microtask, etc), then it doesn't work since the popover reshows itself.


Patch:
diff --git a/src/components/popover-trigger/popover-trigger.js b/src/components/popover-trigger/popover-trigger.js
index fb5d259..ad81095 100644
--- a/src/components/popover-trigger/popover-trigger.js
+++ b/src/components/popover-trigger/popover-trigger.js
@@ -128,7 +128,10 @@ export default class PopoverTrigger extends React.Component {
       (!this.popoverElement || !this.popoverElement.contains(event.target))
     ) {
       this.hide();
-    } else {
+    } else if (
+      !this.popoverContentElement ||
+      !this.popoverContentElement.contains(event.target)
+    ) {
       this.showBecauseClick();
     }
   };
@@ -319,9 +322,16 @@ export default class PopoverTrigger extends React.Component {
   };
 
   setPopoverElement = (element) => {
+    if (this.props.popoverProps.ref) {
+      this.props.popoverProps.ref(element);
+    }
     this.popoverElement = element;
   };
 
+  setPopoverContentElement = (element) => {
+    this.popoverContentElement = element;
+  };
+
   getPopoverContent = () => {
     const { content } = this.props;
     if (typeof content === 'function') {
@@ -351,6 +361,10 @@ export default class PopoverTrigger extends React.Component {
           receiveFocus={receiveFocus}
           trapFocus={trapFocus}
           onElement={this.setPopoverElement}
+          passthroughProps={{
+            ...(props.popoverProps.passthroughProps || {}),
+            ref: this.setPopoverContentElement
+          }}
         >
           {this.getPopoverContent()}
         </Popover>


Test patch:
diff --git a/src/components/popover-trigger/__tests__/popover-trigger-test-cases.js b/src/components/popover-trigger/__tests__/popover-trigger-test-cases.js
index c065520..39e6314 100644
--- a/src/components/popover-trigger/__tests__/popover-trigger-test-cases.js
+++ b/src/components/popover-trigger/__tests__/popover-trigger-test-cases.js
@@ -73,4 +73,35 @@ testCases.callbacks = {
   }
 };
 
+let popoverTriggerRef = null;
+function setPopoverTriggerRef(ref) {
+  popoverTriggerRef = ref;
+}
+
+testCases.clickInsideContent = {
+  description: 'click inside content',
+  element: (
+    <PopoverTrigger
+      ref={setPopoverTriggerRef}
+      content={() => {
+        return (
+          <button
+            aria-label="Close"
+            onClick={() => {
+              if (popoverTriggerRef) popoverTriggerRef.hide();
+            }}
+          >
+            Close
+          </button>
+        );
+      }}
+      respondsToClick={true}
+    >
+      <button aria-label="Trigger" className="btn">
+        Trigger
+      </button>
+    </PopoverTrigger>
+  )
+};
+
 export { testCases };
diff --git a/src/components/popover-trigger/__tests__/popover-trigger.test.js b/src/components/popover-trigger/__tests__/popover-trigger.test.js
index 3ed1611..600179d 100644
--- a/src/components/popover-trigger/__tests__/popover-trigger.test.js
+++ b/src/components/popover-trigger/__tests__/popover-trigger.test.js
@@ -1,5 +1,5 @@
 import React from 'react';
-import { shallow } from 'enzyme';
+import { mount, shallow } from 'enzyme';
 import { testCases } from './popover-trigger-test-cases';
 
 describe('PopoverTrigger', () => {
@@ -116,4 +116,24 @@ describe('PopoverTrigger', () => {
       expect(testCase.props.onPopoverClose).toHaveBeenCalledTimes(1);
     });
   });
+
+  describe(testCases.clickInsideContent.description, () => {
+    beforeEach(() => {
+      testCase = testCases.clickInsideContent;
+      wrapper = mount(testCase.element);
+    });
+
+    test('when click inside content, popover closes', () => {
+      expect(wrapper.find('Popover').length).toBe(0);
+      wrapper.simulate('click');
+      wrapper.update();
+      expect(wrapper.find('Popover').length).toBe(1);
+
+      const closeButtons = wrapper.find('button[aria-label="Close"]');
+      expect(closeButtons.length).toBe(1);
+      closeButtons.at(0).simulate('click');
+      wrapper.update();
+      expect(wrapper.find('Popover').length).toBe(0);
+    });
+  });
 });