diff --git a/crates/ui-egui/src/ribbon/animations.rs b/crates/ui-egui/src/ribbon/animations.rs index d43801603..daa71ea2e 100644 --- a/crates/ui-egui/src/ribbon/animations.rs +++ b/crates/ui-egui/src/ribbon/animations.rs @@ -164,7 +164,7 @@ pub(super) fn animations(app: &mut SlideApp, ui: &mut Ui) { deckcraft_model::AnimStart::WithPrevious => "With Previous", deckcraft_model::AnimStart::AfterPrevious => "After Previous", }); - dropdown(ui, "astart", s.unwrap_or(""), 112.0, cur.is_some(), |ui| { + dropdown(ui, "Start", s.unwrap_or(""), 112.0, cur.is_some(), |ui| { for (l, v) in [("On Click", "onClick"), ("With Previous", "withPrevious"), ("After Previous", "afterPrevious")] { if ui.button(l).clicked() { run(app, "animation.timing", json!({"start": v})); diff --git a/crates/ui-egui/src/ribbon/home.rs b/crates/ui-egui/src/ribbon/home.rs index 1c27dc983..267f161d3 100644 --- a/crates/ui-egui/src/ribbon/home.rs +++ b/crates/ui-egui/src/ribbon/home.rs @@ -171,7 +171,7 @@ fn font_group(app: &mut SlideApp, ui: &mut Ui, st: &Value) { let size = st.get("size").and_then(Value::as_f64); let fams = font_families(app); let theme_fonts: Vec = fams.iter().take(2).cloned().collect(); - dropdown(ui, "font", &font, 150.0, en, |ui| { + dropdown(ui, "Font", &font, 150.0, en, |ui| { for (k, f) in fams.iter().enumerate() { let label = if k == 0 && theme_fonts.len() == 2 { format!("{f} (Headings)") @@ -196,7 +196,7 @@ fn font_group(app: &mut SlideApp, ui: &mut Ui, st: &Value) { } }); let size_text = size.map(|s| widgets::fmt_num(s, "")).unwrap_or_default(); - dropdown(ui, "size", &size_text, 52.0, en, |ui| { + dropdown(ui, "Font size", &size_text, 52.0, en, |ui| { for s in deckcraft_engine::cmd::format::SIZES.iter().chain([72.0, 80.0, 88.0, 96.0].iter()) { if ui.button(widgets::fmt_num(*s, "")).clicked() { run(app, "format.size", json!({"size": s})); diff --git a/crates/ui-egui/src/tests.rs b/crates/ui-egui/src/tests.rs index f8fe10aab..677e971c5 100644 --- a/crates/ui-egui/src/tests.rs +++ b/crates/ui-egui/src/tests.rs @@ -1223,6 +1223,31 @@ fn step_app(services: Services) -> egui_kittest::Harness<'static, SlideApp> { h } +/// #1956: the ribbon's drop-downs are announced by their purpose, not by their value or an +/// internal key. Animations › Start with a shape that has no animation was "astart"; Home's Font +/// and Font size boxes were "font" and "size" while nothing told them a font or size. +#[test] +fn ribbon_dropdowns_are_named_by_purpose_even_when_empty() { + use egui::accesskit::Role; + let mut h = step_app(Services::default()); + // A fresh shape, selected but not edited and without an animation. + h.state_mut().run("shape.insert", json!({"preset": "rect", "rect": [100.0, 100.0, 200.0, 100.0]})).unwrap(); + h.state_mut().run("text.exit", json!({})).unwrap(); + h.state_mut().ui.tab = "Animations".into(); + h.run_steps(2); + let start = h.get_by_role_and_label(Role::ComboBox, "Start"); + assert_eq!(start.value(), None, "no animation, no start value"); + assert!(h.query_by_label("astart").is_none(), "the internal key is not a name"); + + h.state_mut().ui.tab = "Home".into(); + h.state_mut().run("edit.select", json!({"ids": []})).unwrap(); + h.run_steps(2); + for (name, key) in [("Font", "font"), ("Font size", "size")] { + assert!(h.query_by_role_and_label(Role::ComboBox, name).is_some(), "{name} is named while empty"); + assert!(h.query_by_role_and_label(Role::ComboBox, key).is_none(), "{key} is not announced"); + } +} + /// Copy of a bold selection offers HTML; paste of Pages-like HTML keeps the bold. #[test] fn copy_offers_html_and_paste_keeps_bold() { diff --git a/crates/ui-egui/src/widgets.rs b/crates/ui-egui/src/widgets.rs index 5bfbf041e..238259e5e 100644 --- a/crates/ui-egui/src/widgets.rs +++ b/crates/ui-egui/src/widgets.rs @@ -330,7 +330,9 @@ pub fn rows(ui: &mut Ui, add: impl FnOnce(&mut Ui) -> R) -> R { } /// A text dropdown field (font name, size): shows `current`, opens a popup with `add_items`. -pub fn dropdown(ui: &mut Ui, id: &str, current: &str, width: f32, enabled: bool, add_items: impl FnOnce(&mut Ui)) -> Response { +/// `label` is what assistive technology calls it ("Font", "Start"); `current` is exposed as its +/// value, so an empty box is still announced by its purpose and never by an internal key (#1956). +pub fn dropdown(ui: &mut Ui, label: &str, current: &str, width: f32, enabled: bool, add_items: impl FnOnce(&mut Ui)) -> Response { let t = Tokens::get(ui.ctx()); let (rect, resp) = ui.allocate_exact_size(vec2(width, 22.0), if enabled { Sense::click() } else { Sense::hover() }); let painter = ui.painter_at(rect); @@ -350,8 +352,10 @@ pub fn dropdown(ui: &mut Ui, id: &str, current: &str, width: f32, enabled: bool, if enabled { t.text } else { t.text_faint }, ); chevron(&painter, pos2(rect.max.x - 9.0, rect.center().y), 3.0, t.text_dim); - resp.widget_info(|| egui::WidgetInfo::labeled(egui::Role::ComboBox, enabled, if current.is_empty() { id } else { current })); - let _ = id; + resp.widget_info(|| egui::WidgetInfo { + current_text_value: (!current.is_empty()).then(|| current.to_string()), + ..egui::WidgetInfo::labeled(egui::Role::ComboBox, enabled, label) + }); egui::Popup::menu(&resp).width(width.max(140.0)).show(|ui| { egui::ScrollArea::vertical().max_height(360.0).show(ui, add_items); }); @@ -742,19 +746,26 @@ mod tests { } /// The custom ribbon dropdown is a real combo box in the accessibility tree, named by its - /// current value (0.37's strict unnamed-input check would otherwise reject it). + /// purpose with the shown text as its value. An empty one (no selection to read a font or + /// size from) keeps its name rather than falling back to an internal key (#1956). #[test] - fn the_dropdown_has_a_combo_box_role_and_a_name() { + fn the_dropdown_is_named_by_its_purpose_and_carries_its_value() { let mut harness = egui_kittest::Harness::builder().with_size(vec2(400.0, 200.0)).build_ui(|ui| { theme::install_fonts(ui.ctx()); - dropdown(ui, "font-size", "18 pt", 120.0, true, |ui| { + dropdown(ui, "Font size", "18 pt", 120.0, true, |ui| { ui.label("12 pt"); ui.label("18 pt"); }); + dropdown(ui, "Start", "", 112.0, false, |_| {}); }); harness.run(); - let combo = harness.get_by_role_and_label(Role::ComboBox, "18 pt"); + let combo = harness.get_by_role_and_label(Role::ComboBox, "Font size"); + assert_eq!(combo.value().as_deref(), Some("18 pt")); assert!(combo.rect().width() > 100.0); + assert!(harness.query_by_role_and_label(Role::ComboBox, "18 pt").is_none(), "the value is not its name"); + let empty = harness.get_by_role_and_label(Role::ComboBox, "Start"); + assert_eq!(empty.value(), None, "nothing shown, no value"); + assert!(harness.query_by_label("astart").is_none() && harness.query_by_label("start").is_none(), "no internal key is announced"); } /// #32: clicking the "More Colors" hex field closed the colour menu before anything could be