fix: Popovers in Explore not attached to the fields they are triggered by (#19139)

* fix: Popovers in Explore not attached to the fields they are triggered by

* fix

* PR comment

* remove unused import

superset-frontend/src/components/Popover/index.tsx

@@ -18,6 +18,9 @@
  */
 import { Popover } from 'antd';
 
+export { PopoverProps } from 'antd/lib/popover';
+export { TooltipPlacement } from 'antd/lib/tooltip';
+
 // Eventually Popover can be wrapped and customized in this file
 // for now we're just redirecting
 export default Popover;

superset-frontend/src/explore/components/controls/AnnotationLayerControl/index.jsx

@@ -22,11 +22,11 @@ import { List } from 'src/components';
 import { connect } from 'react-redux';
 import { t, withTheme } from '@superset-ui/core';
 import { InfoTooltipWithTrigger } from '@superset-ui/chart-controls';
-import Popover from 'src/components/Popover';
 import AsyncEsmComponent from 'src/components/AsyncEsmComponent';
 import { getChartKey } from 'src/explore/exploreUtils';
 import { runAnnotationQuery } from 'src/components/Chart/chartAction';
 import CustomListItem from 'src/explore/components/controls/CustomListItem';
+import ControlPopover from '../ControlPopover/ControlPopover';
 
 const AnnotationLayer = AsyncEsmComponent(
   () => import('./AnnotationLayer'),
@@ -167,10 +167,9 @@ class AnnotationLayerControl extends React.PureComponent {
     const addedAnnotation = this.props.value[addedAnnotationIndex];
 
     const annotations = this.props.value.map((anno, i) => (
-      <Popover
+      <ControlPopover
         key={i}
         trigger="click"
-        placement="right"
         title={t('Edit annotation layer')}
         css={theme => ({
           '&:hover': {
@@ -190,7 +189,7 @@ class AnnotationLayerControl extends React.PureComponent {
           <span>{anno.name}</span>
           <span style={{ float: 'right' }}>{this.renderInfo(anno)}</span>
         </CustomListItem>
-      </Popover>
+      </ControlPopover>
     ));
 
     const addLayerPopoverKey = 'add';
@@ -198,9 +197,8 @@ class AnnotationLayerControl extends React.PureComponent {
       <div>
         <List bordered css={theme => ({ borderRadius: theme.gridUnit })}>
           {annotations}
-          <Popover
+          <ControlPopover
             trigger="click"
-            placement="right"
             content={this.renderPopover(addLayerPopoverKey, addedAnnotation)}
             title={t('Add annotation layer')}
             visible={this.state.popoverVisible[addLayerPopoverKey]}
@@ -216,7 +214,7 @@ class AnnotationLayerControl extends React.PureComponent {
               />{' '}
               &nbsp; {t('Add annotation layer')}
             </CustomListItem>
-          </Popover>
+          </ControlPopover>
         </List>
       </div>
     );

superset-frontend/src/explore/components/controls/ControlPopover/ControlPopover.test.tsx

@@ -0,0 +1,126 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React from 'react';
+import { render, screen } from 'spec/helpers/testing-library';
+import userEvent from '@testing-library/user-event';
+import { waitFor } from '@testing-library/react';
+
+import ControlPopover, { PopoverProps } from './ControlPopover';
+
+const createProps = (): Partial<PopoverProps> => ({
+  trigger: 'click',
+  title: 'Control Popover Test',
+  content: <span data-test="control-popover-content">Information</span>,
+});
+
+const setupTest = (props: Partial<PopoverProps> = createProps()) => {
+  const setStateMock = jest.fn();
+  jest
+    .spyOn(React, 'useState')
+    .mockImplementation(((state: any) => [
+      state,
+      state === 'right' ? setStateMock : jest.fn(),
+    ]) as any);
+
+  const { container } = render(
+    <div data-test="outer-container">
+      <div id="controlSections">
+        <ControlPopover {...props}>
+          <span data-test="control-popover">Click me</span>
+        </ControlPopover>
+      </div>
+    </div>,
+  );
+
+  return {
+    props,
+    container,
+    setStateMock,
+  };
+};
+
+afterEach(() => {
+  jest.restoreAllMocks();
+});
+
+test('Should render', () => {
+  setupTest();
+  expect(screen.getByTestId('control-popover')).toBeInTheDocument();
+  userEvent.click(screen.getByTestId('control-popover'));
+  expect(screen.getByText('Control Popover Test')).toBeInTheDocument();
+  expect(screen.getByTestId('control-popover-content')).toBeInTheDocument();
+});
+
+test('Should lock the vertical scroll when the popup is visible', () => {
+  setupTest();
+  expect(screen.getByTestId('control-popover')).toBeInTheDocument();
+  expect(screen.getByTestId('outer-container')).not.toHaveStyle(
+    'overflowY: hidden',
+  );
+  userEvent.click(screen.getByTestId('control-popover'));
+  expect(screen.getByTestId('outer-container')).toHaveStyle(
+    'overflowY: hidden',
+  );
+  userEvent.click(document.body);
+  expect(screen.getByTestId('outer-container')).not.toHaveStyle(
+    'overflowY: hidden',
+  );
+});
+
+test('Should place popup at the top', async () => {
+  const { setStateMock } = setupTest({
+    ...createProps(),
+    getVisibilityRatio: () => 0.2,
+  });
+
+  expect(screen.getByTestId('control-popover')).toBeInTheDocument();
+  userEvent.click(screen.getByTestId('control-popover'));
+
+  await waitFor(() => {
+    expect(setStateMock).toHaveBeenCalledWith('rightTop');
+  });
+});
+
+test('Should place popup at the center', async () => {
+  const { setStateMock } = setupTest({
+    ...createProps(),
+    getVisibilityRatio: () => 0.5,
+  });
+
+  expect(screen.getByTestId('control-popover')).toBeInTheDocument();
+  userEvent.click(screen.getByTestId('control-popover'));
+
+  await waitFor(() => {
+    expect(setStateMock).toHaveBeenCalledWith('right');
+  });
+});
+
+test('Should place popup at the bottom', async () => {
+  const { setStateMock } = setupTest({
+    ...createProps(),
+    getVisibilityRatio: () => 0.7,
+  });
+
+  expect(screen.getByTestId('control-popover')).toBeInTheDocument();
+  userEvent.click(screen.getByTestId('control-popover'));
+
+  await waitFor(() => {
+    expect(setStateMock).toHaveBeenCalledWith('rightBottom');
+  });
+});

superset-frontend/src/explore/components/controls/ControlPopover/ControlPopover.tsx

@@ -0,0 +1,118 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React, { useCallback, useRef, useEffect } from 'react';
+
+import Popover, {
+  PopoverProps as BasePopoverProps,
+  TooltipPlacement,
+} from 'src/components/Popover';
+
+const sectionContainerId = 'controlSections';
+const getSectionContainerElement = () =>
+  document.getElementById(sectionContainerId)?.parentElement;
+
+const getElementYVisibilityRatioOnContainer = (node: HTMLElement) => {
+  const containerHeight = window?.innerHeight;
+  const nodePositionInViewport = node.getBoundingClientRect()?.top;
+  if (!containerHeight || !nodePositionInViewport) {
+    return 0;
+  }
+
+  return nodePositionInViewport / containerHeight;
+};
+
+export type PopoverProps = BasePopoverProps & {
+  getVisibilityRatio?: typeof getElementYVisibilityRatioOnContainer;
+};
+
+const ControlPopover: React.FC<PopoverProps> = ({
+  getPopupContainer,
+  getVisibilityRatio = getElementYVisibilityRatioOnContainer,
+  ...props
+}) => {
+  const triggerElementRef = useRef<HTMLElement>();
+  const [placement, setPlacement] = React.useState<TooltipPlacement>('right');
+
+  const calculatePlacement = useCallback(() => {
+    const visibilityRatio = getVisibilityRatio(triggerElementRef.current!);
+
+    if (visibilityRatio < 0.35) {
+      setPlacement('rightTop');
+    } else if (visibilityRatio > 0.65) {
+      setPlacement('rightBottom');
+    } else {
+      setPlacement('right');
+    }
+  }, [getVisibilityRatio]);
+
+  const changeContainerScrollStatus = useCallback(
+    visible => {
+      if (triggerElementRef.current && visible) {
+        calculatePlacement();
+      }
+
+      const element = getSectionContainerElement();
+      if (element) {
+        element.style.overflowY = visible ? 'hidden' : 'auto';
+      }
+    },
+    [calculatePlacement],
+  );
+
+  const handleGetPopupContainer = useCallback(
+    (triggerNode: HTMLElement) => {
+      triggerElementRef.current = triggerNode;
+      setTimeout(() => {
+        calculatePlacement();
+      }, 0);
+
+      return getPopupContainer?.(triggerNode) || document.body;
+    },
+    [calculatePlacement, getPopupContainer],
+  );
+
+  const handleOnVisibleChange = useCallback(
+    (visible: boolean) => {
+      if (props.visible === undefined) {
+        changeContainerScrollStatus(visible);
+      }
+
+      props.onVisibleChange?.(visible);
+    },
+    [props, changeContainerScrollStatus],
+  );
+
+  useEffect(() => {
+    if (props.visible !== undefined) {
+      changeContainerScrollStatus(props.visible);
+    }
+  }, [props.visible, changeContainerScrollStatus]);
+
+  return (
+    <Popover
+      {...props}
+      arrowPointAtCenter
+      placement={placement}
+      onVisibleChange={handleOnVisibleChange}
+      getPopupContainer={handleGetPopupContainer}
+    />
+  );
+};
+
+export default ControlPopover;

superset-frontend/src/explore/components/controls/DateFilterControl/DateFilterLabel.tsx

@@ -31,7 +31,6 @@ import { getClientErrorObject } from 'src/utils/getClientErrorObject';
 import Button from 'src/components/Button';
 import ControlHeader from 'src/explore/components/ControlHeader';
 import Label, { Type } from 'src/components/Label';
-import Popover from 'src/components/Popover';
 import { Divider } from 'src/components';
 import Icons from 'src/components/Icons';
 import Select from 'src/components/Select/Select';
@@ -42,6 +41,7 @@ import { SLOW_DEBOUNCE } from 'src/constants';
 import { testWithId } from 'src/utils/testUtils';
 import { noOp } from 'src/utils/common';
 import { FrameType } from './types';
+import ControlPopover from '../ControlPopover/ControlPopover';
 
 import {
   CommonFrame,
@@ -86,7 +86,7 @@ const fetchTimeRange = async (timeRange: string) => {
   }
 };
 
-const StyledPopover = styled(Popover)``;
+const StyledPopover = styled(ControlPopover)``;
 const StyledRangeType = styled(Select)`
   width: 272px;
 `;

superset-frontend/src/explore/components/controls/DndColumnSelectControl/ColumnSelectPopoverTrigger.tsx

@@ -23,10 +23,10 @@ import {
   isAdhocColumn,
   isColumnMeta,
 } from '@superset-ui/chart-controls';
-import Popover from 'src/components/Popover';
 import { ExplorePopoverContent } from 'src/explore/components/ExploreContentPopover';
 import ColumnSelectPopover from './ColumnSelectPopover';
 import { DndColumnSelectPopoverTitle } from './DndColumnSelectPopoverTitle';
+import ControlPopover from '../ControlPopover/ControlPopover';
 
 interface ColumnSelectPopoverTriggerProps {
   columns: ColumnMeta[];
@@ -137,8 +137,7 @@ const ColumnSelectPopoverTrigger = ({
   );
 
   return (
-    <Popover
-      placement="right"
+    <ControlPopover
       trigger="click"
       content={overlayContent}
       defaultVisible={visible}
@@ -148,7 +147,7 @@ const ColumnSelectPopoverTrigger = ({
       destroyTooltipOnHide
     >
       {children}
-    </Popover>
+    </ControlPopover>
   );
 };
 

superset-frontend/src/explore/components/controls/FilterBoxItemControl/FilterBoxItemControl.test.jsx

@@ -21,10 +21,10 @@ import React from 'react';
 import sinon from 'sinon';
 import { shallow } from 'enzyme';
 
-import Popover from 'src/components/Popover';
 import FilterBoxItemControl from 'src/explore/components/controls/FilterBoxItemControl';
 import FormRow from 'src/components/FormRow';
 import datasources from 'spec/fixtures/mockDatasource';
+import ControlPopover from '../ControlPopover/ControlPopover';
 
 const defaultProps = {
   label: 'some label',
@@ -46,7 +46,7 @@ describe('FilterBoxItemControl', () => {
   });
 
   it('renders a Popover', () => {
-    expect(wrapper.find(Popover)).toExist();
+    expect(wrapper.find(ControlPopover)).toExist();
   });
 
   it('renderForms does the job', () => {

superset-frontend/src/explore/components/controls/FilterBoxItemControl/FilterBoxItemControl.test.tsx

@@ -38,14 +38,14 @@ const createProps = () => ({
   onChange: jest.fn(),
 });
 
-test('Shoud render', () => {
+test('Should render', () => {
   const props = createProps();
   render(<FilterBoxItemControl {...props} />);
   expect(screen.getByTestId('FilterBoxItemControl')).toBeInTheDocument();
   expect(screen.getByRole('button')).toBeInTheDocument();
 });
 
-test('Shoud open modal', () => {
+test('Should open modal', () => {
   const props = createProps();
   render(<FilterBoxItemControl {...props} />);
   userEvent.click(screen.getByRole('button'));

superset-frontend/src/explore/components/controls/FilterBoxItemControl/index.jsx

@@ -21,12 +21,12 @@ import PropTypes from 'prop-types';
 import { t } from '@superset-ui/core';
 import { InfoTooltipWithTrigger } from '@superset-ui/chart-controls';
 
-import Popover from 'src/components/Popover';
 import FormRow from 'src/components/FormRow';
 import { Select } from 'src/components';
 import CheckboxControl from 'src/explore/components/controls/CheckboxControl';
 import TextControl from 'src/explore/components/controls/TextControl';
 import { FILTER_CONFIG_ATTRIBUTES } from 'src/explore/constants';
+import ControlPopover from '../ControlPopover/ControlPopover';
 
 const INTEGRAL_TYPES = new Set([
   'TINYINT',
@@ -275,9 +275,8 @@ export default class FilterBoxItemControl extends React.Component {
     return (
       <span data-test="FilterBoxItemControl">
         {this.textSummary()}{' '}
-        <Popover
+        <ControlPopover
           trigger="click"
-          placement="right"
           content={this.renderPopover()}
           title={t('Filter configuration')}
         >
@@ -286,7 +285,7 @@ export default class FilterBoxItemControl extends React.Component {
             className="text-primary"
             label="edit-ts-column"
           />
-        </Popover>
+        </ControlPopover>
       </span>
     );
   }

superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterPopoverTrigger/AdhocFilterPopoverTrigger.test.tsx

@@ -73,7 +73,8 @@ test('should be visible when controlled', async () => {
       Click
     </AdhocFilterPopoverTrigger>,
   );
-  expect(screen.getByRole('tooltip')).toBeInTheDocument();
+
+  expect(await screen.findByRole('tooltip')).toBeInTheDocument();
 });
 
 test('should NOT be visible when controlled', () => {

superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterPopoverTrigger/index.tsx

@@ -17,12 +17,12 @@
  * under the License.
  */
 import React from 'react';
-import Popover from 'src/components/Popover';
 import { OptionSortType } from 'src/explore/types';
 import AdhocFilterEditPopover from 'src/explore/components/controls/FilterControl/AdhocFilterEditPopover';
 import AdhocFilter from 'src/explore/components/controls/FilterControl/AdhocFilter';
 import { ExplorePopoverContent } from 'src/explore/components/ExploreContentPopover';
 import { Operators } from 'src/explore/constants';
+import ControlPopover from '../../ControlPopover/ControlPopover';
 
 interface AdhocFilterPopoverTriggerProps {
   sections?: string[];
@@ -101,8 +101,7 @@ class AdhocFilterPopoverTrigger extends React.PureComponent<
     );
 
     return (
-      <Popover
-        placement="right"
+      <ControlPopover
         trigger="click"
         content={overlayContent}
         defaultVisible={visible}
@@ -111,7 +110,7 @@ class AdhocFilterPopoverTrigger extends React.PureComponent<
         destroyTooltipOnHide
       >
         {this.props.children}
-      </Popover>
+      </ControlPopover>
     );
   }
 }

superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricOption.test.jsx

@@ -21,10 +21,10 @@ import React from 'react';
 import sinon from 'sinon';
 import { shallow } from 'enzyme';
 
-import Popover from 'src/components/Popover';
 import { AGGREGATES } from 'src/explore/constants';
 import AdhocMetricOption from 'src/explore/components/controls/MetricControl/AdhocMetricOption';
 import AdhocMetric from 'src/explore/components/controls/MetricControl/AdhocMetric';
+import ControlPopover from '../ControlPopover/ControlPopover';
 
 const columns = [
   { type: 'VARCHAR(255)', column_name: 'source' },
@@ -59,7 +59,7 @@ function setup(overrides) {
 describe('AdhocMetricOption', () => {
   it('renders an overlay trigger wrapper for the label', () => {
     const { wrapper } = setup();
-    expect(wrapper.find(Popover)).toExist();
+    expect(wrapper.find(ControlPopover)).toExist();
     expect(wrapper.find('OptionControlLabel')).toExist();
   });
 

superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricPopoverTrigger.tsx

@@ -18,7 +18,6 @@
  */
 import React, { ReactNode } from 'react';
 import { Datasource, Metric } from '@superset-ui/core';
-import Popover from 'src/components/Popover';
 import AdhocMetricEditPopoverTitle from 'src/explore/components/controls/MetricControl/AdhocMetricEditPopoverTitle';
 import { ExplorePopoverContent } from 'src/explore/components/ExploreContentPopover';
 import AdhocMetricEditPopover, {
@@ -26,6 +25,7 @@ import AdhocMetricEditPopover, {
 } from './AdhocMetricEditPopover';
 import AdhocMetric from './AdhocMetric';
 import { savedMetricType } from './types';
+import ControlPopover from '../ControlPopover/ControlPopover';
 
 export type AdhocMetricPopoverTriggerProps = {
   adhocMetric: AdhocMetric;
@@ -223,7 +223,7 @@ class AdhocMetricPopoverTrigger extends React.PureComponent<
     );
 
     return (
-      <Popover
+      <ControlPopover
         placement="right"
         trigger="click"
         content={overlayContent}
@@ -234,7 +234,7 @@ class AdhocMetricPopoverTrigger extends React.PureComponent<
         destroyTooltipOnHide
       >
         {this.props.children}
-      </Popover>
+      </ControlPopover>
     );
   }
 }

superset-frontend/src/explore/components/controls/TimeSeriesColumnControl/index.jsx

@@ -20,12 +20,12 @@ import React from 'react';
 import PropTypes from 'prop-types';
 import { Input } from 'src/components/Input';
 import Button from 'src/components/Button';
-import Popover from 'src/components/Popover';
 import { Select, Row, Col } from 'src/components';
 import { t, styled } from '@superset-ui/core';
 import { InfoTooltipWithTrigger } from '@superset-ui/chart-controls';
 import BoundsControl from '../BoundsControl';
 import CheckboxControl from '../CheckboxControl';
+import ControlPopover from '../ControlPopover/ControlPopover';
 
 const propTypes = {
   label: PropTypes.string,
@@ -353,9 +353,8 @@ export default class TimeSeriesColumnControl extends React.Component {
     return (
       <span>
         {this.textSummary()}{' '}
-        <Popover
+        <ControlPopover
           trigger="click"
-          placement="right"
           content={this.renderPopover()}
           title="Column Configuration"
           visible={this.state.popoverVisible}
@@ -366,7 +365,7 @@ export default class TimeSeriesColumnControl extends React.Component {
             className="text-primary"
             label="edit-ts-column"
           />
-        </Popover>
+        </ControlPopover>
       </span>
     );
   }