From fa8d535fe716ab91ca531dca51c283850615bc28 Mon Sep 17 00:00:00 2001 From: Wybe Westra Date: Tue, 2 Jul 2024 09:18:30 +0200 Subject: [PATCH] Disabled widgets are now also disabled in the accesskit output (#4750) Marking widgets as disabled was not reflected in the accesskit output, now the disabled status should match. --------- Co-authored-by: Wybe Westra --- .../egui/src/containers/collapsing_header.rs | 5 +- crates/egui/src/containers/combo_box.rs | 7 +- crates/egui/src/data/output.rs | 21 ++- crates/egui/src/menu.rs | 6 +- crates/egui/src/response.rs | 6 + crates/egui/src/widgets/button.rs | 2 +- crates/egui/src/widgets/checkbox.rs | 2 + crates/egui/src/widgets/drag_value.rs | 2 +- crates/egui/src/widgets/hyperlink.rs | 3 +- crates/egui/src/widgets/label.rs | 3 +- crates/egui/src/widgets/progress_bar.rs | 2 +- crates/egui/src/widgets/radio_button.rs | 1 + crates/egui/src/widgets/selected_label.rs | 7 +- crates/egui/src/widgets/slider.rs | 2 +- crates/egui/src/widgets/text_edit/builder.rs | 3 + crates/egui/tests/accesskit.rs | 139 ++++++++++++------ .../egui_demo_lib/src/demo/toggle_switch.rs | 8 +- crates/egui_plot/src/legend.rs | 10 +- 18 files changed, 161 insertions(+), 68 deletions(-) diff --git a/crates/egui/src/containers/collapsing_header.rs b/crates/egui/src/containers/collapsing_header.rs index 388293d19..e0c298980 100644 --- a/crates/egui/src/containers/collapsing_header.rs +++ b/crates/egui/src/containers/collapsing_header.rs @@ -540,8 +540,9 @@ impl CollapsingHeader { header_response.mark_changed(); } - header_response - .widget_info(|| WidgetInfo::labeled(WidgetType::CollapsingHeader, galley.text())); + header_response.widget_info(|| { + WidgetInfo::labeled(WidgetType::CollapsingHeader, ui.is_enabled(), galley.text()) + }); let openness = state.openness(ui.ctx()); diff --git a/crates/egui/src/containers/combo_box.rs b/crates/egui/src/containers/combo_box.rs index 000e907c6..cb29f2776 100644 --- a/crates/egui/src/containers/combo_box.rs +++ b/crates/egui/src/containers/combo_box.rs @@ -213,12 +213,13 @@ impl ComboBox { (width, height), ); if let Some(label) = label { - ir.response - .widget_info(|| WidgetInfo::labeled(WidgetType::ComboBox, label.text())); + ir.response.widget_info(|| { + WidgetInfo::labeled(WidgetType::ComboBox, ui.is_enabled(), label.text()) + }); ir.response |= ui.label(label); } else { ir.response - .widget_info(|| WidgetInfo::labeled(WidgetType::ComboBox, "")); + .widget_info(|| WidgetInfo::labeled(WidgetType::ComboBox, ui.is_enabled(), "")); } ir }) diff --git a/crates/egui/src/data/output.rs b/crates/egui/src/data/output.rs index d3dcc8bfe..58df5da23 100644 --- a/crates/egui/src/data/output.rs +++ b/crates/egui/src/data/output.rs @@ -547,8 +547,9 @@ impl WidgetInfo { } #[allow(clippy::needless_pass_by_value)] - pub fn labeled(typ: WidgetType, label: impl ToString) -> Self { + pub fn labeled(typ: WidgetType, enabled: bool, label: impl ToString) -> Self { Self { + enabled, label: Some(label.to_string()), ..Self::new(typ) } @@ -556,25 +557,28 @@ impl WidgetInfo { /// checkboxes, radio-buttons etc #[allow(clippy::needless_pass_by_value)] - pub fn selected(typ: WidgetType, selected: bool, label: impl ToString) -> Self { + pub fn selected(typ: WidgetType, enabled: bool, selected: bool, label: impl ToString) -> Self { Self { + enabled, label: Some(label.to_string()), selected: Some(selected), ..Self::new(typ) } } - pub fn drag_value(value: f64) -> Self { + pub fn drag_value(enabled: bool, value: f64) -> Self { Self { + enabled, value: Some(value), ..Self::new(WidgetType::DragValue) } } #[allow(clippy::needless_pass_by_value)] - pub fn slider(value: f64, label: impl ToString) -> Self { + pub fn slider(enabled: bool, value: f64, label: impl ToString) -> Self { let label = label.to_string(); Self { + enabled, label: if label.is_empty() { None } else { Some(label) }, value: Some(value), ..Self::new(WidgetType::Slider) @@ -582,7 +586,11 @@ impl WidgetInfo { } #[allow(clippy::needless_pass_by_value)] - pub fn text_edit(prev_text_value: impl ToString, text_value: impl ToString) -> Self { + pub fn text_edit( + enabled: bool, + prev_text_value: impl ToString, + text_value: impl ToString, + ) -> Self { let text_value = text_value.to_string(); let prev_text_value = prev_text_value.to_string(); let prev_text_value = if text_value == prev_text_value { @@ -591,6 +599,7 @@ impl WidgetInfo { Some(prev_text_value) }; Self { + enabled, current_text_value: Some(text_value), prev_text_value, ..Self::new(WidgetType::TextEdit) @@ -599,10 +608,12 @@ impl WidgetInfo { #[allow(clippy::needless_pass_by_value)] pub fn text_selection_changed( + enabled: bool, text_selection: std::ops::RangeInclusive, current_text_value: impl ToString, ) -> Self { Self { + enabled, text_selection: Some(text_selection), current_text_value: Some(current_text_value.to_string()), ..Self::new(WidgetType::TextEdit) diff --git a/crates/egui/src/menu.rs b/crates/egui/src/menu.rs index 4325f07d8..4980f6f98 100644 --- a/crates/egui/src/menu.rs +++ b/crates/egui/src/menu.rs @@ -524,7 +524,11 @@ impl SubMenuButton { let (rect, response) = ui.allocate_at_least(desired_size, sense); response.widget_info(|| { - crate::WidgetInfo::labeled(crate::WidgetType::Button, text_galley.text()) + crate::WidgetInfo::labeled( + crate::WidgetType::Button, + ui.is_enabled(), + text_galley.text(), + ) }); if ui.is_rect_visible(rect) { diff --git a/crates/egui/src/response.rs b/crates/egui/src/response.rs index bb3d6456b..aee287176 100644 --- a/crates/egui/src/response.rs +++ b/crates/egui/src/response.rs @@ -880,6 +880,9 @@ impl Response { #[cfg(feature = "accesskit")] pub(crate) fn fill_accesskit_node_common(&self, builder: &mut accesskit::NodeBuilder) { + if !self.enabled { + builder.set_disabled(); + } builder.set_bounds(accesskit::Rect { x0: self.rect.min.x.into(), y0: self.rect.min.y.into(), @@ -921,6 +924,9 @@ impl Response { WidgetType::ProgressIndicator => Role::ProgressIndicator, WidgetType::Other => Role::Unknown, }); + if !info.enabled { + builder.set_disabled(); + } if let Some(label) = info.label { builder.set_name(label); } diff --git a/crates/egui/src/widgets/button.rs b/crates/egui/src/widgets/button.rs index 24b025211..750c4b1ab 100644 --- a/crates/egui/src/widgets/button.rs +++ b/crates/egui/src/widgets/button.rs @@ -265,7 +265,7 @@ impl Widget for Button<'_> { let (rect, mut response) = ui.allocate_at_least(desired_size, sense); response.widget_info(|| { if let Some(galley) = &galley { - WidgetInfo::labeled(WidgetType::Button, galley.text()) + WidgetInfo::labeled(WidgetType::Button, ui.is_enabled(), galley.text()) } else { WidgetInfo::new(WidgetType::Button) } diff --git a/crates/egui/src/widgets/checkbox.rs b/crates/egui/src/widgets/checkbox.rs index e416a6e06..e6bef72fc 100644 --- a/crates/egui/src/widgets/checkbox.rs +++ b/crates/egui/src/widgets/checkbox.rs @@ -82,11 +82,13 @@ impl<'a> Widget for Checkbox<'a> { if indeterminate { WidgetInfo::labeled( WidgetType::Checkbox, + ui.is_enabled(), galley.as_ref().map_or("", |x| x.text()), ) } else { WidgetInfo::selected( WidgetType::Checkbox, + ui.is_enabled(), *checked, galley.as_ref().map_or("", |x| x.text()), ) diff --git a/crates/egui/src/widgets/drag_value.rs b/crates/egui/src/widgets/drag_value.rs index 614bd77e4..26bbc38cb 100644 --- a/crates/egui/src/widgets/drag_value.rs +++ b/crates/egui/src/widgets/drag_value.rs @@ -621,7 +621,7 @@ impl<'a> Widget for DragValue<'a> { response.changed = get(&mut get_set_value) != old_value; - response.widget_info(|| WidgetInfo::drag_value(value)); + response.widget_info(|| WidgetInfo::drag_value(ui.is_enabled(), value)); #[cfg(feature = "accesskit")] ui.ctx().accesskit_node_builder(response.id, |builder| { diff --git a/crates/egui/src/widgets/hyperlink.rs b/crates/egui/src/widgets/hyperlink.rs index ad3b76eed..038cc761f 100644 --- a/crates/egui/src/widgets/hyperlink.rs +++ b/crates/egui/src/widgets/hyperlink.rs @@ -37,7 +37,8 @@ impl Widget for Link { let label = Label::new(text).sense(Sense::click()); let (galley_pos, galley, response) = label.layout_in_ui(ui); - response.widget_info(|| WidgetInfo::labeled(WidgetType::Link, galley.text())); + response + .widget_info(|| WidgetInfo::labeled(WidgetType::Link, ui.is_enabled(), galley.text())); if ui.is_rect_visible(response.rect) { let color = ui.visuals().hyperlink_color; diff --git a/crates/egui/src/widgets/label.rs b/crates/egui/src/widgets/label.rs index c6d44a3e1..0772e7634 100644 --- a/crates/egui/src/widgets/label.rs +++ b/crates/egui/src/widgets/label.rs @@ -229,7 +229,8 @@ impl Widget for Label { let selectable = self.selectable; let (galley_pos, galley, mut response) = self.layout_in_ui(ui); - response.widget_info(|| WidgetInfo::labeled(WidgetType::Label, galley.text())); + response + .widget_info(|| WidgetInfo::labeled(WidgetType::Label, ui.is_enabled(), galley.text())); if ui.is_rect_visible(response.rect) { if galley.elided { diff --git a/crates/egui/src/widgets/progress_bar.rs b/crates/egui/src/widgets/progress_bar.rs index 5f5d21709..9f1328b7e 100644 --- a/crates/egui/src/widgets/progress_bar.rs +++ b/crates/egui/src/widgets/progress_bar.rs @@ -115,7 +115,7 @@ impl Widget for ProgressBar { response.widget_info(|| { let mut info = if let Some(ProgressBarText::Custom(text)) = &text { - WidgetInfo::labeled(WidgetType::ProgressIndicator, text.text()) + WidgetInfo::labeled(WidgetType::ProgressIndicator, ui.is_enabled(), text.text()) } else { WidgetInfo::new(WidgetType::ProgressIndicator) }; diff --git a/crates/egui/src/widgets/radio_button.rs b/crates/egui/src/widgets/radio_button.rs index ccbe785f6..bff620e0a 100644 --- a/crates/egui/src/widgets/radio_button.rs +++ b/crates/egui/src/widgets/radio_button.rs @@ -63,6 +63,7 @@ impl Widget for RadioButton { response.widget_info(|| { WidgetInfo::selected( WidgetType::RadioButton, + ui.is_enabled(), checked, galley.as_ref().map_or("", |x| x.text()), ) diff --git a/crates/egui/src/widgets/selected_label.rs b/crates/egui/src/widgets/selected_label.rs index 105232b40..037882ac4 100644 --- a/crates/egui/src/widgets/selected_label.rs +++ b/crates/egui/src/widgets/selected_label.rs @@ -50,7 +50,12 @@ impl Widget for SelectableLabel { desired_size.y = desired_size.y.at_least(ui.spacing().interact_size.y); let (rect, response) = ui.allocate_at_least(desired_size, Sense::click()); response.widget_info(|| { - WidgetInfo::selected(WidgetType::SelectableLabel, selected, galley.text()) + WidgetInfo::selected( + WidgetType::SelectableLabel, + ui.is_enabled(), + selected, + galley.text(), + ) }); if ui.is_rect_visible(response.rect) { diff --git a/crates/egui/src/widgets/slider.rs b/crates/egui/src/widgets/slider.rs index f84cc2a1d..d3c9dc949 100644 --- a/crates/egui/src/widgets/slider.rs +++ b/crates/egui/src/widgets/slider.rs @@ -853,7 +853,7 @@ impl<'a> Slider<'a> { let value = self.get_value(); response.changed = value != old_value; - response.widget_info(|| WidgetInfo::slider(value, self.text.text())); + response.widget_info(|| WidgetInfo::slider(ui.is_enabled(), value, self.text.text())); #[cfg(feature = "accesskit")] ui.ctx().accesskit_node_builder(response.id, |builder| { diff --git a/crates/egui/src/widgets/text_edit/builder.rs b/crates/egui/src/widgets/text_edit/builder.rs index 164f928bb..aa8728624 100644 --- a/crates/egui/src/widgets/text_edit/builder.rs +++ b/crates/egui/src/widgets/text_edit/builder.rs @@ -744,6 +744,7 @@ impl<'t> TextEdit<'t> { if response.changed { response.widget_info(|| { WidgetInfo::text_edit( + ui.is_enabled(), mask_if_password(password, prev_text.as_str()), mask_if_password(password, text.as_str()), ) @@ -753,6 +754,7 @@ impl<'t> TextEdit<'t> { let char_range = cursor_range.primary.ccursor.index..=cursor_range.secondary.ccursor.index; let info = WidgetInfo::text_selection_changed( + ui.is_enabled(), char_range, mask_if_password(password, text.as_str()), ); @@ -760,6 +762,7 @@ impl<'t> TextEdit<'t> { } else { response.widget_info(|| { WidgetInfo::text_edit( + ui.is_enabled(), mask_if_password(password, prev_text.as_str()), mask_if_password(password, text.as_str()), ) diff --git a/crates/egui/tests/accesskit.rs b/crates/egui/tests/accesskit.rs index 9f6c9aa72..e3d1c7b56 100644 --- a/crates/egui/tests/accesskit.rs +++ b/crates/egui/tests/accesskit.rs @@ -1,8 +1,8 @@ //! Tests the accesskit accessibility output of egui. #![cfg(feature = "accesskit")] -use accesskit::Role; -use egui::{Context, RawInput}; +use accesskit::{Role, TreeUpdate}; +use egui::{CentralPanel, Context, RawInput}; /// Baseline test that asserts there are no spurious nodes in the /// accesskit output when the ui is empty. @@ -11,86 +11,133 @@ use egui::{Context, RawInput}; /// are put there because of the widgets rendered. #[test] fn empty_ui_should_return_tree_with_only_root_window() { - let ctx = Context::default(); - ctx.enable_accesskit(); - - let output = ctx.run(RawInput::default(), |ctx| { - egui::CentralPanel::default().show(ctx, |_| {}); + let output = accesskit_output_single_egui_frame(|ctx| { + CentralPanel::default().show(ctx, |_| {}); }); - let tree_update = output - .platform_output - .accesskit_update - .expect("Missing accesskit update"); - - let tree = tree_update.tree.unwrap(); - assert_eq!( - tree_update.nodes.len(), + output.nodes.len(), 1, "Empty ui should produce only the root window." ); - let (id, root) = &tree_update.nodes[0]; + let (id, root) = &output.nodes[0]; - assert_eq!(*id, tree.root); + assert_eq!(*id, output.tree.unwrap().root); assert_eq!(root.role(), Role::Window); } #[test] -fn button_text() { +fn button_node() { let button_text = "This is a test button!"; - let ctx = Context::default(); - ctx.enable_accesskit(); - - let output = ctx.run(RawInput::default(), |ctx| { - egui::CentralPanel::default().show(ctx, |ui| ui.button(button_text)); + let output = accesskit_output_single_egui_frame(|ctx| { + CentralPanel::default().show(ctx, |ui| ui.button(button_text)); }); - let nodes = output - .platform_output - .accesskit_update - .expect("Missing accesskit update") - .nodes; + assert_eq!( + output.nodes.len(), + 2, + "Expected only the root node and the button." + ); + + let (_, button) = output + .nodes + .iter() + .find(|(_, node)| node.role() == Role::Button) + .expect("Button should exist in the accesskit output"); + + assert_eq!(button.name(), Some(button_text)); + assert!(!button.is_disabled()); +} + +#[test] +fn disabled_button_node() { + let button_text = "This is a test button!"; + + let output = accesskit_output_single_egui_frame(|ctx| { + CentralPanel::default().show(ctx, |ui| { + ui.add_enabled(false, egui::Button::new(button_text)) + }); + }); assert_eq!( - nodes.len(), + output.nodes.len(), 2, "Expected only the root node and the button." ); - nodes + let (_, button) = output + .nodes .iter() - .find(|(_, node)| node.role() == Role::Button && node.name() == Some(button_text)) + .find(|(_, node)| node.role() == Role::Button) .expect("Button should exist in the accesskit output"); + + assert_eq!(button.name(), Some(button_text)); + assert!(button.is_disabled()); } #[test] -fn toggle_button_text() { +fn toggle_button_node() { let button_text = "A toggle button"; - let ctx = Context::default(); - ctx.enable_accesskit(); - let mut selected = false; - let output = ctx.run(RawInput::default(), |ctx| { - egui::CentralPanel::default().show(ctx, |ui| ui.toggle_value(&mut selected, button_text)); + let output = accesskit_output_single_egui_frame(|ctx| { + CentralPanel::default().show(ctx, |ui| ui.toggle_value(&mut selected, button_text)); }); - let nodes = output - .platform_output - .accesskit_update - .expect("Missing accesskit update") - .nodes; - assert_eq!( - nodes.len(), + output.nodes.len(), 2, "Expected only the root node and the button." ); - nodes + let (_, toggle) = output + .nodes .iter() - .find(|(_, node)| node.role() == Role::ToggleButton && node.name() == Some(button_text)) + .find(|(_, node)| node.role() == Role::ToggleButton) .expect("Toggle button should exist in the accesskit output"); + + assert_eq!(toggle.name(), Some(button_text)); + assert!(!toggle.is_disabled()); +} + +#[test] +fn multiple_disabled_widgets() { + let output = accesskit_output_single_egui_frame(|ctx| { + CentralPanel::default().show(ctx, |ui| { + ui.add_enabled_ui(false, |ui| { + let _ = ui.button("Button 1"); + let _ = ui.button("Button 2"); + let _ = ui.button("Button 3"); + }) + }); + }); + + assert_eq!( + output.nodes.len(), + 4, + "Expected the root node and all the child widgets." + ); + + assert_eq!( + output + .nodes + .iter() + .filter(|(_, node)| node.is_disabled()) + .count(), + 3, + "All widgets should be disabled." + ); +} + +fn accesskit_output_single_egui_frame(run_ui: impl FnOnce(&Context)) -> TreeUpdate { + let ctx = Context::default(); + ctx.enable_accesskit(); + + let output = ctx.run(RawInput::default(), run_ui); + + output + .platform_output + .accesskit_update + .expect("Missing accesskit update") } diff --git a/crates/egui_demo_lib/src/demo/toggle_switch.rs b/crates/egui_demo_lib/src/demo/toggle_switch.rs index fbc42d601..ca32987ef 100644 --- a/crates/egui_demo_lib/src/demo/toggle_switch.rs +++ b/crates/egui_demo_lib/src/demo/toggle_switch.rs @@ -38,7 +38,9 @@ pub fn toggle_ui(ui: &mut egui::Ui, on: &mut bool) -> egui::Response { } // Attach some meta-data to the response which can be used by screen readers: - response.widget_info(|| egui::WidgetInfo::selected(egui::WidgetType::Checkbox, *on, "")); + response.widget_info(|| { + egui::WidgetInfo::selected(egui::WidgetType::Checkbox, ui.is_enabled(), *on, "") + }); // 4. Paint! // Make sure we need to paint: @@ -77,7 +79,9 @@ fn toggle_ui_compact(ui: &mut egui::Ui, on: &mut bool) -> egui::Response { *on = !*on; response.mark_changed(); } - response.widget_info(|| egui::WidgetInfo::selected(egui::WidgetType::Checkbox, *on, "")); + response.widget_info(|| { + egui::WidgetInfo::selected(egui::WidgetType::Checkbox, ui.is_enabled(), *on, "") + }); if ui.is_rect_visible(rect) { let how_on = ui.ctx().animate_bool_responsive(response.id, *on); diff --git a/crates/egui_plot/src/legend.rs b/crates/egui_plot/src/legend.rs index 26e820f64..903cdbb37 100644 --- a/crates/egui_plot/src/legend.rs +++ b/crates/egui_plot/src/legend.rs @@ -117,8 +117,14 @@ impl LegendEntry { let desired_size = total_extra + galley.size(); let (rect, response) = ui.allocate_exact_size(desired_size, Sense::click()); - response - .widget_info(|| WidgetInfo::selected(WidgetType::Checkbox, *checked, galley.text())); + response.widget_info(|| { + WidgetInfo::selected( + WidgetType::Checkbox, + ui.is_enabled(), + *checked, + galley.text(), + ) + }); let visuals = ui.style().interact(&response); let label_on_the_left = ui.layout().horizontal_placement() == Align::RIGHT;