From 1513741e43a4963f89ed1cc2aef174a064403932 Mon Sep 17 00:00:00 2001 From: Jason Lee Date: Sun, 6 Sep 2026 13:28:29 +0800 Subject: [PATCH] select: run on_dismiss however the popup closes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #2973 exposed accessible activation on the controlled root, but its close path called `on_open_change(false)` alone. `Cancel` also runs `on_dismiss` first, so a consumer that wires it — `crates/shell` forwards it to JS as `onDismiss` — saw Escape and an outside click but not a screen reader pressing the same control to close. Both paths now share one `close` closure, so the two cannot drift again. Also from that review: - sync the zh-CN halves of the two docs #2973 changed - record the accessibility contract on `SearchableListItem` itself, where an implementer of `display_title` will see it - open the disabled `Select` in `projects_application_owned_accessible_state`, so its expanded assertion says something about `disabled` rather than about the default open state GPUI exposes no way to dispatch an accessibility action from a test (`Window::handle_a11y_action` is `pub(crate)`), so the new regression test covers the shared close path through Escape. AI assistance: the review that found these, and this patch, were produced with Claude Code and reviewed before commit. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01EC7fHTjQ6nGPKqq4WdryL8 --- crates/base/src/select.rs | 95 +++++++++++++++---- .../component/src/searchable_list/delegate.rs | 8 ++ website/zh-CN/base/primitives/select.md | 5 +- website/zh-CN/docs/components/select.md | 14 +++ 4 files changed, 100 insertions(+), 22 deletions(-) diff --git a/crates/base/src/select.rs b/crates/base/src/select.rs index cb6d0b405c..5f9dca9fc5 100644 --- a/crates/base/src/select.rs +++ b/crates/base/src/select.rs @@ -129,7 +129,8 @@ impl Select { self } - /// Handles a dismissal requested through the Cancel action. + /// Handles a dismissal, however it was requested: the Cancel action, or + /// the accessible activation that closes an open control. /// /// This runs before the controlled open state is asked to close, so a /// caller that commits its pending value on dismissal can still read that @@ -171,6 +172,26 @@ impl RenderOnce for Select { let on_dismiss = self.on_dismiss; let on_confirm = self.on_confirm; + // Every way of closing runs the same steps. A caller that tracks + // dismissal has to see one however the popup was closed, and the + // accessible activation closes exactly what Escape closes. + let close: ActionHandler = Rc::new({ + let on_open_change = on_open_change.clone(); + let on_dismiss = on_dismiss.clone(); + let focus_handle = focus_handle.clone(); + move |window: &mut Window, cx: &mut App| { + if let Some(handler) = on_dismiss.as_ref() { + handler(window, cx); + } + if let Some(handler) = on_open_change.as_ref() { + handler(false, window, cx); + } + if let Some(handle) = focus_handle.as_ref() { + handle.focus(window, cx); + } + } + }); + div() .id(self.id) .role(Role::ComboBox) @@ -189,21 +210,20 @@ impl RenderOnce for Select { .when(!disabled, |this| { let on_open_change = on_open_change.clone(); let content_focus_handle = content_focus_handle.clone(); - let focus_handle = focus_handle.clone(); + let close = close.clone(); // Platform adapters may flatten the trigger child. // Expose activation on the semantic root itself. this.on_a11y_action(AccessibleAction::Click, move |_, window, cx| { - if let Some(handler) = on_open_change.as_ref() { - handler(!open, window, cx); + if open { + close(window, cx); + return; } - let next_focus = if open { - focus_handle.as_ref() - } else { - content_focus_handle.as_ref() - }; - if let Some(handle) = next_focus { + if let Some(handler) = on_open_change.as_ref() { + handler(true, window, cx); + } + if let Some(handle) = content_focus_handle.as_ref() { handle.focus(window, cx); } }) @@ -279,15 +299,7 @@ impl RenderOnce for Select { } cx.stop_propagation(); - if let Some(handler) = on_dismiss.as_ref() { - handler(window, cx); - } - if let Some(handler) = on_open_change.as_ref() { - handler(false, window, cx); - } - if let Some(handle) = focus_handle.as_ref() { - handle.focus(window, cx); - } + close(window, cx); }) .children(self.children) .refine_style(&self.style) @@ -308,6 +320,8 @@ mod tests { focus_handle: FocusHandle, content_focus_handle: FocusHandle, changes: Arc>>, + /// Every step of a close, in the order it ran. + closing: Arc>>, } impl SelectHarness { @@ -318,6 +332,7 @@ mod tests { focus_handle: cx.focus_handle(), content_focus_handle: cx.focus_handle(), changes: Arc::new(Mutex::new(Vec::new())), + closing: Arc::new(Mutex::new(Vec::new())), } } } @@ -332,6 +347,8 @@ mod tests { fn render(&mut self, _: &mut Window, cx: &mut Context) -> impl IntoElement { let state = cx.entity(); let changes = self.changes.clone(); + let opened = self.closing.clone(); + let dismissed = self.closing.clone(); Select::new("select") .open(self.open) @@ -340,11 +357,16 @@ mod tests { .content_focus_handle(&self.content_focus_handle) .on_open_change(move |open, _, cx| { changes.lock().unwrap().push(open); + opened + .lock() + .unwrap() + .push(if open { "open" } else { "close" }); state.update(cx, |state, cx| { state.open = open; cx.notify(); }); }) + .on_dismiss(move |_, _| dismissed.lock().unwrap().push("dismiss")) .child(div().track_focus(&self.content_focus_handle).size(px(20.))) } } @@ -409,6 +431,31 @@ mod tests { ); } + /// Closing runs `on_dismiss`, and runs it before the open state is asked + /// to close, so a caller that commits a pending value on dismissal can + /// still read that value. + /// + /// Every close shares one path, which is the point: the accessible + /// activation used to close by calling `on_open_change` alone, so a + /// consumer wiring `on_dismiss` — `crates/shell` forwards it to JS as + /// `onDismiss` — saw Escape but not a screen reader pressing the same + /// control. GPUI exposes no way to dispatch an accessibility action in a + /// test (`Window::handle_a11y_action` is `pub(crate)`), so this covers the + /// shared path through the route a test can reach. + #[gpui::test] + fn every_close_dismisses_before_it_closes(cx: &mut TestAppContext) { + let (cx, state) = harness(cx, false); + + cx.simulate_keystrokes("down escape"); + assert_eq!( + &*state + .read_with(cx, |state, _| state.closing.clone()) + .lock() + .unwrap(), + &["open", "dismiss", "close"] + ); + } + #[gpui::test] fn disabled_select_is_not_keyboard_interactive(cx: &mut TestAppContext) { let (cx, state) = harness(cx, true); @@ -442,12 +489,18 @@ mod tests { .accessibility_label("Programming language") .accessibility_value("Rust"), ); - let disabled = info(Select::new("disabled").disabled(true)); + // Open, so the expanded assertion below says something about + // `disabled` rather than about the default open state. + let disabled = info(Select::new("disabled").open(true).disabled(true)); assert_eq!(enabled.label(), Some("Programming language")); assert_eq!(enabled.value(), Some("Rust")); assert_eq!(enabled.is_expanded(), Some(true)); - assert_eq!(disabled.is_expanded(), Some(false)); + assert_eq!( + disabled.is_expanded(), + Some(true), + "a disabled control still reports the state it is in" + ); assert!(enabled.supports_action(accesskit::Action::Click)); assert!(!disabled.supports_action(accesskit::Action::Click)); }); diff --git a/crates/component/src/searchable_list/delegate.rs b/crates/component/src/searchable_list/delegate.rs index 5d477933cd..1fd3d3c503 100644 --- a/crates/component/src/searchable_list/delegate.rs +++ b/crates/component/src/searchable_list/delegate.rs @@ -9,11 +9,19 @@ pub trait SearchableListItem: Clone { type Value: Clone + PartialEq; /// Short display label shown in the dropdown row and in the trigger by default. + /// + /// This is also what assistive technology reads as the committed value, so + /// it has to stand on its own as text even when [`Self::display_title`] + /// draws something richer. fn title(&self) -> SharedString; /// Override the trigger display element (e.g. "Country (US)" instead of just "United States"). /// /// Returns `None` to fall back to `title()`. + /// + /// This is presentation only. An element is not text, so the accessible + /// value keeps reporting [`Self::title`]; if the two would read + /// differently, put the meaning a listener needs in `title()`. fn display_title(&self) -> Option { None } diff --git a/website/zh-CN/base/primitives/select.md b/website/zh-CN/base/primitives/select.md index 3caa26cfb2..14aeafa824 100644 --- a/website/zh-CN/base/primitives/select.md +++ b/website/zh-CN/base/primitives/select.md @@ -40,7 +40,10 @@ use gpui_kit::base::{Select}; ## 可访问性 -提供标签，暴露当前值，并保留上下键、Enter、Escape 与类型检索。 +在受控根节点上设置 `.accessibility_label(...)`，并把 `.accessibility_value(...)` +设为已提交的选中项，而不是临时的搜索游标。根节点会暴露展开状态与可访问的激活操作。 +激活会请求切换展开状态，并在 trigger 与内容之间移动焦点。禁用的控件不暴露激活操作。 +带样式的 `Select` 会自动提供已提交的值，未选中时回退到 placeholder。 ## 注意事项 diff --git a/website/zh-CN/docs/components/select.md b/website/zh-CN/docs/components/select.md index 91248eeb14..e44d01e503 100644 --- a/website/zh-CN/docs/components/select.md +++ b/website/zh-CN/docs/components/select.md @@ -65,6 +65,20 @@ Select::new(&state) .placeholder("Select a language...") ``` +### 可访问性 + +给控件一个不随选中项变化的名称： + +```rust +Select::new(&state) + .accessibility_label("Programming language") + .placeholder("Choose a language") +``` + +可访问值取自已提交选项的 `title()` 以及 `title_prefix`。自定义的 `display_title()` +仍然只用于视觉呈现。搜索不会改变这个已提交的值。未选中时，可访问值使用 placeholder。 +启用状态的控件会暴露可访问的激活操作，用于打开或关闭弹层。 + ### 可搜索 启用 `searchable(true)` 后，下拉菜单中会出现搜索能力：