From c74e1a4d994941fa30b35c4a9c67416c15bdd78d Mon Sep 17 00:00:00 2001 From: Richard Date: Wed, 14 Oct 2026 09:29:01 +1100 Subject: [PATCH] fix(ui): menu items highlight under the pointer (#1174) The cell and header right-click menus, the ribbon dropdown menus and the data validation pick list drew their items with Button::frame(false), which turns the background off in every state, hover included. frame_when_inactive(false) keeps them flat at rest and shows the hover background, as Excel's menus do. Co-Authored-By: Claude Opus 5.5 --- crates/ui-egui/src/grid.rs | 6 +-- crates/ui-egui/src/ribbon.rs | 2 +- crates/ui-egui/tests/menu_hover.rs | 71 ++++++++++++++++++++++++++++++ 3 files changed, 75 insertions(+), 4 deletions(-) create mode 100644 crates/ui-egui/tests/menu_hover.rs diff --git a/crates/ui-egui/src/grid.rs b/crates/ui-egui/src/grid.rs index 97b787ef3..9197cd75f 100644 --- a/crates/ui-egui/src/grid.rs +++ b/crates/ui-egui/src/grid.rs @@ -2879,7 +2879,7 @@ fn context_menu(app: &mut SheetApp, ui: &mut egui::Ui) { ui.separator(); continue; } - if ui.add(egui::Button::new(lang.tr(label)).frame(false).min_size(vec2(190.0, 20.0))).clicked() { + if ui.add(egui::Button::new(lang.tr(label)).frame_when_inactive(false).min_size(vec2(190.0, 20.0))).clicked() { close = true; match id { "ui:pasteSpecial" => app.open_dialog("pasteSpecial", json!({})), @@ -3088,7 +3088,7 @@ fn list_picker(app: &mut SheetApp, ui: &mut egui::Ui, geo: &Geo, was_open: bool) ui.set_min_width(r.width().max(100.0)); egui::ScrollArea::vertical().max_height(200.0).show(ui, |ui| { for it in &items { - if ui.add(egui::Button::new(it.as_str()).frame(false).min_size(vec2(r.width().max(100.0), 18.0))).clicked() { + if ui.add(egui::Button::new(it.as_str()).frame_when_inactive(false).min_size(vec2(r.width().max(100.0), 18.0))).clicked() { app.run_or_alert("cell.set", json!({"cell": c.a1(), "input": it})); close = true; } @@ -3151,7 +3151,7 @@ fn header_menu(app: &mut SheetApp, ui: &mut egui::Ui, was_open: bool) { ui.separator(); continue; } - if ui.add(egui::Button::new(lang.tr(label)).frame(false).min_size(vec2(180.0, 20.0))).clicked() { + if ui.add(egui::Button::new(lang.tr(label)).frame_when_inactive(false).min_size(vec2(180.0, 20.0))).clicked() { close = true; match id { "ui:insertPaste" => app.open_dialog("insertPaste", json!({})), diff --git a/crates/ui-egui/src/ribbon.rs b/crates/ui-egui/src/ribbon.rs index 9c6d89eeb..889c62b0a 100644 --- a/crates/ui-egui/src/ribbon.rs +++ b/crates/ui-egui/src/ribbon.rs @@ -485,7 +485,7 @@ fn menu_items(app: &mut SheetApp, ui: &mut Ui, items: &[(&str, &str, serde_json: continue; } let lang = app.ui.language; - let response = ui.add(egui::Button::new(lang.tr(label)).frame(false).min_size(vec2(220.0, 22.0))); + let response = ui.add(egui::Button::new(lang.tr(label)).frame_when_inactive(false).min_size(vec2(220.0, 22.0))); if let Some(sequence) = keytip_table::item_letter(app.keytips.prefix().unwrap_or_default(), id) { keytip(app, ui, sequence, pos2(response.rect.right() - 10.0, response.rect.center().y)); } diff --git a/crates/ui-egui/tests/menu_hover.rs b/crates/ui-egui/tests/menu_hover.rs new file mode 100644 index 000000000..61e265235 --- /dev/null +++ b/crates/ui-egui/tests/menu_hover.rs @@ -0,0 +1,71 @@ +//! Menu items highlight under the pointer, as in Excel (#1174): no background at rest, the hover +//! background while the pointer is on an item. + +use egui::{Event, Modifiers, PointerButton, Pos2, Shape, accesskit::Role}; +use egui_kittest::kittest::Queryable as _; +use gridcraft_engine::Session; +use gridcraft_engine::core::CellRef; +use gridcraft_ui_egui::{SheetApp, grid}; + +type Harness = egui_kittest::Harness<'static, SheetApp>; + +fn harness() -> Harness { + let mut s = Session::new(); + s.new_workbook(); + let mut h = egui_kittest::Harness::builder().with_size(egui::vec2(1200.0, 800.0)).build_ui_state( + |ui, app: &mut SheetApp| { + app.logic(ui.ctx()); + app.ui(ui); + }, + SheetApp::new(s, Default::default()), + ); + h.run_steps(4); + h +} + +fn cell_center(h: &Harness, cell: &str) -> egui::Rect { + let app = h.state(); + let sh = app.session.active().unwrap().wb.active().unwrap(); + let geo = grid::Geo::new(sh, app.grid.rect.unwrap(), app.view().scroll); + geo.cell_rect(sh, CellRef::parse(cell).unwrap()) +} + +fn right_click(h: &mut Harness, pos: Pos2) { + h.input_mut().events.push(Event::PointerMoved(pos)); + for pressed in [true, false] { + h.input_mut().events.push(Event::PointerButton { pos, button: PointerButton::Secondary, pressed, modifiers: Modifiers::NONE }); + h.run_steps(2); + } +} + +/// Is a filled rectangle in the hovered-widget colour painted under `at`? +fn hover_fill_under(h: &Harness, at: Pos2) -> bool { + let fill = h.ctx.global_style().visuals.widgets.hovered.weak_bg_fill; + h.output().shapes.iter().any(|s| matches!(&s.shape, Shape::Rect(r) if r.fill == fill && r.rect.contains(at))) +} + +fn check_item(h: &mut Harness, label: &str) { + let at = h.get_by_role_and_label(Role::Button, label).rect().center(); + assert!(!hover_fill_under(h, at), "{label} has no background before the pointer is on it"); + h.input_mut().events.push(Event::PointerMoved(at)); + h.run_steps(2); + assert!(hover_fill_under(h, at), "{label} is highlighted under the pointer"); +} + +#[test] +fn cell_menu_items_highlight_under_the_pointer() { + let mut h = harness(); + let pos = cell_center(&h, "C5").center(); + right_click(&mut h, pos); + assert!(h.state().grid.context_menu.is_some()); + check_item(&mut h, "Copy"); +} + +#[test] +fn header_menu_items_highlight_under_the_pointer() { + let mut h = harness(); + let c1 = cell_center(&h, "C1"); + right_click(&mut h, egui::pos2(c1.center().x, c1.top() - 6.0)); + assert!(h.state().grid.header_menu.is_some()); + check_item(&mut h, "Copy"); +}