From 9f51879012186fbfce338b2adb2c0be0a92c6a68 Mon Sep 17 00:00:00 2001 From: "jin.2" Date: Sun, 5 Apr 2026 23:10:24 +0900 Subject: [PATCH] fix: rewrite getByRole to use CDP accessibility tree with ref-based element resolution (#1145) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: rewrite getByRole to use CDP accessibility tree instead of CSS selectors The old `handle_getbyrole` generated `querySelectorAll('[role="link"], link')` which matched `` stylesheet elements instead of `` anchor tags. This happened because ARIA role names were used directly as CSS tag selectors, and several roles differ from their HTML element names (e.g. link → a, heading → h1-h6, textbox → input/textarea). The fix replaces the JS-based DOM query with the CDP `Accessibility.getFullAXTree` API, where the browser engine correctly computes implicit ARIA roles per the WAI-ARIA / HTML-AAM spec. This is the same approach already used by `snapshot.rs` and `element.rs` in this codebase. Changes: - Rewrite `handle_getbyrole` to query the browser's accessibility tree via CDP - Add `find_ax_node_by_role` helper for AX tree traversal with role/name/exact matching - Use `DOM.resolveNode` + `Runtime.callFunctionOn` to bridge AX node → DOM marker - Add iframe support via `resolve_ax_session` (missing in old implementation) - Fix cleanup to use correct CDP session (old code used default session, breaking iframe cleanup) - Export `extract_ax_string` as `pub(super)` for reuse - Add 4 regression tests for `find_ax_node_by_role` Fixes #1123 * style: apply cargo fmt * chore: remove redundant comments * refactor: replace marker attribute with temporary ref for element resolution Eliminates 3 CDP round-trips (DOM.resolveNode, Runtime.callFunctionOn, Runtime.evaluate cleanup) by registering a temporary ref in the ref_map. execute_subaction resolves the element via backendNodeId directly. No more DOM pollution with marker attributes. * fix: ref counter collision, ref_map leak, and stale fallback name - Increment next_ref_num after inserting temp ref to prevent id collision - Remove temp ref after execute_subaction to prevent unbounded ref_map growth - Return actual AX name from find_ax_node_by_role for accurate fallback resolution - Add RefMap::remove method --------- Co-authored-by: hyunjinee --- cli/src/native/actions.rs | 212 +++++++++++++++++++++++++++----------- cli/src/native/element.rs | 6 +- 2 files changed, 158 insertions(+), 60 deletions(-) diff --git a/cli/src/native/actions.rs b/cli/src/native/actions.rs index d16a871..e571db0 100644 --- a/cli/src/native/actions.rs +++ b/cli/src/native/actions.rs @@ -5212,78 +5212,90 @@ async fn handle_getbyrole(cmd: &Value, state: &mut DaemonState) -> Result {{ - const els = document.querySelectorAll('[role="{role}"], {role}'); - for (const el of els) {{ - if ({name_match}) {{ - el.setAttribute('data-agent-browser-located', 'true'); - return true; - }} - }} - return false; - }})()"#, - role = role, - name_match = name_match, + // Query the accessibility tree via CDP — the browser engine is the + // authoritative source for implicit ARIA roles (e.g. → "link"). + let (ax_params, effective_session_id) = super::element::resolve_ax_session( + state.active_frame_id.as_deref(), + &session_id, + &state.iframe_sessions, ); - let result: super::cdp::types::EvaluateResult = mgr + let ax_tree: super::cdp::types::GetFullAXTreeResult = mgr .client .send_command_typed( - "Runtime.evaluate", - &super::cdp::types::EvaluateParams { - expression: js, - return_by_value: Some(true), - await_promise: Some(false), - }, - Some(&session_id), + "Accessibility.getFullAXTree", + &ax_params, + Some(effective_session_id), ) .await?; - if !result - .result - .value - .as_ref() - .and_then(|v| v.as_bool()) - .unwrap_or(false) - { - let desc = build_role_selector(role, name, exact); - return Err(format!("No element found: {}", desc)); - } + let (backend_node_id, actual_name) = find_ax_node_by_role(&ax_tree.nodes, role, name, exact)?; - let selector = "[data-agent-browser-located='true']"; - let result = execute_subaction(cmd, state, selector).await; + // Register a temporary ref so execute_subaction can resolve the element + // via backendNodeId directly — no marker attribute needed. + let ref_num = state.ref_map.next_ref_num(); + let temp_ref = format!("e{}", ref_num); + state.ref_map.add_with_frame( + temp_ref.clone(), + Some(backend_node_id), + role, + &actual_name, + None, + state.active_frame_id.as_deref(), + ); + state.ref_map.set_next_ref_num(ref_num + 1); - // Clean up the marker attribute - if let Some(ref browser) = state.browser { - if browser.active_session_id().is_ok() { - let _ = browser - .evaluate( - "document.querySelector('[data-agent-browser-located]')?.removeAttribute('data-agent-browser-located')", - None, + let result = execute_subaction(cmd, state, &format!("@{}", temp_ref)).await; + state.ref_map.remove(&temp_ref); + result +} + +/// Search the accessibility tree for a node matching the given role and +/// optional name. Returns `(backendDOMNodeId, actual_name)` of the first match. +fn find_ax_node_by_role( + nodes: &[super::cdp::types::AXNode], + role: &str, + name: Option<&str>, + exact: bool, +) -> Result<(i64, String), String> { + for node in nodes { + if node.ignored.unwrap_or(false) { + continue; + } + + let node_role = super::element::extract_ax_string(&node.role); + if node_role != role { + continue; + } + + let node_name = super::element::extract_ax_string(&node.name); + + let Some(target_name) = name else { + let id = node + .backend_d_o_m_node_id + .ok_or_else(|| format!("AX node has no backendDOMNodeId for role={}", role))?; + return Ok((id, node_name)); + }; + + let matches = if exact { + node_name == target_name + } else { + node_name.contains(target_name) + }; + + if matches { + let id = node.backend_d_o_m_node_id.ok_or_else(|| { + format!( + "AX node has no backendDOMNodeId for role={} name={}", + role, target_name ) - .await; + })?; + return Ok((id, node_name)); } } - result + let desc = build_role_selector(role, name, exact); + Err(format!("No element found: {}", desc)) } async fn handle_semantic_locator( @@ -8488,4 +8500,86 @@ mod tests { assert!(!auto_handled, "{dialog_type} should NOT be auto-handled"); } } + + use super::super::cdp::types::{AXNode, AXValue}; + + fn make_ax_node( + node_id: &str, + role: &str, + name: &str, + backend_node_id: Option, + ignored: bool, + ) -> AXNode { + AXNode { + node_id: node_id.to_string(), + role: Some(AXValue { + value_type: "role".to_string(), + value: Some(serde_json::Value::String(role.to_string())), + }), + name: Some(AXValue { + value_type: "computedString".to_string(), + value: Some(serde_json::Value::String(name.to_string())), + }), + value: None, + description: None, + properties: None, + child_ids: None, + backend_d_o_m_node_id: backend_node_id, + ignored: Some(ignored), + } + } + + #[test] + fn test_find_ax_node_by_role_matches_link_role() { + // Regression: the old implementation used querySelectorAll('link') + // which matched stylesheet elements instead of anchors. + // The AX tree correctly assigns role="link" to . + let nodes = vec![ + make_ax_node("1", "WebArea", "Page", Some(1), false), + make_ax_node("2", "link", "Example Link", Some(42), false), + make_ax_node("3", "link", "Another Link", Some(43), false), + ]; + + let (id, name) = find_ax_node_by_role(&nodes, "link", Some("Example Link"), true).unwrap(); + assert_eq!(id, 42); + assert_eq!(name, "Example Link"); + } + + #[test] + fn test_find_ax_node_by_role_exact_vs_contains() { + let nodes = vec![ + make_ax_node("1", "link", "More information...", Some(10), false), + make_ax_node("2", "link", "Less info", Some(11), false), + ]; + + assert!(find_ax_node_by_role(&nodes, "link", Some("More"), true).is_err()); + + let (id, _) = find_ax_node_by_role(&nodes, "link", Some("More"), false).unwrap(); + assert_eq!(id, 10); + } + + #[test] + fn test_find_ax_node_by_role_no_name_filter() { + let nodes = vec![ + make_ax_node("1", "heading", "", Some(5), false), + make_ax_node("2", "button", "Submit", Some(6), false), + ]; + + let (id, _) = find_ax_node_by_role(&nodes, "button", None, false).unwrap(); + assert_eq!(id, 6); + } + + #[test] + fn test_find_ax_node_by_role_skips_ignored_nodes() { + let nodes = vec![ + make_ax_node("1", "link", "Hidden Link", Some(99), true), // ignored + make_ax_node("2", "link", "Visible Link", Some(100), false), + ]; + + let result = find_ax_node_by_role(&nodes, "link", Some("Hidden Link"), true); + assert!(result.is_err()); + + let (id, _) = find_ax_node_by_role(&nodes, "link", Some("Visible Link"), true).unwrap(); + assert_eq!(id, 100); + } } diff --git a/cli/src/native/element.rs b/cli/src/native/element.rs index 11853bb..7b22ed9 100644 --- a/cli/src/native/element.rs +++ b/cli/src/native/element.rs @@ -103,6 +103,10 @@ impl RefMap { entries } + pub fn remove(&mut self, ref_id: &str) { + self.map.remove(ref_id); + } + pub fn clear(&mut self) { self.map.clear(); self.next_ref = 1; @@ -380,7 +384,7 @@ async fn find_node_id_by_role_name( )) } -fn extract_ax_string(value: &Option) -> String { +pub(super) fn extract_ax_string(value: &Option) -> String { match value { Some(v) => match &v.value { Some(Value::String(s)) => s.clone(),