diff --git a/crates/xfa/src/layout.rs b/crates/xfa/src/layout.rs index 73c6bde90f..076ebce118 100644 --- a/crates/xfa/src/layout.rs +++ b/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, Widget, WidgetBorder, WidgetKind}; @@ -136,6 +136,11 @@ struct Layouter<'t> { pages: Vec, cur: Option, names: HashMap, + /// Every PDF field name handed out so far. + taken: HashSet, + /// The names the template's fields and groups take as written ([`common::collect_names`]), + /// which a made-up `_2`, `_3` name must not take. + template_names: HashSet, /// Field values by `id`, for embedded fields in rich text. values: HashMap, warnings: Vec, @@ -183,6 +188,8 @@ pub fn layout(tpl: &Template, data: Option<&DataNode>) -> Result // 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())); collect_values(std::slice::from_ref(&root_node), &mut values, 0); + let mut template_names = HashSet::new(); + collect_names(std::slice::from_ref(&root_node), &mut template_names, 0); let mut l = Layouter { tpl, data, @@ -191,6 +198,8 @@ pub fn layout(tpl: &Template, data: Option<&DataNode>) -> Result pages: Vec::new(), cur: None, names: HashMap::new(), + taken: HashSet::new(), + template_names, values, warnings: Vec::new(), warned: HashSet::new(), diff --git a/crates/xfa/src/layout/common.rs b/crates/xfa/src/layout/common.rs index 625f305a6b..de855219e3 100644 --- a/crates/xfa/src/layout/common.rs +++ b/crates/xfa/src/layout/common.rs @@ -1,5 +1,5 @@ use super::items::{Action, Item, Rounded}; -use super::{HashMap, Layout, MAX_DEPTH, Node, Rect, Script, date_letters, iso_to_pattern}; +use super::{HashMap, HashSet, Layout, MAX_DEPTH, Node, Rect, Script, date_letters, iso_to_pattern}; pub(super) fn collect_values(nodes: &[Node], out: &mut HashMap, depth: usize) { if depth > MAX_DEPTH { @@ -26,6 +26,39 @@ pub(super) fn collect_values(nodes: &[Node], out: &mut HashMap, } } +/// The PDF field names the template's fields and exclusion groups 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 PDF fields would share it. +pub(super) fn collect_names(nodes: &[Node], out: &mut HashSet, 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)); + } + } + 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(_) => {} + } + } +} + pub(super) fn clamp_opt(v: f64, min: Option, max: Option) -> f64 { let mut v = v; if let Some(m) = min { diff --git a/crates/xfa/src/layout/draw.rs b/crates/xfa/src/layout/draw.rs index f6520c1628..b71e5114e8 100644 --- a/crates/xfa/src/layout/draw.rs +++ b/crates/xfa/src/layout/draw.rs @@ -316,12 +316,20 @@ 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`, …. + /// 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`). pub(super) fn unique_name(&mut self, base: &str) -> 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}") } + 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 = *n > 1 && self.template_names.contains(&candidate); + if !reserved && self.taken.insert(candidate.clone()) { + return candidate; + } + } } } diff --git a/crates/xfa/src/tests.rs b/crates/xfa/src/tests.rs index ca129abceb..e6b562290f 100644 --- a/crates/xfa/src/tests.rs +++ b/crates/xfa/src/tests.rs @@ -4089,3 +4089,31 @@ fn clear_password_values_clears_static_xfa_password_fields() { let packets = read_packets(&doc).unwrap().unwrap(); assert!(!packets.xdp.contains("hunter2"), "{}", packets.xdp); } + +/// 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#""#) + }) + .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:?})"); + } +}