From 1cea5261a1da89ccad19f889d61053f3a13bd39a Mon Sep 17 00:00:00 2001 From: Ben Brandt Date: Thu, 17 Sep 2026 14:39:57 +0000 Subject: [PATCH] agent_ui: Fix keyboard navigation in ACP elicitation forms (#64193) Closes #61358 Address the keyboard-navigation portions. Image answers are intentionally not included. ACP form responses support primitive values rather than image attachments, and Claude Code's adapter treats the "Other" answer as text. This PR therefore only partially addresses the issue. Release Notes: - acp: Fixed submitting and navigating ACP elicitation forms with the keyboard. --- assets/keymaps/default-linux.json | 9 + assets/keymaps/default-macos.json | 9 + assets/keymaps/default-windows.json | 9 + .../src/conversation_view/elicitation.rs | 397 +++++++++++++++++- .../ui/src/components/button/button_like.rs | 6 +- 5 files changed, 417 insertions(+), 13 deletions(-) diff --git a/assets/keymaps/default-linux.json b/assets/keymaps/default-linux.json index 62f12b87b4..20be29977a 100644 --- a/assets/keymaps/default-linux.json +++ b/assets/keymaps/default-linux.json @@ -379,6 +379,15 @@ "enter": "editor::Newline", }, }, + { + "context": "Elicitation || (Elicitation > Editor)", + "use_key_equivalents": true, + "bindings": { + "enter": "menu::Confirm", + "tab": "menu::SelectNext", + "shift-tab": "menu::SelectPrevious", + }, + }, { "context": "ThreadHistory", "bindings": { diff --git a/assets/keymaps/default-macos.json b/assets/keymaps/default-macos.json index f436e50eee..fce589d760 100644 --- a/assets/keymaps/default-macos.json +++ b/assets/keymaps/default-macos.json @@ -414,6 +414,15 @@ "enter": "editor::Newline", }, }, + { + "context": "Elicitation || (Elicitation > Editor)", + "use_key_equivalents": true, + "bindings": { + "enter": "menu::Confirm", + "tab": "menu::SelectNext", + "shift-tab": "menu::SelectPrevious", + }, + }, { "context": "ThreadHistory", "bindings": { diff --git a/assets/keymaps/default-windows.json b/assets/keymaps/default-windows.json index fff87c886b..77a49442de 100644 --- a/assets/keymaps/default-windows.json +++ b/assets/keymaps/default-windows.json @@ -382,6 +382,15 @@ "enter": "editor::Newline", }, }, + { + "context": "Elicitation || (Elicitation > Editor)", + "use_key_equivalents": true, + "bindings": { + "enter": "menu::Confirm", + "tab": "menu::SelectNext", + "shift-tab": "menu::SelectPrevious", + }, + }, { "context": "ThreadHistory", "use_key_equivalents": true, diff --git a/crates/agent_ui/src/conversation_view/elicitation.rs b/crates/agent_ui/src/conversation_view/elicitation.rs index 4efb8f8b38..872eafbb4e 100644 --- a/crates/agent_ui/src/conversation_view/elicitation.rs +++ b/crates/agent_ui/src/conversation_view/elicitation.rs @@ -4,7 +4,7 @@ use collections::{HashMap, HashSet}; use component::{Component, ComponentScope, example_group_with_title, single_example}; use editor::Editor; use futures::channel::oneshot; -use gpui::{AnyElement, App, Div, Empty, Entity, Hsla, SharedString, Window, div}; +use gpui::{AnyElement, App, Div, Empty, Entity, Focusable, Hsla, SharedString, Window, div}; use std::collections::BTreeMap; use std::rc::Rc; use ui::{ @@ -363,7 +363,352 @@ impl ElicitationFormSubmission { #[cfg(test)] mod tests { use super::*; - use gpui::TestAppContext; + use gpui::{KeyUpEvent, Keystroke, PlatformInput, TestAppContext, VisualTestContext}; + use std::cell::RefCell; + + struct TestElicitationView { + elicitation: Elicitation, + form_state: ElicitationFormState, + events: Rc>>, + } + + impl TestElicitationView { + fn new(schema: acp::ElicitationSchema, window: &mut Window, cx: &mut App) -> Self { + Self { + form_state: ElicitationFormState::new(&schema, window, cx), + elicitation: Elicitation { + id: ElicitationEntryId("keyboard-test".into()), + request: acp::CreateElicitationRequest::new( + acp::ElicitationFormMode::new(preview_request_scope(0), schema), + "Choose an answer.", + ), + status: pending_status(), + }, + events: Rc::default(), + } + } + + fn editor(&self, field_name: &str) -> Entity { + match self.form_state.fields.get(field_name) { + Some(ElicitationFieldState::Text(editor)) => editor.clone(), + _ => panic!("expected a text field named {field_name}"), + } + } + } + + impl Render for TestElicitationView { + fn render(&mut self, _window: &mut Window, cx: &mut Context) -> impl IntoElement { + let response_handler = |response: &'static str| -> RespondHandler { + let events = self.events.clone(); + let expected_id = self.elicitation.id.clone(); + Rc::new(move |id, _, _| { + assert_eq!(id, expected_id); + events.borrow_mut().push(response.into()); + }) + }; + let handlers = ElicitationCardHandlers { + on_submit: response_handler("submit"), + on_decline: response_handler("decline"), + on_cancel: response_handler("cancel"), + on_dismiss_url: response_handler("dismiss"), + on_open_url: { + let events = self.events.clone(); + let expected_id = self.elicitation.id.clone(); + Rc::new(move |id, url, _, _| { + assert_eq!(id, expected_id); + events.borrow_mut().push(format!("open: {url}")); + }) + }, + on_boolean_change: { + let events = self.events.clone(); + Rc::new(move |_, name, value, _| { + events.borrow_mut().push(format!("{name}: {value}")); + }) + }, + on_single_select_change: { + let events = self.events.clone(); + Rc::new(move |_, name, value, _| { + events.borrow_mut().push(format!("{name}: {value}")); + }) + }, + on_multi_select_change: { + let events = self.events.clone(); + Rc::new(move |_, name, value, selected, _| { + events + .borrow_mut() + .push(format!("{name}: {value} = {selected}")); + }) + }, + }; + + div() + .key_context("AcpThread") + .on_action(|_: &crate::CycleModeSelector, _, _| { + panic!("form navigation must not cycle the agent mode"); + }) + .on_action(|_: &zed_actions::agent::Chat, _, _| { + panic!("form submission must not send a chat message"); + }) + .child( + ElicitationCard::new( + 0, + &self.elicitation, + "Claude Code".into(), + Some(&self.form_state), + handlers, + ) + .render(cx), + ) + } + } + + fn init_keyboard_test(cx: &mut TestAppContext) { + crate::conversation_view::tests::init_test(cx); + cx.update(|cx| { + let bindings = settings::KeymapFile::load_asset_allow_partial_failure( + "keymaps/default-linux.json", + cx, + ) + .expect("default keymap should load"); + cx.bind_keys(bindings); + }); + } + + fn press_keys(cx: &mut VisualTestContext, keys: &str) { + for key in keys.split_whitespace() { + cx.simulate_keystrokes(key); + cx.update(|window, cx| { + // Click handlers activate buttons on key release, not key press. + window.dispatch_event( + PlatformInput::KeyUp(KeyUpEvent { + keystroke: Keystroke::parse(key).expect("valid test keystroke"), + }), + cx, + ); + }); + cx.run_until_parked(); + } + } + + #[gpui::test] + fn form_text_fields_submit_on_enter(cx: &mut TestAppContext) { + init_keyboard_test(cx); + let (view, cx) = cx.add_window_view(|window, cx| { + TestElicitationView::new( + acp::ElicitationSchema::new() + .string("other", true) + .property("count", acp::IntegerPropertySchema::new(), true) + .property("amount", acp::NumberPropertySchema::new(), true), + window, + cx, + ) + }); + let events = view.read_with(cx, |view, _| view.events.clone()); + + for (field_name, value) in [ + ("other", " My answer "), + ("count", "2"), + ("amount", "1.5"), + ] { + let editor = view.read_with(cx, |view, _| view.editor(field_name)); + cx.update(|window, cx| window.focus(&editor.focus_handle(cx), cx)); + cx.simulate_input(value); + press_keys(cx, "enter"); + assert_eq!(editor.read_with(cx, |editor, cx| editor.text(cx)), value); + } + assert_eq!(*events.borrow(), ["submit", "submit", "submit"]); + + view.update(cx, |view, cx| { + assert!(view.form_state.begin_submission(cx).is_some()); + cx.notify(); + }); + press_keys(cx, "enter"); + assert_eq!(events.borrow().len(), 3); + } + + #[gpui::test] + fn form_tab_navigation_reaches_fields_and_actions(cx: &mut TestAppContext) { + init_keyboard_test(cx); + let (view, cx) = cx.add_window_view(|window, cx| { + TestElicitationView::new( + acp::ElicitationSchema::new() + .string("first", false) + .string("other", false), + window, + cx, + ) + }); + let (first, other, events) = view.read_with(cx, |view, _| { + ( + view.editor("first"), + view.editor("other"), + view.events.clone(), + ) + }); + cx.update(|window, cx| window.focus(&first.focus_handle(cx), cx)); + + press_keys(cx, "tab"); + cx.update(|window, cx| assert!(other.focus_handle(cx).is_focused(window))); + press_keys(cx, "shift-tab"); + cx.update(|window, cx| assert!(first.focus_handle(cx).is_focused(window))); + + press_keys(cx, "tab tab"); + let submit_focus = + cx.update(|window, cx| window.focused(cx).expect("Submit should be focused")); + press_keys(cx, "shift-tab"); + cx.update(|window, cx| assert!(other.focus_handle(cx).is_focused(window))); + press_keys(cx, "tab"); + cx.update(|window, _| assert!(submit_focus.is_focused(window))); + press_keys(cx, "enter tab enter tab space"); + assert_eq!(*events.borrow(), ["submit", "decline", "cancel"]); + + press_keys(cx, "shift-tab space"); + assert_eq!(*events.borrow(), ["submit", "decline", "cancel", "decline"]); + let decline_focus = + cx.update(|window, cx| window.focused(cx).expect("Decline should be focused")); + + cx.update(|window, cx| window.focus(&submit_focus, cx)); + view.update(cx, |view, cx| { + assert!(view.form_state.begin_submission(cx).is_some()); + cx.notify(); + }); + press_keys(cx, "tab"); + cx.update(|window, _| assert!(decline_focus.is_focused(window))); + press_keys(cx, "shift-tab"); + cx.update(|window, cx| assert!(other.focus_handle(cx).is_focused(window))); + press_keys(cx, "tab"); + cx.update(|window, _| assert!(decline_focus.is_focused(window))); + press_keys(cx, "shift-tab"); + cx.update(|window, cx| assert!(other.focus_handle(cx).is_focused(window))); + + view.update(cx, |view, cx| { + view.form_state.set_errors(HashMap::default()); + cx.notify(); + }); + press_keys(cx, "tab"); + cx.update(|window, _| assert!(submit_focus.is_focused(window))); + } + + #[gpui::test] + fn form_choices_are_keyboard_accessible(cx: &mut TestAppContext) { + init_keyboard_test(cx); + let (view, cx) = cx.add_window_view(|window, cx| { + TestElicitationView::new( + acp::ElicitationSchema::new() + .string("answer", false) + .property("boolean", acp::BooleanPropertySchema::new(), false) + .property( + "choice", + acp::StringPropertySchema::new() + .enum_values(vec!["first".into(), "second".into()]), + false, + ) + .property( + "multiple", + acp::MultiSelectPropertySchema::new(vec!["first".into(), "second".into()]), + false, + ), + window, + cx, + ) + }); + let (editor, events) = + view.read_with(cx, |view, _| (view.editor("answer"), view.events.clone())); + cx.update(|window, cx| window.focus(&editor.focus_handle(cx), cx)); + press_keys( + cx, + "tab space tab enter tab space tab space tab enter tab enter", + ); + assert_eq!( + *events.borrow(), + [ + "boolean: true", + "choice: first", + "choice: second", + "multiple: first = true", + "multiple: second = true", + "submit", + ] + ); + } + + #[gpui::test] + fn tab_navigation_between_form_and_url_cards(cx: &mut TestAppContext) { + struct Cards { + form: Entity, + url: Entity, + } + + impl Render for Cards { + fn render( + &mut self, + _window: &mut Window, + _cx: &mut Context, + ) -> impl IntoElement { + v_flex().child(self.form.clone()).child(self.url.clone()) + } + } + + init_keyboard_test(cx); + let (cards, cx) = cx.add_window_view(|window, cx| Cards { + form: cx.new(|cx| { + TestElicitationView::new( + acp::ElicitationSchema::new().string("other", false), + window, + cx, + ) + }), + url: cx.new(|cx| { + let mut view = TestElicitationView::new(acp::ElicitationSchema::new(), window, cx); + view.elicitation.id = ElicitationEntryId("url-test".into()); + view.elicitation.request = acp::CreateElicitationRequest::new( + acp::ElicitationUrlMode::new( + preview_request_scope(1), + acp::ElicitationId::new("url-test"), + "https://example.com/authorize", + ), + "Authorize access.", + ); + view + }), + }); + let (form, url) = cards.read_with(cx, |cards, _| (cards.form.clone(), cards.url.clone())); + let (editor, form_events) = + form.read_with(cx, |form, _| (form.editor("other"), form.events.clone())); + let url_events = url.read_with(cx, |url, _| url.events.clone()); + cx.update(|window, cx| window.focus(&editor.focus_handle(cx), cx)); + + press_keys(cx, "tab tab tab enter"); + assert_eq!(*form_events.borrow(), ["cancel"]); + press_keys(cx, "tab enter"); + assert_eq!( + *url_events.borrow(), + ["open: https://example.com/authorize", "submit"] + ); + + press_keys(cx, "shift-tab enter"); + assert_eq!(*form_events.borrow(), ["cancel", "cancel"]); + press_keys(cx, "shift-tab shift-tab enter"); + assert_eq!(*form_events.borrow(), ["cancel", "cancel", "submit"]); + press_keys(cx, "shift-tab"); + cx.update(|window, cx| assert!(editor.focus_handle(cx).is_focused(window))); + + url.update(cx, |url, cx| { + url.elicitation.status = ElicitationStatus::Accepted; + cx.notify(); + }); + press_keys(cx, "tab tab tab tab enter tab enter"); + assert_eq!( + *url_events.borrow(), + [ + "open: https://example.com/authorize", + "submit", + "open: https://example.com/authorize", + "dismiss", + ] + ); + assert_eq!(*form_events.borrow(), ["cancel", "cancel", "submit"]); + } #[test] fn string_validation_rejects_email_format_mismatch() { @@ -1489,6 +1834,10 @@ impl<'a> ElicitationCard<'a> { }; v_flex() + .key_context("Elicitation") + .tab_group() + .on_action(|_: &menu::SelectNext, window, cx| window.focus_next(cx)) + .on_action(|_: &menu::SelectPrevious, window, cx| window.focus_prev(cx)) .mx_5() .my_1p5() .rounded_md() @@ -1576,6 +1925,7 @@ impl<'a> ElicitationCard<'a> { let label = property_title(field_name, property); let description = property_description(property); let border_color = cx.theme().colors().border.opacity(0.8); + let focused_border_color = cx.theme().colors().border_focused; let field_border_color = if error.is_some() { Color::Error.color(cx) } else { @@ -1606,10 +1956,12 @@ impl<'a> ElicitationCard<'a> { .child( h_flex() .id(row_id) + .tab_index(0) .w_full() .items_start() .gap_1() .cursor_pointer() + .focus_visible(|this| this.bg(cx.theme().colors().element_hover)) .on_click(move |_, _window, cx| { on_boolean_change( elicitation_id.clone(), @@ -1655,16 +2007,29 @@ impl<'a> ElicitationCard<'a> { ) }) .child(match field { - ElicitationFieldState::Text(editor) => div() - .rounded_sm() - .border_1() - .border_color(field_border_color) - .bg(editor_background) - .px_1() - .py_0p5() - .text_xs() - .child(editor.clone().into_any_element()) - .into_any_element(), + ElicitationFieldState::Text(editor) => { + let on_submit = self.handlers.on_submit.clone(); + let elicitation_id = self.elicitation.id.clone(); + let is_submitting = self.form_state.is_some_and(|state| state.is_submitting); + + div() + .track_focus(&editor.focus_handle(cx).tab_stop(true)) + .on_action(move |_: &menu::Confirm, window, cx| { + if !is_submitting { + on_submit(elicitation_id.clone(), window, cx); + } + }) + .rounded_sm() + .border_1() + .border_color(field_border_color) + .focus_visible(|this| this.border_color(focused_border_color)) + .bg(editor_background) + .px_1() + .py_0p5() + .text_xs() + .child(editor.clone().into_any_element()) + .into_any_element() + } ElicitationFieldState::Boolean(_) => Empty.into_any_element(), ElicitationFieldState::SingleSelect { value } => { let options = match property { @@ -1714,6 +2079,7 @@ impl<'a> ElicitationCard<'a> { "elicitation-multi-option-{}-{field_name}-{}", self.entry_ix, option.value ))) + .tab_index(0) .w_full() .min_h(rems_from_px(28_f32)) .items_start() @@ -1725,6 +2091,7 @@ impl<'a> ElicitationCard<'a> { .px_2() .py_1() .hover(move |this| this.bg(hover_background).cursor_pointer()) + .focus_visible(|this| this.border_color(focused_border_color)) .on_click(move |_, _window, cx| { on_multi_select_change( elicitation_id.clone(), @@ -1781,6 +2148,7 @@ impl<'a> ElicitationCard<'a> { h_flex() .id(option_id) + .tab_index(0) .w_full() .min_h(rems_from_px(28_f32)) .items_start() @@ -1792,6 +2160,7 @@ impl<'a> ElicitationCard<'a> { .px_2() .py_1() .hover(move |this| this.bg(hover_background).cursor_pointer()) + .focus_visible(|this| this.border_color(cx.theme().colors().border_focused)) .on_click(move |_, _window, cx| { on_single_select_change( elicitation_id.clone(), @@ -1976,6 +2345,7 @@ impl<'a> ElicitationCard<'a> { .border_color(border_color) .child( Button::new(("elicitation-accept", self.entry_ix), accept_label) + .tab_index(0_isize) .start_icon( Icon::new(accept_icon) .size(IconSize::XSmall) @@ -1997,6 +2367,7 @@ impl<'a> ElicitationCard<'a> { .when(!is_accepted_url, |this| { this.child( Button::new(("elicitation-decline", self.entry_ix), "Decline") + .tab_index(0_isize) .start_icon( Icon::new(IconName::Close) .size(IconSize::XSmall) @@ -2009,6 +2380,7 @@ impl<'a> ElicitationCard<'a> { ) .child( Button::new(("elicitation-cancel", self.entry_ix), "Cancel") + .tab_index(0_isize) .label_size(LabelSize::Small) .on_click(move |_, window, cx| { on_cancel(cancel_id.clone(), window, cx); @@ -2018,6 +2390,7 @@ impl<'a> ElicitationCard<'a> { .when(is_accepted_url, |this| { this.child( Button::new(("elicitation-dismiss-url", self.entry_ix), "Cancel") + .tab_index(0_isize) .label_size(LabelSize::Small) .on_click(move |_, window, cx| { on_dismiss_url(dismiss_id.clone(), window, cx); diff --git a/crates/ui/src/components/button/button_like.rs b/crates/ui/src/components/button/button_like.rs index 493b12dea3..ed77c80108 100644 --- a/crates/ui/src/components/button/button_like.rs +++ b/crates/ui/src/components/button/button_like.rs @@ -778,7 +778,11 @@ impl RenderOnce for ButtonLike { Toggled::False }) }) - .when_some(self.tab_index, |this, tab_index| this.tab_index(tab_index)) + .when_some(self.tab_index, |this, tab_index| { + // Keep an already-focused button registered so disabling it does not + // move focus outside the view. + this.tab_index(tab_index).tab_stop(!self.disabled) + }) .when_some(self.focus_handle, |this, focus_handle| { this.track_focus(&focus_handle) })