Small table footer and hline placement improvements (#3659)

1 files changed, 136 insertions, 28 deletions

diff --git a/crates/typst/src/layout/grid/layout.rs b/crates/typst/src/layout/grid/layout.rs
index 46993989..6dbe151e 100644
--- a/crates/typst/src/layout/grid/layout.rs
+++ b/crates/typst/src/layout/grid/layout.rs
@@ -427,7 +427,10 @@ impl CellGrid {
         // validity, since the amount of rows isn't known until all items were
         // analyzed in the for loop below.
         // We keep their spans so we can report errors later.
-        let mut pending_hlines: Vec<(Span, Line)> = vec![];
+        // The additional boolean indicates whether the hline had an automatic
+        // 'y' index, and is used to change the index of hlines at the top of a
+        // header or footer.
+        let mut pending_hlines: Vec<(Span, Line, bool)> = vec![];
 
         // For consistency, only push vertical lines later as well.
         let mut pending_vlines: Vec<(Span, Line)> = vec![];
@@ -494,7 +497,9 @@ impl CellGrid {
             let mut child_start = usize::MAX;
             let mut child_end = 0;
             let mut child_span = Span::detached();
-            let mut min_auto_index = 0;
+            let mut start_new_row = false;
+            let mut first_index_of_top_hlines = usize::MAX;
+            let mut first_index_of_non_top_hlines = usize::MAX;
 
             let (header_footer_items, simple_item) = match child {
                 ResolvableGridChild::Header { repeat, span, items, .. } => {
@@ -512,7 +517,11 @@ impl CellGrid {
                     // that row instead of starting a new one.
                     // FIXME: Revise this approach when headers can start from
                     // arbitrary rows.
-                    min_auto_index = auto_index.next_multiple_of(c);
+                    start_new_row = true;
+
+                    // Any hlines at the top of the header will start at this
+                    // index.
+                    first_index_of_top_hlines = pending_hlines.len();
 
                     (Some(items), None)
                 }
@@ -529,7 +538,11 @@ impl CellGrid {
                     // have it skip to the next row. This is to avoid having a
                     // footer after a partially filled row just add cells to
                     // that row instead of starting a new one.
-                    min_auto_index = auto_index.next_multiple_of(c);
+                    start_new_row = true;
+
+                    // Any hlines at the top of the footer will start at this
+                    // index.
+                    first_index_of_top_hlines = pending_hlines.len();
 
                     (Some(items), None)
                 }
@@ -550,7 +563,18 @@ impl CellGrid {
                         span,
                         position,
                     } => {
+                        let has_auto_y = y.is_auto();
                         let y = y.unwrap_or_else(|| {
+                            // Avoid placing the hline inside consecutive
+                            // rowspans occupying all columns, as it'd just
+                            // disappear, at least when there's no column
+                            // gutter.
+                            skip_auto_index_through_fully_merged_rows(
+                                &resolved_cells,
+                                &mut auto_index,
+                                c,
+                            );
+
                             // When no 'y' is specified for the hline, we place
                             // it under the latest automatically positioned
                             // cell.
@@ -572,7 +596,6 @@ impl CellGrid {
                             // the start of a header will always appear above
                             // that header's first row. Similarly for footers.
                             auto_index
-                                .max(min_auto_index)
                                 .checked_sub(1)
                                 .map_or(0, |last_auto_index| last_auto_index / c + 1)
                         });
@@ -594,7 +617,7 @@ impl CellGrid {
                         // one "wins" in case of conflict. Pushing the current
                         // hline before we push pending hlines later would
                         // change their order!
-                        pending_hlines.push((span, line));
+                        pending_hlines.push((span, line, has_auto_y));
                         continue;
                     }
                     ResolvableGridItem::VLine {
@@ -620,17 +643,15 @@ impl CellGrid {
                             // to the left of the table.
                             //
                             // Exceptionally, a vline is also placed to the
-                            // left of the table if the current auto index from
-                            // past iterations is smaller than the minimum auto
-                            // index. For example, this means that a vline at
+                            // left of the table if we should start a new row
+                            // for the next automatically positioned cell.
+                            // For example, this means that a vline at
                             // the beginning of a header will be placed to its
                             // left rather than after the previous
                             // automatically positioned cell. Same for footers.
                             auto_index
                                 .checked_sub(1)
-                                .filter(|last_auto_index| {
-                                    last_auto_index >= &min_auto_index
-                                })
+                                .filter(|_| !start_new_row)
                                 .map_or(0, |last_auto_index| last_auto_index % c + 1)
                         });
                         if end.is_some_and(|end| end.get() < start) {
@@ -661,7 +682,7 @@ impl CellGrid {
                         rowspan,
                         &resolved_cells,
                         &mut auto_index,
-                        min_auto_index,
+                        &mut start_new_row,
                         c,
                     )
                     .at(cell_span)?
@@ -778,12 +799,28 @@ impl CellGrid {
                     // contained within it.
                     child_start = child_start.min(y);
                     child_end = child_end.max(y + rowspan);
+
+                    if start_new_row && child_start <= auto_index.div_ceil(c) {
+                        // No need to start a new row as we already include
+                        // the row of the next automatically positioned cell in
+                        // the header or footer.
+                        start_new_row = false;
+                    }
+
+                    if !start_new_row {
+                        // From now on, upcoming hlines won't be at the top of
+                        // the child, as the first automatically positioned
+                        // cell was placed.
+                        first_index_of_non_top_hlines =
+                            first_index_of_non_top_hlines.min(pending_hlines.len());
+                    }
                 }
             }
 
             if (is_header || is_footer) && child_start == usize::MAX {
                 // Empty header/footer: consider the header/footer to be
-                // one row after the latest auto index.
+                // at the next empty row after the latest auto index.
+                auto_index = find_next_empty_row(&resolved_cells, auto_index, c);
                 child_start = auto_index.div_ceil(c);
                 child_end = child_start + 1;
 
@@ -830,6 +867,22 @@ impl CellGrid {
             }
 
             if is_header || is_footer {
+                let amount_hlines = pending_hlines.len();
+                for (_, top_hline, has_auto_y) in pending_hlines
+                    .get_mut(
+                        first_index_of_top_hlines
+                            ..first_index_of_non_top_hlines.min(amount_hlines),
+                    )
+                    .unwrap_or(&mut [])
+                {
+                    if *has_auto_y {
+                        // Move this hline to the top of the child, as it was
+                        // placed before the first automatically positioned cell
+                        // and had an automatic index.
+                        top_hline.index = child_start;
+                    }
+                }
+
                 // Next automatically positioned cell goes under this header.
                 // FIXME: Consider only doing this if the header has any fully
                 // automatically positioned cells. Otherwise,
@@ -944,7 +997,7 @@ impl CellGrid {
         let mut hlines: Vec<Vec<Line>> = vec![];
         let row_amount = resolved_cells.len().div_ceil(c);
 
-        for (line_span, line) in pending_hlines {
+        for (line_span, line, _) in pending_hlines {
             let y = line.index;
             if y > row_amount {
                 bail!(line_span, "cannot place horizontal line at invalid row {y}");
@@ -1293,11 +1346,10 @@ impl CellGrid {
 /// `(auto, auto)` cell) and the amount of columns in the grid, returns the
 /// final index of this cell in the vector of resolved cells.
 ///
-/// The `min_auto_index` parameter is used to bump the auto index to that value
-/// if it is currently smaller than it and a cell requests fully automatic
-/// positioning. Useful with headers: if a cell in a header has automatic
-/// positioning, it should start at the header's first row, and not at the end
-/// of the previous row.
+/// The `start_new_row` parameter is used to ensure that, if this cell is
+/// fully automatically positioned, it should start a new, empty row. This is
+/// useful for headers and footers, which must start at their own rows, without
+/// interference from previous cells.
 #[allow(clippy::too_many_arguments)]
 fn resolve_cell_position(
     cell_x: Smart<usize>,
@@ -1306,7 +1358,7 @@ fn resolve_cell_position(
     rowspan: usize,
     resolved_cells: &[Option<Entry>],
     auto_index: &mut usize,
-    min_auto_index: usize,
+    start_new_row: &mut bool,
     columns: usize,
 ) -> HintedStrResult<usize> {
     // Translates a (x, y) position to the equivalent index in the final cell vector.
@@ -1322,13 +1374,22 @@ fn resolve_cell_position(
         (Smart::Auto, Smart::Auto) => {
             // Let's find the first available position starting from the
             // automatic position counter, searching in row-major order.
-            let mut resolved_index = min_auto_index.max(*auto_index);
-            while let Some(Some(_)) = resolved_cells.get(resolved_index) {
-                // Skip any non-absent cell positions (`Some(None)`) to
-                // determine where this cell will be placed. An out of bounds
-                // position (thus `None`) is also a valid new position (only
-                // requires expanding the vector).
-                resolved_index += 1;
+            let mut resolved_index = *auto_index;
+            if *start_new_row {
+                resolved_index =
+                    find_next_empty_row(resolved_cells, resolved_index, columns);
+
+                // Next cell won't have to start a new row if we just did that,
+                // in principle.
+                *start_new_row = false;
+            } else {
+                while let Some(Some(_)) = resolved_cells.get(resolved_index) {
+                    // Skip any non-absent cell positions (`Some(None)`) to
+                    // determine where this cell will be placed. An out of
+                    // bounds position (thus `None`) is also a valid new
+                    // position (only requires expanding the vector).
+                    resolved_index += 1;
+                }
             }
 
             // Ensure the next cell with automatic position will be
@@ -1401,6 +1462,53 @@ fn resolve_cell_position(
     }
 }
 
+/// Computes the index of the first cell in the next empty row in the grid,
+/// starting with the given initial index.
+fn find_next_empty_row(
+    resolved_cells: &[Option<Entry>],
+    initial_index: usize,
+    columns: usize,
+) -> usize {
+    let mut resolved_index = initial_index.next_multiple_of(columns);
+    while resolved_cells
+        .get(resolved_index..resolved_index + columns)
+        .is_some_and(|row| row.iter().any(Option::is_some))
+    {
+        // Skip non-empty rows.
+        resolved_index += columns;
+    }
+
+    resolved_index
+}
+
+/// Fully merged rows under the cell of latest auto index indicate rowspans
+/// occupying all columns, so we skip the auto index until the shortest rowspan
+/// ends, such that, in the resulting row, we will be able to place an
+/// automatically positioned cell - and, in particular, hlines under it. The
+/// idea is that an auto hline will be placed after the shortest such rowspan.
+/// Otherwise, the hline would just be placed under the first row of those
+/// rowspans and disappear (except at the presence of column gutter).
+fn skip_auto_index_through_fully_merged_rows(
+    resolved_cells: &[Option<Entry>],
+    auto_index: &mut usize,
+    columns: usize,
+) {
+    // If the auto index isn't currently at the start of a row, that means
+    // there's still at least one auto position left in the row, ignoring
+    // cells with manual positions, so we wouldn't have a problem in placing
+    // further cells or, in this case, hlines here.
+    if *auto_index % columns == 0 {
+        while resolved_cells
+            .get(*auto_index..*auto_index + columns)
+            .is_some_and(|row| {
+                row.iter().all(|entry| matches!(entry, Some(Entry::Merged { .. })))
+            })
+        {
+            *auto_index += columns;
+        }
+    }
+}
+
 /// Performs grid layout.
 pub struct GridLayouter<'a> {
     /// The grid of cells.