diff --git a/crates/core/src/action.rs b/crates/core/src/action.rs index 954d482d..11978d46 100644 --- a/crates/core/src/action.rs +++ b/crates/core/src/action.rs @@ -59,7 +59,12 @@ impl Action { pub fn requires_hit_test(&self) -> bool { matches!( self, - Self::Click | Self::DoubleClick | Self::RightClick | Self::TripleClick | Self::Hover + Self::Click + | Self::DoubleClick + | Self::RightClick + | Self::TripleClick + | Self::Hover + | Self::Drag(_) ) } diff --git a/crates/core/src/action_tests.rs b/crates/core/src/action_tests.rs index 960b635e..0be4b5c1 100644 --- a/crates/core/src/action_tests.rs +++ b/crates/core/src/action_tests.rs @@ -123,3 +123,47 @@ fn hover_and_drag_base_policy_is_headless_independent_of_cursor_requirement() { "Drag.requires_cursor_policy() must still be true" ); } + +#[test] +fn requires_hit_test_covers_ref_targeted_pointer_actions() { + let hit_tested: &[Action] = &[ + Action::Click, + Action::DoubleClick, + Action::RightClick, + Action::TripleClick, + Action::Hover, + Action::Drag(dummy_drag()), + ]; + for action in hit_tested { + assert!( + action.requires_hit_test(), + "{} must require a hit test before dispatch", + action.name() + ); + } + + let not_hit_tested: &[Action] = &[ + Action::SetValue("v".into()), + Action::SetFocus, + Action::Expand, + Action::Collapse, + Action::Select("s".into()), + Action::Toggle, + Action::Check, + Action::Uncheck, + Action::Scroll(Direction::Down, 1), + Action::ScrollTo, + Action::PressKey(dummy_key()), + Action::KeyDown(dummy_key()), + Action::KeyUp(dummy_key()), + Action::TypeText("t".into()), + Action::Clear, + ]; + for action in not_hit_tested { + assert!( + !action.requires_hit_test(), + "{} must not require a hit test", + action.name() + ); + } +} diff --git a/crates/core/src/actionability/check.rs b/crates/core/src/actionability/check.rs index 42b0c500..691838b3 100644 --- a/crates/core/src/actionability/check.rs +++ b/crates/core/src/actionability/check.rs @@ -1,10 +1,27 @@ use super::ActionabilityStatus; +use crate::node::Rect; use serde::Serialize; -#[derive(Debug, Clone, Serialize, PartialEq, Eq)] +#[derive(Debug, Clone, Serialize, PartialEq)] pub struct ActionabilityCheck { pub name: &'static str, pub status: ActionabilityStatus, #[serde(skip_serializing_if = "Option::is_none")] pub reason: Option, + #[serde(skip_serializing_if = "Option::is_none")] + pub occluder: Option, +} + +/// The element a hit test actually landed on when it failed to reach the +/// intended target. `name` carries the occluder's accessible name under a +/// `name`-keyed field so `sanitize_trace_value` redacts it automatically; +/// `role` is a bounded AX vocabulary token, safe to surface in free text. +#[derive(Debug, Clone, Serialize, PartialEq)] +pub struct Occluder { + #[serde(skip_serializing_if = "Option::is_none")] + pub role: Option, + #[serde(skip_serializing_if = "Option::is_none")] + pub name: Option, + #[serde(skip_serializing_if = "Option::is_none")] + pub bounds: Option, } diff --git a/crates/core/src/actionability/mod.rs b/crates/core/src/actionability/mod.rs index 6f3afc63..7bbda3ce 100644 --- a/crates/core/src/actionability/mod.rs +++ b/crates/core/src/actionability/mod.rs @@ -1,11 +1,13 @@ use crate::capability; use crate::{ - action::Action, + action::{Action, Point}, action_request::ActionRequest, adapter::{NativeHandle, PlatformAdapter}, error::{AdapterError, ErrorCode}, + hit_test::HitTestResult, node::Rect, refs::RefEntry, + state, }; use serde_json::json; @@ -13,7 +15,7 @@ mod check; mod report; mod status; -pub use check::ActionabilityCheck; +pub use check::{ActionabilityCheck, Occluder}; pub use report::ActionabilityReport; pub use status::ActionabilityStatus; @@ -120,6 +122,12 @@ fn visibility_check(entry: &RefEntry) -> ActionabilityCheck { let Some(bounds) = entry.bounds else { return unknown("visible", "bounds unavailable"); }; + if state::has_state(&entry.states, state::HIDDEN) { + return fail("visible", "entry state contains hidden"); + } + if state::has_state(&entry.states, state::OFFSCREEN) { + return fail("visible", "entry state contains offscreen"); + } if !bounds_are_visible(Some(bounds)) { return fail("visible", "bounds are zero-sized"); } @@ -209,7 +217,6 @@ fn receives_events_check( adapter: &dyn PlatformAdapter, request: &ActionRequest, ) -> ActionabilityCheck { - use crate::action::Point; if !request.action.requires_hit_test() { return pass("receives_events"); } @@ -221,20 +228,47 @@ fn receives_events_check( y: bounds.y + bounds.height / 2.0, }; match adapter.hit_test(handle, point) { - Ok(result) if result.receives_events => pass("receives_events"), - Ok(_) => fail("receives_events", "element is occluded at its center"), - Err(err) - if matches!( - err.code, - ErrorCode::PlatformNotSupported | ErrorCode::ActionNotSupported - ) => - { - unknown("receives_events", "hit test unavailable") - } - Err(_) => fail("receives_events", "hit test failed"), + Ok(HitTestResult::ReachesTarget) => pass("receives_events"), + Ok(HitTestResult::Unknown) => unknown("receives_events", "hit test result inconclusive"), + Ok(HitTestResult::InterceptedBy { + role, + name, + bounds: occluder_bounds, + }) => occluded(role, name, occluder_bounds), + Err(_) => unknown("receives_events", "hit test unavailable"), } } +/// The occlusion-only counterpart to [`check_live`] for ref-targeted pointer +/// commands (`hover`, `drag`) that resolve a point via `point_resolve` +/// instead of dispatching an [`ActionRequest`] through `check_live` — they +/// have no `supported_action`/`editable` semantics to check, only whether the +/// resolved point actually reaches the target. Mirrors the three-way +/// [`HitTestResult`] contract: `ReachesTarget` and `Unknown` (including probe +/// errors and `not_supported`) both proceed, only `InterceptedBy` fails. +pub(crate) fn require_receives_events( + handle: &NativeHandle, + point: Point, + adapter: &dyn PlatformAdapter, +) -> Result<(), AdapterError> { + let check = match adapter.hit_test(handle, point) { + Ok(HitTestResult::ReachesTarget | HitTestResult::Unknown) | Err(_) => return Ok(()), + Ok(HitTestResult::InterceptedBy { role, name, bounds }) => occluded(role, name, bounds), + }; + let report = ActionabilityReport { + actionable: false, + checks: vec![check], + }; + Err(AdapterError::new( + ErrorCode::ActionFailed, + format!("Target is not actionable: {}", failure_reasons(&report)), + ) + .with_details(json!(report)) + .with_suggestion( + "Wait for the target to become actionable, refresh the snapshot, or use an explicit physical/focus command if intended.", + )) +} + fn failure_reasons(report: &ActionabilityReport) -> String { report .checks @@ -257,6 +291,7 @@ fn pass(name: &'static str) -> ActionabilityCheck { name, status: ActionabilityStatus::Pass, reason: None, + occluder: None, } } @@ -265,6 +300,7 @@ fn fail(name: &'static str, reason: impl Into) -> ActionabilityCheck { name, status: ActionabilityStatus::Fail, reason: Some(reason.into()), + occluder: None, } } @@ -273,6 +309,24 @@ fn unknown(name: &'static str, reason: impl Into) -> ActionabilityCheck name, status: ActionabilityStatus::Unknown, reason: Some(reason.into()), + occluder: None, + } +} + +fn occluded( + role: Option, + name: Option, + bounds: Option, +) -> ActionabilityCheck { + let reason = match role.as_deref() { + Some(role) => format!("occluded by {role}"), + None => "occluded by another element".to_string(), + }; + ActionabilityCheck { + name: "receives_events", + status: ActionabilityStatus::Fail, + reason: Some(reason), + occluder: Some(Occluder { role, name, bounds }), } } diff --git a/crates/core/src/actionability/report.rs b/crates/core/src/actionability/report.rs index 2d0df403..01030035 100644 --- a/crates/core/src/actionability/report.rs +++ b/crates/core/src/actionability/report.rs @@ -1,7 +1,7 @@ use super::ActionabilityCheck; use serde::Serialize; -#[derive(Debug, Clone, Serialize, PartialEq, Eq)] +#[derive(Debug, Clone, Serialize, PartialEq)] pub struct ActionabilityReport { pub actionable: bool, pub checks: Vec, diff --git a/crates/core/src/actionability_tests.rs b/crates/core/src/actionability_tests.rs index 56e05cd7..bc24190c 100644 --- a/crates/core/src/actionability_tests.rs +++ b/crates/core/src/actionability_tests.rs @@ -72,6 +72,26 @@ fn zero_sized_bounds_fail_visibility() { assert!(err.message.contains("visible")); } +#[test] +fn hidden_state_fails_visibility_before_action_dispatch() { + let mut entry = entry(); + entry.states.push(crate::state::HIDDEN.into()); + + let err = check(&entry, &ActionRequest::headless(Action::Click)).unwrap_err(); + + assert!(err.message.contains("visible")); +} + +#[test] +fn offscreen_state_fails_visibility_before_action_dispatch() { + let mut entry = entry(); + entry.states.push(crate::state::OFFSCREEN.into()); + + let err = check(&entry, &ActionRequest::headless(Action::Click)).unwrap_err(); + + assert!(err.message.contains("visible")); +} + #[test] fn text_input_requires_editable_target() { let err = check( diff --git a/crates/core/src/commands/drag_tests.rs b/crates/core/src/commands/drag_tests.rs index 1ba64f87..490df2bd 100644 --- a/crates/core/src/commands/drag_tests.rs +++ b/crates/core/src/commands/drag_tests.rs @@ -5,6 +5,7 @@ use crate::{ adapter::NativeHandle, capability, error::{AdapterError, ErrorCode}, + hit_test::HitTestResult, node::Rect, refs::{RefEntry, RefMap}, refs_store::RefStore, @@ -297,6 +298,72 @@ fn transient_stale_ref_retries_then_succeeds_when_timeout_wired() { assert!(adapter.resolve_calls.load(Ordering::SeqCst) >= 3); } +struct OccludedFromAdapter { + captured: Mutex>, +} + +impl ObservationOps for OccludedFromAdapter { + fn resolve_element_strict(&self, _entry: &RefEntry) -> Result { + Ok(NativeHandle::null()) + } + + fn get_element_bounds(&self, _handle: &NativeHandle) -> Result, AdapterError> { + Ok(Some(Rect { + x: 10.0, + y: 20.0, + width: 40.0, + height: 60.0, + })) + } + + fn hit_test( + &self, + _handle: &NativeHandle, + _point: crate::action::Point, + ) -> Result { + Ok(HitTestResult::InterceptedBy { + role: Some("AXSheet".into()), + name: Some("Save changes?".into()), + bounds: None, + }) + } +} + +impl ActionOps for OccludedFromAdapter {} + +impl InputOps for OccludedFromAdapter { + fn drag(&self, params: DragParams) -> Result<(), AdapterError> { + *self.captured.lock().unwrap() = Some(params); + Ok(()) + } +} + +impl SystemOps for OccludedFromAdapter {} + +/// F27 regression: `drag --from ` previously resolved bounds to a point +/// and dispatched without ever consulting `hit_test`, so a `from` ref +/// occluded by a modal sheet was dragged blind. This proves the preflight +/// now fails before `adapter.drag` is ever called. +#[test] +fn drag_from_occluded_ref_fails_preflight_before_dispatch() { + let _guard = HomeGuard::new(); + let snapshot_id = cross_app_snapshot(); + let adapter = OccludedFromAdapter { + captured: Mutex::new(None), + }; + + let err = execute( + cross_app_args(snapshot_id), + &adapter, + &CommandContext::default().with_headed(true), + ) + .unwrap_err(); + + assert_eq!(err.code(), "ACTION_FAILED"); + assert!(err.to_string().contains("AXSheet")); + assert!(adapter.captured.lock().unwrap().is_none()); +} + #[test] fn timeout_none_makes_exactly_one_resolve_attempt() { let _guard = HomeGuard::new(); diff --git a/crates/core/src/commands/hover_tests.rs b/crates/core/src/commands/hover_tests.rs index 75708b60..455fb4da 100644 --- a/crates/core/src/commands/hover_tests.rs +++ b/crates/core/src/commands/hover_tests.rs @@ -4,6 +4,7 @@ use crate::{ adapter::NativeHandle, capability, error::{AdapterError, ErrorCode}, + hit_test::HitTestResult, node::Rect, refs::{RefEntry, RefMap}, refs_store::RefStore, @@ -215,6 +216,72 @@ fn transient_stale_ref_retries_then_succeeds_when_timeout_wired() { assert!(adapter.resolve_calls.load(Ordering::SeqCst) >= 3); } +struct OccludedTargetAdapter { + moved_to: Mutex>, +} + +impl ObservationOps for OccludedTargetAdapter { + fn resolve_element_strict(&self, _entry: &RefEntry) -> Result { + Ok(NativeHandle::null()) + } + + fn get_element_bounds(&self, _handle: &NativeHandle) -> Result, AdapterError> { + Ok(Some(Rect { + x: 100.0, + y: 200.0, + width: 20.0, + height: 10.0, + })) + } + + fn hit_test( + &self, + _handle: &NativeHandle, + _point: crate::action::Point, + ) -> Result { + Ok(HitTestResult::InterceptedBy { + role: Some("AXSheet".into()), + name: Some("Save changes?".into()), + bounds: None, + }) + } +} + +impl ActionOps for OccludedTargetAdapter {} + +impl InputOps for OccludedTargetAdapter { + fn mouse_event(&self, event: MouseEvent) -> Result<(), AdapterError> { + *self.moved_to.lock().unwrap() = Some(event); + Ok(()) + } +} + +impl SystemOps for OccludedTargetAdapter {} + +/// F27 regression: `hover --ref` previously resolved the ref's bounds to a +/// point and dispatched the mouse move without ever consulting `hit_test`, +/// so an occluded target (e.g. a modal sheet over it) was hovered blind. +/// This proves the preflight now fails before any mouse event is sent. +#[test] +fn hover_on_occluded_ref_fails_preflight_before_dispatch() { + let _guard = HomeGuard::new(); + let snapshot_id = ref_snapshot(42); + let adapter = OccludedTargetAdapter { + moved_to: Mutex::new(None), + }; + + let err = execute( + ref_args(snapshot_id), + &adapter, + &CommandContext::default().with_headed(true), + ) + .unwrap_err(); + + assert_eq!(err.code(), "ACTION_FAILED"); + assert!(err.to_string().contains("AXSheet")); + assert!(adapter.moved_to.lock().unwrap().is_none()); +} + #[test] fn timeout_none_makes_exactly_one_resolve_attempt() { let _guard = HomeGuard::new(); diff --git a/crates/core/src/commands/point_resolve.rs b/crates/core/src/commands/point_resolve.rs index 99f21082..58211aee 100644 --- a/crates/core/src/commands/point_resolve.rs +++ b/crates/core/src/commands/point_resolve.rs @@ -1,5 +1,6 @@ use crate::{ action::Point, + actionability, adapter::PlatformAdapter, commands::helpers::resolve_ref_with_context, context::CommandContext, @@ -44,11 +45,13 @@ pub(crate) fn resolve_point_from_ref_or_xy_with_context( let bounds = adapter .get_element_bounds(handle.handle())? .ok_or_else(|| AppError::invalid_input(format!("Element {ref_id} has no bounds")))?; + let point = Point { + x: bounds.x + bounds.width / 2.0, + y: bounds.y + bounds.height / 2.0, + }; + actionability::require_receives_events(handle.handle(), point.clone(), adapter)?; return Ok(ResolvedPoint { - point: Point { - x: bounds.x + bounds.width / 2.0, - y: bounds.y + bounds.height / 2.0, - }, + point, pid: Some(entry.pid), }); } @@ -87,18 +90,5 @@ pub(crate) fn focus_for_physical_input( } #[cfg(test)] -mod tests { - use super::*; - - #[test] - fn physical_input_requires_headed_context() { - let err = require_cursor_policy(&CommandContext::default(), "mouse-move").unwrap_err(); - - assert_eq!(err.code(), "POLICY_DENIED"); - } - - #[test] - fn headed_context_allows_physical_input() { - require_cursor_policy(&CommandContext::default().with_headed(true), "mouse-move").unwrap(); - } -} +#[path = "point_resolve_tests.rs"] +mod tests; diff --git a/crates/core/src/commands/point_resolve_tests.rs b/crates/core/src/commands/point_resolve_tests.rs new file mode 100644 index 00000000..0d853f9a --- /dev/null +++ b/crates/core/src/commands/point_resolve_tests.rs @@ -0,0 +1,206 @@ +use super::*; +use crate::adapter::{ActionOps, InputOps, ObservationOps, SystemOps}; +use crate::{ + adapter::NativeHandle, + capability, + error::ErrorCode, + hit_test::HitTestResult, + node::Rect, + refs::{RefEntry, RefMap}, + refs_store::RefStore, + refs_test_support::HomeGuard, +}; + +#[test] +fn physical_input_requires_headed_context() { + let err = require_cursor_policy(&CommandContext::default(), "mouse-move").unwrap_err(); + + assert_eq!(err.code(), "POLICY_DENIED"); +} + +#[test] +fn headed_context_allows_physical_input() { + require_cursor_policy(&CommandContext::default().with_headed(true), "mouse-move").unwrap(); +} + +/// An adapter whose `hit_test` outcome is fixed per test, so each occlusion +/// scenario (F27) exercises the real `resolve_point_from_ref_or_xy_with_context` +/// path rather than a mock that always echoes success. +struct HitTestOutcomeAdapter { + outcome: Result, +} + +impl ObservationOps for HitTestOutcomeAdapter { + fn resolve_element_strict(&self, _entry: &RefEntry) -> Result { + Ok(NativeHandle::null()) + } + + fn get_element_bounds(&self, _handle: &NativeHandle) -> Result, AdapterError> { + Ok(Some(Rect { + x: 100.0, + y: 200.0, + width: 20.0, + height: 10.0, + })) + } + + fn hit_test( + &self, + _handle: &NativeHandle, + _point: Point, + ) -> Result { + self.outcome.clone() + } +} + +impl ActionOps for HitTestOutcomeAdapter {} +impl InputOps for HitTestOutcomeAdapter {} +impl SystemOps for HitTestOutcomeAdapter {} + +fn ref_snapshot(pid: i32) -> String { + let store = RefStore::new().unwrap(); + let mut refmap = RefMap::new(); + refmap.allocate(RefEntry { + pid, + role: "button".into(), + name: Some("Target".into()), + value: None, + description: None, + native_id: None, + states: vec![], + bounds: None, + bounds_hash: None, + available_actions: vec![capability::CLICK.into()], + source_app: None, + source_window_id: None, + source_window_title: None, + source_surface: crate::adapter::SnapshotSurface::Window, + root_ref: None, + path_is_absolute: false, + path: smallvec::SmallVec::new(), + }); + store.save_new_snapshot(&refmap).unwrap() +} + +fn ref_args(snapshot_id: &str) -> PointResolveArgs<'_> { + PointResolveArgs { + ref_id: Some("@e1"), + xy: None, + snapshot_id: Some(snapshot_id), + missing_input_message: "Provide a ref (@e1) or --xy x,y", + } +} + +/// F27 regression: previously the ref-targeted path never called +/// `adapter.hit_test` at all, so an occluded target resolved to a point and +/// dispatch proceeded blind. This proves `InterceptedBy` now blocks +/// resolution and names the occluder. +#[test] +fn intercepted_by_blocks_ref_targeted_point_resolution() { + let _guard = HomeGuard::new(); + let snapshot_id = ref_snapshot(42); + let adapter = HitTestOutcomeAdapter { + outcome: Ok(HitTestResult::InterceptedBy { + role: Some("AXSheet".into()), + name: Some("Save changes?".into()), + bounds: None, + }), + }; + + let result = resolve_point_from_ref_or_xy_with_context( + ref_args(&snapshot_id), + &adapter, + &CommandContext::default(), + ); + let err = match result { + Ok(_) => panic!("occluded ref target must not resolve to a point"), + Err(err) => err, + }; + + assert_eq!(err.code(), ErrorCode::ActionFailed.as_str()); + let message = err.to_string(); + assert!(message.contains("AXSheet")); +} + +#[test] +fn reaches_target_allows_ref_targeted_point_resolution() { + let _guard = HomeGuard::new(); + let snapshot_id = ref_snapshot(42); + let adapter = HitTestOutcomeAdapter { + outcome: Ok(HitTestResult::ReachesTarget), + }; + + let resolved = resolve_point_from_ref_or_xy_with_context( + ref_args(&snapshot_id), + &adapter, + &CommandContext::default(), + ) + .unwrap(); + + assert_eq!(resolved.point.x, 110.0); + assert_eq!(resolved.point.y, 205.0); +} + +#[test] +fn unknown_hit_test_result_does_not_block_ref_targeted_resolution() { + let _guard = HomeGuard::new(); + let snapshot_id = ref_snapshot(42); + let adapter = HitTestOutcomeAdapter { + outcome: Ok(HitTestResult::Unknown), + }; + + resolve_point_from_ref_or_xy_with_context( + ref_args(&snapshot_id), + &adapter, + &CommandContext::default(), + ) + .unwrap(); +} + +/// A hit-test probe error must never be treated as occlusion (the reliability +/// learning's evidence rule) — resolution proceeds exactly as if hit-testing +/// were unavailable. +#[test] +fn hit_test_probe_error_does_not_block_ref_targeted_resolution() { + let _guard = HomeGuard::new(); + let snapshot_id = ref_snapshot(42); + let adapter = HitTestOutcomeAdapter { + outcome: Err(AdapterError::internal( + "AXUIElementCopyElementAtPosition failed", + )), + }; + + resolve_point_from_ref_or_xy_with_context( + ref_args(&snapshot_id), + &adapter, + &CommandContext::default(), + ) + .unwrap(); +} + +/// Raw `--xy` input stays raw by design (KTD4): no ref means no occlusion +/// check, even against an adapter that would otherwise report occlusion. +#[test] +fn raw_xy_input_never_calls_hit_test() { + let adapter = HitTestOutcomeAdapter { + outcome: Ok(HitTestResult::InterceptedBy { + role: Some("AXSheet".into()), + name: None, + bounds: None, + }), + }; + + let resolved = resolve_point_from_ref_or_xy_with_context( + PointResolveArgs { + ref_id: None, + xy: Some((5.0, 6.0)), + snapshot_id: None, + missing_input_message: "Provide a ref (@e1) or --xy x,y", + }, + &adapter, + &CommandContext::default(), + ) + .unwrap(); + + assert_eq!((resolved.point.x, resolved.point.y), (5.0, 6.0)); +} diff --git a/crates/core/src/hit_test.rs b/crates/core/src/hit_test.rs index 6e5ef17d..5438b010 100644 --- a/crates/core/src/hit_test.rs +++ b/crates/core/src/hit_test.rs @@ -1,26 +1,25 @@ +use crate::node::Rect; use serde::{Deserialize, Serialize}; -#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] -pub struct HitTestResult { - pub receives_events: bool, - #[serde(skip_serializing_if = "Option::is_none")] - pub topmost_role: Option, -} - -impl HitTestResult { - pub fn receives_events(topmost_role: Option) -> Self { - Self { - receives_events: true, - topmost_role, - } - } - - pub fn blocked(topmost_role: Option) -> Self { - Self { - receives_events: false, - topmost_role, - } - } +/// Classifies whether a hit-tested point reaches the intended target. A hit +/// on the target itself or one of its descendants reaches it; a hit outside +/// the target's ancestor chain names a real occluder (modal, overlay, +/// sibling); a hit on the target's own ancestor is `Unknown` rather than a +/// false occlusion, since composited or custom-drawn views often expose no +/// distinct child node to hit-test. Probe failures are `Unknown` for the +/// same reason: unavailable evidence is never a false failure. +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] +pub enum HitTestResult { + ReachesTarget, + InterceptedBy { + #[serde(skip_serializing_if = "Option::is_none")] + role: Option, + #[serde(skip_serializing_if = "Option::is_none")] + name: Option, + #[serde(skip_serializing_if = "Option::is_none")] + bounds: Option, + }, + Unknown, } #[cfg(test)] diff --git a/crates/core/src/hit_test_tests.rs b/crates/core/src/hit_test_tests.rs index 2e2af432..5d2957c5 100644 --- a/crates/core/src/hit_test_tests.rs +++ b/crates/core/src/hit_test_tests.rs @@ -2,7 +2,7 @@ use super::*; use crate::{ action::Action, action_request::ActionRequest, - actionability::check_live, + actionability::{ActionabilityCheck, ActionabilityStatus, check_live}, adapter::{ActionOps, InputOps, LiveElement, NativeHandle, ObservationOps, SystemOps}, element_state::ElementState, error::{AdapterError, ErrorCode}, @@ -11,7 +11,7 @@ use crate::{ use smallvec::SmallVec; struct HitTestAdapter { - receives: bool, + outcome: Result, } impl ObservationOps for HitTestAdapter { @@ -41,11 +41,7 @@ impl ObservationOps for HitTestAdapter { _handle: &NativeHandle, _point: crate::action::Point, ) -> Result { - Ok(if self.receives { - HitTestResult::receives_events(Some("AXButton".into())) - } else { - HitTestResult::blocked(Some("AXGroup".into())) - }) + self.outcome.clone() } } @@ -80,21 +76,84 @@ fn clickable_entry() -> RefEntry { } } -#[test] -fn occluded_target_fails_receives_events_check() { - let adapter = HitTestAdapter { receives: false }; +/// Runs `check_live` for a `Click` (which requires a hit test) against the +/// given hit-test outcome and returns the `receives_events` check. Only the +/// `InterceptedBy` outcome fails actionability, so every other outcome is +/// safe to unwrap here; the `InterceptedBy` case is asserted separately +/// against the `Err` path so its occluder details can be inspected. +fn run_receives_events_check(outcome: Result) -> ActionabilityCheck { + let adapter = HitTestAdapter { outcome }; let entry = clickable_entry(); let request = ActionRequest::headless(Action::Click); - let err = check_live(&entry, &NativeHandle::null(), &adapter, &request) - .expect_err("occluded targets must fail actionability"); - assert_eq!(err.code, ErrorCode::ActionFailed); - assert!(err.message.contains("receives_events")); + let report = check_live(&entry, &NativeHandle::null(), &adapter, &request) + .expect("only an InterceptedBy hit-test outcome fails actionability"); + report + .checks + .into_iter() + .find(|check| check.name == "receives_events") + .expect("Click requires a receives_events check") } #[test] -fn unoccluded_target_passes_receives_events_check() { - let adapter = HitTestAdapter { receives: true }; +fn reaches_target_result_passes_receives_events_check() { + let check = run_receives_events_check(Ok(HitTestResult::ReachesTarget)); + assert_eq!(check.status, ActionabilityStatus::Pass); +} + +#[test] +fn unknown_hit_test_result_does_not_block_action() { + let check = run_receives_events_check(Ok(HitTestResult::Unknown)); + assert_eq!(check.status, ActionabilityStatus::Unknown); +} + +#[test] +fn not_supported_hit_test_does_not_block_action() { + let check = run_receives_events_check(Err(AdapterError::not_supported("hit_test"))); + assert_eq!(check.status, ActionabilityStatus::Unknown); +} + +#[test] +fn hit_test_probe_error_does_not_block_action() { + let check = run_receives_events_check(Err(AdapterError::internal( + "AXUIElementCopyElementAtPosition failed", + ))); + assert_eq!( + check.status, + ActionabilityStatus::Unknown, + "a probe failure must never be reported as a Fail" + ); +} + +#[test] +fn intercepted_by_result_fails_and_carries_redactable_occluder() { + let adapter = HitTestAdapter { + outcome: Ok(HitTestResult::InterceptedBy { + role: Some("AXSheet".into()), + name: Some("Save changes?".into()), + bounds: None, + }), + }; let entry = clickable_entry(); let request = ActionRequest::headless(Action::Click); - check_live(&entry, &NativeHandle::null(), &adapter, &request).expect("clickable target"); + + let err = check_live(&entry, &NativeHandle::null(), &adapter, &request) + .expect_err("a hit outside the target's ancestor chain must fail actionability"); + + assert_eq!(err.code, ErrorCode::ActionFailed); + assert!(err.message.contains("AXSheet")); + assert!(!err.message.contains("Save changes?")); + + let details = err + .details + .expect("a Fail report attaches actionability details"); + let checks = details["checks"] + .as_array() + .expect("details.checks is an array"); + let receives_events = checks + .iter() + .find(|check| check["name"] == "receives_events") + .expect("receives_events check must be present when Click requires a hit test"); + assert_eq!(receives_events["status"], "fail"); + assert_eq!(receives_events["occluder"]["name"], "Save changes?"); + assert_eq!(receives_events["occluder"]["role"], "AXSheet"); } diff --git a/crates/macos/src/tree/hit_test.rs b/crates/macos/src/tree/hit_test.rs index 7206b9bf..04a9ce77 100644 --- a/crates/macos/src/tree/hit_test.rs +++ b/crates/macos/src/tree/hit_test.rs @@ -1,14 +1,22 @@ #[cfg(target_os = "macos")] mod imp { + use crate::actions::ax_helpers; use crate::tree::{ - AXElement, capabilities::same_element, copy_element_attr, copy_string_attr, read_bounds, + AXElement, capabilities::same_element, element::ABSOLUTE_MAX_DEPTH, read_bounds, + resolve_element_name, }; - use accessibility_sys::{AXUIElementCopyElementAtPosition, kAXErrorSuccess, kAXRoleAttribute}; + use accessibility_sys::{AXUIElementCopyElementAtPosition, kAXErrorSuccess}; use agent_desktop_core::{ action::Point, error::AdapterError, hit_test::HitTestResult, native_handle::NativeHandle, }; use std::mem::ManuallyDrop; + /// Hit-tests `point` against `target`'s owning application, then + /// classifies the result against `target`'s ancestor chain: a hit on + /// `target` or a descendant reaches it, a hit outside that chain names a + /// real occluder, and a hit on `target`'s own ancestor — like any probe + /// failure — is `Unknown` rather than a false occlusion, since composited + /// or custom-drawn views often expose no distinct child node to hit-test. pub fn hit_test_impl( handle: &NativeHandle, point: Point, @@ -28,46 +36,61 @@ mod imp { fn hit_test_element(target: &AXElement, point: Point) -> Result { let Some(bounds) = read_bounds(target) else { - return Err(AdapterError::new( - agent_desktop_core::error::ErrorCode::ActionFailed, - "Element bounds unavailable for hit test", - )); + return Ok(HitTestResult::Unknown); }; if bounds.width <= 0.0 || bounds.height <= 0.0 { - return Ok(HitTestResult::blocked(None)); + return Ok(HitTestResult::Unknown); } - let pid = crate::system::app_ops::pid_from_element(target) - .ok_or_else(|| AdapterError::internal("Could not read pid for hit test"))?; + let Some(pid) = crate::system::app_ops::pid_from_element(target) else { + return Ok(HitTestResult::Unknown); + }; let app = crate::tree::element_for_pid(pid); let mut hit_ref: accessibility_sys::AXUIElementRef = std::ptr::null_mut(); let err = unsafe { AXUIElementCopyElementAtPosition(app.0, point.x as f32, point.y as f32, &mut hit_ref) }; if err != kAXErrorSuccess || hit_ref.is_null() { - return Ok(HitTestResult::blocked(None)); + return Ok(HitTestResult::Unknown); } let hit = AXElement(hit_ref); - let topmost_role = copy_string_attr(&hit, kAXRoleAttribute); - let receives = element_contains(target, &hit) || element_contains(&hit, target); - Ok(if receives { - HitTestResult::receives_events(topmost_role) - } else { - HitTestResult::blocked(topmost_role) - }) + Ok(classify_hit(target, &hit)) } - fn element_contains(ancestor: &AXElement, candidate: &AXElement) -> bool { - if same_element(ancestor, candidate) { - return true; + fn classify_hit(target: &AXElement, hit: &AXElement) -> HitTestResult { + let limit = ABSOLUTE_MAX_DEPTH as usize; + let reaches_target = same_element(target, hit) + || ax_helpers::try_each_ancestor(hit, |ancestor| same_element(ancestor, target), limit); + let is_ancestor_of_target = !reaches_target + && ax_helpers::try_each_ancestor(target, |ancestor| same_element(ancestor, hit), limit); + match classify_relation(reaches_target, is_ancestor_of_target) { + HitClassification::ReachesTarget => HitTestResult::ReachesTarget, + HitClassification::AncestorOfTarget => HitTestResult::Unknown, + HitClassification::Unrelated => HitTestResult::InterceptedBy { + role: ax_helpers::element_role(hit), + name: resolve_element_name(hit), + bounds: read_bounds(hit), + }, } - let mut current = copy_element_attr(candidate, "AXParent"); - while let Some(parent) = current { - if same_element(ancestor, &parent) { - return true; - } - current = copy_element_attr(&parent, "AXParent"); + } + + #[derive(Debug, Clone, Copy, PartialEq, Eq)] + pub(super) enum HitClassification { + ReachesTarget, + AncestorOfTarget, + Unrelated, + } + + pub(super) fn classify_relation( + reaches_target: bool, + is_ancestor_of_target: bool, + ) -> HitClassification { + if reaches_target { + HitClassification::ReachesTarget + } else if is_ancestor_of_target { + HitClassification::AncestorOfTarget + } else { + HitClassification::Unrelated } - false } } @@ -86,3 +109,7 @@ mod imp { } pub use imp::hit_test_impl; + +#[cfg(all(test, target_os = "macos"))] +#[path = "hit_test_tests.rs"] +mod tests; diff --git a/crates/macos/src/tree/hit_test_tests.rs b/crates/macos/src/tree/hit_test_tests.rs new file mode 100644 index 00000000..701d2763 --- /dev/null +++ b/crates/macos/src/tree/hit_test_tests.rs @@ -0,0 +1,33 @@ +use super::imp::{HitClassification, classify_relation}; + +#[test] +fn self_hit_reaches_target() { + assert_eq!( + classify_relation(true, false), + HitClassification::ReachesTarget + ); +} + +#[test] +fn reaches_target_takes_priority_over_ancestor() { + assert_eq!( + classify_relation(true, true), + HitClassification::ReachesTarget + ); +} + +#[test] +fn ancestor_hit_is_reported_separately_from_unrelated() { + assert_eq!( + classify_relation(false, true), + HitClassification::AncestorOfTarget + ); +} + +#[test] +fn unrelated_hit_is_neither_target_nor_ancestor() { + assert_eq!( + classify_relation(false, false), + HitClassification::Unrelated + ); +}