diff --git a/crates/xfa/src/layout.rs b/crates/xfa/src/layout.rs index e62c76dcc3..2e61338b67 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, Turned, Widget, WidgetBorder, WidgetKind}; @@ -168,6 +168,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, groups and groups' members 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, @@ -222,6 +227,8 @@ pub fn layout(tpl: &Template, data: Option<&DataNode>) -> Result 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 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 cee2b99e4e..1952bc7982 100644 --- a/crates/xfa/src/layout/common.rs +++ b/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, 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 { diff --git a/crates/xfa/src/layout/draw.rs b/crates/xfa/src/layout/draw.rs index ba00f85a28..b25e1ddb66 100644 --- a/crates/xfa/src/layout/draw.rs +++ b/crates/xfa/src/layout/draw.rs @@ -356,7 +356,7 @@ impl Layouter<'_> { let selected: Option = 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 = 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; + } + } } } diff --git a/crates/xfa/src/layout/field.rs b/crates/xfa/src/layout/field.rs index bcf6466a6b..9558ee27be 100644 --- a/crates/xfa/src/layout/field.rs +++ b/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. diff --git a/crates/xfa/src/tests.rs b/crates/xfa/src/tests.rs index 3598cfb26f..6356f4892d 100644 --- a/crates/xfa/src/tests.rs +++ b/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#""#) + }) + .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#""#); + let group = r#"AB"#; + // 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#""#) + }) + .collect(); + let form = layout_xml(&one_area_form(r#"layout="tb""#, &body)).unwrap(); + let got: Vec = 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#" +"#; + 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 {