From ef2e14c17372f2629cbee10343ef328fff2173e4 Mon Sep 17 00:00:00 2001 From: Oss Fixer Date: Wed, 28 Oct 2026 00:23:56 +0000 Subject: [PATCH] :bug: Fix TUI Escape stranding focus in home screen (issue #2) Navigation could require an extra Tab/Down because focus sometimes rested on a reblessed Form container (non-actionable): - escaping the selected-host dialog now cancels it and returns focus to the host list, and escaping main actions returns to the primary action; - the dialog's internal Form selection cursor is reset on every reopen so the first Tab lands on the next action instead of being a no-op; - Escape is bound per view mount instead of subscribing once per 'Select host' press on the session-wide emitter (listeners accumulated and kept stealing focus with stale closures); - add node-based regression tests for the focus flows. Co-Authored-By: Claude Fable 5 --- package.json | 3 +- src/tui/views/home.js | 36 ++++- tests/tui-home-focus.test.mjs | 245 ++++++++++++++++++++++++++++++++++ 3 files changed, 277 insertions(+), 7 deletions(-) create mode 100644 tests/tui-home-focus.test.mjs diff --git a/package.json b/package.json index ca1cd57..b052f91 100644 --- a/package.json +++ b/package.json @@ -30,7 +30,8 @@ }, "scripts": { "keygen": "ssh-keygen -f host.rsa.key -N '' && ssh-keygen -t ed25519 -f host.ed25519.key -N '' && rm *.pub", - "start": "node main.js" + "start": "node main.js", + "test": "node --test tests/" }, "type": "module" } diff --git a/src/tui/views/home.js b/src/tui/views/home.js index 71c890b..dcd39fb 100644 --- a/src/tui/views/home.js +++ b/src/tui/views/home.js @@ -13,12 +13,12 @@ export default ( ) => ({ hosts, callback, - isTotpEnabled, - emitter + isTotpEnabled }) => { let n = 3; const - mainActionsFormContainer = flexContainer(form(screen)), + mainActionsForm = form(screen), + mainActionsFormContainer = flexContainer(mainActionsForm), selectButton = button(mainActionsFormContainer, { content: getString('home.selectHost'), hidden: hosts.length === 0 }), addButton = button(mainActionsFormContainer, { content: getString('addHost') }); addButton.on('press', () => callback({ action: 'add-host' })); @@ -30,6 +30,13 @@ export default ( .on('press', () => callback({ action: 'delete-account' })); button(mainActionsFormContainer, { content: getString('quit') }) .on('press', () => callback({ action: 'quit' })); + // Same reblessed Form Escape workaround for the main actions: registered after + // the Form's built-in Escape->self-focus handler (so it has the final word), it + // returns focus to the primary action instead of the non-actionable container. + mainActionsForm.on('element keypress', (element, chars, key) => { + if(key.name === 'escape') + (hosts.length ? selectButton : addButton).focus(); + }); let selectedHost; const tableHeight = screen.height - 15, @@ -47,9 +54,16 @@ export default ( ]) } ); + // The host list is built per view mount (the dashboard remounts this view on + // every navigation cycle), so leave the list with its own Escape binding + // instead of subscribing to the session-wide emitter, where stale closures + // would accumulate and keep stealing focus (issue #2). + hostsTable.rows.on('element keypress', (element, chars, key) => { + if(key.name === 'escape') + selectButton.focus(); + }); selectButton.on('press', () => { hostsTable.focus(); - emitter.on('escape', () => selectButton.focus()); }); const selectedHostForm = form( @@ -70,15 +84,25 @@ export default ( button(selectedHostActions, { content: getString('home.edit') }) .on('press', () => callback({ action: 'edit-host', host: selectedHost })); const removeButton = button(selectedHostActions, { content: getString('home.remove') }, { color: 'yellow' }); - cancelButton.on('press', () => { + const cancelSelectedHostForm = () => { selectedHostForm.hide(); + removeButton.setContent(getString('home.remove')); screen.render(); hostsTable.focus(); - removeButton.setContent(getString('home.remove')); + }; + cancelButton.on('press', cancelSelectedHostForm); + // reblessed Forms steal Escape to focus their own, non-actionable container. + // This listener is registered after the Form's built-in one, so it has the final + // word and Escape actually cancels the dialog instead of stranding focus where + // the next Tab/Down would be consumed without moving (issue #2). + selectedHostForm.on('element keypress', (element, chars, key) => { + if(key.name === 'escape') + cancelSelectedHostForm(); }); hostsTable.rows.on('select', (undefined, index) => { selectedHost = hosts[index]; selectedHostText.setContent(`${getString('home.selectedHost')} : ${selectedHost.name}`); + selectedHostForm.resetSelected(); selectedHostForm.show(); screen.render(); cancelButton.focus(); diff --git a/tests/tui-home-focus.test.mjs b/tests/tui-home-focus.test.mjs new file mode 100644 index 0000000..609947a --- /dev/null +++ b/tests/tui-home-focus.test.mjs @@ -0,0 +1,245 @@ +// Regression tests for KaKi87/sshception issue #2: +// "[TUI] Navigation requiring tab/arrow down twice when moving focus to next element" +// +// The TUI is a reblessed (https://github.com/kenan238/reblessed) screen driven by an +// SSH client PTY, so focus navigation is exercised the same way: a virtual terminal +// input stream is fed raw key sequences and the focused widget is asserted. +// +// Run with: node --test tests/ + +import { test } from 'node:test'; +import assert from 'node:assert/strict'; +import { PassThrough } from 'node:stream'; + +import blessed from 'blessed'; +import { createEmitter } from 'simpler-emitter'; + +import { home } from '../src/tui/views/main.js'; + +const hosts = [ + { name: 'host1', address: '192.0.2.10', port: 22, username: 'u1', password: 'p1' }, + { name: 'host2', address: '192.0.2.20', port: 22, username: 'u2', password: 'p2' } +]; + +const ENTER = '\r'; +const TAB = '\t'; +const SHIFT_TAB = '\x1b[Z'; +const ESCAPE = '\x1b'; +const DOWN = '\x1b[B'; +const UP = '\x1b[A'; + +const boot = ({ hostList = hosts, emitter: providedEmitter } = {}) => { + const input = new PassThrough(); + const output = new PassThrough(); + output.on('data', () => {}); + + const screen = blessed.screen({ + title: 'SSHception-test', + smartCSR: true, + terminal: 'xterm-256color', + input, + output, + autoPadding: true, + width: 100, + height: 30 + }); + + // Mirror src/tui/main.js: Escape is forwarded globally to the emitter bus. + const emitter = providedEmitter || createEmitter(); + screen.on('keypress', (undefined, { name }) => { + if(name === 'escape') + emitter.emit('escape'); + }); + + home(screen)({ + hosts: hostList, + isTotpEnabled: false, + emitter, + callback: () => {} + }); + + return { + screen, + emitter, + press: async sequence => { + input.write(sequence); + await new Promise(resolve => setTimeout(resolve, 25)); + } + }; +}; + +const textOf = element => (element?.content || '').toString().replace(/\s+/g, ' ').trim(); + +const containsButton = (element, label) => { + if(element.type === 'button' && textOf(element) === label) + return true; + return (element.children || []).some(child => containsButton(child, label)); +}; + +const find = (screen, predicate) => { + let found; + const walk = element => { + if(!found && predicate(element)) + found = element; + element.children?.forEach(walk); + }; + screen.children.forEach(walk); + return found; +}; + +const findHostList = screen => find(screen, element => element.type === 'list'); + +const findSelectedHostForm = screen => + find(screen, element => element.type === 'form' && containsButton(element, 'Cancel')); + +const focusedLabel = screen => { + const element = screen.focused; + return element ? `${element.type}:${textOf(element).slice(0, 30)}` : ''; +}; + +test('selecting a host and pressing Escape cancels the dialog in one step', async () => { + const { screen, press } = boot(); + + await press(ENTER); // "Select host" + const hostList = findHostList(screen); + assert.equal(screen.focused, hostList, 'host list gets focus when selecting a host'); + + await press(ENTER); // select first host + const selectedHostForm = findSelectedHostForm(screen); + assert.equal(selectedHostForm.visible, true, 'selected-host actions are shown'); + assert.equal(textOf(screen.focused), 'Cancel', 'Cancel is the focused action'); + + await press(ESCAPE); + assert.equal(selectedHostForm.visible, false, 'Escape hides the selected-host dialog'); + assert.equal(screen.focused, hostList, 'Escape returns focus to the host list'); + assert.equal(screen.focused.visible, true, 'nothing invisible is focused afterwards'); + + screen.destroy(); +}); + +test('Tab and arrow move to an actionable element on every press, also after reopening', async () => { + const { screen, press } = boot(); + const hostList = findHostList(screen); + + await press(ENTER); // "Select host" -> host list + assert.equal(screen.focused, hostList, 'starts inside the host list'); + + for(let attempt = 0; attempt < 2; attempt++){ + await press(ENTER); // select first host row -> Cancel + await press(TAB); + assert.equal(textOf(screen.focused), 'Connect', `Tab #1 (attempt ${attempt + 1})`); + await press(TAB); + assert.equal(textOf(screen.focused), 'Edit', `Tab #2 (attempt ${attempt + 1})`); + await press(TAB); + assert.equal(textOf(screen.focused), 'Remove', `Tab #3 (attempt ${attempt + 1})`); + await press(TAB); + assert.equal(textOf(screen.focused), 'Cancel', `Tab #4 wraps back (attempt ${attempt + 1})`); + await press(SHIFT_TAB); + assert.equal(textOf(screen.focused), 'Remove', 'Shift-Tab moves backwards'); + await press(UP); + assert.equal(textOf(screen.focused), 'Edit', 'Up moves backwards'); + await press(DOWN); + assert.equal(textOf(screen.focused), 'Remove', 'Down moves forwards'); + + await press(ESCAPE); + assert.equal(screen.focused, hostList, `Escape lands back on the list (attempt ${attempt + 1})`); + assert.notEqual(screen.focused.type, 'form', 'focus never rests on a form container'); + } + + screen.destroy(); +}); + +test('Escape from main actions or the host list reaches an actionable element', async () => { + const { screen, press } = boot(); + + assert.equal(textOf(screen.focused), 'Select host'); + await press(TAB); + assert.equal(textOf(screen.focused), 'Add host'); + await press(ESCAPE); + assert.equal(textOf(screen.focused), 'Select host', 'escape from Add host selects the primary action'); + + await press(ENTER); // into host list + await press(ESCAPE); + assert.equal(textOf(screen.focused), 'Select host', 'escape from the host list restores Select host'); + assert.equal(screen.focused.visible, true); + + screen.destroy(); +}); + +test('Escape bindings are mount-local: repeated home mounts add no global listeners', async () => { + // src/tui/controllers/dashboard.js remounts the home view (same screen, same + // session-wide emitter) every time the user returns from a sub-screen. The old + // code subscribed a new emitter listener on every "Select host" press, piling up + // stale closures; the view must not subscribe to the shared emitter at all. + const emitter = createEmitter(); + let escapeSubscriptions = 0; + emitter.on = (event, listener) => { + if(event === 'escape') + escapeSubscriptions++; + return undefined; + }; + + const input = new PassThrough(); + const output = new PassThrough(); + output.on('data', () => {}); + const screen = blessed.screen({ + title: 'SSHception-test', + smartCSR: true, + terminal: 'xterm-256color', + input, + output, + autoPadding: true, + width: 100, + height: 30 + }); + screen.on('keypress', (undefined, { name }) => { + if(name === 'escape') + emitter.emit('escape'); + }); + + for(let mount = 0; mount < 3; mount++) + home(screen)({ hosts, isTotpEnabled: false, emitter, callback: () => {} }); + + assert.equal(escapeSubscriptions, 0, `home must own no shared Emitter listeners, found ${escapeSubscriptions}`); + + // Drive the live (latest) mount and check focus semantics, not widget identity: + // previous mounts remain stacked as stale, hidden siblings. + input.write(ENTER); + await new Promise(resolve => setTimeout(resolve, 25)); + assert.equal(screen.focused.type, 'list', 'Select host opens the host list on the live mount'); + assert.equal(screen.focused.visible, true); + input.write(ESCAPE); + await new Promise(resolve => setTimeout(resolve, 25)); + assert.equal(textOf(screen.focused), 'Select host', 'Escape from the list is handled on the live mount'); + assert.equal(screen.focused.visible, true); + + screen.destroy(); +}); + +test('with no hosts, traversal stays on visible actionable elements only', async () => { + const { screen, press } = boot({ hostList: [] }); + const expected = [ 'Add host', 'Enable TOTP', 'Edit user/pass', 'Delete account', 'Quit', 'Add host' ]; + + assert.equal(screen.focused.visible, true); + for(let i = 0; i < expected.length; i++){ + await press(DOWN); + assert.equal(screen.focused.visible, true, `Down #${i + 1} reaches an actionable element: ${focusedLabel(screen)}`); + assert.notEqual(screen.focused.type, 'form', 'focus never rests on the non-actionable form container'); + assert.equal(textOf(screen.focused), expected[i], `navigation order step ${i + 1}`); + } + + screen.destroy(); +}); + +test('the hidden selected-host dialog is never focusable', async () => { + const { screen, press } = boot(); + const selectedHostForm = findSelectedHostForm(screen); + + for(let i = 0; i < 8; i++){ + await press(TAB); + assert.notEqual(screen.focused, selectedHostForm, 'hidden dialog form skipped'); + assert.notEqual(screen.focused.parent, selectedHostForm, 'hidden dialog buttons skipped'); + } + + screen.destroy(); +}); -- 2.47.3