From 698a2a8009b5fa9dcb23f539b50d0bbee7ed54fe Mon Sep 17 00:00:00 2001 From: harrisonoconnorhover <130776285+harrisonoconnorhover@users.noreply.github.com> Date: Fri, 9 Oct 2026 21:26:06 -0400 Subject: [PATCH] Keep keyboard navigation separate from pointer clicks (#87) --- crates/ui-egui/src/grid.rs | 16 ++++---- crates/ui-egui/tests/interaction.rs | 57 ++++++++++++++++++++++++++++- 2 files changed, 64 insertions(+), 9 deletions(-) diff --git a/crates/ui-egui/src/grid.rs b/crates/ui-egui/src/grid.rs index 4b326cc..5ae1beb 100644 --- a/crates/ui-egui/src/grid.rs +++ b/crates/ui-egui/src/grid.rs @@ -983,6 +983,8 @@ fn paint_headers(p: &Painter, geo: &Geo, sh: &Sheet, sel: &gridcraft_engine::Sel fn interact(app: &mut SheetApp, ui: &mut egui::Ui, resp: &egui::Response, geo: &Geo, sh: &Sheet, wb: &Workbook) { let ctx = ui.ctx().clone(); + // Enter/Space synthesize egui clicks; only real pointer clicks use sheet coordinates. + let primary_clicked = resp.clicked_by(egui::PointerButton::Primary); let pointer = ctx.input(|i| i.pointer.clone()); let mods = ctx.input(|i| i.modifiers); let pos = pointer.interact_pos().or(pointer.hover_pos()); @@ -1048,7 +1050,7 @@ fn interact(app: &mut SheetApp, ui: &mut egui::Ui, resp: &egui::Response, geo: & } return; } - if app.session.draw_tool == "eraser" && (resp.clicked() || resp.dragged()) { + if app.session.draw_tool == "eraser" && (primary_clicked || resp.dragged()) { if let Some(p) = pos && let Some(("shape", id, _)) = crate::chartview::hit(app, geo, sh, p) && sh.shapes.iter().any(|s| s.id == id && s.kind == gridcraft_engine::model::ShapeKind::Ink) @@ -1059,7 +1061,7 @@ fn interact(app: &mut SheetApp, ui: &mut egui::Ui, resp: &egui::Response, geo: & } } // Objects (charts, pictures, shapes) take clicks first. - if resp.drag_started() || resp.clicked() { + if resp.drag_started() || primary_clicked { if let Some(p) = pos && let Some((kind, id, rect)) = crate::chartview::hit(app, geo, sh, p) { @@ -1075,13 +1077,13 @@ fn interact(app: &mut SheetApp, ui: &mut egui::Ui, resp: &egui::Response, geo: & } } return; - } else if resp.clicked() || resp.drag_started() { + } else if primary_clicked || resp.drag_started() { app.selected_chart = None; } } // Press: decide the drag mode. - if resp.drag_started() || (resp.clicked() && app.grid.drag == Drag::None) { + if resp.drag_started() || (primary_clicked && app.grid.drag == Drag::None) { // Where the press began (a drag is only recognised after the pointer has moved). let Some(p) = pointer.press_origin().or(pos) else { return }; if in_corner { @@ -1177,12 +1179,12 @@ fn interact(app: &mut SheetApp, ui: &mut egui::Ui, resp: &egui::Response, geo: & let _ = app.session.run("selection.set", json!({"range": ranges.join(","), "active": c.a1()})); } else { let _ = app.session.run("selection.set", json!({"cell": c.a1()})); - if resp.clicked() && wb.styles.get(sh.style_id(c)).num_fmt.as_str() == "checkbox" { + if primary_clicked && wb.styles.get(sh.style_id(c)).num_fmt.as_str() == "checkbox" { let _ = app.run("cell.toggleCheckbox", json!({"cell": c.a1()})); } // A click on a hyperlink's text follows it (like Excel); elsewhere in the cell selects. if let Some(h) = sh.hyperlinks.get(&c).cloned() - && resp.clicked() + && primary_clicked && !mods.any() { let r = geo.cell_rect(sh, c); @@ -1386,7 +1388,7 @@ fn interact(app: &mut SheetApp, ui: &mut egui::Ui, resp: &egui::Response, geo: & } } - if resp.clicked() || resp.drag_started() { + if primary_clicked || resp.drag_started() { resp.request_focus(); } keyboard(app, &ctx, resp, geo, sh); diff --git a/crates/ui-egui/tests/interaction.rs b/crates/ui-egui/tests/interaction.rs index 74bbeb4..82f3d50 100644 --- a/crates/ui-egui/tests/interaction.rs +++ b/crates/ui-egui/tests/interaction.rs @@ -1,9 +1,9 @@ //! UI interaction tests: real egui input through the whole app (no GPU needed). -use egui::{Event, Key, Modifiers}; +use egui::{Event, Key, Modifiers, PointerButton}; use gridcraft_engine::Session; use gridcraft_engine::core::{CellRef, Value}; -use gridcraft_ui_egui::SheetApp; +use gridcraft_ui_egui::{SheetApp, grid::Geo}; use serde_json::json; fn harness(session: Session) -> egui_kittest::Harness<'static, SheetApp> { @@ -40,6 +40,59 @@ fn value(h: &egui_kittest::Harness<'static, SheetApp>, a: &str) -> Value { h.state().session.active().and_then(|d| d.wb.active().map(|s| s.value(CellRef::parse(a).unwrap_or_default()))).unwrap_or_default() } +fn click_cell(h: &mut egui_kittest::Harness<'static, SheetApp>, cell: &str) { + let app = h.state(); + let sheet = app.session.active().unwrap().wb.active().unwrap(); + let geo = Geo::new(sheet, app.grid.rect.unwrap(), app.view().scroll); + let pos = geo.cell_rect(sheet, CellRef::parse(cell).unwrap()).center(); + h.input_mut().events.extend([ + Event::PointerMoved(pos), + Event::PointerButton { pos, button: PointerButton::Primary, pressed: true, modifiers: Modifiers::NONE }, + Event::PointerButton { pos, button: PointerButton::Primary, pressed: false, modifiers: Modifiers::NONE }, + ]); + h.run_steps(2); +} + +#[test] +fn keyboard_navigation_does_not_replay_the_last_pointer_click() { + let mut h = harness(blank()); + click_cell(&mut h, "B2"); + for (key_code, modifiers, expected) in [ + (Key::Enter, Modifiers::NONE, "B3"), + (Key::Enter, Modifiers::NONE, "B4"), + (Key::Enter, Modifiers::NONE, "B5"), + (Key::Enter, Modifiers::SHIFT, "B4"), + (Key::Tab, Modifiers::NONE, "C4"), + (Key::Enter, Modifiers::NONE, "C5"), + ] { + key(&mut h, key_code, modifiers); + let selection = &h.state().session.active().unwrap().selection; + assert_eq!(selection.active.a1(), expected); + assert_eq!(selection.anchor, selection.active); + assert!(h.state().editor.is_none(), "navigation must not start editing at the old pointer location"); + } + // A subsequent real pointer click must still select the clicked cell. + click_cell(&mut h, "D6"); + assert_eq!(h.state().session.active().unwrap().selection.active.a1(), "D6"); + key(&mut h, Key::Enter, Modifiers::NONE); + assert_eq!(h.state().session.active().unwrap().selection.active.a1(), "D7"); +} + +#[test] +fn space_toggles_the_active_checkbox_without_clicking_the_hovered_cell() { + let mut session = blank(); + session.execute("insert.checkbox", json!({"range": "A1:B1"})).unwrap(); + let mut h = harness(session); + click_cell(&mut h, "A1"); + assert_eq!(value(&h, "A1"), Value::Bool(true)); + key(&mut h, Key::ArrowRight, Modifiers::NONE); + key(&mut h, Key::Space, Modifiers::NONE); + assert_eq!(h.state().session.active().unwrap().selection.active.a1(), "B1"); + assert_eq!(value(&h, "A1"), Value::Bool(true), "the old pointer location must remain unchanged"); + assert_eq!(value(&h, "B1"), Value::Bool(true)); + assert!(h.state().editor.is_none()); +} + #[test] fn typing_enters_values_and_moves_down() { let mut h = harness(blank());