diff --git a/superset-frontend/src/explore/components/controls/ControlPopover/ControlPopover.test.tsx b/superset-frontend/src/explore/components/controls/ControlPopover/ControlPopover.test.tsx index de14f78f7530..3da003be4a6a 100644 --- a/superset-frontend/src/explore/components/controls/ControlPopover/ControlPopover.test.tsx +++ b/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 => ({ content: Information, }); +const TestComponent: React.FC = props => ( +
+
+ + Click me + +
+
+); + const setupTest = (props: Partial = createProps()) => { const setStateMock = jest.fn(); jest @@ -38,19 +48,12 @@ const setupTest = (props: Partial = createProps()) => { state === 'right' ? setStateMock : jest.fn(), ]) as any); - const { container } = render( -
-
- - Click me - -
-
, - ); + const { container, rerender } = render(); 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(); + expect(await screen.findByText('Control Popover Test')).toBeInTheDocument(); + + rerender(); + await waitFor(() => { + expect(screen.queryByText('Control Popover Test')).not.toBeInTheDocument(); + }); +}); diff --git a/superset-frontend/src/explore/components/controls/ControlPopover/ControlPopover.tsx b/superset-frontend/src/explore/components/controls/ControlPopover/ControlPopover.tsx index 70ade69f279e..f84194c43caa 100644 --- a/superset-frontend/src/explore/components/controls/ControlPopover/ControlPopover.tsx +++ b/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 = ({ getPopupContainer, getVisibilityRatio = getElementYVisibilityRatioOnContainer, + visible: visibleProp, ...props }) => { const triggerElementRef = useRef(); + + const [visible, setVisible] = useState( + visibleProp === undefined ? props.defaultVisible : visibleProp, + ); const [placement, setPlacement] = React.useState('right'); const calculatePlacement = useCallback(() => { @@ -69,7 +74,11 @@ const ControlPopover: React.FC = ({ 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 = ({ ); 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 (