From f6512349ffc46d452e99cd8b0f5a4fa57f67cb6b Mon Sep 17 00:00:00 2001 From: Cameron Mcloughlin Date: Wed, 17 Jun 2026 18:42:38 +0100 Subject: [PATCH] a11y: Settings UI (#59429) Adds: - aria attributes to most UI elements in settings UI - a new a11y API to GPUI - a fix for a GPUI keyboard focus bug ## Accesible settings UI Settings UI should now be fully accessible to users of assistive technology. **However**, there are some caveats: - accessibility features require zed to be launched with the `ZED_EXPERIMENTAL_A11Y=1` env var to be set - I have not exhaustively checked every control - There are some quite surprising keyboard focus behaviours which predate this code - The main Zed UI is still largely inaccessible, though a handful of shared components will now report themselves, but the experience is suboptimal. For anyone wishing to try out the settings UI: - make sure `ZED_EXPERIMENTAL_A11Y=1` is set - open zed - press `ctrl-,` (or `cmd+,` on a mac) to open settings UI in a separate window ## `aria_active_descendant` This is equivalent to the [`aria-activedescendant`](https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Reference/Attributes/aria-activedescendant) API on the web. It allows a container to maintain focus, while indicating that one of its children should be active. Zed uses this for menus and combo boxes, for example. GPUI will report a div with `.aria_active_descendant()` as long as: - an ancestor is focused - only one descendant of the focused ancestor has `.aria_active_descendant()` Two descendants with this property where the focused node is a common ancestor is an error. ## GPUI keyboard fix GPUI will map a space or enter keypress to a div's `on_click` handler, if it exists. However, the keyboard-driven path was missing some checks that the mouseclick path has. With this PR, for an enter/space keypress to be considered a click, between the "key down" and "key up" events: - there must be no other key events - focus must not move to a different node This fixes an existing bug with combo boxes in zed where: - enter maps to `menu::Confirm` **on key down* - the menu item is selected - focus moves to the combo box - enter then triggers the combo box's `on_click` **on key up**, re-opening the menu --- Release Notes: - N/A or Added/Fixed/Improved ... --------- Co-authored-by: zed-zippy[bot] <234243425+zed-zippy[bot]@users.noreply.github.com> --- Cargo.lock | 42 +- Cargo.toml | 2 +- crates/gpui/src/_accessibility.rs | 54 +- crates/gpui/src/element.rs | 30 +- crates/gpui/src/elements/div.rs | 383 +++++++++++- crates/gpui/src/window.rs | 41 +- crates/gpui/src/window/a11y.rs | 543 ++++++++++++++++-- crates/settings_ui/src/components/dropdown.rs | 12 +- .../settings_ui/src/components/input_field.rs | 430 +++++++++++--- .../src/components/number_field.rs | 154 ++++- .../src/components/section_items.rs | 7 +- .../pages/edit_prediction_provider_setup.rs | 3 +- .../settings_ui/src/pages/sandbox_settings.rs | 12 +- .../src/pages/tool_permissions_setup.rs | 6 +- crates/settings_ui/src/settings_ui.rs | 114 +++- crates/title_bar/src/application_menu.rs | 3 +- crates/title_bar/src/title_bar.rs | 3 +- crates/ui/src/components/button/button.rs | 38 +- .../ui/src/components/button/button_like.rs | 66 ++- .../ui/src/components/button/icon_button.rs | 27 + crates/ui/src/components/context_menu.rs | 41 +- crates/ui/src/components/disclosure.rs | 2 + crates/ui/src/components/dropdown_menu.rs | 52 +- crates/ui/src/components/list/list_item.rs | 55 +- crates/ui/src/components/toggle.rs | 19 +- crates/ui/src/components/tree_view_item.rs | 16 +- 26 files changed, 1911 insertions(+), 244 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 836727e463..b286bc7be7 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4,9 +4,9 @@ version = 4 [[package]] name = "accesskit" -version = "0.24.0" +version = "0.24.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "5351dcebb14b579ccab05f288596b2ae097005be7ee50a7c3d4ca9d0d5a66f6a" +checksum = "d3b7f7f85a7e5f68090000ed7622545829afd484d210358702ae4cb97dd0c320" dependencies = [ "uuid", ] @@ -18,7 +18,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "842fd8203e6dfcf531d24f5bac792088edfba7d6b35844fead191603fb32a260" dependencies = [ "accesskit", - "accesskit_consumer", + "accesskit_consumer 0.35.0", "atspi-common", "phf 0.13.1", "serde", @@ -35,6 +35,16 @@ dependencies = [ "hashbrown 0.16.1", ] +[[package]] +name = "accesskit_consumer" +version = "0.37.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f950720ce064757a1b629caad3a408e8d2c63bb01f29b8a3ff8daa331053ffeb" +dependencies = [ + "accesskit", + "hashbrown 0.16.1", +] + [[package]] name = "accesskit_macos" version = "0.26.0" @@ -42,7 +52,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "534bc3fdc89a64a1db3c46b33c198fde2b7c3c7d094e5809c8c8bf2970c18243" dependencies = [ "accesskit", - "accesskit_consumer", + "accesskit_consumer 0.35.0", "hashbrown 0.16.1", "objc2 0.5.2", "objc2-app-kit 0.2.2", @@ -69,12 +79,12 @@ dependencies = [ [[package]] name = "accesskit_windows" -version = "0.32.1" +version = "0.33.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "eff7009f1a532e917d66970a1e80c965140c6cfbbabbdde3d64e5431e6c78e21" +checksum = "36e93ac7bf50b964f1cbb75f741629a4e950571baa1ef1274457ab5a80d9bcc2" dependencies = [ "accesskit", - "accesskit_consumer", + "accesskit_consumer 0.37.0", "hashbrown 0.16.1", "static_assertions", "windows 0.62.2", @@ -5270,7 +5280,7 @@ dependencies = [ "libc", "option-ext", "redox_users 0.5.2", - "windows-sys 0.59.0", + "windows-sys 0.61.2", ] [[package]] @@ -6067,7 +6077,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "39cab71617ae0d63f51a36d69f866391735b51691dbda63cf6f96d042b63efeb" dependencies = [ "libc", - "windows-sys 0.52.0", + "windows-sys 0.61.2", ] [[package]] @@ -7277,7 +7287,7 @@ dependencies = [ "gobject-sys", "libc", "system-deps", - "windows-sys 0.52.0", + "windows-sys 0.61.2", ] [[package]] @@ -11148,7 +11158,7 @@ version = "0.6.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "536bfad37a309d62069485248eeaba1e8d9853aaf951caaeaed0585a95346f08" dependencies = [ - "windows-sys 0.60.2", + "windows-sys 0.61.2", ] [[package]] @@ -11578,7 +11588,7 @@ version = "0.50.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "7957b9740744892f114936ab4a57b3f487491bbeafaf8083688b16841a4240e5" dependencies = [ - "windows-sys 0.59.0", + "windows-sys 0.61.2", ] [[package]] @@ -15669,7 +15679,7 @@ dependencies = [ "errno 0.3.14", "libc", "linux-raw-sys 0.11.0", - "windows-sys 0.52.0", + "windows-sys 0.61.2", ] [[package]] @@ -17111,7 +17121,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "3a766e1110788c36f4fa1c2b71b387a7815aa65f88ce0229841826633d93723e" dependencies = [ "libc", - "windows-sys 0.60.2", + "windows-sys 0.61.2", ] [[package]] @@ -18228,7 +18238,7 @@ dependencies = [ "getrandom 0.3.4", "once_cell", "rustix 1.1.2", - "windows-sys 0.52.0", + "windows-sys 0.61.2", ] [[package]] @@ -21230,7 +21240,7 @@ version = "0.1.11" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "c2a7b1c03c876122aa43f3020e6c3c3ee5c05081c9a00739faf7503aeba10d22" dependencies = [ - "windows-sys 0.48.0", + "windows-sys 0.61.2", ] [[package]] diff --git a/Cargo.toml b/Cargo.toml index 5da6cd1622..7f813d6dc9 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -502,7 +502,7 @@ ztracing_macro = { path = "crates/ztracing_macro" } accesskit = "0.24.0" accesskit_macos = "0.26.0" accesskit_unix = "0.21.0" -accesskit_windows = "0.32.1" +accesskit_windows = "0.33.1" agent-client-protocol = { version = "=0.14.0", features = ["unstable"] } aho-corasick = "1.1" alacritty_terminal = { git = "https://github.com/zed-industries/alacritty", rev = "fcf32feacb367b75ec84dd40f041e4fd411d3cc1" } diff --git a/crates/gpui/src/_accessibility.rs b/crates/gpui/src/_accessibility.rs index 155dd6506e..af14b638cd 100644 --- a/crates/gpui/src/_accessibility.rs +++ b/crates/gpui/src/_accessibility.rs @@ -9,7 +9,8 @@ //! //! This guide is focused on **programmatic accessibility**. This allows //! assistive technology, such as screen readers or Braille displays, to inspect -//! and interact with your app. +//! and interact with your app. Docs for contributors working on accessibility +//! support can be found in the `a11y` module's doc comment. //! //! GPUI integrates with [AccessKit] to provide programmatic accessibility //! features (referred to as simply "accessibility" for the rest of this guide). @@ -219,6 +220,57 @@ //! [`.on_click()`][StatefulInteractiveElement::on_click] adds an //! [`AccessibleAction::Click`] handler that calls the click handler. //! +//! ## Synthetic children +//! +//! Sometimes, a custom [`Element`] may want to appear as if it is really made +//! of multiple nodes. For example, a totally hypothetical custom text editor +//! element may want to have [`Role::TextInput`], while presenting children +//! consisting of [`Role::TextRun`]s. +//! +//! This is possible using [`Element::a11y_synthetic_children`]. For example: +//! ```rust,ignore +//! # use gpui::*; +//! impl Element for MyCustomTextField { +//! +//! // ... +//! +//! fn a11y_role(&self) -> Option { +//! Some(Role::TextInput) +//! } +//! +//! fn a11y_synthetic_children( +//! &mut self, +//! _prepaint: &mut Self::PrepaintState, +//! builder: &mut A11ySubtreeBuilder, +//! ) { +//! // Create the synthetic child node +//! let mut run = accesskit::Node::new(Role::TextRun); +//! run.set_value(self.text.clone()); +//! run.set_character_lengths( +//! self.text.chars().map(|c| c.len_utf8() as u8).collect::>(), +//! ); +//! +//! // Insert it as a child of `MyCustomTextField` +//! let run_id = builder.synthetic_node_id(0); +//! builder.push_child(run_id, run); +//! +//! // You can also mutate the parent (i.e. the `MyCustomTextField`) +//! let caret = accesskit::TextPosition { +//! node: run_id, +//! character_index: self.cursor, +//! }; +//! builder.parent_node().set_text_selection(accesskit::TextSelection { +//! anchor: caret, +//! focus: caret, +//! }); +//! } +//! } +//! ``` +//! +//! Notably, synthetic children are added *after* an element is +//! [prepainted][Element::prepaint], so prepaint state can be used (for example, +//! to determine what is visible on screen). +//! //! ## Further reading //! //! Designing high-quality accessible interfaces can be challenging, in the same diff --git a/crates/gpui/src/element.rs b/crates/gpui/src/element.rs index e05d6e97f4..f212f246ca 100644 --- a/crates/gpui/src/element.rs +++ b/crates/gpui/src/element.rs @@ -32,8 +32,8 @@ //! your own custom layout algorithm or rendering a code editor. use crate::{ - App, ArenaBox, AvailableSpace, Bounds, Context, DispatchNodeId, ElementId, FocusHandle, - InspectorElementId, LayoutId, Pixels, Point, SharedString, Size, Style, Window, + A11ySubtreeBuilder, App, ArenaBox, AvailableSpace, Bounds, Context, DispatchNodeId, ElementId, + FocusHandle, InspectorElementId, LayoutId, Pixels, Point, SharedString, Size, Style, Window, util::FluentBuilder, window::with_element_arena, }; use derive_more::{Deref, DerefMut}; @@ -119,6 +119,22 @@ pub trait Element: 'static + IntoElement { /// See the [accessibility guide](crate::_accessibility) for an overview. fn write_a11y_info(&self, _node: &mut accesskit::Node) {} + /// Add synthetic child nodes to an [`Element`] that has an + /// [`.id()`][Element::id] and a [`.role()`][Element::a11y_role]. + /// + /// Some elements may want to inject accessibility nodes that do not + /// correspond to any GPUI element. For example, a custom text field element + /// may want to inject synthetic child nodes for the text content. + /// + /// See [Synthetic children](crate::_accessibility#synthetic-children) in + /// the accessibility guide for more detail. + fn a11y_synthetic_children( + &mut self, + _prepaint: &mut Self::PrepaintState, + _builder: &mut A11ySubtreeBuilder, + ) { + } + /// Convert this element into a dynamically-typed [`AnyElement`]. fn into_any(self) -> AnyElement { AnyElement::new(self) @@ -477,7 +493,7 @@ impl Drawable { } let node_id = window.next_frame.dispatch_tree.push_node(); - let prepaint = self.element.prepaint( + let mut prepaint = self.element.prepaint( global_id.as_ref(), inspector_id.as_ref(), bounds, @@ -488,6 +504,14 @@ impl Drawable { window.next_frame.dispatch_tree.pop_node(); if pushed_a11y_node { + if let Some(global_id) = global_id.as_ref() { + let mut builder = A11ySubtreeBuilder::new( + global_id.accesskit_node_id(), + &mut window.a11y.nodes, + ); + self.element + .a11y_synthetic_children(&mut prepaint, &mut builder); + } window.a11y.nodes.pop(); } diff --git a/crates/gpui/src/elements/div.rs b/crates/gpui/src/elements/div.rs index 436cc1a9d6..4a61069a0a 100644 --- a/crates/gpui/src/elements/div.rs +++ b/crates/gpui/src/elements/div.rs @@ -1201,6 +1201,43 @@ pub trait StatefulInteractiveElement: InteractiveElement { self } + /// Report this element as the focused node in the accessibility tree, + /// overriding the element that holds real keyboard focus — but only while + /// one of its ancestors actually holds focus. + /// + /// This implements the `aria-activedescendant` pattern for composite + /// widgets that keep keyboard focus on a container (e.g. a menu or + /// listbox) while a child is "selected": set this on the selected child so + /// assistive technology announces and highlights it as focused. + /// + /// The element must also have a [`role`][Self::role] (and an id) so it + /// produces an accessibility node. Unlike the web's container-side + /// `aria-activedescendant`, this is set on the descendant; GPUI honors it + /// only when a focused ancestor is present in the tree, so it is safe to + /// set unconditionally on the selected child — if the container isn't + /// focused, the claim is ignored. + fn aria_active_descendant(mut self) -> Self { + self.interactivity().report_active_descendant_focus = true; + self + } + + /// Contribute synthetic accessibility nodes — nodes that don't correspond + /// to any element — as children of this element's a11y node. For example, + /// text runs describing an editor's text content. + /// + /// The closure is called after this element is prepainted, and only if it + /// contributed a node to the accessibility tree (i.e. it has an id and a + /// [`role`][StatefulInteractiveElement::role]). + /// + /// See [`Element::a11y_synthetic_children`] for details. + fn a11y_synthetic_children( + mut self, + f: impl FnOnce(&mut crate::A11ySubtreeBuilder) + 'static, + ) -> Self { + self.interactivity().a11y_synthetic_children = Some(Box::new(f)); + self + } + /// Set the selected state for this element. fn aria_selected(mut self, selected: bool) -> Self { self.interactivity().aria_selected = Some(selected); @@ -1225,6 +1262,27 @@ pub trait StatefulInteractiveElement: InteractiveElement { self } + /// Set the step by which assistive technology should expect the numeric + /// value of this element to change (e.g. when incrementing a spin button). + fn aria_numeric_value_step(mut self, step: f64) -> Self { + self.interactivity().aria_numeric_value_step = Some(step); + self + } + + /// Set the string value of this element, e.g. the text content of a simple + /// text input. + fn aria_value(mut self, value: impl Into) -> Self { + self.interactivity().aria_value = Some(value.into()); + self + } + + /// Set the placeholder text reported to assistive technology for this + /// element, shown when a text input is empty. + fn aria_placeholder(mut self, placeholder: impl Into) -> Self { + self.interactivity().aria_placeholder = Some(placeholder.into()); + self + } + /// Set the minimum numeric value for this element. fn aria_min_numeric_value(mut self, value: f64) -> Self { self.interactivity().aria_min_numeric_value = Some(value); @@ -1622,6 +1680,16 @@ impl Element for Div { self.interactivity.write_a11y_info(node); } + fn a11y_synthetic_children( + &mut self, + _prepaint: &mut Self::PrepaintState, + builder: &mut crate::A11ySubtreeBuilder, + ) { + if let Some(f) = self.interactivity.a11y_synthetic_children.take() { + f(builder); + } + } + #[stacksafe] fn request_layout( &mut self, @@ -1861,6 +1929,8 @@ pub struct Interactivity { pub(crate) a11y_action_listeners: Vec<(accesskit::Action, crate::window::a11y::A11yActionListener)>, + pub(crate) a11y_synthetic_children: Option>, + pub(crate) report_active_descendant_focus: bool, pub(crate) override_role: Option, pub(crate) aria_label: Option, pub(crate) aria_selected: Option, @@ -1869,6 +1939,9 @@ pub struct Interactivity { pub(crate) aria_numeric_value: Option, pub(crate) aria_min_numeric_value: Option, pub(crate) aria_max_numeric_value: Option, + pub(crate) aria_numeric_value_step: Option, + pub(crate) aria_value: Option, + pub(crate) aria_placeholder: Option, pub(crate) aria_orientation: Option, pub(crate) aria_level: Option, pub(crate) aria_position_in_set: Option, @@ -2002,13 +2075,21 @@ impl Interactivity { if window.a11y.is_active() { if let Some(global_id) = global_id { let node_id = global_id.accesskit_node_id(); - window.a11y.focus_ids.insert(node_id, focus_handle.id); - if focus_handle.is_focused(window) && window.a11y.nodes.has_node(node_id) { - window.a11y.nodes.set_focus(node_id); + window.a11y.set_focusable(node_id, focus_handle.id); + if focus_handle.is_focused(window) { + window.a11y.set_focus(node_id); } } } } + + if self.report_active_descendant_focus && window.a11y.is_active() { + if let Some(global_id) = global_id { + window + .a11y + .set_active_descendant(global_id.accesskit_node_id()); + } + } window.with_optional_element_state::( global_id, |element_state, window| { @@ -2565,6 +2646,11 @@ impl Interactivity { .get_or_insert_with(Default::default) .clone(); + let pending_keyboard_down = element_state + .pending_keyboard_down + .get_or_insert_with(Default::default) + .clone(); + let clicked_state = element_state .clicked_state .get_or_insert_with(Default::default) @@ -2618,6 +2704,29 @@ impl Interactivity { }); if is_focused { + // Record the focus generation at which an enter/space key + // down event happened on this element. The next key up + // event will be mapped to a click event if both of the + // following are true: + // - no other key events happen in between + // - the focus generation is the same (implying focus did not move) + // + // This design avoids an ABA problem that happens if you + // store the focus handle that registered the keypress. + window.on_key_event({ + let pending_keyboard_down = pending_keyboard_down.clone(); + move |event: &KeyDownEvent, phase, window, _cx| { + if phase.bubble() && !window.default_prevented() { + let stroke = &event.keystroke; + let is_activation_key = (stroke.key.eq("enter") + || stroke.key.eq("space")) + && !stroke.modifiers.modified(); + *pending_keyboard_down.borrow_mut() = + is_activation_key.then_some(window.focus_generation); + } + } + }); + // Press enter, space to trigger click, when the element is focused. window.on_key_event({ let click_listeners = click_listeners.clone(); @@ -2636,6 +2745,12 @@ impl Interactivity { if let Some(button) = keyboard_button && !stroke.modifiers.modified() { + let pending = + std::mem::take(&mut *pending_keyboard_down.borrow_mut()); + if pending != Some(window.focus_generation) { + return; + } + let click_event = ClickEvent::Keyboard(KeyboardClickEvent { button, bounds: hitbox.bounds, @@ -2644,6 +2759,11 @@ impl Interactivity { for listener in &click_listeners { listener(&click_event, window, cx); } + } else { + // Releasing any other key mid-press means + // this isn't a clean activation, so cancel + // the pending keydown. + *pending_keyboard_down.borrow_mut() = None; } } } @@ -3075,6 +3195,15 @@ impl Interactivity { if let Some(value) = self.aria_max_numeric_value { node.set_max_numeric_value(value); } + if let Some(step) = self.aria_numeric_value_step { + node.set_numeric_value_step(step); + } + if let Some(value) = &self.aria_value { + node.set_value(value.to_string()); + } + if let Some(placeholder) = &self.aria_placeholder { + node.set_placeholder(placeholder.to_string()); + } if let Some(orientation) = self.aria_orientation { node.set_orientation(orientation); } @@ -3120,6 +3249,14 @@ pub struct InteractiveElementState { pub(crate) hover_state: Option>>, pub(crate) hover_listener_state: Option>>, pub(crate) pending_mouse_down: Option>>>, + /// Set to the window's [`focus_generation`](crate::Window::focus_generation) + /// when an Enter/Space keydown is received while this element is focused, + /// recording that we are waiting for the matching keyup to fire a keyboard + /// click. On keyup the click only fires if the stored generation still + /// matches the window's current one, i.e. focus never moved during the + /// press (mirroring the browser clearing a control's pressed state on + /// blur). `None` means no activation key is pending. + pub(crate) pending_keyboard_down: Option>>>, pub(crate) scroll_offset: Option>>>, pub(crate) active_tooltip: Option>>>, } @@ -3552,6 +3689,14 @@ where self.element.write_a11y_info(node); } + fn a11y_synthetic_children( + &mut self, + prepaint: &mut Self::PrepaintState, + builder: &mut crate::A11ySubtreeBuilder, + ) { + self.element.a11y_synthetic_children(prepaint, builder); + } + fn request_layout( &mut self, id: Option<&GlobalElementId>, @@ -3869,8 +4014,8 @@ impl ScrollHandle { mod tests { use super::*; use crate::{ - AppContext as _, Context, InputEvent, MouseMoveEvent, TestAppContext, - util::FluentBuilder as _, + AnyWindowHandle, AppContext as _, Context, InputEvent, Keystroke, MouseMoveEvent, + TestAppContext, util::FluentBuilder as _, }; use std::rc::Weak; @@ -4179,4 +4324,232 @@ mod tests { assert!(active_tooltip.borrow().is_none()); } + + #[test] + fn test_write_a11y_info_string_and_numeric_properties() { + let mut interactivity = Interactivity::default(); + interactivity.aria_label = Some("Buffer Font Size".into()); + interactivity.aria_value = Some("15".into()); + interactivity.aria_placeholder = Some("Search".into()); + interactivity.aria_numeric_value = Some(15.0); + interactivity.aria_min_numeric_value = Some(6.0); + interactivity.aria_max_numeric_value = Some(72.0); + interactivity.aria_numeric_value_step = Some(1.0); + + let mut node = accesskit::Node::new(accesskit::Role::SpinButton); + interactivity.write_a11y_info(&mut node); + + assert_eq!(node.label(), Some("Buffer Font Size")); + assert_eq!(node.value(), Some("15")); + assert_eq!(node.placeholder(), Some("Search")); + assert_eq!(node.numeric_value(), Some(15.0)); + assert_eq!(node.min_numeric_value(), Some(6.0)); + assert_eq!(node.max_numeric_value(), Some(72.0)); + assert_eq!(node.numeric_value_step(), Some(1.0)); + } + + /// Two focusable, clickable elements ("a" and "b") used to exercise the + /// Enter/Space -> synthesized click press/release pairing. + struct KeyboardActivationTest { + focus_a: FocusHandle, + focus_b: FocusHandle, + clicks: Rc>>, + } + + impl Render for KeyboardActivationTest { + fn render(&mut self, _window: &mut Window, _cx: &mut Context) -> impl IntoElement { + let clicks_a = self.clicks.clone(); + let clicks_b = self.clicks.clone(); + div() + .size_full() + .child( + div() + .id("a") + .w(px(50.)) + .h(px(50.)) + .track_focus(&self.focus_a) + .on_click(move |_, _, _| clicks_a.borrow_mut().push("a")), + ) + .child( + div() + .id("b") + .w(px(50.)) + .h(px(50.)) + .track_focus(&self.focus_b) + .on_click(move |_, _, _| clicks_b.borrow_mut().push("b")), + ) + } + } + + fn setup_keyboard_activation_test() -> ( + TestAppContext, + AnyWindowHandle, + Rc>>, + FocusHandle, + FocusHandle, + ) { + let mut cx = TestAppContext::single(); + let (focus_a, focus_b) = cx.update(|cx| (cx.focus_handle(), cx.focus_handle())); + let clicks: Rc>> = Rc::new(RefCell::new(Vec::new())); + let window = cx.add_window({ + let focus_a = focus_a.clone(); + let focus_b = focus_b.clone(); + let clicks = clicks.clone(); + move |_, _| KeyboardActivationTest { + focus_a, + focus_b, + clicks, + } + }); + (cx, window.into(), clicks, focus_a, focus_b) + } + + /// Move focus to `handle`, flush effects, then paint so the newly focused + /// element registers its key handlers for the next dispatched event. + fn focus_and_draw(cx: &mut TestAppContext, window: AnyWindowHandle, handle: &FocusHandle) { + cx.update_window(window, |_, window, cx| window.focus(handle, cx)) + .unwrap(); + cx.run_until_parked(); + cx.update_window(window, |_, window, cx| { + window.draw(cx).clear(); + }) + .unwrap(); + } + + fn key_down(cx: &mut TestAppContext, window: AnyWindowHandle, key: &str) { + let keystroke = Keystroke::parse(key).unwrap(); + cx.update_window(window, |_, window, cx| { + window.dispatch_event( + KeyDownEvent { + keystroke, + is_held: false, + prefer_character_input: false, + } + .to_platform_input(), + cx, + ); + }) + .unwrap(); + } + + fn key_up(cx: &mut TestAppContext, window: AnyWindowHandle, key: &str) { + let keystroke = Keystroke::parse(key).unwrap(); + cx.update_window(window, |_, window, cx| { + window.dispatch_event(KeyUpEvent { keystroke }.to_platform_input(), cx); + }) + .unwrap(); + } + + /// Pressing and releasing Enter on the same focused element fires a click. + #[test] + fn keyboard_activation_fires_click_on_same_element() { + let (mut cx, window, clicks, focus_a, _focus_b) = setup_keyboard_activation_test(); + + focus_and_draw(&mut cx, window, &focus_a); + key_down(&mut cx, window, "enter"); + key_up(&mut cx, window, "enter"); + + assert_eq!(*clicks.borrow(), vec!["a"]); + } + + /// A key-down whose key-up lands on a *different* element (because focus + /// moved in between) must not leak a synthesized click onto the newly + /// focused element. This is the core regression: previously the key-up + /// handler fired unconditionally on whatever was focused at key-up time. + #[test] + fn keyboard_activation_does_not_leak_across_focus_change() { + let (mut cx, window, clicks, focus_a, focus_b) = setup_keyboard_activation_test(); + + // Enter pressed while "a" is focused... + focus_and_draw(&mut cx, window, &focus_a); + key_down(&mut cx, window, "enter"); + + // ...focus moves to "b" before the release (as a confirm action would)... + focus_and_draw(&mut cx, window, &focus_b); + key_up(&mut cx, window, "enter"); + + // ...so neither element is clicked: "a" never saw the up, and "b" + // never saw the down. + assert!(clicks.borrow().is_empty(), "clicks: {:?}", clicks.borrow()); + } + + /// A keydown whose flag is left pending because focus moved away before + /// the keyup must not fire a click when focus later *returns* to the same + /// element (the menu trigger reopening case). The stamped focus generation + /// no longer matches, so the stale pending state is ignored. + #[test] + fn keyboard_activation_does_not_leak_when_focus_returns() { + let (mut cx, window, clicks, focus_a, focus_b) = setup_keyboard_activation_test(); + + // Enter pressed on "a"... + focus_and_draw(&mut cx, window, &focus_a); + key_down(&mut cx, window, "enter"); + + // ...focus leaves "a" before its keyup (so the pending state is never + // consumed), then comes back to "a"... + focus_and_draw(&mut cx, window, &focus_b); + focus_and_draw(&mut cx, window, &focus_a); + key_up(&mut cx, window, "enter"); + + // ...and the now-stale pending keydown must not fire a click. + assert!(clicks.borrow().is_empty(), "clicks: {:?}", clicks.borrow()); + } + + /// A non-activation key *released* during the press must cancel the pending + /// activation. For the sequence escape-down, space-down, escape-up, + /// space-up the space forms a clean down/up pair, but the intervening + /// escape-up means this isn't a plain space activation, so no click fires. + #[test] + fn keyboard_activation_cleared_by_intervening_key_release() { + let (mut cx, window, clicks, focus_a, _focus_b) = setup_keyboard_activation_test(); + + focus_and_draw(&mut cx, window, &focus_a); + key_down(&mut cx, window, "escape"); + key_down(&mut cx, window, "space"); + key_up(&mut cx, window, "escape"); + key_up(&mut cx, window, "space"); + + assert!(clicks.borrow().is_empty(), "clicks: {:?}", clicks.borrow()); + } + + /// The flag is a single activation marker, not keyed by which activation + /// key was used, so a Space down paired with an Enter up on the same + /// element still fires a click. + #[test] + fn keyboard_activation_does_not_distinguish_space_and_enter() { + let (mut cx, window, clicks, focus_a, _focus_b) = setup_keyboard_activation_test(); + + focus_and_draw(&mut cx, window, &focus_a); + key_down(&mut cx, window, "space"); + key_up(&mut cx, window, "enter"); + + assert_eq!(*clicks.borrow(), vec!["a"]); + } + + /// A non-activation key pressed between the activation down and up clears + /// the pending flag, suppressing the click. + #[test] + fn keyboard_activation_cleared_by_intervening_keydown() { + let (mut cx, window, clicks, focus_a, _focus_b) = setup_keyboard_activation_test(); + + focus_and_draw(&mut cx, window, &focus_a); + key_down(&mut cx, window, "enter"); + key_down(&mut cx, window, "a"); + key_up(&mut cx, window, "enter"); + + assert!(clicks.borrow().is_empty(), "clicks: {:?}", clicks.borrow()); + } + + /// A modified Enter (e.g. cmd-enter) is not treated as an activation key, + /// so it neither sets the pending flag nor fires a click on release. + #[test] + fn keyboard_activation_ignores_modified_keys() { + let (mut cx, window, clicks, focus_a, _focus_b) = setup_keyboard_activation_test(); + + focus_and_draw(&mut cx, window, &focus_a); + key_down(&mut cx, window, "cmd-enter"); + key_up(&mut cx, window, "cmd-enter"); + + assert!(clicks.borrow().is_empty(), "clicks: {:?}", clicks.borrow()); + } } diff --git a/crates/gpui/src/window.rs b/crates/gpui/src/window.rs index d5fce7a0aa..7b8b6af63f 100644 --- a/crates/gpui/src/window.rs +++ b/crates/gpui/src/window.rs @@ -62,6 +62,8 @@ use uuid::Uuid; pub(crate) mod a11y; mod prompts; +pub use a11y::A11ySubtreeBuilder; + use self::a11y::A11y; #[cfg(not(target_family = "wasm"))] use self::a11y::ROOT_NODE_ID; @@ -1041,6 +1043,9 @@ pub struct Window { pub(crate) activation_observers: SubscriberSet<(), AnyObserver>, pub(crate) focus: Option, focus_enabled: bool, + /// Incremented every time focus moves. Used to invalidate a + /// pending keyboard activation state when focus changes. + pub(crate) focus_generation: u64, pending_input: Option, pending_modifier: ModifierState, pub(crate) pending_input_observers: SubscriberSet<(), AnyObserver>, @@ -1303,6 +1308,10 @@ impl Window { tabbing_identifier, } = options; + let initial_window_title = titlebar + .as_ref() + .and_then(|titlebar| titlebar.title.clone()); + let window_bounds = window_bounds.unwrap_or_else(|| default_bounds(display_id, cx)); let mut platform_window = cx.platform.open_window( handle, @@ -1361,8 +1370,12 @@ impl Window { #[cfg(not(target_family = "wasm"))] if !accessibility_force_disabled { + let mut initial_root_node = accesskit::Node::new(accesskit::Role::Window); + if let Some(title) = &initial_window_title { + initial_root_node.set_label(title.to_string()); + } let initial_tree = accesskit::TreeUpdate { - nodes: vec![(ROOT_NODE_ID, accesskit::Node::new(accesskit::Role::Window))], + nodes: vec![(ROOT_NODE_ID, initial_root_node)], tree: Some(accesskit::Tree::new(ROOT_NODE_ID)), tree_id: accesskit::TreeId::ROOT, focus: ROOT_NODE_ID, @@ -1735,6 +1748,7 @@ impl Window { activation_observers: SubscriberSet::new(), focus: None, focus_enabled: true, + focus_generation: 0, pending_input: None, pending_modifier: ModifierState::default(), pending_input_observers: SubscriberSet::new(), @@ -1744,7 +1758,11 @@ impl Window { captured_hitbox: None, #[cfg(any(feature = "inspector", debug_assertions))] inspector: None, - a11y: A11y::new(a11y_active_flag, accessibility_force_disabled), + a11y: A11y::new( + a11y_active_flag, + accessibility_force_disabled, + initial_window_title, + ), }) } @@ -1891,6 +1909,7 @@ impl Window { } self.focus = Some(handle.id); + self.focus_generation = self.focus_generation.wrapping_add(1); self.clear_pending_keystrokes(); // Avoid re-entrant entity updates by deferring observer notifications to the end of the @@ -1913,6 +1932,9 @@ impl Window { return; } + if self.focus.is_some() { + self.focus_generation = self.focus_generation.wrapping_add(1); + } self.focus = None; self.refresh(); } @@ -2335,6 +2357,7 @@ impl Window { /// Updates the window's title at the platform level. pub fn set_window_title(&mut self, title: &str) { self.platform_window.set_title(title); + self.a11y.set_window_title(title.to_string()); } /// Sets the position of the macOS traffic light buttons. @@ -5474,6 +5497,20 @@ impl Window { self.platform_window.play_system_bell() } + /// Returns whether accessibility features are active for this frame, + /// i.e. whether assistive technology (such as a screen reader) is + /// connected and an accessibility tree is being built. + /// + /// Use this to skip computing data during rendering that is only + /// observable through the accessibility tree. When accessibility is + /// activated, a redraw is forced, so gated work is recomputed before the + /// next tree update is sent to the platform. + /// + /// See the [accessibility guide](crate::_accessibility) for an overview. + pub fn is_a11y_active(&self) -> bool { + self.a11y.is_active() + } + /// Register a listener for an accessibility action on a specific node. /// The listener will be called when a screen reader requests the given /// action on the node identified by `node_id`. diff --git a/crates/gpui/src/window/a11y.rs b/crates/gpui/src/window/a11y.rs index dfb818ead9..68140fd123 100644 --- a/crates/gpui/src/window/a11y.rs +++ b/crates/gpui/src/window/a11y.rs @@ -64,8 +64,26 @@ //! a frame have been prepainted, we send the resulting [`TreeUpdate`] object to //! the adapter and the screen reader can announce the changes. //! +//! #### Synthetic children +//! +//! Additionally, some nodes can register "synthetic children" using +//! [`Element::a11y_synthetic_children`]. Normally, one accesskit node is pushed +//! for every [`Element`] with a role and id. However, sometimes a single +//! element may want to produce many accesskit nodes. These extra nodes are +//! referred to as "synthetic children" of the element providing a non-default +//! [`Element::a11y_synthetic_children`] implementation. +//! +//! The user is provided a builder-style API using [`A11ySubtreeBuilder`], which +//! allows them to create push nodes that are children of the current node, as +//! well as modify the current node itself. +//! +//! GPUI calls this callback *after* prepainting (and just before popping the +//! corresponding element), since this step may need prepaint information to be +//! available. In the future, we may want to add prepaint information more +//! generally to [`Element::write_a11y_info`], but for now that's not necessary. +//! //! ### Responding to actions -//! +//! //! On adapter creation, we provide a callback to the adapter, which can be used //! to dispatch actions. This callback forwards to [`A11y::action_listeners`], a //! mapping from [`NodeId`]s to action handlers (basically just `Box, pub(crate) node_bounds: FxHashMap>, pub(crate) action_listeners: FxHashMap>, + /// The window's title, used to label the root node so assistive + /// technology can tell windows apart. + window_title: Option, } impl A11y { - pub(crate) fn new(active_flag: Arc, force_disabled: bool) -> Self { + pub(crate) fn new( + active_flag: Arc, + force_disabled: bool, + window_title: Option, + ) -> Self { Self { force_disabled, active_flag, @@ -144,9 +166,14 @@ impl A11y { focus_ids: FxHashMap::default(), node_bounds: FxHashMap::default(), action_listeners: FxHashMap::default(), + window_title, } } + pub(crate) fn set_window_title(&mut self, title: impl Into) { + self.window_title = Some(title.into()); + } + /// Ensures that [`Self::is_active`] returns up to date information. /// /// See the docs for [`Self::active_flag`] and [`Self::active_this_frame`] @@ -159,31 +186,99 @@ impl A11y { self.active_this_frame } + pub(crate) fn set_focusable(&mut self, node_id: NodeId, focus_id: FocusId) { + self.focus_ids.insert(node_id, focus_id); + } + + /// Report `node_id` as the currently-focused node, if it is present in the + /// tree. + /// + /// Must only be called once per frame. + pub(crate) fn set_focus(&mut self, node_id: NodeId) { + // A focused node must have been registered as focusable this frame. + if !self.focus_ids.contains_key(&node_id) { + if cfg!(debug_assertions) { + panic!("set_focus called for a node that was not registered with set_focusable"); + } else { + log::warn!( + "a11y: set_focus called for a node that was not registered with \ + set_focusable ({node_id:?})" + ); + } + } + if self.nodes.has_node(node_id) { + self.nodes.set_focus(node_id); + } + } + + pub(crate) fn set_active_descendant(&mut self, node_id: NodeId) { + // The active descendant must be a descendant of the focused container, + // not the focused node itself. + if self.nodes.node_is_focused(node_id) { + if cfg!(debug_assertions) { + panic!("set_active_descendant called on the focused node"); + } else { + log::warn!("a11y: set_active_descendant called on the focused node ({node_id:?})"); + } + return; + } + if self.nodes.has_node(node_id) && self.nodes.focus_is_ancestor_of_current() { + self.nodes.set_active_descendant(node_id); + } + } + /// Clear per-frame state and push the root node to start a new frame. pub(crate) fn begin_frame(&mut self) { self.focus_ids.clear(); self.node_bounds.clear(); self.action_listeners.clear(); - self.nodes.begin_frame(); + self.nodes.begin_frame(self.window_title.as_ref()); } /// Finalize the tree and produce a [`TreeUpdate`] for the platform adapter. pub(crate) fn end_frame(&mut self) -> TreeUpdate { - let tree_update = self.nodes.finalize(); + self.nodes.finalize() + } +} - // Zed currently doesn't set any a11y APIs on *any* UI elements, so a - // tree with nodes other than the root indicates a bug in the - // `TreeUpdate`-producing logic. - // - // Remove this when adding aria attributes. - if tree_update.nodes.len() > 1 { - log::warn!( - "expected an empty a11y tree update (only the root node), but got {} nodes; Zed has no accessible UI elements yet", - tree_update.nodes.len() - ); - } +/// Builder API for synthetic children. See the docs for +/// [`Element::a11y_synthetic_children`]. +pub struct A11ySubtreeBuilder<'a> { + parent_id: NodeId, + nodes: &'a mut A11yNodeBuilder, +} - tree_update +impl<'a> A11ySubtreeBuilder<'a> { + pub(crate) fn new(parent_id: NodeId, nodes: &'a mut A11yNodeBuilder) -> Self { + Self { parent_id, nodes } + } + + /// Derive a [`NodeId`] for a synthetic child. + /// + /// The generated ID is based on the hash of `key`, as well as the parent's + /// ID. This means that `key`s must be unique within the same + /// [`Element::a11y_synthetic_children`] call, but may be duplicated across + /// different calls. + pub fn synthetic_node_id(&self, key: impl Hash) -> NodeId { + let mut hasher = std::hash::DefaultHasher::default(); + self.parent_id.0.hash(&mut hasher); + key.hash(&mut hasher); + NodeId(hasher.finish()) + } + + /// Append a synthetic leaf node as a child of this element's node. + /// + /// Returns `false` if a node with this id is already present in the tree, + /// in which case the node is discarded. + pub fn push_child(&mut self, id: NodeId, node: accesskit::Node) -> bool { + self.nodes.push_leaf(id, node) + } + + /// A mutable reference to the parent node. + pub fn parent_node(&mut self) -> &mut accesskit::Node { + self.nodes + .current_node_mut() + .expect("A11ySubtreeBuilder exists only while its element's node is on the stack") } } @@ -194,9 +289,14 @@ pub(crate) struct A11yNodeBuilder { /// `HashMap` to remove the need for `seen_ids` all_nodes: Vec<(NodeId, accesskit::Node)>, seen_ids: FxHashSet, - focus: NodeId, - #[cfg(debug_assertions)] - has_set_focus: bool, + /// The node that GPUI considers focused. Note that this may be different to + /// what is reported to accesskit - see [`Self::active_descendant`] + focus: Option, + /// If a node calls `.aria_active_descendant()`, AND an ancestor is focused, + /// override it as the focused node. This supports the "active descendant" + /// pattern, which allows a focused container to act as if a descendant is + /// focused. + active_descendant: Option, } impl A11yNodeBuilder { @@ -206,26 +306,32 @@ impl A11yNodeBuilder { nodes_stack: SmallVec::new(), all_nodes: Vec::new(), seen_ids: FxHashSet::default(), - focus: ROOT_NODE_ID, - #[cfg(debug_assertions)] - has_set_focus: false, + focus: None, + active_descendant: None, } } + #[must_use] + fn can_push(&mut self, id: NodeId) -> bool { + debug_assert!(!self.ids_stack.is_empty(), "node pushed before push_root"); + + if !self.seen_ids.insert(id) { + debug_assert!( + false, + "Duplicate a11y node id: {id:?}. In a release build, this node would be silently discarded from the a11y tree." + ); + return false; + } + + true + } + /// Push a new node onto the stack. It becomes a child of the current /// top-of-stack node. /// /// Returns `true` if the node was successfully pushed. pub(crate) fn push(&mut self, id: NodeId, node: accesskit::Node) -> bool { - debug_assert!(!self.ids_stack.is_empty(), "push called before push_root"); - - if !self.seen_ids.insert(id) { - debug_assert!( - false, - "Duplicate a11y node id: {id:?}. In a release build, this node would be silently discarded from the a11y tree." - ); - // We need to return `false` here because inserting a duplicate - // node will cause a panic in accesskit + if !self.can_push(id) { return false; } @@ -237,6 +343,27 @@ impl A11yNodeBuilder { true } + /// Add a leaf node as a child of the current top-of-stack node, without + /// pushing it onto the stack. Semantically equivalent to a [`Self::push`] + /// followed by a [`Self::pop`]. + /// + /// Returns `true` if the node was successfully pushed. + pub(crate) fn push_leaf(&mut self, id: NodeId, node: accesskit::Node) -> bool { + if !self.can_push(id) { + return false; + } + + if let Some(parent) = self.nodes_stack.last_mut() { + parent.push_child(id); + } + self.all_nodes.push((id, node)); + true + } + + pub(crate) fn current_node_mut(&mut self) -> Option<&mut accesskit::Node> { + self.nodes_stack.last_mut() + } + /// Pop the current node off the stack and finalize it into the all_nodes /// list. pub(crate) fn pop(&mut self) { @@ -248,20 +375,20 @@ impl A11yNodeBuilder { } /// Push the root node to start a new frame. - fn begin_frame(&mut self) { + fn begin_frame(&mut self, window_title: Option<&SharedString>) { self.all_nodes.clear(); self.ids_stack.clear(); self.nodes_stack.clear(); self.seen_ids.clear(); - #[cfg(debug_assertions)] - { - self.has_set_focus = false; + let mut root_node = accesskit::Node::new(accesskit::Role::Window); + if let Some(title) = window_title { + root_node.set_label(title.to_string()); } - let root_node = accesskit::Node::new(accesskit::Role::Window); self.ids_stack.push(ROOT_NODE_ID); self.nodes_stack.push(root_node); - self.focus = ROOT_NODE_ID; + self.focus = None; + self.active_descendant = None; } /// Returns whether a node with the given ID has been pushed in this frame. @@ -269,17 +396,51 @@ impl A11yNodeBuilder { id == ROOT_NODE_ID || self.seen_ids.contains(&id) } - /// Set the focused node for this frame. - pub(crate) fn set_focus(&mut self, id: NodeId) { - #[cfg(debug_assertions)] + /// Returns whether `id` is the node currently reported as focused. + pub(crate) fn node_is_focused(&self, id: NodeId) -> bool { + self.focus == Some(id) + } + + pub(crate) fn focus_is_ancestor_of_current(&self) -> bool { + let Some(focus) = self.focus else { + return false; + }; + + // The current node is on top of the stack; everything below it is an + // ancestor. + let ancestor_count = self.ids_stack.len().saturating_sub(1); + self.ids_stack[..ancestor_count].contains(&focus) + } + + pub(crate) fn set_active_descendant(&mut self, id: NodeId) { + if self + .active_descendant + .is_some_and(|existing| existing != id) { - debug_assert!( - !self.has_set_focus, - "set_focus called more than once in a single frame" - ); - self.has_set_focus = true; + if cfg!(debug_assertions) { + panic!("active descendant claimed by multiple nodes in one frame"); + } else { + log::warn!( + "a11y: multiple nodes claimed the active descendant this frame; \ + using last-wins ({id:?})" + ); + } } - self.focus = id; + self.active_descendant = Some(id); + } + + pub(crate) fn set_focus(&mut self, id: NodeId) { + if self.focus.is_some() { + if cfg!(debug_assertions) { + panic!("set_focus called more than once in a single frame"); + } else { + log::warn!( + "a11y: set_focus called more than once in a single frame; \ + using last-wins ({id:?})" + ); + } + } + self.focus = Some(id); } fn finalize(&mut self) -> TreeUpdate { @@ -302,12 +463,26 @@ impl A11yNodeBuilder { } } + let focus = match self.active_descendant { + Some(id) if self.has_node(id) => id, + Some(id) => { + if cfg!(debug_assertions) { + panic!("active_descendant set to {id:?}, which is not in the tree"); + } else { + log::warn!("active_descendant set to {id:?}, which is not in the tree"); + self.focus.unwrap_or(ROOT_NODE_ID) + } + } + + _ => self.focus.unwrap_or(ROOT_NODE_ID), + }; + let nodes = std::mem::take(&mut self.all_nodes); let update = TreeUpdate { nodes, tree: Some(accesskit::Tree::new(ROOT_NODE_ID)), tree_id: accesskit::TreeId::ROOT, - focus: self.focus, + focus, }; Self::repair_tree_update(update) @@ -359,3 +534,261 @@ impl A11yNodeBuilder { update } } + +#[cfg(test)] +mod tests { + // Import specific items rather than glob-importing `super`, which would pull + // in gpui's own `test` attribute macro and shadow the standard one. + use super::{A11y, A11yNodeBuilder, ROOT_NODE_ID}; + use crate::FocusId; + use accesskit::{NodeId, Role}; + use std::sync::{Arc, atomic::AtomicBool}; + + fn test_node() -> accesskit::Node { + accesskit::Node::new(Role::GenericContainer) + } + + fn new_builder() -> A11yNodeBuilder { + let mut builder = A11yNodeBuilder::new(); + builder.begin_frame(None); + builder + } + + fn new_a11y() -> A11y { + let mut a11y = A11y::new(Arc::new(AtomicBool::new(true)), false, None); + a11y.begin_frame(); + a11y + } + + #[test] + fn active_descendant_honored_when_container_focused() { + let mut builder = new_builder(); + let container = NodeId(1); + let item = NodeId(2); + + assert!(builder.push(container, test_node())); + builder.set_focus(container); + assert!(builder.push(item, test_node())); + + // The item is on top of the stack; the focused container is its + // ancestor, so the claim is honored. + assert!(builder.focus_is_ancestor_of_current()); + builder.set_active_descendant(item); + + builder.pop(); // item + builder.pop(); // container + let update = builder.finalize(); + assert_eq!(update.focus, item); + } + + #[test] + fn active_descendant_honored_for_deep_descendant() { + let mut builder = new_builder(); + let container = NodeId(1); + let group = NodeId(2); + let item = NodeId(3); + + assert!(builder.push(container, test_node())); + builder.set_focus(container); + assert!(builder.push(group, test_node())); + assert!(builder.push(item, test_node())); + + // The item is a grandchild of the focused container; depth doesn't + // matter, the focused ancestor is still on the stack. + assert!(builder.focus_is_ancestor_of_current()); + builder.set_active_descendant(item); + + builder.pop(); // item + builder.pop(); // group + builder.pop(); // container + let update = builder.finalize(); + assert_eq!(update.focus, item); + } + + #[test] + fn active_descendant_ignored_when_focus_in_other_subtree() { + let mut builder = new_builder(); + let focused_container = NodeId(1); + let focused_leaf = NodeId(2); + let other_container = NodeId(3); + let other_item = NodeId(4); + + // First subtree holds real focus. + assert!(builder.push(focused_container, test_node())); + assert!(builder.push(focused_leaf, test_node())); + builder.set_focus(focused_leaf); + builder.pop(); // focused_leaf + builder.pop(); // focused_container + + // Second subtree: its item would claim the active descendant, but the + // focus is not on any of its ancestors, so the gate rejects it. + assert!(builder.push(other_container, test_node())); + assert!(builder.push(other_item, test_node())); + assert!(!builder.focus_is_ancestor_of_current()); + builder.pop(); // other_item + builder.pop(); // other_container + + let update = builder.finalize(); + assert_eq!(update.focus, focused_leaf); + } + + #[test] + fn active_descendant_ignored_when_nothing_focused() { + let mut builder = new_builder(); + let container = NodeId(1); + let item = NodeId(2); + + assert!(builder.push(container, test_node())); + assert!(builder.push(item, test_node())); + + // Nothing is focused (focus defaults to the root window node), so the + // gate rejects the claim. + assert!(!builder.focus_is_ancestor_of_current()); + builder.pop(); + builder.pop(); + + let update = builder.finalize(); + assert_eq!(update.focus, ROOT_NODE_ID); + } + + #[test] + fn regular_focus_used_when_no_active_descendant() { + let mut builder = new_builder(); + let focused = NodeId(1); + + assert!(builder.push(focused, test_node())); + builder.set_focus(focused); + builder.pop(); + + let update = builder.finalize(); + assert_eq!(update.focus, focused); + } + + #[test] + fn focus_is_ancestor_excludes_self_and_non_ancestors() { + let mut builder = new_builder(); + let container = NodeId(1); + let item = NodeId(2); + + assert!(builder.push(container, test_node())); + builder.set_focus(container); + + // With the focused container itself on top, it is not its own (strict) + // ancestor, so the gate is false. + assert!(!builder.focus_is_ancestor_of_current()); + + assert!(builder.push(item, test_node())); + // Now the focused container is a strict ancestor of the item on top. + assert!(builder.focus_is_ancestor_of_current()); + + builder.pop(); + builder.pop(); + } + + // The double-claim guard panics only in debug builds; in release it falls + // back to last-wins with a warning. + #[test] + #[cfg_attr( + debug_assertions, + should_panic(expected = "active descendant claimed by multiple nodes") + )] + fn multiple_active_descendant_claims_panic_in_debug() { + let mut builder = new_builder(); + builder.set_active_descendant(NodeId(1)); + builder.set_active_descendant(NodeId(2)); + } + + // Setting focus twice in one frame means two elements both claimed window + // focus; that panics in debug and falls back to last-wins in release. + #[test] + #[cfg_attr( + debug_assertions, + should_panic(expected = "set_focus called more than once") + )] + fn setting_focus_twice_panics_in_debug() { + let mut builder = new_builder(); + builder.set_focus(NodeId(1)); + builder.set_focus(NodeId(2)); + } + + // Focusing a node that was never registered as focusable is a bug: panic in + // debug, warn in release. + #[test] + #[cfg_attr( + debug_assertions, + should_panic(expected = "was not registered with set_focusable") + )] + fn set_focus_without_set_focusable() { + let mut a11y = new_a11y(); + let node = NodeId(1); + assert!(a11y.nodes.push(node, test_node())); + // set_focusable was never called for `node`. + a11y.set_focus(node); + } + + // The focused node cannot also be its own active descendant: panic in + // debug, warn in release. + #[test] + #[cfg_attr(debug_assertions, should_panic(expected = "on the focused node"))] + fn set_active_descendant_on_focused_node() { + let mut a11y = new_a11y(); + let node = NodeId(1); + assert!(a11y.nodes.push(node, test_node())); + a11y.set_focusable(node, FocusId::default()); + a11y.set_focus(node); + a11y.set_active_descendant(node); + } + + // Two sibling children of a focused container both claim the active + // descendant (both pass the focus gate). The second claim is a bug: panic + // in debug, last-wins + warn in release. + #[test] + #[cfg_attr( + debug_assertions, + should_panic(expected = "active descendant claimed by multiple nodes") + )] + fn two_siblings_claiming_active_descendant() { + let mut a11y = new_a11y(); + let container = NodeId(1); + let first = NodeId(2); + let second = NodeId(3); + + assert!(a11y.nodes.push(container, test_node())); + a11y.set_focusable(container, FocusId::default()); + a11y.set_focus(container); + + assert!(a11y.nodes.push(first, test_node())); + a11y.set_active_descendant(first); + a11y.nodes.pop(); // first + + assert!(a11y.nodes.push(second, test_node())); + a11y.set_active_descendant(second); + a11y.nodes.pop(); // second + + a11y.nodes.pop(); // container + } + + // Node A is focused; node C (a child of the unfocused node B) claims the + // active descendant. The final tree must still report A as focused. + #[test] + fn active_descendant_in_unfocused_subtree_keeps_real_focus() { + let mut a11y = new_a11y(); + let a = NodeId(1); + let b = NodeId(2); + let c = NodeId(3); + + assert!(a11y.nodes.push(a, test_node())); + a11y.set_focusable(a, FocusId::default()); + a11y.set_focus(a); + a11y.nodes.pop(); // a + + assert!(a11y.nodes.push(b, test_node())); + assert!(a11y.nodes.push(c, test_node())); + a11y.set_active_descendant(c); + a11y.nodes.pop(); // c + a11y.nodes.pop(); // b + + let update = a11y.end_frame(); + assert_eq!(update.focus, a); + } +} diff --git a/crates/settings_ui/src/components/dropdown.rs b/crates/settings_ui/src/components/dropdown.rs index 37d18a8349..ca2faa486d 100644 --- a/crates/settings_ui/src/components/dropdown.rs +++ b/crates/settings_ui/src/components/dropdown.rs @@ -1,6 +1,6 @@ use std::rc::Rc; -use gpui::{App, ElementId, IntoElement, RenderOnce}; +use gpui::{App, ElementId, IntoElement, RenderOnce, SharedString}; use heck::ToTitleCase as _; use ui::{ ButtonSize, ContextMenu, Disableable as _, DropdownMenu, DropdownStyle, FluentBuilder as _, @@ -19,6 +19,7 @@ where should_do_title_case: bool, tab_index: Option, disabled: bool, + aria_label: Option, on_change: Rc, } @@ -41,6 +42,7 @@ where should_do_title_case: true, tab_index: None, disabled: false, + aria_label: None, on_change: Rc::new(on_change), } } @@ -59,6 +61,13 @@ where self.disabled = disabled; self } + + /// Sets the label announced by assistive technology. + /// Defaults to the currently selected value's label. + pub fn aria_label(mut self, label: impl Into) -> Self { + self.aria_label = Some(label.into()); + self + } } impl RenderOnce for EnumVariantDropdown @@ -104,6 +113,7 @@ where }, context_menu, ) + .when_some(self.aria_label, |this, label| this.aria_label(label)) .disabled(self.disabled) .when_some(self.tab_index, |elem, tab_index| elem.tab_index(tab_index)) .trigger_size(ButtonSize::Medium) diff --git a/crates/settings_ui/src/components/input_field.rs b/crates/settings_ui/src/components/input_field.rs index 90bc048480..81b98c14fc 100644 --- a/crates/settings_ui/src/components/input_field.rs +++ b/crates/settings_ui/src/components/input_field.rs @@ -1,14 +1,18 @@ use std::rc::Rc; -use editor::Editor; -use gpui::{AnyElement, ElementId, Focusable, TextStyleRefinement}; +use editor::{Editor, MultiBufferOffset}; +use gpui::{ + A11ySubtreeBuilder, AccessibleAction, AnyElement, ElementId, Entity, Focusable, Role, + TextStyleRefinement, + accesskit::{self, ActionData}, +}; use settings::Settings as _; use theme_settings::ThemeSettings; use ui::{Tooltip, prelude::*, rems}; #[derive(IntoElement)] pub struct SettingsInputField { - id: Option, + id: ElementId, initial_text: Option, placeholder: Option<&'static str>, confirm: Option, &mut Window, &mut App)>>, @@ -20,12 +24,19 @@ pub struct SettingsInputField { confirm_on_focus_out: bool, action_slot: Option, color: Option, + aria_label: Option, } impl SettingsInputField { - pub fn new() -> Self { + /// Creates a new input field. + /// + /// The `id` must be unique among sibling elements: it keys the underlying + /// editor's state across frames and identifies this field in the + /// accessibility tree. Derive it from data unique to the setting being + /// edited (e.g. its JSON path). + pub fn new(id: impl Into) -> Self { Self { - id: None, + id: id.into(), initial_text: None, placeholder: None, confirm: None, @@ -37,14 +48,10 @@ impl SettingsInputField { confirm_on_focus_out: false, action_slot: None, color: None, + aria_label: None, } } - pub fn with_id(mut self, id: impl Into) -> Self { - self.id = Some(id.into()); - self - } - pub fn with_initial_text(mut self, initial_text: String) -> Self { self.initial_text = Some(initial_text); self @@ -102,6 +109,13 @@ impl SettingsInputField { self.color = Some(color); self } + + /// Sets the label announced by assistive technology. + /// Defaults to the placeholder text, if any. + pub fn aria_label(mut self, label: impl Into) -> Self { + self.aria_label = Some(label.into()); + self + } } impl RenderOnce for SettingsInputField { @@ -116,83 +130,48 @@ impl RenderOnce for SettingsInputField { ..Default::default() }; - let first_render_initial_text = window.use_state(cx, |_, _| self.initial_text.clone()); + let first_render_initial_text = window.use_keyed_state( + (self.id.clone(), "first-render-initial-text"), + cx, + |_, _| self.initial_text.clone(), + ); - let editor = if let Some(id) = self.id { - window.use_keyed_state(id, cx, { - let initial_text = self.initial_text.clone(); - let placeholder = self.placeholder; - let mut confirm = self.confirm.clone(); + let editor = window.use_keyed_state((self.id.clone(), "editor"), cx, { + let initial_text = self.initial_text.clone(); + let placeholder = self.placeholder; + let mut confirm = self.confirm.clone(); - move |window, cx| { - let mut editor = Editor::single_line(window, cx); - let editor_focus_handle = editor.focus_handle(cx); - if let Some(text) = initial_text { - editor.set_text(text, window, cx); - } - - if let Some(confirm) = confirm.take() - && (self.confirm_on_focus_out - || (!self.display_confirm_button - && !self.display_clear_button - && !self.clear_on_confirm)) - { - cx.on_focus_out( - &editor_focus_handle, - window, - move |editor, _, window, cx| { - let text = Some(editor.text(cx)); - confirm(text, window, cx); - }, - ) - .detach(); - } - - if let Some(placeholder) = placeholder { - editor.set_placeholder_text(placeholder, window, cx); - } - editor.set_text_style_refinement(styles); - editor + move |window, cx| { + let mut editor = Editor::single_line(window, cx); + let editor_focus_handle = editor.focus_handle(cx); + if let Some(text) = initial_text { + editor.set_text(text, window, cx); } - }) - } else { - window.use_state(cx, { - let initial_text = self.initial_text.clone(); - let placeholder = self.placeholder; - let mut confirm = self.confirm.clone(); - move |window, cx| { - let mut editor = Editor::single_line(window, cx); - let editor_focus_handle = editor.focus_handle(cx); - if let Some(text) = initial_text { - editor.set_text(text, window, cx); - } - - if let Some(confirm) = confirm.take() - && (self.confirm_on_focus_out - || (!self.display_confirm_button - && !self.display_clear_button - && !self.clear_on_confirm)) - { - cx.on_focus_out( - &editor_focus_handle, - window, - move |editor, _, window, cx| { - let text = Some(editor.text(cx)); - confirm(text, window, cx); - }, - ) - .detach(); - } - - if let Some(placeholder) = placeholder { - editor.set_placeholder_text(placeholder, window, cx); - } - editor.set_text_style_refinement(styles); - editor + if let Some(confirm) = confirm.take() + && (self.confirm_on_focus_out + || (!self.display_confirm_button + && !self.display_clear_button + && !self.clear_on_confirm)) + { + cx.on_focus_out( + &editor_focus_handle, + window, + move |editor, _, window, cx| { + let text = Some(editor.text(cx)); + confirm(text, window, cx); + }, + ) + .detach(); } - }) - }; + + if let Some(placeholder) = placeholder { + editor.set_placeholder_text(placeholder, window, cx); + } + editor.set_text_style_refinement(styles); + editor + } + }); let is_editor_focused = editor.read(cx).is_focused(window); let editor_text = editor.read(cx).text(cx); @@ -224,14 +203,49 @@ impl RenderOnce for SettingsInputField { let clear_on_confirm = self.clear_on_confirm; let clear_on_confirm_for_button = self.clear_on_confirm; - let theme_colors = cx.theme().colors(); - let display_confirm_button = self.display_confirm_button; let display_clear_button = self.display_clear_button; let confirm_for_button = self.confirm.clone(); let is_editor_empty = editor_text.trim().is_empty(); + let aria_label = self + .aria_label + .or_else(|| self.placeholder.map(SharedString::new_static)); + + let (a11y_value, a11y_text_runs) = + text_field_a11y_state(self.id.clone(), &editor, window, cx); + + let theme_colors = cx.theme().colors(); + h_flex() + .id(self.id.clone()) + .role(Role::TextInput) + .when_some(aria_label, |this, label| this.aria_label(label)) + .aria_value(a11y_value) + .when_some(self.placeholder, |this, placeholder| { + this.aria_placeholder(placeholder) + }) + .a11y_synthetic_children(a11y_text_runs) + .on_a11y_action(AccessibleAction::SetValue, { + let weak_editor = editor.downgrade(); + let confirm = self.confirm.clone(); + move |data, window, cx| { + let Some(ActionData::Value(text)) = data else { + return; + }; + let Some(editor) = weak_editor.upgrade() else { + return; + }; + let text = text.to_string(); + editor.update(cx, |editor, cx| { + editor.set_text(text.clone(), window, cx); + }); + if let Some(confirm) = confirm.as_ref() { + let new_value = (!text.is_empty()).then_some(text); + confirm(new_value, window, cx); + } + } + }) .group("settings-input-field-editor") .relative() .py_1() @@ -242,8 +256,13 @@ impl RenderOnce for SettingsInputField { .border_1() .border_color(theme_colors.border) .bg(theme_colors.editor_background) - .when_some(self.tab_index, |this, tab_index| { - let focus_handle = editor.focus_handle(cx).tab_index(tab_index).tab_stop(true); + .map(|this| { + let focus_handle = editor.focus_handle(cx); + let focus_handle = if let Some(tab_index) = self.tab_index { + focus_handle.tab_index(tab_index).tab_stop(true) + } else { + focus_handle + }; this.track_focus(&focus_handle) .focus(|s| s.border_color(theme_colors.border_focused)) }) @@ -263,6 +282,7 @@ impl RenderOnce for SettingsInputField { IconButton::new("clear-button", IconName::Close) .icon_size(IconSize::Small) .icon_color(Color::Muted) + .aria_label("Clear") .tooltip(Tooltip::text("Clear")) .on_click(move |_, window, cx| { let Some(editor) = weak_editor_for_clear.upgrade() else { @@ -282,6 +302,7 @@ impl RenderOnce for SettingsInputField { IconButton::new("confirm-button", IconName::Check) .icon_size(IconSize::Small) .icon_color(Color::Success) + .aria_label("Confirm") .tooltip(Tooltip::text("Enter to Confirm")) .on_click(move |_, window, cx| { let Some(confirm) = confirm_for_button.as_ref() else { @@ -325,3 +346,240 @@ impl RenderOnce for SettingsInputField { }) } } + +/// Compute the shared accessibility state for a focusable wrapper around a +/// single-line [`Editor`]: +/// +/// - The value to report via `aria_value`. While the editor is focused this +/// is frozen at its focus-time content: screen readers announce the full value +/// on every change of a focused control, which would re-read the whole content +/// on each keystroke. The snapshot re-syncs on blur. +/// - A closure for `a11y_synthetic_children` exposing the editor's live text +/// and selection as AccessKit text runs, enabling the platform text +/// pattern (caret tracking, review commands, typed-character echo). +/// +/// The caller must also give the element an id, a text input role (e.g. +/// [`Role::TextInput`]), a label, and track the editor's focus handle. +/// +/// All work is skipped when accessibility is inactive (no assistive +/// technology connected), since the results are only observable through the +/// accessibility tree. +/// +/// Note: much of this may want +pub(crate) fn text_field_a11y_state( + state_key: impl Into, + editor: &Entity, + window: &mut Window, + cx: &mut App, +) -> (String, impl FnOnce(&mut A11ySubtreeBuilder) + 'static) { + let state = window.is_a11y_active().then(|| { + let (text, selection_head, selection_tail) = editor.update(cx, |editor, cx| { + let display_snapshot = editor.display_snapshot(cx); + let selection = editor + .selections + .newest::(&display_snapshot); + (editor.text(cx), selection.head().0, selection.tail().0) + }); + let is_focused = editor.read(cx).is_focused(window); + + let a11y_value = window.use_keyed_state((state_key.into(), "a11y-value"), cx, { + let text = text.clone(); + move |_, _| text + }); + if !is_focused && *a11y_value.read(cx) != text { + *a11y_value.as_mut(cx) = text.clone(); + } + let frozen_value = a11y_value.read(cx).clone(); + + (frozen_value, text, selection_head, selection_tail) + }); + + let (frozen_value, run_data) = match state { + Some((frozen_value, text, selection_head, selection_tail)) => { + (frozen_value, Some((text, selection_head, selection_tail))) + } + None => (String::new(), None), + }; + + let text_runs = move |builder: &mut A11ySubtreeBuilder| { + if let Some((text, selection_head, selection_tail)) = run_data { + push_a11y_text_runs(builder, &text, selection_tail, selection_head); + } + }; + + (frozen_value, text_runs) +} + +/// AccessKit's `word_starts` uses `u8` indices, so a single text run cannot +/// exceed this many characters. Longer text is split into multiple runs. +const MAX_CHARS_PER_TEXT_RUN: usize = 255; + +fn is_word_char(c: char) -> bool { + c.is_alphanumeric() || c == '_' +} + +fn char_index_for_byte(text: &str, byte_offset: usize) -> usize { + text.char_indices() + .take_while(|(byte_ix, _)| *byte_ix < byte_offset) + .count() +} + +/// Convert a character index into an AccessKit text position, accounting for +/// text that is split into multiple runs. +/// +/// `synthetic_node_id` maps a chunk index to the run's node id (in practice +/// [`A11ySubtreeBuilder::synthetic_node_id`]); it is a parameter so this +/// arithmetic can be property-tested without constructing a builder. +fn a11y_text_position( + char_index: usize, + synthetic_node_id: impl Fn(u64) -> accesskit::NodeId, +) -> accesskit::TextPosition { + // A position landing exactly on a chunk boundary refers to the end of the + // previous chunk rather than the start of the next one. + let chunk_index = if char_index > 0 && char_index.is_multiple_of(MAX_CHARS_PER_TEXT_RUN) { + char_index / MAX_CHARS_PER_TEXT_RUN - 1 + } else { + char_index / MAX_CHARS_PER_TEXT_RUN + }; + accesskit::TextPosition { + node: synthetic_node_id(chunk_index as u64), + character_index: char_index - chunk_index * MAX_CHARS_PER_TEXT_RUN, + } +} + +/// Split `text` into AccessKit text runs (chunked small enough that per-run +/// character indices fit AccessKit's `u8`-indexed `word_starts`), and compute +/// the text selection for the given byte offsets. +/// +/// `synthetic_node_id` maps a chunk index to that run's node id. Returns the +/// runs in order plus the selection, leaving it to the caller to push them — +/// this keeps the logic free of [`A11ySubtreeBuilder`] so it can be +/// property-tested against arbitrary strings. +/// +/// `selection_tail` and `selection_head` are byte offsets into `text`. +fn build_a11y_text_runs( + text: &str, + selection_tail: usize, + selection_head: usize, + synthetic_node_id: impl Fn(u64) -> accesskit::NodeId, +) -> ( + Vec<(accesskit::NodeId, accesskit::Node)>, + accesskit::TextSelection, +) { + let chars: Vec = text.chars().collect(); + let total_chars = chars.len(); + // Build at least one (possibly empty) run so the text pattern remains + // supported when the field is empty. + let num_chunks = total_chars.div_ceil(MAX_CHARS_PER_TEXT_RUN).max(1); + + let mut word_starts = Vec::new(); + let mut was_word_char = false; + for (ix, c) in chars.iter().enumerate() { + let is_word = is_word_char(*c); + if is_word && !was_word_char { + word_starts.push(ix); + } + was_word_char = is_word; + } + + let mut runs = Vec::with_capacity(num_chunks); + for chunk_index in 0..num_chunks { + let char_start = chunk_index * MAX_CHARS_PER_TEXT_RUN; + let char_end = (char_start + MAX_CHARS_PER_TEXT_RUN).min(total_chars); + let chunk_chars = &chars[char_start..char_end]; + + let mut node = accesskit::Node::new(accesskit::Role::TextRun); + node.set_text_direction(accesskit::TextDirection::LeftToRight); + node.set_value(chunk_chars.iter().collect::()); + node.set_character_lengths( + chunk_chars + .iter() + .map(|c| c.len_utf8() as u8) + .collect::>(), + ); + node.set_word_starts( + word_starts + .iter() + .filter(|&&word_start| word_start >= char_start && word_start < char_end) + .map(|&word_start| (word_start - char_start) as u8) + .collect::>(), + ); + if chunk_index > 0 { + node.set_previous_on_line(synthetic_node_id(chunk_index as u64 - 1)); + } + if chunk_index + 1 < num_chunks { + node.set_next_on_line(synthetic_node_id(chunk_index as u64 + 1)); + } + + runs.push((synthetic_node_id(chunk_index as u64), node)); + } + + let anchor = a11y_text_position( + char_index_for_byte(text, selection_tail), + &synthetic_node_id, + ); + let focus = a11y_text_position( + char_index_for_byte(text, selection_head), + &synthetic_node_id, + ); + (runs, accesskit::TextSelection { anchor, focus }) +} + +/// Expose the field's text content as AccessKit text runs, plus the current +/// selection/caret, enabling the platform's text pattern (e.g. UIA +/// TextPattern on Windows) so screen readers can track the caret, review the +/// text, and handle typed-character echo natively. +/// +/// `selection_tail` and `selection_head` are byte offsets into `text`. +fn push_a11y_text_runs( + builder: &mut A11ySubtreeBuilder, + text: &str, + selection_tail: usize, + selection_head: usize, +) { + let (runs, selection) = build_a11y_text_runs(text, selection_tail, selection_head, |chunk| { + builder.synthetic_node_id(chunk) + }); + for (id, node) in runs { + builder.push_child(id, node); + } + builder.parent_node().set_text_selection(selection); +} + +#[cfg(test)] +mod tests { + use super::build_a11y_text_runs; + use gpui::accesskit::NodeId; + use gpui::proptest::strategy::Strategy; + + /// A strategy producing strings with a deliberate mix of character + /// categories — ASCII, Latin accents, Cyrillic, Arabic, CJK, emoji, and + /// arbitrary scalars — so run-splitting is exercised across scripts and + /// byte widths (1–4 UTF-8 bytes). Lengths reach past one chunk (255 chars). + fn arbitrary_text() -> impl Strategy { + let character = gpui::proptest::prop_oneof![ + gpui::proptest::char::range(' ', '~'), // ASCII printable + gpui::proptest::char::range('\u{00A1}', '\u{00FF}'), // Latin-1 (accents) + gpui::proptest::char::range('\u{0100}', '\u{024F}'), // Latin Extended-A/B + gpui::proptest::char::range('\u{0400}', '\u{04FF}'), // Cyrillic + gpui::proptest::char::range('\u{0600}', '\u{06FF}'), // Arabic + gpui::proptest::char::range('\u{4E00}', '\u{9FFF}'), // CJK Unified Ideographs + gpui::proptest::char::range('\u{1F300}', '\u{1FAFF}'), // emoji & pictographs + gpui::proptest::char::any(), // anything else + ]; + gpui::proptest::collection::vec(character, 0..600) + .prop_map(|chars| chars.into_iter().collect::()) + } + + /// Splitting an arbitrary string into AccessKit text runs must never panic, + /// for any text and any byte selection offsets — including empty text, text + /// spanning multiple chunks, multi-byte characters, and offsets past the end. + #[gpui::property_test] + fn building_text_runs_never_panics( + #[strategy = arbitrary_text()] text: String, + selection_tail: usize, + selection_head: usize, + ) { + let _ = build_a11y_text_runs(&text, selection_tail, selection_head, NodeId); + } +} diff --git a/crates/settings_ui/src/components/number_field.rs b/crates/settings_ui/src/components/number_field.rs index 7ffec007fe..011c517e95 100644 --- a/crates/settings_ui/src/components/number_field.rs +++ b/crates/settings_ui/src/components/number_field.rs @@ -7,8 +7,8 @@ use std::{ use editor::Editor; use gpui::{ - ClickEvent, Entity, FocusHandle, Focusable, FontWeight, Modifiers, TextAlign, - TextStyleRefinement, WeakEntity, + AccessibleAction, ClickEvent, Entity, FocusHandle, Focusable, FontWeight, Modifiers, Role, + TextAlign, TextStyleRefinement, WeakEntity, }; use settings::{ @@ -274,6 +274,7 @@ pub struct NumberField { on_reset: Option>, on_change: Rc, tab_index: Option, + aria_label: Option, } impl NumberField { @@ -314,6 +315,7 @@ impl NumberField { on_reset: None, on_change: Rc::new(|_, _, _| {}), tab_index: None, + aria_label: None, } } @@ -342,6 +344,12 @@ impl NumberField { self } + /// Sets the label announced by assistive technology. + pub fn aria_label(mut self, label: impl Into) -> Self { + self.aria_label = Some(label.into()); + self + } + fn sync_on_change_state(&self, cx: &mut App) { self.on_change_state .update(cx, |state, _| *state = Some(self.on_change.clone())); @@ -354,6 +362,22 @@ enum ValueChangeDirection { Decrement, } +/// Best-effort conversion of a numeric field value to `f64` for reporting to +/// assistive technology. Goes through the `Display` representation because +/// `NumberFieldType` has no general numeric conversion. +fn a11y_numeric_value(value: &impl Display) -> Option { + format!("{}", value).parse::().ok() +} + +/// Best-effort conversion of an assistive-technology-provided `f64` back into +/// a field value. Falls back to the rounded value for integer field types. +fn a11y_value_to_field_value(value: f64) -> Option { + format!("{}", value) + .parse::() + .ok() + .or_else(|| format!("{}", value.round()).parse::().ok()) +} + impl RenderOnce for NumberField { fn render(self, window: &mut Window, cx: &mut App) -> impl IntoElement { // Sync the on_change callback to Entity state so focus_out handlers can access it @@ -437,6 +461,40 @@ impl RenderOnce for NumberField { }) }; + let step_value = { + let on_change = self.on_change.clone(); + let get_current_value = get_current_value.clone(); + let update_editor_text = update_editor_text.clone(); + + Rc::new( + move |direction: ValueChangeDirection, + modifiers: Modifiers, + window: &mut Window, + cx: &mut App| { + let current_value = get_current_value(cx); + let step = get_step(modifiers); + let new_value = change_value(current_value, step, direction); + + update_editor_text(new_value, window, cx); + on_change(&new_value, window, cx); + }, + ) + }; + + let set_value = { + let on_change = self.on_change.clone(); + let update_editor_text = update_editor_text.clone(); + + move |raw_value: f64, window: &mut Window, cx: &mut App| { + let Some(parsed) = a11y_value_to_field_value::(raw_value) else { + return; + }; + let new_value = clamp_value(parsed); + update_editor_text(new_value, window, cx); + on_change(&new_value, window, cx); + } + }; + let bg_color = cx.theme().colors().surface_background; let hover_bg_color = cx.theme().colors().element_hover; @@ -460,43 +518,86 @@ impl RenderOnce for NumberField { h_flex() .id(self.id.clone()) + .role(Role::SpinButton) + .when_some(self.aria_label.clone(), |this, label| { + this.aria_label(label) + }) + .when_some(a11y_numeric_value(&self.value), |this, value| { + this.aria_numeric_value(value) + }) + .when_some(a11y_numeric_value(&self.min_value), |this, min| { + this.aria_min_numeric_value(min) + }) + .when_some(a11y_numeric_value(&self.max_value), |this, max| { + this.aria_max_numeric_value(max) + }) + .when_some(a11y_numeric_value(&self.step), |this, step| { + this.aria_numeric_value_step(step) + }) + // Some assistive technology (e.g. Orca via AT-SPI's Value + // interface) can only set an absolute value, while other screen + // readers dispatch Increment/Decrement, so support all three. + .on_a11y_action(AccessibleAction::SetValue, { + move |data, window, cx| { + if let Some(gpui::accesskit::ActionData::NumericValue(value)) = data { + set_value(*value, window, cx); + } + } + }) + .on_a11y_action(AccessibleAction::Increment, { + let step_value = step_value.clone(); + move |_, window, cx| { + step_value( + ValueChangeDirection::Increment, + Modifiers::default(), + window, + cx, + ); + } + }) + .on_a11y_action(AccessibleAction::Decrement, { + let step_value = step_value.clone(); + move |_, window, cx| { + step_value( + ValueChangeDirection::Decrement, + Modifiers::default(), + window, + cx, + ); + } + }) .track_focus(&self.focus_handle) .gap_1() .when_some(self.on_reset, |this, on_reset| { this.child( IconButton::new("reset", IconName::RotateCcw) .icon_size(IconSize::Small) + .aria_label("Reset to Default") .when_some(self.tab_index, |this, _| this.tab_index(0isize)) .on_click(on_reset), ) }) .child({ - let on_change_for_increment = self.on_change.clone(); - h_flex() .map(|decrement| { let decrement_handler = { - let on_change = self.on_change.clone(); - let get_current_value = get_current_value.clone(); - let update_editor_text = update_editor_text.clone(); + let step_value = step_value.clone(); move |click: &ClickEvent, window: &mut Window, cx: &mut App| { - let current_value = get_current_value(cx); - let step = get_step(click.modifiers()); - let new_value = change_value( - current_value, - step, + step_value( ValueChangeDirection::Decrement, + click.modifiers(), + window, + cx, ); - - update_editor_text(new_value, window, cx); - on_change(&new_value, window, cx); } }; decrement.child( base_button(IconName::Dash) .id((self.id.clone(), "decrement_button")) + .role(Role::Button) + .aria_label("Decrement") .rounded_tl_sm() .rounded_bl_sm() .when_some(self.tab_index, |this, _| this.tab_index(0isize)) @@ -668,6 +769,11 @@ impl RenderOnce for NumberField { }; h_flex() + .id((self.id.clone(), "editor")) + .role(Role::TextInput) + .when_some(self.aria_label.clone(), |this, label| { + this.aria_label(label) + }) .flex_1() .h_full() .track_focus(&focus_handle) @@ -687,27 +793,23 @@ impl RenderOnce for NumberField { }) .map(|increment| { let increment_handler = { - let on_change = on_change_for_increment.clone(); - let get_current_value = get_current_value.clone(); - let update_editor_text = update_editor_text.clone(); + let step_value = step_value.clone(); move |click: &ClickEvent, window: &mut Window, cx: &mut App| { - let current_value = get_current_value(cx); - let step = get_step(click.modifiers()); - let new_value = change_value( - current_value, - step, + step_value( ValueChangeDirection::Increment, + click.modifiers(), + window, + cx, ); - - update_editor_text(new_value, window, cx); - on_change(&new_value, window, cx); } }; increment.child( base_button(IconName::Plus) .id((self.id.clone(), "increment_button")) + .role(Role::Button) + .aria_label("Increment") .rounded_tr_sm() .rounded_br_sm() .when_some(self.tab_index, |this, _| this.tab_index(0isize)) diff --git a/crates/settings_ui/src/components/section_items.rs b/crates/settings_ui/src/components/section_items.rs index 9c98a80737..d6bc4d0b2d 100644 --- a/crates/settings_ui/src/components/section_items.rs +++ b/crates/settings_ui/src/components/section_items.rs @@ -1,4 +1,4 @@ -use gpui::{IntoElement, ParentElement, Styled}; +use gpui::{IntoElement, ParentElement, Role, Styled}; use ui::{Divider, DividerColor, prelude::*}; #[derive(IntoElement)] @@ -30,12 +30,17 @@ impl SettingsSectionHeader { impl RenderOnce for SettingsSectionHeader { fn render(self, _: &mut Window, cx: &mut App) -> impl IntoElement { + let label_text = self.label.clone(); let label = Label::new(self.label) .size(LabelSize::Small) .color(Color::Muted) .buffer_font(cx); v_flex() + .id(label_text.clone()) + .role(Role::Heading) + .aria_level(2) + .aria_label(label_text) .w_full() .when(!self.no_padding, |this| this.px_8()) .gap_1p5() diff --git a/crates/settings_ui/src/pages/edit_prediction_provider_setup.rs b/crates/settings_ui/src/pages/edit_prediction_provider_setup.rs index 2e93f6c65c..0f7b9d512f 100644 --- a/crates/settings_ui/src/pages/edit_prediction_provider_setup.rs +++ b/crates/settings_ui/src/pages/edit_prediction_provider_setup.rs @@ -311,9 +311,10 @@ fn render_api_key_provider( }), ) .child( - SettingsInputField::new() + SettingsInputField::new(format!("{}-api-key-input", title)) .tab_index(0) .with_placeholder("xxxxxxxxxxxxxxxxxxxx") + .aria_label(format!("{} API Key", title)) .on_confirm(move |api_key, _window, cx| { write_key(api_key.filter(|key| !key.is_empty()), cx); }), diff --git a/crates/settings_ui/src/pages/sandbox_settings.rs b/crates/settings_ui/src/pages/sandbox_settings.rs index cf0000d1c2..b1d1c46ac8 100644 --- a/crates/settings_ui/src/pages/sandbox_settings.rs +++ b/crates/settings_ui/src/pages/sandbox_settings.rs @@ -214,8 +214,7 @@ fn render_host_row(index: usize, host: String, cx: &mut Context) let host_for_update = host.clone(); let settings_window = cx.entity().downgrade(); - SettingsInputField::new() - .with_id(format!("sandbox-host-{}", index)) + SettingsInputField::new(format!("sandbox-host-{}", index)) .with_initial_text(host) .tab_index(0) .with_buffer_font() @@ -259,8 +258,7 @@ fn render_host_row(index: usize, host: String, cx: &mut Context) fn render_add_host_input(cx: &mut Context) -> AnyElement { let settings_window = cx.entity().downgrade(); - SettingsInputField::new() - .with_id("sandbox-host-new") + SettingsInputField::new("sandbox-host-new") .with_placeholder("Add domain (e.g. github.com or *.npmjs.org)…") .tab_index(0) .with_buffer_font() @@ -299,8 +297,7 @@ fn render_path_row(index: usize, path: PathBuf, cx: &mut Context let path_for_update = path.clone(); let settings_window = cx.entity().downgrade(); - SettingsInputField::new() - .with_id(format!("sandbox-path-{}", index)) + SettingsInputField::new(format!("sandbox-path-{}", index)) .with_initial_text(path.to_string_lossy().into_owned()) .tab_index(0) .with_buffer_font() @@ -335,8 +332,7 @@ fn render_path_row(index: usize, path: PathBuf, cx: &mut Context fn render_add_path_input(cx: &mut Context) -> AnyElement { let settings_window = cx.entity().downgrade(); - SettingsInputField::new() - .with_id("sandbox-path-new") + SettingsInputField::new("sandbox-path-new") .with_placeholder("Add an absolute path (e.g. /path/to/directory)…") .tab_index(0) .with_buffer_font() diff --git a/crates/settings_ui/src/pages/tool_permissions_setup.rs b/crates/settings_ui/src/pages/tool_permissions_setup.rs index feb0ab173d..25e8d1254c 100644 --- a/crates/settings_ui/src/pages/tool_permissions_setup.rs +++ b/crates/settings_ui/src/pages/tool_permissions_setup.rs @@ -977,8 +977,7 @@ fn render_user_pattern_row( let delete_id = format!("{}-{:?}-delete-{}", tool_id, rule_type, index); let settings_window = cx.entity().downgrade(); - SettingsInputField::new() - .with_id(input_id) + SettingsInputField::new(input_id) .with_initial_text(pattern) .tab_index(0) .with_buffer_font() @@ -1038,8 +1037,7 @@ fn render_add_pattern_input( let input_id = format!("{}-{:?}-new-pattern", tool_id, rule_type); let settings_window = cx.entity().downgrade(); - SettingsInputField::new() - .with_id(input_id) + SettingsInputField::new(input_id) .with_placeholder("Add regex pattern…") .tab_index(0) .with_buffer_font() diff --git a/crates/settings_ui/src/settings_ui.rs b/crates/settings_ui/src/settings_ui.rs index 7e8967aa6d..1428c72901 100644 --- a/crates/settings_ui/src/settings_ui.rs +++ b/crates/settings_ui/src/settings_ui.rs @@ -10,11 +10,12 @@ use futures::{StreamExt, channel::mpsc}; use fuzzy::StringMatchCandidate; use gpui::{ Action, App, AsyncApp, ClipboardItem, DEFAULT_ADDITIONAL_WINDOW_SIZE, Div, Entity, FocusHandle, - Focusable, Global, KeyContext, ListState, ReadGlobal as _, ScrollHandle, Stateful, + Focusable, Global, KeyContext, ListState, ReadGlobal as _, Role, ScrollHandle, Stateful, Subscription, Task, Tiling, TitlebarOptions, UniformListScrollHandle, WeakEntity, Window, WindowBounds, WindowHandle, WindowOptions, actions, div, list, point, prelude::*, px, uniform_list, }; +use heck::ToTitleCase as _; use language::Buffer; use platform_title_bar::PlatformTitleBar; @@ -56,7 +57,7 @@ use zed_actions::{ use crate::components::{ EnumVariantDropdown, NumberField, NumberFieldMode, NumberFieldType, SettingsInputField, SettingsSectionHeader, font_picker, icon_theme_picker, render_ollama_model_picker, - theme_picker, + text_field_a11y_state, theme_picker, }; use crate::pages::{ CustomAgentForm, McpServerForm, render_input_audio_device_dropdown, @@ -1148,6 +1149,7 @@ impl SettingsPageItem { ("sub-page".into(), sub_page_link.title.clone()), "Configure", ) + .aria_label(format!("Configure {}", sub_page_link.title)) .tab_index(0_isize) .end_icon( Icon::new(IconName::ChevronRight) @@ -1322,6 +1324,8 @@ fn render_settings_item_layout( ) -> Stateful
{ h_flex() .id(title) + .role(Role::Group) + .aria_label(SharedString::new_static(title)) .min_w_0() .justify_between() .child( @@ -1340,6 +1344,7 @@ fn render_settings_item_layout( IconButton::new("reset-to-default-btn", IconName::Undo) .icon_color(Color::Muted) .icon_size(IconSize::Small) + .aria_label("Reset to Default") .tooltip(Tooltip::text("Reset to Default")) .on_click(move |_, window, cx| { reset_to_default(window, cx); @@ -1471,6 +1476,7 @@ fn render_settings_item_link( .icon_color(link_icon_color) .icon_size(IconSize::Small) .shape(IconButtonShape::Square) + .aria_label("Copy Link") .tooltip(Tooltip::text("Copy Link")) .when_some(json_path, |this, path| { this.on_click(cx.listener(move |this, _, _, cx| { @@ -2751,6 +2757,9 @@ impl SettingsWindow { let edit_in_json_id = SharedString::new(format!("edit-in-json-{}", selected_file_ix)); h_flex() + .id("settings-ui-files-header") + .role(Role::Group) + .aria_label("Settings File") .w_full() .gap_1() .justify_between() @@ -2888,8 +2897,17 @@ impl SettingsWindow { // } // } - fn render_search(&self, _window: &mut Window, cx: &mut App) -> Div { + fn render_search(&self, window: &mut Window, cx: &mut App) -> impl IntoElement { + let (a11y_value, a11y_text_runs) = + text_field_a11y_state("settings-ui-search", &self.search_bar, window, cx); + h_flex() + .id("settings-ui-search") + .role(Role::SearchInput) + .aria_label("Search Settings") + .aria_value(a11y_value) + .track_focus(&self.search_bar.focus_handle(cx)) + .a11y_synthetic_children(a11y_text_runs) .py_1() .px_1p5() .mb_3() @@ -3059,6 +3077,9 @@ impl SettingsWindow { .child(self.render_search(window, cx)) .child( v_flex() + .id("settings-ui-nav") + .role(Role::Tree) + .aria_label("Settings Navigation") .flex_1() .overflow_hidden() .track_focus(&self.navbar_focus_handle.focus_handle(cx)) @@ -3432,7 +3453,11 @@ impl SettingsWindow { cx: &mut Context, ) -> impl IntoElement { let current_page_index = self.current_page_index(); - let mut page_content = v_flex().id("settings-ui-page").size_full(); + let mut page_content = v_flex() + .id("settings-ui-page") + .role(Role::Group) + .aria_label("Settings Content") + .size_full(); let has_active_search = !self.search_bar.read(cx).is_empty(cx); let has_no_results = self.visible_page_items().next().is_none() && has_active_search; @@ -4685,6 +4710,12 @@ fn update_project_setting_file( Ok(()) } +/// Derives a human-readable label for assistive technology from a setting's +/// JSON path, e.g. `"buffer_font_size"` becomes `"Buffer Font Size"`. +fn a11y_label_for_json_path(json_path: Option<&'static str>) -> Option { + json_path.map(|path| SharedString::from(path.to_title_case())) +} + struct CurrentSettingsValue<'a, T> { value: &'a T, disabled: bool, @@ -4733,8 +4764,14 @@ fn render_text_field + Into + AsRef + Clone>( .map(|text| text.as_ref().to_string()) }; - SettingsInputField::new() + // The JSON path uniquely identifies the setting this field edits, making + // it a stable, collision-free element ID within the page. + SettingsInputField::new(field.json_path.unwrap_or("settings-text-field")) .tab_index(0) + .when_some( + a11y_label_for_json_path(field.json_path), + |editor, label| editor.aria_label(label), + ) .when_some(initial_text, |editor, text| editor.with_initial_text(text)) .when_some( metadata.and_then(|metadata| metadata.placeholder), @@ -4789,6 +4826,9 @@ fn render_toggle_button + From + Copy>( Switch::new("toggle_button", toggle_state) .tab_index(0_isize) + .when_some(a11y_label_for_json_path(field.json_path), |this, label| { + this.aria_label(label) + }) .disabled(disabled) .on_click({ move |state, window, cx| { @@ -4822,6 +4862,9 @@ fn render_editable_number_field( NumberField::new(id, value, window, cx) .mode(NumberFieldMode::Edit, cx) .tab_index(0_isize) + .when_some(a11y_label_for_json_path(field.json_path), |this, label| { + this.aria_label(label) + }) .on_change({ move |value, window, cx| { let value = *value; @@ -4878,6 +4921,9 @@ where .log_err(); // todo(settings_ui) don't log err } }) + .when_some(a11y_label_for_json_path(field.json_path), |this, label| { + this.aria_label(label) + }) .disabled(disabled) .tab_index(0) .title_case(should_do_titlecase) @@ -4886,6 +4932,7 @@ where fn render_picker_trigger_button(id: SharedString, label: SharedString) -> Button { Button::new(id, label) + .aria_role(Role::ComboBox) .tab_index(0_isize) .style(ButtonStyle::Outlined) .size(ButtonSize::Medium) @@ -4896,6 +4943,24 @@ fn render_picker_trigger_button(id: SharedString, label: SharedString) -> Button ) } +/// Wires the Expand/Collapse accessibility actions on a picker trigger button to +/// the popover handle, so assistive technology can open and close the picker +/// (used by UIA on Windows and AX on macOS; Linux/AT-SPI uses the click action). +fn wire_picker_trigger_a11y( + button: Button, + handle: ui::PopoverMenuHandle, +) -> Button { + let show_handle = handle.clone(); + let hide_handle = handle; + button + .on_a11y_action(gpui::accesskit::Action::Expand, move |_, window, cx| { + show_handle.show(window, cx); + }) + .on_a11y_action(gpui::accesskit::Action::Collapse, move |_, _window, cx| { + hide_handle.hide(cx); + }) +} + fn render_font_picker( field: SettingField, file: SettingsUiFile, @@ -4909,10 +4974,17 @@ fn render_font_picker( .cloned() .map_or_else(|| SharedString::default(), |value| value.into_gpui()); + let handle = ui::PopoverMenuHandle::default(); PopoverMenu::new("font-picker") - .trigger(render_picker_trigger_button( - "font_family_picker_trigger".into(), - current_value.clone(), + .trigger(wire_picker_trigger_a11y( + render_picker_trigger_button( + "font_family_picker_trigger".into(), + current_value.clone(), + ) + .when_some(a11y_label_for_json_path(field.json_path), |this, label| { + this.aria_label(format!("{}: {}", label, current_value.clone())) + }), + handle.clone(), )) .menu(move |window, cx| { let file = file.clone(); @@ -4943,7 +5015,7 @@ fn render_font_picker( x: px(0.0), y: px(2.0), }) - .with_handle(ui::PopoverMenuHandle::default()) + .with_handle(handle) .into_any_element() } @@ -4960,10 +5032,14 @@ fn render_theme_picker( .map(|theme_name| theme_name.0.into()) .unwrap_or_else(|| cx.theme().name.clone()); + let handle = ui::PopoverMenuHandle::default(); PopoverMenu::new("theme-picker") - .trigger(render_picker_trigger_button( - "theme_picker_trigger".into(), - current_value.clone(), + .trigger(wire_picker_trigger_a11y( + render_picker_trigger_button("theme_picker_trigger".into(), current_value.clone()) + .when_some(a11y_label_for_json_path(field.json_path), |this, label| { + this.aria_label(format!("{}: {}", label, current_value.clone())) + }), + handle.clone(), )) .menu(move |window, cx| { Some(cx.new(|cx| { @@ -4997,7 +5073,7 @@ fn render_theme_picker( x: px(0.0), y: px(2.0), }) - .with_handle(ui::PopoverMenuHandle::default()) + .with_handle(handle) .into_any_element() } @@ -5014,10 +5090,14 @@ fn render_icon_theme_picker( .map(|theme_name| theme_name.0.into()) .unwrap_or_else(|| cx.theme().name.clone()); + let handle = ui::PopoverMenuHandle::default(); PopoverMenu::new("icon-theme-picker") - .trigger(render_picker_trigger_button( - "icon_theme_picker_trigger".into(), - current_value.clone(), + .trigger(wire_picker_trigger_a11y( + render_picker_trigger_button("icon_theme_picker_trigger".into(), current_value.clone()) + .when_some(a11y_label_for_json_path(field.json_path), |this, label| { + this.aria_label(format!("{}: {}", label, current_value.clone())) + }), + handle.clone(), )) .menu(move |window, cx| { Some(cx.new(|cx| { @@ -5051,7 +5131,7 @@ fn render_icon_theme_picker( x: px(0.0), y: px(2.0), }) - .with_handle(ui::PopoverMenuHandle::default()) + .with_handle(handle) .into_any_element() } diff --git a/crates/title_bar/src/application_menu.rs b/crates/title_bar/src/application_menu.rs index b5cb07f757..5429686ebd 100644 --- a/crates/title_bar/src/application_menu.rs +++ b/crates/title_bar/src/application_menu.rs @@ -167,7 +167,8 @@ impl ApplicationMenu { ui::IconName::Menu, ) .style(ButtonStyle::Subtle) - .icon_size(IconSize::Small), + .icon_size(IconSize::Small) + .aria_label("Application menu"), Tooltip::text("Open Application Menu"), ) .with_handle(handle), diff --git a/crates/title_bar/src/title_bar.rs b/crates/title_bar/src/title_bar.rs index f16f07bb89..ed4848c59a 100644 --- a/crates/title_bar/src/title_bar.rs +++ b/crates/title_bar/src/title_bar.rs @@ -1251,7 +1251,7 @@ impl TitleBar { } }); - ButtonLike::new("user-menu").child( + ButtonLike::new("user-menu").aria_label("User menu").child( h_flex() .when_some(business_organization, |this, organization| { this.gap_2() @@ -1261,6 +1261,7 @@ impl TitleBar { ) } else { ButtonLike::new("user-menu") + .aria_label("User menu") .child(Icon::new(IconName::ChevronDown).size(IconSize::Small)) }; diff --git a/crates/ui/src/components/button/button.rs b/crates/ui/src/components/button/button.rs index bf19b09496..094c0988ba 100644 --- a/crates/ui/src/components/button/button.rs +++ b/crates/ui/src/components/button/button.rs @@ -123,6 +123,38 @@ impl Button { self } + /// Sets the label announced by assistive technology. + /// Defaults to the button's visible label. + pub fn aria_label(mut self, label: impl Into) -> Self { + self.base = self.base.aria_label(label); + self + } + + /// Overrides the role reported to assistive technology. + /// Defaults to [`gpui::Role::Button`]. + pub fn aria_role(mut self, role: gpui::Role) -> Self { + self.base = self.base.aria_role(role); + self + } + + /// Sets the expanded state reported to assistive technology, for buttons + /// that control a popup (e.g. dropdown or disclosure triggers). + pub fn aria_expanded(mut self, expanded: bool) -> Self { + self.base = self.base.aria_expanded(expanded); + self + } + + /// Registers a handler for an accessibility action (e.g. + /// [`gpui::accesskit::Action::Expand`]) dispatched by assistive technology. + pub fn on_a11y_action( + mut self, + action: gpui::accesskit::Action, + listener: impl FnMut(Option<&gpui::accesskit::ActionData>, &mut Window, &mut App) + 'static, + ) -> Self { + self.base = self.base.on_a11y_action(action, listener); + self + } + /// Defines the size of the button's label. pub fn label_size(mut self, label_size: impl Into>) -> Self { self.label_size = label_size.into(); @@ -369,7 +401,7 @@ impl ButtonCommon for Button { impl RenderOnce for Button { #[allow(refining_impl_trait)] - fn render(self, _window: &mut Window, cx: &mut App) -> ButtonLike { + fn render(mut self, _window: &mut Window, cx: &mut App) -> ButtonLike { let is_disabled = self.base.disabled; let is_selected = self.base.selected; @@ -378,6 +410,10 @@ impl RenderOnce for Button { .filter(|_| is_selected) .unwrap_or(self.label); + if self.base.aria_label.is_none() { + self.base.aria_label = Some(label.clone()); + } + let label_color = if is_disabled { Color::Disabled } else if is_selected { diff --git a/crates/ui/src/components/button/button_like.rs b/crates/ui/src/components/button/button_like.rs index 05999daef4..3436b8dca5 100644 --- a/crates/ui/src/components/button/button_like.rs +++ b/crates/ui/src/components/button/button_like.rs @@ -1,7 +1,7 @@ use documented::Documented; use gpui::{ AnyElement, AnyView, ClickEvent, CursorStyle, DefiniteLength, FocusHandle, Hsla, MouseButton, - MouseClickEvent, MouseDownEvent, MouseUpEvent, Rems, StyleRefinement, relative, + MouseClickEvent, MouseDownEvent, MouseUpEvent, Rems, Role, StyleRefinement, Toggled, relative, transparent_black, }; use smallvec::SmallVec; @@ -492,11 +492,19 @@ pub struct ButtonLike { tab_index: Option, size: ButtonSize, rounding: Option, + pub(super) aria_label: Option, + aria_role: Option, + aria_expanded: Option, + toggled: Option, tooltip: Option AnyView>>, hoverable_tooltip: Option AnyView>>, cursor_style: CursorStyle, on_click: Option>, on_right_click: Option>, + a11y_actions: Vec<( + gpui::accesskit::Action, + Box, &mut Window, &mut App) + 'static>, + )>, children: SmallVec<[AnyElement; 2]>, focus_handle: Option, } @@ -514,12 +522,17 @@ impl ButtonLike { height: None, size: ButtonSize::Default, rounding: Some(ButtonLikeRounding::ALL), + aria_label: None, + aria_role: None, + aria_expanded: None, + toggled: None, tooltip: None, hoverable_tooltip: None, children: SmallVec::new(), cursor_style: CursorStyle::PointingHand, on_click: None, on_right_click: None, + a11y_actions: Vec::new(), layer: None, tab_index: None, focus_handle: None, @@ -568,6 +581,38 @@ impl ButtonLike { self.hoverable_tooltip = Some(Box::new(tooltip)); self } + + /// Sets the label announced by assistive technology for this button. + pub fn aria_label(mut self, label: impl Into) -> Self { + self.aria_label = Some(label.into()); + self + } + + /// Overrides the role reported to assistive technology. + /// Defaults to [`Role::Button`]. + pub fn aria_role(mut self, role: Role) -> Self { + self.aria_role = Some(role); + self + } + + /// Sets the expanded state reported to assistive technology, for buttons + /// that control a popup (e.g. dropdown or disclosure triggers). + pub fn aria_expanded(mut self, expanded: bool) -> Self { + self.aria_expanded = Some(expanded); + self + } + + /// Registers a handler for an accessibility action (e.g. + /// [`accesskit::Action::Expand`]) dispatched by assistive technology to + /// this button. Also advertises the action to assistive technology. + pub fn on_a11y_action( + mut self, + action: gpui::accesskit::Action, + listener: impl FnMut(Option<&gpui::accesskit::ActionData>, &mut Window, &mut App) + 'static, + ) -> Self { + self.a11y_actions.push((action, Box::new(listener))); + self + } } impl Disableable for ButtonLike { @@ -580,6 +625,7 @@ impl Disableable for ButtonLike { impl Toggleable for ButtonLike { fn toggle_state(mut self, selected: bool) -> Self { self.selected = selected; + self.toggled = Some(selected); self } } @@ -679,6 +725,18 @@ impl RenderOnce for ButtonLike { self.base .h_flex() .id(self.id.clone()) + .role(self.aria_role.unwrap_or(Role::Button)) + .when_some(self.aria_label, |this, label| this.aria_label(label)) + .when_some(self.aria_expanded, |this, expanded| { + this.aria_expanded(expanded) + }) + .when_some(self.toggled, |this, toggled| { + this.aria_toggled(if toggled { + Toggled::True + } else { + Toggled::False + }) + }) .when_some(self.tab_index, |this, tab_index| this.tab_index(tab_index)) .when_some(self.focus_handle, |this, focus_handle| { this.track_focus(&focus_handle) @@ -779,6 +837,12 @@ impl RenderOnce for ButtonLike { .when_some(self.hoverable_tooltip, |this, tooltip| { this.hoverable_tooltip(move |window, cx| tooltip(window, cx)) }) + .map(|mut this| { + for (action, listener) in self.a11y_actions { + this = this.on_a11y_action(action, listener); + } + this + }) .children(self.children) } } diff --git a/crates/ui/src/components/button/icon_button.rs b/crates/ui/src/components/button/icon_button.rs index 246f46b911..a05a538b4a 100644 --- a/crates/ui/src/components/button/icon_button.rs +++ b/crates/ui/src/components/button/icon_button.rs @@ -53,6 +53,33 @@ impl IconButton { self } + /// Sets the label announced by assistive technology. + /// + /// Icon buttons have no visible text, so they should always set this for + /// screen reader users. Often this matches the tooltip text. + pub fn aria_label(mut self, label: impl Into) -> Self { + self.base = self.base.aria_label(label); + self + } + + /// Sets the expanded state reported to assistive technology, for buttons + /// that control a popup (e.g. dropdown or disclosure triggers). + pub fn aria_expanded(mut self, expanded: bool) -> Self { + self.base = self.base.aria_expanded(expanded); + self + } + + /// Registers a handler for an accessibility action (e.g. + /// [`gpui::accesskit::Action::Expand`]) dispatched by assistive technology. + pub fn on_a11y_action( + mut self, + action: gpui::accesskit::Action, + listener: impl FnMut(Option<&gpui::accesskit::ActionData>, &mut Window, &mut App) + 'static, + ) -> Self { + self.base = self.base.on_a11y_action(action, listener); + self + } + pub fn icon_size(mut self, icon_size: IconSize) -> Self { self.icon_size = icon_size; self diff --git a/crates/ui/src/components/context_menu.rs b/crates/ui/src/components/context_menu.rs index ef21abaf3e..4b98199239 100644 --- a/crates/ui/src/components/context_menu.rs +++ b/crates/ui/src/components/context_menu.rs @@ -4,8 +4,8 @@ use crate::{ }; use gpui::{ Action, Anchor, AnyElement, App, Bounds, DismissEvent, Entity, EventEmitter, FocusHandle, - Focusable, MouseButton, MouseDownEvent, MouseMoveEvent, MouseUpEvent, Pixels, Point, Size, - Subscription, TaskExt, anchored, canvas, prelude::*, px, + Focusable, MouseButton, MouseDownEvent, MouseMoveEvent, MouseUpEvent, Pixels, Point, Role, + Size, Subscription, TaskExt, anchored, canvas, prelude::*, px, }; use menu::{SelectChild, SelectFirst, SelectLast, SelectNext, SelectParent, SelectPrevious}; use std::{ @@ -1391,6 +1391,11 @@ impl ContextMenu { window: &mut Window, cx: &mut Context, ) -> impl IntoElement + use<> { + // The menu keeps real focus on its container, so for assistive + // technology to track the selected item we report it as the active + // descendant. GPUI only honors this while the menu actually holds + // focus, so we mark the selected item unconditionally here. + let is_active_descendant = |selectable: bool| selectable && Some(ix) == self.selected_index; match item { ContextMenuItem::Separator => ListSeparator.into_any_element(), ContextMenuItem::Header(header) => ListSubHeader::new(header.clone()) @@ -1420,9 +1425,9 @@ impl ContextMenu { .disabled(true) .child(Label::new(label.clone())) .into_any_element(), - ContextMenuItem::Entry(entry) => { - self.render_menu_entry(ix, entry, cx).into_any_element() - } + ContextMenuItem::Entry(entry) => self + .render_menu_entry(ix, entry, is_active_descendant(true), cx) + .into_any_element(), ContextMenuItem::CustomEntry { entry_render, handler, @@ -1469,6 +1474,10 @@ impl ContextMenu { .child( ListItem::new(ix) .inset(true) + .when(selectable, |item| item.aria_role(Role::MenuItem)) + .when(is_active_descendant(selectable), |item| { + item.aria_active_descendant() + }) .toggle_state(Some(ix) == self.selected_index) .selectable(selectable) .when(selectable, |item| { @@ -1500,7 +1509,14 @@ impl ContextMenu { icon_color, .. } => self - .render_submenu_item_trigger(ix, label.clone(), *icon, *icon_color, cx) + .render_submenu_item_trigger( + ix, + label.clone(), + *icon, + *icon_color, + is_active_descendant(true), + cx, + ) .into_any_element(), } } @@ -1511,6 +1527,7 @@ impl ContextMenu { label: SharedString, icon: Option, icon_color: Option, + is_active_descendant: bool, cx: &mut Context, ) -> impl IntoElement { let toggle_state = Some(ix) == self.selected_index @@ -1544,6 +1561,9 @@ impl ContextMenu { .child( ListItem::new(ix) .inset(true) + .aria_role(Role::MenuItem) + .when(is_active_descendant, |item| item.aria_active_descendant()) + .aria_label(label.clone()) .toggle_state(toggle_state) .child( canvas( @@ -1722,6 +1742,7 @@ impl ContextMenu { &self, ix: usize, entry: &ContextMenuEntry, + is_active_descendant: bool, cx: &mut Context, ) -> impl IntoElement { let ContextMenuEntry { @@ -1862,6 +1883,13 @@ impl ContextMenu { .group_name("label_container") .inset(true) .disabled(*disabled) + .aria_role(if toggle.is_some() { + Role::MenuItemCheckBox + } else { + Role::MenuItem + }) + .when(is_active_descendant, |item| item.aria_active_descendant()) + .aria_label(label.clone()) .toggle_state(Some(ix) == self.selected_index) .when(self.main_menu.is_none() && !*disabled, |item| { item.on_hover(cx.listener(move |this, hovered, window, cx| { @@ -2155,6 +2183,7 @@ impl Render for ContextMenu { .child( v_flex() .id("context-menu") + .role(Role::Menu) .max_h(vh(0.75, window)) .flex_shrink_0() .child(menu_bounds_measure) diff --git a/crates/ui/src/components/disclosure.rs b/crates/ui/src/components/disclosure.rs index 387041bbd1..8cd1d5c0b9 100644 --- a/crates/ui/src/components/disclosure.rs +++ b/crates/ui/src/components/disclosure.rs @@ -100,6 +100,8 @@ impl RenderOnce for Disclosure { ) .icon_color(Color::Muted) .icon_size(IconSize::Small) + .aria_label(if self.is_open { "Collapse" } else { "Expand" }) + .aria_expanded(self.is_open) .disabled(self.disabled) .toggle_state(self.selected) .when_some(self.visible_on_hover.clone(), |this, group_name| { diff --git a/crates/ui/src/components/dropdown_menu.rs b/crates/ui/src/components/dropdown_menu.rs index ebce559d7d..3be19d333f 100644 --- a/crates/ui/src/components/dropdown_menu.rs +++ b/crates/ui/src/components/dropdown_menu.rs @@ -1,4 +1,4 @@ -use gpui::{Anchor, AnyView, Entity, Pixels, Point}; +use gpui::{Anchor, AnyView, Entity, Pixels, Point, Role}; use crate::{ButtonLike, ContextMenu, PopoverMenu, prelude::*}; @@ -34,6 +34,7 @@ pub struct DropdownMenu { offset: Option>, tab_index: Option, chevron: bool, + aria_label: Option, } impl DropdownMenu { @@ -57,6 +58,7 @@ impl DropdownMenu { offset: None, tab_index: None, chevron: true, + aria_label: None, } } @@ -80,6 +82,7 @@ impl DropdownMenu { offset: None, tab_index: None, chevron: true, + aria_label: None, } } @@ -137,6 +140,13 @@ impl DropdownMenu { self.chevron = false; self } + + /// Sets the label announced by assistive technology. + /// Defaults to the trigger's visible label (typically the current value). + pub fn aria_label(mut self, label: impl Into) -> Self { + self.aria_label = Some(label.into()); + self + } } impl Disableable for DropdownMenu { @@ -157,11 +167,42 @@ impl RenderOnce for DropdownMenu { let full_width = self.full_width; let trigger_size = self.trigger_size; + // Ensure a handle exists so assistive technology can open/close the menu + // via the Expand/Collapse accessibility actions (used by UIA on Windows + // and AX on macOS; on Linux/AT-SPI the click action is used instead). + let handle = self.handle.unwrap_or_default(); + let expanded = handle.is_deployed(); + + let a11y_actions = |button: Button| { + let show_handle = handle.clone(); + let hide_handle = handle.clone(); + button + .on_a11y_action(gpui::accesskit::Action::Expand, move |_, window, cx| { + show_handle.show(window, cx); + }) + .on_a11y_action(gpui::accesskit::Action::Collapse, move |_, _window, cx| { + hide_handle.hide(cx); + }) + }; + let a11y_actions_element = |button: ButtonLike| { + let show_handle = handle.clone(); + let hide_handle = handle.clone(); + button + .on_a11y_action(gpui::accesskit::Action::Expand, move |_, window, cx| { + show_handle.show(window, cx); + }) + .on_a11y_action(gpui::accesskit::Action::Collapse, move |_, _window, cx| { + hide_handle.hide(cx); + }) + }; let (text_button, element_button) = match self.label { LabelKind::Text(text) => ( Some( - Button::new(self.id.clone(), text) + a11y_actions(Button::new(self.id.clone(), text)) + .aria_role(Role::ComboBox) + .when_some(self.aria_label, |this, label| this.aria_label(label)) + .aria_expanded(expanded) .style(button_style) .when_some(self.trigger_icon.filter(|_| self.chevron), |this, icon| { this.end_icon( @@ -178,7 +219,10 @@ impl RenderOnce for DropdownMenu { LabelKind::Element(element) => ( None, Some( - ButtonLike::new(self.id.clone()) + a11y_actions_element(ButtonLike::new(self.id.clone())) + .aria_role(Role::ComboBox) + .when_some(self.aria_label, |this, label| this.aria_label(label)) + .aria_expanded(expanded) .child(element) .style(button_style) .when(self.chevron, |this| { @@ -198,6 +242,7 @@ impl RenderOnce for DropdownMenu { let mut popover = PopoverMenu::new((self.id.clone(), "popover")) .full_width(self.full_width) + .with_handle(handle) .menu(move |_window, _cx| Some(self.menu.clone())); popover = match (text_button, element_button, self.trigger_tooltip) { @@ -218,7 +263,6 @@ impl RenderOnce for DropdownMenu { None => Anchor::BottomRight, }) .when_some(self.offset, |this, offset| this.offset(offset)) - .when_some(self.handle, |this, handle| this.with_handle(handle)) } } diff --git a/crates/ui/src/components/list/list_item.rs b/crates/ui/src/components/list/list_item.rs index 8f5ccadcaf..c03de61e1e 100644 --- a/crates/ui/src/components/list/list_item.rs +++ b/crates/ui/src/components/list/list_item.rs @@ -1,7 +1,7 @@ use std::sync::Arc; use component::{Component, ComponentScope, example_group_with_title, single_example}; -use gpui::{AnyElement, AnyView, ClickEvent, MouseButton, MouseDownEvent, Pixels, px}; +use gpui::{AnyElement, AnyView, ClickEvent, MouseButton, MouseDownEvent, Pixels, Role, px}; use smallvec::SmallVec; use crate::{Disclosure, prelude::*}; @@ -53,6 +53,9 @@ pub struct ListItem { focused: Option, docked_right: bool, height: Option, + aria_role: Option, + aria_label: Option, + aria_active_descendant: bool, } impl ListItem { @@ -84,9 +87,40 @@ impl ListItem { focused: None, docked_right: false, height: None, + aria_role: None, + aria_label: None, + aria_active_descendant: false, } } + /// Sets the accessible role reported to assistive technology (e.g. + /// [`Role::MenuItem`] when this item is part of a menu). When unset, the + /// item is not reported as a distinct node, matching list defaults. + pub fn aria_role(mut self, role: Role) -> Self { + self.aria_role = Some(role); + self + } + + /// Sets the label announced by assistive technology. List items render + /// arbitrary children, so this should be set when a meaningful textual + /// label exists. + pub fn aria_label(mut self, label: impl Into) -> Self { + self.aria_label = Some(label.into()); + self + } + + /// Reports this item as the accessibility focus, overriding the element + /// that holds real keyboard focus. Requires [`Self::aria_role`] to be set. + /// + /// Use this on the selected item of a composite widget (e.g. a menu) that + /// keeps keyboard focus on its container. It is honored only while the + /// container actually holds focus, so it is safe to set unconditionally on + /// the selected item. See [`StatefulInteractiveElement::aria_active_descendant`]. + pub fn aria_active_descendant(mut self) -> Self { + self.aria_active_descendant = true; + self + } + pub fn group_name(mut self, group_name: impl Into) -> Self { self.group_name = Some(group_name.into()); self @@ -270,6 +304,25 @@ impl RenderOnce for ListItem { .child( h_flex() .id("inner_list_item") + // The accessible role/label live here, alongside the click + // handler, so assistive technology reports one actionable + // node (e.g. a menu item) rather than an inert container. + .when_some(self.aria_role, |this, role| this.role(role)) + .when( + self.aria_role.is_some() && self.aria_active_descendant, + |this| this.aria_active_descendant(), + ) + .when_some(self.aria_label, |this, label| this.aria_label(label)) + .when(self.aria_role.is_some(), |this| { + this.aria_selected(self.selected) + .when_some(self.toggle, |this, toggled| { + this.aria_toggled(if toggled { + gpui::Toggled::True + } else { + gpui::Toggled::False + }) + }) + }) .group("list_item") .w_full() .relative() diff --git a/crates/ui/src/components/toggle.rs b/crates/ui/src/components/toggle.rs index 85568dbb01..edf90aca43 100644 --- a/crates/ui/src/components/toggle.rs +++ b/crates/ui/src/components/toggle.rs @@ -1,6 +1,6 @@ use gpui::{ AnyElement, AnyView, ClickEvent, ElementId, Hsla, IntoElement, KeybindingKeystroke, Keystroke, - Styled, Window, div, hsla, prelude::*, + Role, Styled, Toggled, Window, div, hsla, prelude::*, }; use std::{rc::Rc, sync::Arc}; @@ -347,6 +347,7 @@ pub struct Switch { key_binding: Option, color: SwitchColor, tab_index: Option, + aria_label: Option, } impl Switch { @@ -364,6 +365,7 @@ impl Switch { key_binding: None, color: SwitchColor::default(), tab_index: None, + aria_label: None, } } @@ -422,6 +424,13 @@ impl Switch { self.tab_index = Some(tab_index.into()); self } + + /// Sets the label announced by assistive technology. + /// Defaults to the switch's visible label, if any. + pub fn aria_label(mut self, label: impl Into) -> Self { + self.aria_label = Some(label.into()); + self + } } impl RenderOnce for Switch { @@ -447,9 +456,17 @@ impl RenderOnce for Switch { let group_id = format!("switch_group_{:?}", self.id); let label = self.label; + let aria_label = self.aria_label.or_else(|| label.clone()); let switch = div() .id((self.id.clone(), "switch")) + .role(Role::Switch) + .when_some(aria_label, |this, label| this.aria_label(label)) + .aria_toggled(match self.toggle_state { + ToggleState::Selected => Toggled::True, + ToggleState::Indeterminate => Toggled::Mixed, + ToggleState::Unselected => Toggled::False, + }) .p(px(1.0)) .border_2() .border_color(cx.theme().colors().border_transparent) diff --git a/crates/ui/src/components/tree_view_item.rs b/crates/ui/src/components/tree_view_item.rs index 31cdfd33bd..6d4df1d619 100644 --- a/crates/ui/src/components/tree_view_item.rs +++ b/crates/ui/src/components/tree_view_item.rs @@ -1,6 +1,6 @@ use std::sync::Arc; -use gpui::{AnyElement, AnyView, ClickEvent, MouseButton, MouseDownEvent}; +use gpui::{AnyElement, AnyView, ClickEvent, MouseButton, MouseDownEvent, Role}; use crate::{Disclosure, prelude::*}; @@ -151,6 +151,11 @@ impl RenderOnce for TreeViewItem { .bg(cx.theme().colors().border.opacity(0.5)), ); + let aria_label = self.label.clone(); + let root_item = self.root_item; + let expanded = self.expanded; + let selected = self.selected; + h_flex() .id(self.id) .when_some(self.group_name, |this, group| this.group(group)) @@ -158,6 +163,15 @@ impl RenderOnce for TreeViewItem { .child( h_flex() .id("inner_tree_view_item") + // The accessible role and state live on this element + // because it also carries the click handler and focus, so + // assistive technology reports a single actionable tree + // item rather than an inert container. + .role(Role::TreeItem) + .aria_label(aria_label) + .aria_selected(selected) + .aria_level(if root_item { 1 } else { 2 }) + .when(root_item, |this| this.aria_expanded(expanded)) .cursor_pointer() .size_full() .h(item_size)