From 6b9310657555596f5dbb77c77a0d887af8026162 Mon Sep 17 00:00:00 2001 From: prxnvt <53838599+prxnvt@users.noreply.github.com> Date: Mon, 2 Nov 2026 05:14:17 -0500 Subject: [PATCH] XLSX: a kept control is matched to the modelled ones as the reader reads it, in either quote style without_controls found the controls to drop from a kept fragment by splitting its text on `shapeId="`. The control reader takes the attribute through the XML parser, which also accepts `shapeId='1025'`, so a producer's single-quoted control was modelled, its VML shape taken out of the kept drawing, and the control itself still kept: a save wrote it twice, the kept copy pointing at a shape that no longer existed. The kept child is now parsed and its controls found with the reader's own helpers (controls_of_child, shape_number), so what the reader modelled is what is dropped. The shape type check of the VML merge (``) accepts either quote style too, so a kept drawing that defines our shape type with single quotes is not given a second definition. Test: a_single_quoted_shape_id_is_dropped_from_the_kept_controls_too (the control is read once, nothing of it is kept, the saved sheet holds one control and no single-quoted copy, the VML one shape, and the file reads back with one control). --- crates/xlsx/src/controls.rs | 31 +++++++++++---------- crates/xlsx/src/passthrough/read.rs | 31 ++++++++++++++++++--- crates/xlsx/src/tests/controls_tests.rs | 36 +++++++++++++++++++++++++ 3 files changed, 81 insertions(+), 17 deletions(-) diff --git a/crates/xlsx/src/controls.rs b/crates/xlsx/src/controls.rs index 75c8032df..da22eb127 100644 --- a/crates/xlsx/src/controls.rs +++ b/crates/xlsx/src/controls.rs @@ -357,22 +357,25 @@ fn build(p: Props, kind: ControlKind, anchor: Anchor, name: Option<&str>, id: u3 c } +/// The `` elements one child of a `` element holds: the child itself, or those +/// inside its `mc:AlternateContent` (the Choice, else the Fallback). +pub(crate) fn controls_of_child(k: &El) -> Vec<&El> { + match k.name.as_str() { + "control" => vec![k], + "AlternateContent" => k + .child("Choice") + .filter(|c| c.child("control").is_some()) + .or_else(|| k.child("Fallback")) + .map(|inner| inner.kids("control").collect()) + .unwrap_or_default(), + _ => Vec::new(), + } +} + /// The `` elements under a `` element, looking through `mc:AlternateContent` /// (the Choice, else the Fallback). fn controls_in(e: &El) -> Vec<&El> { - let mut out = Vec::new(); - for k in &e.children { - match k.name.as_str() { - "control" => out.push(k), - "AlternateContent" => { - if let Some(inner) = k.child("Choice").filter(|c| c.child("control").is_some()).or_else(|| k.child("Fallback")) { - out.extend(inner.kids("control")); - } - } - _ => {} - } - } - out + e.children.iter().flat_map(controls_of_child).collect() } /// The `` element of a worksheet child (`controls`, or one inside AlternateContent). @@ -385,7 +388,7 @@ pub fn controls_of(e: &El) -> Option<&El> { } /// The shape number of a VML shape id (`_x0000_s1025` → 1025) or a `shapeId` attribute. -fn shape_number(id: &str) -> Option { +pub(crate) fn shape_number(id: &str) -> Option { id.trim().trim_start_matches("_x0000_s").parse().ok() } diff --git a/crates/xlsx/src/passthrough/read.rs b/crates/xlsx/src/passthrough/read.rs index dad81a9c5..4fc55b9c6 100644 --- a/crates/xlsx/src/passthrough/read.rs +++ b/crates/xlsx/src/passthrough/read.rs @@ -630,8 +630,8 @@ pub(super) fn merge_vml(kept: &str, ours: &str) -> String { while let Some(i) = ours.get(from..).and_then(|s| s.find("")).map(|j| i + j + "".len()) else { break }; let ty = ours.get(i..j).unwrap_or(""); - let id = ty.split("id=\"").nth(1).and_then(|r| r.split('"').next()).unwrap_or(""); - if !id.is_empty() && !kept.contains(&format!("id=\"{id}\"")) { + let id = attr_value(ty, "id").unwrap_or(""); + if !id.is_empty() && !kept.contains(&format!("id=\"{id}\"")) && !kept.contains(&format!("id='{id}'")) { add.push_str(ty); } from = j; @@ -660,6 +660,25 @@ pub(super) fn merge_vml(kept: &str, ours: &str) -> String { format!("{head}{add}{}", kept.get(close..).unwrap_or("")) } +/// The value of the first attribute `name` in XML text `x`, in either quote style (`name="v"` or +/// `name='v'`), when it has one. The name must start a token (`id` is not the end of `spid`). +fn attr_value<'a>(x: &'a str, name: &str) -> Option<&'a str> { + let mut from = 0; + while let Some(i) = x.get(from..)?.find(name).map(|i| i + from) { + from = i + name.len(); + let starts_token = x.get(..i).is_some_and(|before| before.ends_with(|c: char| c.is_whitespace() || c == '<')); + let Some(rest) = x.get(from..).map(str::trim_start).and_then(|r| r.strip_prefix('=')).map(str::trim_start) else { continue }; + if !starts_token { + continue; + } + if let Some(q) = rest.chars().next().filter(|q| *q == '"' || *q == '\'') { + let body = rest.get(q.len_utf8()..)?; + return body.find(q).and_then(|e| body.get(..e)); + } + } + None +} + /// A kept `` fragment without the controls whose `shapeId` is in `drop` (they are /// written from the model). `None` when no control is left. A fragment that doesn't parse is /// returned unchanged. @@ -667,8 +686,14 @@ fn without_controls(frag: &str, drop: &HashSet) -> Option { if drop.is_empty() { return Some(frag.to_string()); } + // A kept child names its shapes the way the control reader reads them (`shapeId` in either + // quote style, through mc:AlternateContent), so what the reader modelled is what is dropped. let numbered = |x: &str| { - x.split("shapeId=\"").skip(1).any(|r| r.split('"').next().and_then(|n| n.trim().parse::().ok()).is_some_and(|n| drop.contains(&n))) + crate::xml::parse(x.as_bytes()).is_ok_and(|e| { + crate::controls::controls_of_child(&e) + .iter() + .any(|c| c.attr("shapeId").and_then(crate::controls::shape_number).is_some_and(|n| drop.contains(&n))) + }) }; let mut reader = Reader::from_str(frag); let mut out = String::with_capacity(frag.len()); diff --git a/crates/xlsx/src/tests/controls_tests.rs b/crates/xlsx/src/tests/controls_tests.rs index 7aedc51d3..323ca14f3 100644 --- a/crates/xlsx/src/tests/controls_tests.rs +++ b/crates/xlsx/src/tests/controls_tests.rs @@ -181,6 +181,42 @@ fn vml_only_controls_are_read_and_clamped() { assert_eq!(c[2].action, Some(ControlAction::Macro { name: "Tidy".into(), input: MacroInput::Selection, output: None })); } +/// A producer that quotes `shapeId` with single quotes: the control is read, and the kept +/// `` fragment drops it as it drops a double-quoted one, so a save holds the control +/// once and no kept copy points at a VML shape that was taken out. +#[test] +fn a_single_quoted_shape_id_is_dropped_from_the_kept_controls_too() { + let rels = r#""#; + let vml = r##"2, 0, 3, 0, 5, 0, 4, 0$B$1400100110"##; + let bytes = minimal( + r#""#, + &[("xl/worksheets/_rels/sheet1.xml.rels", rels), ("xl/drawings/vmlDrawing1.vml", vml)], + "", + "", + ); + let (wb, report) = read_xlsx(&bytes).unwrap(); + // The minimal package has no styles part; nothing about the control is reported. + assert!(report.warnings.iter().all(|w| w == "no styles part"), "{:?}", report.warnings); + let sh = wb.sheet(0).unwrap(); + assert_eq!(sh.controls.len(), 1); + assert_eq!((sh.controls[0].kind, sh.controls[0].value, sh.controls[0].name.as_str()), (ControlKind::Slider, 40, "Scroll 1")); + let kept = sh.preserved.as_ref().and_then(|k| k.elements.iter().find(|e| e.slot == "controls")).map(|e| e.xml.clone()); + assert_eq!(kept, None, "nothing of the modelled control is kept for pass-through"); + // The saved file holds the control once, with its one VML shape, and reads back once. + let out = write_xlsx(&wb).unwrap(); + let mut pkg = Package::open(&out).unwrap(); + let sheet = text(&mut pkg, "xl/worksheets/sheet1.xml"); + // One element with one control (each control is written as a Choice with a + // Fallback inside the element's own AlternateContent), and no kept copy. + assert_eq!(sheet.matches("").count(), 1, "{sheet}"); + assert!(!sheet.contains("shapeId='1025'"), "the kept copy would point at a shape that is gone: {sheet}"); + assert_eq!(text(&mut pkg, "xl/drawings/vmlDrawing1.vml").matches("
Go
1, 0, 6, 0, 3, 0, 7, 0[0]!Tidy