AutoFilter: Enter in the filter menu applies the filter and closes it (fixes #1516)

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<egui::Id> = 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<String> = 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::<Vec<(String, bool)>>(key);
             d.remove::<String>(search_key);
+            d.remove::<Vec<egui::Id>>(buttons_key);
         });
         app.grid.filter_menu = None;
     }

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<bool> {
+    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");
+}