select: run on_dismiss however the popup closes (#2984)

Follow-up to #2973, which is already merged. That PR is good work — this
closes the gaps a post-merge review turned up.

## `on_dismiss` was skipped by the accessible close

`Select`'s `Cancel` handler runs `on_dismiss` before asking the
controlled open state to close. The `on_a11y_action(Click)` path added
in #2973 called `on_open_change(false)` alone, so a consumer wiring
`on_dismiss` saw Escape and an outside click but *not* a screen-reader
user pressing the same control to close.

That is not hypothetical:
`crates/shell/src/materialize/components/select.rs:240` forwards
`on_dismiss` to JS as `onDismiss`, so a shell app silently lost the
callback on exactly the interaction #2973 was written to support.

Both paths now share one `close` closure, so they cannot drift apart
again:

```rust
let close: ActionHandler = Rc::new({ /* on_dismiss → on_open_change(false) → focus trigger */ });
```

## Also in this patch

- **zh-CN docs.** #2973 changed `website/docs/components/select.md` and
`website/base/primitives/select.md`; both zh-CN counterparts exist and
were left behind. Synced.
- **`SearchableListItem` docs.** The rule that the accessible value
reports `title()` while `display_title()` stays presentational was
documented only on the site. Moved onto the trait, where someone
implementing `display_title` will actually read it.
- **A test assertion that asserted nothing.**
`projects_application_owned_accessible_state` checked
`disabled.is_expanded() == Some(false)` on a `Select` that was never
opened, so it passed for a reason unrelated to `disabled`. It is now
opened, and asserts a disabled control still reports the state it is in.

## Testing

```
cargo test -p gpui-base -p gpui-component --lib   # 777 + 418 pass
cargo clippy -p gpui-base -p gpui-component --all-targets --locked   # clean
cargo fmt --all --check   # clean
```

GPUI exposes no way to dispatch an accessibility action from a test —
`Window::handle_a11y_action` is `pub(crate)` — so
`every_close_dismisses_before_it_closes` covers the shared close path
through Escape, the route a test can reach. I confirmed it fails
(`["open", "close"]` vs `["open", "dismiss", "close"]`) when the
`on_dismiss` call is removed again.

## Left open deliberately

Two findings from the same review are **not** in this PR, because both
are judgement calls rather than defects:

1. **The accessible value and the drawn trigger disagree during a
search.** `accessibility_value()` reads the committed `state.selection`;
`display_title()` reads the list cursor, which `set_query` clears.
Verified on `main`:

   ```
   after set_selected_value("Rust") then set_query("Go"):
     a11y value = "Rust"     trigger draws = <placeholder>
   ```

I think the committed value is right for assistive technology and the
*display* is the bug — a Select with a committed selection should not
visually revert to its placeholder while you type in the popup's search
box. That is pre-existing behaviour, so fixing it is a separate change.
Worth deciding whether to fix it or stop documenting the mismatch as
intended.

2. **Placeholder as the accessible value.** With nothing selected the AX
value becomes the placeholder, so AT reads "Programming language, Choose
a language". GPUI has `aria_placeholder` (`gpui-pre-0.3.2`,
`src/elements/div.rs:1391`), which is the semantically correct slot —
that would also keep name, placeholder and value as three separate
things, the way
`an_explicit_accessibility_label_does_not_replace_the_placeholder`
already insists.

## AI assistance

The post-merge review that found these and this patch were produced with
Claude Code. I reviewed the diff and the checks above before opening
this.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

https://claude.ai/code/session_01EC7fHTjQ6nGPKqq4WdryL8

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

Author: huacnlee

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<Mutex<Vec<bool>>>,
+        /// Every step of a close, in the order it ran.
+        closing: Arc<Mutex<Vec<&'static str>>>,
     }
 
     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<Self>) -> 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));
         });

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<AnyElement> {
         None
     }

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。
 
 ## 注意事项
 

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)` 后，下拉菜单中会出现搜索能力：