feat: Explore popovers should close on escape (#19902)

* feat: Explore popovers should close on escape

* review comments

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

@@ -17,7 +17,7 @@
  * under the License.
  */
 import React from 'react';
-import { render, screen } from 'spec/helpers/testing-library';
+import { render, screen, fireEvent } from 'spec/helpers/testing-library';
 import userEvent from '@testing-library/user-event';
 import { waitFor } from '@testing-library/react';
 
@@ -29,6 +29,16 @@ const createProps = (): Partial<PopoverProps> => ({
   content: <span data-test="control-popover-content">Information</span>,
 });
 
+const TestComponent: React.FC<PopoverProps> = props => (
+  <div id="controlSections">
+    <div data-test="outer-container">
+      <ControlPopover {...props}>
+        <span data-test="control-popover">Click me</span>
+      </ControlPopover>
+    </div>
+  </div>
+);
+
 const setupTest = (props: Partial<PopoverProps> = createProps()) => {
   const setStateMock = jest.fn();
   jest
@@ -38,19 +48,12 @@ const setupTest = (props: Partial<PopoverProps> = createProps()) => {
       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>,
-  );
+  const { container, rerender } = render(<TestComponent {...props} />);
 
   return {
     props,
     container,
+    rerender,
     setStateMock,
   };
 };
@@ -67,7 +70,7 @@ test('Should render', () => {
   expect(screen.getByTestId('control-popover-content')).toBeInTheDocument();
 });
 
-test('Should lock the vertical scroll when the popup is visible', () => {
+test('Should lock the vertical scroll when the popover is visible', () => {
   setupTest();
   expect(screen.getByTestId('control-popover')).toBeInTheDocument();
   expect(screen.getByTestId('outer-container')).not.toHaveStyle(
@@ -83,7 +86,7 @@ test('Should lock the vertical scroll when the popup is visible', () => {
   );
 });
 
-test('Should place popup at the top', async () => {
+test('Should place popover at the top', async () => {
   const { setStateMock } = setupTest({
     ...createProps(),
     getVisibilityRatio: () => 0.2,
@@ -97,7 +100,7 @@ test('Should place popup at the top', async () => {
   });
 });
 
-test('Should place popup at the center', async () => {
+test('Should place popover at the center', async () => {
   const { setStateMock } = setupTest({
     ...createProps(),
     getVisibilityRatio: () => 0.5,
@@ -111,7 +114,7 @@ test('Should place popup at the center', async () => {
   });
 });
 
-test('Should place popup at the bottom', async () => {
+test('Should place popover at the bottom', async () => {
   const { setStateMock } = setupTest({
     ...createProps(),
     getVisibilityRatio: () => 0.7,
@@ -124,3 +127,55 @@ test('Should place popup at the bottom', async () => {
     expect(setStateMock).toHaveBeenCalledWith('rightBottom');
   });
 });
+
+test('Should close popover on escape press', async () => {
+  setupTest({
+    ...createProps(),
+    destroyTooltipOnHide: true,
+  });
+
+  expect(screen.getByTestId('control-popover')).toBeInTheDocument();
+  expect(screen.queryByText('Control Popover Test')).not.toBeInTheDocument();
+  userEvent.click(screen.getByTestId('control-popover'));
+  expect(await screen.findByText('Control Popover Test')).toBeInTheDocument();
+
+  // Ensure that pressing any other key than escape does nothing
+  fireEvent.keyDown(screen.getByTestId('control-popover'), {
+    key: 'Enter',
+    code: 13,
+    charCode: 13,
+  });
+  expect(await screen.findByText('Control Popover Test')).toBeInTheDocument();
+
+  // Escape should close the popover
+  fireEvent.keyDown(screen.getByTestId('control-popover'), {
+    key: 'Escape',
+    code: 27,
+    charCode: 0,
+  });
+
+  await waitFor(() => {
+    expect(screen.queryByText('Control Popover Test')).not.toBeInTheDocument();
+  });
+});
+
+test('Controlled mode', async () => {
+  const baseProps = {
+    ...createProps(),
+    destroyTooltipOnHide: true,
+    visible: false,
+  };
+
+  const { rerender } = setupTest(baseProps);
+
+  expect(screen.getByTestId('control-popover')).toBeInTheDocument();
+  expect(screen.queryByText('Control Popover Test')).not.toBeInTheDocument();
+
+  rerender(<TestComponent {...baseProps} visible />);
+  expect(await screen.findByText('Control Popover Test')).toBeInTheDocument();
+
+  rerender(<TestComponent {...baseProps} />);
+  await waitFor(() => {
+    expect(screen.queryByText('Control Popover Test')).not.toBeInTheDocument();
+  });
+});

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

@@ -16,7 +16,7 @@
  * specific language governing permissions and limitations
  * under the License.
  */
-import React, { useCallback, useRef, useEffect } from 'react';
+import React, { useCallback, useRef, useEffect, useState } from 'react';
 
 import Popover, {
   PopoverProps as BasePopoverProps,
@@ -25,7 +25,7 @@ import Popover, {
 
 const sectionContainerId = 'controlSections';
 const getSectionContainerElement = () =>
-  document.getElementById(sectionContainerId)?.parentElement;
+  document.getElementById(sectionContainerId)?.lastElementChild as HTMLElement;
 
 const getElementYVisibilityRatioOnContainer = (node: HTMLElement) => {
   const containerHeight = window?.innerHeight;
@@ -44,9 +44,14 @@ export type PopoverProps = BasePopoverProps & {
 const ControlPopover: React.FC<PopoverProps> = ({
   getPopupContainer,
   getVisibilityRatio = getElementYVisibilityRatioOnContainer,
+  visible: visibleProp,
   ...props
 }) => {
   const triggerElementRef = useRef<HTMLElement>();
+
+  const [visible, setVisible] = useState(
+    visibleProp === undefined ? props.defaultVisible : visibleProp,
+  );
   const [placement, setPlacement] = React.useState<TooltipPlacement>('right');
 
   const calculatePlacement = useCallback(() => {
@@ -69,7 +74,11 @@ const ControlPopover: React.FC<PopoverProps> = ({
 
       const element = getSectionContainerElement();
       if (element) {
-        element.style.overflowY = visible ? 'hidden' : 'auto';
+        element.style.setProperty(
+          'overflow-y',
+          visible ? 'hidden' : 'auto',
+          'important',
+        );
       }
     },
     [calculatePlacement],
@@ -88,25 +97,53 @@ const ControlPopover: React.FC<PopoverProps> = ({
   );
 
   const handleOnVisibleChange = useCallback(
-    (visible: boolean) => {
-      if (props.visible === undefined) {
+    (visible: boolean | undefined) => {
+      if (visible === undefined) {
         changeContainerScrollStatus(visible);
       }
 
-      props.onVisibleChange?.(visible);
+      setVisible(!!visible);
+      props.onVisibleChange?.(!!visible);
     },
     [props, changeContainerScrollStatus],
   );
 
+  const handleDocumentKeyDownListener = useCallback(
+    (event: KeyboardEvent) => {
+      if (event.key === 'Escape') {
+        setVisible(false);
+        props.onVisibleChange?.(false);
+      }
+    },
+    [props],
+  );
+
   useEffect(() => {
-    if (props.visible !== undefined) {
-      changeContainerScrollStatus(props.visible);
+    if (visibleProp !== undefined) {
+      setVisible(!!visibleProp);
     }
-  }, [props.visible, changeContainerScrollStatus]);
+  }, [visibleProp]);
+
+  useEffect(() => {
+    if (visible !== undefined) {
+      changeContainerScrollStatus(visible);
+    }
+  }, [visible, changeContainerScrollStatus]);
+
+  useEffect(() => {
+    if (visible) {
+      document.addEventListener('keydown', handleDocumentKeyDownListener);
+    }
+
+    return () => {
+      document.removeEventListener('keydown', handleDocumentKeyDownListener);
+    };
+  }, [handleDocumentKeyDownListener, visible]);
 
   return (
     <Popover
       {...props}
+      visible={visible}
       arrowPointAtCenter
       placement={placement}
       onVisibleChange={handleOnVisibleChange}