Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Binary file added assets/bugfixes/issue-452/after.jpg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added assets/bugfixes/issue-452/before.jpg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added assets/bugfixes/issue-452/gt.jpg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
15 changes: 10 additions & 5 deletions crates/office2pdf/src/parser/docx.rs
Original file line number Diff line number Diff line change
Expand Up @@ -70,11 +70,16 @@ mod text;
/// Parser for DOCX (Office Open XML Word) documents.
pub struct DocxParser;

/// Word supplies this built-in paragraph spacing when neither a paragraph
/// nor its style hierarchy specifies an override. Line height is left to the
/// renderer, which derives Word's single-spacing pitch from the actual font
/// metrics (issue #354).
pub(super) const WORD_COMPATIBLE_PARAGRAPH_SPACE_AFTER_PT: f64 = 8.0;
/// The paragraph spacing Word applies when neither a paragraph nor its
/// style hierarchy specifies `w:spacing w:after`: ECMA-376 leaves the gap
/// at zero, and a Word PDF export of a document whose `styles.xml` defines
/// no `Normal` spacing confirms it. Recording it explicitly (rather than
/// leaving `space_after` unset) also pins the paragraph block's `below`, so
/// Typst's own 1.2em default block spacing cannot leak into the gap.
///
/// Line height is left to the renderer, which derives Word's single-spacing
/// pitch from the actual font metrics (issues #354, #452).
pub(super) const WORD_COMPATIBLE_PARAGRAPH_SPACE_AFTER_PT: f64 = 0.0;

fn apply_word_compatible_paragraph_defaults(style: &mut ParagraphStyle) {
style
Expand Down
6 changes: 4 additions & 2 deletions crates/office2pdf/src/parser/docx_foundation_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -595,9 +595,11 @@ fn test_paragraph_uses_word_default_spacing_when_unspecified() {
let paragraph = first_paragraph(&doc);

// Line height stays unset in the IR: the renderer derives Word's
// single-spacing pitch from the actual font metrics (issue #354).
// single-spacing pitch from the actual font metrics (issue #354). The
// gap below is zero, because neither the paragraph nor the style
// hierarchy sets `w:spacing w:after` (issue #452).
assert_eq!(paragraph.style.line_box, None);
assert_eq!(paragraph.style.space_after, Some(8.0));
assert_eq!(paragraph.style.space_after, Some(0.0));
}

#[test]
Expand Down
5 changes: 3 additions & 2 deletions crates/office2pdf/src/parser/docx_table_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -400,9 +400,10 @@ fn test_table_cell_paragraph_uses_word_default_line_box_and_spacing() {
other => panic!("expected table cell paragraph, got {other:?}"),
};

// Line height stays unset in the IR (issue #354).
// Line height stays unset in the IR (issue #354); an unspecified
// `w:spacing w:after` is zero (issue #452).
assert_eq!(paragraph(0).style.line_box, None);
assert_eq!(paragraph(0).style.space_after, Some(8.0));
assert_eq!(paragraph(0).style.space_after, Some(0.0));

assert_eq!(paragraph(1).style.line_box, None);
assert_eq!(paragraph(1).style.space_after, Some(6.0));
Expand Down
2 changes: 1 addition & 1 deletion crates/office2pdf/src/parser/docx_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -304,7 +304,7 @@ fn test_list_paragraphs_use_word_compatible_spacing_when_unspecified() {
})
.expect("numbered paragraphs should become a list");

assert_eq!(list.items[0].content[0].style.space_after, Some(8.0));
assert_eq!(list.items[0].content[0].style.space_after, Some(0.0));
assert_eq!(list.items[1].content[0].style.space_after, Some(6.0));
}

Expand Down
9 changes: 4 additions & 5 deletions crates/office2pdf/src/render/typst_gen.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1564,20 +1564,19 @@ fn generate_block(out: &mut String, block: &Block, ctx: &mut GenCtx) -> Result<(
// Grid-snapped line height applies to list items too (Word's
// document grid covers all body text).
let first_paragraph = list.items.first().and_then(|item| item.content.first());
let metric_leading_pt: Option<f64> = first_paragraph.and_then(|paragraph| {
word_line_leading_pt(&paragraph.runs, &paragraph.style, ctx.line_grid_pitch)
});
let settings: Option<String> = first_paragraph.and_then(|paragraph| {
word_line_height_settings(&paragraph.runs, &paragraph.style, ctx.line_grid_pitch)
});
if let Some(settings) = settings {
out.push_str("#block(width: 100%)[\n");
out.push_str(&settings);
generate_list(out, list, metric_leading_pt)?;
// The wrapper's line box spans Word's full line advance, so
// the item gaps are the raw `w:spacing` values.
generate_list(out, list, true)?;
out.push_str("]\n");
Ok(())
} else {
generate_list(out, list, None)
generate_list(out, list, false)
}
}
Block::MathEquation(math) => {
Expand Down
31 changes: 12 additions & 19 deletions crates/office2pdf/src/render/typst_gen_list_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -839,12 +839,13 @@ fn test_generate_list_uses_first_item_level_marker_when_list_starts_nested() {
}

#[test]
fn test_generate_list_metric_spacing_adds_gap_to_single_space_leading() {
fn test_generate_list_metric_spacing_is_the_raw_paragraph_gap() {
// Word adds `w:spacing w:after` directly to the single-space line
// advance between list items: next item top = previous line advance +
// after. Under metric text edges that whitespace is the single-space
// leading plus the paragraph gap; adding a whole line height instead
// stretched every list block by ~8pt per item (issue #384).
// after. The wrapper's line box already spans that advance, so the
// Typst list `spacing` that replaces the automatic leading is the raw
// paragraph gap — adding a whole line height instead stretched every
// list block by ~8pt per item (issues #384, #452).
use crate::ir::List;

let Some((ascender, descender, word_pitch_em)) =
Expand All @@ -853,8 +854,7 @@ fn test_generate_list_metric_spacing_adds_gap_to_single_space_leading() {
return; // no font book available (e.g. exotic CI sandbox)
};
let font_size: f64 = 10.0;
let single_space_leading_pt: f64 =
((word_pitch_em - (ascender + descender)) * font_size).max(0.0);
let metric_em: f64 = ascender + descender;

let make_item = |text: &str| ListItem {
content: vec![Paragraph {
Expand Down Expand Up @@ -886,21 +886,14 @@ fn test_generate_list_metric_spacing_adds_gap_to_single_space_leading() {
.unwrap()
.source;

let advance_pt: f64 = (word_pitch_em * font_size).max(metric_em * font_size);
assert_line_advance(&source, "Libertinus Serif", font_size, advance_pt);
assert!(
source.contains(&format!(
"top-edge: {}em, bottom-edge: -{}em",
format_f64(ascender),
format_f64(descender)
)),
"fixed nominal-metric em edges expected in: {source}"
source.contains("spacing: 4pt"),
"expected inter-item spacing to be the raw 4pt gap in: {source}"
);
let expected = format_f64(single_space_leading_pt + 4.0);
assert!(
source.contains(&format!("spacing: {expected}pt")),
"expected inter-item spacing {expected}pt (leading + gap) in: {source}"
);
assert!(
source.contains(&format!("below: {expected}pt")),
"expected list below spacing {expected}pt (leading + gap) in: {source}"
source.contains("below: 4pt"),
"expected list below spacing to be the raw 4pt gap in: {source}"
);
}
54 changes: 26 additions & 28 deletions crates/office2pdf/src/render/typst_gen_lists.rs
Original file line number Diff line number Diff line change
Expand Up @@ -242,7 +242,7 @@ fn paragraph_line_height(paragraph: &Paragraph) -> f64 {
fn list_boundary_spacing(
previous: &crate::ir::ListItem,
next: &crate::ir::ListItem,
metric_leading_pt: Option<f64>,
wrapper_spans_full_line: bool,
) -> Option<f64> {
let previous_paragraph = previous.content.last()?;
let next_paragraph = next.content.first()?;
Expand All @@ -256,16 +256,16 @@ fn list_boundary_spacing(
if previous_paragraph.style.line_box.is_some() && next_paragraph.style.line_box.is_some() {
return Some(paragraph_gap);
}
// Under Word metric text edges the enclosing wrapper's leading already
// tops each line box up to the single-space (or grid) advance; Word adds
// w:spacing before/after on top of that advance, so the item gap is the
// leading plus the paragraph gap. Adding a whole line height instead
// stretched every spaced list by roughly a line per item (issue #384).
if let Some(leading) = metric_leading_pt
&& paragraph_uses_metric_edges(previous_paragraph)
&& paragraph_uses_metric_edges(next_paragraph)
// The enclosing wrapper's line box already spans Word's full single-space
// (or grid) advance, and Word adds w:spacing before/after on top of that
// advance, so the item gap is exactly the paragraph gap. Adding a whole
// line height instead stretched every spaced list by roughly a line per
// item (issues #384, #452).
if wrapper_spans_full_line
&& paragraph_uses_full_line_box(previous_paragraph)
&& paragraph_uses_full_line_box(next_paragraph)
{
return Some(leading + paragraph_gap);
return Some(paragraph_gap);
}

// An explicit Typst list spacing replaces its automatic paragraph
Expand All @@ -276,24 +276,24 @@ fn list_boundary_spacing(
Some(line_height + paragraph_gap)
}

/// Whether the paragraph renders under the wrapper's metric text edges
/// Whether the paragraph renders under the wrapper's full-advance line box
/// (no explicit line spacing or fixed line box of its own).
fn paragraph_uses_metric_edges(paragraph: &Paragraph) -> bool {
fn paragraph_uses_full_line_box(paragraph: &Paragraph) -> bool {
paragraph.style.line_spacing.is_none() && paragraph.style.line_box.is_none()
}

fn common_list_level_spacing(
items: &[crate::ir::ListItem],
level: u32,
metric_leading_pt: Option<f64>,
wrapper_spans_full_line: bool,
) -> Option<f64> {
let level_items = items
.iter()
.filter(|item| item.level == level)
.collect::<Vec<_>>();
let mut boundaries = level_items
.windows(2)
.map(|pair| list_boundary_spacing(pair[0], pair[1], metric_leading_pt));
.map(|pair| list_boundary_spacing(pair[0], pair[1], wrapper_spans_full_line));
let first = boundaries.next()??;

(first > 0.0001
Expand All @@ -304,7 +304,7 @@ fn common_list_level_spacing(
fn list_edge_spacing(
list: &List,
level: u32,
metric_leading_pt: Option<f64>,
wrapper_spans_full_line: bool,
) -> (Option<f64>, Option<f64>) {
let first = list.items.iter().find(|item| item.level == level);
let last = list.items.iter().rev().find(|item| item.level == level);
Expand All @@ -313,13 +313,11 @@ fn list_edge_spacing(
return Some(spacing.unwrap_or(0.0).max(0.0));
}
// Same Word semantics as the item boundaries: before/after extends
// the line advance, so under metric edges the whitespace is the
// wrapper leading plus the gap (issue #384).
if let Some(leading) = metric_leading_pt
&& paragraph_uses_metric_edges(paragraph)
{
// the line advance, which the wrapper's line box already spans, so
// the whitespace is the gap alone (issues #384, #452).
if wrapper_spans_full_line && paragraph_uses_full_line_box(paragraph) {
return spacing
.map(|spacing| leading + spacing.max(0.0))
.map(|spacing| spacing.max(0.0))
.filter(|spacing| *spacing > 0.0001);
}
spacing
Expand Down Expand Up @@ -352,14 +350,14 @@ fn common_list_line_box(list: &List) -> Option<LineBox> {
pub(super) fn generate_list(
out: &mut String,
list: &List,
metric_leading_pt: Option<f64>,
wrapper_spans_full_line: bool,
) -> Result<(), ConvertError> {
let root_level: u32 = list_root_level(list);
let style = list_style_for_level(list, root_level);
let fallback_marker_style = common_list_level_text_style(&list.items, root_level);
let indent = common_list_level_indent(&list.items, root_level);
let spacing_pt = common_list_level_spacing(&list.items, root_level, metric_leading_pt);
let (space_before, space_after) = list_edge_spacing(list, root_level, metric_leading_pt);
let spacing_pt = common_list_level_spacing(&list.items, root_level, wrapper_spans_full_line);
let (space_before, space_after) = list_edge_spacing(list, root_level, wrapper_spans_full_line);
let line_box = common_list_line_box(list);
let start_at = list.items.first().and_then(|item| item.start_at);
if space_before.is_some() || space_after.is_some() {
Expand All @@ -384,7 +382,7 @@ pub(super) fn generate_list(
spacing_pt,
start_at,
);
generate_list_items(out, list, &list.items, root_level, metric_leading_pt)?;
generate_list_items(out, list, &list.items, root_level, wrapper_spans_full_line)?;
out.push_str(")\n");
if space_before.is_some() || space_after.is_some() {
out.push_str("]\n");
Expand Down Expand Up @@ -1155,7 +1153,7 @@ fn generate_list_items(
list: &List,
items: &[crate::ir::ListItem],
base_level: u32,
metric_leading_pt: Option<f64>,
wrapper_spans_full_line: bool,
) -> Result<(), ConvertError> {
let style = list_style_for_level(list, base_level);
let (_, item_func) = list_funcs(style.kind);
Expand Down Expand Up @@ -1202,7 +1200,7 @@ fn generate_list_items(
let spacing_pt = common_list_level_spacing(
&items[nested_start..nested_end],
base_level + 1,
metric_leading_pt,
wrapper_spans_full_line,
);
let nested_start_at = items[nested_start].start_at;
write_list_open(
Expand All @@ -1219,7 +1217,7 @@ fn generate_list_items(
list,
&items[nested_start..nested_end],
base_level + 1,
metric_leading_pt,
wrapper_spans_full_line,
)?;
out.push(')');
i = nested_end;
Expand Down
Loading
Loading