backport elastic#1761

Shigawire · Mar 26, 2019 · d1b17ee · d1b17ee
1 parent 12c4d4c
commit d1b17ee
Show file tree

Hide file tree

Showing 5 changed files with 73 additions and 15 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,6 +1,8 @@
 ## [`master`](https://github.com/elastic/eui/tree/master)
 
-No public interface changes since `6.10.5`.
+**Bug fixes**
+
+- Fixed outside click detection inconsistencies by comparing `mouseup` and `mousedown` event targets rather than using `click` event target ([#1761](https://github.com/elastic/eui/pull/1761))
 
 ## [`6.10.5`](https://github.com/elastic/eui/tree/v6.10.5)
 

diff --git a/src/components/basic_table/__snapshots__/in_memory_table.test.js.snap b/src/components/basic_table/__snapshots__/in_memory_table.test.js.snap
@@ -314,8 +314,11 @@ exports[`EuiInMemoryTable behavior pagination 1`] = `
                         <div
                           className="euiPopover euiPopover--anchorUpRight euiPopover--withTitle"
                           id="customizablePagination"
-                          onClick={[Function]}
                           onKeyDown={[Function]}
+                          onMouseDown={[Function]}
+                          onMouseUp={[Function]}
+                          onTouchEnd={[Function]}
+                          onTouchStart={[Function]}
                         >
                           <div
                             className="euiPopover__anchor"

diff --git a/src/components/form/super_select/__snapshots__/super_select.test.js.snap b/src/components/form/super_select/__snapshots__/super_select.test.js.snap
@@ -486,8 +486,11 @@ exports[`EuiSuperSelect props more props are propogated to each option 2`] = `
     >
       <div
         className="euiPopover euiPopover--anchorDownCenter euiSuperSelect"
-        onClick={[Function]}
         onKeyDown={[Function]}
+        onMouseDown={[Function]}
+        onMouseUp={[Function]}
+        onTouchEnd={[Function]}
+        onTouchStart={[Function]}
       >
         <div
           className="euiPopover__anchor euiSuperSelect__popoverAnchor"

diff --git a/src/components/outside_click_detector/outside_click_detector.js b/src/components/outside_click_detector/outside_click_detector.js
@@ -7,6 +7,17 @@ import PropTypes from 'prop-types';
 import { htmlIdGenerator } from '../../services/accessibility';
 
 export class EuiOutsideClickDetector extends Component {
+  // We are working with the assumption that a click event is
+  // equivalent to a sequential, compound press and release of
+  // the pointing device (mouse, finger, stylus, etc.).
+  // A click event's target can be imprecise, as the value will be
+  // the closest common ancestor of the press (mousedown, touchstart)
+  // and relase (mouseup, touchend) events (often <body />) if
+  // the the target of each event differs.
+  // We need the actual event targets to make the correct decisions
+  // about user intention. So, consider the down/start and up/end
+  // items below as the deconstruction of a click event.
+
   static propTypes = {
     children: PropTypes.node.isRequired,
     onOutsideClick: PropTypes.func.isRequired,
@@ -33,6 +44,8 @@ export class EuiOutsideClickDetector extends Component {
     // stamping the id even though the event originates outside
     // this component's reified DOM tree.
     this.id = htmlIdGenerator()();
+
+    this.capturedDownIds = [];
   }
 
   onClickOutside = event => {
@@ -42,38 +55,65 @@ export class EuiOutsideClickDetector extends Component {
     } = this.props;
 
     if (isDisabled) {
+      this.capturedDownIds = [];
       return;
     }
 
-    if (event.euiGeneratedBy && event.euiGeneratedBy.includes(this.id)) {
+    if (
+      (event.euiGeneratedBy && event.euiGeneratedBy.includes(this.id))
+      || this.capturedDownIds.includes(this.id)
+    ) {
+      this.capturedDownIds = [];
       return;
     }
 
-    onOutsideClick();
-  }
+    this.capturedDownIds = [];
+    return onOutsideClick(event);
+  };
 
   componentDidMount() {
-    document.addEventListener('click', this.onClickOutside);
+    document.addEventListener('mouseup', this.onClickOutside);
+    document.addEventListener('touchend', this.onClickOutside);
   }
 
   componentWillUnmount() {
-    document.removeEventListener('click', this.onClickOutside);
+    document.removeEventListener('mouseup', this.onClickOutside);
+    document.removeEventListener('touchend', this.onClickOutside);
   }
 
-  onChildClick = event => {
+  onChildClick = (event, cb) => {
     // to support nested click detectors, build an array
     // of detector ids that have been encountered
     if (event.nativeEvent.hasOwnProperty('euiGeneratedBy')) {
       event.nativeEvent.euiGeneratedBy.push(this.id);
     } else {
       event.nativeEvent.euiGeneratedBy = [this.id];
     }
-    if (this.props.onClick) this.props.onClick(event);
+    if (cb) cb(event);
+  }
+
+  onChildMouseDown = event => {
+    this.onChildClick(event, e => {
+      this.capturedDownIds = e.nativeEvent.euiGeneratedBy;
+      if (this.props.onMouseDown) this.props.onMouseDown(e);
+      if (this.props.onTouchStart) this.props.onTouchStart(e);
+    });
+
+  }
+
+  onChildMouseUp = event => {
+    this.onChildClick(event, e => {
+      if (this.props.onMouseUp) this.props.onMouseUp(e);
+      if (this.props.onTouchEnd) this.props.onTouchEnd(e);
+    });
   }
 
   render() {
     const props = ({ ...this.props.children.props, ...{
-      onClick: this.onChildClick,
+      onMouseDown: this.onChildMouseDown,
+      onTouchStart: this.onChildMouseDown,
+      onMouseUp: this.onChildMouseUp,
+      onTouchEnd: this.onChildMouseUp,
     } });
 
     const child = Children.only(this.props.children);

diff --git a/src/components/outside_click_detector/outside_click_detector.test.js b/src/components/outside_click_detector/outside_click_detector.test.js
@@ -24,14 +24,23 @@ describe('EuiOutsideClickDetector', () => {
       // enzyme doesn't mount the components into the global jsdom `document`
       // but that's where the click detector listener is,
       // pass the top-level mounted component's click event on to document
-      const triggerDocumentClick = e => {
-        const event = new Event('click');
+      const triggerDocumentMouseDown = e => {
+        const event = new Event('mousedown');
+        event.euiGeneratedBy = e.nativeEvent.euiGeneratedBy;
+        document.dispatchEvent(event);
+      };
+
+      const triggerDocumentMouseUp = e => {
+        const event = new Event('mouseup');
         event.euiGeneratedBy = e.nativeEvent.euiGeneratedBy;
         document.dispatchEvent(event);
       };
 
       const component = mount(
-        <div onClick={triggerDocumentClick}>
+        <div
+          onMouseDown={triggerDocumentMouseDown}
+          onMouseUp={triggerDocumentMouseUp}
+        >
           <div>
             <EuiOutsideClickDetector onOutsideClick={parentDetector}>
               <div>
@@ -48,7 +57,8 @@ describe('EuiOutsideClickDetector', () => {
         </div>
       );
 
-      component.find('[data-test-subj="target"]').simulate('click');
+      component.find('[data-test-subj="target"]').simulate('mousedown');
+      component.find('[data-test-subj="target"]').simulate('mouseup');
 
       expect(unrelatedDetector).toHaveBeenCalledTimes(1);
       expect(parentDetector).toHaveBeenCalledTimes(0);