From e4138f305ee8df16bd61dd1a87be46b22a7e56fd Mon Sep 17 00:00:00 2001 From: Dave Page Date: Wed, 10 Jun 2026 13:55:26 +0100 Subject: [PATCH 1/6] 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 0121624f7dd..eab8f013aee 100644 --- a/docs/en_US/keyboard_shortcuts.rst +++ b/docs/en_US/keyboard_shortcuts.rst @@ -46,9 +46,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 | +----------------------------+--------------------+------------------------------------+ @@ -100,6 +100,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 79da8ed5123..33d47358bbf 100644 --- a/web/pgadmin/browser/register_browser_preferences.py +++ b/web/pgadmin/browser/register_browser_preferences.py @@ -185,9 +185,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 @@ -215,9 +215,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 e9779329903..b36383bb9aa 100644 --- a/web/pgadmin/browser/static/js/keyboard.js +++ b/web/pgadmin/browser/static/js/keyboard.js @@ -153,42 +153,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 c986e94e5a473e8225c4f7dbfd37a6d9e0832ada Mon Sep 17 00:00:00 2001 From: Dave Page Date: Mon, 17 Aug 2026 13:55:36 +0100 Subject: [PATCH 2/6] 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 b36383bb9aa..edf92aef140 100644 --- a/web/pgadmin/browser/static/js/keyboard.js +++ b/web/pgadmin/browser/static/js/keyboard.js @@ -166,8 +166,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]; @@ -179,7 +190,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 6ca950e2e5f0b88e7adc4623fed97491eaaf1d0b Mon Sep 17 00:00:00 2001 From: Dave Page Date: Thu, 20 Aug 2026 09:24:00 +0100 Subject: [PATCH 3/6] 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()); + }); }); From 0ae8a56053b165e27dbff25d0d820a6baae0aa33 Mon Sep 17 00:00:00 2001 From: Dave Page Date: Wed, 23 Sep 2026 16:04:47 +0100 Subject: [PATCH 4/6] Test the Mod-Shift-D duplicate-line binding in the SQL editor Covers both the single-line case and a multi-line selection, as asked for in review. --- .../components/CodeMirrorCustomEditor.spec.js | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/web/regression/javascript/components/CodeMirrorCustomEditor.spec.js b/web/regression/javascript/components/CodeMirrorCustomEditor.spec.js index 3f09b9ba732..7f8b74a8fa9 100755 --- a/web/regression/javascript/components/CodeMirrorCustomEditor.spec.js +++ b/web/regression/javascript/components/CodeMirrorCustomEditor.spec.js @@ -11,6 +11,7 @@ import { withTheme } from '../fake_theme'; import CodeMirror from 'sources/components/ReactCodeMirror'; import { syntaxTree } from '@codemirror/language'; +import { runScopeHandlers } from '@codemirror/view'; import { render } from '@testing-library/react'; @@ -320,4 +321,25 @@ describe('CodeMirrorCustomEditorView', ()=>{ expect(result.to).toBe(stmts[1].to); }); + + it('Mod-Shift-D duplicates the current line',()=>{ + cmRerender({value: 'select 1;\nselect 2;'}); + editor.dispatch({selection: {anchor: 0}}); + // jsdom does not report a Mac platform, so Mod is Ctrl here. + const handled = runScopeHandlers(editor, new KeyboardEvent('keydown', { + key: 'D', code: 'KeyD', keyCode: 68, ctrlKey: true, shiftKey: true, + }), 'editor'); + expect(handled).toBe(true); + expect(editor.state.doc.toString()).toEqual('select 1;\nselect 1;\nselect 2;'); + }); + + it('Mod-Shift-D duplicates every line in the selection',()=>{ + cmRerender({value: 'select 1;\nselect 2;\nselect 3;'}); + editor.dispatch({selection: {anchor: 0, head: 12}}); + runScopeHandlers(editor, new KeyboardEvent('keydown', { + key: 'D', code: 'KeyD', keyCode: 68, ctrlKey: true, shiftKey: true, + }), 'editor'); + expect(editor.state.doc.toString()).toEqual( + 'select 1;\nselect 2;\nselect 1;\nselect 2;\nselect 3;'); + }); }); From ee3b0d861ecc7ab2bdc399440adc4fbce7c8fa00 Mon Sep 17 00:00:00 2001 From: Dave Page Date: Wed, 23 Sep 2026 16:37:03 +0100 Subject: [PATCH 5/6] Ignore Ctrl/Cmd+Enter in a dialog whilst a save is in progress The Save button is disabled during a save, but the shortcut calls onSaveClick directly, so a second press could send the same changes again and close the dialog twice. --- .../static/js/SchemaView/SchemaDialogView.jsx | 5 +++- .../SchemaDialogViewKeyboard.spec.js | 23 +++++++++++++++++++ 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/web/pgadmin/static/js/SchemaView/SchemaDialogView.jsx b/web/pgadmin/static/js/SchemaView/SchemaDialogView.jsx index bf3067ccb3f..11d67206c9f 100644 --- a/web/pgadmin/static/js/SchemaView/SchemaDialogView.jsx +++ b/web/pgadmin/static/js/SchemaView/SchemaDialogView.jsx @@ -125,7 +125,10 @@ export default function SchemaDialogView({ // 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 + // Do nothing when a save is already in flight, when there is no change, + // or when there is an error. The Save button is disabled whilst saving, + // but the Ctrl/Cmd+Enter shortcut calls this directly. + if (schemaState.isSaving) return; if ( !schemaState._changes || Object.keys(schemaState._changes).length === 0 || schemaState.errors.name diff --git a/web/regression/javascript/SchemaView/SchemaDialogViewKeyboard.spec.js b/web/regression/javascript/SchemaView/SchemaDialogViewKeyboard.spec.js index fc022a8809d..29d6129fc08 100644 --- a/web/regression/javascript/SchemaView/SchemaDialogViewKeyboard.spec.js +++ b/web/regression/javascript/SchemaView/SchemaDialogViewKeyboard.spec.js @@ -147,4 +147,27 @@ describe('SchemaDialogView keyboard handling', () => { expect(onSave).toHaveBeenCalled(); await waitFor(() => expect(onClose).toHaveBeenCalled()); }); + + // A second Ctrl/Cmd+Enter whilst the first save is still in flight must not + // start another save: the Save button is disabled during a save, but the + // shortcut reaches onSaveClick directly. + it('ignores Ctrl/Cmd+Enter whilst a save is in progress', async () => { + const onClose = jest.fn(); + let resolveSave; + const onSave = jest.fn(() => new Promise(resolve => { resolveSave = resolve; })); + const ctrl = await renderDialog(onClose, onSave, new MinimalSchema()); + + await act(async () => { + await new Promise(resolve => setTimeout(resolve, 500)); + }); + await user.type(ctrl.container.querySelector('[name="field1"]'), 'val1'); + + await pressCtrlEnter(ctrl); + await pressCtrlEnter(ctrl); + + expect(onSave).toHaveBeenCalledTimes(1); + + await act(async () => { resolveSave(); }); + await waitFor(() => expect(onClose).toHaveBeenCalledTimes(1)); + }); }); From cedf2e3e634e32fb2007782117025834e4f6340c Mon Sep 17 00:00:00 2001 From: Dave Page Date: Thu, 24 Sep 2026 11:42:07 +0100 Subject: [PATCH 6/6] Test that Escape consumed by an inner control leaves the dialog open --- .../SchemaDialogViewKeyboard.spec.js | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/web/regression/javascript/SchemaView/SchemaDialogViewKeyboard.spec.js b/web/regression/javascript/SchemaView/SchemaDialogViewKeyboard.spec.js index 29d6129fc08..5eb3f26e67e 100644 --- a/web/regression/javascript/SchemaView/SchemaDialogViewKeyboard.spec.js +++ b/web/regression/javascript/SchemaView/SchemaDialogViewKeyboard.spec.js @@ -101,6 +101,24 @@ describe('SchemaDialogView keyboard handling', () => { expect(onClose).toHaveBeenCalled(); }); + // An inner control that handles Escape itself (react-select closing an open + // menu, for example) calls preventDefault, and the event still bubbles up + // to the dialog, which must then leave the dialog open. + it('does not close when an inner control consumes Escape', async () => { + const onClose = jest.fn(); + const ctrl = await renderDialog(onClose, undefined, new MinimalSchema()); + const control = ctrl.container.querySelector('[name="field1"]'); + control.addEventListener( + 'keydown', (event) => event.preventDefault(), {once: true} + ); + + await act(async () => { + fireEvent.keyDown(control, {key: 'Escape'}); + }); + + expect(onClose).not.toHaveBeenCalled(); + }); + it('calls the current onClose, not the one from the first render', async () => { const firstOnClose = jest.fn();