From 175ed76562ae28afeeddde1b3a3819d5470279d2 Mon Sep 17 00:00:00 2001 From: Lahfir Date: Thu, 2 Jul 2026 19:34:37 -0700 Subject: [PATCH] fix: enforce canonical state vocabulary and live is visible evidence Add role/state vocabulary modules and move is --property visible onto live bounds plus hidden/offscreen tokens so off-screen elements no longer pass. Co-authored-by: Cursor --- crates/core/src/commands/is_check.rs | 74 ++++---- crates/core/src/commands/is_check_tests.rs | 199 +++++++++++++++++++-- crates/core/src/commands/wait_predicate.rs | 19 +- crates/core/src/lib.rs | 2 + crates/core/src/role.rs | 139 ++++++++++++++ crates/core/src/roles.rs | 4 +- crates/core/src/state.rs | 133 ++++++++++++++ 7 files changed, 520 insertions(+), 50 deletions(-) create mode 100644 crates/core/src/role.rs create mode 100644 crates/core/src/state.rs diff --git a/crates/core/src/commands/is_check.rs b/crates/core/src/commands/is_check.rs index 55d44961..e68df496 100644 --- a/crates/core/src/commands/is_check.rs +++ b/crates/core/src/commands/is_check.rs @@ -5,6 +5,7 @@ use crate::{ element_state::ElementState, error::AppError, refs::RefEntry, + state::{self, CHECKED, DISABLED, EXPANDED, FOCUSED, VisibilityEvidence}, }; use serde_json::{Value, json}; @@ -22,7 +23,6 @@ pub enum IsProperty { Expanded, } -/// State is read live when the platform supports it, then falls back to snapshot state. pub fn execute( args: IsArgs, adapter: &dyn PlatformAdapter, @@ -30,8 +30,6 @@ pub fn execute( ) -> Result { let (entry, handle) = resolve_ref_with_context(&args.ref_id, args.snapshot_id.as_deref(), adapter, context)?; - let state = optional_live_read(adapter.get_live_state(handle.handle()))? - .unwrap_or_else(|| state_from_ref_entry(&entry)); let prop_name = match args.property { IsProperty::Visible => "visible", @@ -41,14 +39,46 @@ pub fn execute( IsProperty::Expanded => "expanded", }; - let applicable = is_applicable(&args.property, &entry, &state); + let live_state = optional_live_read(adapter.get_live_state(handle.handle()))?; + let state = live_state + .clone() + .unwrap_or_else(|| state_from_ref_entry(&entry)); + let states_from_live = live_state.is_some(); + let live_bounds = optional_live_read(adapter.get_element_bounds(handle.handle()))?; + let visibility = VisibilityEvidence { + bounds: live_bounds.or(entry.bounds), + states: state.states.clone(), + bounds_from_live: live_bounds.is_some(), + states_from_live, + }; + + let applicable = match args.property { + IsProperty::Visible => visibility.applicable(), + IsProperty::Enabled | IsProperty::Focused => true, + IsProperty::Checked => { + crate::roles::is_toggleable_role(&entry.role) + || state::has_state(&state.states, CHECKED) + || crate::capability::contains_any( + &entry.available_actions, + crate::capability::CHECKED_APPLICABILITY, + ) + } + IsProperty::Expanded => { + crate::roles::is_expandable_role(&entry.role) + || state::has_state(&state.states, EXPANDED) + || crate::capability::contains_any( + &entry.available_actions, + crate::capability::EXPANDED_APPLICABILITY, + ) + } + }; let result = match args.property { - IsProperty::Visible => !has_state(&state, "hidden"), - IsProperty::Enabled => !has_state(&state, "disabled"), - IsProperty::Checked => has_state(&state, "checked"), - IsProperty::Focused => has_state(&state, "focused"), - IsProperty::Expanded => has_state(&state, "expanded"), + IsProperty::Visible => visibility.result(), + IsProperty::Enabled => !state::has_state(&state.states, DISABLED), + IsProperty::Checked => state::has_state(&state.states, CHECKED), + IsProperty::Focused => state::has_state(&state.states, FOCUSED), + IsProperty::Expanded => state::has_state(&state.states, EXPANDED), }; Ok( @@ -64,32 +94,6 @@ fn state_from_ref_entry(entry: &RefEntry) -> ElementState { } } -fn has_state(state: &ElementState, name: &str) -> bool { - state.states.iter().any(|s| s == name) -} - -fn is_applicable(property: &IsProperty, entry: &RefEntry, state: &ElementState) -> bool { - match property { - IsProperty::Visible | IsProperty::Enabled | IsProperty::Focused => true, - IsProperty::Checked => { - crate::roles::is_toggleable_role(&entry.role) - || has_state(state, "checked") - || crate::capability::contains_any( - &entry.available_actions, - crate::capability::CHECKED_APPLICABILITY, - ) - } - IsProperty::Expanded => { - crate::roles::is_expandable_role(&entry.role) - || has_state(state, "expanded") - || crate::capability::contains_any( - &entry.available_actions, - crate::capability::EXPANDED_APPLICABILITY, - ) - } - } -} - #[cfg(test)] #[path = "is_check_tests.rs"] mod tests; diff --git a/crates/core/src/commands/is_check_tests.rs b/crates/core/src/commands/is_check_tests.rs index 672afbf2..f5f96061 100644 --- a/crates/core/src/commands/is_check_tests.rs +++ b/crates/core/src/commands/is_check_tests.rs @@ -1,13 +1,40 @@ use super::*; use crate::adapter::{ActionOps, InputOps, ObservationOps, SystemOps}; use crate::{ - adapter::NativeHandle, error::AdapterError, refs::RefMap, refs_store::RefStore, - refs_test_support::HomeGuard, + adapter::NativeHandle, error::AdapterError, node::Rect, refs::RefMap, refs_store::RefStore, + refs_test_support::HomeGuard, state, }; use std::sync::Mutex; struct LiveStateAdapter { state: Mutex>, + bounds: Mutex>, + bounds_supported: bool, + state_supported: bool, +} + +impl LiveStateAdapter { + fn with_live(bounds: Rect, states: Vec) -> Self { + Self { + state: Mutex::new(Some(ElementState { + role: "button".into(), + states, + value: None, + })), + bounds: Mutex::new(Some(bounds)), + bounds_supported: true, + state_supported: true, + } + } + + fn without_live_support() -> Self { + Self { + state: Mutex::new(None), + bounds: Mutex::new(None), + bounds_supported: false, + state_supported: false, + } + } } impl ObservationOps for LiveStateAdapter { @@ -16,8 +43,18 @@ impl ObservationOps for LiveStateAdapter { } fn get_live_state(&self, _handle: &NativeHandle) -> Result, AdapterError> { + if !self.state_supported { + return Err(AdapterError::not_supported("get_live_state")); + } Ok(self.state.lock().unwrap().clone()) } + + fn get_element_bounds(&self, _handle: &NativeHandle) -> Result, AdapterError> { + if !self.bounds_supported { + return Err(AdapterError::not_supported("get_element_bounds")); + } + Ok(*self.bounds.lock().unwrap()) + } } impl ActionOps for LiveStateAdapter {} @@ -53,6 +90,128 @@ fn entry(states: Vec, value: Option<&str>, actions: Vec<&str>) -> RefEnt } } +fn visible_bounds() -> Rect { + Rect { + x: 0.0, + y: 0.0, + width: 10.0, + height: 10.0, + } +} + +#[test] +fn hidden_element_reports_not_visible() { + let _guard = HomeGuard::new(); + let snapshot_id = save_entry(entry(vec![], None, vec![])); + let adapter = LiveStateAdapter::with_live(visible_bounds(), vec![state::HIDDEN.into()]); + + let result = execute( + IsArgs { + ref_id: "@e1".into(), + snapshot_id: Some(snapshot_id), + property: IsProperty::Visible, + }, + &adapter, + &CommandContext::default(), + ) + .unwrap(); + + assert_eq!(result["result"], false); + assert_eq!(result["applicable"], true); +} + +#[test] +fn zero_sized_bounds_report_not_visible() { + let _guard = HomeGuard::new(); + let snapshot_id = save_entry(entry(vec![], None, vec![])); + let adapter = LiveStateAdapter::with_live( + Rect { + x: 0.0, + y: 0.0, + width: 0.0, + height: 10.0, + }, + vec![], + ); + + let result = execute( + IsArgs { + ref_id: "@e1".into(), + snapshot_id: Some(snapshot_id), + property: IsProperty::Visible, + }, + &adapter, + &CommandContext::default(), + ) + .unwrap(); + + assert_eq!(result["result"], false); + assert_eq!(result["applicable"], true); +} + +#[test] +fn offscreen_element_reports_not_visible() { + let _guard = HomeGuard::new(); + let snapshot_id = save_entry(entry(vec![], None, vec![])); + let adapter = LiveStateAdapter::with_live(visible_bounds(), vec![state::OFFSCREEN.into()]); + + let result = execute( + IsArgs { + ref_id: "@e1".into(), + snapshot_id: Some(snapshot_id), + property: IsProperty::Visible, + }, + &adapter, + &CommandContext::default(), + ) + .unwrap(); + + assert_eq!(result["result"], false); + assert_eq!(result["applicable"], true); +} + +#[test] +fn visible_element_with_live_evidence_reports_true() { + let _guard = HomeGuard::new(); + let snapshot_id = save_entry(entry(vec![], None, vec![])); + let adapter = LiveStateAdapter::with_live(visible_bounds(), vec![]); + + let result = execute( + IsArgs { + ref_id: "@e1".into(), + snapshot_id: Some(snapshot_id), + property: IsProperty::Visible, + }, + &adapter, + &CommandContext::default(), + ) + .unwrap(); + + assert_eq!(result["result"], true); + assert_eq!(result["applicable"], true); +} + +#[test] +fn visible_degrades_applicability_when_live_reads_unsupported() { + let _guard = HomeGuard::new(); + let snapshot_id = save_entry(entry(vec![], None, vec![])); + let adapter = LiveStateAdapter::without_live_support(); + + let result = execute( + IsArgs { + ref_id: "@e1".into(), + snapshot_id: Some(snapshot_id), + property: IsProperty::Visible, + }, + &adapter, + &CommandContext::default(), + ) + .unwrap(); + + assert_eq!(result["result"], false); + assert_eq!(result["applicable"], false); +} + #[test] fn checked_uses_live_canonical_state() { let _guard = HomeGuard::new(); @@ -63,6 +222,9 @@ fn checked_uses_live_canonical_state() { states: vec!["checked".into()], value: Some("1".into()), })), + bounds: Mutex::new(None), + bounds_supported: false, + state_supported: true, }; let result = execute( @@ -86,6 +248,9 @@ fn checked_does_not_infer_platform_values_in_core() { let snapshot_id = save_entry(entry(vec![], Some("1"), vec!["Toggle"])); let adapter = LiveStateAdapter { state: Mutex::new(None), + bounds: Mutex::new(None), + bounds_supported: false, + state_supported: true, }; let result = execute( @@ -109,6 +274,9 @@ fn checked_falls_back_to_snapshot_state_when_live_state_is_missing() { let snapshot_id = save_entry(entry(vec!["checked".into()], None, vec!["Toggle"])); let adapter = LiveStateAdapter { state: Mutex::new(None), + bounds: Mutex::new(None), + bounds_supported: false, + state_supported: true, }; let result = execute( @@ -130,16 +298,10 @@ fn checked_falls_back_to_snapshot_state_when_live_state_is_missing() { fn basic_state_properties_use_live_state() { let _guard = HomeGuard::new(); let snapshot_id = save_entry(entry(vec![], None, vec![])); - let adapter = LiveStateAdapter { - state: Mutex::new(Some(ElementState { - role: "button".into(), - states: vec!["focused".into(), "expanded".into()], - value: None, - })), - }; + let adapter = + LiveStateAdapter::with_live(visible_bounds(), vec!["focused".into(), "expanded".into()]); for (property, expected) in [ - (IsProperty::Visible, true), (IsProperty::Enabled, true), (IsProperty::Focused, true), (IsProperty::Expanded, true), @@ -183,6 +345,9 @@ fn action_availability_makes_toggle_and_expand_applicable() { }); let adapter = LiveStateAdapter { state: Mutex::new(None), + bounds: Mutex::new(None), + bounds_supported: false, + state_supported: true, }; for property in [IsProperty::Checked, IsProperty::Expanded] { @@ -200,3 +365,17 @@ fn action_availability_makes_toggle_and_expand_applicable() { assert_eq!(result["applicable"], true); } } + +#[test] +fn state_vocabulary_conformance_guard() { + for token in [ + state::FOCUSED, + state::DISABLED, + state::CHECKED, + state::EXPANDED, + state::HIDDEN, + state::OFFSCREEN, + ] { + state::assert_states_in_vocabulary(&[token.to_string()]); + } +} diff --git a/crates/core/src/commands/wait_predicate.rs b/crates/core/src/commands/wait_predicate.rs index 952160b9..8b32ace2 100644 --- a/crates/core/src/commands/wait_predicate.rs +++ b/crates/core/src/commands/wait_predicate.rs @@ -1,7 +1,7 @@ use crate::{ action::Action, action_request::ActionRequest, - actionability::{bounds_are_visible, states_are_enabled}, + actionability::states_are_enabled, adapter::{NativeHandle, PlatformAdapter, optional_live_read}, error::{AdapterError, AppError, ErrorCode}, refs::RefEntry, @@ -146,12 +146,23 @@ fn enabled( } fn visible( - entry: &RefEntry, + _entry: &RefEntry, handle: &NativeHandle, adapter: &dyn PlatformAdapter, ) -> Result { - let bounds = optional_live_read(adapter.get_element_bounds(handle))?.or(entry.bounds); - Ok(json!({ "visible": bounds_are_visible(bounds) })) + let live_bounds = optional_live_read(adapter.get_element_bounds(handle))?; + let live_state = optional_live_read(adapter.get_live_state(handle))?; + let states_from_live = live_state.is_some(); + let evidence = crate::state::VisibilityEvidence { + bounds: live_bounds, + states: live_state.map(|state| state.states).unwrap_or_default(), + bounds_from_live: live_bounds.is_some(), + states_from_live, + }; + Ok(json!({ + "visible": evidence.result(), + "applicable": evidence.applicable(), + })) } fn actionable( diff --git a/crates/core/src/lib.rs b/crates/core/src/lib.rs index 8a2e5203..9fb07f81 100644 --- a/crates/core/src/lib.rs +++ b/crates/core/src/lib.rs @@ -29,6 +29,7 @@ pub mod refs_store; #[cfg(test)] mod refs_test_support; pub(crate) mod resolved_element; +pub mod role; pub mod roles; pub mod screenshot_target; pub(crate) mod search_text; @@ -36,6 +37,7 @@ pub mod session; pub mod snapshot; pub mod snapshot_ref; pub mod snapshot_surface; +pub mod state; pub(crate) mod trace; pub(crate) mod trace_artifacts; pub mod trace_read; diff --git a/crates/core/src/role.rs b/crates/core/src/role.rs new file mode 100644 index 00000000..24718e06 --- /dev/null +++ b/crates/core/src/role.rs @@ -0,0 +1,139 @@ +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] +pub enum Role { + Button, + Cell, + Checkbox, + Colorwell, + Combobox, + Disclosure, + Dockitem, + Group, + Image, + Incrementor, + Link, + List, + Menubutton, + Menuitem, + Radiobutton, + Scrollarea, + Slider, + Statictext, + Switch, + Tab, + Table, + Textfield, + Treeitem, + Unknown, +} + +impl Role { + pub fn as_str(self) -> &'static str { + match self { + Self::Button => "button", + Self::Cell => "cell", + Self::Checkbox => "checkbox", + Self::Colorwell => "colorwell", + Self::Combobox => "combobox", + Self::Disclosure => "disclosure", + Self::Dockitem => "dockitem", + Self::Group => "group", + Self::Image => "image", + Self::Incrementor => "incrementor", + Self::Link => "link", + Self::List => "list", + Self::Menubutton => "menubutton", + Self::Menuitem => "menuitem", + Self::Radiobutton => "radiobutton", + Self::Scrollarea => "scrollarea", + Self::Slider => "slider", + Self::Statictext => "statictext", + Self::Switch => "switch", + Self::Tab => "tab", + Self::Table => "table", + Self::Textfield => "textfield", + Self::Treeitem => "treeitem", + Self::Unknown => "unknown", + } + } + + pub fn parse(role: &str) -> Self { + role.parse().unwrap_or(Self::Unknown) + } +} + +impl std::str::FromStr for Role { + type Err = std::convert::Infallible; + + fn from_str(role: &str) -> Result { + Ok(match role.trim().to_ascii_lowercase().as_str() { + "button" => Self::Button, + "cell" => Self::Cell, + "checkbox" => Self::Checkbox, + "colorwell" => Self::Colorwell, + "combobox" => Self::Combobox, + "disclosure" => Self::Disclosure, + "dockitem" => Self::Dockitem, + "group" => Self::Group, + "image" => Self::Image, + "incrementor" => Self::Incrementor, + "link" => Self::Link, + "list" => Self::List, + "menubutton" => Self::Menubutton, + "menuitem" => Self::Menuitem, + "radiobutton" => Self::Radiobutton, + "scrollarea" => Self::Scrollarea, + "slider" => Self::Slider, + "statictext" => Self::Statictext, + "switch" => Self::Switch, + "tab" => Self::Tab, + "table" => Self::Table, + "textfield" => Self::Textfield, + "treeitem" => Self::Treeitem, + _ => Self::Unknown, + }) + } +} + +impl Role { + pub fn is_interactive(self) -> bool { + matches!( + self, + Self::Button + | Self::Cell + | Self::Checkbox + | Self::Colorwell + | Self::Combobox + | Self::Dockitem + | Self::Incrementor + | Self::Link + | Self::Menubutton + | Self::Menuitem + | Self::Radiobutton + | Self::Slider + | Self::Switch + | Self::Tab + | Self::Textfield + | Self::Treeitem + ) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn from_str_unknown_for_bogus_role() { + assert_eq!(Role::parse("bogus"), Role::Unknown); + } + + #[test] + fn is_interactive_matches_interactive_roles_list() { + for role in crate::roles::INTERACTIVE_ROLES { + assert!( + Role::parse(role).is_interactive(), + "{role} should be interactive" + ); + } + } +} diff --git a/crates/core/src/roles.rs b/crates/core/src/roles.rs index f209ceb6..b0323aee 100644 --- a/crates/core/src/roles.rs +++ b/crates/core/src/roles.rs @@ -1,3 +1,5 @@ +use crate::role::Role; + /// Interactive roles that receive refs during snapshot allocation. /// /// Each entry must be produced by at least one platform adapter's native-to-canonical @@ -39,7 +41,7 @@ pub fn normalize_role_query(role: &str) -> String { /// Returns true when `role` is in [`INTERACTIVE_ROLES`]. pub fn is_interactive_role(role: &str) -> bool { - INTERACTIVE_ROLES.contains(&role) + Role::parse(role).is_interactive() } /// Returns true for roles whose checked/unchecked state can be queried and set. diff --git a/crates/core/src/state.rs b/crates/core/src/state.rs new file mode 100644 index 00000000..a5509ce3 --- /dev/null +++ b/crates/core/src/state.rs @@ -0,0 +1,133 @@ +use crate::node::Rect; + +pub const FOCUSED: &str = "focused"; +pub const DISABLED: &str = "disabled"; +pub const SECURE: &str = "secure"; +pub const EXPANDED: &str = "expanded"; +pub const CHECKED: &str = "checked"; +pub const SELECTED: &str = "selected"; +pub const HIDDEN: &str = "hidden"; +pub const BUSY: &str = "busy"; +pub const MODAL: &str = "modal"; +pub const REQUIRED: &str = "required"; +pub const INDETERMINATE: &str = "indeterminate"; +pub const PRESSED: &str = "pressed"; +pub const READONLY: &str = "readonly"; +pub const OFFSCREEN: &str = "offscreen"; +pub const INVALID: &str = "invalid"; +pub const MULTISELECTABLE: &str = "multiselectable"; +pub const HASPOPUP: &str = "haspopup"; + +pub const STATE_VOCABULARY: &[&str] = &[ + FOCUSED, + DISABLED, + SECURE, + EXPANDED, + CHECKED, + SELECTED, + HIDDEN, + BUSY, + MODAL, + REQUIRED, + INDETERMINATE, + PRESSED, + READONLY, + OFFSCREEN, + INVALID, + MULTISELECTABLE, + HASPOPUP, +]; + +pub fn has_state(states: &[String], token: &str) -> bool { + states.iter().any(|state| state == token) +} + +pub fn assert_states_in_vocabulary(states: &[String]) { + for state in states { + assert!( + STATE_VOCABULARY.contains(&state.as_str()), + "state token '{state}' is not in STATE_VOCABULARY" + ); + } +} + +pub fn is_visible(bounds: Option, states: &[String]) -> bool { + crate::actionability::bounds_are_visible(bounds) + && !has_state(states, HIDDEN) + && !has_state(states, OFFSCREEN) +} + +pub struct VisibilityEvidence { + pub bounds: Option, + pub states: Vec, + pub bounds_from_live: bool, + pub states_from_live: bool, +} + +impl VisibilityEvidence { + pub fn applicable(&self) -> bool { + self.bounds_from_live && self.states_from_live + } + + pub fn result(&self) -> bool { + if !self.applicable() { + return false; + } + is_visible(self.bounds, &self.states) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::node::Rect; + + #[test] + fn vocabulary_contains_seventeen_tokens() { + assert_eq!(STATE_VOCABULARY.len(), 17); + } + + #[test] + fn hidden_element_is_not_visible() { + let bounds = Some(Rect { + x: 0.0, + y: 0.0, + width: 10.0, + height: 10.0, + }); + assert!(!is_visible(bounds, &[HIDDEN.to_string()])); + } + + #[test] + fn zero_sized_bounds_are_not_visible() { + let bounds = Some(Rect { + x: 0.0, + y: 0.0, + width: 0.0, + height: 10.0, + }); + assert!(!is_visible(bounds, &[])); + } + + #[test] + fn offscreen_element_is_not_visible() { + let bounds = Some(Rect { + x: 0.0, + y: 0.0, + width: 10.0, + height: 10.0, + }); + assert!(!is_visible(bounds, &[OFFSCREEN.to_string()])); + } + + #[test] + fn visible_element_with_live_evidence() { + let bounds = Some(Rect { + x: 0.0, + y: 0.0, + width: 10.0, + height: 10.0, + }); + assert!(is_visible(bounds, &[])); + } +}