mirror of
https://github.com/lahfir/agent-desktop.git
synced 2026-08-16 03:56:16 +00:00
fix: make action-bearing elements ref-able so scroll/expand can target them
E2E testing against a diverse fixture app surfaced that disclosure (Expand/Collapse/Click) and scrollarea (Scroll) advertise actions but never received refs — they are not in INTERACTIVE_ROLES — so the scroll, expand, and collapse commands required a <REF> their own target roles could never have. The commands were uninvokable against their primary targets. Ref allocation now gates on addressability, not role alone: an element is ref-able if its role is interactive OR it advertises a primary action (any action other than a bare SetFocus, which would ref-allocate inert focusable containers). scrollarea and disclosure become ref-able; scroll now works against a real app. Ref-count impact is modest (fixture 61->72, Finder ~262). Tests assert action-bearing containers get refs, SetFocus-only and inert elements do not, and interactive roles stay ref-able without actions. Contract docs (CLAUDE.md, SKILL.md) updated.
This commit is contained in:
parent
9c31e19345
commit
b2f14fcbf5
4 changed files with 60 additions and 11 deletions
|
|
@ -310,8 +310,9 @@ Error responses:
|
|||
## Ref System
|
||||
|
||||
- Refs allocated in depth-first document order: `@e1`, `@e2`, etc.
|
||||
- Only interactive roles receive refs: `button`, `textfield`, `checkbox`, `link`, `menuitem`, `tab`, `slider`, `combobox`, `treeitem`, `cell`
|
||||
- Static text, groups, containers do NOT get refs (they remain in tree for context)
|
||||
- An element receives a ref when it is **addressable for an action**: its role is interactive (`button`, `textfield`, `checkbox`, `link`, `menuitem`, `tab`, `slider`, `combobox`, `treeitem`, `cell`, `radiobutton`, `switch`, `colorwell`, `menubutton`, `incrementor`, `dockitem`), **or** it advertises an available action regardless of role. Container roles such as `scrollarea` (Scroll) and `disclosure` (Expand/Collapse/Click) are not interactive by role but are genuinely actionable, so they are ref-able — `scroll` / `expand` / `collapse` need a ref to target them
|
||||
- A bare `SetFocus` affordance does not qualify on its own (focusability is not a primary action), so inert focusable containers stay ref-less
|
||||
- Static text and non-actionable groups/containers do NOT get refs (they remain in tree for context)
|
||||
- Refs are deterministic within a snapshot but NOT stable across snapshots if UI changed
|
||||
- Snapshot refs are stored by snapshot ID under `~/.agent-desktop/snapshots/{snapshot_id}/refmap.json`, with a `latest_snapshot_id` pointer for commands that omit `--snapshot`
|
||||
- `~/.agent-desktop/last_refmap.json` is written only as a latest-snapshot inspection artifact; command code must use `RefStore`
|
||||
|
|
@ -387,7 +388,7 @@ pub trait PlatformAdapter {
|
|||
|
||||
### Unit Tests (core)
|
||||
- `AccessibilityNode` ser/de roundtrips
|
||||
- Ref allocator only assigns interactive roles
|
||||
- Ref allocator assigns interactive roles and action-bearing elements (not `SetFocus`-only)
|
||||
- `SnapshotEngine` filtering
|
||||
- Error serialization
|
||||
- MockAdapter: in-memory `PlatformAdapter` returning hardcoded trees
|
||||
|
|
|
|||
|
|
@ -37,6 +37,24 @@ pub(crate) fn ref_entry_from_node(
|
|||
}
|
||||
}
|
||||
|
||||
/// An element receives a ref when it is addressable for an action: either its
|
||||
/// role is interactive, or it advertises an available action regardless of
|
||||
/// role. Container roles like `scrollarea` (Scroll) and `disclosure`
|
||||
/// (Expand/Collapse) are not "interactive" by role but are genuinely
|
||||
/// actionable, and `scroll` / `expand` / `collapse` need a ref to target
|
||||
/// them — so action-bearing elements must be ref-able. A bare `SetFocus`
|
||||
/// affordance does not qualify on its own: focusability is not a primary
|
||||
/// action and would ref-allocate large numbers of inert containers.
|
||||
pub(crate) fn is_ref_able(node: &AccessibilityNode) -> bool {
|
||||
INTERACTIVE_ROLES.contains(&node.role.as_str()) || advertises_primary_action(node)
|
||||
}
|
||||
|
||||
fn advertises_primary_action(node: &AccessibilityNode) -> bool {
|
||||
node.available_actions
|
||||
.iter()
|
||||
.any(|action| action != crate::capability::SET_FOCUS)
|
||||
}
|
||||
|
||||
pub(crate) fn is_collapsible(node: &AccessibilityNode) -> bool {
|
||||
node.ref_id.is_none()
|
||||
&& node.name.as_deref().is_none_or(str::is_empty)
|
||||
|
|
@ -118,9 +136,9 @@ fn allocate_refs_at_path(
|
|||
config: &RefAllocConfig,
|
||||
path: &mut Vec<usize>,
|
||||
) -> AccessibilityNode {
|
||||
let is_interactive = INTERACTIVE_ROLES.contains(&node.role.as_str());
|
||||
let is_ref_able = is_ref_able(&node);
|
||||
|
||||
if is_interactive {
|
||||
if is_ref_able {
|
||||
let mut entry = ref_entry_from_node(
|
||||
&node,
|
||||
config.pid,
|
||||
|
|
@ -138,10 +156,8 @@ fn allocate_refs_at_path(
|
|||
|
||||
let has_label = node.name.as_deref().is_some_and(|n| !n.is_empty())
|
||||
|| node.description.as_deref().is_some_and(|d| !d.is_empty());
|
||||
let is_skeleton_anchor = !is_interactive
|
||||
&& node.children_count.is_some()
|
||||
&& has_label
|
||||
&& config.root_ref_id.is_none();
|
||||
let is_skeleton_anchor =
|
||||
!is_ref_able && node.children_count.is_some() && has_label && config.root_ref_id.is_none();
|
||||
|
||||
if is_skeleton_anchor {
|
||||
let mut entry = ref_entry_from_node(
|
||||
|
|
|
|||
|
|
@ -115,6 +115,37 @@ fn ref_entry_preserves_meaningful_identity_text() {
|
|||
assert_eq!(entry.description.as_deref(), Some("Commits changes"));
|
||||
}
|
||||
|
||||
/// scrollarea/disclosure are not interactive roles, but they advertise real
|
||||
/// actions and `scroll` / `expand` need a ref to target them.
|
||||
#[test]
|
||||
fn actionable_container_roles_receive_refs() {
|
||||
let mut scroll = node("scrollarea", Some("Log"));
|
||||
scroll.available_actions = vec!["Scroll".into()];
|
||||
assert!(is_ref_able(&scroll));
|
||||
|
||||
let mut disclosure = node("disclosure", Some("Details"));
|
||||
disclosure.available_actions = vec!["Click".into()];
|
||||
assert!(is_ref_able(&disclosure));
|
||||
}
|
||||
|
||||
/// A bare SetFocus affordance is not a primary action; ref-allocating every
|
||||
/// focusable container would bloat the refmap.
|
||||
#[test]
|
||||
fn focus_only_container_does_not_receive_a_ref() {
|
||||
let mut group = node("group", Some("Panel"));
|
||||
group.available_actions = vec!["SetFocus".into()];
|
||||
assert!(!is_ref_able(&group));
|
||||
|
||||
let inert = node("statictext", Some("Label"));
|
||||
assert!(!is_ref_able(&inert));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn interactive_role_is_ref_able_even_without_actions() {
|
||||
let button = node("button", Some("OK"));
|
||||
assert!(is_ref_able(&button));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn allocate_refs_records_structural_paths() {
|
||||
let mut root = node("window", Some("w"));
|
||||
|
|
|
|||
|
|
@ -83,9 +83,10 @@ Use **progressive skeleton traversal** as the default approach. It reduces token
|
|||
## Ref System
|
||||
|
||||
- Refs assigned depth-first: `@e1`, `@e2`, `@e3`...
|
||||
- Only interactive elements get refs: button, textfield, checkbox, link, menuitem, tab, slider, combobox, treeitem, cell
|
||||
- An element gets a ref when it is addressable for an action: an interactive role (button, textfield, checkbox, link, menuitem, tab, slider, combobox, treeitem, cell, radiobutton, switch, ...) **or** any element advertising an action — so `scrollarea` (Scroll) and `disclosure` (Expand/Collapse) are ref-able and `scroll`/`expand`/`collapse` can target them
|
||||
- A `SetFocus`-only affordance does not earn a ref on its own
|
||||
- In skeleton mode, named/described containers at truncation boundary also get refs (drill-down targets with empty `available_actions`)
|
||||
- Static text, groups, containers remain in tree for context but have no ref
|
||||
- Static text and non-actionable groups/containers remain in tree for context but have no ref
|
||||
- Refs are deterministic within a snapshot but NOT stable across snapshots if UI changed
|
||||
- Every snapshot returns `snapshot_id`; ref-consuming commands accept `--snapshot <snapshot_id>`, and explicit snapshot IDs do not require also passing `--session`
|
||||
- `last_refmap.json` is only a latest-snapshot inspection artifact. The command path uses snapshot-scoped storage.
|
||||
|
|
|
|||
Loading…
Reference in a new issue