From 1d2df6126b8e86b06b3414c610c460187686b15c Mon Sep 17 00:00:00 2001 From: harrisonoconnorhover <130776285+harrisonoconnorhover@users.noreply.github.com> Date: Fri, 9 Oct 2026 10:32:07 -0400 Subject: [PATCH] Dismiss worksheet popups on outside clicks and focus loss (#41) --- crates/ui-egui/src/grid.rs | 36 +++-- crates/ui-egui/tests/popup_dismissal.rs | 176 ++++++++++++++++++++++++ 2 files changed, 197 insertions(+), 15 deletions(-) create mode 100644 crates/ui-egui/tests/popup_dismissal.rs diff --git a/crates/ui-egui/src/grid.rs b/crates/ui-egui/src/grid.rs index 4b326cc36..6e28ec7bb 100644 --- a/crates/ui-egui/src/grid.rs +++ b/crates/ui-egui/src/grid.rs @@ -416,13 +416,14 @@ pub fn show(app: &mut SheetApp, ui: &mut egui::Ui) { paint_headers(&painter, &geo, sh, &sel, &t, &quads); } // Interaction. + let previous_popups = (app.grid.filter_menu, app.grid.list_picker, app.grid.header_menu); interact(app, ui, &resp, &geo, sh, &wb); if app.editor.as_ref().is_some_and(|e| !e.from_formula_bar) { in_cell_editor(app, ui, &geo, sh, &wb); } - filter_menu(app, ui, &geo); - list_picker(app, ui, &geo); - header_menu(app, ui); + filter_menu(app, ui, &geo, previous_popups.0 == app.grid.filter_menu); + list_picker(app, ui, &geo, previous_popups.1 == app.grid.list_picker); + header_menu(app, ui, previous_popups.2 == app.grid.header_menu); context_menu(app, ui); if app.session.clipboard.is_some() { app.grid.marching_phase = (app.grid.marching_phase + 0.5) % 8.0; @@ -1643,7 +1644,13 @@ fn in_cell_editor(app: &mut SheetApp, ui: &mut egui::Ui, geo: &Geo, sh: &Sheet, crate::formula_bar::editor_widget(app, &mut child, id, font, false, erect.width() - 4.0); } -fn filter_menu(app: &mut SheetApp, ui: &mut egui::Ui, geo: &Geo) { +/// Test the popup itself, not whether the pointer is over any part of the app. +/// A trigger can sit outside its popup, so its opening click must be ignored. +fn popup_dismissed(response: &egui::Response, was_open: bool) -> bool { + response.ctx.input(|i| !i.focused || i.key_pressed(egui::Key::Escape)) || (was_open && response.clicked_elsewhere()) +} + +fn filter_menu(app: &mut SheetApp, ui: &mut egui::Ui, geo: &Geo, was_open: bool) { let Some((col, at)) = app.grid.filter_menu else { return }; let Some(d) = app.session.active() else { return }; let wb = d.wb.clone(); @@ -1676,9 +1683,10 @@ fn filter_menu(app: &mut SheetApp, ui: &mut egui::Ui, geo: &Geo) { } let wb = app.session.active().map(|d| d.wb.clone()).unwrap_or(wb); let values = gridcraft_engine::cmd::data::filter_values(&wb, si, col); + let key = egui::Id::new(("filter_sel", col)); 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); - area.show(ui.ctx(), |ui| { + 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() { @@ -1691,7 +1699,6 @@ fn filter_menu(app: &mut SheetApp, ui: &mut egui::Ui, geo: &Geo) { } ui.separator(); ui.label(egui::RichText::new("Filter").strong()); - let key = egui::Id::new(("filter_sel", col)); let mut checks: Vec<(String, bool)> = ui.ctx().data_mut(|d| d.get_temp::>(key)).unwrap_or(values.clone()); let all = checks.iter().all(|(_, b)| *b); let mut all_new = all; @@ -1710,7 +1717,6 @@ fn filter_menu(app: &mut SheetApp, ui: &mut egui::Ui, geo: &Geo) { ui.horizontal(|ui| { if ui.button("Clear Filter").clicked() { let _ = app.run("data.filterBy", json!({"column": col_to_letters(col), "clear": true})); - ui.ctx().data_mut(|d| d.remove::>(key)); close = true; } if ui.button("Apply").clicked() { @@ -1722,13 +1728,13 @@ fn filter_menu(app: &mut SheetApp, ui: &mut egui::Ui, geo: &Geo) { json!({"column": col_to_letters(col), "values": vals, "blanks": blanks}) }; let _ = app.run("data.filterBy", p); - ui.ctx().data_mut(|d| d.remove::>(key)); close = true; } }); }); }); - if close || ui.input(|i| i.key_pressed(egui::Key::Escape)) || (ui.input(|i| i.pointer.any_pressed()) && !ui.ctx().is_pointer_over_egui()) { + if close || popup_dismissed(&response.response, was_open) { + ui.ctx().data_mut(|d| d.remove::>(key)); app.grid.filter_menu = None; } } @@ -1939,7 +1945,7 @@ fn paint_overlays( let _ = (wb, si); } -fn list_picker(app: &mut SheetApp, ui: &mut egui::Ui, geo: &Geo) { +fn list_picker(app: &mut SheetApp, ui: &mut egui::Ui, geo: &Geo, was_open: bool) { let Some(c) = app.grid.list_picker else { return }; let Some(d) = app.session.active() else { return }; let wb = d.wb.clone(); @@ -1952,7 +1958,7 @@ fn list_picker(app: &mut SheetApp, ui: &mut egui::Ui, geo: &Geo) { let items = gridcraft_engine::cmd::data::list_items(&wb, si, &dv); let r = geo.cell_rect(sh, c); let mut close = false; - egui::Area::new(egui::Id::new("dv_list")).fixed_pos(r.left_bottom()).order(egui::Order::Foreground).show(ui.ctx(), |ui| { + let response = egui::Area::new(egui::Id::new("dv_list")).fixed_pos(r.left_bottom()).order(egui::Order::Foreground).show(ui.ctx(), |ui| { egui::Frame::popup(ui.style()).inner_margin(2.0).show(ui, |ui| { ui.set_min_width(r.width().max(100.0)); egui::ScrollArea::vertical().max_height(200.0).show(ui, |ui| { @@ -1965,15 +1971,15 @@ fn list_picker(app: &mut SheetApp, ui: &mut egui::Ui, geo: &Geo) { }); }); }); - if close || ui.input(|i| i.key_pressed(egui::Key::Escape)) || (ui.input(|i| i.pointer.any_pressed()) && !ui.ctx().is_pointer_over_egui()) { + if close || popup_dismissed(&response.response, was_open) { app.grid.list_picker = None; } } -fn header_menu(app: &mut SheetApp, ui: &mut egui::Ui) { +fn header_menu(app: &mut SheetApp, ui: &mut egui::Ui, was_open: bool) { let Some((at, rows)) = app.grid.header_menu else { return }; let mut close = false; - egui::Area::new(egui::Id::new("header_menu")).fixed_pos(at).order(egui::Order::Foreground).show(ui.ctx(), |ui| { + let response = egui::Area::new(egui::Id::new("header_menu")).fixed_pos(at).order(egui::Order::Foreground).show(ui.ctx(), |ui| { egui::Frame::popup(ui.style()).show(ui, |ui| { ui.set_min_width(190.0); let items: Vec<(&str, &str)> = if rows { @@ -2033,7 +2039,7 @@ fn header_menu(app: &mut SheetApp, ui: &mut egui::Ui) { } }); }); - if close || ui.input(|i| i.key_pressed(egui::Key::Escape)) || (ui.input(|i| i.pointer.any_pressed()) && !ui.ctx().is_pointer_over_egui()) { + if close || popup_dismissed(&response.response, was_open) { app.grid.header_menu = None; } } diff --git a/crates/ui-egui/tests/popup_dismissal.rs b/crates/ui-egui/tests/popup_dismissal.rs new file mode 100644 index 000000000..391ccd6af --- /dev/null +++ b/crates/ui-egui/tests/popup_dismissal.rs @@ -0,0 +1,176 @@ +//! Custom worksheet popups must keep inside interactions and dismiss outside clicks. + +use egui::{Event, Key, Modifiers, PointerButton, Pos2, pos2}; +use egui_kittest::kittest::Queryable; +use gridcraft_engine::Session; +use gridcraft_engine::core::{CellRef, Value}; +use gridcraft_ui_egui::{SheetApp, grid}; +use serde_json::json; + +type Harness = egui_kittest::Harness<'static, SheetApp>; + +#[derive(Clone, Copy, Debug)] +enum Menu { + Header, + Filter, + Validation, +} + +fn harness() -> Harness { + let mut session = Session::new(); + session.new_workbook(); + session.execute("range.setValues", json!({"range": "A1", "values": [["Name", "Qty"], ["Red", 1], ["Blue", 2]]})).unwrap(); + session.execute("data.filter", json!({"range": "A1:B3"})).unwrap(); + session.execute("insert.table", json!({"range": "A1:B3"})).unwrap(); + session.execute("range.setValues", json!({"range": "H1", "values": [["Other", "Qty"], ["Green", 3]]})).unwrap(); + session.execute("insert.table", json!({"range": "H1:I2"})).unwrap(); + session.execute("data.validation", json!({"range": "E3", "type": "list", "formula1": "\"First,Second\""})).unwrap(); + session.execute("home.insertSheet", json!({})).unwrap(); + session.execute("sheet.activate", json!({"sheet": 0})).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); + h +} + +fn geo(h: &Harness) -> grid::Geo { + let app = h.state(); + let sh = app.session.active().unwrap().wb.active().unwrap(); + grid::Geo::new(sh, app.grid.rect.unwrap(), app.view().scroll) +} + +fn cell_pos(h: &Harness, cell: &str) -> Pos2 { + geo(h).cell_rect(h.state().session.active().unwrap().wb.active().unwrap(), CellRef::parse(cell).unwrap()).center() +} + +fn click(h: &mut Harness, pos: Pos2, button: PointerButton) { + h.input_mut().events.push(Event::PointerMoved(pos)); + h.input_mut().events.push(Event::PointerButton { pos, button, pressed: true, modifiers: Modifiers::NONE }); + h.input_mut().events.push(Event::PointerButton { pos, button, pressed: false, modifiers: Modifiers::NONE }); + h.run_steps(2); +} + +fn open(h: &mut Harness, menu: Menu) { + let geometry = geo(h); + let sh = h.state().session.active().unwrap().wb.active().unwrap(); + let (pos, button) = match menu { + Menu::Header => (pos2(cell_pos(h, "A1").x, (geometry.rect.top() + geometry.cells.top()) / 2.0), PointerButton::Secondary), + Menu::Filter => (grid::filter_button_rect(&geometry, sh, CellRef::parse("A1").unwrap()).center(), PointerButton::Primary), + Menu::Validation => { + let cell = CellRef::parse("E3").unwrap(); + let pos = grid::validation_arrow(sh, &geometry, cell).unwrap().center(); + h.state_mut().run("selection.set", json!({"cell": "E3"})).unwrap(); + h.run_steps(2); + (pos, PointerButton::Primary) + } + }; + click(h, pos, button); + assert!(is_open(h, menu), "{menu:?} must survive its opening click"); +} + +fn is_open(h: &Harness, menu: Menu) -> bool { + match menu { + Menu::Header => h.state().grid.header_menu.is_some(), + Menu::Filter => h.state().grid.filter_menu.is_some(), + Menu::Validation => h.state().grid.list_picker.is_some(), + } +} + +#[test] +fn worksheet_popups_close_on_grid_and_ribbon_clicks() { + for menu in [Menu::Header, Menu::Filter, Menu::Validation] { + for ribbon in [false, true] { + let mut h = harness(); + open(&mut h, menu); + let outside = if ribbon { pos2(1050.0, 52.0) } else { cell_pos(&h, "H7") }; + click(&mut h, outside, PointerButton::Primary); + assert!(!is_open(&h, menu), "{menu:?} stayed open after outside click (ribbon={ribbon})"); + if !ribbon { + assert_eq!(h.state().session.active().unwrap().selection.active.a1(), "H7"); + } + } + } +} + +#[test] +fn filter_closes_when_clicking_another_table_or_sheet() { + for sheet in [false, true] { + let mut h = harness(); + open(&mut h, Menu::Filter); + let outside = if sheet { pos2(44.0, h.state().grid.rect.unwrap().bottom() + 16.0) } else { cell_pos(&h, "H2") }; + click(&mut h, outside, PointerButton::Primary); + assert!(!is_open(&h, Menu::Filter), "filter stayed open after switching table/sheet"); + if sheet { + assert_eq!(h.state().session.active().unwrap().wb.active_sheet, 1); + } else { + assert_eq!(h.state().session.active().unwrap().selection.active.a1(), "H2"); + } + } +} + +#[test] +fn popup_actions_still_execute_and_filter_checkboxes_stay_open() { + let mut h = harness(); + open(&mut h, Menu::Header); + h.get_by_label("Copy").click(); + h.run_steps(2); + assert!(!is_open(&h, Menu::Header)); + assert!(h.state().session.clipboard.is_some()); + + open(&mut h, Menu::Filter); + h.get_by_label("(Select All)").click(); + h.run_steps(2); + assert!(is_open(&h, Menu::Filter), "clicking a checkbox must keep the filter open"); + let outside = cell_pos(&h, "H7"); + click(&mut h, outside, PointerButton::Primary); + open(&mut h, Menu::Filter); + h.get_by_label("Apply").click(); + h.run_steps(2); + let sh = h.state().session.active().unwrap().wb.active().unwrap(); + assert!(!sh.is_row_hidden(1) && !sh.is_row_hidden(2), "cancelled checkbox edits must not be applied after reopening"); + + open(&mut h, Menu::Filter); + h.get_by_label("(Select All)").click(); + h.run_steps(2); + h.get_by_label("Apply").click(); + h.run_steps(2); + assert!(!is_open(&h, Menu::Filter)); + assert!(h.state().session.active().unwrap().wb.active().unwrap().is_row_hidden(1)); + h.state_mut().run("data.clearFilter", json!({})).unwrap(); + h.run_steps(2); + + open(&mut h, Menu::Validation); + h.get_by_label("Second").click(); + h.run_steps(2); + assert!(!is_open(&h, Menu::Validation)); + assert_eq!(h.state().session.active().unwrap().wb.active().unwrap().value(CellRef::parse("E3").unwrap()), Value::from("Second")); +} + +#[test] +fn worksheet_popups_close_on_escape_or_window_focus_loss() { + for menu in [Menu::Header, Menu::Filter, Menu::Validation] { + for focus_loss in [false, true] { + let mut h = harness(); + open(&mut h, menu); + if focus_loss { + h.input_mut().focused = false; + h.input_mut().events.push(Event::WindowFocused(false)); + } else { + h.input_mut().events.push(Event::Key { + key: Key::Escape, + physical_key: None, + pressed: true, + repeat: false, + modifiers: Modifiers::NONE, + }); + } + h.run_steps(2); + assert!(!is_open(&h, menu), "{menu:?} stayed open (focus_loss={focus_loss})"); + } + } +}