select: expose committed values and accessible activation - #2973
Merged
Conversation
Expose the committed value rather than the temporary search cursor. Handle accessible activation on the semantic root because native accessibility can flatten the painted trigger child.
3 tasks
huacnlee
added a commit
that referenced
this pull request
Sep 6, 2026
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
On macOS, Select's
AXPopUpButtonhas no current value orAXPressaction. Its trigger child can be flattened out of the accessibility tree, so the child's click handler is not enough.This exposes activation on the enabled root and supplies the committed selection as its accessible value. It reuses the existing open-state callback and focus handles. The value includes the item's title and prefix, or the placeholder when unselected. Filtering does not change it. Base Select exposes
.accessibility_value(...)for application-owned values.Screenshots
These screenshots are only for context. They show two states of the same example, not a visual before/after change.
Accessibility check
While searching for Rust, the root still reports the committed value Go. Captured from the macOS accessibility tree:
{ "role": "AXPopUpButton", "title": "Programming language", "value": "Language: Go", "actions": ["AXPress"] }How to Test
Scroll to Search and find Programming language in macOS Accessibility Inspector.
AXPress, typeGo, and press Enter. ExpectLanguage: Go, a closed popup, and focus on the trigger.Rust. The accessible value must remainLanguage: Go.AXPressto close. Check focus returns. Also check Down opens and Escape closes.AXPressaction.Automated checks
cargo test -p gpui-base -p gpui-component --lib --locked cargo clippy -p gpui-base -p gpui-component -p gpui-component-story --all-targets --locked -- -D warnings771 base and 419 component tests passed, along with strict Clippy and formatting. Regression coverage includes accessible metadata, disabled activation, filtering, prefixes, and placeholder fallback after clearing.
Windows and Linux runtime were not tested. Visual search preview and the existing disabled-root
AXEnabled=1reporting are outside this patch.Checklist
AI assistance
I tested the original change in my application, then asked OpenAI's
gpt-6-astrato extract it into a PR. The agent prepared the code, tests, verification scripts, and description, and ran the automated checks. I reviewed the extracted patch and manually tested the scenarios above on macOS using Accessibility Inspector.