From 2bc50341a01615716162077481f1dc0d4c0168aa Mon Sep 17 00:00:00 2001 From: vangel76 Date: Sun, 8 Nov 2026 22:18:34 +0100 Subject: [PATCH] fix(selection): Shift+arrows and Enter/Tab treat a merged cell as one cell (#3619) Shift+Left/Up from a selected merged cell now widen the selection past its edges, Shift+arrows shrink an area back over a merged cell, and Enter/Tab leave a selected merged cell instead of walking the cells it covers. Inside a larger selection Enter visits a merged cell once. Fixes #3619 Co-Authored-By: Claude Opus 5.5 --- crates/engine/src/cmd/edit/selection.rs | 45 ++++- crates/engine/src/selection.rs | 158 +++++++++++++++-- crates/engine/tests/it/main.rs | 1 + .../engine/tests/it/merged_selection_keys.rs | 162 ++++++++++++++++++ crates/ui-egui/src/app/editing.rs | 38 +++- 5 files changed, 377 insertions(+), 27 deletions(-) create mode 100644 crates/engine/tests/it/merged_selection_keys.rs diff --git a/crates/engine/src/cmd/edit/selection.rs b/crates/engine/src/cmd/edit/selection.rs index f171c65a8..6d1625f03 100644 --- a/crates/engine/src/cmd/edit/selection.rs +++ b/crates/engine/src/cmd/edit/selection.rs @@ -63,6 +63,11 @@ pub(super) fn selection_move(s: &mut Session, p: &Json) -> Result { let d = s.doc_mut()?; let Some(sheet) = d.wb.active() else { return Err(EngineError::NoDocument) }; let sel = &mut d.selection; + if extend && !jmp { + // Shift+arrows move the edge the anchor doesn't hold, keeping merged cells whole. + sel.extend_by(sheet, dr, dc); + return Ok(json!({"selection": sel.a1(), "active": sel.active.a1()})); + } // Extension moves the far corner, not the active cell. let from = if extend { sel.moving_end() } else { sel.active }; // Skip hidden rows/columns and step over merged areas. @@ -77,6 +82,14 @@ pub(super) fn selection_move(s: &mut Session, p: &Json) -> Result { jump(sheet, from, dr.signum(), dc.signum()) } else { let mut t = from.offset_clamped(dr, dc); + if !extend && let Some(m) = sheet.merge_at(from) { + // Leaving a merged cell moves from its edge. + if dr > 0 { + t = CellRef::new(m.end.offset_clamped(dr, 0).row, from.col); + } else if dc > 0 { + t = CellRef::new(from.row, m.end.offset_clamped(0, dc).col); + } + } let mut guard = 0; while (sheet.is_row_hidden(t.row) || sheet.is_col_hidden(t.col)) && guard < 100_000 { let n = t.offset_clamped(dr.signum(), dc.signum()); @@ -86,14 +99,6 @@ pub(super) fn selection_move(s: &mut Session, p: &Json) -> Result { t = n; guard += 1; } - if !extend && let Some(m) = sheet.merge_at(from) { - // Leaving a merged cell moves from its edge. - if dr > 0 { - t = CellRef::new(m.end.offset_clamped(dr, 0).row, from.col); - } else if dc > 0 { - t = CellRef::new(from.row, m.end.offset_clamped(0, dc).col); - } - } t }; if let Some(m) = sheet.merge_at(to) @@ -140,7 +145,29 @@ pub(super) fn selection_next(s: &mut Session, p: &Json) -> Result { let fwd = bool_param(p, "forward").unwrap_or(true); let by_row = bool_param(p, "byRow").unwrap_or(false); let d = s.doc_mut()?; - d.selection.cycle(fwd, by_row); + let Some(sheet) = d.wb.active() else { return Err(EngineError::NoDocument) }; + let sel = &d.selection; + if sel.is_one_merged_cell(sheet) { + // A selection that is just one merged cell is one cell, as in Excel: Enter and Tab leave + // it for the next cell instead of walking the cells it covers. + let step = if fwd { 1 } else { -1 }; + let (dr, dc) = if by_row { (0, step) } else { (step, 0) }; + selection_move(s, &json!({"dr": dr, "dc": dc}))?; + return Ok(json!({"active": s.doc()?.selection.active.a1()})); + } + // Inside a larger selection the cells a merged cell covers are skipped: it is visited once, + // at its top-left cell. + let area = sel.current(); + let cells = + (u64::from(area.end.row.saturating_sub(area.start.row)) + 1).saturating_mul(u64::from(area.end.col.saturating_sub(area.start.col)) + 1); + let mut next = sel.clone(); + for _ in 0..cells.min(1 << 20) { + next.cycle(fwd, by_row); + if sheet.merge_at(next.active).is_none_or(|m| m.start == next.active) { + break; + } + } + d.selection = next; Ok(json!({"active": d.selection.active.a1()})) } diff --git a/crates/engine/src/selection.rs b/crates/engine/src/selection.rs index 3f3feee9d..c0c6959cd 100644 --- a/crates/engine/src/selection.rs +++ b/crates/engine/src/selection.rs @@ -43,6 +43,10 @@ impl Selection { pub fn is_single_cell(&self) -> bool { self.ranges.len() == 1 && self.current().is_single() } + /// Whether the selection is exactly one merged cell, which Enter and Tab treat as one cell. + pub fn is_one_merged_cell(&self, sheet: &Sheet) -> bool { + self.ranges.len() == 1 && sheet.merge_at(self.active).is_some_and(|m| m == self.current()) + } /// The corner of the current area opposite the anchor: the end that Shift+arrows move. pub fn moving_end(&self) -> CellRef { let r = self.current(); @@ -67,23 +71,28 @@ impl Selection { /// they are: clicking column B's header selects just B even when B1:C1 is merged, as in Excel. pub fn expand_merges(&mut self, sheet: &Sheet) { for r in self.ranges.iter_mut() { - if r.is_full_cols() || r.is_full_rows() { - continue; - } - let mut cur = *r; - loop { - let mut grown = cur; - for m in &sheet.merges { - if m.intersects(&grown) { - grown = grown.union(m); - } - } - if grown == cur { - break; - } - cur = grown; - } - *r = cur; + *r = with_merges(sheet, *r); + } + } + /// Shift+arrows: moves the edge of the current area that the anchor doesn't hold by `dr` + /// rows and `dc` columns, keeping merged cells whole, as Excel does. The anchor is the whole + /// merged cell it is in, so from a selected merged cell Shift+Left and Shift+Up widen the + /// selection past its left and top edges. A step that would shrink the area onto the inside + /// of a merged cell (where keeping it whole would undo the step) goes on to the next line + /// that does shrink it, and when the area can't shrink any more it grows in that direction. + pub fn extend_by(&mut self, sheet: &Sheet, dr: i64, dc: i64) { + let anchor = sheet.merge_at(self.anchor).unwrap_or(RangeRef::cell(self.anchor)); + let mut r = self.current(); + if dc != 0 { + r = step_edge(sheet, r, anchor, dc, true); + } + if dr != 0 { + r = step_edge(sheet, r, anchor, dr, false); + } + if let Some(last) = self.ranges.last_mut() { + *last = r; + } else { + self.ranges.push(r); } } /// Moves the active cell within the selection (Enter/Tab inside a multi-cell selection). @@ -128,6 +137,91 @@ impl Selection { } } +/// `r` grown until every merged cell it touches is inside it. Whole columns and whole rows stay +/// as they are. +fn with_merges(sheet: &Sheet, r: RangeRef) -> RangeRef { + if r.is_full_cols() || r.is_full_rows() { + return r; + } + let mut cur = r; + loop { + let mut grown = cur; + for m in &sheet.merges { + if m.intersects(&grown) { + grown = grown.union(m); + } + } + if grown == cur { + return cur; + } + cur = grown; + } +} + +/// One Shift+arrow step of [`Selection::extend_by`] along one axis (`cols`: columns, else rows). +fn step_edge(sheet: &Sheet, r: RangeRef, anchor: RangeRef, d: i64, cols: bool) -> RangeRef { + let max = i64::from(if cols { MAX_COLS } else { MAX_ROWS }) - 1; + let hidden = |i: i64| if cols { sheet.is_col_hidden(i as u32) } else { sheet.is_row_hidden(i as u32) }; + // `from` moved by `by`, then on past hidden lines in the same direction (not off the sheet). + let land = |from: i64, by: i64| { + let mut t = from.saturating_add(by).clamp(0, max); + // Bounded like the arrow keys' walk over hidden lines. + for _ in 0..100_000 { + if !hidden(t) { + break; + } + let n = (t + by.signum()).clamp(0, max); + if n == t { + break; + } + t = n; + } + t + }; + let span = |x: &RangeRef| { + if cols { (i64::from(x.start.col), i64::from(x.end.col)) } else { (i64::from(x.start.row), i64::from(x.end.row)) } + }; + let with = |lo: i64, hi: i64| { + let (lo, hi) = (lo.clamp(0, max) as u32, hi.clamp(0, max) as u32); + let (s, e) = if cols { + (CellRef::new(r.start.row, lo), CellRef::new(r.end.row, hi)) + } else { + (CellRef::new(lo, r.start.col), CellRef::new(hi, r.end.col)) + }; + with_merges(sheet, RangeRef::new(s, e)) + }; + let (lo, hi) = span(&r); + let (alo, ahi) = span(&anchor); + // The anchor holds the low edge unless the area reaches past it on that side. + let moving_hi = lo >= alo; + let shrinking = if moving_hi { d < 0 && hi > ahi } else { d > 0 && lo < alo }; + if shrinking { + let mut t = if moving_hi { hi } else { lo }; + let mut by = d; + loop { + t = if moving_hi { land(t, by).max(ahi) } else { land(t, by).min(alo) }; + let cand = if moving_hi { with(lo, t) } else { with(t, hi) }; + if cand != r { + return cand; + } + if t == if moving_hi { ahi } else { alo } { + break; + } + // The step was undone because a merged cell crosses the new edge, and so is every + // edge up to where that merged cell starts (ends, for the low edge): go on from there + // so a tall or wide merged cell costs one step, not one per line it covers. + let across = sheet.merges.iter().filter(|m| m.intersects(&r)).filter_map(|m| { + let (ms, me) = span(m); + if moving_hi { (ms <= t && me > t).then_some(ms) } else { (ms < t && me >= t).then_some(me) } + }); + t = if moving_hi { across.min().map_or(t, |ms| ms.min(t)) } else { across.max().map_or(t, |me| me.max(t)) }; + by = d.signum(); + } + } + // Grow the area on the side the arrow points to. + if d > 0 { with(lo, land(hi, d)) } else { with(land(lo, d), hi) } +} + impl Selection { /// Ctrl+. (Next Corner of Selection): the active cell moves clockwise to the next corner of /// the current area (top-left, top-right, bottom-right, bottom-left) and becomes the anchor, @@ -287,6 +381,36 @@ mod tests { assert_eq!(current_region(&s, CellRef::new(5, 0)).a1(), "A1:B20000"); } + #[test] + fn shift_arrows_from_a_merged_cell() { + let mut s = Sheet::new("S"); + s.merges.push(RangeRef::parse("B2:C3").unwrap()); + let at = |s: &Sheet, a1: &str| { + let mut sel = Selection::at(CellRef::parse(a1).unwrap()); + sel.expand_merges(s); + sel + }; + for ((dr, dc), want) in [((0, -1), "A2:C3"), ((-1, 0), "B1:C3"), ((0, 1), "B2:D3"), ((1, 0), "B2:C4")] { + let mut sel = at(&s, "B2"); + sel.extend_by(&s, dr, dc); + assert_eq!(sel.a1(), want); + } + // Shrinking onto the inside of the merged cell goes on past it. + let mut sel = at(&s, "A1"); + sel.extend_by(&s, 1, 1); + assert_eq!(sel.a1(), "A1:C3"); + sel.extend_by(&s, 0, -1); + assert_eq!(sel.a1(), "A1:A3"); + // A hidden column is stepped over, and the sheet edge stops the area. + s.cols.entry(3).or_default().hidden = true; + let mut sel = at(&s, "B2"); + sel.extend_by(&s, 0, 1); + assert_eq!(sel.a1(), "B2:E3"); + let mut sel = at(&s, "A1"); + sel.extend_by(&s, -1, -1); + assert_eq!(sel.a1(), "A1"); + } + #[test] fn cycling() { let mut sel = Selection::range(RangeRef::parse("A1:B2").unwrap()); diff --git a/crates/engine/tests/it/main.rs b/crates/engine/tests/it/main.rs index ca960d759..d73a70235 100644 --- a/crates/engine/tests/it/main.rs +++ b/crates/engine/tests/it/main.rs @@ -18,6 +18,7 @@ mod hidden_sheet_navigation; mod ink_roundtrip; mod line_count_overflow; mod long_operator_chains; +mod merged_selection_keys; mod offset_overflow; mod percent_entry; mod pivot_unknown_func; diff --git a/crates/engine/tests/it/merged_selection_keys.rs b/crates/engine/tests/it/merged_selection_keys.rs new file mode 100644 index 000000000..dcb689843 --- /dev/null +++ b/crates/engine/tests/it/merged_selection_keys.rs @@ -0,0 +1,162 @@ +//! Shift+arrows and Enter/Tab on merged cells, as Excel does them. + +use gridcraft_engine::Session; +use serde_json::json; + +/// A new workbook with B2:C3 merged and nothing selected but A1. +fn merged_book() -> Session { + let mut s = Session::new(); + s.new_workbook(); + s.execute("selection.set", json!({"range": "B2:C3"})).unwrap(); + s.execute("home.mergeCells", json!({})).unwrap(); + s.execute("selection.set", json!({"cell": "A1"})).unwrap(); + s +} + +fn shift(s: &mut Session, dr: i64, dc: i64) -> String { + s.execute("selection.move", json!({"dr": dr, "dc": dc, "extend": true})).unwrap(); + s.doc().unwrap().selection.a1() +} + +fn enter(s: &mut Session, forward: bool, by_row: bool) -> (String, String) { + s.execute("selection.next", json!({"forward": forward, "byRow": by_row})).unwrap(); + let sel = &s.doc().unwrap().selection; + (sel.active.a1(), sel.a1()) +} + +#[test] +fn shift_arrows_from_a_merged_cell_widen_past_each_edge() { + let mut s = merged_book(); + for ((dr, dc), want) in [((0, -1), "A2:C3"), ((-1, 0), "B1:C3"), ((0, 1), "B2:D3"), ((1, 0), "B2:C4")] { + s.execute("selection.set", json!({"cell": "B2"})).unwrap(); + assert_eq!(s.doc().unwrap().selection.a1(), "B2:C3"); + assert_eq!(shift(&mut s, dr, dc), want, "Shift+({dr},{dc}) from the merged cell B2:C3"); + assert_eq!(s.doc().unwrap().selection.active.a1(), "B2", "the active cell stays"); + } +} + +#[test] +fn shift_arrows_keep_going_and_come_back_to_the_merged_cell() { + let mut s = merged_book(); + s.execute("selection.set", json!({"cell": "B2"})).unwrap(); + assert_eq!(shift(&mut s, 1, 0), "B2:C4"); + assert_eq!(shift(&mut s, 1, 0), "B2:C5"); + assert_eq!(shift(&mut s, 1, 0), "B2:C6"); + assert_eq!(shift(&mut s, -1, 0), "B2:C5"); + s.execute("selection.set", json!({"cell": "B2"})).unwrap(); + assert_eq!(shift(&mut s, 0, -1), "A2:C3"); + assert_eq!(shift(&mut s, 0, 1), "B2:C3", "back to just the merged cell"); + assert_eq!(shift(&mut s, 0, 1), "B2:D3"); + assert_eq!(shift(&mut s, -1, 0), "B1:D3"); + assert_eq!(shift(&mut s, 1, 0), "B2:D3"); +} + +#[test] +fn shift_arrows_shrink_past_a_merged_cell_they_pulled_in() { + let mut s = merged_book(); + assert_eq!(shift(&mut s, 0, 1), "A1:B1"); + assert_eq!(shift(&mut s, 1, 0), "A1:C3", "B2 pulls in the whole merged cell"); + assert_eq!(shift(&mut s, 0, -1), "A1:A3", "one step back leaves the merged cell out"); + assert_eq!(shift(&mut s, 0, 1), "A1:C3"); + assert_eq!(shift(&mut s, -1, 0), "A1:C1"); + assert_eq!(shift(&mut s, 1, 0), "A1:C3"); + assert_eq!(shift(&mut s, 1, 0), "A1:C4"); +} + +#[test] +fn enter_and_tab_leave_a_selected_merged_cell() { + let mut s = merged_book(); + for (forward, by_row, want) in [(true, false, "B4"), (false, false, "B1"), (true, true, "D2"), (false, true, "A2")] { + s.execute("selection.set", json!({"cell": "B2"})).unwrap(); + assert_eq!(enter(&mut s, forward, by_row), (want.to_string(), want.to_string()), "forward {forward} byRow {by_row}"); + } +} + +#[test] +fn enter_and_tab_out_of_a_merged_cell_step_over_hidden_lines() { + let mut s = merged_book(); + s.execute("selection.set", json!({"range": "A4"})).unwrap(); + s.execute("home.hideRows", json!({})).unwrap(); + s.execute("selection.set", json!({"range": "D1"})).unwrap(); + s.execute("home.hideColumns", json!({})).unwrap(); + s.execute("selection.set", json!({"cell": "B2"})).unwrap(); + assert_eq!(enter(&mut s, true, false).0, "B5", "row 4 is hidden"); + s.execute("selection.set", json!({"cell": "B2"})).unwrap(); + assert_eq!(enter(&mut s, true, true).0, "E2", "column D is hidden"); + s.execute("selection.set", json!({"cell": "B2"})).unwrap(); + s.execute("selection.move", json!({"dr": 1})).unwrap(); + assert_eq!(s.doc().unwrap().selection.a1(), "B5", "the Down arrow too"); +} + +#[test] +fn enter_inside_a_selection_visits_a_merged_cell_once() { + let mut s = merged_book(); + s.execute("selection.set", json!({"range": "A1:C3", "active": "A1"})).unwrap(); + let seen: Vec = (0..6).map(|_| enter(&mut s, true, false).0).collect(); + assert_eq!(seen, ["A2", "A3", "B1", "B2", "C1", "A1"]); + assert_eq!(s.doc().unwrap().selection.a1(), "A1:C3", "the selection stays"); + let back: Vec = (0..2).map(|_| enter(&mut s, false, false).0).collect(); + assert_eq!(back, ["C1", "B2"]); +} + +#[test] +fn shift_arrow_that_cannot_shrink_the_area_grows_it() { + // A2:B3 merged, anchor B1: Shift+Down pulls the area out to column A, past the anchor. + // Shift+Right can't shrink it from the left (the merged cell would come back), so it grows. + let mut s = Session::new(); + s.new_workbook(); + s.execute("selection.set", json!({"range": "A2:B3"})).unwrap(); + s.execute("home.mergeCells", json!({})).unwrap(); + s.execute("selection.set", json!({"cell": "B1"})).unwrap(); + assert_eq!(shift(&mut s, 1, 0), "A1:B3"); + assert_eq!(shift(&mut s, 0, 1), "A1:C3"); + assert_eq!(s.doc().unwrap().selection.active.a1(), "B1"); +} + +#[test] +fn shift_left_shrinks_past_a_chain_of_merged_cells_in_one_step() { + // B2:C3 and D3:E4 are both inside A1:E4. Shift+Left from E can't stop in column D (D3:E4 + // would come back) and stops in C, where B2:C3 is whole. + let mut s = merged_book(); + s.execute("selection.set", json!({"range": "D3:E4"})).unwrap(); + s.execute("home.mergeCells", json!({})).unwrap(); + s.execute("selection.set", json!({"cell": "A1"})).unwrap(); + assert_eq!(shift(&mut s, 3, 4), "A1:E4"); + assert_eq!(shift(&mut s, 0, -1), "A1:C4"); + assert_eq!(shift(&mut s, -1, 0), "A1:C3", "row 4 holds nothing merged once column D is out"); + assert_eq!(shift(&mut s, 0, -1), "A1:A3", "C can't stop inside B2:C3"); +} + +#[test] +fn shift_up_over_a_tall_merged_cell_takes_one_step() { + let mut s = Session::new(); + s.new_workbook(); + s.execute("selection.set", json!({"range": "B2:C500000"})).unwrap(); + s.execute("home.mergeCells", json!({})).unwrap(); + s.execute("selection.set", json!({"cell": "A1"})).unwrap(); + assert_eq!(shift(&mut s, 0, 1), "A1:B1"); + assert_eq!(shift(&mut s, 1, 0), "A1:C500000"); + assert_eq!(shift(&mut s, -1, 0), "A1:C1", "row 1 is the last row above the merged cell"); + assert_eq!(shift(&mut s, 1, 0), "A1:C500000"); + assert_eq!(shift(&mut s, 0, -1), "A1:A500000"); +} + +#[test] +fn shift_arrows_change_only_the_last_area() { + let mut s = merged_book(); + s.execute("selection.set", json!({"range": "E5:F6,B2"})).unwrap(); + assert_eq!(s.doc().unwrap().selection.a1(), "E5:F6,B2:C3"); + assert_eq!(shift(&mut s, 0, -1), "E5:F6,A2:C3"); +} + +#[test] +fn enter_skips_a_merged_cell_whose_start_is_outside_the_selection() { + // A whole column isn't widened to the merged cells it touches. B1 is covered by A1:B1, whose + // top-left cell is outside column B, so Enter skips it. + let mut s = Session::new(); + s.new_workbook(); + s.execute("selection.set", json!({"range": "A1:B1"})).unwrap(); + s.execute("home.mergeCells", json!({})).unwrap(); + s.execute("selection.set", json!({"range": "B:B", "active": "B2"})).unwrap(); + assert_eq!(enter(&mut s, false, false).0, "B1048576", "B1 is skipped, going up wraps to the bottom"); +} diff --git a/crates/ui-egui/src/app/editing.rs b/crates/ui-egui/src/app/editing.rs index 8682a435c..92689eb4f 100644 --- a/crates/ui-egui/src/app/editing.rs +++ b/crates/ui-egui/src/app/editing.rs @@ -404,7 +404,11 @@ impl SheetApp { /// Enter/Tab move inside a multi-cell selection, otherwise to the next cell. pub fn move_after_enter(&mut self, dr: i64, dc: i64) { - let multi = self.session.active().is_some_and(|d| !d.selection.is_single_cell()); + // A selection that is just one merged cell moves like one cell (Enter leaves it). + let multi = self + .session + .active() + .is_some_and(|d| !d.selection.is_single_cell() && !d.wb.active().is_some_and(|sh| d.selection.is_one_merged_cell(sh))); // Any other move (arrows, clicks, another sheet) leaves the selection off the cell the // last Tab landed on, which ends the run. let here = self.session.active().map(|d| (d.uid, d.wb.active_sheet(), d.selection.active)); @@ -503,3 +507,35 @@ mod protected_cell_tests { assert!(app.run("file.saveBytes", json!({"format": "xlsx"})).is_ok()); } } + +#[cfg(test)] +mod merged_cell_enter_tests { + use super::*; + use gridcraft_engine::Session; + + #[test] + fn enter_and_tab_leave_a_selected_merged_cell_and_keep_the_tab_run() { + let mut session = Session::new(); + session.new_workbook(); + let mut app = SheetApp::new(session, Default::default()); + app.run("selection.set", json!({"range": "B2:C3"})).unwrap(); + app.run("home.mergeCells", json!({})).unwrap(); + let sel = |app: &SheetApp| app.session.active().map(|d| d.selection.a1()).unwrap_or_default(); + for ((dr, dc), want) in [((1, 0), "B4"), ((-1, 0), "B1"), ((0, 1), "D2"), ((0, -1), "A2")] { + app.run("selection.set", json!({"cell": "B2"})).unwrap(); + assert_eq!(sel(&app), "B2:C3"); + app.move_after_enter(dr, dc); + assert_eq!(sel(&app), want, "Enter/Tab ({dr},{dc}) from the merged cell B2:C3"); + } + // A Tab run through the merged cell goes on past it: Enter then goes back to the column + // the run started in, on the next row. + app.tab_run = None; // the Shift+Tab above ended on A2 and started a run there + app.run("selection.set", json!({"cell": "A2"})).unwrap(); + app.move_after_enter(0, 1); + assert_eq!(sel(&app), "B2:C3"); + app.move_after_enter(0, 1); + assert_eq!(sel(&app), "D2"); + app.move_after_enter(1, 0); + assert_eq!(sel(&app), "A3"); + } +}