From 35d11bde0303ad9f9faf4088dbf6a97a32e87ec5 Mon Sep 17 00:00:00 2001 From: Davy Kager Date: Fri, 23 Oct 2026 21:29:38 +0200 Subject: [PATCH 1/3] Fix mfenced separators in canonicalization convert_mfenced_to_mrow did not follow the MathML rules for the separators attribute: - separators="" inserted a comma between all children instead of no separators - with too few separators a comma was used instead of repeating the last separator - whitespace in separators was used as a separator instead of being ignored Each case has a unit test. The expected output of mfenced_with_separators is corrected to repeat the last separator. Co-Authored-By: Claude Opus 5.5 --- src/canonicalize.rs | 70 ++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 63 insertions(+), 7 deletions(-) diff --git a/src/canonicalize.rs b/src/canonicalize.rs index 86ea90945..6ef0431a2 100644 --- a/src/canonicalize.rs +++ b/src/canonicalize.rs @@ -1882,7 +1882,7 @@ impl CanonicalizeContext { let close = close_raw.as_deref().unwrap_or(")").replace('>', "⟩"); // debug!("open={}, close={}", open, close); let separators_raw = mfenced.attribute_value("separators"); - let mut separators = separators_raw.as_deref().unwrap_or(",").chars(); + let separators: Vec = separators_raw.as_deref().unwrap_or(",").chars().filter(|ch| !ch.is_whitespace()).collect(); set_mathml_name(mfenced, "mrow"); mfenced.remove_attribute("open"); mfenced.remove_attribute("close"); @@ -1894,9 +1894,10 @@ impl CanonicalizeContext { } if !children.is_empty() { new_children.push(children[0]); - for child in &children[1..] { - let sep = separators.next().unwrap_or(',').to_string(); - new_children.push( ChildOfElement::Element( create_mo(mfenced.document(), &sep, MFENCED_ATTR_VALUE)) ); + for (i, child) in children[1..].iter().enumerate() { + if let Some(sep) = separators.get(i).or(separators.last()) { + new_children.push( ChildOfElement::Element( create_mo(mfenced.document(), &sep.to_string(), MFENCED_ATTR_VALUE)) ); + } new_children.push(*child); } } @@ -4991,14 +4992,69 @@ mod canonicalize_tests { y ; + z + ; + a + + ) + + + "; + are_strs_canonically_equal_result(test_str, target_str, &[]) + } + + #[test] + fn mfenced_empty_separators() -> Result<()> { + let test_str = "5+3"; + let target_str = " + + ( - z - , - a + 5 + + + 3 + ) + + "; + are_strs_canonically_equal_result(test_str, target_str, &[]) + } + + #[test] + fn mfenced_repeat_last_separator() -> Result<()> { + let test_str = "xyz"; + let target_str = " + + ( + + x + ; + y + ; + z ) + "; + are_strs_canonically_equal_result(test_str, target_str, &[]) + } + + #[test] + fn mfenced_separators_with_whitespace() -> Result<()> { + let test_str = "xyz"; + let target_str = " + + ( + + x + ; + + y + , + z + + + ) "; are_strs_canonically_equal_result(test_str, target_str, &[]) From cf4a444d11b120c63d83ff0f0ac157dcdf717414 Mon Sep 17 00:00:00 2001 From: Davy Kager Date: Fri, 23 Oct 2026 22:27:58 +0200 Subject: [PATCH 2/3] Add mfenced tests for too many, default and whitespace-only separators --- src/canonicalize.rs | 51 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 51 insertions(+) diff --git a/src/canonicalize.rs b/src/canonicalize.rs index 6ef0431a2..2900e4d55 100644 --- a/src/canonicalize.rs +++ b/src/canonicalize.rs @@ -5060,6 +5060,57 @@ mod canonicalize_tests { are_strs_canonically_equal_result(test_str, target_str, &[]) } + #[test] + fn mfenced_too_many_separators() -> Result<()> { + let test_str = "xy"; + let target_str = " + + ( + + x + ; + y + + ) + + "; + are_strs_canonically_equal_result(test_str, target_str, &[]) + } + + #[test] + fn mfenced_default_separator() -> Result<()> { + let test_str = "xy"; + let target_str = " + + ( + + x + , + y + + ) + + "; + are_strs_canonically_equal_result(test_str, target_str, &[]) + } + + #[test] + fn mfenced_whitespace_only_separators() -> Result<()> { + let test_str = "5+3"; + let target_str = " + + ( + + 5 + + + 3 + + ) + + "; + are_strs_canonically_equal_result(test_str, target_str, &[]) + } + #[test] fn canonical_one_element_mrow_around_mrow() -> Result<()> { let test_str = "-a"; From 9aab1bcdddcc724c285335b6a2a987e8c1310ee4 Mon Sep 17 00:00:00 2001 From: NSoiffer Date: Fri, 23 Oct 2026 15:14:58 -0700 Subject: [PATCH 3/3] Refactor separator handling in canonicalize.rs Small tweak for more efficient code/more idiomatic code: `.or(separators.last())` => `or_else(|| separators.last())` This avoids evaluating `separators.last()` when `separators.get(i)` is already `Some`. --- src/canonicalize.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/canonicalize.rs b/src/canonicalize.rs index 2900e4d55..ffb2151b2 100644 --- a/src/canonicalize.rs +++ b/src/canonicalize.rs @@ -1895,8 +1895,8 @@ impl CanonicalizeContext { if !children.is_empty() { new_children.push(children[0]); for (i, child) in children[1..].iter().enumerate() { - if let Some(sep) = separators.get(i).or(separators.last()) { - new_children.push( ChildOfElement::Element( create_mo(mfenced.document(), &sep.to_string(), MFENCED_ATTR_VALUE)) ); + if let Some(&sep) = separators.get(i).or_else(|| separators.last()) { + new_children.push(ChildOfElement::Element( create_mo(mfenced.document(), &sep.to_string(), MFENCED_ATTR_VALUE)) ); } new_children.push(*child); }