mirror of
https://github.com/lahfir/agent-desktop.git
synced 2026-08-04 13:16:06 +00:00
fix: unify accessible-name computation across builder and resolver
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.
This commit is contained in:
parent
952eec407a
commit
1246ef4acb
7 changed files with 104 additions and 98 deletions
|
|
@ -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<String>,
|
||||
#[serde(skip_serializing_if = "Option::is_none")]
|
||||
pub static_role_value: Option<String>,
|
||||
#[serde(skip_serializing_if = "Option::is_none")]
|
||||
pub description: Option<String>,
|
||||
}
|
||||
|
|
@ -1,4 +1,3 @@
|
|||
pub mod accname;
|
||||
pub mod action;
|
||||
pub mod action_request;
|
||||
pub mod action_result;
|
||||
|
|
|
|||
|
|
@ -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<String>) -> Option<S
|
|||
}
|
||||
}
|
||||
|
||||
/// One-owner accessible-name reduction shared by the snapshot builder (which
|
||||
/// stores a ref's name) and `element::resolve_element_name` (which recomputes
|
||||
/// it during strict re-resolution), so a stored name always equals what the
|
||||
/// resolver recomputes. Precedence: the element's own title, then description,
|
||||
/// then a static-text value promoted to a name, then a label aggregated from
|
||||
/// descendant text — each trimmed and treated as absent when blank.
|
||||
#[cfg(target_os = "macos")]
|
||||
pub(crate) fn accessible_name(
|
||||
el: &AXElement,
|
||||
attrs: &super::node_attrs::NodeAttrs,
|
||||
) -> Option<String> {
|
||||
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<String> {
|
||||
non_empty(title)
|
||||
.or_else(|| non_empty(description))
|
||||
.or_else(|| non_empty(static_value))
|
||||
}
|
||||
|
||||
fn non_empty(text: Option<&str>) -> Option<String> {
|
||||
text.filter(|value| !value.trim().is_empty())
|
||||
.map(str::to_string)
|
||||
}
|
||||
|
||||
pub fn label_from_children(children: &[AXElement]) -> Option<String> {
|
||||
#[cfg(target_os = "macos")]
|
||||
{
|
||||
|
|
@ -295,17 +331,6 @@ pub fn label_from_children(children: &[AXElement]) -> Option<String> {
|
|||
}
|
||||
}
|
||||
|
||||
#[cfg(target_os = "macos")]
|
||||
fn label_from_child_attrs(el: &AXElement, ax_role: Option<&str>) -> Option<String> {
|
||||
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<Vec<AXElement>> {
|
||||
for attr in child_attributes(ax_role) {
|
||||
|
|
|
|||
|
|
@ -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 ")
|
||||
);
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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<String> {
|
||||
let evidence = super::name_evidence::name_evidence_impl(el);
|
||||
let non_empty = |text: Option<String>| 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<String> {
|
||||
None
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
|
|
|
|||
|
|
@ -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<String> {
|
||||
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;
|
||||
Loading…
Reference in a new issue