From d09694ff619a8836615ca7df658becf838670f1b Mon Sep 17 00:00:00 2001 From: prxnvt <53838599+prxnvt@users.noreply.github.com> Date: Mon, 2 Nov 2026 04:45:33 -0500 Subject: [PATCH] Form controls: a slicer tile under a control is not toggled by a click meant for the control A click on a form control that form_controls::interact leaves to the grid (Ctrl or Cmd+click, which selects the control; a click on a selected control, which moves it) fell through to the slicer tile branch, which looks only at slicers, so a control placed over a slicer's value tile had the tile toggled and the slicer selected before the generic object handling could select or drag the control. The slicer tile branch now runs only when the press lands on a slicer by the same test the generic path uses (the selected object's handle first, then the topmost object), so the z-order decides once; a control over a tile, and a selected object's handle over one, keep the generic path. Charts, pictures and shapes lie beneath slicers in that order, so they were not affected. Test (kittest): a_control_over_a_slicer_tile_keeps_the_generic_path_on_a_selecting_click (a button over the first tile; Cmd+click selects the button and the slicer's filter is unchanged; a click on the selected button still toggles nothing; an uncovered tile still toggles). --- crates/ui-egui/src/grid/interact.rs | 21 ++++++++-- crates/ui-egui/tests/it/objects.rs | 60 +++++++++++++++++++++++++++++ 2 files changed, 77 insertions(+), 4 deletions(-) diff --git a/crates/ui-egui/src/grid/interact.rs b/crates/ui-egui/src/grid/interact.rs index 65214813b..deb75d338 100644 --- a/crates/ui-egui/src/grid/interact.rs +++ b/crates/ui-egui/src/grid/interact.rs @@ -109,10 +109,14 @@ pub(super) fn interact(app: &mut SheetApp, ui: &mut egui::Ui, resp: &egui::Respo return; } // A plain click on a slicer value tile toggles it (Ctrl/Cmd for multi-select); the clear - // button resets it. Runs before the generic object select/drag so tiles stay clickable. + // button resets it. Runs before the generic object select/drag so tiles stay clickable, but + // only when the slicer is what the press lands on: a form control over a tile (Ctrl or + // Cmd+clicked, or clicked while selected, so `form_controls::interact` left the click) and a + // selected object's handle keep the generic path, which selects, moves or resizes them. if primary_clicked && !resp.drag_started() && let Some(p) = pos + && object_under(app, geo, sh, p).is_some_and(|(o, _, handle)| o.kind == ObjKind::Slicer && handle.is_none()) && let Some((id, act)) = crate::chartview::slicer_click(geo, sh, wb, wb.active_sheet(), p) { app.selected_object = Some(ObjRef::new(ObjKind::Slicer, id)); @@ -135,10 +139,10 @@ pub(super) fn interact(app: &mut SheetApp, ui: &mut egui::Ui, resp: &egui::Respo // The selected object's handles reach a little outside it, so they win over hit-testing. // Only they resize: a press anywhere on another object selects and moves it, as the // hover cursor shows. - let on_handle = at.and_then(|p| crate::chartview::selected_handle(app, geo, sh, p)); - let handle = on_handle.map(|(.., h)| h); + let under = at.and_then(|p| object_under(app, geo, sh, p)); + let handle = under.and_then(|(.., h)| h); if let Some(p) = at - && let Some((obj, _)) = on_handle.map(|(o, r, _)| (o, r)).or_else(|| crate::chartview::hit(app, geo, sh, p)) + && let Some((obj, _, _)) = under { app.selected_object = Some(obj); if resp.double_clicked() && app.open_object(obj) != crate::objects::OpenAction::Select { @@ -544,6 +548,15 @@ fn handle_cell_double_click(app: &mut SheetApp, input: &InteractionInput<'_>, c: false } +/// What a press at `p` lands on, for slicer tiles and the generic object handling alike, so the +/// z-order decides once: the selected object's handle (handles reach a little past the frame and +/// win over hit-testing), else the topmost object under `p` and its rect. +fn object_under(app: &SheetApp, geo: &Geo, sh: &Sheet, p: Pos2) -> Option<(ObjRef, Rect, Option)> { + crate::chartview::selected_handle(app, geo, sh, p) + .map(|(o, r, h)| (o, r, Some(h))) + .or_else(|| crate::chartview::hit(app, geo, sh, p).map(|(o, r)| (o, r, None))) +} + fn handle_context_menu_and_keyboard(app: &mut SheetApp, input: &InteractionInput<'_>) { let (geo, sh, sel, pos) = (input.geo, input.sheet, &input.selection, input.position); let resp = input.response; diff --git a/crates/ui-egui/tests/it/objects.rs b/crates/ui-egui/tests/it/objects.rs index e0c272817..8870f0709 100644 --- a/crates/ui-egui/tests/it/objects.rs +++ b/crates/ui-egui/tests/it/objects.rs @@ -108,6 +108,18 @@ fn click(h: &mut Harness, p: Pos2) { h.run_steps(2); } +fn click_with(h: &mut Harness, p: Pos2, modifiers: Modifiers) { + h.run_steps(30); + for pressed in [true, false] { + h.input_mut().events.push(Event::ModifiersChanged(modifiers)); + h.input_mut().events.push(Event::PointerMoved(p)); + h.input_mut().events.push(Event::PointerButton { pos: p, button: PointerButton::Primary, pressed, modifiers }); + h.step(); + } + h.input_mut().events.push(Event::ModifiersChanged(Modifiers::NONE)); + h.run_steps(2); +} + fn double_click(h: &mut Harness, p: Pos2) { // Separate the gesture from earlier clicks (egui's triple-click window). h.run_steps(45); @@ -136,6 +148,54 @@ fn kept(h: &Harness) -> Value { h.state().session.doc().unwrap().wb.active().unwrap().value(at("R30")) } +/// A form control over a slicer's value tile is the topmost object there, so a click that +/// `form_controls::interact` leaves to the grid (Cmd+click selects the control; a click on a +/// selected control moves it) takes the generic object path and never toggles the tile beneath. +#[test] +fn a_control_over_a_slicer_tile_keeps_the_generic_path_on_a_selecting_click() { + let (mut h, o) = fixture(); + let slicer = rect(&h, o.slicer); + // A button exactly over the slicer's first value tile: the tile in screen units at the + // grid's scale, then the button's anchor in sheet units from the slicer's own. + let (z, anchor, n, columns) = { + let app = h.state(); + let d = app.session.doc().unwrap(); + let sh = d.wb.active().unwrap(); + let sl = sh.slicers.iter().find(|s| s.id == o.slicer.id).unwrap(); + let geo = Geo::new(sh, app.grid.rect.unwrap(), app.view().scroll); + (geo.z, sl.anchor, gridcraft_engine::cmd::slicer::slicer_items(&d.wb, d.wb.active_sheet(), sl).len(), sl.columns) + }; + let layout = chartview::slicer_layout(slicer, z, n, columns); + let tile = layout.tiles[0]; + let params = json!({ + "at": anchor.cell.a1(), + "dx": anchor.dx + (tile.left() - slicer.left()) / z, + "dy": anchor.dy + (tile.top() - slicer.top()) / z, + "width": tile.width() / z, + "height": tile.height() / z, + "text": "Over", + }); + let button = ObjRef::new(ObjKind::Control, id(&h.state_mut().run("insert.button", params).unwrap(), "control")); + h.run_steps(2); + assert!(rect(&h, button).contains(tile.center()), "the button covers the tile: {:?} vs {tile:?}", rect(&h, button)); + let filter = + |h: &Harness| h.state().session.doc().unwrap().wb.active().unwrap().slicers.iter().find(|s| s.id == o.slicer.id).unwrap().selected.clone(); + assert_eq!(filter(&h), None); + // Cmd+click selects the control without working it, and the tile beneath stays as it was. + click_with(&mut h, tile.center(), Modifiers::COMMAND); + assert_eq!(h.state().selected_object, Some(button), "the control is selected"); + assert_eq!(filter(&h), None, "the tile under the control was not toggled"); + // A click on the selected control is for moving it, not for the tile beneath either. + click(&mut h, tile.center()); + assert_eq!(h.state().selected_object, Some(button)); + assert_eq!(filter(&h), None, "still no toggle"); + // The tile itself, clear of the control, still toggles. + let free = layout.tiles[1].center(); + h.state_mut().selected_object = None; + click(&mut h, free); + assert!(filter(&h).is_some(), "a click on an uncovered tile toggles it"); +} + #[test] fn a_click_selects_every_kind_and_slicers_win_over_charts() { let (mut h, o) = fixture();