Ensure table headers trigger rowbreak (#6687)

This commit is contained in:
PgBiel 2025-08-07 12:04:21 -03:00 committed by GitHub
parent 0b639c7510
commit 3119ba3104
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
17 changed files with 272 additions and 127 deletions

View File

@ -19,7 +19,7 @@ use typst_library::text::TextElem;
use typst_library::visualize::{Paint, Stroke};
use typst_syntax::Span;
use typst_utils::NonZeroExt;
use typst_utils::{NonZeroExt, SmallBitSet};
use crate::introspection::SplitLocator;
@ -1048,11 +1048,6 @@ impl<'x> CellGridResolver<'_, '_, 'x> {
// automatically-positioned cell.
let mut auto_index: usize = 0;
// The next header after the latest auto-positioned cell. This is used
// to avoid checking for collision with headers that were already
// skipped.
let mut next_header = 0;
// We have to rebuild the grid to account for fixed cell positions.
//
// Create at least 'children.len()' positions, since there could be at
@ -1067,17 +1062,26 @@ impl<'x> CellGridResolver<'_, '_, 'x> {
let Some(child_count) = children.len().checked_next_multiple_of(columns) else {
bail!(self.span, "too many cells or lines were given")
};
// Rows in this bitset are occupied by an existing header.
// This allows for efficiently checking whether a cell would collide
// with a header at a certain row. (For footers, it's easy as there is
// only one.)
//
// TODO(subfooters): how to add a footer here while avoiding
// unnecessary allocations?
let mut header_rows: SmallBitSet = SmallBitSet::new();
let mut resolved_cells: Vec<Option<Entry>> = Vec::with_capacity(child_count);
for child in children {
self.resolve_grid_child(
columns,
&mut pending_hlines,
&mut pending_vlines,
&mut header_rows,
&mut headers,
&mut footer,
&mut repeat_footer,
&mut auto_index,
&mut next_header,
&mut resolved_cells,
&mut at_least_one_cell,
child,
@ -1129,11 +1133,11 @@ impl<'x> CellGridResolver<'_, '_, 'x> {
columns: usize,
pending_hlines: &mut Vec<(Span, Line, bool)>,
pending_vlines: &mut Vec<(Span, Line)>,
header_rows: &mut SmallBitSet,
headers: &mut Vec<Repeatable<Header>>,
footer: &mut Option<(usize, Span, Footer)>,
repeat_footer: &mut bool,
auto_index: &mut usize,
next_header: &mut usize,
resolved_cells: &mut Vec<Option<Entry<'x>>>,
at_least_one_cell: &mut bool,
child: ResolvableGridChild<T, I>,
@ -1149,18 +1153,35 @@ impl<'x> CellGridResolver<'_, '_, 'x> {
// in which case this variable remains 'None'.
let mut row_group_data: Option<RowGroupData> = None;
// The normal auto index should only be stepped (upon placing an
// automatically-positioned cell, to indicate the position of the
// next) outside of headers or footers, in which case the auto
// index will be updated with the local auto index. Inside headers
// and footers, however, cells can only start after the first empty
// row (as determined by 'first_available_row' below), meaning that
// the next automatically-positioned cell will be in a different
// position than it would usually be if it would be in a non-empty
// row, so we must step a local index inside headers and footers
// instead, and use a separate counter outside them.
// Usually, the global auto index is stepped only when a cell with
// fully automatic position (no fixed x/y) is placed, advancing one
// position. In that usual case, 'local_auto_index == auto_index'
// holds, as 'local_auto_index' is what the code below actually
// updates.
//
// However, headers and footers must trigger a rowbreak if the
// previous row isn't empty, given they cannot occupy only part of a
// row. Therefore, the initial auto index used by their auto cells
// should be right below the first empty row.
//
// The problem is that we don't know whether the header will actually
// have an auto cell or not, and we don't want the external auto index
// to change (no rowbreak should be triggered) if the header has no
// auto cells (although a fully empty header does count as having
// auto cells, albeit empty).
//
// So, we use a separate auto index counter inside the header. It starts
// below the first non-empty row. If the header only has fixed-position
// cells, the external counter is unchanged. Otherwise (has auto cells
// or is empty), the external counter is synchronized and moved to
// below the header.
//
// This ensures lines and cells specified below the header in the
// source code also appear below it in the final grid/table.
let local_auto_index = if matches!(child, ResolvableGridChild::Item(_)) {
auto_index
// Re-borrow the original auto index so we can reuse this mutable
// reference later.
&mut *auto_index
} else {
// Although 'usize' is Copy, we need to be explicit here that we
// aren't reborrowing the original auto index but rather making a
@ -1168,24 +1189,6 @@ impl<'x> CellGridResolver<'_, '_, 'x> {
&mut (*auto_index).clone()
};
// NOTE: usually, if 'next_header' were to be updated inside a row
// group (indicating a header was skipped by a cell), that would
// indicate a collision between the row group and that header, which
// is an error. However, the exception is for the first auto cell of
// the row group, which may skip headers while searching for a position
// where to begin the row group in the first place.
//
// Therefore, we cannot safely share the counter in the row group with
// the counter used by auto cells outside, as it might update it in a
// valid situation, whereas it must not, since its auto cells use a
// different auto index counter and will have seen different headers,
// so we copy the next header counter while inside a row group.
let local_next_header = if matches!(child, ResolvableGridChild::Item(_)) {
next_header
} else {
&mut (*next_header).clone()
};
// The first row in which this table group can fit.
//
// Within headers and footers, this will correspond to the first
@ -1253,7 +1256,9 @@ impl<'x> CellGridResolver<'_, '_, 'x> {
}
};
let mut had_auto_cells = false;
let items = header_footer_items.into_iter().flatten().chain(simple_item);
for item in items {
let cell = match item {
ResolvableGridItem::HLine { y, start, end, stroke, span, position } => {
@ -1364,16 +1369,17 @@ impl<'x> CellGridResolver<'_, '_, 'x> {
let resolved_index = {
let cell_x = cell.x(self.styles);
let cell_y = cell.y(self.styles);
had_auto_cells |= cell_x.is_auto() && cell_y.is_auto();
resolve_cell_position(
cell_x,
cell_y,
colspan,
rowspan,
header_rows,
headers,
footer.as_ref(),
resolved_cells,
local_auto_index,
local_next_header,
first_available_row,
columns,
row_group_data.is_some(),
@ -1511,8 +1517,10 @@ impl<'x> CellGridResolver<'_, '_, 'x> {
None => {
// Empty header/footer: consider the header/footer to be
// at the next empty row after the latest auto index.
// automatically positioned at the next empty row after the
// latest auto index.
*local_auto_index = first_available_row * columns;
had_auto_cells = true;
let group_start = first_available_row;
let group_end = group_start + 1;
@ -1556,6 +1564,13 @@ impl<'x> CellGridResolver<'_, '_, 'x> {
}
}
if had_auto_cells {
// Header/footer was automatically positioned (either by having
// auto cells or by virtue of being empty), so trigger a
// rowbreak. Move auto index counter right below it.
*auto_index = group_range.end * columns;
}
match row_group.kind {
RowGroupKind::Header => {
let data = Header {
@ -1563,7 +1578,7 @@ impl<'x> CellGridResolver<'_, '_, 'x> {
// is gutter. But only once all cells have been analyzed
// and the header has fully expanded in the fixup loop
// below.
range: group_range,
range: group_range.clone(),
level: row_group.repeatable_level.get(),
@ -1572,24 +1587,11 @@ impl<'x> CellGridResolver<'_, '_, 'x> {
short_lived: false,
};
// Mark consecutive headers right before this one as short
// lived if they would have a higher or equal level, as
// then they would immediately stop repeating during
// layout.
let mut consecutive_header_start = data.range.start;
for conflicting_header in
headers.iter_mut().rev().take_while(move |h| {
let conflicts = h.range.end == consecutive_header_start
&& h.level >= data.level;
consecutive_header_start = h.range.start;
conflicts
})
{
conflicting_header.short_lived = true;
}
headers.push(Repeatable { inner: data, repeated: row_group.repeat });
for row in group_range {
header_rows.insert(row);
}
}
RowGroupKind::Footer => {
@ -1786,9 +1788,13 @@ impl<'x> CellGridResolver<'_, '_, 'x> {
row_amount: usize,
at_least_one_cell: bool,
) -> SourceResult<Option<Repeatable<Footer>>> {
// Mark consecutive headers right before the end of the table, or the
// final footer, as short lived, given that there are no normal rows
// after them, so repeating them is pointless.
headers.sort_unstable_by_key(|h| h.range.start);
// Mark consecutive headers in those positions as short-lived:
// (a) before a header of lower level;
// (b) right before the end of the table or the final footer;
// That's because they would stop repeating immediately, so don't even
// attempt to.
//
// It is important to do this BEFORE we update header and footer ranges
// due to gutter below as 'row_amount' doesn't consider gutter.
@ -1796,16 +1802,20 @@ impl<'x> CellGridResolver<'_, '_, 'x> {
// TODO(subfooters): take the last footer if it is at the end and
// backtrack through consecutive footers until the first one in the
// sequence is found. If there is no footer at the end, there are no
// haeders to turn short-lived.
// headers to turn short-lived.
let mut consecutive_header_start =
footer.as_ref().map(|(_, _, f)| f.start).unwrap_or(row_amount);
for header_at_the_end in headers.iter_mut().rev().take_while(move |h| {
let at_the_end = h.range.end == consecutive_header_start;
let mut last_consec_level = 0;
for header in headers.iter_mut().rev() {
if header.range.end == consecutive_header_start
&& header.level >= last_consec_level
{
header.short_lived = true;
} else {
last_consec_level = header.level;
}
consecutive_header_start = h.range.start;
at_the_end
}) {
header_at_the_end.short_lived = true;
consecutive_header_start = header.range.start;
}
// Repeat the gutter below a header (hence why we don't
@ -2066,32 +2076,37 @@ fn expand_row_group(
/// Check if a cell's fixed row would conflict with a header or footer.
fn check_for_conflicting_cell_row(
header_rows: &SmallBitSet,
headers: &[Repeatable<Header>],
footer: Option<&(usize, Span, Footer)>,
cell_y: usize,
rowspan: usize,
) -> HintedStrResult<()> {
// NOTE: y + rowspan >, not >=, header.start, to check if the rowspan
// enters the header. For example, consider a rowspan of 1: if
// `y + 1 = header.start` holds, that means `y < header.start`, and it
// only occupies one row (`y`), so the cell is actually not in
// conflict.
if headers
.iter()
.any(|header| cell_y < header.range.end && cell_y + rowspan > header.range.start)
if !headers.is_empty()
&& let Some(row) =
(cell_y..cell_y + rowspan).find(|&row| header_rows.contains(row))
{
bail!(
"cell would conflict with header spanning the same position";
"cell would conflict with header also spanning row {row}";
hint: "try moving the cell or the header"
);
}
// NOTE: y + rowspan >, not >=, footer.start, to check if the rowspan
// enters the footer. For example, consider a rowspan of 1: if
// `y + 1 = footer.start` holds, that means `y < footer.start`, and it
// only occupies one row (`y`), so the cell is actually not in
// conflict.
if let Some((_, _, footer)) = footer
&& cell_y < footer.end
&& cell_y + rowspan > footer.start
{
let row = (cell_y..cell_y + rowspan)
.find(|row| (footer.start..footer.end).contains(row))
.unwrap();
bail!(
"cell would conflict with footer spanning the same position";
"cell would conflict with footer also spanning row {row}";
hint: "try reducing the cell's rowspan or moving the footer"
);
}
@ -2114,11 +2129,11 @@ fn resolve_cell_position(
cell_y: Smart<usize>,
colspan: usize,
rowspan: usize,
header_rows: &SmallBitSet,
headers: &[Repeatable<Header>],
footer: Option<&(usize, Span, Footer)>,
resolved_cells: &[Option<Entry>],
auto_index: &mut usize,
next_header: &mut usize,
first_available_row: usize,
columns: usize,
in_row_group: bool,
@ -2140,12 +2155,11 @@ fn resolve_cell_position(
// but automatically-positioned cells will avoid conflicts by
// simply skipping existing cells, headers and footers.
let resolved_index = find_next_available_position(
headers,
header_rows,
footer,
resolved_cells,
columns,
*auto_index,
next_header,
false,
)?;
@ -2182,7 +2196,13 @@ fn resolve_cell_position(
// footer (but only if it isn't already in one, otherwise there
// will already be a separate check).
if !in_row_group {
check_for_conflicting_cell_row(headers, footer, cell_y, rowspan)?;
check_for_conflicting_cell_row(
header_rows,
headers,
footer,
cell_y,
rowspan,
)?;
}
cell_index(cell_x, cell_y)
@ -2200,26 +2220,11 @@ fn resolve_cell_position(
// ('None'), in which case we'd create a new row to place this
// cell in.
find_next_available_position(
headers,
header_rows,
footer,
resolved_cells,
columns,
initial_index,
// Make our own copy of the 'next_header' counter, since it
// should only be updated by auto cells. However, we cannot
// start with the same value as we are searching from the
// start, and not from 'auto_index', so auto cells might
// have skipped some headers already which this cell will
// also need to skip.
//
// We could, in theory, keep a separate 'next_header'
// counter for cells with fixed columns. But then we would
// need one for every column, and much like how there isn't
// an index counter for each column either, the potential
// speed gain seems less relevant for a less used feature.
// Still, it is something to consider for the future if
// this turns out to be a bottleneck in important cases.
&mut 0,
true,
)
}
@ -2230,7 +2235,13 @@ fn resolve_cell_position(
// footer (but only if it isn't already in one, otherwise there
// will already be a separate check).
if !in_row_group {
check_for_conflicting_cell_row(headers, footer, cell_y, rowspan)?;
check_for_conflicting_cell_row(
header_rows,
headers,
footer,
cell_y,
rowspan,
)?;
}
// Let's find the first column which has that row available.
@ -2267,12 +2278,11 @@ fn resolve_cell_position(
/// the column. That is used to find a position for a fixed column cell.
#[inline]
fn find_next_available_position(
headers: &[Repeatable<Header>],
header_rows: &SmallBitSet,
footer: Option<&(usize, Span, Footer)>,
resolved_cells: &[Option<Entry<'_>>],
columns: usize,
initial_index: usize,
next_header: &mut usize,
skip_rows: bool,
) -> HintedStrResult<usize> {
let mut resolved_index = initial_index;
@ -2296,33 +2306,30 @@ fn find_next_available_position(
// would become impractically large before this overflows.
resolved_index += 1;
}
} else if let Some(header) = headers
.get(*next_header)
.filter(|header| resolved_index >= header.range.start * columns)
{
// Skip header (can't place a cell inside it from outside it).
// No changes needed if we already passed this header (which
// also triggers this branch) - in that case, we only update the
// counter.
if resolved_index < header.range.end * columns {
resolved_index = header.range.end * columns;
if skip_rows {
// Ensure the cell's chosen column is kept after the
// header.
resolved_index += initial_index % columns;
}
} else if header_rows.contains(resolved_index / columns) {
// Skip header rows (can't place a cell inside it from outside it).
if skip_rows {
// Ensure the cell's chosen column is kept after the header.
resolved_index += columns;
} else {
// Skip to the start of the next row.
//
// Add 1 to resolved index to force moving to the next row if
// this is at the start of one. At the end of one, '+ 1'
// already pushes to the next one and 'next_multiple_of' does
// not modify it, so nothing bad happens then either.
resolved_index = (resolved_index + 1).next_multiple_of(columns);
}
// From now on, only check the headers afterwards.
*next_header += 1;
} else if let Some((footer_end, _, _)) = footer.filter(|(end, _, footer)| {
resolved_index >= footer.start * columns && resolved_index < *end * columns
}) {
} else if let Some((footer_end, _, footer)) = footer
&& resolved_index >= footer.start * columns
&& resolved_index < *footer_end * columns
{
// Skip footer, for the same reason.
resolved_index = *footer_end * columns;
if skip_rows {
// Ensure the cell's chosen column is kept after the
// footer.
resolved_index += initial_index % columns;
}
} else {

Binary file not shown.

After

Width:  |  Height:  |  Size: 327 B

Binary file not shown.

After

Width:  |  Height:  |  Size: 469 B

Binary file not shown.

After

Width:  |  Height:  |  Size: 279 B

Binary file not shown.

After

Width:  |  Height:  |  Size: 452 B

Binary file not shown.

After

Width:  |  Height:  |  Size: 616 B

Binary file not shown.

After

Width:  |  Height:  |  Size: 659 B

Binary file not shown.

After

Width:  |  Height:  |  Size: 294 B

Binary file not shown.

Before

Width:  |  Height:  |  Size: 1.3 KiB

After

Width:  |  Height:  |  Size: 1.3 KiB

Binary file not shown.

Before

Width:  |  Height:  |  Size: 1.1 KiB

After

Width:  |  Height:  |  Size: 1.2 KiB

Binary file not shown.

Before

Width:  |  Height:  |  Size: 1.5 KiB

After

Width:  |  Height:  |  Size: 1.6 KiB

Binary file not shown.

Before

Width:  |  Height:  |  Size: 1.2 KiB

After

Width:  |  Height:  |  Size: 1.1 KiB

Binary file not shown.

Before

Width:  |  Height:  |  Size: 1.0 KiB

After

Width:  |  Height:  |  Size: 1.0 KiB

Binary file not shown.

After

Width:  |  Height:  |  Size: 683 B

Binary file not shown.

After

Width:  |  Height:  |  Size: 1.3 KiB

View File

@ -105,7 +105,7 @@
grid.cell(x: 1, y: 3, rowspan: 4)[b],
grid.cell(y: 2, rowspan: 2)[a],
grid.footer(),
// Error: 3-27 cell would conflict with footer spanning the same position
// Error: 3-27 cell would conflict with footer also spanning row 7
// Hint: 3-27 try reducing the cell's rowspan or moving the footer
grid.cell(x: 1, y: 7)[d],
)
@ -120,7 +120,7 @@
grid.cell(x: 1, y: 3, rowspan: 4)[b],
grid.cell(y: 2, rowspan: 2)[a],
grid.footer(),
// Error: 3-33 cell would conflict with footer spanning the same position
// Error: 3-33 cell would conflict with footer also spanning row 7
// Hint: 3-33 try reducing the cell's rowspan or moving the footer
grid.cell(y: 6, rowspan: 2)[d],
)
@ -160,11 +160,20 @@
columns: 2,
grid.header(),
grid.footer(grid.cell(y: 2)[a]),
// Error: 3-39 cell would conflict with footer spanning the same position
// Error: 3-39 cell would conflict with footer also spanning row 2
// Hint: 3-39 try reducing the cell's rowspan or moving the footer
grid.cell(x: 1, y: 1, rowspan: 2)[a],
)
--- grid-footer-rowbreak-line ---
#grid(
columns: 1,
[a],
grid.hline(stroke: red),
grid.footer([b]),
grid.hline(stroke: 3pt),
)
--- grid-footer-multiple ---
// Error: 4:3-4:19 cannot have more than one footer
#grid(
@ -570,3 +579,11 @@
table.cell(x: 1)[D],
),
)
--- issue-6666-auto-hlines-around-footer ---
#table(
columns: 2,
table.hline(stroke: 2pt + blue),
table.footer([*foo*], [*bar*]),
table.hline(stroke: 8pt),
)

View File

@ -137,6 +137,19 @@
[a],
)
--- grid-header-multiple-unordered ---
#set page(height: 4em)
#grid(
grid.header(grid.cell(x: 0, y: 4)[y]),
grid.header([x]),
[a],
[b],
[c],
[d],
[e],
[f],
)
--- grid-header-skip ---
#grid(
columns: 2,
@ -148,6 +161,63 @@
[f], grid.cell(x: 1)[g]
)
--- grid-header-skip-unordered ---
#grid(
columns: 2,
[a],
grid.header(grid.cell(x: 0, y: 2)[y]),
[b],
grid.header([x]),
[c]
)
--- grid-header-rowbreak-auto-pos ---
#grid(
columns: 2,
[x],
grid.hline(stroke: red),
grid.header([a]),
grid.hline(stroke: 3pt),
[y],
grid.header(),
[z],
)
--- grid-header-rowbreak-fixed-pos ---
#grid(
columns: 2,
[z],
grid.hline(stroke: red),
grid.header(grid.cell(x: 0)[b]),
grid.hline(stroke: 3pt),
[w],
[j],
grid.header(grid.cell(x: 0, y: 9)[c]),
[k]
)
--- grid-header-rowbreak-mixed-pos ---
#grid(
columns: 2,
[a],
grid.header([x], grid.cell(x: 0)[b]),
[c],
grid.hline(stroke: red),
grid.header([y], grid.cell(x: 0, y: 8)[d]),
grid.hline(stroke: 3pt),
[e]
)
--- grid-header-rowbreak-auto-and-fixed-pos ---
#grid(
columns: 2,
[a],
grid.header([x]),
[b],
grid.header(grid.cell(x: 0, y: 3)[y]),
[c]
)
--- grid-header-too-large-non-repeating-orphan ---
#set page(height: 8em)
#grid(
@ -398,7 +468,7 @@
[a], [b],
[c],
),
// Error: 3-48 cell would conflict with header spanning the same position
// Error: 3-48 cell would conflict with header also spanning row 1
// Hint: 3-48 try moving the cell or the header
table.cell(x: 1, y: 1, rowspan: 2, lorem(80))
)
@ -411,7 +481,7 @@
[a], [b],
[c],
),
// Error: 3-42 cell would conflict with header spanning the same position
// Error: 3-42 cell would conflict with header also spanning row 1
// Hint: 3-42 try moving the cell or the header
table.cell(y: 1, rowspan: 2, lorem(80))
)
@ -616,6 +686,45 @@
),
)
--- grid-header-collision-multiple-ordered ---
#grid(
columns: 2,
grid.cell(x: 0, y: 0)[a],
grid.cell(x: 1, y: 0)[a],
grid.cell(x: 0, y: 3)[a],
grid.header(grid.cell(x: 0, y: 2)[y]),
// Error: 15-39 cell would cause header to expand to non-empty row 3
// Hint: 15-39 try moving its cells to available rows
grid.header(grid.cell(x: 0, y: 3)[y]),
grid.header(grid.cell(x: 0, y: 4)[y]),
)
--- grid-header-collision-multiple-unordered ---
#grid(
columns: 2,
grid.cell(x: 0, y: 0)[a],
grid.cell(x: 1, y: 0)[a],
grid.header(grid.cell(x: 0, y: 2)[y]),
grid.header(grid.cell(x: 0, y: 3)[y]),
grid.header(grid.cell(x: 0, y: 4)[y]),
// Error: 3-27 cell would conflict with header also spanning row 3
// Hint: 3-27 try moving the cell or the header
grid.cell(x: 0, y: 3)[a]
)
--- grid-header-collision-multiple-rowspan ---
#grid(
columns: 2,
grid.cell(x: 0, y: 0)[a],
grid.cell(x: 1, y: 0)[a],
grid.header(grid.cell(x: 0, y: 2)[y]),
grid.header(grid.cell(x: 0, y: 3)[y]),
grid.header(grid.cell(x: 0, y: 4)[y]),
// Error: 3-39 cell would conflict with header also spanning row 2
// Hint: 3-39 try moving the cell or the header
grid.cell(x: 0, y: 1, rowspan: 2)[a]
)
--- issue-5359-column-override-stays-inside-header ---
#table(
columns: 3,
@ -625,3 +734,15 @@
table.cell(x: 1)[D],
),
)
--- issue-6666-auto-hlines-around-header ---
#table(
columns: 2,
table.hline(stroke: 2pt + blue),
table.header([*foo*], [*bar*]),
table.hline(stroke: 1.5pt + red),
table.cell(colspan: 2)[_asdf_],
table.hline(stroke: 1.5pt + red),
[a], [b],
[c], [d],
)