Skip to content

select: run on_dismiss however the popup closes - #2984

Merged
huacnlee merged 1 commit into
mainfrom
fix/select-a11y-followup
Sep 6, 2026
Merged

select: run on_dismiss however the popup closes#2984
huacnlee merged 1 commit into
mainfrom
fix/select-a11y-followup

Conversation

@huacnlee

@huacnlee huacnlee commented Sep 6, 2026

Copy link
Copy Markdown
Member

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:

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

Also in this patch

  • zh-CN docs. select: expose committed values and accessible activation #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.ai/code/session_01EC7fHTjQ6nGPKqq4WdryL8

#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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EC7fHTjQ6nGPKqq4WdryL8
@huacnlee

huacnlee commented Sep 6, 2026

Copy link
Copy Markdown
Member Author

Raising the severity on this after tracing the consumers.

gpui_base::Combobox renders a gpui_base::Select and forwards on_dismiss to it (crates/base/src/combobox.rs:126,141). The styled Combobox uses that callback to commit the pending selection (crates/component/src/combobox.rs:944):

// This combobox commits its pending selection when the popup
// closes, so it listens for dismissal rather than Confirm.
.on_dismiss(move |_, cx| {
    confirm_state.update(cx, |state, cx| {
        cx.emit(ComboboxEvent::Confirm(state.selected_values()));
    });
})

So before this patch, a screen-reader user closing a Combobox through the accessible activation never got ComboboxEvent::Confirm — their selection was silently discarded, while Escape and an outside click committed it normally. That is data loss on the exact interaction #2973 was written to enable, not just a missing callback.

crates/component-shell/src/shell/delegate_combobox/mod.rs:213 consumes that event, so the loss reached the shell delegate too.

@huacnlee
huacnlee enabled auto-merge (squash) September 6, 2026 05:36
@huacnlee
huacnlee merged commit 5f5ba08 into main Sep 6, 2026
9 checks passed
@huacnlee
huacnlee deleted the fix/select-a11y-followup branch September 6, 2026 05:41
feigeCode pushed a commit to feigeCode/gpui-component that referenced this pull request Sep 7, 2026
Follow-up to longbridge#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 longbridge#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 longbridge#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.** longbridge#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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant