From c55a7e35aadefeb482d759af7d3fec64c3d59578 Mon Sep 17 00:00:00 2001 From: Harrison Smith Date: Sat, 31 Oct 2026 19:48:37 -0600 Subject: [PATCH] #2964: Alt, H, B opens the Borders menu with its item letters At the "HB" level every Borders sequence (HBA..HBW) shares the prefix, so control_badge drew no badge and nothing opened: after B the screen showed no key tips at all, although the next letter still applied its preset. The Borders menu now opens at its group level, as Excel's does, with each preset's letter (told apart by its params) and More Borders' M. A letter or Esc changes the prefix, which closes the menu as before; clicking the arrow still toggles it. Co-Authored-By: Claude Opus 5.5 --- crates/ui-egui/src/keytips/keytip_table.rs | 29 ++++++++++++++++++++++ crates/ui-egui/src/ribbon/home.rs | 18 +++++++++++--- crates/ui-egui/tests/keytips.rs | 26 +++++++++++++++++++ 3 files changed, 70 insertions(+), 3 deletions(-) diff --git a/crates/ui-egui/src/keytips/keytip_table.rs b/crates/ui-egui/src/keytips/keytip_table.rs index d14af141b..5303298bc 100644 --- a/crates/ui-egui/src/keytips/keytip_table.rs +++ b/crates/ui-egui/src/keytips/keytip_table.rs @@ -92,6 +92,25 @@ pub fn item_letter(prefix: &str, target: &str) -> Option<&'static str> { sequences_for(target).into_iter().find(|seq| seq.len() == prefix.len() + 1 && seq.starts_with(prefix)) } +/// True when `prefix` is the group level of a menu control: some of `target`'s sequences finish +/// one key below it (Borders' "HB" before HBA..HBW). The menu opens there, as Excel's does, so its +/// item letters show. +pub fn opens_menu(prefix: &str, target: &str) -> bool { + !prefix.is_empty() && is_group_level(prefix) && sequences_for(target).iter().any(|seq| seq.len() == prefix.len() + 1 && seq.starts_with(prefix)) +} + +/// The badge for a menu item that runs `target` with `params` (compact JSON, as `keytips.json` +/// writes it): its finish sequence one key below the prefix. Items sharing one command (the +/// Borders presets) are told apart by their params. +pub fn item_letter_with(prefix: &str, target: &str, params: &str) -> Option<&'static str> { + SEQUENCES.iter().find_map(|(seq, entry)| match entry { + Entry::Finish(Action::CommandWith(id, p)) if *id == target && *p == params && seq.len() == prefix.len() + 1 && seq.starts_with(prefix) => { + Some(*seq) + } + _ => None, + }) +} + /// Badge sequences for a command control: every sequence for `target` that continues from the /// current prefix (the painter strips the prefix, so the remaining letters show). pub fn control_badges(prefix: &str, target: &str) -> Vec<&'static str> { @@ -197,6 +216,16 @@ mod tests { assert!(group_extension("E", &["dialog:pasteSpecial"]).is_empty(), "the finish letter paints on the menu item"); } + #[test] + fn menu_levels_and_item_letters_with_params() { + assert!(opens_menu("HB", "home.borders")); + assert!(!opens_menu("H", "home.borders"), "the tab level shows the HB badge, not the menu"); + assert!(!opens_menu("", "home.borders")); + assert_eq!(item_letter_with("HB", "home.borders", "{\"preset\":\"all\"}"), Some("HBA")); + assert_eq!(item_letter_with("HB", "home.borders", "{\"preset\":\"bottom\"}"), Some("HBO")); + assert_eq!(item_letter_with("H", "home.borders", "{\"preset\":\"all\"}"), None); + } + #[test] fn item_letter_is_one_level_deep() { assert_eq!(item_letter("HV", "dialog:pasteSpecial"), Some("HVS")); diff --git a/crates/ui-egui/src/ribbon/home.rs b/crates/ui-egui/src/ribbon/home.rs index c061dfac3..34369be51 100644 --- a/crates/ui-egui/src/ribbon/home.rs +++ b/crates/ui-egui/src/ribbon/home.rs @@ -158,7 +158,11 @@ pub(super) fn home(app: &mut SheetApp, ui: &mut Ui) { act(app, "home.borders", json!({"preset": app.grid.last_border.clone()})); } ba.widget_info(|| egui::WidgetInfo::labeled(egui::Role::Button, ui.is_enabled(), "Border presets")); - egui::Popup::menu(&ba).show(|ui| { + // Alt, H, B opens the menu with each item's letter, as Excel's does (#2964); a letter + // or Esc changes the prefix, which closes it again (`ribbon_keys`). + let prefix = app.keytips.prefix().unwrap_or_default().to_owned(); + let keyed = keytip_table::opens_menu(&prefix, "home.borders"); + egui::Popup::menu(&ba).open_memory(popup_command(keyed, ba.clicked())).show(|ui| { for (label, preset) in [ ("Bottom Border", "bottom"), ("Top Border", "top"), @@ -175,13 +179,21 @@ pub(super) fn home(app: &mut SheetApp, ui: &mut Ui) { ("Top and Double Bottom Border", "topDoubleBottom"), ("Inside Borders", "inside"), ] { - if crate::border_preview::preset_button(ui, label, preset).clicked() { + let item = crate::border_preview::preset_button(ui, label, preset); + if let Some(sequence) = keytip_table::item_letter_with(&prefix, "home.borders", &json!({"preset": preset}).to_string()) { + keytip(app, ui, sequence, egui::pos2(item.rect.right() - 10.0, item.rect.center().y)); + } + if item.clicked() { app.grid.last_border = preset.to_string(); act(app, "home.borders", json!({"preset": preset})); } } ui.separator(); - if ui.button("More Borders…").clicked() { + let more = ui.button("More Borders…"); + if let Some(sequence) = keytip_table::item_letter(&prefix, "dialog:formatCells") { + keytip(app, ui, sequence, egui::pos2(more.rect.right() - 10.0, more.rect.center().y)); + } + if more.clicked() { app.open_dialog("formatCells", json!({"tab": "Border"})); } }); diff --git a/crates/ui-egui/tests/keytips.rs b/crates/ui-egui/tests/keytips.rs index b4418018a..68056f9ea 100644 --- a/crates/ui-egui/tests/keytips.rs +++ b/crates/ui-egui/tests/keytips.rs @@ -768,3 +768,29 @@ fn border_keytips_without_a_command_stay_silent() { assert_eq!(borders(&h, "A1"), Default::default(), "H,B,{k:?} must not draw a border"); } } + +/// #2964: Alt, H, B opens the Borders menu (Excel shows it with each item's letter) instead of +/// leaving nothing on screen; a letter then picks its item and the menu closes, and Esc backs out. +#[test] +fn borders_keytip_opens_the_borders_menu() { + let mut h = harness(selected("A1:B2")); + sequence(&mut h, &[Key::H, Key::B], false); + h.run_steps(2); + assert_eq!(h.state().keytips.prefix(), Some("HB")); + assert!(egui::Popup::is_any_open(&h.ctx), "Alt, H, B opens the Borders menu"); + assert!(h.query_by_label("All Borders").is_some(), "the menu's items show"); + letter(&mut h, Key::A, false); + h.run_steps(2); + assert!(!egui::Popup::is_any_open(&h.ctx), "picking an item closes the menu"); + assert_eq!(h.state().keytips.prefix(), None); + assert!(sides(&h, "B2").iter().all(|s| *s != gridcraft_engine::model::BorderStyle::None), "A applied All Borders"); + + let mut h = harness(blank()); + sequence(&mut h, &[Key::H, Key::B], false); + h.run_steps(2); + assert!(egui::Popup::is_any_open(&h.ctx)); + key(&mut h, Key::Escape); + h.run_steps(2); + assert_eq!(h.state().keytips.prefix(), Some("H"), "Esc backs out to the Home letters"); + assert!(!egui::Popup::is_any_open(&h.ctx), "and closes the menu"); +}