Name every silent input rejection with WebSurfaceInputOutcome

bool returns on the five record_* surface inputs collapsed nine real
outcomes into one bit. The cascade we found in this round — silent
click drop because viewport_bounds wasn't measured yet, then
keyboard_focus stays None, then typing also "fails" — looked like
three independent symptoms but was one root cause hiding inside
that bit. Replace the bool with a #[must_use] WebSurfaceInputOutcome
enum so each rejection names itself at the call site.

Variants map 1:1 to real return points in web_surface.rs:
  Applied / NoChange / Buffered — three distinct success-ish states
  the controller already needed to disambiguate (only Applied notifies)
  DroppedInvalidBounds — geometry rejected zero/NaN viewport
  DroppedNoViewportBounds — input arrived before the viewport tracker
  DroppedOutOfBounds — window position outside the viewport rect
  DroppedZeroDelta — wheel rounded to zero device px
  DroppedEmptyText — empty record_typed_text
  DroppedNoKeyboardFocus — type without a prior click
  DroppedFocusMismatch — focus belongs to another tab/url

Behavior preserved: Applied is the only notify trigger, matching the
old `true` semantics. Three new negative-path tests (no_viewport_bounds,
zero_delta, no_keyboard_focus) lock the named drops so a future
regression surfaces as a wrong variant in tests instead of a missing
repaint. cargo test ely_app --bin ely_app web_surface: 17 passed.
This commit is contained in:
2026-05-10 02:00:52 -04:00
parent c20daf36b9
commit b8795bf903
5 changed files with 159 additions and 54 deletions
+24 -24
View File
@@ -11,8 +11,8 @@ use super::{
web_surface_permissions::WebSurfaceSitePermission, web_surface_permissions::WebSurfaceSitePermission,
web_surface_runtime::{WebSurfaceRuntime, WebSurfaceRuntimeFrame}, web_surface_runtime::{WebSurfaceRuntime, WebSurfaceRuntimeFrame},
web_surface_state::{ web_surface_state::{
PerTabSurface, WebSurfaceClickState, WebSurfaceKeyboardFocusState, WebSurfacePendingInput, PerTabSurface, WebSurfaceClickState, WebSurfaceInputOutcome, WebSurfaceKeyboardFocusState,
WebSurfaceScrollState, WebSurfaceState, WebSurfaceTextInputState, WebSurfacePendingInput, WebSurfaceScrollState, WebSurfaceState, WebSurfaceTextInputState,
}, },
}; };
@@ -103,9 +103,9 @@ impl WebSurfaceStore {
requested_url: &str, requested_url: &str,
delta: Point<Pixels>, delta: Point<Pixels>,
scale_factor: f32, scale_factor: f32,
) -> bool { ) -> WebSurfaceInputOutcome {
let Some(delta) = WebSurfaceScrollDelta::from_point(delta, scale_factor) else { let Some(delta) = WebSurfaceScrollDelta::from_point(delta, scale_factor) else {
return false; return WebSurfaceInputOutcome::DroppedZeroDelta;
}; };
let surface = self.surface_mut(tab_id); let surface = self.surface_mut(tab_id);
@@ -128,7 +128,7 @@ impl WebSurfaceStore {
// its own DOM focus across scrolls, so a focused input keeps // its own DOM focus across scrolls, so a focused input keeps
// accepting the user's keystrokes after they wheel-scroll. // accepting the user's keystrokes after they wheel-scroll.
surface.click_point = None; surface.click_point = None;
true WebSurfaceInputOutcome::Applied
} }
pub(super) fn record_viewport_size( pub(super) fn record_viewport_size(
@@ -136,9 +136,9 @@ impl WebSurfaceStore {
tab_id: &TabId, tab_id: &TabId,
bounds: Bounds<Pixels>, bounds: Bounds<Pixels>,
scale_factor: f32, scale_factor: f32,
) -> bool { ) -> WebSurfaceInputOutcome {
let Some(size) = WebSurfaceSize::from_bounds(bounds, scale_factor) else { let Some(size) = WebSurfaceSize::from_bounds(bounds, scale_factor) else {
return false; return WebSurfaceInputOutcome::DroppedInvalidBounds;
}; };
let surface = self.surface_mut(tab_id); let surface = self.surface_mut(tab_id);
surface.viewport_bounds = Some(bounds); surface.viewport_bounds = Some(bounds);
@@ -146,22 +146,22 @@ impl WebSurfaceStore {
let Some(current_size) = surface.viewport_size else { let Some(current_size) = surface.viewport_size else {
surface.viewport_size = Some(size); surface.viewport_size = Some(size);
surface.pending_viewport_size = None; surface.pending_viewport_size = None;
return true; return WebSurfaceInputOutcome::Applied;
}; };
if current_size == size { if current_size == size {
surface.pending_viewport_size = None; surface.pending_viewport_size = None;
return false; return WebSurfaceInputOutcome::NoChange;
} }
if surface.pending_viewport_size != Some(size) { if surface.pending_viewport_size != Some(size) {
surface.pending_viewport_size = Some(size); surface.pending_viewport_size = Some(size);
return false; return WebSurfaceInputOutcome::Buffered;
} }
surface.pending_viewport_size = None; surface.pending_viewport_size = None;
surface.viewport_size = Some(size); surface.viewport_size = Some(size);
true WebSurfaceInputOutcome::Applied
} }
pub(super) fn record_hover_point( pub(super) fn record_hover_point(
@@ -169,18 +169,18 @@ impl WebSurfaceStore {
tab_id: &TabId, tab_id: &TabId,
position: Point<Pixels>, position: Point<Pixels>,
scale_factor: f32, scale_factor: f32,
) -> bool { ) -> WebSurfaceInputOutcome {
let surface = self.surfaces.get_mut(tab_id).filter(|surface| surface.viewport_bounds.is_some()); let surface = self.surfaces.get_mut(tab_id).filter(|surface| surface.viewport_bounds.is_some());
let Some(surface) = surface else { let Some(surface) = surface else {
return false; return WebSurfaceInputOutcome::DroppedNoViewportBounds;
}; };
let bounds = surface.viewport_bounds.expect("viewport_bounds checked above"); let bounds = surface.viewport_bounds.expect("viewport_bounds checked above");
let Some(point) = WebSurfaceClickPoint::from_window_position(bounds, position, scale_factor) let Some(point) = WebSurfaceClickPoint::from_window_position(bounds, position, scale_factor)
else { else {
return false; return WebSurfaceInputOutcome::DroppedOutOfBounds;
}; };
surface.hover_point = Some(point); surface.hover_point = Some(point);
true WebSurfaceInputOutcome::Applied
} }
pub(super) fn record_click_point( pub(super) fn record_click_point(
@@ -189,15 +189,15 @@ impl WebSurfaceStore {
requested_url: &str, requested_url: &str,
position: Point<Pixels>, position: Point<Pixels>,
scale_factor: f32, scale_factor: f32,
) -> bool { ) -> WebSurfaceInputOutcome {
let Some(bounds) = let Some(bounds) =
self.surfaces.get(tab_id).and_then(|surface| surface.viewport_bounds) self.surfaces.get(tab_id).and_then(|surface| surface.viewport_bounds)
else { else {
return false; return WebSurfaceInputOutcome::DroppedNoViewportBounds;
}; };
let Some(point) = WebSurfaceClickPoint::from_window_position(bounds, position, scale_factor) let Some(point) = WebSurfaceClickPoint::from_window_position(bounds, position, scale_factor)
else { else {
return false; return WebSurfaceInputOutcome::DroppedOutOfBounds;
}; };
let scroll_offset = self let scroll_offset = self
@@ -220,7 +220,7 @@ impl WebSurfaceStore {
let surface = self.surface_mut(tab_id); let surface = self.surface_mut(tab_id);
surface.typed_text = None; surface.typed_text = None;
surface.click_point = Some(state); surface.click_point = Some(state);
true WebSurfaceInputOutcome::Applied
} }
pub(super) fn record_typed_text( pub(super) fn record_typed_text(
@@ -228,15 +228,15 @@ impl WebSurfaceStore {
tab_id: &TabId, tab_id: &TabId,
requested_url: &str, requested_url: &str,
text: &str, text: &str,
) -> bool { ) -> WebSurfaceInputOutcome {
if text.is_empty() { if text.is_empty() {
return false; return WebSurfaceInputOutcome::DroppedEmptyText;
} }
let Some(focus) = self.keyboard_focus.as_ref() else { 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 { if focus.tab_id != *tab_id || focus.requested_url != requested_url {
return false; return WebSurfaceInputOutcome::DroppedFocusMismatch;
} }
let scroll_offset = focus.scroll_offset; let scroll_offset = focus.scroll_offset;
@@ -262,7 +262,7 @@ impl WebSurfaceStore {
} }
entry.text.push_str(text); entry.text.push_str(text);
true WebSurfaceInputOutcome::Applied
} }
fn take_pending_input( fn take_pending_input(
@@ -7,7 +7,7 @@ use crate::services::ProfileDataMode;
use super::{ use super::{
ElyShell, ElyShell,
web_surface_permissions::web_surface_site_permissions_for_tab, web_surface_permissions::web_surface_site_permissions_for_tab,
web_surface_state::WebSurfaceState, web_surface_state::{WebSurfaceInputOutcome, WebSurfaceState},
web_surface_view::{ web_surface_view::{
render_failed_web_surface, render_loading_web_surface, render_ready_web_surface, render_failed_web_surface, render_loading_web_surface, render_ready_web_surface,
}, },
@@ -55,7 +55,9 @@ impl ElyShell {
scale_factor: f32, scale_factor: f32,
cx: &mut Context<Self>, cx: &mut Context<Self>,
) { ) {
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(); cx.notify();
} }
} }
@@ -73,7 +75,8 @@ impl ElyShell {
requested_url.as_str(), requested_url.as_str(),
delta, delta,
scale_factor, scale_factor,
) { ) == WebSurfaceInputOutcome::Applied
{
cx.notify(); cx.notify();
} }
} }
@@ -85,7 +88,9 @@ impl ElyShell {
scale_factor: f32, scale_factor: f32,
cx: &mut Context<Self>, cx: &mut Context<Self>,
) { ) {
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(); cx.notify();
} }
} }
@@ -105,7 +110,8 @@ impl ElyShell {
requested_url.as_str(), requested_url.as_str(),
position, position,
scale_factor, scale_factor,
) { ) == WebSurfaceInputOutcome::Applied
{
cx.notify(); cx.notify();
} }
} }
@@ -125,7 +131,9 @@ impl ElyShell {
text: &str, text: &str,
cx: &mut Context<Self>, cx: &mut Context<Self>,
) -> bool { ) -> 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(); cx.notify();
return true; return true;
} }
@@ -20,7 +20,7 @@ use crate::{
shell::{ shell::{
web_surface_frame::WebSurfaceFrame, web_surface_frame::WebSurfaceFrame,
web_surface_geometry::{WebSurfaceScrollOffset, WebSurfaceSize}, 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 { for attempt in 0..LIVE_SITE_RENDER_ATTEMPTS {
let tab = web_tab(profile_id.clone(), case.url)?; 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, &[]); store.ensure_surface(&tab, ProfileDataMode::Transient, &[]);
match wait_for_ready_frame(store, tab.id(), case) { match wait_for_ready_frame(store, tab.id(), case) {
@@ -50,6 +50,46 @@ pub(super) struct WebSurfacePendingInput {
pub(super) typed_text: Option<String>, pub(super) typed_text: Option<String>,
} }
/// 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 { pub(super) enum WebSurfaceState {
Loading { requested_url: String, previous_frame: Option<WebSurfaceFrame> }, Loading { requested_url: String, previous_frame: Option<WebSurfaceFrame> },
Ready(WebSurfaceFrame), Ready(WebSurfaceFrame),
+74 -22
View File
@@ -4,21 +4,26 @@ use ely_domain::{BrowserTab, ProfileId, SpaceId, TabId, UrlText};
use gpui::{Bounds, point, px, size}; use gpui::{Bounds, point, px, size};
use super::WebSurfaceStore; use super::WebSurfaceStore;
use crate::shell::web_surface_state::WebSurfaceInputOutcome;
fn assert_applied(outcome: WebSurfaceInputOutcome) {
assert_eq!(outcome, WebSurfaceInputOutcome::Applied);
}
#[test] #[test]
fn typed_text_enters_pending_input_after_clicked_viewport() -> Result<(), Box<dyn Error>> { fn typed_text_enters_pending_input_after_clicked_viewport() -> Result<(), Box<dyn Error>> {
let mut store = WebSurfaceStore::new(); let mut store = WebSurfaceStore::new();
let tab = web_tab("https://example.com/form")?; let tab = web_tab("https://example.com/form")?;
assert!(store.record_viewport_size(tab.id(), web_bounds(), 1.0)); assert_applied(store.record_viewport_size(tab.id(), web_bounds(), 1.0));
assert!(store.record_click_point( assert_applied(store.record_click_point(
tab.id(), tab.id(),
tab.url().as_str(), tab.url().as_str(),
point(px(160.0), px(120.0)), point(px(160.0), px(120.0)),
1.0, 1.0,
)); ));
assert!(store.record_typed_text(tab.id(), tab.url().as_str(), "e")); assert_applied(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(), "l"));
let input = store.take_pending_input(tab.id(), tab.url().as_str()); 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<dyn Error>>
let mut store = WebSurfaceStore::new(); let mut store = WebSurfaceStore::new();
let tab = web_tab("https://example.com/list")?; let tab = web_tab("https://example.com/list")?;
assert!(store.record_viewport_size(tab.id(), web_bounds(), 1.0)); assert_applied(store.record_viewport_size(tab.id(), web_bounds(), 1.0));
assert!(store.record_scroll_delta( assert_applied(store.record_scroll_delta(
tab.id(), tab.id(),
tab.url().as_str(), tab.url().as_str(),
point(px(0.0), px(140.0)), point(px(0.0), px(140.0)),
1.0, 1.0,
)); ));
assert!(store.record_scroll_delta( assert_applied(store.record_scroll_delta(
tab.id(), tab.id(),
tab.url().as_str(), tab.url().as_str(),
point(px(0.0), px(60.0)), point(px(0.0), px(60.0)),
@@ -58,9 +63,13 @@ fn viewport_size_changes_after_stable_second_measurement() -> Result<(), Box<dyn
let mut store = WebSurfaceStore::new(); let mut store = WebSurfaceStore::new();
let tab = web_tab("https://example.com/resize")?; let tab = web_tab("https://example.com/resize")?;
assert!(store.record_viewport_size(tab.id(), web_bounds(), 1.0)); assert_applied(store.record_viewport_size(tab.id(), web_bounds(), 1.0));
assert!(!store.record_viewport_size(tab.id(), resized_once_bounds(), 1.0)); assert_eq!(
assert!(store.record_viewport_size(tab.id(), resized_once_bounds(), 1.0)); store.record_viewport_size(tab.id(), resized_once_bounds(), 1.0),
WebSurfaceInputOutcome::Buffered,
"first sighting of a new size must wait for a confirming second measurement",
);
assert_applied(store.record_viewport_size(tab.id(), resized_once_bounds(), 1.0));
Ok(()) Ok(())
} }
@@ -78,14 +87,15 @@ fn scroll_after_click_keeps_keyboard_focus_and_typed_text() -> Result<(), Box<dy
let tab = web_tab("https://example.com/form")?; let tab = web_tab("https://example.com/form")?;
let url = tab.url().as_str(); let url = tab.url().as_str();
assert!(store.record_viewport_size(tab.id(), web_bounds(), 1.0)); assert_applied(store.record_viewport_size(tab.id(), web_bounds(), 1.0));
assert!(store.record_click_point(tab.id(), url, point(px(160.0), px(120.0)), 1.0)); assert_applied(store.record_click_point(tab.id(), url, point(px(160.0), px(120.0)), 1.0));
assert!(store.record_typed_text(tab.id(), url, "h")); assert_applied(store.record_typed_text(tab.id(), url, "h"));
assert!(store.record_scroll_delta(tab.id(), url, point(px(0.0), px(140.0)), 1.0)); assert_applied(store.record_scroll_delta(tab.id(), url, point(px(0.0), px(140.0)), 1.0));
assert!( assert_eq!(
store.record_typed_text(tab.id(), url, "i"), store.record_typed_text(tab.id(), url, "i"),
WebSurfaceInputOutcome::Applied,
"scroll must not erase keyboard focus — typing after a scroll should still buffer", "scroll must not erase keyboard focus — typing after a scroll should still buffer",
); );
@@ -121,10 +131,10 @@ fn retina_scale_factor_doubles_every_input_coordinate() -> Result<(), Box<dyn Er
let tab = web_tab("https://example.com/form")?; let tab = web_tab("https://example.com/form")?;
let url = tab.url().as_str(); let url = tab.url().as_str();
assert!(store.record_viewport_size(tab.id(), web_bounds(), 2.0)); assert_applied(store.record_viewport_size(tab.id(), web_bounds(), 2.0));
assert!(store.record_click_point(tab.id(), url, point(px(160.0), px(120.0)), 2.0)); assert_applied(store.record_click_point(tab.id(), url, point(px(160.0), px(120.0)), 2.0));
assert!(store.record_typed_text(tab.id(), url, "h")); assert_applied(store.record_typed_text(tab.id(), url, "h"));
assert!(store.record_scroll_delta(tab.id(), url, point(px(0.0), px(140.0)), 2.0)); assert_applied(store.record_scroll_delta(tab.id(), url, point(px(0.0), px(140.0)), 2.0));
let input = store.take_pending_input(tab.id(), url); let input = store.take_pending_input(tab.id(), url);
@@ -153,14 +163,56 @@ fn typing_without_a_prior_click_is_rejected() -> Result<(), Box<dyn Error>> {
let tab = web_tab("https://example.com/form")?; let tab = web_tab("https://example.com/form")?;
let url = tab.url().as_str(); let url = tab.url().as_str();
assert!(store.record_viewport_size(tab.id(), web_bounds(), 1.0)); assert_applied(store.record_viewport_size(tab.id(), web_bounds(), 1.0));
assert!( assert_eq!(
!store.record_typed_text(tab.id(), url, "x"), store.record_typed_text(tab.id(), url, "x"),
WebSurfaceInputOutcome::DroppedNoKeyboardFocus,
"typing must fail until a click establishes keyboard focus on this tab and url", "typing must fail until a click establishes keyboard focus on this tab and url",
); );
Ok(()) 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<dyn Error>> {
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<dyn Error>> {
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<gpui::Pixels> { fn web_bounds() -> Bounds<gpui::Pixels> {
Bounds::new(point(px(0.0), px(0.0)), size(px(640.0), px(480.0))) Bounds::new(point(px(0.0), px(0.0)), size(px(640.0), px(480.0)))
} }