change some ticks for consistency

They weren't doing much for us

crates/typst-library/src/layout/grid/resolve.rs

@@ -781,19 +781,21 @@ impl<'a> CellGrid<'a> {
             // instead, and use a separate counter outside them.
             let mut local_auto_index = auto_index;
 
-            // The range of rows of cells inside this child in the grid. The
+            // The range of rows of cells inside this grid row group. The
             // first and last rows are guaranteed to have cells (an exception
-            // is made when there is gutter, in which case the child range may
+            // is made when there is gutter, in which case the group range may
             // be expanded to include an additional gutter row when there is a
-            // repeatable header or footer).
+            // repeatable header or footer). This is 'None' until the first
+            // cell of the row group is placed, then it is continually adjusted
+            // to fit the cells inside the row group.
             //
-            // Note that cells outside headers and footers are children
+            // Note that cells outside headers and footers are grid children
-            // with a single cell inside, in which case the range has a single
+            // with a single cell inside, and thus not considered row groups,
-            // row.
+            // in which case this variable remains 'None'.
-            let mut child_range: Option<Range<usize>> = None;
+            let mut group_range: Option<Range<usize>> = None;
-            let mut child_span = Span::detached();
+            let mut group_span = Span::detached();
 
-            // The first row in which this table child can fit.
+            // The first row in which this table group can fit.
             //
             // Within headers and footers, this will correspond to the first
             // fully empty row available in the grid. This is because headers
@@ -809,7 +811,7 @@ impl<'a> CellGrid<'a> {
 
                     is_header = true;
                     is_row_group = true;
-                    child_span = span;
+                    group_span = span;
                     repeat_header = repeat;
 
                     first_available_row =
@@ -835,21 +837,12 @@ impl<'a> CellGrid<'a> {
 
                     is_footer = true;
                     is_row_group = true;
-                    child_span = span;
+                    group_span = span;
                     repeat_footer = repeat;
 
                     first_available_row =
                         find_next_empty_row(&resolved_cells, local_auto_index, c);
 
-                    // If any cell in the footer is automatically positioned,
-                    // have it skip to the next empty 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.
-                    //
-                    // Note that the first fully empty row is always after the
-                    // latest auto-position cell, since each auto-position cell
-                    // always occupies the first available position after the
-                    // previous one. Therefore, this will be >= auto_index.
                     local_auto_index = first_available_row * c;
 
                     (Some(items), None)
@@ -1025,6 +1018,22 @@ impl<'a> CellGrid<'a> {
                     )
                 };
 
+                // Cell's header or footer must expand to include the cell's
+                // occupied positions, if possible.
+                if is_row_group {
+                    group_range = Some(
+                        expand_row_group(
+                            &resolved_cells,
+                            group_range.as_ref(),
+                            first_available_row,
+                            y,
+                            rowspan,
+                            c,
+                        )
+                        .at(cell_span)?,
+                    );
+                }
+
                 // Let's resolve the cell so it can determine its own fields
                 // based on its final position.
                 let cell = cell.resolve_cell(
@@ -1039,120 +1048,6 @@ impl<'a> CellGrid<'a> {
                     styles,
                 );
 
-                // Check if the cell's header or footer can expand to the
-                // cell's position before placing it.
-                if is_row_group {
-                    // Ensure each cell in a header or footer is fully
-                    // contained within it by expanding the header or footer
-                    // towards this new cell.
-                    let (new_child_start, new_child_end) =
-                        child_range.clone().map_or((y, y + rowspan), |r| {
-                            (r.start.min(y), r.end.max(y + rowspan))
-                        });
-
-                    // TODO: Maybe remove this check and just keep the loop,
-                    // we will loop in the "good case" anyway
-                    //
-                    // Quickly detect the case:
-                    // y = 0 => occupied
-                    // y = 1 => empty
-                    // y = 2 => header
-                    // and header tries to expand to y = 0 - invalid, as
-                    // `y = 1` is the earliest row it can occupy.
-                    if new_child_start < first_available_row {
-                        bail!(
-                            cell_span,
-                            "cell would cause header or footer to expand to non-empty row {}",
-                            first_available_row.saturating_sub(1);
-                            hint: "try moving its cells to later rows"
-                        );
-                    }
-
-                    // The check above isn't enough, however, even when the
-                    // header is expanding upwards, as it might expand upwards
-                    // towards an occupied row after the first empty row, e.g.
-                    // y = 0 => occupied
-                    // y = 1 => empty (first_available_row = 1)
-                    // y = 2 => occupied
-                    // y = 3 => header
-                    // Here, we should bail if the header tries to expand
-                    // upwards. Note that expanding upwards is only possible
-                    // when row-positioned cells are specified, in one of the
-                    // following cases:
-                    //
-                    // 1. We place e.g. `table.cell(y: 3)` followed by
-                    // `table.cell(y: 2)` (earlier row => upwards);
-                    // 2. We place e.g. `table.cell(y: 3)` followed by
-                    // `[a]` (auto-pos cell favors 'first_available_row', so
-                    // the header tries to expand upwards to place the cell at
-                    // `y = 1`).
-                    //
-                    // Of course, we also need to check for downward expansion
-                    // as there could be a non-empty row below the header, but
-                    // the upward case is highlighted due to its differences.
-                    let new_rows = child_range.clone().map_or(
-                        (new_child_start..new_child_end).chain(0..0),
-                        |r| {
-                            // NOTE: To keep types the same, we have to always
-                            // return (range).chain(range), which justifies
-                            // chaining an empty range above.
-                            (new_child_start..r.start).chain(r.end..new_child_end)
-                        },
-                    );
-
-                    // Note that simply checking for non-empty rows like below
-                    // not only prevents conflicts with top-level cells
-                    // (outside of headers and footers), but also prevents
-                    // conflicts with other headers or footers, since we have
-                    // an invariant that even empty headers and footers must
-                    // contain at least one 'Some(...)' position in
-                    // 'resolved_cells'. More precisely, each header and footer
-                    // has at least one 'Some(...)' cell at 'child_range.start'
-                    // and at 'child_range.end - 1' - non-empty headers and
-                    // footers don't span any unnecessary rows.
-                    for new_y in new_rows {
-                        if let Some(new_row @ [_non_empty, ..]) = resolved_cells
-                            .get(new_y * c..)
-                            .map(|cells| &cells[..c.min(cells.len())])
-                        {
-                            if new_row.iter().any(Option::is_some) {
-                                // TODO:
-                                // - Later/earlier rows might be confusing
-                                // (moving to the end always works...)
-                                // - Detect when header or footer collided with
-                                // another header or footer and provide a
-                                // better error message if so.
-                                if child_range.is_none_or(|r| new_y < r.start) {
-                                    bail!(
-                                        cell_span,
-                                        "cell would cause header or footer to expand to non-empty row {new_y}";
-                                        hint: "try moving its cells to later rows"
-                                    );
-                                } else {
-                                    bail!(
-                                        cell_span,
-                                        "cell would cause header or footer to expand to non-empty row {new_y}";
-                                        hint: "try moving its cells to earlier rows"
-                                    );
-                                }
-                            }
-                        } else {
-                            // Received `None` or an empty slice, so we are
-                            // expanding the header or footer into new rows,
-                            // which is always valid and cannot conflict with
-                            // existing cells. (Note that we only resize
-                            // `resolved_cells` after this check, so, if this
-                            // header or footer is at the bottom of the table
-                            // so far, this loop will end quite early,
-                            // regardless of where this cell was placed or of
-                            // its rowspan value.)
-                            break;
-                        }
-                    }
-
-                    child_range = Some(new_child_start..new_child_end);
-                }
-
                 if largest_index >= resolved_cells.len() {
                     // Ensure the length of the vector of resolved cells is
                     // always a multiple of 'c' by pushing full rows every
@@ -1224,27 +1119,27 @@ impl<'a> CellGrid<'a> {
             }
 
             if is_row_group {
-                let child_range = match child_range {
+                let group_range = match group_range {
-                    Some(child_range) => child_range,
+                    Some(group_range) => group_range,
 
                     None => {
                         // Empty header/footer: consider the header/footer to be
                         // at the next empty row after the latest auto index.
                         local_auto_index = first_available_row * c;
-                        let child_start = first_available_row;
+                        let group_start = first_available_row;
-                        let child_end = child_start + 1;
+                        let group_end = group_start + 1;
 
-                        if resolved_cells.len() <= c * child_start {
+                        if resolved_cells.len() <= c * group_start {
                             // Ensure the automatically chosen row actually exists.
-                            resolved_cells.resize_with(c * (child_start + 1), || None);
+                            resolved_cells.resize_with(c * (group_start + 1), || None);
                         }
 
                         // Even though this header or footer is fully empty, we add one
                         // default cell to maintain the invariant that each header and
-                        // footer has at least one `Some(...)` cell at its first row
+                        // footer has at least one 'Some(...)' cell at its first row
                         // and at least one at its last row (here they are the same
                         // row, of course). This invariant is important to ensure
-                        // `find_next_empty_row` will skip through any existing headers
+                        // 'find_next_empty_row' will skip through any existing headers
                         // and footers without having to loop through them each time.
                         // Cells themselves, unfortunately, still have to.
                         assert!(resolved_cells[local_auto_index].is_none());
@@ -1262,14 +1157,14 @@ impl<'a> CellGrid<'a> {
                                 styles,
                             )));
 
-                        child_start..child_end
+                        group_start..group_end
                     }
                 };
 
                 if is_header {
-                    if child_range.start != 0 {
+                    if group_range.start != 0 {
                         bail!(
-                            child_span,
+                            group_span,
                             "header must start at the first row";
                             hint: "remove any rows before the header"
                         );
@@ -1280,7 +1175,7 @@ impl<'a> CellGrid<'a> {
                         // is gutter. But only once all cells have been analyzed
                         // and the header has fully expanded in the fixup loop
                         // below.
-                        end: child_range.end,
+                        end: group_range.end,
                     });
                 }
 
@@ -1288,8 +1183,8 @@ impl<'a> CellGrid<'a> {
                     // Only check if the footer is at the end later, once we know
                     // the final amount of rows.
                     footer = Some((
-                        child_range.end,
+                        group_range.end,
-                        child_span,
+                        group_span,
                         Footer {
                             // Later on, we have to correct this number in case there
                             // is gutter, but only once all cells have been analyzed
@@ -1297,7 +1192,7 @@ impl<'a> CellGrid<'a> {
                             // known. That is because the gutter row immediately
                             // before the footer might not be included as part of
                             // the footer if it is contained within the header.
-                            start: child_range.start,
+                            start: group_range.start,
                         },
                     ));
                 }
@@ -1713,6 +1608,134 @@ impl<'a> CellGrid<'a> {
     }
 }
 
+/// Given the existing range of a row group (header or footer), tries to expand
+/// it to fit the new cell placed inside it. If the newly-expanded row group
+/// would conflict with existing cells or other row groups, an error is
+/// returned. Otherwise, the new `start..end` range of rows in the row group is
+/// returned.
+fn expand_row_group(
+    resolved_cells: &[Option<Entry<'_>>],
+    group_range: Option<&Range<usize>>,
+    first_available_row: usize,
+    cell_y: usize,
+    rowspan: usize,
+    columns: usize,
+) -> HintedStrResult<Range<usize>> {
+    // Ensure each cell in a header or footer is fully contained within it by
+    // expanding the header or footer towards this new cell.
+    let (new_group_start, new_group_end) = group_range
+        .map_or((cell_y, cell_y + rowspan), |r| {
+            (r.start.min(cell_y), r.end.max(cell_y + rowspan))
+        });
+
+    // This check might be unnecessary with the loop below, but let's keep it
+    // here for full correctness.
+    //
+    // Quickly detect the case:
+    // y = 0 => occupied
+    // y = 1 => empty
+    // y = 2 => header
+    // and header tries to expand to y = 0 - invalid, as
+    // 'y = 1' is the earliest row it can occupy.
+    if new_group_start < first_available_row {
+        bail!(
+            "cell would cause header or footer to expand to non-empty row {}",
+            first_available_row.saturating_sub(1);
+            hint: "try moving its cells to later rows"
+        );
+    }
+
+    let new_rows =
+        group_range.map_or((new_group_start..new_group_end).chain(0..0), |r| {
+            // NOTE: 'r.end' is one row AFTER the row group's last row, so it
+            // makes sense to check it if 'new_group_end > r.end', that is, if
+            // the row group is going to expand. It is NOT a duplicate check,
+            // as we hadn't checked it before (in a previous run, it was
+            // 'new_group_end' at the exclusive end of the range)!
+            //
+            // NOTE: To keep types the same, we have to always return
+            // '(range).chain(range)', which justifies chaining an empty
+            // range above.
+            (new_group_start..r.start).chain(r.end..new_group_end)
+        });
+
+    // The check above isn't enough, however, even when the header is expanding
+    // upwards, as it might expand upwards towards an occupied row after the
+    // first empty row, e.g.
+    //
+    // y = 0 => occupied
+    // y = 1 => empty (first_available_row = 1)
+    // y = 2 => occupied
+    // y = 3 => header
+    //
+    // Here, we should bail if the header tries to expand upwards, regardless
+    // of the fact that the conflicting row (y = 2) comes after the first
+    // available row.
+    //
+    // Note that expanding upwards is only possible when row-positioned cells
+    // are specified, in one of the following cases:
+    //
+    // 1. We place e.g. 'table.cell(y: 3)' followed by 'table.cell(y: 2)'
+    // (earlier row => upwards);
+    //
+    // 2. We place e.g. 'table.cell(y: 3)' followed by '[a]' (auto-pos cell
+    // favors 'first_available_row', so the header tries to expand upwards to
+    // place the cell at 'y = 1' and conflicts at 'y = 2') or
+    // 'table.cell(x: 1)' (same deal).
+    //
+    // Of course, we also need to check for downward expansion as usual as
+    // there could be a non-empty row below the header, but the upward case is
+    // highlighted as it was checked separately before (and also to explain
+    // what kind of situation we are preventing with this check).
+    //
+    // Note that simply checking for non-empty rows like below not only
+    // prevents conflicts with top-level cells (outside of headers and
+    // footers), but also prevents conflicts with other headers or footers,
+    // since we have an invariant that even empty headers and footers must
+    // contain at least one 'Some(...)' position in 'resolved_cells'. More
+    // precisely, each header and footer has at least one 'Some(...)' cell at
+    // 'group_range.start' and at 'group_range.end - 1' - non-empty headers and
+    // footers don't span any unnecessary rows. Therefore, we don't have to
+    // loop over headers and footers, only check if the new rows are empty.
+    for new_y in new_rows {
+        if let Some(new_row @ [_non_empty, ..]) = resolved_cells
+            .get(new_y * columns..)
+            .map(|cells| &cells[..columns.min(cells.len())])
+        {
+            if new_row.iter().any(Option::is_some) {
+                // TODO:
+                // - Later/earlier rows might be confusing
+                // (moving to the end always works...)
+                // - Detect when header or footer collided with
+                // another header or footer and provide a
+                // better error message if so.
+                if group_range.is_none_or(|r| new_y < r.start) {
+                    bail!(
+                        "cell would cause header or footer to expand to non-empty row {new_y}";
+                        hint: "try moving its cells to later rows"
+                    );
+                } else {
+                    bail!(
+                        "cell would cause header or footer to expand to non-empty row {new_y}";
+                        hint: "try moving its cells to earlier rows"
+                    );
+                }
+            }
+        } else {
+            // Received 'None' or an empty slice, so we are expanding the
+            // header or footer into new rows, which is always valid and cannot
+            // conflict with existing cells. (Note that we only resize
+            // 'resolved_cells' after this function is called, so, if this
+            // header or footer is at the bottom of the table so far, this loop
+            // will end quite early, regardless of where this cell was placed
+            // or of its rowspan value.)
+            break;
+        }
+    }
+
+    Ok(new_group_start..new_group_end)
+}
+
 /// Check if a cell's fixed row would conflict with a header or footer.
 fn check_for_conflicting_cell_row(
     header: Option<&Header>,
@@ -1783,37 +1806,33 @@ 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.
+            // Note that the counter ignores any cells with fixed positions,
+            // but automatically-positioned cells will avoid conflicts by
+            // simply skipping existing cells, headers and footers.
             let mut resolved_index = *auto_index;
-            if header.is_some() || footer.is_some() {
+
-                // Need to skip existing headers and footers.
+            loop {
-                loop {
+                if let Some(Some(_)) = resolved_cells.get(resolved_index) {
-                    if matches!(resolved_cells.get(resolved_index), Some(Some(_))) {
-                        resolved_index += 1;
-                    } else if let Some(header) =
-                        header.filter(|header| resolved_index / columns < header.end)
-                    {
-                        // Skip header
-                        resolved_index = header.end * columns;
-                    } else if let Some((footer_end, _, _)) =
-                        footer.filter(|(end, _, footer)| {
-                            resolved_index / columns >= footer.start
-                                && resolved_index / columns < *end
-                        })
-                    {
-                        // Skip footer
-                        resolved_index = *footer_end * columns;
-                    } else {
-                        break;
-                    }
-                }
-            } else {
-                // No row groups to skip, so only skip non-empty cells.
-                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;
+                } else if let Some(header) =
+                    header.filter(|header| resolved_index / columns < header.end)
+                {
+                    // Skip header
+                    resolved_index = header.end * columns;
+                } else if let Some((footer_end, _, _)) =
+                    footer.filter(|(end, _, footer)| {
+                        resolved_index / columns >= footer.start
+                            && resolved_index / columns < *end
+                    })
+                {
+                    // Skip footer
+                    resolved_index = *footer_end * columns;
+                } else {
+                    break;
                 }
             }
 
@@ -1821,7 +1840,9 @@ fn resolve_cell_position(
             // placed after this one (maybe not immediately after).
             //
             // The calculation below also affects the position of the upcoming
-            // automatically-positioned lines.
+            // automatically-positioned lines, as they are placed below
+            // (horizontal lines) or to the right (vertical lines) of the cell
+            // that would be placed at 'auto_index'.
             *auto_index = if colspan == columns {
                 // The cell occupies all columns, so no cells can be placed
                 // after it until all of its rows have been spanned.
@@ -1860,45 +1881,47 @@ fn resolve_cell_position(
                 // 'first_available_row'). Otherwise, start searching at the
                 // first row.
                 let mut resolved_y = first_available_row;
-                if header.is_some() || footer.is_some() {
+
-                    // There are row groups, so we have to not only skip
+                // There may be row groups, so we have to not only skip
-                    // rows where the requested column is occupied to find the
+                // rows where the requested column is occupied to find the
-                    // first suitable row, but also skip rows belonging to
+                // first suitable row, but also skip rows belonging to
-                    // headers or footers.
+                // headers or footers.
-                    loop {
+                loop {
-                        if let Some(Some(_)) =
+                    if let Some(Some(_)) =
-                            resolved_cells.get(cell_index(cell_x, resolved_y)?)
-                        {
-                            // Try each row until either we reach an absent position
-                            // (`Some(None)`) or an out of bounds position (`None`),
-                            // in which case we'd create a new row to place this cell in.
-                            resolved_y += 1;
-                        } else if let Some(header) =
-                            header.filter(|header| resolved_y < header.end)
-                        {
-                            // Skip header
-                            resolved_y = header.end;
-                        } else if let Some((footer_end, _, _)) =
-                            footer.filter(|(end, _, footer)| {
-                                resolved_y >= footer.start && resolved_y < *end
-                            })
-                        {
-                            // Skip footer
-                            resolved_y = *footer_end;
-                        } else {
-                            break;
-                        }
-                    }
-                } else {
-                    // No row groups to skip, so only skip a row if the
-                    // requested column is occupied, and find the first row
-                    // where it isn't.
-                    while let Some(Some(_)) =
                         resolved_cells.get(cell_index(cell_x, resolved_y)?)
                     {
+                        // Try each row until either we reach an absent
+                        // position at the requested column (`Some(None)`)
+                        // or an out of bounds position (`None`), in which
+                        // case we'd create a new row to place this cell
+                        // in.
+                        //
+                        // Therefore, this loop will always finish, even if
+                        // the cell has to go all the way to the end to be
+                        // at its requested column. However, if the cell is
+                        // in a header or footer, that could cause the
+                        // header or footer to expand and conflict with
+                        // other cells along the way, but that is checked
+                        // separately later in the 'expand_row_group'
+                        // function.
                         resolved_y += 1;
+                    } else if let Some(header) =
+                        header.filter(|header| resolved_y < header.end)
+                    {
+                        // Skip header
+                        resolved_y = header.end;
+                    } else if let Some((footer_end, _, _)) =
+                        footer.filter(|(end, _, footer)| {
+                            resolved_y >= footer.start && resolved_y < *end
+                        })
+                    {
+                        // Skip footer
+                        resolved_y = *footer_end;
+                    } else {
+                        break;
                     }
                 }
+
                 cell_index(cell_x, resolved_y)
             }
         }

tests/suite/layout/grid/headers.typ

@@ -65,7 +65,6 @@
   columns: 2,
   stroke: black,
   inset: 5pt,
-  // grid.cell(x: 1)[a],
   grid.header(grid.cell(x: 0)[b1], grid.cell(x: 0)[b2]),
   // This should skip the header
   grid.cell(x: 1)[c]