From a9ddf79a19bae076d8c10d4f0c9e7784e99ce094 Mon Sep 17 00:00:00 2001 From: KenM76 Date: Tue, 27 Oct 2026 08:05:28 -0400 Subject: [PATCH] M6: a radio button renamed to a radio group's name joins that group (#4949) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Renaming a radio button to an existing radio group's name was refused as a duplicate, so buttons placed separately could never become one group. set_props now moves the renamed field's buttons into the named group (ISO 32000-2 §12.7.5.2.4: a group is one field whose widgets are its buttons). A clashing export value is renamed to the first free ChoiceN in /AP /N, /D and /R; a moved button is on only if its value is the group's value; merged field+widgets are handled on both sides (the target split via split_merged, now shared with duplicate_field). Refused when either group has /Opt, has no button, or the target is locked. Adapted from pdfcer (MIT, same author) 79870b56 and 116f48d3. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01SqmHYx8FiWM5PcsoiErgp9 --- crates/automation/tests/it/automation.rs | 23 +++++ crates/forms/src/author.rs | 1 + crates/forms/src/author/edit.rs | 23 ++++- crates/forms/src/author/join.rs | 114 +++++++++++++++++++++ crates/forms/src/author/lifecycle.rs | 72 ++++++------- crates/forms/src/tests.rs | 124 +++++++++++++++++++++++ parity/acrobat-features.toml | 4 +- 7 files changed, 322 insertions(+), 39 deletions(-) create mode 100644 crates/forms/src/author/join.rs diff --git a/crates/automation/tests/it/automation.rs b/crates/automation/tests/it/automation.rs index 23351db9aa..04b7241798 100644 --- a/crates/automation/tests/it/automation.rs +++ b/crates/automation/tests/it/automation.rs @@ -4004,6 +4004,29 @@ fn preparing_a_form_through_tools() { assert!(matches!(a.call("form_set_image", &json!({ "doc": doc, "field": "size", "path": "pics/a_page_1.png" })), Err(ToolError::Failed(_)))); } +#[test] +fn renaming_a_radio_button_to_a_groups_name_joins_the_group_through_tools() { + // #4949: two buttons placed separately are two groups; naming one after the other makes + // them one group, and a clashing export value is renamed instead of shared. + let dir = workdir("radio-join"); + let mut a = auto(&dir); + let doc = ok(&mut a, "doc_open", json!({ "path": "a.pdf" }))["doc"].as_u64().unwrap(); + ok(&mut a, "form_add_field", json!({ "doc": doc, "page": 1, "type": "radio", "rect": [20, 100, 34, 114], "group": "size", "export": "S" })); + ok(&mut a, "form_add_field", json!({ "doc": doc, "page": 1, "type": "radio", "rect": [40, 100, 54, 114], "group": "other", "export": "S" })); + ok(&mut a, "form_fill", json!({ "doc": doc, "values": { "size": "S" } })); + let r = ok(&mut a, "form_set_props", json!({ "doc": doc, "field": "other", "name": "size" })); + assert_eq!(r["field"], "size"); + ok(&mut a, "doc_save", json!({ "doc": doc, "path": "joined.pdf" })); + let doc = ok(&mut a, "doc_open", json!({ "path": "joined.pdf" }))["doc"].as_u64().unwrap(); + let fields = ok(&mut a, "form_fields", json!({ "doc": doc })); + let f = fields["fields"].as_array().unwrap(); + assert_eq!(f.iter().map(|f| f["name"].as_str().unwrap()).collect::>(), ["size"], "one group"); + assert_eq!(f[0]["options"], json!(["S", "Choice1"]), "the joining button takes a free export value"); + assert_eq!(f[0]["value"], "S", "the group keeps its selection"); + ok(&mut a, "form_fill", json!({ "doc": doc, "values": { "size": "Choice1" } })); + assert_eq!(ok(&mut a, "form_fields", json!({ "doc": doc }))["fields"][0]["value"], "Choice1"); +} + #[test] fn rotating_a_field_through_tools() { let dir = workdir("rotate-field"); diff --git a/crates/forms/src/author.rs b/crates/forms/src/author.rs index ce89fad806..5de3e9b3e7 100644 --- a/crates/forms/src/author.rs +++ b/crates/forms/src/author.rs @@ -11,6 +11,7 @@ use pdfcraft_cos::{Dict, Document, ObjRef, Object, PdfString, Stream}; use crate::{Field, FieldKind, FormError, Widget, appearance, fields, flags, page_refs}; mod edit; +mod join; mod lifecycle; mod look; mod tree; diff --git a/crates/forms/src/author/edit.rs b/crates/forms/src/author/edit.rs index dc7d91d4fc..1b8caee92a 100644 --- a/crates/forms/src/author/edit.rs +++ b/crates/forms/src/author/edit.rs @@ -1,3 +1,4 @@ +use super::join::{check_join, join_radio_group}; use super::look::{BorderStyle, da_string, patch_da, patch_widget_look, redraw_widgets}; use super::tree::{attach, check_place, child_named, clash, detach, ensure_form, inherited, name_parts, own_inherited}; use super::*; @@ -71,6 +72,9 @@ pub fn set_props(doc: &mut Document, name: &str, props: &FieldProps) -> Result Result own, _ => n, }; - if n.contains('.') { + let full = match f.name.rsplit_once('.') { + Some((parents, _)) if !n.contains('.') => format!("{parents}.{n}"), + _ => n.to_string(), + }; + let group = all.iter().find(|x| x.obj != f.obj && x.kind == FieldKind::Radio && x.name == full); + if let Some(group) = group.filter(|_| f.kind == FieldKind::Radio) { + check_join(doc, &f, group)?; + join = Some(group.clone()); + new_name = full; + } else if n.contains('.') { // A full name: the field moves under the parent fields it names (#2696). let parts = name_parts(n)?; if let Some(why) = all.iter().filter(|x| x.obj != f.obj).find_map(|x| clash(n, &x.name)) { @@ -385,7 +398,13 @@ pub fn set_props(doc: &mut Document, name: &str, props: &FieldProps) -> Result bool { + doc.get(field).as_dict().is_some_and(|d| d.contains(b"Opt")) +} + +/// Refuse what [`join_radio_group`] can't do, before anything is written. +pub(super) fn check_join(doc: &Document, f: &Field, target: &Field) -> Result<(), FormError> { + for x in [f, target] { + if has_opt(doc, x.obj) { + return invalid(format!("{} lists its export values in /Opt, so its buttons can't move between groups", x.name)); + } + if x.widgets.is_empty() { + return invalid(format!("{} has no button", x.name)); + } + } + if target.locked() { + return invalid(format!("{} is locked: unlock it first", target.name)); + } + Ok(()) +} + +/// Move every button of radio field `f` into radio field `target`. Each keeps its export value +/// unless a button of `target` already has it; then it takes the first free "ChoiceN", as a +/// button added to a group does. A moved button shows selected only if it is `target`'s +/// selected value. `f` leaves the form; its own value and field-level properties go with it, +/// and `target`'s stay. Call [`check_join`] first. +pub(super) fn join_radio_group(doc: &mut Document, f: &Field, target: &Field) -> Result<(), FormError> { + // A group that is its own one button gets a field with a separate widget first, so it can + // hold more than one. + if let [only] = target.widgets.as_slice() + && only.obj == target.obj + { + split_merged(doc, target, only)?; + } + let mut taken: HashSet = target.widgets.iter().filter_map(|w| w.on_state.clone()).collect(); + let selected = target.value.first().cloned(); + detach(doc, f.obj)?; + let moved: HashSet = f.widgets.iter().map(|w| w.obj).collect(); + drop_from_calculation_order(doc, |r| r == f.obj && !moved.contains(&r))?; + let mut kids = Vec::with_capacity(f.widgets.len()); + for w in &f.widgets { + let mut d = doc.get(w.obj).as_dict().cloned().unwrap_or_default(); + if w.obj == f.obj { + for k in FIELD_ONLY_KEYS { + d.remove(k); + } + } + let mut state = w.on_state.clone(); + if let Some(old) = w.on_state.as_deref().filter(|s| taken.contains(*s)) { + let fresh = (1..=MAX_FRESH_STATES).map(|i| format!("Choice{i}")).find(|c| !taken.contains(c)); + if let Some(fresh) = fresh + && rename_state(doc, &mut d, old, &fresh) + { + state = Some(fresh); + } + } + if let Some(s) = &state { + taken.insert(s.clone()); + } + let on = state.is_some() && state == selected; + let shown = match (&state, on) { + (Some(s), true) => Object::Name(crate::name_bytes(s)), + _ => Object::name("Off"), + }; + d.set(b"AS".to_vec(), shown); + d.set(b"Parent".to_vec(), Object::Ref(target.obj)); + doc.set(w.obj, Object::Dict(d)); + kids.push(Object::Ref(w.obj)); + } + doc.update_dict(target.obj, |d| { + let mut list = d.get(b"Kids").and_then(|k| k.as_array().cloned()).unwrap_or_default(); + list.extend(kids); + d.set(b"Kids".to_vec(), Object::Array(list)); + })?; + Ok(()) +} + +/// Rename the appearance state `old` to `new` in the widget dictionary `d` (`/AP` `/N`, `/D`, +/// `/R`). A shared `/AP` or state dictionary is copied into `d` rather than renamed for its other +/// users. Whether `/N` had the state: when it didn't, nothing is changed. +fn rename_state(doc: &Document, d: &mut Dict, old: &str, new: &str) -> bool { + let (old, new) = (crate::name_bytes(old), crate::name_bytes(new)); + let Some(mut ap) = d.get(b"AP").and_then(|a| doc.resolve(a).as_dict().cloned()) else { return false }; + let states = |ap: &Dict, key: &[u8]| ap.get(key).and_then(|s| doc.resolve(s).as_dict().cloned()); + if !states(&ap, b"N").is_some_and(|n| n.contains(&old)) { + return false; + } + for key in [&b"N"[..], b"D", b"R"] { + let Some(mut sub) = states(&ap, key) else { continue }; + if let Some(v) = sub.remove(&old) { + sub.set(new.clone(), v); + ap.set(key.to_vec(), Object::Dict(sub)); + } + } + d.set(b"AP".to_vec(), Object::Dict(ap)); + true +} diff --git a/crates/forms/src/author/lifecycle.rs b/crates/forms/src/author/lifecycle.rs index 745ab3b341..c62b9bde2c 100644 --- a/crates/forms/src/author/lifecycle.rs +++ b/crates/forms/src/author/lifecycle.rs @@ -215,7 +215,7 @@ fn delete_found_field(doc: &mut Document, f: Field) -> Result<(), FormError> { /// Remove the `/CO` entries `gone` matches, whether the form and the array are held directly or /// by reference. -fn drop_from_calculation_order(doc: &mut Document, gone: impl Fn(ObjRef) -> bool) -> Result<(), FormError> { +pub(super) fn drop_from_calculation_order(doc: &mut Document, gone: impl Fn(ObjRef) -> bool) -> Result<(), FormError> { let Some(root) = doc.root() else { return Ok(()) }; let Some(af_obj) = doc.get(root).as_dict().and_then(|d| d.get(b"AcroForm").cloned()) else { return Ok(()) }; let Some(mut af) = doc.resolve(&af_obj).as_dict().cloned() else { return Ok(()) }; @@ -258,40 +258,7 @@ pub fn duplicate_field(doc: &mut Document, name: &str, pages: &[usize]) -> Resul return Ok(0); } // A merged field/widget becomes a field with one widget kid. - let mut model_ref = model.obj; - if model.obj == f.obj { - let d = doc.get(f.obj).as_dict().cloned().unwrap_or_default(); - let mut kid = Dict::new(); - for k in WIDGET_KEYS { - if let Some(v) = d.get(k) { - kid.set(k.to_vec(), v.clone()); - } - } - kid.set(b"Parent".to_vec(), Object::Ref(f.obj)); - let kr = doc.add(Object::Dict(kid)); - doc.update_dict(f.obj, |d| { - for k in WIDGET_KEYS { - d.remove(k); - } - d.set(b"Kids".to_vec(), Object::Array(vec![Object::Ref(kr)])); - })?; - // The page lists the widget now, not the field. - if let Some(p) = model.page { - let pr = refs[p]; - let annots = doc.get(pr).as_dict().and_then(|d| d.get(b"Annots").cloned()); - let swap = - |a: Vec| -> Vec { a.into_iter().map(|o| if o.as_ref() == Some(f.obj) { Object::Ref(kr) } else { o }).collect() }; - match annots { - Some(Object::Ref(ar)) => { - let a = doc.get(ar).as_array().cloned().unwrap_or_default(); - doc.set(ar, Object::Array(swap(a))); - } - Some(Object::Array(a)) => doc.update_dict(pr, |d| d.set(b"Annots".to_vec(), Object::Array(swap(a))))?, - _ => {} - } - } - model_ref = kr; - } + let model_ref = if model.obj == f.obj { split_merged(doc, &f, &model)? } else { model.obj }; let template = doc.get(model_ref).as_dict().cloned().unwrap_or_default(); let mut added = Vec::new(); for p in &targets { @@ -324,3 +291,38 @@ pub fn duplicate_field(doc: &mut Document, name: &str, pages: &[usize]) -> Resul redraw_field(doc, name)?; Ok(added.len()) } + +/// Split field `f`, which is also its only widget `model`, into a field and one widget kid that +/// takes the widget's keys and its place in the page's `/Annots`. Returns the kid. +pub(super) fn split_merged(doc: &mut Document, f: &Field, model: &Widget) -> Result { + let refs = page_refs(doc); + let d = doc.get(f.obj).as_dict().cloned().unwrap_or_default(); + let mut kid = Dict::new(); + for k in WIDGET_KEYS { + if let Some(v) = d.get(k) { + kid.set(k.to_vec(), v.clone()); + } + } + kid.set(b"Parent".to_vec(), Object::Ref(f.obj)); + let kr = doc.add(Object::Dict(kid)); + doc.update_dict(f.obj, |d| { + for k in WIDGET_KEYS { + d.remove(k); + } + d.set(b"Kids".to_vec(), Object::Array(vec![Object::Ref(kr)])); + })?; + // The page lists the widget now, not the field. + if let Some(&pr) = model.page.and_then(|p| refs.get(p)) { + let annots = doc.get(pr).as_dict().and_then(|d| d.get(b"Annots").cloned()); + let swap = |a: Vec| -> Vec { a.into_iter().map(|o| if o.as_ref() == Some(f.obj) { Object::Ref(kr) } else { o }).collect() }; + match annots { + Some(Object::Ref(ar)) => { + let a = doc.get(ar).as_array().cloned().unwrap_or_default(); + doc.set(ar, Object::Array(swap(a))); + } + Some(Object::Array(a)) => doc.update_dict(pr, |d| d.set(b"Annots".to_vec(), Object::Array(swap(a))))?, + _ => {} + } + } + Ok(kr) +} diff --git a/crates/forms/src/tests.rs b/crates/forms/src/tests.rs index 61dab131e0..d2c9fedb32 100644 --- a/crates/forms/src/tests.rs +++ b/crates/forms/src/tests.rs @@ -4577,3 +4577,127 @@ fn set_values_reads_the_form_once_not_per_value() { let reads = form_reads() - before; assert_eq!(reads, 3, "one to fill, one when f0 comes again, one to recalculate"); } + +/// Two new radio groups on one page, "Group1" (export `a`) and "Group2" (export `b`). +fn two_radio_groups(a: &str, b: &str) -> Document { + let mut doc = one_page(); + let radio = |export: &str| NewField::Radio { group: None, export: export.into() }; + assert_eq!(add_field(&mut doc, 0, [50.0, 600.0, 64.0, 614.0], &radio(a), None).unwrap(), "Group1"); + assert_eq!(add_field(&mut doc, 0, [80.0, 600.0, 94.0, 614.0], &radio(b), None).unwrap(), "Group2"); + doc +} + +fn rename_to(doc: &mut Document, from: &str, to: &str) -> Result { + set_props(doc, from, &FieldProps { name: Some(to.into()), ..FieldProps::default() }) +} + +fn page_widgets(doc: &Document) -> Vec { + let page = page_refs(doc)[0]; + let annots = doc.get(page).as_dict().unwrap().get(b"Annots").cloned().unwrap(); + doc.resolve(&annots).as_array().unwrap().iter().filter_map(|o| o.as_ref()).collect() +} + +/// #4949: a radio button given its group's name is one of the group's buttons, so the group can +/// be built up by naming buttons alike, as in Acrobat. +#[test] +fn a_radio_button_renamed_to_a_groups_name_joins_the_group() { + let mut doc = two_radio_groups("Yes", "No"); + set_value(&mut doc, "Group1", &FieldValue::Radio(Some("Yes".into()))).unwrap(); + let before = page_widgets(&doc); + assert_eq!(rename_to(&mut doc, "Group2", "Group1").unwrap(), "Group1"); + assert_eq!(page_widgets(&doc), before, "the buttons stay on the page where they were"); + let mut doc = reopen(&doc); + let all = fields(&doc); + assert_eq!(all.iter().map(|f| f.name.as_str()).collect::>(), ["Group1"]); + let group = field(&all, "Group1"); + assert_eq!(group.widgets.iter().filter_map(|w| w.on_state.clone()).collect::>(), ["Yes", "No"]); + assert_eq!(group.value, ["Yes"], "the group keeps its value"); + assert_eq!(group.widgets.iter().map(|w| w.state.clone()).collect::>(), [Some("Yes".into()), Some("Off".into())]); + assert_eq!(page_widgets(&doc), group.widgets.iter().map(|w| w.obj).collect::>()); + // Choosing the moved button turns the other off: they are one group. + set_value(&mut doc, "Group1", &FieldValue::Radio(Some("No".into()))).unwrap(); + let group = field(&fields(&doc), "Group1").clone(); + assert_eq!(group.widgets.iter().map(|w| w.state.clone()).collect::>(), [Some("Off".into()), Some("No".into())]); +} + +/// A joining button whose export value a button of the group already has takes the next free +/// "ChoiceN", in its appearance states too, so the two can be chosen apart. +#[test] +fn a_joining_radio_button_with_a_taken_export_value_takes_a_free_one() { + let mut doc = two_radio_groups("Choice1", "Choice1"); + rename_to(&mut doc, "Group2", "Group1").unwrap(); + let doc = reopen(&doc); + let all = fields(&doc); + let group = field(&all, "Group1"); + assert_eq!(group.widgets.iter().filter_map(|w| w.on_state.clone()).collect::>(), ["Choice1", "Choice2"]); + let moved = doc.get(group.widgets[1].obj).as_dict().unwrap().clone(); + let normal = doc.resolve(moved.get(b"AP").unwrap()).as_dict().unwrap().get(b"N").cloned().unwrap(); + let normal = doc.resolve(&normal).as_dict().unwrap().clone(); + assert!(normal.contains(b"Choice2") && !normal.contains(b"Choice1")); +} + +/// A radio field that is its own (only) widget, as other writers make one-button groups, can +/// both join a group and be joined. +#[test] +fn radio_fields_that_are_their_own_widget_join_and_are_joined() { + let mut doc = one_page(); + let page = page_refs(&doc)[0]; + let af = crate::author::ensure_form(&mut doc).unwrap(); + let mut made = Vec::new(); + for (name, state, x) in [("solo", "A", 50.0), ("other", "B", 80.0)] { + let look = doc.add(Object::Stream(pdfcraft_cos::Stream::from_raw(pdfcraft_cos::Dict::new(), b"0 g".to_vec()))); + let mut n = pdfcraft_cos::Dict::new(); + n.set(state.as_bytes().to_vec(), Object::Ref(look)); + n.set(b"Off".to_vec(), Object::Ref(look)); + let mut ap = pdfcraft_cos::Dict::new(); + ap.set(b"N".to_vec(), Object::Dict(n)); + let mut d = pdfcraft_cos::Dict::new(); + d.set(b"Type".to_vec(), Object::name("Annot")); + d.set(b"Subtype".to_vec(), Object::name("Widget")); + d.set(b"FT".to_vec(), Object::name("Btn")); + d.set(b"Ff".to_vec(), Object::Int(i64::from(flags::RADIO))); + d.set(b"T".to_vec(), pdfcraft_cos::PdfString::text(name)); + d.set(b"Rect".to_vec(), Object::Array(vec![x.into(), 600.into(), (x + 14.0).into(), 614.into()])); + d.set(b"P".to_vec(), Object::Ref(page)); + d.set(b"AP".to_vec(), Object::Dict(ap)); + d.set(b"AS".to_vec(), Object::name("Off")); + let r = doc.add(Object::Dict(d)); + doc.update_dict(af, |a| { + let mut list = a.get(b"Fields").and_then(|l| l.as_array().cloned()).unwrap_or_default(); + list.push(Object::Ref(r)); + a.set(b"Fields".to_vec(), Object::Array(list)); + }) + .unwrap(); + made.push(r); + } + doc.update_dict(page, |p| p.set(b"Annots".to_vec(), Object::Array(made.iter().map(|r| Object::Ref(*r)).collect()))).unwrap(); + assert_eq!(rename_to(&mut doc, "other", "solo").unwrap(), "solo"); + let doc = reopen(&doc); + let all = fields(&doc); + assert_eq!(all.iter().map(|f| f.name.as_str()).collect::>(), ["solo"]); + let group = field(&all, "solo"); + assert_eq!(group.widgets.iter().filter_map(|w| w.on_state.clone()).collect::>(), ["A", "B"]); + // The joined field's widget became a kid; the moved one shed its field entries. + let kids: Vec = group.widgets.iter().map(|w| w.obj).collect(); + assert!(!kids.contains(&group.obj)); + let moved = doc.get(group.widgets[1].obj).as_dict().unwrap().clone(); + assert!(!moved.contains(b"T") && !moved.contains(b"FT") && moved.reference(b"Parent") == Some(group.obj)); + let mut on_page = page_widgets(&doc); + on_page.sort(); + let mut kids = kids; + kids.sort(); + assert_eq!(on_page, kids, "the page lists the widgets, not the field"); +} + +/// Only a radio button joins by name: other clashes are refused as before, and a locked group +/// isn't changed. +#[test] +fn joining_by_name_is_only_for_radio_buttons_and_unlocked_groups() { + let mut doc = two_radio_groups("Yes", "No"); + add_field(&mut doc, 0, [50.0, 700.0, 250.0, 720.0], &NewField::Text { multiline: false }, Some("note")).unwrap(); + assert!(rename_to(&mut doc, "note", "Group1").is_err(), "a text field doesn't join a radio group"); + assert!(rename_to(&mut doc, "Group2", "note").is_err(), "a radio button doesn't join a text field"); + set_props(&mut doc, "Group1", &FieldProps { locked: Some(true), ..FieldProps::default() }).unwrap(); + assert!(rename_to(&mut doc, "Group2", "Group1").is_err(), "a locked group"); + assert_eq!(fields(&doc).len(), 3); +} diff --git a/parity/acrobat-features.toml b/parity/acrobat-features.toml index bcba7a0ecd..497a6878a7 100644 --- a/parity/acrobat-features.toml +++ b/parity/acrobat-features.toml @@ -4847,8 +4847,8 @@ milestone = "M6" status = "shipped" commands = ["form.add.radio"] tools = ["form_add_field"] -evidence = ["crates/forms/src/tests.rs::every_field_type_can_be_added_named_and_drawn", "crates/automation/tests/it/automation.rs::preparing_a_form_through_tools"] -notes = "new group Group1… or join the selected group with the next ChoiceN" +evidence = ["crates/forms/src/tests.rs::every_field_type_can_be_added_named_and_drawn", "crates/automation/tests/it/automation.rs::preparing_a_form_through_tools", "crates/forms/src/tests.rs::a_radio_button_renamed_to_a_groups_name_joins_the_group", "crates/forms/src/tests.rs::a_joining_radio_button_with_a_taken_export_value_takes_a_free_one", "crates/forms/src/tests.rs::radio_fields_that_are_their_own_widget_join_and_are_joined", "crates/automation/tests/it/automation.rs::renaming_a_radio_button_to_a_groups_name_joins_the_group_through_tools"] +notes = "new group Group1… or join the selected group with the next ChoiceN; renaming a radio button (Properties or form_set_props) to another radio group's name moves its buttons into that group (#4949), a clashing export value becoming the next free ChoiceN; refused when either group lists its values in /Opt; a button's export value is not yet editable in Properties" [[feature]] id = "form.check-box-style"