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:
@@ -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<Pixels>,
|
||||
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<Pixels>,
|
||||
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<Pixels>,
|
||||
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<Pixels>,
|
||||
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(
|
||||
|
||||
@@ -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<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();
|
||||
}
|
||||
}
|
||||
@@ -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<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();
|
||||
}
|
||||
}
|
||||
@@ -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<Self>,
|
||||
) -> 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;
|
||||
}
|
||||
|
||||
@@ -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) {
|
||||
|
||||
@@ -50,6 +50,46 @@ pub(super) struct WebSurfacePendingInput {
|
||||
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 {
|
||||
Loading { requested_url: String, previous_frame: Option<WebSurfaceFrame> },
|
||||
Ready(WebSurfaceFrame),
|
||||
|
||||
@@ -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<dyn Error>> {
|
||||
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<dyn Error>>
|
||||
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<dyn
|
||||
let mut store = WebSurfaceStore::new();
|
||||
let tab = web_tab("https://example.com/resize")?;
|
||||
|
||||
assert!(store.record_viewport_size(tab.id(), web_bounds(), 1.0));
|
||||
assert!(!store.record_viewport_size(tab.id(), resized_once_bounds(), 1.0));
|
||||
assert!(store.record_viewport_size(tab.id(), resized_once_bounds(), 1.0));
|
||||
assert_applied(store.record_viewport_size(tab.id(), web_bounds(), 1.0));
|
||||
assert_eq!(
|
||||
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(())
|
||||
}
|
||||
|
||||
@@ -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 url = tab.url().as_str();
|
||||
|
||||
assert!(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!(store.record_typed_text(tab.id(), url, "h"));
|
||||
assert_applied(store.record_viewport_size(tab.id(), web_bounds(), 1.0));
|
||||
assert_applied(store.record_click_point(tab.id(), url, point(px(160.0), px(120.0)), 1.0));
|
||||
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"),
|
||||
WebSurfaceInputOutcome::Applied,
|
||||
"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 url = tab.url().as_str();
|
||||
|
||||
assert!(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!(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_viewport_size(tab.id(), web_bounds(), 2.0));
|
||||
assert_applied(store.record_click_point(tab.id(), url, point(px(160.0), px(120.0)), 2.0));
|
||||
assert_applied(store.record_typed_text(tab.id(), url, "h"));
|
||||
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);
|
||||
|
||||
@@ -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 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<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> {
|
||||
Bounds::new(point(px(0.0), px(0.0)), size(px(640.0), px(480.0)))
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user