Repository: uber-web/baseui
Pull request #1008 (2019-03-11 23:35:07)

fix(datepicker): ensures sub-popovers do not trigger close
#### Description

<img width="1570" alt="Screen Shot 2019-03-11 at 4 35 46 PM" src="https://user-images.githubusercontent.com/5317799/54164945-e77c4e80-441b-11e9-9425-d00ee72e8804.png">


#### Scope

- [x] Patch: Bug Fix
- [ ] Minor: New Feature
- [ ] Major: Breaking Change


gergelyke: can we add some test coverage to make sure regressions won't happen?

Patch:
diff --git a/src/datepicker/datepicker.js b/src/datepicker/datepicker.js
index 2d5f934c7..d4b712348 100644
--- a/src/datepicker/datepicker.js
+++ b/src/datepicker/datepicker.js
@@ -148,7 +148,25 @@ export default class Datepicker extends React.Component<
               mountNode={this.props.mountNode}
               placement={PLACEMENT.bottom}
               isOpen={this.state.isOpen}
-              onClickOutside={this.close}
+              onClickOutside={event => {
+                // Required to check that items rendered in a sub-popover does not trigger close.
+                // For example, upon selecting an option from the month dropdown it would cause
+                // this code to run since the two popovers are DOM siblings rather than parent/child.
+                // There's likely a more robust way to check this, but ignores clicks from elements
+                // that are select options for now.
+                function isOption(element) {
+                  if (!element) return false;
+                  return element.getAttribute('role') === 'option';
+                }
+                if (
+                  isOption(event.target) ||
+                  isOption(event.target.parentElement)
+                ) {
+                  return;
+                }
+
+                this.close();
+              }}
               onEsc={this.handleEsc}
               content={
                 <Calendar
diff --git a/src/popover/popover.js b/src/popover/popover.js
index dcaad4a5a..ff1c84c98 100644
--- a/src/popover/popover.js
+++ b/src/popover/popover.js
@@ -276,14 +276,14 @@ class Popover extends React.Component<PopoverPropsT, PopoverPrivateStateT> {
 
   addDomEvents() {
     if (__BROWSER__) {
-      document.addEventListener('mousedown', this.onDocumentClick);
+      document.addEventListener('click', this.onDocumentClick);
       document.addEventListener('keyup', this.onKeyPress);
     }
   }
 
   removeDomEvents() {
     if (__BROWSER__) {
-      document.removeEventListener('mousedown', this.onDocumentClick);
+      document.removeEventListener('click', this.onDocumentClick);
       document.removeEventListener('keyup', this.onKeyPress);
     }
   }
diff --git a/src/select/dropdown.js b/src/select/dropdown.js
index ae30e9e05..98ead95f0 100644
--- a/src/select/dropdown.js
+++ b/src/select/dropdown.js
@@ -134,7 +134,7 @@ export default class SelectDropdown extends React.Component<DropdownPropsT> {
                   overrides: {
                     ListItem: {
                       component: ListItem,
-                      props: listItemProps,
+                      props: {...listItemProps, role: 'option'},
                       // slightly a hacky way to handle the list item style overrides
                       // since the menu component doesn't provide a top level overrides for it
                       // $FlowFixMe


Test patch:
diff --git a/src/datepicker/__tests__/datepicker.e2e.js b/src/datepicker/__tests__/datepicker.e2e.js
index af6dee09e..e400cc2c1 100644
--- a/src/datepicker/__tests__/datepicker.e2e.js
+++ b/src/datepicker/__tests__/datepicker.e2e.js
@@ -22,7 +22,9 @@ const selectors = {
   leftArrow: '[aria-label="Previous month"]',
   rightArrow: '[aria-label="Next month"]',
   monthSelect: '[data-id="monthSelect"]',
+  monthDropdown: '[data-id="monthDropdown"]',
   yearSelect: '[data-id="yearSelect"]',
+  yearDropdown: '[data-id="yearDropdown"]',
   selectDropdown: 'ul[role="listbox"]',
 };
 
@@ -155,7 +157,7 @@ describe('Datepicker', () => {
     await page.waitFor(selectors.calendar);
     await page.waitFor(selectors.day);
     await page.click(selectors.yearSelect);
-    await page.waitFor(selectors.selectDropdown);
+    await page.waitFor(selectors.yearDropdown);
 
     await page.$$eval('ul[role="listbox"] li', items => {
       const option = items.find(item => {
@@ -165,6 +167,7 @@ describe('Datepicker', () => {
       return option;
     });
 
+    await page.waitFor(selectors.yearDropdown, {hidden: true});
     await page.waitFor(selectors.calendar);
     await page.waitFor(selectors.day5);
   });
@@ -176,7 +179,7 @@ describe('Datepicker', () => {
     await page.waitFor(selectors.calendar);
     await page.waitFor(selectors.day);
     await page.click(selectors.monthSelect);
-    await page.waitFor(selectors.selectDropdown);
+    await page.waitFor(selectors.monthDropdown);
 
     await page.$$eval('ul[role="listbox"] li', items => {
       const option = items.find(item => {
@@ -186,6 +189,7 @@ describe('Datepicker', () => {
       return option;
     });
 
+    await page.waitFor(selectors.monthDropdown, {hidden: true});
     await page.waitFor(selectors.calendar);
     await page.waitFor(selectors.day6);
   });
diff --git a/src/datepicker/__tests__/datepicker.scenario.js b/src/datepicker/__tests__/datepicker.scenario.js
index 99599ec49..0ad5abdd5 100644
--- a/src/datepicker/__tests__/datepicker.scenario.js
+++ b/src/datepicker/__tests__/datepicker.scenario.js
@@ -18,10 +18,20 @@ export const component = () => (
     highlightedDate={new Date('March 10, 2019')}
     overrides={{
       MonthSelect: {
-        props: {overrides: {Root: {props: {'data-id': 'monthSelect'}}}},
+        props: {
+          overrides: {
+            Root: {props: {'data-id': 'monthSelect'}},
+            Dropdown: {props: {'data-id': 'monthDropdown'}},
+          },
+        },
       },
       YearSelect: {
-        props: {overrides: {Root: {props: {'data-id': 'yearSelect'}}}},
+        props: {
+          overrides: {
+            Root: {props: {'data-id': 'yearSelect'}},
+            Dropdown: {props: {'data-id': 'yearDropdown'}},
+          },
+        },
       },
     }}
   />
diff --git a/src/popover/__tests__/popover.test.js b/src/popover/__tests__/popover.test.js
index 8178dc38c..2798f1cf1 100644
--- a/src/popover/__tests__/popover.test.js
+++ b/src/popover/__tests__/popover.test.js
@@ -178,7 +178,7 @@ describe('Popover', () => {
     );
 
     const calls = document.addEventListener.mock.calls;
-    expect(calls[0][0]).toBe('mousedown');
+    expect(calls[0][0]).toBe('click');
     expect(calls[1][0]).toBe('keyup');
 
     calls[1][1]({
diff --git a/src/select/__tests__/__snapshots__/dropdown.test.js.snap b/src/select/__tests__/__snapshots__/dropdown.test.js.snap
index 3e4d9f027..d29304b6e 100644
--- a/src/select/__tests__/__snapshots__/dropdown.test.js.snap
+++ b/src/select/__tests__/__snapshots__/dropdown.test.js.snap
@@ -58,7 +58,9 @@ Object {
       "overrides": Object {
         "ListItem": Object {
           "component": [MockFunction],
-          "props": Object {},
+          "props": Object {
+            "role": "option",
+          },
           "style": undefined,
         },
       },