diff --git a/crates/ui-egui/src/grid.rs b/crates/ui-egui/src/grid.rs index 97b787ef3..00e0c18be 100644 --- a/crates/ui-egui/src/grid.rs +++ b/crates/ui-egui/src/grid.rs @@ -2573,7 +2573,8 @@ fn keyboard(app: &mut SheetApp, ctx: &egui::Context, resp: &egui::Response, geo: resp.request_focus(); } let other_focus = ctx.memory(|m| m.focused()).is_some_and(|f| f != resp.id && (editing || !stale.contains(&f))); - if editing || app.dialog.is_some() || other_focus || app.message.is_some() { + // An open filter menu owns the keyboard even when a click inside it left focus on nothing (#1516). + if editing || app.dialog.is_some() || other_focus || app.message.is_some() || app.grid.filter_menu.is_some() { return; } if ctx.memory(|m| m.focused()).is_none() { @@ -2768,16 +2769,24 @@ fn filter_menu(app: &mut SheetApp, ui: &mut egui::Ui, geo: &Geo, was_open: bool) let values = gridcraft_engine::cmd::data::filter_values(&wb, si, col); let key = egui::Id::new(("filter_sel", col)); let search_key = egui::Id::new(("filter_search", col)); + let buttons_key = egui::Id::new(("filter_buttons", col)); + // Enter is the menu's OK, as in Excel: taken before the search box (which would only drop its + // focus) and the value checkboxes (which would toggle) see it. A focused button keeps it as its click. + let buttons: Vec = ui.ctx().data_mut(|d| d.get_temp(buttons_key)).unwrap_or_default(); + let on_button = ui.ctx().memory(|m| m.focused()).is_some_and(|f| buttons.contains(&f)); + let enter = !on_button && ui.input_mut(|i| i.consume_key(egui::Modifiers::NONE, egui::Key::Enter)); let mut close = false; let area = egui::Area::new(egui::Id::new("filter_menu")).fixed_pos(at + vec2(-180.0, 10.0)).order(egui::Order::Foreground); let response = area.show(ui.ctx(), |ui| { egui::Frame::popup(ui.style()).show(ui, |ui| { ui.set_width(230.0); - if ui.button("↑ Sort Ascending").clicked() { + let ascending = ui.button("↑ Sort Ascending"); + if ascending.clicked() { let _ = app.run("data.sortAscending", json!({"column": col_to_letters(col)})); close = true; } - if ui.button("↓ Sort Descending").clicked() { + let descending = ui.button("↓ Sort Descending"); + if descending.clicked() { let _ = app.run("data.sortDescending", json!({"column": col_to_letters(col)})); close = true; } @@ -2814,11 +2823,16 @@ fn filter_menu(app: &mut SheetApp, ui: &mut egui::Ui, geo: &Geo, was_open: bool) let checks: Vec<(String, bool)> = checks.iter().map(|(v, on)| (v.clone(), *on && matches(v))).collect(); ui.separator(); ui.horizontal(|ui| { - if ui.button("Clear Filter").clicked() { + let clear = ui.button("Clear Filter"); + if clear.clicked() { let _ = app.run("data.filterBy", json!({"column": col_to_letters(col), "clear": true})); close = true; } - if ui.button("Apply").clicked() { + // Nothing checked would hide every row, so Enter (unlike a click on Apply) does nothing then. + let any = checks.iter().any(|(_, on)| *on); + let apply = ui.button("Apply"); + ui.ctx().data_mut(|d| d.insert_temp(buttons_key, vec![ascending.id, descending.id, clear.id, apply.id])); + if apply.clicked() || (enter && any) { let blanks = checks.iter().any(|(v, on)| v == "(Blanks)" && *on); let vals: Vec = checks.iter().filter(|(v, on)| *on && v != "(Blanks)").map(|(v, _)| v.clone()).collect(); let p = if checks.iter().all(|(_, b)| *b) { @@ -2836,6 +2850,7 @@ fn filter_menu(app: &mut SheetApp, ui: &mut egui::Ui, geo: &Geo, was_open: bool) ui.ctx().data_mut(|d| { d.remove::>(key); d.remove::(search_key); + d.remove::>(buttons_key); }); app.grid.filter_menu = None; } diff --git a/crates/ui-egui/tests/filter_menu_enter.rs b/crates/ui-egui/tests/filter_menu_enter.rs new file mode 100644 index 000000000..bd45a2cc1 --- /dev/null +++ b/crates/ui-egui/tests/filter_menu_enter.rs @@ -0,0 +1,87 @@ +//! Enter in the AutoFilter menu works like its Apply button, as Excel's OK (issue #1516): from the +//! search box or with a value focused it applies the filter and closes the menu, and the key never +//! reaches the grid underneath. + +use egui::{Event, Key, Modifiers, PointerButton}; +use egui_kittest::kittest::Queryable; +use gridcraft_engine::Session; +use gridcraft_ui_egui::SheetApp; +use serde_json::json; + +type Harness = egui_kittest::Harness<'static, SheetApp>; + +/// A1:A4 holds a header and three fruits under an AutoFilter, with the menu opened by Alt+Down on A1. +fn filter_menu_open() -> Harness { + let mut session = Session::new(); + session.new_workbook(); + session.execute("range.setValues", json!({"range": "A1", "values": [["Fruit"], ["Apple"], ["Banana"], ["Cherry"]]})).unwrap(); + session.execute("data.filter", json!({"range": "A1:A4"})).unwrap(); + session.execute("selection.set", json!({"cell": "A1"})).unwrap(); + 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(session, Default::default()), + ); + h.run_steps(4); + key(&mut h, Key::ArrowDown, Modifiers::ALT); + assert!(h.state().grid.filter_menu.is_some(), "Alt+Down opens the filter menu"); + h +} + +fn key(h: &mut Harness, k: Key, m: Modifiers) { + h.input_mut().events.push(Event::Key { key: k, physical_key: Some(k), pressed: true, repeat: false, modifiers: m }); + h.input_mut().events.push(Event::Key { key: k, physical_key: Some(k), pressed: false, repeat: false, modifiers: m }); + h.run_steps(2); +} + +/// Rows 2..=4 (Apple, Banana, Cherry): which ones the filter hides. +fn hidden(h: &Harness) -> Vec { + let sh = h.state().session.active().unwrap().wb.active().unwrap(); + (1..=3).map(|r| sh.is_row_hidden(r)).collect() +} + +fn active(h: &Harness) -> String { + h.state().session.active().unwrap().selection.active.a1() +} + +#[test] +fn enter_in_the_search_box_applies_the_filter() { + let mut h = filter_menu_open(); + h.input_mut().events.push(Event::Text("an".into())); + h.run_steps(2); + key(&mut h, Key::Enter, Modifiers::NONE); + assert_eq!(hidden(&h), [true, false, true], "only Banana matches the search"); + assert!(h.state().grid.filter_menu.is_none(), "Enter closes the menu"); + assert_eq!(active(&h), "A1", "Enter must not move the active cell"); +} + +#[test] +fn enter_with_a_value_clicked_applies_the_filter() { + let mut h = filter_menu_open(); + let apple = h.get_by_label("Apple").rect().center(); + h.input_mut().events.push(Event::PointerMoved(apple)); + for pressed in [true, false] { + h.input_mut().events.push(Event::PointerButton { pos: apple, button: PointerButton::Primary, pressed, modifiers: Modifiers::NONE }); + } + h.run_steps(2); + assert!(h.state().grid.filter_menu.is_some(), "clicking a value keeps the menu open"); + key(&mut h, Key::Enter, Modifiers::NONE); + // Enter applies the list as it is: it must not toggle the value back on. + assert_eq!(hidden(&h), [true, false, false], "Apple was unchecked"); + assert!(h.state().grid.filter_menu.is_none(), "Enter closes the menu"); + assert_eq!(active(&h), "A1", "Enter must not move the active cell"); +} + +#[test] +fn enter_with_nothing_checked_keeps_the_menu_open() { + let mut h = filter_menu_open(); + // A search that matches nothing leaves no value to show, as Excel's OK is then disabled. + h.input_mut().events.push(Event::Text("Bananna".into())); + h.run_steps(2); + key(&mut h, Key::Enter, Modifiers::NONE); + assert_eq!(hidden(&h), [false, false, false], "an empty selection must not hide every row"); + assert!(h.state().grid.filter_menu.is_some(), "the menu stays open"); + assert_eq!(active(&h), "A1"); +}