fix: rewrite getByRole to use CDP accessibility tree with ref-based element resolution (#1145)
* fix: rewrite getByRole to use CDP accessibility tree instead of CSS selectors
The old `handle_getbyrole` generated `querySelectorAll('[role="link"], link')`
which matched `<link>` stylesheet elements instead of `<a>` 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 <leehj0110@kakao.com>
This commit is contained in:
+153
-59
@@ -5212,78 +5212,90 @@ async fn handle_getbyrole(cmd: &Value, state: &mut DaemonState) -> Result<Value,
|
|||||||
let name = cmd.get("name").and_then(|v| v.as_str());
|
let name = cmd.get("name").and_then(|v| v.as_str());
|
||||||
let exact = cmd.get("exact").and_then(|v| v.as_bool()).unwrap_or(false);
|
let exact = cmd.get("exact").and_then(|v| v.as_bool()).unwrap_or(false);
|
||||||
|
|
||||||
let name_match = name
|
// Query the accessibility tree via CDP — the browser engine is the
|
||||||
.map(|n| {
|
// authoritative source for implicit ARIA roles (e.g. <a href> → "link").
|
||||||
if exact {
|
let (ax_params, effective_session_id) = super::element::resolve_ax_session(
|
||||||
format!(
|
state.active_frame_id.as_deref(),
|
||||||
"el.getAttribute('aria-label') === {} || el.textContent.trim() === {}",
|
&session_id,
|
||||||
serde_json::to_string(n).unwrap_or_default(),
|
&state.iframe_sessions,
|
||||||
serde_json::to_string(n).unwrap_or_default()
|
|
||||||
)
|
|
||||||
} else {
|
|
||||||
format!(
|
|
||||||
"(el.getAttribute('aria-label') || '').includes({n}) || el.textContent.includes({n})",
|
|
||||||
n = serde_json::to_string(n).unwrap_or_default()
|
|
||||||
)
|
|
||||||
}
|
|
||||||
})
|
|
||||||
.unwrap_or_else(|| "true".to_string());
|
|
||||||
|
|
||||||
let js = format!(
|
|
||||||
r#"(() => {{
|
|
||||||
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,
|
|
||||||
);
|
);
|
||||||
|
|
||||||
let result: super::cdp::types::EvaluateResult = mgr
|
let ax_tree: super::cdp::types::GetFullAXTreeResult = mgr
|
||||||
.client
|
.client
|
||||||
.send_command_typed(
|
.send_command_typed(
|
||||||
"Runtime.evaluate",
|
"Accessibility.getFullAXTree",
|
||||||
&super::cdp::types::EvaluateParams {
|
&ax_params,
|
||||||
expression: js,
|
Some(effective_session_id),
|
||||||
return_by_value: Some(true),
|
|
||||||
await_promise: Some(false),
|
|
||||||
},
|
|
||||||
Some(&session_id),
|
|
||||||
)
|
)
|
||||||
.await?;
|
.await?;
|
||||||
|
|
||||||
if !result
|
let (backend_node_id, actual_name) = find_ax_node_by_role(&ax_tree.nodes, role, name, exact)?;
|
||||||
.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 selector = "[data-agent-browser-located='true']";
|
// Register a temporary ref so execute_subaction can resolve the element
|
||||||
let result = execute_subaction(cmd, state, selector).await;
|
// via backendNodeId directly — no marker attribute needed.
|
||||||
|
let ref_num = state.ref_map.next_ref_num();
|
||||||
// Clean up the marker attribute
|
let temp_ref = format!("e{}", ref_num);
|
||||||
if let Some(ref browser) = state.browser {
|
state.ref_map.add_with_frame(
|
||||||
if browser.active_session_id().is_ok() {
|
temp_ref.clone(),
|
||||||
let _ = browser
|
Some(backend_node_id),
|
||||||
.evaluate(
|
role,
|
||||||
"document.querySelector('[data-agent-browser-located]')?.removeAttribute('data-agent-browser-located')",
|
&actual_name,
|
||||||
None,
|
None,
|
||||||
|
state.active_frame_id.as_deref(),
|
||||||
|
);
|
||||||
|
state.ref_map.set_next_ref_num(ref_num + 1);
|
||||||
|
|
||||||
|
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(
|
async fn handle_semantic_locator(
|
||||||
@@ -8488,4 +8500,86 @@ mod tests {
|
|||||||
assert!(!auto_handled, "{dialog_type} should NOT be auto-handled");
|
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<i64>,
|
||||||
|
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 <link> stylesheet elements instead of <a> anchors.
|
||||||
|
// The AX tree correctly assigns role="link" to <a href="...">.
|
||||||
|
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);
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -103,6 +103,10 @@ impl RefMap {
|
|||||||
entries
|
entries
|
||||||
}
|
}
|
||||||
|
|
||||||
|
pub fn remove(&mut self, ref_id: &str) {
|
||||||
|
self.map.remove(ref_id);
|
||||||
|
}
|
||||||
|
|
||||||
pub fn clear(&mut self) {
|
pub fn clear(&mut self) {
|
||||||
self.map.clear();
|
self.map.clear();
|
||||||
self.next_ref = 1;
|
self.next_ref = 1;
|
||||||
@@ -380,7 +384,7 @@ async fn find_node_id_by_role_name(
|
|||||||
))
|
))
|
||||||
}
|
}
|
||||||
|
|
||||||
fn extract_ax_string(value: &Option<AXValue>) -> String {
|
pub(super) fn extract_ax_string(value: &Option<AXValue>) -> String {
|
||||||
match value {
|
match value {
|
||||||
Some(v) => match &v.value {
|
Some(v) => match &v.value {
|
||||||
Some(Value::String(s)) => s.clone(),
|
Some(Value::String(s)) => s.clone(),
|
||||||
|
|||||||
Reference in New Issue
Block a user