diff --git a/superset-frontend/src/SqlLab/components/SqlEditorTabHeader/SqlEditorTabHeader.test.tsx b/superset-frontend/src/SqlLab/components/SqlEditorTabHeader/SqlEditorTabHeader.test.tsx index 242fd937be0..4a331e6b894 100644 --- a/superset-frontend/src/SqlLab/components/SqlEditorTabHeader/SqlEditorTabHeader.test.tsx +++ b/superset-frontend/src/SqlLab/components/SqlEditorTabHeader/SqlEditorTabHeader.test.tsx @@ -135,14 +135,17 @@ describe('SqlEditorTabHeader', () => { test('should dispatch queryEditorSetTitle action', async () => { await waitFor(() => - expect(screen.getByTestId('close-tab-menu-option')).toBeInTheDocument(), + expect( + screen.getByTestId('rename-tab-menu-option'), + ).toBeInTheDocument(), ); const expectedTitle = 'typed text'; - const mockPrompt = jest - .spyOn(window, 'prompt') - .mockImplementation(() => expectedTitle); fireEvent.click(screen.getByTestId('rename-tab-menu-option')); + const input = await screen.findByTestId('rename-tab-input'); + fireEvent.change(input, { target: { value: expectedTitle } }); + fireEvent.click(screen.getByRole('button', { name: 'Save' })); + const actions = store.getActions(); await waitFor(() => expect(actions[0]).toEqual({ @@ -153,7 +156,127 @@ describe('SqlEditorTabHeader', () => { }), }), ); - mockPrompt.mockClear(); + }); + + test('prefills the rename input with the current tab name', async () => { + await waitFor(() => + expect( + screen.getByTestId('rename-tab-menu-option'), + ).toBeInTheDocument(), + ); + fireEvent.click(screen.getByTestId('rename-tab-menu-option')); + + const input = await screen.findByTestId('rename-tab-input'); + expect(input).toHaveValue(defaultQueryEditor.name); + }); + + test('focuses the rename input when the modal opens', async () => { + await waitFor(() => + expect( + screen.getByTestId('rename-tab-menu-option'), + ).toBeInTheDocument(), + ); + fireEvent.click(screen.getByTestId('rename-tab-menu-option')); + + const input = await screen.findByTestId('rename-tab-input'); + await waitFor(() => expect(input).toHaveFocus()); + }); + + test('disables Save when the input is empty or whitespace', async () => { + await waitFor(() => + expect( + screen.getByTestId('rename-tab-menu-option'), + ).toBeInTheDocument(), + ); + fireEvent.click(screen.getByTestId('rename-tab-menu-option')); + + const input = await screen.findByTestId('rename-tab-input'); + fireEvent.change(input, { target: { value: ' ' } }); + expect(screen.getByRole('button', { name: 'Save' })).toBeDisabled(); + }); + + test('does not dispatch or dismiss on Enter when the input is empty', async () => { + await waitFor(() => + expect( + screen.getByTestId('rename-tab-menu-option'), + ).toBeInTheDocument(), + ); + fireEvent.click(screen.getByTestId('rename-tab-menu-option')); + + const input = await screen.findByTestId('rename-tab-input'); + fireEvent.change(input, { target: { value: ' ' } }); + fireEvent.keyDown(input, { key: 'Enter', keyCode: 13, charCode: 13 }); + + const dispatchedTitleChange = store + .getActions() + .some(action => action.type === QUERY_EDITOR_SET_TITLE); + expect(dispatchedTitleChange).toBe(false); + // the modal must stay open so the user can correct the name, + // mirroring the disabled Save button rather than dismissing like Escape + expect(screen.queryByRole('dialog')).toBeInTheDocument(); + }); + + test('does not dispatch a title change when the modal is cancelled', async () => { + await waitFor(() => + expect( + screen.getByTestId('rename-tab-menu-option'), + ).toBeInTheDocument(), + ); + fireEvent.click(screen.getByTestId('rename-tab-menu-option')); + + const input = await screen.findByTestId('rename-tab-input'); + fireEvent.change(input, { target: { value: 'discarded text' } }); + fireEvent.click(screen.getByRole('button', { name: 'Cancel' })); + + expect(store.getActions()).toEqual([]); + }); + + test('does not dispatch a title change when dismissed with the close button', async () => { + await waitFor(() => + expect( + screen.getByTestId('rename-tab-menu-option'), + ).toBeInTheDocument(), + ); + fireEvent.click(screen.getByTestId('rename-tab-menu-option')); + + const input = await screen.findByTestId('rename-tab-input'); + fireEvent.change(input, { target: { value: 'discarded text' } }); + fireEvent.click(screen.getByTestId('close-modal-btn')); + + expect(store.getActions()).toEqual([]); + }); + + test('returns focus to the tab header after the modal is cancelled', async () => { + await waitFor(() => + expect( + screen.getByTestId('rename-tab-menu-option'), + ).toBeInTheDocument(), + ); + fireEvent.click(screen.getByTestId('rename-tab-menu-option')); + + await screen.findByTestId('rename-tab-input'); + fireEvent.click(screen.getByRole('button', { name: 'Cancel' })); + + await waitFor(() => + expect(screen.getByTestId('sql-editor-tab-header')).toHaveFocus(), + ); + }); + + test('returns focus to the tab header after a successful rename', async () => { + await waitFor(() => + expect( + screen.getByTestId('rename-tab-menu-option'), + ).toBeInTheDocument(), + ); + fireEvent.click(screen.getByTestId('rename-tab-menu-option')); + + const input = await screen.findByTestId('rename-tab-input'); + fireEvent.change(input, { target: { value: 'renamed tab' } }); + fireEvent.click(screen.getByRole('button', { name: 'Save' })); + + await waitFor(() => + expect(screen.getByTestId('sql-editor-tab-header')).toHaveFocus(), + ); }); test('should dispatch removeAllOtherQueryEditors action', async () => { @@ -196,4 +319,42 @@ describe('SqlEditorTabHeader', () => { ); }); }); + + test('does not leak tab-editing keystrokes from the rename input to the surrounding tabs', async () => { + const onContainerKeyDown = jest.fn(); + const store = mockStore(initialState); + render( +
+ +
, + { useRedux: true, store }, + ); + + userEvent.click(screen.getByTestId('dropdown-trigger')); + await waitFor(() => + expect(screen.getByTestId('rename-tab-menu-option')).toBeInTheDocument(), + ); + fireEvent.click(screen.getByTestId('rename-tab-menu-option')); + const input = await screen.findByTestId('rename-tab-input'); + + // The modal portals over the editable-card tabs, whose keyboard handler would + // otherwise remove, navigate, or activate a tab (and swallow Space). None of + // these keys should escape the modal to the surrounding container. + [ + 'Delete', + 'Backspace', + 'ArrowLeft', + 'ArrowRight', + 'Home', + 'End', + ' ', + ].forEach(key => fireEvent.keyDown(input, { key })); + expect(onContainerKeyDown).not.toHaveBeenCalled(); + + // Escape (close) and Tab (focus trap) must still reach the Modal. + fireEvent.keyDown(input, { key: 'Tab' }); + fireEvent.keyDown(input, { key: 'Escape' }); + const reached = onContainerKeyDown.mock.calls.map(call => call[0].key); + expect(reached).toEqual(expect.arrayContaining(['Tab', 'Escape'])); + }); }); diff --git a/superset-frontend/src/SqlLab/components/SqlEditorTabHeader/index.tsx b/superset-frontend/src/SqlLab/components/SqlEditorTabHeader/index.tsx index dc7804d4c4a..d11326f2010 100644 --- a/superset-frontend/src/SqlLab/components/SqlEditorTabHeader/index.tsx +++ b/superset-frontend/src/SqlLab/components/SqlEditorTabHeader/index.tsx @@ -16,12 +16,17 @@ * specific language governing permissions and limitations * under the License. */ -import { useMemo, FC } from 'react'; +import { useEffect, useMemo, useRef, useState, FC } from 'react'; import { bindActionCreators } from 'redux'; import { useSelector, shallowEqual } from 'react-redux'; import { useAppDispatch } from 'src/SqlLab/hooks/useAppDispatch'; -import { MenuDotsDropdown } from '@superset-ui/core/components'; +import { + MenuDotsDropdown, + Modal, + Input, + InputRef, +} from '@superset-ui/core/components'; import { Menu, MenuItemType } from '@superset-ui/core/components/Menu'; import { t } from '@apache-superset/core/translation'; import { QueryState } from '@superset-ui/core'; @@ -107,13 +112,35 @@ const SqlEditorTabHeader: FC = ({ queryEditor }) => { [dispatch], ); - function renameTab() { - // TODO: Replace native prompt with a proper modal dialog - // eslint-disable-next-line no-alert - const newTitle = prompt(t('Enter a new title for the tab')); - if (newTitle) { - actions.queryEditorSetTitle(qe, newTitle, qe.id); + const [isRenameModalOpen, setIsRenameModalOpen] = useState(false); + const [newTitle, setNewTitle] = useState(''); + const renameInputRef = useRef(null); + const tabHeaderRef = useRef(null); + const trimmedTitle = newTitle.trim(); + + function openRenameModal() { + setNewTitle(qe.name); + setIsRenameModalOpen(true); + } + + // antd's Modal moves focus to the dialog container on open, which overrides + // the Input's autoFocus, so focus and select the field via a ref once the + // modal is open (select lets the prefilled name be overtyped, like prompt()). + useEffect(() => { + if (isRenameModalOpen) { + renameInputRef.current?.focus(); + renameInputRef.current?.select(); } + }, [isRenameModalOpen]); + + function handleRenameTab() { + if (trimmedTitle) { + actions.queryEditorSetTitle(qe, trimmedTitle, qe.id); + } + setIsRenameModalOpen(false); + // Save closes via the show prop rather than the Modal's onHide, so return + // focus to the tab header here, matching what openerRef does on dismiss. + tabHeaderRef.current?.focus(); } const getStatusColor = (state: QueryState, theme: SupersetTheme): string => { const statusColors: Record = { @@ -131,7 +158,11 @@ const SqlEditorTabHeader: FC = ({ queryEditor }) => { return statusColors[state] || theme.colorIcon; }; return ( - + = ({ queryEditor }) => { } as MenuItemType, { key: '2', - onClick: renameTab, + onClick: openRenameModal, 'data-test': 'rename-tab-menu-option', label: ( <> @@ -220,6 +251,37 @@ const SqlEditorTabHeader: FC = ({ queryEditor }) => { iconSize="m" iconColor={getStatusColor(queryState, theme)} />{' '} + setIsRenameModalOpen(false)} + title={t('Rename tab')} + onHandledPrimaryAction={handleRenameTab} + primaryButtonName={t('Save')} + disablePrimaryButton={!trimmedTitle} + openerRef={tabHeaderRef} + > + setNewTitle(e.target.value)} + onPressEnter={() => { + if (trimmedTitle) { + handleRenameTab(); + } + }} + onKeyDown={e => { + // The modal portals over the editable-card tabs; without this, keys + // bubble to their handler and remove, navigate, or activate a tab + // (Space included). Escape and Tab are left to bubble so the Modal + // can close and trap focus. + if (e.key !== 'Escape' && e.key !== 'Tab') { + e.stopPropagation(); + } + }} + /> + ); };