diff --git a/crates/cli/src/app.rs b/crates/cli/src/app.rs index 0ef66081..97d4be51 100644 --- a/crates/cli/src/app.rs +++ b/crates/cli/src/app.rs @@ -22694,27 +22694,68 @@ mod tests { } #[tokio::test] - async fn playbook_tab_focuses_selection_menu_for_multi_line_selection() { + async fn playbook_tab_indents_every_line_of_a_multi_line_selection() { let (mut app, _dir, server) = empty_app().await; - app.playbook_popup = Some(playbook_popup_for_test("s1", "- one\n- two", 0)); + app.playbook_popup = Some(playbook_popup_for_test("s1", "- one\n- two\n- three\n", 0)); // Select from the start of the buffer through the end of the second - // list line so both lines fall inside the selection. + // list line (char offset 11), so the first two lines fall inside the + // selection and the (unfocused) selection menu is showing — issue + // #1106's repro. The head is set directly because cursor stepping + // treats list markers as atomic, which makes step counts opaque here. app.begin_playbook_selection(); - app.move_playbook_cursor(11); + { + let popup = app.playbook_popup.as_mut().unwrap(); + popup.selection.as_mut().unwrap().head = 11; + popup.cursor = 11; + } app.handle_playbook_key(KeyEvent::new(KeyCode::Tab, KeyModifiers::NONE)) .await; + let popup = app.playbook_popup.as_ref().unwrap(); + assert_eq!( + popup.buffer, " - one\n - two\n- three\n", + "Tab nests every list line the selection spans (spec 0094)" + ); + assert!( + popup + .selection_menu + .as_ref() + .is_some_and(|menu| !menu.focused), + "the selection menu stays visible but must not steal Tab to focus itself" + ); + + // S-Tab is the symmetric outdent over the same (remapped) selection. + app.handle_playbook_key(KeyEvent::new(KeyCode::BackTab, KeyModifiers::SHIFT)) + .await; + let popup = app.playbook_popup.as_ref().unwrap(); + assert_eq!( + popup.buffer, "- one\n- two\n- three\n", + "S-Tab un-nests the same selected lines" + ); + server.abort(); + } + + #[tokio::test] + async fn playbook_ctrl_o_focuses_selection_menu_without_editing() { + let (mut app, _dir, server) = empty_app().await; + app.playbook_popup = Some(playbook_popup_for_test("s1", "- one\n- two", 0)); + app.begin_playbook_selection(); + app.move_playbook_cursor(11); + + app.handle_playbook_key(KeyEvent::new(KeyCode::Char('o'), KeyModifiers::CONTROL)) + .await; + let popup = app.playbook_popup.as_ref().unwrap(); assert_eq!( popup.buffer, "- one\n- two", - "Tab focuses the selected-text run menu instead of editing the selection" + "C-o only moves focus; the document is untouched" ); let menu = popup .selection_menu .as_ref() .expect("selection run menu should be present"); - assert!(menu.focused, "Tab should focus the selection run menu"); + assert!(menu.focused, "C-o should focus the selection run menu"); server.abort(); } @@ -22895,12 +22936,16 @@ mod tests { !text.contains("reduce to the minimum") && !text.contains("Execute the selection") && !text.contains("Free-text guidance"), - "no description shows before Tab focuses the menu: {text:?}" + "no description shows before C-o focuses the menu: {text:?}" + ); + assert!( + text.contains("C-o menu"), + "the unfocused menu must advertise its focus key (issue #1106): {text:?}" ); server.abort(); } - /// spec 0089: once Tab focuses the menu, the highlighted row's + /// spec 0089: once C-o focuses the menu, the highlighted row's /// description appears, and it updates as Up/Down moves the highlight /// to a different row instead of getting stuck on the first one shown. #[tokio::test] @@ -23449,10 +23494,10 @@ mod tests { assert_ne!( run_cell.style().bg, Some(app.theme.accent), - "Run button should not be highlighted before Tab focuses the menu" + "Run button should not be highlighted before C-o focuses the menu" ); - app.handle_playbook_key(KeyEvent::new(KeyCode::Tab, KeyModifiers::NONE)) + app.handle_playbook_key(KeyEvent::new(KeyCode::Char('o'), KeyModifiers::CONTROL)) .await; term.draw(|f| crate::ui::render(f, &mut app)) .expect("playbook should render after focus"); @@ -23461,13 +23506,13 @@ mod tests { .layout .playbook_selection_run_hit .expect("selection menu hit registered"); - let tab_focused_run_cell = buf + let focused_run_cell = buf .cell((hit.1.saturating_sub(3), hit.2)) - .expect("Run text cell right after Tab"); + .expect("Run text cell right after C-o"); assert_ne!( - tab_focused_run_cell.style().bg, + focused_run_cell.style().bg, Some(app.theme.accent), - "Tab focuses the comment row by default (spec 0089) — Run stays plain until Down selects it" + "C-o focuses the comment row by default (spec 0089) — Run stays plain until Down selects it" ); // Down moves keyboard selection off Comment onto Run (spec 0089). @@ -27120,7 +27165,7 @@ mod tests { app.begin_playbook_selection(); app.move_playbook_cursor(5); - app.handle_playbook_key(KeyEvent::new(KeyCode::Tab, KeyModifiers::NONE)) + app.handle_playbook_key(KeyEvent::new(KeyCode::Char('o'), KeyModifiers::CONTROL)) .await; for ch in "focus tests".chars() { app.handle_playbook_key(KeyEvent::new(KeyCode::Char(ch), KeyModifiers::NONE)) @@ -27160,7 +27205,7 @@ mod tests { app.begin_playbook_selection(); app.move_playbook_cursor(5); - app.handle_playbook_key(KeyEvent::new(KeyCode::Tab, KeyModifiers::NONE)) + app.handle_playbook_key(KeyEvent::new(KeyCode::Char('o'), KeyModifiers::CONTROL)) .await; for ch in "abcd".chars() { app.handle_playbook_key(KeyEvent::new(KeyCode::Char(ch), KeyModifiers::NONE)) @@ -27242,12 +27287,12 @@ mod tests { .selected_action }; - app.handle_playbook_key(KeyEvent::new(KeyCode::Tab, KeyModifiers::NONE)) + app.handle_playbook_key(KeyEvent::new(KeyCode::Char('o'), KeyModifiers::CONTROL)) .await; assert_eq!( selected_action(&app), PlaybookSelectionAction::Comment, - "Tab focuses the menu with Comment selected by default" + "C-o focuses the menu with Comment selected by default" ); app.handle_playbook_key(KeyEvent::new(KeyCode::Down, KeyModifiers::NONE)) diff --git a/crates/cli/src/app/editor.rs b/crates/cli/src/app/editor.rs index e3bb0b06..88ef60c1 100644 --- a/crates/cli/src/app/editor.rs +++ b/crates/cli/src/app/editor.rs @@ -1116,7 +1116,13 @@ impl App { .and_then(|popup| popup.selection_menu.as_ref()) .is_some_and(|menu| menu.focused); if !menu_focused { - if matches!(key.code, KeyCode::Tab) && !ctrl && !alt && !super_mod { + // C-o focuses the menu (advertised on its unfocused frame). It + // must NOT be Tab: while the menu is merely shown the editor's + // own keymap keeps working, and Tab / S-Tab there nest and + // un-nest every list line the selection spans (spec 0094) — + // claiming Tab here made multi-line indent unreachable while + // leaving S-Tab live, an asymmetry reported as issue #1106. + if ctrl_char == Some('o') && !alt && !super_mod { let Some(popup) = self.playbook_popup.as_mut() else { return true; }; diff --git a/crates/cli/src/ui.rs b/crates/cli/src/ui.rs index 6fd0b16c..7e71deda 100644 --- a/crates/cli/src/ui.rs +++ b/crates/cli/src/ui.rs @@ -18744,9 +18744,24 @@ fn render_playbook_selection_context_menu( .add_modifier(Modifier::UNDERLINED) }; let run_style = row_style(run_selected); - let block = Block::default() + let mut block = Block::default() .borders(Borders::ALL) .border_style(Style::default().fg(app.theme.border)); + // While the menu is passive, advertise the key that focuses it + // (issues #1106/#1092): every editing key — including Tab / S-Tab + // list nesting on the selected lines — keeps reaching the editor + // until C-o hands the menu focus, and nothing else on screen says so. + if !menu.focused { + block = block.title_bottom( + Line::from(Span::styled( + " C-o menu ", + Style::default() + .fg(app.theme.dim) + .add_modifier(Modifier::ITALIC), + )) + .right_aligned(), + ); + } f.render_widget(Clear, rect); f.render_widget(block, rect); if rect.height >= 3 { diff --git a/specs/0196-playbook-selection-menu-focus-model.md b/specs/0196-playbook-selection-menu-focus-model.md new file mode 100644 index 00000000..db5fdf0b --- /dev/null +++ b/specs/0196-playbook-selection-menu-focus-model.md @@ -0,0 +1,47 @@ +# 0196-playbook-selection-menu-focus-model + +Status: accepted +Date: 2026-08-10 +Area: ux +Scope: how keyboard focus moves between the playbook editor and the selection action menu while a selection is active. + +## Decision + +Making a selection in the playbook shows the selection action menu, but the +menu starts passive: every key — cursor movement, typing, and in particular +the Tab / S-Tab list nesting pair — keeps reaching the editor and operating +on the selection. The menu only starts consuming keys once the user focuses +it with a dedicated chord (`C-o` in the TUI), and the passive menu itself +advertises that chord on its frame. Esc returns focus to the editor without +dismissing the selection; the existing cancel keys still dismiss both. + +The focus chord must never be a key that has an editing meaning while a +selection exists. Tab is the canonical counterexample: selections are exactly +when multi-line list nesting is wanted, so a menu that claims Tab makes the +editor's advertised behavior unreachable, and claiming only Tab but not S-Tab +splits a symmetric pair. + +## Reason + +The first implementation focused the menu with bare Tab. That shadowed the +editor's "Tab nests every list line the selection spans" behavior while +leaving S-Tab live (issue #1106), and the unfocused menu advertised nothing, +so the focus model was undiscoverable and Enter fell through to the editor +with destructive results (issue #1092). A passive popup may not steal keys +the surface underneath it documents. + +## Consequences + +- Adding keys to the *unfocused* menu path requires proving the editor gives + that key no meaning while a selection is active. +- The advertised chord and the actual binding must not drift apart; the hint + lives on the menu frame so a rebinding forces the two to be updated + together. +- Accepted tradeoff: reaching the menu costs a chord rather than a single + Tab press. + +## Non-Goals + +- What Enter should do while the menu is merely shown (issue #1092's + remaining half) — this spec fixes who owns keys, not Enter's meaning. +- Web UI menu focus, which has its own pointer-first interaction model.