Merge #5225: M12: XFA field names made up for repeated fields never take a name the template uses (#5224)

# Conflicts:
#	crates/xfa/src/layout/common.rs
#	crates/xfa/src/tests.rs

crates/xfa/src/layout.rs

@@ -28,9 +28,9 @@ mod items;
 mod measure;
 mod pages;
 
-use common::collect_values;
 pub(crate) use common::date_pattern;
 pub use common::formatted_value;
+use common::{collect_names, collect_values};
 pub(crate) use items::UNDERLINE_DROP;
 pub use items::{Action, BorderShape, Form, Item, Page, Rounded, Turned, Widget, WidgetBorder, WidgetKind};
 
@@ -168,6 +168,11 @@ struct Layouter<'t> {
     pages: Vec<Page>,
     cur: Option<Cursor>,
     names: HashMap<String, usize>,
+    /// Every PDF field name handed out so far.
+    taken: HashSet<String>,
+    /// The names the template's fields, groups and groups' members take as written
+    /// ([`common::collect_names`]), which a made-up `_2`, `_3` name must not take.
+    template_names: HashSet<String>,
     /// Field values by `id`, for embedded fields in rich text.
     values: HashMap<String, common::EmbedSource>,
     warnings: Vec<String>,
@@ -222,6 +227,8 @@ pub fn layout(tpl: &Template, data: Option<&DataNode>) -> Result<Form, XfaError>
     let values = collect_values(tpl, data);
     // One copy of the root for the whole layout: measurements are cached by node address.
     let root_node = Node::Subform(Box::new(tpl.root.clone()));
+    let mut template_names = HashSet::new();
+    collect_names(std::slice::from_ref(&root_node), &mut template_names, 0);
     let mut l = Layouter {
         tpl,
         data,
@@ -230,6 +237,8 @@ pub fn layout(tpl: &Template, data: Option<&DataNode>) -> Result<Form, XfaError>
         pages: Vec::new(),
         cur: None,
         names: HashMap::new(),
+        taken: HashSet::new(),
+        template_names,
         values,
         warnings: Vec::new(),
         warned: HashSet::new(),

crates/xfa/src/layout/common.rs

@@ -1,5 +1,5 @@
 use super::items::{Action, BorderShape, Item, Rounded, Widget, WidgetBorder};
-use super::{DataNode, DataPath, HashMap, Layout, Node, Rect, Script, Template, Ui, data_path, date_letters, iso_to_pattern};
+use super::{DataNode, DataPath, HashMap, HashSet, Layout, MAX_DEPTH, Node, Rect, Script, Template, Ui, data_path, date_letters, iso_to_pattern};
 use crate::data::{data_date_letters, data_to_iso, fields_with_ids};
 
 /// The value of each field with an `id`, for the rich text that embeds it (floating fields,
@@ -43,6 +43,47 @@ impl EmbedSource {
     }
 }
 
+/// The names the template's fields, exclusion groups and the groups' member fields take as
+/// written ([`clean_name`]): a name the layout makes up for a repeated field (`Cell4` →
+/// `Cell4_2`) must not be one of these, or two fields would share it. (A group's radio buttons
+/// are the group's PDF field; a member that isn't one is a PDF field with its own name.)
+pub(super) fn collect_names(nodes: &[Node], out: &mut HashSet<String>, depth: usize) {
+    if depth > MAX_DEPTH {
+        return;
+    }
+    for n in nodes {
+        match n {
+            Node::Field(f) => {
+                if let Some(name) = &f.common.name {
+                    out.insert(clean_name(name));
+                }
+            }
+            Node::ExclGroup(g) => {
+                if let Some(name) = &g.common.name {
+                    out.insert(clean_name(name));
+                }
+                // Its members are drawn as fields with their own names (`draw_field`), so a
+                // repeat drawn before the group must not take one.
+                for f in &g.fields {
+                    if let Some(name) = &f.common.name {
+                        out.insert(clean_name(name));
+                    }
+                }
+            }
+            Node::Subform(s) => {
+                collect_names(&s.children, out, depth + 1);
+                if let Some(ps) = &s.page_set {
+                    for a in &ps.areas {
+                        collect_names(&a.items, out, depth + 1);
+                    }
+                }
+            }
+            Node::Area(a) => collect_names(&a.children, out, depth + 1),
+            Node::Draw(_) => {}
+        }
+    }
+}
+
 /// `path` with the instance numbers `scope` gives the same names, segment by segment from the
 /// root, up to where the two part: the instance of a repeating subform the text is laid out in.
 pub(super) fn instance_path(path: &DataPath, scope: &DataPath) -> DataPath {

crates/xfa/src/layout/draw.rs

@@ -356,7 +356,7 @@ impl Layouter<'_> {
         let selected: Option<String> =
             self.data.and_then(|d| d.text_at(&data_path(self.tpl, &ctx.som))).map(|t| t.trim().to_string()).filter(|t| !t.is_empty());
         let inner = rect.inset(&g.common.margin);
-        let group = self.unique_name(g.common.name.as_deref().unwrap_or("group"));
+        let group = self.unique_name(g.common.name.as_deref().unwrap_or("group"), g.common.name.is_none());
         let nodes: Vec<Node> = g.fields.iter().map(|f| Node::Field(Box::new(f.clone()))).collect();
         // Each field is measured under the SOM index it is drawn at.
         let sibs = SomIndices::of(&nodes);
@@ -433,12 +433,26 @@ impl Layouter<'_> {
         self.push(Item::Image { rect: fitted, data: Arc::new(data.to_vec()), content_type: content_type.to_string() })
     }
 
-    /// A field name made from `base` that no earlier field of the form has: the first keeps it,
-    /// later ones get `_2`, `_3`, ….
-    pub(super) fn unique_name(&mut self, base: &str) -> String {
+    /// A field name made from `base` that no other field of the form has: the first keeps it,
+    /// later ones get `_2`, `_3`, …, skipping a name another field already has or that the
+    /// template gives another field (a template field named `Cell4_2` keeps it, and the second
+    /// `Cell4` becomes `Cell4_3`). A `fallback` base, the name an unnamed field or group gets
+    /// (`field`, `group`), is made up too: it doesn't take a name the template gives another
+    /// field either, and a template field named `field` still gets it.
+    pub(super) fn unique_name(&mut self, base: &str, fallback: bool) -> String {
         let base = clean_name(base);
-        let n = self.names.entry(base.clone()).or_insert(0);
-        *n += 1;
-        if *n == 1 { base } else { format!("{base}_{n}") }
+        // The template's own name, while no other field has it.
+        if !fallback && self.taken.insert(base.clone()) {
+            return base;
+        }
+        loop {
+            let n = self.names.entry(base.clone()).or_insert(0);
+            *n = n.saturating_add(1);
+            let candidate = if *n == 1 { base.clone() } else { format!("{base}_{n}") };
+            let reserved = (fallback || *n > 1) && self.template_names.contains(&candidate);
+            if !reserved && self.taken.insert(candidate.clone()) {
+                return candidate;
+            }
+        }
     }
 }

crates/xfa/src/layout/field.rs

@@ -157,7 +157,7 @@ impl Layouter<'_> {
             (Ui::Button, Some(cap)) => ctx.para.apply(&cap.para).h_align,
             _ => fctx.para.h_align,
         };
-        let name = self.unique_name(f.common.name.as_deref().unwrap_or("field"));
+        let name = self.unique_name(f.common.name.as_deref().unwrap_or("field"), f.common.name.is_none());
         let som = som_path();
         let read_only = matches!(f.access, Access::ReadOnly | Access::Protected);
         // The data's value wins over the template's default.

crates/xfa/src/tests.rs

@@ -5666,6 +5666,87 @@ fn a_split_lr_tb_subform_keeps_the_border_of_the_section_round_it() {
     assert!(form.warnings.is_empty(), "{:?}", form.warnings);
 }
 
+/// Two PDF fields never share a name: a made-up `_2`, `_3` name skips one the template gives
+/// another field. Whichever comes first in layout order, the field the template names `Cell4_2`
+/// keeps that name and the second `Cell4` takes the next free one. Before, the second `Cell4`
+/// became `Cell4_2` as well, so a script writing by that name hit the wrong cell.
+#[test]
+fn a_made_up_field_name_never_takes_one_the_template_uses() {
+    for rows in [["Cell4", "Cell4_2", "Cell4"], ["Cell4", "Cell4", "Cell4_2"]] {
+        let body: String = rows
+            .iter()
+            .enumerate()
+            .map(|(i, name)| {
+                format!(r#"<subform name="row{i}" layout="tb"><field name="{name}" w="2in" h="0.3in"><ui><textEdit/></ui></field></subform>"#)
+            })
+            .collect();
+        let form = layout_xml(&one_area_form(r#"layout="tb""#, &body)).unwrap();
+        let all = widgets(&form.pages[0]);
+        let names: Vec<&str> = all.iter().map(|w| w.name.as_str()).collect();
+        let mut unique = names.clone();
+        unique.sort_unstable();
+        unique.dedup();
+        assert_eq!(unique.len(), names.len(), "{rows:?}: {names:?}");
+        let template_named = rows.iter().position(|n| *n == "Cell4_2").unwrap();
+        let som = format!("form[0].row{template_named}[0].Cell4_2[0]");
+        let got = all.iter().find(|w| w.som == som).map(|w| w.name.as_str());
+        assert_eq!(got, Some("Cell4_2"), "{rows:?}: the template's own name is kept ({names:?})");
+    }
+}
+
+/// #5225 review: a member of an exclusion group keeps the name the template gives it, whatever
+/// is drawn before it: a repeated field drawn first doesn't take it as its made-up name.
+#[test]
+fn a_made_up_field_name_never_takes_an_exclusion_group_members_name() {
+    let field = |name: &str| format!(r#"<field name="{name}" w="2in" h="0.3in"><ui><textEdit/></ui></field>"#);
+    let group = r#"<exclGroup name="Choice" layout="tb"><field name="Cell4_2" w="1in" h="0.3in"><ui><checkButton shape="round"/></ui><items><text>A</text></items></field><field name="Other" w="1in" h="0.3in"><ui><checkButton shape="round"/></ui><items><text>B</text></items></field></exclGroup>"#;
+    // The repeated field first, then the group; and the other way round.
+    for body in [format!("{}{}{group}", field("Cell4"), field("Cell4")), format!("{group}{}{}", field("Cell4"), field("Cell4"))] {
+        let form = layout_xml(&one_area_form(r#"layout="tb""#, &body)).unwrap();
+        let all = widgets(&form.pages[0]);
+        let shown: Vec<(String, String)> = all.iter().map(|w| (w.som.clone(), w.name.clone())).collect();
+        let member = all.iter().find(|w| w.som.ends_with(".Choice[0].Cell4_2[0]")).map(|w| w.name.as_str());
+        assert_eq!(member, Some("Cell4_2"), "the member keeps its name: {shown:?}");
+        let mut names: Vec<&str> = all.iter().map(|w| w.name.as_str()).collect();
+        names.sort_unstable();
+        names.dedup();
+        assert_eq!(names.len(), all.len(), "no two widgets share a name: {shown:?}");
+    }
+}
+
+/// #5225 review: a form whose fields don't repeat keeps every template name as it is, a name
+/// that looks like a made-up one (`Total_2`) included, in every subform. Reserving the template's
+/// names must not push a field off its own.
+#[test]
+fn a_form_without_repeats_keeps_every_template_name() {
+    let names = ["Name", "Total", "Total_2", "Date"];
+    let body: String = names
+        .iter()
+        .enumerate()
+        .map(|(i, name)| {
+            format!(r#"<subform name="part{i}" layout="tb"><field name="{name}" w="2in" h="0.3in"><ui><textEdit/></ui></field></subform>"#)
+        })
+        .collect();
+    let form = layout_xml(&one_area_form(r#"layout="tb""#, &body)).unwrap();
+    let got: Vec<String> = widgets(&form.pages[0]).iter().map(|w| w.name.clone()).collect();
+    assert_eq!(got, names, "the template's names, unchanged");
+}
+
+/// An unnamed field's made-up name (`field`) doesn't take the name a later field has in the
+/// template: the field the template names `field` keeps it, and the unnamed one before it takes
+/// the next free name.
+#[test]
+fn an_unnamed_field_never_takes_a_name_the_template_uses() {
+    let body = r#"<field w="2in" h="0.3in"><ui><textEdit/></ui></field>
+<field name="field" w="2in" h="0.3in"><ui><textEdit/></ui></field>"#;
+    let form = layout_xml(&one_area_form(r#"layout="tb""#, body)).unwrap();
+    let all = widgets(&form.pages[0]);
+    // In layout order: the unnamed field, then the one the template names `field`.
+    let names: Vec<&str> = all.iter().map(|w| w.name.as_str()).collect();
+    assert_eq!(names.get(1), Some(&"field"), "{names:?}");
+    assert_ne!(names.first(), Some(&"field"), "{names:?}");
+}
+
 /// A form with `draws` in a subform placed by position, on a letter page whose content area is a
 /// quarter inch in, with `master` on the page itself.
 fn form_of_draws(master: &str, draws: &str) -> String {