T14 review fixes: numbers sort before words, a removed filter takes its field, sort rows take free columns, grids fit their cursor, land on merges and keep Tab, formulas guard long references and count only numbers

TASKS.md

@@ -44,11 +44,11 @@ Per-component lines live in `tasks/`. Tags there:
 - [x] T11d Motion · Effects
 - [x] T12a Data Display · People & numbers
 - [x] T12b Data Display · Records
-- [ ] T12c Data Display · Media
+- [x] T12c Data Display · Media
 - [x] T12d Data Display · Codes & measures
-- [ ] T13a Lists & Trees · Lists
+- [x] T13a Lists & Trees · Lists
-- [ ] T13b Lists & Trees · Trees
+- [x] T13b Lists & Trees · Trees
-- [ ] T13c Lists & Trees · Files
+- [x] T13c Lists & Trees · Files
 - [ ] T14a Tables · Core
 - [ ] T14b Tables · Columns & rows
 - [ ] T14c Tables · Grids
@@ -192,11 +192,15 @@ Per-component lines live in `tasks/`. Tags there:
 | T12b | 2 | PASS | |
 | T12c | 1 | FAIL | the gallery kept an open index past a shrunk list, and the lightbox asserted on it |
 | T12c | 2 | FAIL | emptying the list removed the open lightbox without handing focus back |
+| T12c | 3 | PASS | |
 | T12d | 1 | FAIL | Code 128 took a set-switch character, a usage bar's float sum tripped its assert, and a dense QR floored its modules to nothing |
 | T12d | 2 | PASS | |
 | T13a | 1 | FAIL | Down and Cmd-A picked disabled rows, and Enter opened them |
+| T13a | 2 | PASS | |
 | T13b | 1 | FAIL | Down stuck on a loading row and never reached the nodes past it |
+| T13b | 2 | PASS | |
 | T13c | 1 | FAIL | the cursor kept its index through a re-sort, so Enter opened another file |
-| T14a | 1 | — | pending: codex still logged out |
+| T13c | 2 | PASS | |
-| T14b | 1 | — | pending: codex still logged out |
+| T14a | 1 | FAIL | numbers and words compared out of order in a mixed column, and the sort panicked |
-| T14c | 1 | — | pending review |
+| T14b | 1 | FAIL | a removed filter rule left its typing to the next rule, and two sort rows could take one column |
+| T14c | 1 | FAIL | a shrunk grid kept its cursor outside, long column letters overflowed, presses landed in merged cells, Tab left the grid, and ranges counted text and blanks |

src/tables/builders.rs

@@ -20,7 +20,7 @@ type OnSorts = Rc<dyn Fn(&[SortKey], &mut Window, &mut App)>;
 /// The newest rules and hearer, for value fields whose edits come later.
 type Latest = Rc<RefCell<(Vec<FilterRule>, bool, Option<OnFilters>)>>;
 
-/// A builder's value fields, one per rule, and what their edits report to.
+/// A builder's value fields, one per rule in the rules' order, and what their edits report to.
 #[derive(Default)]
 struct Fields {
     fields: Vec<(Entity<TextInput>, Subscription)>,
@@ -34,6 +34,19 @@ fn choices(columns: &[(SharedString, SharedString)]) -> Vec<Choice> {
         .collect()
 }
 
+/// The columns a sort row may take: its own, and those no other row holds.
+fn open(
+    columns: &[(SharedString, SharedString)],
+    keys: &[SortKey],
+    own: &SharedString,
+) -> Vec<(SharedString, SharedString)> {
+    columns
+        .iter()
+        .filter(|(key, _)| key == own || !keys.iter().any(|sort| sort.column == *key))
+        .cloned()
+        .collect()
+}
+
 /// Filter rules to build: each a column, a test and a value, and a switch between keeping rows that pass all and rows that pass any. The owner keeps the rules and hands them to a `DataTable`.
 #[derive(IntoElement)]
 pub struct FilterBuilder {
@@ -86,13 +99,19 @@ impl RenderOnce for FilterBuilder {
         let latest = fields.read(cx).latest.clone();
         *latest.borrow_mut() = (self.rules.clone(), self.any, self.on_change.clone());
         while fields.read(cx).fields.len() < self.rules.len() {
-            let ix = fields.read(cx).fields.len();
             let field = cx.new(|cx| TextInput::new(window, cx));
-            let latest = latest.clone();
+            let (latest, owner) = (latest.clone(), fields.downgrade());
             let heard = window.subscribe(&field, cx, move |field, event, window, cx| {
                 if !matches!(event, InputEvent::Changed) {
                     return;
                 }
+                let Some(ix) = owner.upgrade().and_then(|owner| {
+                    let held = &owner.read(cx).fields;
+                    held.iter()
+                        .position(|(held, _)| held.entity_id() == field.entity_id())
+                }) else {
+                    return;
+                };
                 let (mut rules, any, on_change) = latest.borrow().clone();
                 let Some(rule) = rules.get_mut(ix) else {
                     return;
@@ -141,7 +160,8 @@ impl RenderOnce for FilterBuilder {
             .iter()
             .enumerate()
             .map(|(ix, rule)| {
-                let (column, test, remove) = (emit.clone(), emit.clone(), emit.clone());
+                let (column, test, remove, held) =
+                    (emit.clone(), emit.clone(), emit.clone(), fields.clone());
                 let (for_column, for_test, for_remove) =
                     (rules.clone(), rules.clone(), rules.clone());
                 div()
@@ -189,6 +209,7 @@ impl RenderOnce for FilterBuilder {
                             .on_click(move |_, window, cx| {
                                 let mut next = for_remove.clone();
                                 next.remove(ix);
+                                held.update(cx, |held, _| drop(held.fields.remove(ix)));
                                 remove(next, any, window, cx)
                             }),
                     )
@@ -259,8 +280,15 @@ impl SortBuilder {
         }
     }
 
+    /// The keys, each on its own column.
     pub fn keys(mut self, keys: impl IntoIterator<Item = SortKey>) -> Self {
         self.keys = keys.into_iter().collect();
+        assert!(
+            self.keys.iter().enumerate().all(|(ix, key)| self.keys[..ix]
+                .iter()
+                .all(|before| before.column != key.column)),
+            "sort keys take a column each"
+        );
         self
     }
 
@@ -318,7 +346,7 @@ impl RenderOnce for SortBuilder {
                             div().w(theme.label_width()).child(
                                 Select::new(
                                     (id.clone(), format!("sort-column-{ix}")),
-                                    choices(&self.columns),
+                                    choices(&open(&self.columns, &keys, &sort.column)),
                                 )
                                 .selected(sort.column.clone())
                                 .size(ControlSize::Sm)
@@ -381,3 +409,35 @@ impl RenderOnce for SortBuilder {
             }))
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use gpui::SharedString;
+
+    use super::{SortKey, open};
+
+    #[test]
+    fn a_sort_row_offers_its_own_column_and_the_unused_ones() {
+        let columns: Vec<(SharedString, SharedString)> = ["a", "b", "c"]
+            .map(|key| (key.into(), key.to_uppercase().into()))
+            .to_vec();
+        let keys = [
+            SortKey {
+                column: "a".into(),
+                rising: true,
+            },
+            SortKey {
+                column: "b".into(),
+                rising: false,
+            },
+        ];
+        let offered = |own: &'static str| {
+            open(&columns, &keys, &own.into())
+                .into_iter()
+                .map(|(key, _)| key.to_string())
+                .collect::<Vec<_>>()
+        };
+        assert_eq!(offered("a"), ["a", "c"]);
+        assert_eq!(offered("b"), ["b", "c"]);
+    }
+}

src/tables/formula.rs

@@ -20,6 +20,9 @@ static REFERENCE: LazyLock<Regex> =
 /// How deep formulas may lean on each other before a loop is assumed.
 const DEPTH: usize = 64;
 
+/// The most cells a range may cover.
+const RANGE_CELLS: usize = 10_000;
+
 /// A column's letters: A to Z, then AA.
 pub(crate) fn letters(mut col: usize) -> String {
     let mut out = Vec::new();
@@ -39,50 +42,71 @@ pub(crate) fn name(row: usize, col: usize) -> String {
     format!("{}{}", letters(col), row + 1)
 }
 
-/// The row and column an A1 name points to, counted from zero.
+/// The row and column an A1 name points to, counted from zero; none when it is too long to count.
 pub(crate) fn place(text: &str) -> Option<(usize, usize)> {
     let caps = REFERENCE
         .captures(text)
         .filter(|caps| caps.get(0).is_some_and(|all| all.as_str() == text))?;
-    let col = caps[1].bytes().fold(0usize, |col, letter| {
+    let col = caps[1].bytes().try_fold(0usize, |col, letter| {
-        col * 26 + usize::from(letter - b'A' + 1)
+        col.checked_mul(26)?
-    }) - 1;
+            .checked_add(usize::from(letter - b'A' + 1))
+    })? - 1;
     let row = caps[2].parse::<usize>().ok()?.checked_sub(1)?;
     Some((row, col))
 }
 
-/// Every cell name in a rectangle, row by row.
+/// Every cell in a rectangle, row by row.
-fn span(from: &str, to: &str) -> Result<Vec<String>, String> {
+fn span(from: &str, to: &str) -> Result<Vec<(usize, usize)>, String> {
     let ((r0, c0), (r1, c1)) = (
         place(from).ok_or("a bad cell")?,
         place(to).ok_or("a bad cell")?,
     );
     let (rows, cols) = (r0.min(r1)..=r0.max(r1), c0.min(c1)..=c0.max(c1));
+    let covered = (r0.abs_diff(r1) + 1).saturating_mul(c0.abs_diff(c1) + 1);
+    if covered > RANGE_CELLS {
+        return Err(format!("{from}:{to} covers more than {RANGE_CELLS} cells"));
+    }
     Ok(rows
-        .flat_map(|row| cols.clone().map(move |col| name(row, col)))
+        .flat_map(|row| cols.clone().map(move |col| (row, col)))
         .collect())
 }
 
-/// A formula with its ranges written out and its functions in the evaluator's words.
+/// A range function over the numbers in its range; text and blanks are left out, as in a spreadsheet.
-fn expand(source: &str) -> Result<String, String> {
+fn aggregate(function: &str, numbers: &[f64]) -> Result<f64, String> {
+    let sum: f64 = numbers.iter().sum();
+    let least = || numbers.iter().copied().reduce(f64::min).unwrap_or(0.0);
+    let most = || numbers.iter().copied().reduce(f64::max).unwrap_or(0.0);
+    match function.to_uppercase().as_str() {
+        "SUM" => Ok(sum),
+        "AVERAGE" | "AVG" if numbers.is_empty() => Err("no numbers to average".into()),
+        "AVERAGE" | "AVG" => Ok(sum / numbers.len() as f64),
+        "MIN" => Ok(least()),
+        "MAX" => Ok(most()),
+        "COUNT" => Ok(numbers.len() as f64),
+        other => Err(format!("{other} takes no range")),
+    }
+}
+
+/// A formula with each range function worked out and its other functions in the evaluator's words.
+fn expand(
+    cells: &dyn Fn(usize, usize) -> SharedString,
+    source: &str,
+    depth: usize,
+) -> Result<String, String> {
     let mut failure = None;
     let written = RANGE.replace_all(source, |caps: &Captures| {
-        let cells = match span(&caps[2], &caps[3]) {
+        let worked = span(&caps[2], &caps[3]).and_then(|places| {
-            Ok(cells) => cells,
+            let values = places
+                .iter()
+                .map(|(row, col)| value(cells, *row, *col, depth + 1))
+                .collect::<Result<Vec<_>, _>>()?;
+            let numbers: Vec<f64> = values.into_iter().flatten().collect();
+            aggregate(&caps[1], &numbers)
+        });
+        match worked {
+            Ok(number) => format!("({number})"),
             Err(error) => {
-                failure = Some(error);
+                failure.get_or_insert(error);
-                return String::new();
-            }
-        };
-        let (sum, count) = (cells.join("+"), cells.len());
-        match caps[1].to_uppercase().as_str() {
-            "SUM" => format!("({sum})"),
-            "AVERAGE" | "AVG" => format!("(({sum})/{count})"),
-            "MIN" => format!("min({})", cells.join(",")),
-            "MAX" => format!("max({})", cells.join(",")),
-            "COUNT" => count.to_string(),
-            other => {
-                failure = Some(format!("{other} takes no range"));
                 String::new()
             }
         }
@@ -119,7 +143,7 @@ fn formula(
     if depth > DEPTH {
         return Err("a loop".into());
     }
-    let expanded = expand(source)?;
+    let expanded = expand(cells, source, depth)?;
     let mut variables: Vec<(SharedString, f64)> = Vec::new();
     for caps in REFERENCE.captures_iter(&expanded) {
         let reference = caps[0].to_string();
@@ -262,6 +286,31 @@ mod tests {
         );
     }
 
+    #[test]
+    fn range_functions_read_only_the_numbers_in_their_range() {
+        let cells = sheet(&[&[
+            "8",
+            "text",
+            "",
+            "=COUNT(A1:C1)",
+            "=AVERAGE(A1:C1)",
+            "=MIN(A1:C1)",
+        ]]);
+        assert_eq!(shown(&cells, 0, 3), "1");
+        assert_eq!(shown(&cells, 0, 4), "8");
+        assert_eq!(shown(&cells, 0, 5), "8");
+        let blank = sheet(&[&["", "=AVERAGE(A1:A1)"]]);
+        assert_eq!(shown(&blank, 0, 1), "#ERR", "nothing to average");
+    }
+
+    #[test]
+    fn a_reference_too_long_to_count_fails_as_a_formula() {
+        assert_eq!(place("ZZZZZZZZZZZZZZZZZZZZ1"), None);
+        let cells = sheet(&[&["=ZZZZZZZZZZZZZZZZZZZZ1", "=SUM(A1:ZZZZ99999)"]]);
+        assert_eq!(shown(&cells, 0, 0), "#ERR");
+        assert_eq!(shown(&cells, 0, 1), "#ERR", "a range too wide to walk");
+    }
+
     #[test]
     fn fills_step_numbers_shift_formulas_and_repeat_text() {
         let texts = |list: &[&'static str]| {

src/tables/grid.rs

@@ -6,9 +6,10 @@ use gpui::{
     SharedString, StatefulInteractiveElement, Styled, Window, canvas, div, point, prelude::*,
 };
 
-use super::gridkeys::{begin, fill, keys};
+use super::gridkeys::{Metrics, across, begin, fill, fit, keys, land};
 use crate::{
     forms::{Editing, Input},
+    primitives::{FocusNext, FocusPrev},
     theme::{ActiveTheme, ControlSize, TextSize},
 };
 
@@ -49,34 +50,6 @@ pub(crate) struct Face {
     pub on_change: Option<OnCells>,
 }
 
-/// Sizes in pixels: a cell, a header, the row labels, and the frozen stretch.
-#[derive(Clone, Copy)]
-pub(crate) struct Metrics {
-    pub wide: Pixels,
-    pub tall: Pixels,
-    pub label: Pixels,
-    pub frozen: Point<Pixels>,
-}
-
-impl Metrics {
-    /// The cell under a point within the grid, if any.
-    pub(crate) fn cell_at(
-        &self,
-        at: Point<Pixels>,
-        offset: Point<Pixels>,
-        face: (usize, usize),
-    ) -> Option<(usize, usize)> {
-        let (x, y) = (at.x - self.label, at.y - self.tall);
-        if x < Pixels::ZERO || y < Pixels::ZERO {
-            return None;
-        }
-        let x = if x < self.frozen.x { x } else { x + offset.x };
-        let y = if y < self.frozen.y { y } else { y + offset.y };
-        let (col, row) = ((x / self.wide) as usize, (y / self.tall) as usize);
-        (row < face.0 && col < face.1).then_some((row, col))
-    }
-}
-
 /// A drag of a grid's fill handle, named by its owner.
 pub(crate) struct FillDrag {
     owner: EntityId,
@@ -96,7 +69,13 @@ pub(crate) fn grid(face: Face, base: Div, window: &mut Window, cx: &mut App) ->
     let sheet: Entity<Sheet> =
         window.use_keyed_state((id.clone(), "sheet"), cx, |_, _| Sheet::default());
     let editor = window.use_keyed_state((id.clone(), "editor"), cx, |_, _| Editing::default());
-    sheet.update(cx, |sheet, _| sheet.rows = face.rows);
+    let shrunk = sheet.update(cx, |sheet, _| {
+        sheet.rows = face.rows;
+        fit(sheet, (face.rows, face.cols))
+    });
+    if shrunk {
+        editor.update(cx, |editing, cx| editing.finish(true, window, cx));
+    }
     if sheet.read(cx).editing && editor.read(cx).field().is_none() {
         sheet.update(cx, |sheet, _| sheet.editing = false);
         window.focus(&focus);
@@ -104,25 +83,7 @@ pub(crate) fn grid(face: Face, base: Div, window: &mut Window, cx: &mut App) ->
     let theme = cx.theme();
     let colors = theme.colors.clone();
     let rem = window.rem_size();
-    let metrics = Metrics {
+    let metrics = Metrics::new(&face, window, cx);
-        wide: theme.grid_column().to_pixels(rem),
-        tall: theme
-            .table_row(crate::theme::Density::Compact)
-            .to_pixels(rem),
-        label: if face.numbered {
-            theme.control_height(ControlSize::Lg).to_pixels(rem) * 1.25
-        } else {
-            Pixels::ZERO
-        },
-        frozen: Point::default(),
-    };
-    let metrics = Metrics {
-        frozen: point(
-            metrics.wide * face.frozen.1 as f32,
-            metrics.tall * face.frozen.0 as f32,
-        ),
-        ..metrics
-    };
     let (rows, cols) = (face.rows, face.cols);
     let (offset, bounds, editing, fill_to) = {
         let sheet = sheet.read(cx);
@@ -348,6 +309,12 @@ pub(crate) fn grid(face: Face, base: Div, window: &mut Window, cx: &mut App) ->
     let (raw, on_change, begin_with) = (face.raw.clone(), face.on_change.clone(), editor.clone());
     let (fill_raw, fill_hears) = (face.raw.clone(), face.on_change.clone());
     let numbered = face.numbered;
+    let (merges, frozen) = (Rc::new(face.merges.clone()), face.frozen);
+    let (hovered_merges, stepped, stepped_back) = (
+        merges.clone(),
+        (sheet.clone(), editor.clone()),
+        (sheet.clone(), editor.clone()),
+    );
     let keyed = keys(face, metrics, sheet.clone(), editor.clone());
     let focus_on_press = focus.clone();
     let text_size = theme.text_size(TextSize::Sm);
@@ -361,12 +328,33 @@ pub(crate) fn grid(face: Face, base: Div, window: &mut Window, cx: &mut App) ->
         .border_1()
         .border_color(colors.border)
         .on_key_down(keyed)
+        .capture_action(move |_: &FocusNext, window, cx| {
+            across(
+                &stepped.0,
+                &stepped.1,
+                (face_cols, false),
+                (metrics, frozen),
+                window,
+                cx,
+            )
+        })
+        .capture_action(move |_: &FocusPrev, window, cx| {
+            across(
+                &stepped_back.0,
+                &stepped_back.1,
+                (face_cols, true),
+                (metrics, frozen),
+                window,
+                cx,
+            )
+        })
         .on_mouse_down(MouseButton::Left, move |event, window, cx| {
             let at = event.position - pressed.read(cx).bounds.origin;
             let Some(cell) = metrics.cell_at(at, pressed.read(cx).offset, (face_rows, face_cols))
             else {
                 return;
             };
+            let cell = land(&merges, cell);
             window.focus(&focus_on_press);
             pressed.update(cx, |sheet, cx| {
                 sheet.cursor = cell;
@@ -394,6 +382,7 @@ pub(crate) fn grid(face: Face, base: Div, window: &mut Window, cx: &mut App) ->
             }
             let at = event.position - moved.read(cx).bounds.origin;
             if let Some(cell) = metrics.cell_at(at, moved.read(cx).offset, (face_rows, face_cols)) {
+                let cell = land(&hovered_merges, cell);
                 moved.update(cx, |sheet, cx| {
                     if sheet.cursor != cell && !sheet.editing {
                         sheet.cursor = cell;

src/tables/gridkeys.rs

@@ -1,12 +1,115 @@
 use std::rc::Rc;
 
-use gpui::{App, ClipboardItem, Entity, KeyDownEvent, Pixels, SharedString, Window};
+use gpui::{App, ClipboardItem, Entity, KeyDownEvent, Pixels, Point, SharedString, Window, point};
 
 use super::{
     formula::filled,
-    grid::{Face, Metrics, OnCells, Sheet, Text, range},
+    grid::{Face, Merge, OnCells, Sheet, Text, range},
 };
-use crate::forms::Editing;
+use crate::{
+    forms::Editing,
+    theme::{ActiveTheme, ControlSize, Density},
+};
+
+/// Sizes in pixels: a cell, a header, the row labels, and the frozen stretch.
+#[derive(Clone, Copy)]
+pub(crate) struct Metrics {
+    pub wide: Pixels,
+    pub tall: Pixels,
+    pub label: Pixels,
+    pub frozen: Point<Pixels>,
+}
+
+impl Metrics {
+    /// A face's sizes at the window's rem.
+    pub(crate) fn new(face: &Face, window: &Window, cx: &App) -> Self {
+        let theme = cx.theme();
+        let rem = window.rem_size();
+        let (wide, tall) = (
+            theme.grid_column().to_pixels(rem),
+            theme.table_row(Density::Compact).to_pixels(rem),
+        );
+        Self {
+            wide,
+            tall,
+            label: if face.numbered {
+                theme.control_height(ControlSize::Lg).to_pixels(rem) * 1.25
+            } else {
+                Pixels::ZERO
+            },
+            frozen: point(wide * face.frozen.1 as f32, tall * face.frozen.0 as f32),
+        }
+    }
+
+    /// The cell under a point within the grid, if any.
+    pub(crate) fn cell_at(
+        &self,
+        at: Point<Pixels>,
+        offset: Point<Pixels>,
+        face: (usize, usize),
+    ) -> Option<(usize, usize)> {
+        let (x, y) = (at.x - self.label, at.y - self.tall);
+        if x < Pixels::ZERO || y < Pixels::ZERO {
+            return None;
+        }
+        let x = if x < self.frozen.x { x } else { x + offset.x };
+        let y = if y < self.frozen.y { y } else { y + offset.y };
+        let (col, row) = ((x / self.wide) as usize, (y / self.tall) as usize);
+        (row < face.0 && col < face.1).then_some((row, col))
+    }
+}
+
+/// The cell a place stands for: a merge's first cell for any cell it covers.
+pub(crate) fn land(merges: &[Merge], (row, col): (usize, usize)) -> (usize, usize) {
+    merges
+        .iter()
+        .find(|merge| {
+            (merge.from.0..=merge.to.0).contains(&row) && (merge.from.1..=merge.to.1).contains(&col)
+        })
+        .map_or((row, col), |merge| merge.from)
+}
+
+/// Pulls the cursor, its anchor and a fill back inside a grid of `size`; whether anything moved.
+pub(crate) fn fit(sheet: &mut Sheet, size: (usize, usize)) -> bool {
+    let last = (size.0.saturating_sub(1), size.1.saturating_sub(1));
+    let inside = |(row, col): (usize, usize)| (row.min(last.0), col.min(last.1));
+    let (cursor, anchor) = (inside(sheet.cursor), inside(sheet.anchor));
+    let moved = (cursor, anchor) != (sheet.cursor, sheet.anchor);
+    if moved {
+        log::info!("grid: now {size:?}; the cursor moved to {cursor:?}");
+        (sheet.cursor, sheet.anchor, sheet.fill) = (cursor, anchor, None);
+    }
+    moved
+}
+
+/// Tab and Shift-Tab: keep an open edit, then step across, staying inside the grid.
+pub(crate) fn across(
+    sheet: &Entity<Sheet>,
+    editor: &Entity<Editing>,
+    (cols, back): (usize, bool),
+    (metrics, frozen): (Metrics, (usize, usize)),
+    window: &mut Window,
+    cx: &mut App,
+) {
+    cx.stop_propagation();
+    let (row, col) = sheet.read(cx).cursor;
+    if sheet.read(cx).editing {
+        editor.update(cx, |editing, cx| editing.finish(false, window, cx));
+    }
+    let to = (
+        row,
+        if back {
+            col.saturating_sub(1)
+        } else {
+            (col + 1).min(cols.saturating_sub(1))
+        },
+    );
+    sheet.update(cx, |sheet, cx| {
+        (sheet.cursor, sheet.anchor) = (to, to);
+        reveal(sheet, metrics, frozen);
+        cx.notify();
+    });
+}
 
 /// Scrolls just enough that the cursor shows, past the frozen rows and columns.
 pub(crate) fn reveal(sheet: &mut Sheet, metrics: Metrics, frozen: (usize, usize)) {
@@ -100,7 +203,7 @@ pub(crate) fn pasted(
         .collect()
 }
 
-/// The key handler: arrows walk and Shift stretches the range, Tab and Enter step, typing or F2 edits, Delete clears, Cmd-C and Cmd-V copy and paste.
+/// The key handler: arrows walk, stepping over merges, and Shift stretches the range; Enter steps down, typing or F2 edits, Delete clears, Cmd-C and Cmd-V copy and paste.
 pub(crate) fn keys(
     face: Face,
     metrics: Metrics,
@@ -118,20 +221,23 @@ pub(crate) fn keys(
         let stroke = &event.keystroke;
         let held = &stroke.modifiers;
         let (row, col) = sheet.read(cx).cursor;
+        let far = face
+            .merges
+            .iter()
+            .find(|merge| merge.from == (row, col))
+            .map_or((row, col), |merge| merge.to);
         let to = match stroke.key.as_str() {
             "up" => Some((row.saturating_sub(1), col)),
-            "down" | "enter" => Some(((row + 1).min(rows - 1), col)),
+            "down" | "enter" => Some(((far.0 + 1).min(rows - 1), col)),
             "left" => Some((row, col.saturating_sub(1))),
-            "right" => Some((row, (col + 1).min(cols - 1))),
+            "right" => Some((row, (far.1 + 1).min(cols - 1))),
-            "tab" if held.shift => Some((row, col.saturating_sub(1))),
-            "tab" => Some((row, (col + 1).min(cols - 1))),
             "home" => Some((row, 0)),
             "end" => Some((row, cols - 1)),
             _ => None,
         };
-        if let Some(to) = to {
+        if let Some(to) = to.map(|to| land(&face.merges, to)) {
             cx.stop_propagation();
-            let stretch = held.shift && !matches!(stroke.key.as_str(), "tab" | "enter");
+            let stretch = held.shift && stroke.key != "enter";
             sheet.update(cx, |sheet, cx| {
                 sheet.cursor = to;
                 if !stretch {

src/tables/rules.rs

@@ -112,15 +112,17 @@ pub(crate) fn kept(
         .collect()
 }
 
-/// `order` sorted by each key in turn, the first deciding most; numbers by value, the rest by their words.
+/// `order` sorted by each key in turn, the first deciding most; numbers by value and before the rest, the rest by their words.
 pub(crate) fn sorted_by(rows: &[Row], mut order: Vec<usize>, keys: &[(usize, bool)]) -> Vec<usize> {
     order.sort_by(|a, b| {
         keys.iter().fold(Ordering::Equal, |so_far, (col, rising)| {
             so_far.then_with(|| {
                 let (a, b) = (&rows[*a].cells[*col], &rows[*b].cells[*col]);
                 let by = match (a.number(), b.number()) {
-                    (Some(a), Some(b)) => a.partial_cmp(&b).unwrap_or(Ordering::Equal),
+                    (Some(a), Some(b)) => a.total_cmp(&b),
-                    _ => a.words().to_lowercase().cmp(&b.words().to_lowercase()),
+                    (Some(_), None) => Ordering::Less,
+                    (None, Some(_)) => Ordering::Greater,
+                    (None, None) => a.words().to_lowercase().cmp(&b.words().to_lowercase()),
                 };
                 if *rising { by } else { by.reverse() }
             })
@@ -273,6 +275,44 @@ mod tests {
         );
     }
 
+    #[test]
+    fn numbers_sort_before_words_in_a_mixed_column() {
+        let cells: [Cell; 6] = [
+            "11".into(),
+            10.0.into(),
+            "apple".into(),
+            2.0.into(),
+            Cell::Empty,
+            "Banana".into(),
+        ];
+        let rows: Vec<Row> = cells
+            .into_iter()
+            .enumerate()
+            .map(|(ix, cell)| Row::new(ix.to_string(), [cell]))
+            .collect();
+        let rising = sorted_by(&rows, (0..rows.len()).collect(), &[(0, true)]);
+        assert_eq!(
+            rising,
+            [3, 1, 4, 0, 2, 5],
+            "2, 10, then the words: empty, 11, apple, banana"
+        );
+        let many: Vec<Row> = (0..64)
+            .map(|ix| {
+                Row::new(
+                    ix.to_string(),
+                    [if ix % 3 == 0 {
+                        Cell::from(format!("{ix}"))
+                    } else {
+                        Cell::from(ix as f64)
+                    }],
+                )
+            })
+            .collect();
+        let order = sorted_by(&many, (0..64).rev().collect(), &[(0, true)]);
+        let numbers = order.iter().take_while(|ix| *ix % 3 != 0).count();
+        assert_eq!(numbers, 42, "every number comes before every word");
+    }
+
     #[test]
     fn csv_quotes_what_needs_it() {
         let columns = vec![Column::new("a", "Name"), Column::new("b", "Note")];

src/tables/tests.rs

@@ -1,12 +1,19 @@
-use std::{cell::RefCell, rc::Rc};
+use std::{
-
+    cell::{Cell as Count, RefCell},
-use gpui::{
+    collections::HashMap,
-    Context, IntoElement, Modifiers, ParentElement, Render, SharedString, Styled, TestAppContext,
+    rc::Rc,
-    VisualTestContext, Window, point, px,
 };
 
-use super::{Column, DataTable, Row};
+use gpui::{
-use crate::theme::Theme;
+    Context, FocusHandle, IntoElement, KeyBinding, Modifiers, ParentElement, Render, SharedString,
+    Styled, TestAppContext, VisualTestContext, Window, div, point, px,
+};
+
+use super::{Column, DataTable, FilterBuilder, FilterRule, Row};
+use crate::{
+    primitives::{FocusNext, FocusPrev, FocusScope},
+    theme::Theme,
+};
 
 /// Five selectable rows, 36 tall under a 36 tall header; it keeps what the table reports.
 struct Picks(Rc<RefCell<Vec<SharedString>>>);
@@ -188,6 +195,12 @@ struct Grid(Rc<RefCell<Vec<(usize, usize, String)>>>);
 
 impl Render for Grid {
     fn render(&mut self, _: &mut Window, _: &mut Context<Self>) -> impl IntoElement {
+        self.render_grid()
+    }
+}
+
+impl Grid {
+    fn render_grid(&self) -> impl IntoElement + use<> {
         let heard = self.0.clone();
         let rows: Vec<Vec<SharedString>> = (0..3)
             .map(|row| vec![format!("a{row}").into(), format!("b{row}").into()])
@@ -230,3 +243,186 @@ fn typing_edits_enter_keeps_and_backspace_clears_a_range(cx: &mut TestAppContext
         "enter moved down; the range clears"
     );
 }
+
+/// Filter rules the owner keeps, 600 across, rebuilt from what the builder reports.
+struct Filters(Rc<RefCell<Vec<FilterRule>>>);
+
+impl Render for Filters {
+    fn render(&mut self, _: &mut Window, cx: &mut Context<Self>) -> impl IntoElement {
+        let (rules, store, view) = (self.0.borrow().clone(), self.0.clone(), cx.entity());
+        div().w(px(600.0)).child(
+            FilterBuilder::new("filters", [("name", "Name"), ("region", "Region")])
+                .rules(rules, false)
+                .on_change(move |rules, _, _, cx| {
+                    *store.borrow_mut() = rules.to_vec();
+                    view.update(cx, |_, cx| cx.notify());
+                }),
+        )
+    }
+}
+
+#[gpui::test]
+fn removing_the_rule_being_typed_leaves_the_next_rule_its_own_value(cx: &mut TestAppContext) {
+    cx.update(|cx| {
+        Theme::init(cx);
+        crate::forms::bind_keys(cx);
+    });
+    let rule = |column: &'static str, value: &'static str| FilterRule {
+        column: column.into(),
+        value: value.into(),
+        ..FilterRule::default()
+    };
+    let rules = Rc::new(RefCell::new(vec![
+        rule("name", "one"),
+        rule("region", "keep"),
+    ]));
+    let seen = rules.clone();
+    let (_, cx) = cx.add_window_view(|_, _| Filters(seen));
+    settle(cx);
+    cx.simulate_click(point(px(330.0), px(12.0)), Modifiers::none());
+    settle(cx);
+    cx.simulate_input("typed");
+    settle(cx);
+    assert_eq!(
+        rules.borrow()[0].value.as_ref(),
+        "onetyped",
+        "the first field took the typing"
+    );
+    cx.simulate_click(point(px(588.0), px(12.0)), Modifiers::none());
+    settle(cx);
+    cx.simulate_input("X");
+    settle(cx);
+    assert_eq!(
+        *rules.borrow(),
+        [rule("region", "keep")],
+        "the removed rule's draft went with it"
+    );
+}
+
+/// A two-column grid whose row count the test sets.
+struct Shrinking(Rc<Count<usize>>);
+
+impl Render for Shrinking {
+    fn render(&mut self, _: &mut Window, _: &mut Context<Self>) -> impl IntoElement {
+        let rows: Vec<Vec<SharedString>> = (0..self.0.get())
+            .map(|row| vec![format!("a{row}").into(), format!("b{row}").into()])
+            .collect();
+        super::DataGrid::new("shrink", ["A", "B"], rows)
+            .w(px(300.0))
+            .h(px(200.0))
+    }
+}
+
+fn grid_keys(cx: &mut TestAppContext) {
+    cx.update(|cx| {
+        Theme::init(cx);
+        crate::forms::bind_keys(cx);
+        cx.bind_keys([
+            KeyBinding::new("tab", FocusNext, None),
+            KeyBinding::new("shift-tab", FocusPrev, None),
+        ]);
+    });
+}
+
+#[gpui::test]
+fn a_grid_that_shrinks_keeps_its_cursor_inside(cx: &mut TestAppContext) {
+    grid_keys(cx);
+    let count = Rc::new(Count::new(3));
+    let (view, cx) = cx.add_window_view(|_, _| Shrinking(count.clone()));
+    settle(cx);
+    cx.simulate_click(point(px(146.0), px(98.0)), Modifiers::none());
+    settle(cx);
+    count.set(1);
+    view.update(cx, |_, cx| cx.notify());
+    settle(cx);
+    cx.simulate_keystrokes("cmd-c");
+    settle(cx);
+    let copied = cx.update(|_, cx| cx.read_from_clipboard().and_then(|item| item.text()));
+    assert_eq!(
+        copied.as_deref(),
+        Some("b0"),
+        "the cursor moved to the last row left"
+    );
+}
+
+/// A sheet with A1:C1 merged, 4 across, that keeps every edit it hears.
+struct Merged(Rc<RefCell<Vec<(usize, usize, String)>>>);
+
+impl Render for Merged {
+    fn render(&mut self, _: &mut Window, _: &mut Context<Self>) -> impl IntoElement {
+        let heard = self.0.clone();
+        let cells: HashMap<(usize, usize), SharedString> =
+            [((0, 0), "Title".into())].into_iter().collect();
+        super::Spreadsheet::new("merged", 3, 4)
+            .cells(cells)
+            .merge((0, 0), (0, 2))
+            .on_change(move |edits, _, _| {
+                heard.borrow_mut().extend(
+                    edits
+                        .iter()
+                        .map(|(row, col, text)| (*row, *col, text.to_string())),
+                )
+            })
+            .w(px(500.0))
+            .h(px(200.0))
+    }
+}
+
+#[gpui::test]
+fn a_merge_answers_for_every_cell_it_covers(cx: &mut TestAppContext) {
+    grid_keys(cx);
+    let heard = Rc::new(RefCell::new(Vec::new()));
+    let seen = heard.clone();
+    let (_, cx) = cx.add_window_view(|_, _| Merged(seen));
+    settle(cx);
+    cx.simulate_click(point(px(146.0), px(42.0)), Modifiers::none());
+    settle(cx);
+    cx.simulate_keystrokes("7 enter");
+    settle(cx);
+    cx.simulate_keystrokes("up right 8 enter");
+    settle(cx);
+    assert_eq!(
+        *heard.borrow(),
+        [(0, 0, "7".to_string()), (0, 3, "8".to_string())],
+        "a press inside lands on the merge; right steps past it"
+    );
+}
+
+/// A grid inside a focus scope, as apps hold it, that keeps every edit it hears.
+struct Scoped(FocusHandle, Rc<RefCell<Vec<(usize, usize, String)>>>);
+
+impl Render for Scoped {
+    fn render(&mut self, _: &mut Window, _: &mut Context<Self>) -> impl IntoElement {
+        FocusScope::new(&self.0)
+            .size_full()
+            .child(Grid(self.1.clone()).render_grid())
+    }
+}
+
+#[gpui::test]
+fn tab_steps_across_the_grid_and_keeps_an_edit(cx: &mut TestAppContext) {
+    grid_keys(cx);
+    let heard = Rc::new(RefCell::new(Vec::new()));
+    let seen = heard.clone();
+    let (_, cx) = cx.add_window_view(|_, cx| Scoped(cx.focus_handle(), seen));
+    settle(cx);
+    cx.simulate_click(point(px(48.0), px(42.0)), Modifiers::none());
+    settle(cx);
+    cx.simulate_keystrokes("tab 7 enter");
+    settle(cx);
+    cx.simulate_click(point(px(48.0), px(70.0)), Modifiers::none());
+    settle(cx);
+    cx.simulate_keystrokes("5 tab");
+    settle(cx);
+    cx.simulate_keystrokes("6 enter shift-tab");
+    settle(cx);
+    assert_eq!(
+        *heard.borrow(),
+        [
+            (0, 1, "7".to_string()),
+            (1, 0, "5".to_string()),
+            (1, 1, "6".to_string())
+        ],
+        "tab moves right, and keeps an open edit first"
+    );
+}