fix: Explore popovers issues (#11428)

* Fix spaces and comas not working in filter popover

* Fix popup not opening automatically

* Add e2e test

* Remove only from test

* Remove redundant test, add checking label content

* Add comments to e2e test

* Fix unit test

* Use destructuring

* Always open popup for functions and saved metrics, too

* Fix popover for adhoc metrics too

* Small refactor to consistency

* Refactor for consistency

* Remove redundant functions

* Test fix

Co-authored-by: Jesse Yang <jesse.yang@airbnb.com>

superset-frontend/src/explore/components/AdhocFilterOption.jsx

@@ -18,10 +18,10 @@
  */
 import React from 'react';
 import PropTypes from 'prop-types';
-import Popover from 'src/common/components/Popover';
-import { t, withTheme } from '@superset-ui/core';
+import { t } from '@superset-ui/core';
 import { InfoTooltipWithTrigger } from '@superset-ui/chart-controls';
 
+import Popover from 'src/common/components/Popover';
 import Label from 'src/components/Label';
 import AdhocFilterEditPopover from './AdhocFilterEditPopover';
 import AdhocFilter from '../AdhocFilter';
@@ -44,57 +44,63 @@ const propTypes = {
 class AdhocFilterOption extends React.PureComponent {
   constructor(props) {
     super(props);
-    this.closeFilterEditOverlay = this.closeFilterEditOverlay.bind(this);
     this.onPopoverResize = this.onPopoverResize.bind(this);
-    this.onOverlayEntered = this.onOverlayEntered.bind(this);
-    this.onOverlayExited = this.onOverlayExited.bind(this);
-    this.handleVisibleChange = this.handleVisibleChange.bind(this);
-    this.state = { overlayShown: false };
+    this.closePopover = this.closePopover.bind(this);
+    this.togglePopover = this.togglePopover.bind(this);
+    this.state = {
+      // automatically open the popover the the metric is new
+      popoverVisible: !!props.adhocFilter.isNew,
+    };
+  }
+
+  componentDidMount() {
+    const { adhocFilter } = this.props;
+    // isNew is used to auto-open the popup. Once popup is opened, it's not
+    // considered new anymore.
+    // put behind setTimeout so in case consequetive re-renderings are triggered
+    // for some reason, the popup can still show up.
+    setTimeout(() => {
+      adhocFilter.isNew = false;
+    });
   }
 
   onPopoverResize() {
     this.forceUpdate();
   }
 
-  onOverlayEntered() {
-    // isNew is used to indicate whether to automatically open the overlay
-    // once the overlay has been opened, the metric/filter will never be
-    // considered new again.
-    this.props.adhocFilter.isNew = false;
-    this.setState({ overlayShown: true });
+  closePopover() {
+    this.setState({ popoverVisible: false });
   }
 
-  onOverlayExited() {
-    this.setState({ overlayShown: false });
-  }
-
-  closeFilterEditOverlay() {
-    this.setState({ overlayShown: false });
-  }
-
-  handleVisibleChange(visible) {
-    if (visible) {
-      this.onOverlayEntered();
-    } else {
-      this.onOverlayExited();
-    }
+  togglePopover(visible) {
+    this.setState(({ popoverVisible }) => {
+      return {
+        popoverVisible: visible === undefined ? !popoverVisible : visible,
+      };
+    });
   }
 
   render() {
     const { adhocFilter } = this.props;
-    const content = (
+    const overlayContent = (
       <AdhocFilterEditPopover
-        onResize={this.onPopoverResize}
         adhocFilter={adhocFilter}
-        onChange={this.props.onFilterEdit}
-        onClose={this.closeFilterEditOverlay}
         options={this.props.options}
         datasource={this.props.datasource}
         partitionColumn={this.props.partitionColumn}
+        onResize={this.onPopoverResize}
+        onClose={this.closePopover}
+        onChange={this.props.onFilterEdit}
       />
     );
+
     return (
-      <div role="button" tabIndex={0} onMouseDown={e => e.stopPropagation()}>
+      <div
+        role="button"
+        tabIndex={0}
+        onMouseDown={e => e.stopPropagation()}
+        onKeyDown={e => e.stopPropagation()}
+      >
         {adhocFilter.isExtra && (
           <InfoTooltipWithTrigger
             icon="exclamation-triangle"
@@ -109,19 +115,14 @@ class AdhocFilterOption extends React.PureComponent {
         <Popover
           placement="right"
           trigger="click"
-          disabled
-          content={content}
+          content={overlayContent}
           defaultVisible={adhocFilter.isNew}
-          visible={this.state.overlayShown}
-          onVisibleChange={this.handleVisibleChange}
+          visible={this.state.popoverVisible}
+          onVisibleChange={this.togglePopover}
         >
           <Label className="option-label adhoc-option adhoc-filter-option">
             {adhocFilter.getDefaultLabel()}
-            <i
-              className={`fa fa-caret-${
-                this.state.overlayShown ? 'left' : 'right'
-              } adhoc-label-arrow`}
-            />
+            <i className="fa fa-caret-right adhoc-label-arrow" />
           </Label>
         </Popover>
       </div>
@@ -129,6 +130,6 @@ class AdhocFilterOption extends React.PureComponent {
   }
 }
 
-export default withTheme(AdhocFilterOption);
+export default AdhocFilterOption;
 
 AdhocFilterOption.propTypes = propTypes;

superset-frontend/src/explore/components/AdhocMetricOption.jsx

@@ -18,7 +18,6 @@
  */
 import React from 'react';
 import PropTypes from 'prop-types';
-import { withTheme } from '@superset-ui/core';
 
 import Popover from 'src/common/components/Popover';
 import Label from 'src/components/Label';
@@ -38,13 +37,12 @@ const propTypes = {
 class AdhocMetricOption extends React.PureComponent {
   constructor(props) {
     super(props);
-    this.closeMetricEditOverlay = this.closeMetricEditOverlay.bind(this);
-    this.onOverlayEntered = this.onOverlayEntered.bind(this);
     this.onPopoverResize = this.onPopoverResize.bind(this);
-    this.handleVisibleChange = this.handleVisibleChange.bind(this);
     this.onLabelChange = this.onLabelChange.bind(this);
+    this.closePopover = this.closePopover.bind(this);
+    this.togglePopover = this.togglePopover.bind(this);
     this.state = {
-      overlayShown: false,
+      popoverVisible: undefined,
       title: {
         label: props.adhocMetric.label,
         hasCustomLabel: props.adhocMetric.hasCustomLabel,
@@ -52,12 +50,23 @@ class AdhocMetricOption extends React.PureComponent {
     };
   }
 
+  componentDidMount() {
+    const { adhocMetric } = this.props;
+    // isNew is used to auto-open the popup. Once popup is opened, it's not
+    // considered new anymore.
+    // put behind setTimeout so in case consequetive re-renderings are triggered
+    // for some reason, the popup can still show up.
+    setTimeout(() => {
+      adhocMetric.isNew = false;
+    });
+  }
+
   onLabelChange(e) {
     const label = e.target.value;
     this.setState({
       title: {
-        label,
-        hasCustomLabel: true,
+        label: label || this.props.adhocMetric.label,
+        hasCustomLabel: !!label,
       },
     });
   }
@@ -66,43 +75,30 @@ class AdhocMetricOption extends React.PureComponent {
     this.forceUpdate();
   }
 
-  onOverlayEntered() {
-    // isNew is used to indicate whether to automatically open the overlay
-    // once the overlay has been opened, the metric/filter will never be
-    // considered new again.
-    this.props.adhocMetric.isNew = false;
-    this.setState({
-      overlayShown: true,
-      title: {
-        label: this.props.adhocMetric.label,
-        hasCustomLabel: this.props.adhocMetric.hasCustomLabel,
-      },
+  closePopover() {
+    this.setState({ popoverVisible: false });
+  }
+
+  togglePopover(visible) {
+    this.setState(({ popoverVisible }) => {
+      return {
+        popoverVisible: visible === undefined ? !popoverVisible : visible,
+      };
     });
   }
 
-  closeMetricEditOverlay() {
-    this.setState({ overlayShown: false });
-  }
-
-  handleVisibleChange(visible) {
-    if (visible) {
-      this.onOverlayEntered();
-    } else {
-      this.closeMetricEditOverlay();
-    }
-  }
-
   render() {
     const { adhocMetric } = this.props;
+
     const overlayContent = (
       <AdhocMetricEditPopover
-        onResize={this.onPopoverResize}
         adhocMetric={adhocMetric}
         title={this.state.title}
-        onChange={this.props.onMetricEdit}
-        onClose={this.closeMetricEditOverlay}
         columns={this.props.columns}
         datasourceType={this.props.datasourceType}
+        onResize={this.onPopoverResize}
+        onClose={this.closePopover}
+        onChange={this.props.onMetricEdit}
       />
     );
 
@@ -129,17 +125,13 @@ class AdhocMetricOption extends React.PureComponent {
           disabled
           content={overlayContent}
           defaultVisible={adhocMetric.isNew}
-          onVisibleChange={this.handleVisibleChange}
-          visible={this.state.overlayShown}
+          visible={this.state.popoverVisible}
+          onVisibleChange={this.togglePopover}
           title={popoverTitle}
         >
           <Label className="option-label adhoc-option" data-test="option-label">
             {adhocMetric.label}
-            <i
-              className={`fa fa-caret-${
-                this.state.overlayShown ? 'left' : 'right'
-              } adhoc-label-arrow`}
-            />
+            <i className="fa fa-caret-right adhoc-label-arrow" />
           </Label>
         </Popover>
       </div>
@@ -147,6 +139,6 @@ class AdhocMetricOption extends React.PureComponent {
   }
 }
 
-export default withTheme(AdhocMetricOption);
+export default AdhocMetricOption;
 
 AdhocMetricOption.propTypes = propTypes;

superset-frontend/src/explore/components/controls/AdhocFilterControl.jsx

@@ -180,9 +180,10 @@ export default class AdhocFilterControl extends React.Component {
             operator: OPERATORS['>'],
             comparator: 0,
             clause: CLAUSES.HAVING,
+            isNew: true,
           });
         }
-        // has a custom label
+        // has a custom label, meaning it's custom column
         if (option.label) {
           return new AdhocFilter({
             expressionType:
@@ -196,6 +197,7 @@ export default class AdhocFilterControl extends React.Component {
             operator: OPERATORS['>'],
             comparator: 0,
             clause: CLAUSES.HAVING,
+            isNew: true,
           });
         }
         // add a new filter item
@@ -262,7 +264,7 @@ export default class AdhocFilterControl extends React.Component {
 
   render() {
     return (
-      <div className="metrics-select">
+      <div className="metrics-select" data-test="adhoc-filter-control">
         <ControlHeader {...this.props} />
         <OnPasteSelect
           isMulti