From 18a21fab0d4a777a696d17616a1e9125972d3f71 Mon Sep 17 00:00:00 2001 From: Dave Page Date: Wed, 10 Jun 2026 13:55:26 +0100 Subject: [PATCH 1/3] Keyboard shortcut improvements across tabs, the SQL editor and dialogs. - Fix the main "tabbed panel forward/backward" shortcut not switching the workspace tabs when keyboard focus is inside a tool (SQL editor, PSQL terminal, ERD or Schema Diff). bindRightPanel now locates the active workspace tab via rc-dock's dock-tab-active class, independent of focus, and restricts cycling to the workspace tab-set. The default shortcut is changed to Ctrl/Cmd+Alt+] / [ so it no longer collides with the Query Tool's inner-panel navigation (Alt+Shift+] / [) and does not emit glyphs on macOS; the bogus key codes (Meta/ContextMenu) are corrected to the bracket key codes. - Add Ctrl/Cmd+Shift+D to duplicate the current line or selection in the SQL editor. - Add Ctrl/Cmd+Enter to save and close object/utility dialogs (including the Query Tool sort/filter dialog), and Escape to close them - dialogs rendered as dockable panels (Properties, Backup, etc.) previously had neither. The Escape handler is scoped to panel dialogs (skips MUI modals, which already close on Escape) and yields to inner controls that handle Escape first. Closes #7232 Closes #3834 Closes #7167 Closes #5691 Closes #5196 --- docs/en_US/keyboard_shortcuts.rst | 6 +- .../browser/register_browser_preferences.py | 12 ++-- web/pgadmin/browser/static/js/keyboard.js | 61 +++++++++---------- .../static/js/SchemaView/SchemaDialogView.jsx | 25 +++++++- .../ReactCodeMirror/components/Editor.jsx | 8 ++- 5 files changed, 69 insertions(+), 43 deletions(-) diff --git a/docs/en_US/keyboard_shortcuts.rst b/docs/en_US/keyboard_shortcuts.rst index 5f766637865..32091d8e44f 100644 --- a/docs/en_US/keyboard_shortcuts.rst +++ b/docs/en_US/keyboard_shortcuts.rst @@ -44,9 +44,9 @@ When using main browser window, the following keyboard shortcuts are available: +----------------------------+--------------------+------------------------------------+ | Shift + Alt + s | Shift + Option + s | Search objects | +----------------------------+--------------------+------------------------------------+ - | Shift + Alt + [ | Shift + Option + [ | Tabbed panel backward | + | Ctrl + Alt + [ | Ctrl + Option + [ | Tabbed panel backward | +----------------------------+--------------------+------------------------------------+ - | Shift + Alt + ] | Shift + Option + ] | Tabbed panel forward | + | Ctrl + Alt + ] | Ctrl + Option + ] | Tabbed panel forward | +----------------------------+--------------------+------------------------------------+ | Shift + Alt + w | Shift + Ctrl + w | Close tab panel | +----------------------------+--------------------+------------------------------------+ @@ -98,6 +98,8 @@ When using the syntax-highlighting SQL editors, the following shortcuts are avai +--------------------------+----------------------+-------------------------------------+ | Ctrl + / | Cmd + / | Comment/Uncomment code (Block) | +--------------------------+----------------------+-------------------------------------+ + | Ctrl + Shift + d | Cmd + Shift + d | Duplicate current line/selection | + +--------------------------+----------------------+-------------------------------------+ | Ctrl + a | Cmd + a | Select all | +--------------------------+----------------------+-------------------------------------+ | Ctrl + c | Cmd + c | Copy selected text to the clipboard | diff --git a/web/pgadmin/browser/register_browser_preferences.py b/web/pgadmin/browser/register_browser_preferences.py index d2fe51dc5fa..064f23d2a2c 100644 --- a/web/pgadmin/browser/register_browser_preferences.py +++ b/web/pgadmin/browser/register_browser_preferences.py @@ -170,9 +170,9 @@ def register_browser_preferences(self): 'keyboardshortcut', { 'alt': True, - 'shift': True, - 'control': False, - 'key': {'key_code': 91, 'char': '['} + 'shift': False, + 'control': True, + 'key': {'key_code': 219, 'char': '['} }, category_label=PREF_LABEL_KEYBOARD_SHORTCUTS, fields=fields @@ -200,9 +200,9 @@ def register_browser_preferences(self): 'keyboardshortcut', { 'alt': True, - 'shift': True, - 'control': False, - 'key': {'key_code': 93, 'char': ']'} + 'shift': False, + 'control': True, + 'key': {'key_code': 221, 'char': ']'} }, category_label=PREF_LABEL_KEYBOARD_SHORTCUTS, fields=fields diff --git a/web/pgadmin/browser/static/js/keyboard.js b/web/pgadmin/browser/static/js/keyboard.js index 662b8dd0239..b2b6f1b2202 100644 --- a/web/pgadmin/browser/static/js/keyboard.js +++ b/web/pgadmin/browser/static/js/keyboard.js @@ -150,42 +150,37 @@ _.extend(pgBrowser.keyboardNavigation, { bindRightPanel: function(event, combo) { const self = this; const shortcutObj = this.keyboardShortcut; - const activeElement = document.activeElement; + const rootDock = document.getElementById('root'); + if (!rootDock) return; - if (activeElement.closest('.dock-tab-btn')) { - const currDockTab = activeElement.closest('.dock-tab-btn'); - const dockLayout = currDockTab.closest('.dock-layout'); - const dockLayoutTabs = dockLayout ? Array.from(dockLayout.querySelectorAll('.dock-tab-btn')) : null; + // Find the active workspace tab independently of where the keyboard focus + // currently sits. Previously this relied on document.activeElement, which + // breaks when focus is inside a tool's own (nested) dock layout - the SQL + // editor, an ERD/Schema Diff canvas - or inside the PSQL iframe, because + // the resolved tab id then belonged to the tool's inner tab rather than a + // main workspace tab (issue #7232). + // + // rc-dock renders the object explorer and the workspace as separate + // tab-sets, and each marks its own active tab with `dock-tab-active`, so + // prefer the active tab that is not the object explorer. + const activeTabBtns = Array.from( + rootDock.querySelectorAll('.dock-tab.dock-tab-active .dock-tab-btn')); + const activeTabBtn = + activeTabBtns.find(tab => !tab.id.includes('id-object-explorer')) || + activeTabBtns[0]; + if (!activeTabBtn) return; - if (dockLayoutTabs && dockLayoutTabs.length > 1) { - const activeTabIndex = dockLayoutTabs.indexOf(currDockTab); - self._focusTab(dockLayoutTabs, activeTabIndex, shortcutObj, combo); - } - } - else if (activeElement.nodeName === 'IFRAME' || activeElement.closest('.dock-tabpane.dock-tabpane-active')) { - let activeTabId = ''; - activeTabId = (activeElement.nodeName === 'IFRAME') ? activeElement.id : activeElement.closest('.dock-tabpane.dock-tabpane-active').id; - const dockLayout = document.getElementById('root'); - const dockLayoutTabs = dockLayout ? Array.from(dockLayout.querySelectorAll('.dock-tab-btn')) : null; - - if (dockLayoutTabs && dockLayoutTabs.length > 1 && activeTabId) { - const activeTabIndex = dockLayoutTabs.findIndex(tab => tab.id.slice(14) === activeTabId); - self._focusTab(dockLayoutTabs, activeTabIndex, shortcutObj, combo); - } - } - else if (activeElement === document.body || document.querySelector('div[data-test="app-menu-bar"]')) { - const activeTabs = document.getElementsByClassName('dock-tabpane dock-tabpane-active'); - - if (activeTabs.length > 1) { - const activeTabId = activeTabs[1].id; - const dockLayout = document.getElementById('root'); - const dockLayoutTabs = dockLayout ? Array.from(dockLayout.querySelectorAll('.dock-tab-btn')) : null; + // Restrict navigation to the tabs of the same tab-set (dock panel) as the + // active tab, so cycling stays within the workspace tabs and does not + // include the object explorer or a tool's nested tabs. + const panel = activeTabBtn.closest('.dock-panel'); + const dockLayoutTabs = panel ? Array.from( + panel.querySelectorAll('.dock-tab-btn')) + .filter(tab => tab.closest('.dock-panel') === panel) : []; - if (dockLayoutTabs && dockLayoutTabs.length > 1 && activeTabId) { - const activeTabIndex = dockLayoutTabs.findIndex(tab => tab.id.slice(14) === activeTabId); - self._focusTab(dockLayoutTabs, activeTabIndex, shortcutObj, combo); - } - } + if (dockLayoutTabs.length > 1) { + const activeTabIndex = dockLayoutTabs.indexOf(activeTabBtn); + self._focusTab(dockLayoutTabs, activeTabIndex, shortcutObj, combo); } }, _focusTab: function(dockLayoutTabs, activeTabIdx, shortcut_obj, combo){ diff --git a/web/pgadmin/static/js/SchemaView/SchemaDialogView.jsx b/web/pgadmin/static/js/SchemaView/SchemaDialogView.jsx index 3c83f8e7ab3..8538f7809cf 100644 --- a/web/pgadmin/static/js/SchemaView/SchemaDialogView.jsx +++ b/web/pgadmin/static/js/SchemaView/SchemaDialogView.jsx @@ -177,9 +177,32 @@ export default function SchemaDialogView({ return ; }; + const onKeyDown = (e) => { + // Ctrl/Cmd+Enter saves and closes the dialog from anywhere within it + // (issue #7167). onSaveClick is a no-op when there is nothing to save or + // there is a validation error, so this is safe to call unconditionally. + if ((e.ctrlKey || e.metaKey) && !e.altKey && e.key === 'Enter') { + e.preventDefault(); + onSaveClick(); + return; + } + + // Escape closes the dialog, mirroring the Close button (issue #5691). + // This is needed for dialogs rendered as dockable panels (Properties, + // Backup, and other utility dialogs); dialogs rendered inside a MUI modal + // already close on Escape, so skip those to avoid a double close. The + // !e.defaultPrevented guard lets an inner control that handles Escape + // (e.g. an open dropdown) consume it first. + if (e.key === 'Escape' && !e.defaultPrevented && props.onClose && + !e.currentTarget.closest('.MuiDialog-root')) { + e.preventDefault(); + props.onClose(); + } + }; + /* I am Groot */ return useMemo(() => - + diff --git a/web/pgadmin/static/js/components/ReactCodeMirror/components/Editor.jsx b/web/pgadmin/static/js/components/ReactCodeMirror/components/Editor.jsx index 013aa3a03cf..cc9b54448f6 100644 --- a/web/pgadmin/static/js/components/ReactCodeMirror/components/Editor.jsx +++ b/web/pgadmin/static/js/components/ReactCodeMirror/components/Editor.jsx @@ -30,7 +30,7 @@ import { keymap, } from '@codemirror/view'; import { EditorState, Compartment } from '@codemirror/state'; -import { history, defaultKeymap, historyKeymap, indentLess, indentMore, deleteCharBackwardStrict } from '@codemirror/commands'; +import { history, defaultKeymap, historyKeymap, indentLess, indentMore, deleteCharBackwardStrict, copyLineDown } from '@codemirror/commands'; import { closeBrackets, autocompletion, closeBracketsKeymap, completionKeymap, acceptCompletion } from '@codemirror/autocomplete'; import { foldGutter, @@ -140,6 +140,12 @@ const defaultExtensions = [ key: 'Backspace', preventDefault: true, run: deleteCharBackwardStrict, + },{ + // Duplicate the current line, or the selected lines if there is a + // selection (issue #3834). + key: 'Mod-Shift-d', + preventDefault: true, + run: copyLineDown, }]), PgSQL.language.data.of({ autocomplete: false, From 1aaa9aa5af4a36fcfb8e1cc59757dd64c49e885f Mon Sep 17 00:00:00 2001 From: Dave Page Date: Mon, 17 Aug 2026 13:55:36 +0100 Subject: [PATCH 2/3] Fix the stale Escape handler and pin the tab selection with tests The Escape and Ctrl+Enter handler was attached to the memoized element, so it captured whichever props.onClose and onSaveClick existed when the memo deps last changed. Any parent re-render supplying a new onClose, which is the normal case where it is defined inline, left Escape calling the stale one. Only the dialog body is memoized now; the wrapper carrying the handler is created on every render, which is what the review recommended and costs nothing since the body is what is expensive. For the tab navigation I could not reproduce the reported failure. With rc-dock's DOM as it is rendered, a panel's tab buttons precede the nested DockLayout inside its own tab pane, so the search for "the active tab that is not the object explorer" finds the workspace tab before it reaches the SQL editor's Data Output tab, and the existing .dock-panel filter then keeps the cycling within the workspace. What is true is that this only holds by accident of document order. The search is now confined to the outermost dock layout, so a tool's tabs cannot take part however rc-dock chooses to order its panels, and the test builds exactly that case: with the nested layout placed first, the previous code cycled Data Output and Messages instead of the workspace tabs. Both fixes have tests that fail without them: web/regression/javascript/SchemaView/SchemaDialogViewKeyboard.spec.js holds the callback case, keeping the same schema instance so nothing invalidates the memo, and web/regression/javascript/browser/keyboard_navigation_spec.js covers the tab selection, including the object explorer being excluded and the case where no workspace tabs exist. On the missing migration for the tabbed_panel_backward/forward defaults: Preference.get() falls back to the registered default whenever the user has no saved row, so anyone who has not customised these shortcuts picks up Ctrl+Alt+[ and ] with no migration at all. Anyone who has saved a value keeps it, which is the behaviour I would want: silently rewriting a shortcut somebody chose deliberately is worse than leaving it alone. --- web/pgadmin/browser/static/js/keyboard.js | 16 +- .../static/js/SchemaView/SchemaDialogView.jsx | 11 +- .../SchemaDialogViewKeyboard.spec.js | 102 ++++++++++ .../browser/keyboard_navigation_spec.js | 176 ++++++++++++++++++ 4 files changed, 300 insertions(+), 5 deletions(-) create mode 100644 web/regression/javascript/SchemaView/SchemaDialogViewKeyboard.spec.js create mode 100644 web/regression/javascript/browser/keyboard_navigation_spec.js diff --git a/web/pgadmin/browser/static/js/keyboard.js b/web/pgadmin/browser/static/js/keyboard.js index b2b6f1b2202..cf9c7bbbbbd 100644 --- a/web/pgadmin/browser/static/js/keyboard.js +++ b/web/pgadmin/browser/static/js/keyboard.js @@ -163,8 +163,19 @@ _.extend(pgBrowser.keyboardNavigation, { // rc-dock renders the object explorer and the workspace as separate // tab-sets, and each marks its own active tab with `dock-tab-active`, so // prefer the active tab that is not the object explorer. + // + // Only the outermost dock layout is considered. The SQL editor, ERD and + // Schema Diff each render a DockLayout of their own inside a workspace + // tab, and their active tabs (Data Output, Messages, Notifications and + // so on) carry the same `dock-tab-active` class, so a search across the + // whole tree can land on one of those and cycle a tool's inner tabs + // instead of the workspace tabs it was asked to move between. + const topDockLayout = rootDock.querySelector('.dock-layout'); + if (!topDockLayout) return; + const activeTabBtns = Array.from( - rootDock.querySelectorAll('.dock-tab.dock-tab-active .dock-tab-btn')); + topDockLayout.querySelectorAll('.dock-tab.dock-tab-active .dock-tab-btn') + ).filter(tab => tab.closest('.dock-layout') === topDockLayout); const activeTabBtn = activeTabBtns.find(tab => !tab.id.includes('id-object-explorer')) || activeTabBtns[0]; @@ -176,7 +187,8 @@ _.extend(pgBrowser.keyboardNavigation, { const panel = activeTabBtn.closest('.dock-panel'); const dockLayoutTabs = panel ? Array.from( panel.querySelectorAll('.dock-tab-btn')) - .filter(tab => tab.closest('.dock-panel') === panel) : []; + .filter(tab => tab.closest('.dock-panel') === panel && + tab.closest('.dock-layout') === topDockLayout) : []; if (dockLayoutTabs.length > 1) { const activeTabIndex = dockLayoutTabs.indexOf(activeTabBtn); diff --git a/web/pgadmin/static/js/SchemaView/SchemaDialogView.jsx b/web/pgadmin/static/js/SchemaView/SchemaDialogView.jsx index 8538f7809cf..df4dd5dd02b 100644 --- a/web/pgadmin/static/js/SchemaView/SchemaDialogView.jsx +++ b/web/pgadmin/static/js/SchemaView/SchemaDialogView.jsx @@ -201,8 +201,11 @@ export default function SchemaDialogView({ }; /* I am Groot */ - return useMemo(() => - + // Only the children are memoized: the wrapper carries onKeyDown, which + // closes over props.onClose and onSaveClick, and memoizing it would pin + // whichever versions of those existed when the deps last changed. + const dialogContent = useMemo(() => + <> @@ -257,8 +260,10 @@ export default function SchemaDialogView({ } - , [schema._id, viewHelperProps.mode, resetKey] + , [schema._id, viewHelperProps.mode, resetKey] ); + + return {dialogContent}; } SchemaDialogView.propTypes = { diff --git a/web/regression/javascript/SchemaView/SchemaDialogViewKeyboard.spec.js b/web/regression/javascript/SchemaView/SchemaDialogViewKeyboard.spec.js new file mode 100644 index 00000000000..171cd88e1b5 --- /dev/null +++ b/web/regression/javascript/SchemaView/SchemaDialogViewKeyboard.spec.js @@ -0,0 +1,102 @@ +///////////////////////////////////////////////////////////// +// +// pgAdmin 4 - PostgreSQL Tools +// +// Copyright (C) 2013 - 2026, The pgAdmin Development Team +// This software is released under the PostgreSQL Licence +// +////////////////////////////////////////////////////////////// + +import { act, fireEvent, render } from '@testing-library/react'; + +import SchemaView from '../../../pgadmin/static/js/SchemaView'; +import { TestSchema } from './TestSchema.ui'; +import { withBrowser } from '../genericFunctions'; + +// Escape closes a dialog rendered as a dockable panel (issue #5691). The +// handler sits on the dialog wrapper, which must not be memoized along with +// the dialog body: the body only changes with the schema, the mode or the +// reset key, whereas onClose is a fresh function on most parent renders, and +// a memoized wrapper would keep calling whichever one it captured first. +describe('SchemaDialogView keyboard handling', () => { + const SchemaViewWithBrowser = withBrowser(SchemaView); + + const dialog = (schema, onClose) => ( + Promise.resolve())} + onClose={onClose} + onHelp={jest.fn()} + onEdit={jest.fn()} + onDataChange={jest.fn()} + hasSQL={false} + disableSqlHelp={true} + disableDialogHelp={true} + /> + ); + + const renderDialog = async (onClose) => { + let ctrl; + await act(async () => { + ctrl = render( + Promise.resolve())} + onClose={onClose} + onHelp={jest.fn()} + onEdit={jest.fn()} + onDataChange={jest.fn()} + hasSQL={false} + disableSqlHelp={true} + disableDialogHelp={true} + /> + ); + }); + return ctrl; + }; + + const pressEscape = async (ctrl) => { + await act(async () => { + fireEvent.keyDown(ctrl.container.firstChild, {key: 'Escape'}); + }); + }; + + it('closes the dialog on Escape', async () => { + const onClose = jest.fn(); + const ctrl = await renderDialog(onClose); + + await pressEscape(ctrl); + + expect(onClose).toHaveBeenCalled(); + }); + + it('calls the current onClose, not the one from the first render', + async () => { + const firstOnClose = jest.fn(); + const secondOnClose = jest.fn(); + // The same schema throughout: the memo deps are the schema id, the mode + // and the reset key, so this is the case where nothing invalidates the + // memo and only the callback has changed. + const schema = new TestSchema(); + + let ctrl; + await act(async () => { + ctrl = render(dialog(schema, firstOnClose)); + }); + + // The parent re-renders with a new callback, as it does whenever it + // defines onClose inline. + await act(async () => { + ctrl.rerender(dialog(schema, secondOnClose)); + }); + + await pressEscape(ctrl); + + expect(secondOnClose).toHaveBeenCalled(); + expect(firstOnClose).not.toHaveBeenCalled(); + }); +}); diff --git a/web/regression/javascript/browser/keyboard_navigation_spec.js b/web/regression/javascript/browser/keyboard_navigation_spec.js new file mode 100644 index 00000000000..357c4c1d08d --- /dev/null +++ b/web/regression/javascript/browser/keyboard_navigation_spec.js @@ -0,0 +1,176 @@ +///////////////////////////////////////////////////////////// +// +// pgAdmin 4 - PostgreSQL Tools +// +// Copyright (C) 2013 - 2026, The pgAdmin Development Team +// This software is released under the PostgreSQL Licence +// +////////////////////////////////////////////////////////////// + +// keyboard.js reaches pgadmin.js by relative path, which skips the +// sources/pgadmin alias that maps to the fake, so point it there explicitly. +jest.mock('../../../pgadmin/static/js/pgadmin', () => + jest.requireActual('../fake_pgadmin')); + +import pgAdmin from 'sources/pgadmin'; +import '../../../pgadmin/browser/static/js/keyboard'; + +// Cycling between workspace tabs must stay in the workspace: the object +// explorer is a tab-set of its own, and the SQL editor, ERD and Schema Diff +// each render a nested DockLayout whose own tabs carry the same +// dock-tab-active class (issue #7232). +describe('keyboardNavigation.bindRightPanel', () => { + const shortcutObj = { + tabbed_panel_forward: 'ctrl+alt+]', + tabbed_panel_backward: 'ctrl+alt+[', + close_tab_panel: 'shift+alt+w', + }; + + const tabBtn = (id, isActive) => { + const tab = document.createElement('div'); + tab.className = isActive ? 'dock-tab dock-tab-active' : 'dock-tab'; + const btn = document.createElement('div'); + btn.className = 'dock-tab-btn'; + btn.id = `rc-dock-tab-btn-${id}`; + tab.appendChild(btn); + return tab; + }; + + const panel = (tabs) => { + const el = document.createElement('div'); + el.className = 'dock-panel'; + tabs.forEach((tab) => el.appendChild(tab)); + return el; + }; + + /* A workspace holding the object explorer, three workspace tabs and, inside + * the active workspace tab, a tool with its own dock layout. */ + const buildLayout = ({activeWorkspaceTab = 'id-dashboard'} = {}) => { + const root = document.createElement('div'); + root.id = 'root'; + + const topLayout = document.createElement('div'); + topLayout.className = 'dock-layout'; + root.appendChild(topLayout); + + topLayout.appendChild(panel([tabBtn('id-object-explorer', true)])); + + const workspaceTabs = ['id-dashboard', 'id-properties', 'id-sql'].map( + (id) => tabBtn(id, id === activeWorkspaceTab)); + const workspacePanel = panel(workspaceTabs); + topLayout.appendChild(workspacePanel); + + // The tool's own dock layout, nested inside the workspace panel exactly + // as the SQL editor's is. + const innerLayout = document.createElement('div'); + innerLayout.className = 'dock-layout'; + innerLayout.appendChild(panel([ + tabBtn('id-dataoutput', true), + tabBtn('id-messages', false), + ])); + workspacePanel.appendChild(innerLayout); + + document.body.appendChild(root); + return {root, workspacePanel}; + }; + + let focusTabSpy; + + beforeEach(() => { + document.body.innerHTML = ''; + pgAdmin.Browser.keyboardNavigation.keyboardShortcut = shortcutObj; + focusTabSpy = jest.spyOn( + pgAdmin.Browser.keyboardNavigation, '_focusTab' + ).mockImplementation(() => {}); + }); + + afterEach(() => { + focusTabSpy.mockRestore(); + document.body.innerHTML = ''; + }); + + it('cycles the workspace tabs, not a tool\'s nested tabs', () => { + buildLayout(); + + pgAdmin.Browser.keyboardNavigation.bindRightPanel( + new Event('keydown'), {key: shortcutObj.tabbed_panel_forward}); + + expect(focusTabSpy).toHaveBeenCalled(); + const [tabs, activeIdx] = focusTabSpy.mock.calls[0]; + const ids = tabs.map((tab) => tab.id); + + expect(ids).toEqual([ + 'rc-dock-tab-btn-id-dashboard', + 'rc-dock-tab-btn-id-properties', + 'rc-dock-tab-btn-id-sql', + ]); + // Neither the nested tool tabs nor the object explorer may take part. + expect(ids).not.toContain('rc-dock-tab-btn-id-dataoutput'); + expect(ids).not.toContain('rc-dock-tab-btn-id-object-explorer'); + expect(tabs[activeIdx].id).toBe('rc-dock-tab-btn-id-dashboard'); + }); + + it('starts from whichever workspace tab is active', () => { + buildLayout({activeWorkspaceTab: 'id-sql'}); + + pgAdmin.Browser.keyboardNavigation.bindRightPanel( + new Event('keydown'), {key: shortcutObj.tabbed_panel_backward}); + + const [tabs, activeIdx] = focusTabSpy.mock.calls[0]; + expect(tabs[activeIdx].id).toBe('rc-dock-tab-btn-id-sql'); + }); + + /* The selection must not depend on where the nested layout happens to sit + * in the DOM: rc-dock is free to order panels as it likes, and an inner + * layout appearing first would otherwise win the search for the active + * tab and cycle a tool's own tabs. */ + it('ignores nested tool tabs even when they come first in the DOM', () => { + const root = document.createElement('div'); + root.id = 'root'; + const topLayout = document.createElement('div'); + topLayout.className = 'dock-layout'; + root.appendChild(topLayout); + + const innerLayout = document.createElement('div'); + innerLayout.className = 'dock-layout'; + innerLayout.appendChild(panel([ + tabBtn('id-dataoutput', true), + tabBtn('id-messages', false), + ])); + topLayout.appendChild(innerLayout); + + topLayout.appendChild(panel([tabBtn('id-object-explorer', true)])); + topLayout.appendChild(panel([ + tabBtn('id-dashboard', false), + tabBtn('id-sql', true), + ])); + document.body.appendChild(root); + + pgAdmin.Browser.keyboardNavigation.bindRightPanel( + new Event('keydown'), {key: shortcutObj.tabbed_panel_forward}); + + expect(focusTabSpy).toHaveBeenCalled(); + const [tabs, activeIdx] = focusTabSpy.mock.calls[0]; + const ids = tabs.map((tab) => tab.id); + expect(ids).toEqual([ + 'rc-dock-tab-btn-id-dashboard', + 'rc-dock-tab-btn-id-sql', + ]); + expect(tabs[activeIdx].id).toBe('rc-dock-tab-btn-id-sql'); + }); + + it('does nothing when only the object explorer is present', () => { + const root = document.createElement('div'); + root.id = 'root'; + const topLayout = document.createElement('div'); + topLayout.className = 'dock-layout'; + topLayout.appendChild(panel([tabBtn('id-object-explorer', true)])); + root.appendChild(topLayout); + document.body.appendChild(root); + + pgAdmin.Browser.keyboardNavigation.bindRightPanel( + new Event('keydown'), {key: shortcutObj.tabbed_panel_forward}); + + expect(focusTabSpy).not.toHaveBeenCalled(); + }); +}); From 4951c70ec34fc376db85f9a34160cb863b7a95ec Mon Sep 17 00:00:00 2001 From: Dave Page Date: Thu, 20 Aug 2026 09:24:00 +0100 Subject: [PATCH 3/3] Close the dialog after a successful Ctrl/Cmd+Enter save onSaveClick() only ever called props.onSave; nothing subsequently closed the dialog, so the shortcut saved but left the panel open despite the inline comment (and issue #7167) saying it should save and close. onSaveClick now takes an explicit closeOnSave flag, set only by the Ctrl/Cmd+Enter handler, and calls props.onClose once the save promise resolves. The Save button's onClick still passes its click event as the first argument, which is never === true, so a plain Save click keeps its existing per-dialog behaviour (e.g. object properties dialogs staying open). Adds a test that types a change and confirms both onSave and onClose fire on Ctrl/Cmd+Enter; the existing tests only covered Escape. --- .../static/js/SchemaView/SchemaDialogView.jsx | 17 ++++-- .../SchemaDialogViewKeyboard.spec.js | 56 +++++++++++++++++-- 2 files changed, 64 insertions(+), 9 deletions(-) diff --git a/web/pgadmin/static/js/SchemaView/SchemaDialogView.jsx b/web/pgadmin/static/js/SchemaView/SchemaDialogView.jsx index df4dd5dd02b..bf3067ccb3f 100644 --- a/web/pgadmin/static/js/SchemaView/SchemaDialogView.jsx +++ b/web/pgadmin/static/js/SchemaView/SchemaDialogView.jsx @@ -97,7 +97,7 @@ export default function SchemaDialogView({ ); }; - const save = (changeData) => { + const save = (changeData, onSaved) => { props.onSave(schemaState.isNew, changeData) .then(()=>{ if(schema.informText) { @@ -106,6 +106,7 @@ export default function SchemaDialogView({ schema.informText, ); } + onSaved?.(); }).catch((err)=>{ schemaState.setError({ name: 'apierror', @@ -119,7 +120,11 @@ export default function SchemaDialogView({ }); }; - const onSaveClick = () => { + // closeOnSave is only ever passed explicitly as true, by the Ctrl/Cmd+Enter + // handler below. The Save button's onClick passes its click event instead, + // which is never === true, so a plain Save click keeps its existing + // behaviour (some dialogs, e.g. object properties, stay open after Save). + const onSaveClick = (closeOnSave) => { // Do nothing when there is no change or there is an error if ( !schemaState._changes || Object.keys(schemaState._changes).length === 0 || @@ -129,15 +134,17 @@ export default function SchemaDialogView({ setSaving(true); setLoaderText(schemaState.customLoadingText || gettext('Saving...')); + const onSaved = closeOnSave === true ? () => props.onClose?.() : undefined; + if (!schema.warningText) { - save(schemaState.changes(true)); + save(schemaState.changes(true), onSaved); return; } Notifier.confirm( gettext('Warning'), schema.warningText, - () => { save(schemaState.changes(true)); }, + () => { save(schemaState.changes(true), onSaved); }, () => { setSaving(false); setLoaderText(''); @@ -183,7 +190,7 @@ export default function SchemaDialogView({ // there is a validation error, so this is safe to call unconditionally. if ((e.ctrlKey || e.metaKey) && !e.altKey && e.key === 'Enter') { e.preventDefault(); - onSaveClick(); + onSaveClick(true); return; } diff --git a/web/regression/javascript/SchemaView/SchemaDialogViewKeyboard.spec.js b/web/regression/javascript/SchemaView/SchemaDialogViewKeyboard.spec.js index 171cd88e1b5..fc022a8809d 100644 --- a/web/regression/javascript/SchemaView/SchemaDialogViewKeyboard.spec.js +++ b/web/regression/javascript/SchemaView/SchemaDialogViewKeyboard.spec.js @@ -7,12 +7,32 @@ // ////////////////////////////////////////////////////////////// -import { act, fireEvent, render } from '@testing-library/react'; +import { act, fireEvent, render, waitFor } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; +import BaseUISchema from 'sources/SchemaView/base_schema.ui'; import SchemaView from '../../../pgadmin/static/js/SchemaView'; import { TestSchema } from './TestSchema.ui'; import { withBrowser } from '../genericFunctions'; +// A single required field is all the Ctrl/Cmd+Enter save-and-close tests +// need; TestSchema's nested tab and row collection require a lot more +// simulated input just to get to a savable state. +class MinimalSchema extends BaseUISchema { + constructor() { + super({field1: null}); + } + + get baseFields() { + return [ + { + id: 'field1', label: 'Field1', type: 'text', group: null, + mode: ['properties', 'edit', 'create'], disabled: false, visible: true, + }, + ]; + } +} + // Escape closes a dialog rendered as a dockable panel (issue #5691). The // handler sits on the dialog wrapper, which must not be memoized along with // the dialog body: the body only changes with the schema, the mode or the @@ -20,6 +40,7 @@ import { withBrowser } from '../genericFunctions'; // a memoized wrapper would keep calling whichever one it captured first. describe('SchemaDialogView keyboard handling', () => { const SchemaViewWithBrowser = withBrowser(SchemaView); + const user = userEvent.setup(); const dialog = (schema, onClose) => ( { /> ); - const renderDialog = async (onClose) => { + const renderDialog = async (onClose, onSave = jest.fn(() => Promise.resolve()), schema = new TestSchema()) => { let ctrl; await act(async () => { ctrl = render( Promise.resolve())} + onSave={onSave} onClose={onClose} onHelp={jest.fn()} onEdit={jest.fn()} @@ -65,6 +86,12 @@ describe('SchemaDialogView keyboard handling', () => { }); }; + const pressCtrlEnter = async (ctrl) => { + await act(async () => { + fireEvent.keyDown(ctrl.container.firstChild, {key: 'Enter', ctrlKey: true}); + }); + }; + it('closes the dialog on Escape', async () => { const onClose = jest.fn(); const ctrl = await renderDialog(onClose); @@ -99,4 +126,25 @@ describe('SchemaDialogView keyboard handling', () => { expect(secondOnClose).toHaveBeenCalled(); expect(firstOnClose).not.toHaveBeenCalled(); }); + + // Ctrl/Cmd+Enter is meant to save and close in one step (issue #7167). + // onSaveClick alone is not enough: it only calls onSave, so this covers the + // dialog actually closing once that save resolves. + it('saves and closes the dialog on Ctrl/Cmd+Enter', async () => { + const onClose = jest.fn(); + const onSave = jest.fn(() => Promise.resolve()); + const ctrl = await renderDialog(onClose, onSave, new MinimalSchema()); + + // Wait for the dialog's auto-focus to settle before typing, as the other + // SchemaDialogView specs do. + await act(async () => { + await new Promise(resolve => setTimeout(resolve, 500)); + }); + await user.type(ctrl.container.querySelector('[name="field1"]'), 'val1'); + + await pressCtrlEnter(ctrl); + + expect(onSave).toHaveBeenCalled(); + await waitFor(() => expect(onClose).toHaveBeenCalled()); + }); });