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 <controls> 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 (`<v:shapetype id>`) 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

@@ -357,22 +357,25 @@ fn build(p: Props, kind: ControlKind, anchor: Anchor, name: Option<&str>, id: u3
     c
 }
 
+/// The `<control>` elements one child of a `<controls>` 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 `<control>` elements under a `<controls>` 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 `<controls>` 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<u64> {
+pub(crate) fn shape_number(id: &str) -> Option<u64> {
     id.trim().trim_start_matches("_x0000_s").parse().ok()
 }
 

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("<v:shapetype")).map(|i| i + from) {
         let Some(j) = ours.get(i..).and_then(|s| s.find("</v:shapetype>")).map(|j| i + j + "</v:shapetype>".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 `<controls>` 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<u64>) -> Option<String> {
     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::<u64>().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());

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
+/// `<controls>` 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#"<?xml version="1.0"?><Relationships xmlns="http://schemas.openxmlformats.org/package/2006/relationships"><Relationship Id="rId9" Type="http://schemas.openxmlformats.org/officeDocument/2006/relationships/vmlDrawing" Target="../drawings/vmlDrawing1.vml"/></Relationships>"#;
+    let vml = r##"<xml xmlns:v="urn:schemas-microsoft-com:vml" xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:x="urn:schemas-microsoft-com:office:excel"><v:shape id="_x0000_s1025" type="#_x0000_t201"><x:ClientData ObjectType="Scroll"><x:Anchor>2, 0, 3, 0, 5, 0, 4, 0</x:Anchor><x:FmlaLink>$B$1</x:FmlaLink><x:Val>40</x:Val><x:Min>0</x:Min><x:Max>100</x:Max><x:Inc>1</x:Inc><x:Page>10</x:Page></x:ClientData></v:shape></xml>"##;
+    let bytes = minimal(
+        r#"<sheetData/><legacyDrawing r:id="rId9"/><controls><control shapeId='1025' name='Scroll 1'/></controls>"#,
+        &[("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 <controls> 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("<controls").count(), 1, "{sheet}");
+    assert_eq!(sheet.matches("<mc:Fallback>").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("<v:shape ").count(), 1);
+    let (back, report) = read_xlsx(&out).unwrap();
+    assert!(report.warnings.is_empty(), "{:?}", report.warnings);
+    assert_eq!(back.sheet(0).unwrap().controls.len(), 1);
+}
+
 /// Shapes with a named `id` and their number in `o:spid`, as some producers write them.
 const VML_SPID: &str = r##"<xml xmlns:v="urn:schemas-microsoft-com:vml" xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:x="urn:schemas-microsoft-com:office:excel">
 <v:shape id="Button 1" o:spid="_x0000_s1025" o:button="t"><v:textbox><div>Go</div></v:textbox><x:ClientData ObjectType="Button"><x:Anchor>1, 0, 6, 0, 3, 0, 7, 0</x:Anchor><x:FmlaMacro>[0]!Tidy</x:FmlaMacro></x:ClientData></v:shape>