diff --git a/crates/ely_app/src/shell/web_surface.rs b/crates/ely_app/src/shell/web_surface.rs index 5197ff5..304d2f2 100644 --- a/crates/ely_app/src/shell/web_surface.rs +++ b/crates/ely_app/src/shell/web_surface.rs @@ -11,8 +11,8 @@ use super::{ web_surface_permissions::WebSurfaceSitePermission, web_surface_runtime::{WebSurfaceRuntime, WebSurfaceRuntimeFrame}, web_surface_state::{ - PerTabSurface, WebSurfaceClickState, WebSurfaceKeyboardFocusState, WebSurfacePendingInput, - WebSurfaceScrollState, WebSurfaceState, WebSurfaceTextInputState, + PerTabSurface, WebSurfaceClickState, WebSurfaceInputOutcome, WebSurfaceKeyboardFocusState, + WebSurfacePendingInput, WebSurfaceScrollState, WebSurfaceState, WebSurfaceTextInputState, }, }; @@ -103,9 +103,9 @@ impl WebSurfaceStore { requested_url: &str, delta: Point, scale_factor: f32, - ) -> bool { + ) -> WebSurfaceInputOutcome { let Some(delta) = WebSurfaceScrollDelta::from_point(delta, scale_factor) else { - return false; + return WebSurfaceInputOutcome::DroppedZeroDelta; }; let surface = self.surface_mut(tab_id); @@ -128,7 +128,7 @@ impl WebSurfaceStore { // its own DOM focus across scrolls, so a focused input keeps // accepting the user's keystrokes after they wheel-scroll. surface.click_point = None; - true + WebSurfaceInputOutcome::Applied } pub(super) fn record_viewport_size( @@ -136,9 +136,9 @@ impl WebSurfaceStore { tab_id: &TabId, bounds: Bounds, scale_factor: f32, - ) -> bool { + ) -> WebSurfaceInputOutcome { let Some(size) = WebSurfaceSize::from_bounds(bounds, scale_factor) else { - return false; + return WebSurfaceInputOutcome::DroppedInvalidBounds; }; let surface = self.surface_mut(tab_id); surface.viewport_bounds = Some(bounds); @@ -146,22 +146,22 @@ impl WebSurfaceStore { let Some(current_size) = surface.viewport_size else { surface.viewport_size = Some(size); surface.pending_viewport_size = None; - return true; + return WebSurfaceInputOutcome::Applied; }; if current_size == size { surface.pending_viewport_size = None; - return false; + return WebSurfaceInputOutcome::NoChange; } if surface.pending_viewport_size != Some(size) { surface.pending_viewport_size = Some(size); - return false; + return WebSurfaceInputOutcome::Buffered; } surface.pending_viewport_size = None; surface.viewport_size = Some(size); - true + WebSurfaceInputOutcome::Applied } pub(super) fn record_hover_point( @@ -169,18 +169,18 @@ impl WebSurfaceStore { tab_id: &TabId, position: Point, scale_factor: f32, - ) -> bool { + ) -> WebSurfaceInputOutcome { let surface = self.surfaces.get_mut(tab_id).filter(|surface| surface.viewport_bounds.is_some()); let Some(surface) = surface else { - return false; + return WebSurfaceInputOutcome::DroppedNoViewportBounds; }; let bounds = surface.viewport_bounds.expect("viewport_bounds checked above"); let Some(point) = WebSurfaceClickPoint::from_window_position(bounds, position, scale_factor) else { - return false; + return WebSurfaceInputOutcome::DroppedOutOfBounds; }; surface.hover_point = Some(point); - true + WebSurfaceInputOutcome::Applied } pub(super) fn record_click_point( @@ -189,15 +189,15 @@ impl WebSurfaceStore { requested_url: &str, position: Point, scale_factor: f32, - ) -> bool { + ) -> WebSurfaceInputOutcome { let Some(bounds) = self.surfaces.get(tab_id).and_then(|surface| surface.viewport_bounds) else { - return false; + return WebSurfaceInputOutcome::DroppedNoViewportBounds; }; let Some(point) = WebSurfaceClickPoint::from_window_position(bounds, position, scale_factor) else { - return false; + return WebSurfaceInputOutcome::DroppedOutOfBounds; }; let scroll_offset = self @@ -220,7 +220,7 @@ impl WebSurfaceStore { let surface = self.surface_mut(tab_id); surface.typed_text = None; surface.click_point = Some(state); - true + WebSurfaceInputOutcome::Applied } pub(super) fn record_typed_text( @@ -228,15 +228,15 @@ impl WebSurfaceStore { tab_id: &TabId, requested_url: &str, text: &str, - ) -> bool { + ) -> WebSurfaceInputOutcome { if text.is_empty() { - return false; + return WebSurfaceInputOutcome::DroppedEmptyText; } let Some(focus) = self.keyboard_focus.as_ref() else { - return false; + return WebSurfaceInputOutcome::DroppedNoKeyboardFocus; }; if focus.tab_id != *tab_id || focus.requested_url != requested_url { - return false; + return WebSurfaceInputOutcome::DroppedFocusMismatch; } let scroll_offset = focus.scroll_offset; @@ -262,7 +262,7 @@ impl WebSurfaceStore { } entry.text.push_str(text); - true + WebSurfaceInputOutcome::Applied } fn take_pending_input( diff --git a/crates/ely_app/src/shell/web_surface_controller.rs b/crates/ely_app/src/shell/web_surface_controller.rs index 824e489..4c53ce6 100644 --- a/crates/ely_app/src/shell/web_surface_controller.rs +++ b/crates/ely_app/src/shell/web_surface_controller.rs @@ -7,7 +7,7 @@ use crate::services::ProfileDataMode; use super::{ ElyShell, web_surface_permissions::web_surface_site_permissions_for_tab, - web_surface_state::WebSurfaceState, + web_surface_state::{WebSurfaceInputOutcome, WebSurfaceState}, web_surface_view::{ render_failed_web_surface, render_loading_web_surface, render_ready_web_surface, }, @@ -55,7 +55,9 @@ impl ElyShell { scale_factor: f32, cx: &mut Context, ) { - if self.web_surfaces.record_viewport_size(&tab_id, bounds, scale_factor) { + if self.web_surfaces.record_viewport_size(&tab_id, bounds, scale_factor) + == WebSurfaceInputOutcome::Applied + { cx.notify(); } } @@ -73,7 +75,8 @@ impl ElyShell { requested_url.as_str(), delta, scale_factor, - ) { + ) == WebSurfaceInputOutcome::Applied + { cx.notify(); } } @@ -85,7 +88,9 @@ impl ElyShell { scale_factor: f32, cx: &mut Context, ) { - if self.web_surfaces.record_hover_point(&tab_id, position, scale_factor) { + if self.web_surfaces.record_hover_point(&tab_id, position, scale_factor) + == WebSurfaceInputOutcome::Applied + { cx.notify(); } } @@ -105,7 +110,8 @@ impl ElyShell { requested_url.as_str(), position, scale_factor, - ) { + ) == WebSurfaceInputOutcome::Applied + { cx.notify(); } } @@ -125,7 +131,9 @@ impl ElyShell { text: &str, cx: &mut Context, ) -> bool { - if self.web_surfaces.record_typed_text(&tab_id, requested_url.as_str(), text) { + if self.web_surfaces.record_typed_text(&tab_id, requested_url.as_str(), text) + == WebSurfaceInputOutcome::Applied + { cx.notify(); return true; } diff --git a/crates/ely_app/src/shell/web_surface_live_site_tests.rs b/crates/ely_app/src/shell/web_surface_live_site_tests.rs index 54c1abf..b8f699c 100644 --- a/crates/ely_app/src/shell/web_surface_live_site_tests.rs +++ b/crates/ely_app/src/shell/web_surface_live_site_tests.rs @@ -20,7 +20,7 @@ use crate::{ shell::{ web_surface_frame::WebSurfaceFrame, web_surface_geometry::{WebSurfaceScrollOffset, WebSurfaceSize}, - web_surface_state::WebSurfaceState, + web_surface_state::{WebSurfaceInputOutcome, WebSurfaceState}, }, }; @@ -101,7 +101,12 @@ fn render_web_surface_frame( for attempt in 0..LIVE_SITE_RENDER_ATTEMPTS { let tab = web_tab(profile_id.clone(), case.url)?; - assert!(store.record_viewport_size(tab.id(), live_surface_bounds(), 1.0), "{}", case.url); + assert_eq!( + store.record_viewport_size(tab.id(), live_surface_bounds(), 1.0), + WebSurfaceInputOutcome::Applied, + "{}", + case.url, + ); store.ensure_surface(&tab, ProfileDataMode::Transient, &[]); match wait_for_ready_frame(store, tab.id(), case) { diff --git a/crates/ely_app/src/shell/web_surface_state.rs b/crates/ely_app/src/shell/web_surface_state.rs index 0be37ad..8632389 100644 --- a/crates/ely_app/src/shell/web_surface_state.rs +++ b/crates/ely_app/src/shell/web_surface_state.rs @@ -50,6 +50,46 @@ pub(super) struct WebSurfacePendingInput { pub(super) typed_text: Option, } +/// Outcome of a `WebSurfaceStore::record_*` call. +/// +/// Replaces the previous `-> bool` return so silent rejections name +/// themselves at the call site. Callers only branch on `Applied` +/// (every other variant means "do nothing, don't notify"), but each +/// `Dropped*` / non-`Applied` variant pins down *why* a coordinate +/// or keystroke never reached the runtime — so the next regression +/// shows up as a specific variant in tests instead of a missing +/// repaint. `#[must_use]` keeps a future caller from dropping the +/// outcome on the floor and reintroducing the silent-fail pattern. +#[must_use] +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub(super) enum WebSurfaceInputOutcome { + /// State changed and the renderer should re-notify. + Applied, + /// Same value as currently recorded — nothing to flush downstream. + NoChange, + /// First sighting of a new value; held back until a second + /// matching measurement confirms it (viewport-resize debounce). + Buffered, + /// Geometry constructor rejected the input (zero/NaN/negative + /// bounds). The viewport never measured cleanly. + DroppedInvalidBounds, + /// Viewport bounds have not been recorded yet, so window-relative + /// coordinates can't be translated into the page coordinate space. + DroppedNoViewportBounds, + /// Window position falls outside the viewport rect after scaling. + DroppedOutOfBounds, + /// Wheel delta rounded to zero device pixels in both axes. + DroppedZeroDelta, + /// Empty string passed to `record_typed_text` — nothing to buffer. + DroppedEmptyText, + /// `record_typed_text` ran before any click established + /// keyboard focus on this surface. + DroppedNoKeyboardFocus, + /// Keyboard focus belongs to a different tab or the URL drifted + /// (redirect / trailing-slash mismatch) since the focusing click. + DroppedFocusMismatch, +} + pub(super) enum WebSurfaceState { Loading { requested_url: String, previous_frame: Option }, Ready(WebSurfaceFrame), diff --git a/crates/ely_app/src/shell/web_surface_tests.rs b/crates/ely_app/src/shell/web_surface_tests.rs index 557d766..9457071 100644 --- a/crates/ely_app/src/shell/web_surface_tests.rs +++ b/crates/ely_app/src/shell/web_surface_tests.rs @@ -4,21 +4,26 @@ use ely_domain::{BrowserTab, ProfileId, SpaceId, TabId, UrlText}; use gpui::{Bounds, point, px, size}; use super::WebSurfaceStore; +use crate::shell::web_surface_state::WebSurfaceInputOutcome; + +fn assert_applied(outcome: WebSurfaceInputOutcome) { + assert_eq!(outcome, WebSurfaceInputOutcome::Applied); +} #[test] fn typed_text_enters_pending_input_after_clicked_viewport() -> Result<(), Box> { let mut store = WebSurfaceStore::new(); let tab = web_tab("https://example.com/form")?; - assert!(store.record_viewport_size(tab.id(), web_bounds(), 1.0)); - assert!(store.record_click_point( + assert_applied(store.record_viewport_size(tab.id(), web_bounds(), 1.0)); + assert_applied(store.record_click_point( tab.id(), tab.url().as_str(), point(px(160.0), px(120.0)), 1.0, )); - assert!(store.record_typed_text(tab.id(), tab.url().as_str(), "e")); - assert!(store.record_typed_text(tab.id(), tab.url().as_str(), "l")); + assert_applied(store.record_typed_text(tab.id(), tab.url().as_str(), "e")); + assert_applied(store.record_typed_text(tab.id(), tab.url().as_str(), "l")); let input = store.take_pending_input(tab.id(), tab.url().as_str()); @@ -32,14 +37,14 @@ fn scroll_delta_enters_pending_input_after_wheel() -> Result<(), Box> let mut store = WebSurfaceStore::new(); let tab = web_tab("https://example.com/list")?; - assert!(store.record_viewport_size(tab.id(), web_bounds(), 1.0)); - assert!(store.record_scroll_delta( + assert_applied(store.record_viewport_size(tab.id(), web_bounds(), 1.0)); + assert_applied(store.record_scroll_delta( tab.id(), tab.url().as_str(), point(px(0.0), px(140.0)), 1.0, )); - assert!(store.record_scroll_delta( + assert_applied(store.record_scroll_delta( tab.id(), tab.url().as_str(), point(px(0.0), px(60.0)), @@ -58,9 +63,13 @@ fn viewport_size_changes_after_stable_second_measurement() -> Result<(), Box Result<(), Box Result<(), Box Result<(), Box> { let tab = web_tab("https://example.com/form")?; let url = tab.url().as_str(); - assert!(store.record_viewport_size(tab.id(), web_bounds(), 1.0)); - assert!( - !store.record_typed_text(tab.id(), url, "x"), + assert_applied(store.record_viewport_size(tab.id(), web_bounds(), 1.0)); + assert_eq!( + store.record_typed_text(tab.id(), url, "x"), + WebSurfaceInputOutcome::DroppedNoKeyboardFocus, "typing must fail until a click establishes keyboard focus on this tab and url", ); Ok(()) } +/// Negative-path coverage for `DroppedNoViewportBounds`: a click that +/// arrives before the viewport has reported its bounds (race during +/// first paint) must surface as a typed outcome, not a silent `false`. +#[test] +fn click_before_viewport_measured_reports_no_viewport_bounds() -> Result<(), Box> { + let mut store = WebSurfaceStore::new(); + let tab = web_tab("https://example.com/form")?; + + assert_eq!( + store.record_click_point( + tab.id(), + tab.url().as_str(), + point(px(160.0), px(120.0)), + 1.0, + ), + WebSurfaceInputOutcome::DroppedNoViewportBounds, + ); + Ok(()) +} + +/// Negative-path coverage for `DroppedZeroDelta`: a wheel event whose +/// device-pixel delta rounds to zero must surface as the explicit +/// outcome so the renderer skips an unnecessary repaint. +#[test] +fn zero_wheel_delta_reports_zero_delta() -> Result<(), Box> { + let mut store = WebSurfaceStore::new(); + let tab = web_tab("https://example.com/list")?; + + assert_applied(store.record_viewport_size(tab.id(), web_bounds(), 1.0)); + assert_eq!( + store.record_scroll_delta( + tab.id(), + tab.url().as_str(), + point(px(0.0), px(0.0)), + 1.0, + ), + WebSurfaceInputOutcome::DroppedZeroDelta, + ); + Ok(()) +} + fn web_bounds() -> Bounds { Bounds::new(point(px(0.0), px(0.0)), size(px(640.0), px(480.0))) }