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) })