Repository: mapbox/mr-ui
Pull request #19 (2018-07-31 17:08:43)

PopoverTrigger: Improve focus & click interaction
Addresses part of #10.

Still a WIP until I verify that it fixes some buggy-feeling behavior in Studio ...

davidtheclark: Ok, I think this fix is working as intended. @jseppi are you up for reviewing this?
jseppi: @davidtheclark: sure thing!

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 62f71a1..ed569d2 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,5 +1,10 @@
 # Changelog
 
+## HEAD
+
+- **PopoverTrigger**
+  - If the trigger responds to focus but not click, and you focus the trigger *first* and *then* click, that click *closes* the popover instead of leaving it open even after you move the mouse away.
+
 ## 0.2.1
 
 - Add TabList component.
diff --git a/src/components/popover-trigger/popover-trigger.js b/src/components/popover-trigger/popover-trigger.js
index c3489f7..0de5950 100644
--- a/src/components/popover-trigger/popover-trigger.js
+++ b/src/components/popover-trigger/popover-trigger.js
@@ -125,10 +125,19 @@ export default class PopoverTrigger extends React.Component {
     );
   };
 
-  // A trigger interaction always takes precedence over hover or focus interactions.
-  // If the popover is already open by hover or focus, a click will change its
-  // priorities, so it stays open even if you mouseleave or blur
   onAnyClick = event => {
+    // If the trigger responds to focus but not click and you *first*
+    // focus *then* click, that click closes the popover instead of
+    // leaving it open even when the mouse moves away.
+    if (
+      !this.props.respondsToClick &&
+      this.state.visible &&
+      this.state.activeTriggerType === TRIGGER_FOCUS
+    ) {
+      this.hide();
+      return;
+    }
+
     if (
       !this.hoverIsBlocked &&
       (this.props.disabled || !this.props.respondsToClick)
@@ -136,6 +145,9 @@ export default class PopoverTrigger extends React.Component {
       return;
     }
 
+    // A trigger click always takes precedence over hover or focus interactions.
+    // If the popover is already open by hover or focus, a click will change its
+    // priorities, so it stays open even if you mouseleave or blur
     if (
       this.state.visible &&
       this.state.activeTriggerType === TRIGGER_CLICK &&


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 fdb5346..041193d 100644
--- a/src/components/popover-trigger/__tests__/popover-trigger-test-cases.js
+++ b/src/components/popover-trigger/__tests__/popover-trigger-test-cases.js
@@ -32,6 +32,20 @@ testCases.respondsToAllInteractions = {
   )
 };
 
+testCases.focusHoverNotClick = {
+  description: 'responds to focus and hover but not click',
+  element: (
+    <PopoverTrigger
+      content={getPopoverContent}
+      respondsToHover={true}
+      respondsToFocus={true}
+      respondsToClick={false}
+    >
+      <button className="btn">Trigger</button>
+    </PopoverTrigger>
+  )
+};
+
 testCases.callbacks = {
   description: 'callbacks when the popover opens and closes',
   component: PopoverTrigger,
diff --git a/src/components/popover-trigger/__tests__/popover-trigger.test.js b/src/components/popover-trigger/__tests__/popover-trigger.test.js
index 8ac916b..3ed1611 100644
--- a/src/components/popover-trigger/__tests__/popover-trigger.test.js
+++ b/src/components/popover-trigger/__tests__/popover-trigger.test.js
@@ -28,6 +28,35 @@ describe('PopoverTrigger', () => {
     });
   });
 
+  describe(testCases.focusHoverNotClick.description, () => {
+    beforeEach(() => {
+      testCase = testCases.focusHoverNotClick;
+      wrapper = shallow(testCase.element);
+    });
+
+    test('when focus triggers the popover, click closes it', () => {
+      expect(wrapper.find('Popover').length).toBe(0);
+      wrapper.props().onFocus();
+      wrapper.update();
+      expect(wrapper.find('Popover').length).toBe(1);
+      wrapper.props().onClick();
+      wrapper.update();
+      expect(wrapper.find('Popover').length).toBe(0);
+    });
+
+    test('when hover triggers the popover, click does not close it', () => {
+      expect(wrapper.find('Popover').length).toBe(0);
+      // onTriggerMouseEnter is triggered by a regular DOM event listener,
+      // so we're calling it directly.
+      wrapper.instance().onTriggerMouseEnter();
+      wrapper.update();
+      expect(wrapper.find('Popover').length).toBe(1);
+      wrapper.props().onClick();
+      wrapper.update();
+      expect(wrapper.find('Popover').length).toBe(1);
+    });
+  });
+
   describe(testCases.callbacks.description, () => {
     beforeEach(() => {
       testCase = testCases.callbacks;