From 975d5407a1058a3af455be40b6774d3de54edfaa Mon Sep 17 00:00:00 2001 From: prxnvt <53838599+prxnvt@users.noreply.github.com> Date: Mon, 2 Nov 2026 02:13:19 -0500 Subject: [PATCH] Form controls: a kept button action must carry the button's name and anchor cell An entry of the hidden name without `name` or `at`, or with something other than text there, skipped those checks (is_none_or) and could pin its action onto whatever button sat at the stored index. The writer always stores both keys, so such an entry is now dropped like an unparsable one (is_some_and); the doc comment says so. Test: a_kept_action_without_the_buttons_name_or_anchor_is_dropped (an entry without `name`, without `at`, with a number for `name` and with null for `at` gives the button no action and the hidden name is removed; the writer's own entry still comes back). --- crates/engine/src/cmd/controls.rs | 16 ++++++------ crates/engine/src/tests/controls.rs | 38 +++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 8 deletions(-) diff --git a/crates/engine/src/cmd/controls.rs b/crates/engine/src/cmd/controls.rs index 773262480..0e03f8f9f 100644 --- a/crates/engine/src/cmd/controls.rs +++ b/crates/engine/src/cmd/controls.rs @@ -571,7 +571,7 @@ pub(crate) fn sync_linked_values(wb: &mut Workbook) { // ---------------------------------------------------------------- xlsx /// The hidden-data key of the button actions xlsx has no place for. -const ACTIONS_KEY: &str = "control_actions"; +pub(crate) const ACTIONS_KEY: &str = "control_actions"; /// A button's action that xlsx carries itself: a macro on the selection (`macro="[0]!Name"`). fn plain(a: &ControlAction) -> bool { @@ -604,11 +604,11 @@ pub(crate) fn with_action_names(wb: &Workbook) -> Result> { } /// After a file is opened: gives buttons back the actions kept in the hidden name, and removes -/// it. Entries that don't parse, or don't match a button, are dropped: the button at the stored -/// place must have the stored name and anchor cell (another application may have added, removed -/// or reordered controls), and must not have been given another macro since (a stored macro -/// action comes back only on a button that still runs that macro, a stored command only on one -/// the file gives no macro). +/// it. Entries that don't parse, or don't match a button, are dropped: an entry must carry the +/// button's name and anchor cell, the button at the stored place must have that name and anchor +/// cell (another application may have added, removed or reordered controls), and it must not +/// have been given another macro since (a stored macro action comes back only on a button that +/// still runs that macro, a stored command only on one the file gives no macro). pub(crate) fn load_action_names(wb: &mut Workbook) { let Some(items) = stash::get_json(wb, ACTIONS_KEY) else { return }; stash::remove(wb, ACTIONS_KEY); @@ -624,8 +624,8 @@ pub(crate) fn load_action_names(wb: &mut Workbook) { let text = |k: &str| item.get(k).and_then(Json::as_str); let same = |c: &FormControl| { c.kind == ControlKind::Button - && text("name").is_none_or(|n| n == c.display_name()) - && text("at").is_none_or(|a| a == c.anchor.cell.a1()) + && text("name").is_some_and(|n| n == c.display_name()) + && text("at").is_some_and(|a| a == c.anchor.cell.a1()) && match (&c.action, &action) { (None, ControlAction::Command { .. }) => true, (Some(ControlAction::Macro { name: now, .. }), ControlAction::Macro { name: kept, .. }) => now.eq_ignore_ascii_case(kept), diff --git a/crates/engine/src/tests/controls.rs b/crates/engine/src/tests/controls.rs index 8dce29fcd..e567fb913 100644 --- a/crates/engine/src/tests/controls.rs +++ b/crates/engine/src/tests/controls.rs @@ -429,6 +429,44 @@ fn a_kept_button_action_comes_back_only_on_the_same_button() { assert_eq!(cleared.sheets[0].controls[0].action, actions(&wb)[0], "the command comes back"); } +/// An entry of the hidden name must carry the button's name and anchor cell (the writer always +/// stores both): a hand-edited entry without either, or with something else there, is dropped +/// like an unparsable one, so it cannot pin its action onto whatever button sits at that index. +#[test] +fn a_kept_action_without_the_buttons_name_or_anchor_is_dropped() { + use crate::cmd::controls::{ACTIONS_KEY, load_action_names, with_action_names}; + use crate::cmd::stash; + let mut s = s(); + s.execute("insert.button", json!({"at": "D2", "text": "Run", "command": "cell.set", "params": {"cell": "A9", "input": "cmd"}})).unwrap(); + let stored = with_action_names(&s.doc().unwrap().wb).unwrap().unwrap(); + let items = stash::get_json(&stored, ACTIONS_KEY).unwrap(); + assert!(items[0]["name"].is_string() && items[0]["at"].is_string(), "the writer stores both keys: {items}"); + // As the file brings a command button back: with no action. + let kept = |items: &Json| { + let mut wb = stored.clone(); + wb.sheet_mut(0).unwrap().controls[0].action = None; + stash::remove(&mut wb, ACTIONS_KEY); + stash::set_json(&mut wb, ACTIONS_KEY, items).unwrap(); + load_action_names(&mut wb); + assert!(stash::get(&wb, ACTIONS_KEY).is_none(), "the hidden name is removed"); + wb.sheets[0].controls[0].action.clone() + }; + let edited = |key: &str, value: Option| { + let mut items = items.clone(); + let entry = items[0].as_object_mut().unwrap(); + match value { + Some(v) => entry.insert(key.to_string(), v), + None => entry.remove(key), + }; + items + }; + assert_eq!(kept(&edited("name", None)), None, "no `name`"); + assert_eq!(kept(&edited("at", None)), None, "no `at`"); + assert_eq!(kept(&edited("name", Some(json!(1)))), None, "`name` is not text"); + assert_eq!(kept(&edited("at", Some(Json::Null))), None, "`at` is null"); + assert!(matches!(kept(&items), Some(ControlAction::Command { .. })), "the writer's own entry still comes back"); +} + /// The saved xlsx bytes of `s`'s workbook. fn saved_xlsx(s: &mut Session) -> Vec { let r = s.execute("file.saveBytes", json!({"format": "xlsx"})).unwrap();