From 1246ef4acbf68909728687571bf60fec5bfa9dba Mon Sep 17 00:00:00 2001 From: Lahfir Date: Fri, 3 Jul 2026 20:39:32 -0700 Subject: [PATCH] fix: unify accessible-name computation across builder and resolver MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Code review found the STALE_REF name-divergence class was re-introduced: the snapshot builder stores a ref's name via its own chain (title -> description -> static value -> label_from_children child text), but resolve_element_name — used by strict ref re-resolution — dropped the child-label rung and trimmed blanks differently. So an interactive element named only by descendant text (Finder / Mail / System Settings sidebar cells) or by a whitespace/blank title stored one name and recomputed another, failing identity_matches -> STALE_REF on click/type/get. Confirmed: 5/5 Finder sidebar cells returned STALE_REF. - One shared reducer `builder::accessible_name` (title -> description -> static-text value -> aggregated child label, each trimmed and blank-as-absent), with the own-text portion factored into the pure, unit-testable `reduce_text_name`. Both the snapshot builder and resolve_element_name reduce through it, so a stored ref name always equals what the resolver recomputes. - Deleted the now-single-producer/single-consumer NameEvidence indirection (crates/core/src/accname.rs, crates/macos/src/tree/name_evidence.rs) and the now-dead label_from_child_attrs. - Added reduce_text_name unit tests covering the rung precedence and the blank/whitespace handling that accname_tests used to guard. Verified: 5/5 Finder sidebar cells now re-resolve, e2e 71/0, clippy clean, workspace tests green. --- crates/core/src/accname.rs | 20 -------- crates/core/src/lib.rs | 1 - crates/macos/src/tree/builder.rs | 67 ++++++++++++++++++-------- crates/macos/src/tree/builder_tests.rs | 45 ++++++++++++++++- crates/macos/src/tree/element.rs | 26 +++++----- crates/macos/src/tree/mod.rs | 1 - crates/macos/src/tree/name_evidence.rs | 42 ---------------- 7 files changed, 104 insertions(+), 98 deletions(-) delete mode 100644 crates/core/src/accname.rs delete mode 100644 crates/macos/src/tree/name_evidence.rs diff --git a/crates/core/src/accname.rs b/crates/core/src/accname.rs deleted file mode 100644 index ada3220..0000000 --- a/crates/core/src/accname.rs +++ /dev/null @@ -1,20 +0,0 @@ -use serde::{Deserialize, Serialize}; - -/// Raw accessible-name evidence an adapter gathers for one element: its own -/// title, its description, and — for static-text roles only — its value -/// promoted to a name. Each platform's `resolve_element_name` reduces this to a -/// single accessible name; the macOS precedence is title → description → -/// static value. Keeping the evidence typed (rather than returning a bare -/// `String`) keeps every name consumer — the snapshot builder, strict ref -/// re-resolution, hit-test occluder naming, and ambiguity classification — -/// reducing the *same* evidence the same way, so a stored ref name always -/// matches what the resolver recomputes. -#[derive(Debug, Clone, Default, Serialize, Deserialize, PartialEq, Eq)] -pub struct NameEvidence { - #[serde(skip_serializing_if = "Option::is_none")] - pub native_title: Option, - #[serde(skip_serializing_if = "Option::is_none")] - pub static_role_value: Option, - #[serde(skip_serializing_if = "Option::is_none")] - pub description: Option, -} diff --git a/crates/core/src/lib.rs b/crates/core/src/lib.rs index 484503c..4dae396 100644 --- a/crates/core/src/lib.rs +++ b/crates/core/src/lib.rs @@ -1,4 +1,3 @@ -pub mod accname; pub mod action; pub mod action_request; pub mod action_result; diff --git a/crates/macos/src/tree/builder.rs b/crates/macos/src/tree/builder.rs index 2491662..6fbaf3a 100644 --- a/crates/macos/src/tree/builder.rs +++ b/crates/macos/src/tree/builder.rs @@ -91,8 +91,8 @@ pub fn build_subtree( .unwrap_or("unknown") .to_string(); let is_secure_text = is_secure_text_role(attrs.role.as_deref()); + let name = accessible_name(el, &attrs); let value = redact_secure_value(attrs.role.as_deref(), attrs.value); - let name = attrs.title.or(attrs.description); let child_count = count_children(el, attrs.role.as_deref()); let bounds = context.bounds_for(attrs.bounds); let mut states = Vec::new(); @@ -136,19 +136,13 @@ pub fn build_subtree( platform_available_actions(el, &role, attrs.has_scrollbars) }; - let name = promoted_label.or_else(|| attrs.title.clone().or_else(|| attrs.description.clone())); + let name = promoted_label.or_else(|| accessible_name(el, &attrs)); let description = if attrs.title.is_some() { attrs.description.clone() } else { None }; - let name = if name.is_none() && attrs.role.as_deref() == Some("AXStaticText") { - value.clone().or(name) - } else { - name - }; - let state_ctx = super::state_reader::StateReaderContext { focused: context.focused.as_ref(), window_bounds: context.window_bounds, @@ -177,7 +171,6 @@ pub fn build_subtree( } else { None }; - let name = name.or_else(|| label_from_child_attrs(el, attrs.role.as_deref())); ancestors.remove(&ptr_key); return Some(AccessibilityNode { ref_id: None, @@ -196,7 +189,6 @@ pub fn build_subtree( } let children_raw = copy_children(el, attrs.role.as_deref()).unwrap_or_default(); - let name = name.or_else(|| label_from_children(&children_raw)); let child_window_bounds = if attrs.role.as_deref() == Some("AXWindow") { attrs.bounds.or(context.window_bounds) @@ -254,6 +246,50 @@ fn redact_secure_value(ax_role: Option<&str>, value: Option) -> Option Option { + let ax_role = attrs.role.as_deref(); + let static_value = if ax_role == Some("AXStaticText") { + attrs.value.as_deref() + } else { + None + }; + reduce_text_name( + attrs.title.as_deref(), + attrs.description.as_deref(), + static_value, + ) + .or_else(|| label_from_children(©_children(el, ax_role).unwrap_or_default())) +} + +/// The own-text portion of [`accessible_name`] (title -> description -> +/// static-text value), factored out pure and platform-agnostic so the +/// precedence and blank/whitespace handling are unit-testable without a live +/// AX element. +pub(crate) fn reduce_text_name( + title: Option<&str>, + description: Option<&str>, + static_value: Option<&str>, +) -> Option { + non_empty(title) + .or_else(|| non_empty(description)) + .or_else(|| non_empty(static_value)) +} + +fn non_empty(text: Option<&str>) -> Option { + text.filter(|value| !value.trim().is_empty()) + .map(str::to_string) +} + pub fn label_from_children(children: &[AXElement]) -> Option { #[cfg(target_os = "macos")] { @@ -295,17 +331,6 @@ pub fn label_from_children(children: &[AXElement]) -> Option { } } -#[cfg(target_os = "macos")] -fn label_from_child_attrs(el: &AXElement, ax_role: Option<&str>) -> Option { - for attr in child_attributes(ax_role) { - let children = copy_ax_array_prefix(el, attr, 5).unwrap_or_default(); - if let Some(label) = label_from_children(&children) { - return Some(label); - } - } - None -} - #[cfg(target_os = "macos")] fn copy_children(el: &AXElement, ax_role: Option<&str>) -> Option> { for attr in child_attributes(ax_role) { diff --git a/crates/macos/src/tree/builder_tests.rs b/crates/macos/src/tree/builder_tests.rs index 31c93d0..cb2c4f9 100644 --- a/crates/macos/src/tree/builder_tests.rs +++ b/crates/macos/src/tree/builder_tests.rs @@ -1,5 +1,5 @@ use super::{ - child_attributes, redact_secure_value, window_titles_are_exact_match, + child_attributes, redact_secure_value, reduce_text_name, window_titles_are_exact_match, window_titles_are_partial_match, }; @@ -52,3 +52,46 @@ fn window_title_matching_accepts_exact_and_truncated_titles() { "noy4/agent-desktop: Native desk" )); } + +#[test] +fn reduce_text_name_prefers_title_then_description_then_value() { + assert_eq!( + reduce_text_name(Some("T"), Some("D"), Some("V")).as_deref(), + Some("T") + ); + assert_eq!( + reduce_text_name(None, Some("D"), Some("V")).as_deref(), + Some("D") + ); + assert_eq!( + reduce_text_name(None, None, Some("V")).as_deref(), + Some("V") + ); + assert_eq!(reduce_text_name(None, None, None), None); +} + +/// Guards the STALE_REF divergence class: a blank or whitespace-only title must +/// fall through to the next rung so a stored ref name (computed by the builder +/// through this same reducer) equals what strict re-resolution recomputes. +#[test] +fn reduce_text_name_treats_blank_and_whitespace_as_absent() { + assert_eq!( + reduce_text_name(Some(""), Some("D"), None).as_deref(), + Some("D") + ); + assert_eq!( + reduce_text_name(Some(" "), Some("D"), None).as_deref(), + Some("D") + ); + assert_eq!( + reduce_text_name(Some("\t "), None, Some("V")).as_deref(), + Some("V") + ); + assert_eq!(reduce_text_name(Some(" "), Some(" "), Some(" ")), None); + // Real content with surrounding whitespace is preserved verbatim (matches + // how the builder stores it), so stored and recomputed names still agree. + assert_eq!( + reduce_text_name(Some(" Recents "), None, None).as_deref(), + Some(" Recents ") + ); +} diff --git a/crates/macos/src/tree/element.rs b/crates/macos/src/tree/element.rs index 8a6a8f0..a5a2c03 100644 --- a/crates/macos/src/tree/element.rs +++ b/crates/macos/src/tree/element.rs @@ -305,19 +305,21 @@ mod imp { pub use imp::{count_children, element_for_pid, fetch_node_attrs}; -/// The element's accessible name: its own title, else its description, else -/// (for static text only) its value promoted to a name. This is the single -/// source of truth every name consumer shares — the snapshot builder, strict -/// ref re-resolution, hit-test occluder naming, and ambiguity classification — -/// so a ref's stored name always matches what the resolver recomputes. The -/// per-platform `name_evidence` supplier gathers the raw attributes; this -/// reducer owns the precedence. +/// The element's accessible name, computed by the one shared reducer +/// [`super::builder::accessible_name`] (title -> description -> static-text +/// value -> aggregated child label, each trimmed and blank-as-absent). The +/// snapshot builder stores a ref's name through the same reducer, so strict ref +/// re-resolution here always recomputes exactly what was stored — the single +/// source of truth every name consumer shares (builder, strict resolver, +/// hit-test occluder naming, ambiguity classification). +#[cfg(target_os = "macos")] pub fn resolve_element_name(el: &super::ax_element::AXElement) -> Option { - let evidence = super::name_evidence::name_evidence_impl(el); - let non_empty = |text: Option| text.filter(|value| !value.trim().is_empty()); - non_empty(evidence.native_title) - .or_else(|| non_empty(evidence.description)) - .or_else(|| non_empty(evidence.static_role_value)) + super::builder::accessible_name(el, &fetch_node_attrs(el)) +} + +#[cfg(not(target_os = "macos"))] +pub fn resolve_element_name(_el: &super::ax_element::AXElement) -> Option { + None } #[cfg(test)] diff --git a/crates/macos/src/tree/mod.rs b/crates/macos/src/tree/mod.rs index 437947d..e6b53f8 100644 --- a/crates/macos/src/tree/mod.rs +++ b/crates/macos/src/tree/mod.rs @@ -9,7 +9,6 @@ pub mod element; pub mod element_bounds; pub(crate) mod element_dedupe; pub mod hit_test; -pub mod name_evidence; pub mod native_id; pub(crate) mod node_attrs; pub mod resolve; diff --git a/crates/macos/src/tree/name_evidence.rs b/crates/macos/src/tree/name_evidence.rs deleted file mode 100644 index b5921e0..0000000 --- a/crates/macos/src/tree/name_evidence.rs +++ /dev/null @@ -1,42 +0,0 @@ -#[cfg(target_os = "macos")] -mod imp { - use crate::tree::AXElement; - use crate::tree::attributes::copy_string_attr; - use accessibility_sys::{ - kAXDescriptionAttribute, kAXRoleAttribute, kAXTitleAttribute, kAXValueAttribute, - }; - use agent_desktop_core::accname::NameEvidence; - - const STATIC_TEXT_ROLE: &str = "AXStaticText"; - - /// Gathers raw `NameEvidence` from an element's own AX attributes: its - /// title, its description, and (for static text only) its value. Precedence - /// between them is `resolve_element_name`'s job, never decided here. - pub fn name_evidence_impl(el: &AXElement) -> NameEvidence { - let ax_role = copy_string_attr(el, kAXRoleAttribute); - NameEvidence { - native_title: copy_string_attr(el, kAXTitleAttribute), - static_role_value: static_role_value(el, ax_role.as_deref()), - description: copy_string_attr(el, kAXDescriptionAttribute), - } - } - - fn static_role_value(el: &AXElement, ax_role: Option<&str>) -> Option { - if ax_role != Some(STATIC_TEXT_ROLE) { - return None; - } - copy_string_attr(el, kAXValueAttribute) - } -} - -#[cfg(not(target_os = "macos"))] -mod imp { - use crate::tree::AXElement; - use agent_desktop_core::accname::NameEvidence; - - pub fn name_evidence_impl(_el: &AXElement) -> NameEvidence { - NameEvidence::default() - } -} - -pub use imp::name_evidence_impl;