From a8b5a59d1c1b2c1bcb495ebb5039eeed8b993a5d Mon Sep 17 00:00:00 2001 From: Lahfir Date: Sun, 5 Jul 2026 16:59:28 -0700 Subject: [PATCH] =?UTF-8?q?fix:=20address=204th-pass=20review=20=E2=80=94?= =?UTF-8?q?=20visibility=20ordering,=20mouse=5Fwheel=20tests,=20poll=20dia?= =?UTF-8?q?gnostics?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - actionability::visibility_check: run HIDDEN/OFFSCREEN state checks BEFORE the bounds read (P1 #2). Previously bounds=None short-circuited to unknown, so a hidden/offscreen element whose live bounds read failed slipped the visibility gate. Regression tests added (hidden/offscreen + bounds=None now fail, not unknown). - mouse_wheel: add unit tests (P0 #1) mirroring the sibling mouse-command tests — args reach the adapter unchanged, the scrolled envelope shape, error propagation. - ref_action_wait poll loop: on a retryable resolve failure, record the resolve error into last_report so an actionability_timeout on deadline expiry carries the last STALE_REF/AMBIGUOUS_TARGET context instead of an empty report (P2 #7). Already-fixed in prior passes (review base predates 640e8e8): StepMechanism serde tests (#3), clipboard trait removal doc note (#10). Deferred/accepted: test-adapter boilerplate (#5 -> issue #95), hit-test ancestor + scroll thrashing (#15/#16/#17, intentional design tradeoffs). Verified with cargo test --workspace (1186 passed). --- crates/core/src/actionability/mod.rs | 6 +- crates/core/src/actionability_tests.rs | 28 ++++ crates/core/src/commands/mouse_wheel.rs | 4 + crates/core/src/commands/mouse_wheel_tests.rs | 131 ++++++++++++++++++ crates/core/src/ref_action_wait.rs | 7 + 5 files changed, 173 insertions(+), 3 deletions(-) create mode 100644 crates/core/src/commands/mouse_wheel_tests.rs diff --git a/crates/core/src/actionability/mod.rs b/crates/core/src/actionability/mod.rs index 45c04b27..ba4c3389 100644 --- a/crates/core/src/actionability/mod.rs +++ b/crates/core/src/actionability/mod.rs @@ -117,15 +117,15 @@ fn check_with_stability( } 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"); } + let Some(bounds) = entry.bounds else { + return unknown("visible", "bounds unavailable"); + }; if !bounds_are_visible(Some(bounds)) { return fail("visible", "bounds are zero-sized"); } diff --git a/crates/core/src/actionability_tests.rs b/crates/core/src/actionability_tests.rs index 03788c49..40e448d0 100644 --- a/crates/core/src/actionability_tests.rs +++ b/crates/core/src/actionability_tests.rs @@ -99,6 +99,34 @@ fn offscreen_state_fails_visibility_before_action_dispatch() { assert!(err.message.contains("visible")); } +#[test] +fn hidden_entry_fails_visibility_even_when_bounds_are_none() { + let mut entry = entry(); + entry.states.push(crate::state::HIDDEN.into()); + entry.bounds = None; + entry.bounds_hash = None; + + let err = check(&entry, &ActionRequest::headless(Action::Click)).unwrap_err(); + + assert_eq!(err.code, ErrorCode::ActionFailed); + assert!(err.message.contains("visible")); + assert!(err.message.contains("hidden")); +} + +#[test] +fn offscreen_entry_fails_visibility_even_when_bounds_are_none() { + let mut entry = entry(); + entry.states.push(crate::state::OFFSCREEN.into()); + entry.bounds = None; + entry.bounds_hash = None; + + let err = check(&entry, &ActionRequest::headless(Action::Click)).unwrap_err(); + + assert_eq!(err.code, ErrorCode::ActionFailed); + assert!(err.message.contains("visible")); + assert!(err.message.contains("offscreen")); +} + #[test] fn text_input_requires_editable_target() { let err = check( diff --git a/crates/core/src/commands/mouse_wheel.rs b/crates/core/src/commands/mouse_wheel.rs index 3235c86f..2b5979b3 100644 --- a/crates/core/src/commands/mouse_wheel.rs +++ b/crates/core/src/commands/mouse_wheel.rs @@ -13,3 +13,7 @@ pub fn execute(args: MouseWheelArgs, adapter: &dyn PlatformAdapter) -> Result, +} + +struct WheelCaptureAdapter { + captured: Mutex>, + fail: bool, +} + +impl WheelCaptureAdapter { + fn recording() -> Self { + Self { + captured: Mutex::new(None), + fail: false, + } + } + + fn failing() -> Self { + Self { + captured: Mutex::new(None), + fail: true, + } + } +} + +impl ObservationOps for WheelCaptureAdapter {} +impl ActionOps for WheelCaptureAdapter {} +impl SystemOps for WheelCaptureAdapter {} + +impl InputOps for WheelCaptureAdapter { + fn mouse_wheel( + &self, + x: f64, + y: f64, + dy: i32, + dx: i32, + modifiers: &[Modifier], + ) -> Result<(), AdapterError> { + *self.captured.lock().unwrap() = Some(WheelCall { + x, + y, + dy, + dx, + modifiers: modifiers.to_vec(), + }); + if self.fail { + return Err(AdapterError::not_supported("mouse_wheel")); + } + Ok(()) + } +} + +#[test] +fn requested_wheel_args_reach_the_adapter_unchanged() { + let adapter = WheelCaptureAdapter::recording(); + + execute( + MouseWheelArgs { + x: 10.0, + y: 20.0, + dy: -3, + dx: 5, + modifiers: vec![Modifier::Shift, Modifier::Alt], + }, + &adapter, + ) + .unwrap(); + + let captured = adapter.captured.lock().unwrap(); + let call = captured + .as_ref() + .expect("mouse_wheel must have been called"); + assert_eq!( + *call, + WheelCall { + x: 10.0, + y: 20.0, + dy: -3, + dx: 5, + modifiers: vec![Modifier::Shift, Modifier::Alt], + } + ); +} + +#[test] +fn returns_scrolled_envelope_with_requested_deltas() { + let adapter = WheelCaptureAdapter::recording(); + + let value = execute( + MouseWheelArgs { + x: 0.0, + y: 0.0, + dy: 7, + dx: -2, + modifiers: Vec::new(), + }, + &adapter, + ) + .unwrap(); + + assert_eq!(value, json!({ "scrolled": true, "dy": 7, "dx": -2 })); +} + +#[test] +fn adapter_error_propagates_as_err() { + let adapter = WheelCaptureAdapter::failing(); + + let result = execute( + MouseWheelArgs { + x: 1.0, + y: 2.0, + dy: 1, + dx: 0, + modifiers: Vec::new(), + }, + &adapter, + ); + + assert!(result.is_err()); +} diff --git a/crates/core/src/ref_action_wait.rs b/crates/core/src/ref_action_wait.rs index 8c125634..bf2ba2d6 100644 --- a/crates/core/src/ref_action_wait.rs +++ b/crates/core/src/ref_action_wait.rs @@ -226,6 +226,13 @@ fn execute_poll_loop( trace_resolve_error(ctx.context, ctx.ref_id, &err); return Err(err); } + last_report = Some(json!({ + "resolve_error": { + "code": code.as_str(), + "message": err.message.clone(), + "details": err.details.clone(), + } + })); sleep_poll_interval(deadline); } }