From 840255f88c37c0637d37776bea6b43e208c23413 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E9=9B=B7=E7=94=B5=E8=8A=BD=E8=A1=A3?= Date: Sun, 10 May 2026 17:38:10 -0400 Subject: [PATCH] Lift the web canvas out of in-flow so the input overlay lands on screen MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A GPUI harness boots a real ElyShell, navigates to an external URL, and asks the input_overlay's sibling canvas tracker where it laid out. On main before this change the canvas reports Bounds { origin: (309, window_height - 17), size: (W - 326, content_h) } i.e. the overlay's top edge sits at the very bottom of the visible window. Every user click in the visible area lands above (or beside) the overlay; the on_mouse_down + capture_any_mouse_up listeners never even see the event because the hitbox is off-screen. Twelve commits chased focus/coords/outcome enums on the sidecar side while every click in the live shell hit empty space. Root cause: in render_web_surface the rendered web image (img / loading div / error page) was a non-absolute child of a `.relative().size_full()` wrapper. The non-absolute child claims `size_full` block-flow height inside that wrapper, which made the wrapper's intrinsic height content_height + content_height. The two `.absolute().size_full()` siblings (viewport_tracker, input_overlay) then sized against that inflated parent and were positioned in the bottom half — exactly content_height below where they were supposed to be. Fix: keep the relative wrapper as the layout owner of the panel slot (size_full, overflow_hidden, min_w_0) and put the rendered image into an absolute `inset_0` child of its own. viewport_tracker and input_overlay stay as absolute siblings. With the image out of in-flow the wrapper sizes to its parent and the overlay's hitbox lands at y = top of content area (71 in a 1080-tall window) instead of y = window_height - 17. GPUI test harness (`gpui_harness_tests.rs`) is the holdout set: - `baseline_overlay_div_receives_simulated_click` proves GPUI's occlude + capture_any_mouse_up primitive works under TestAppContext. - `baseline_overlay_with_full_listener_combo_receives_click` proves the exact listener combo render_input_overlay uses works in isolation. - `ely_shell_external_canvas_lays_out_inside_window` boots a real ElyShell, navigates, and asserts the overlay's measured bounds fit inside the visible window. Without the fix above, this test trips on bounds extending below the window bottom. The three new store-layer tests in web_surface_tests.rs pin per-tab isolation, zero-delta short-circuit, and resize-mid-drain decoupling invariants the harness work flushed out. ely_app picks up gpui's test-support feature as a dev-dependency so the harness can use VisualTestContext + simulate_mouse_*. cargo test --bin ely_app: 112 passed (was 108 + 4 new harness/store tests). Remaining work (not in this commit): even with the layout fixed, the harness shows MouseUp's capture_any_mouse_up still doesn't fire on the ElyShell tree, while MouseDown's bubble does. Some sibling/ancestor listener in the live shell is eating the MouseUp capture phase that the standalone listener-combo baseline does not. Tracked separately. --- Cargo.lock | 29 +++ crates/ely_app/Cargo.toml | 3 + .../ely_app/src/shell/gpui_harness_tests.rs | 197 ++++++++++++++++++ crates/ely_app/src/shell/mod.rs | 3 + crates/ely_app/src/shell/web_surface.rs | 5 + crates/ely_app/src/shell/web_surface_tests.rs | 114 ++++++++++ crates/ely_app/src/shell/web_surface_view.rs | 15 +- 7 files changed, 358 insertions(+), 8 deletions(-) create mode 100644 crates/ely_app/src/shell/gpui_harness_tests.rs diff --git a/Cargo.lock b/Cargo.lock index 34202fc..f9a5ac8 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3125,6 +3125,19 @@ version = "0.32.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "e629b9b98ef3dd8afe6ca2bd0f89306cec16d43d907889945bc5d6687f2f13c7" +[[package]] +name = "git2" +version = "0.20.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7b88256088d75a56f8ecfa070513a775dd9107f6530ef14919dac831af9cfe2b" +dependencies = [ + "bitflags 2.11.1", + "libc", + "libgit2-sys", + "log", + "url", +] + [[package]] name = "gl_generator" version = "0.14.0" @@ -3289,6 +3302,7 @@ dependencies = [ "as-raw-xcb-connection", "ashpd 0.11.1", "async-task", + "backtrace", "bindgen 0.71.1", "blade-graphics", "blade-macros", @@ -3567,12 +3581,15 @@ dependencies = [ "dunce", "futures", "futures-lite 1.13.0", + "git2", "globset", "gpui_collections", + "gpui_util_macros", "itertools 0.14.0", "libc", "log", "nix 0.29.0", + "rand 0.9.4", "regex", "rust-embed", "schemars", @@ -5122,6 +5139,18 @@ dependencies = [ "cc", ] +[[package]] +name = "libgit2-sys" +version = "0.18.4+1.9.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9b26f66f35e1871b22efcf7191564123d2a446ca0538cde63c23adfefa9b15b7" +dependencies = [ + "cc", + "libc", + "libz-sys", + "pkg-config", +] + [[package]] name = "libloading" version = "0.8.9" diff --git a/crates/ely_app/Cargo.toml b/crates/ely_app/Cargo.toml index c654ace..8ca27fb 100644 --- a/crates/ely_app/Cargo.toml +++ b/crates/ely_app/Cargo.toml @@ -25,6 +25,9 @@ thiserror.workspace = true ureq.workspace = true url.workspace = true +[dev-dependencies] +gpui = { workspace = true, features = ["test-support"] } + [build-dependencies] toml.workspace = true diff --git a/crates/ely_app/src/shell/gpui_harness_tests.rs b/crates/ely_app/src/shell/gpui_harness_tests.rs new file mode 100644 index 0000000..279fcfd --- /dev/null +++ b/crates/ely_app/src/shell/gpui_harness_tests.rs @@ -0,0 +1,197 @@ +//! GPUI test harness for the input pipeline. +//! +//! Twelve sidecar-side commits and one shell-side commit had all claimed to +//! fix "click does nothing" while the user kept reporting the same symptom. +//! The roundtable consensus: every store-layer test passed GREEN, every +//! sidecar integration test passed GREEN, but nothing in the repo exercised +//! the real GPUI event tree (`render_input_overlay` + window-level mouse +//! handlers + sidebar capture interactions). This module is that missing +//! holdout set. +//! +//! What we have so far: +//! 1. `baseline_overlay_div_receives_simulated_click` — proves GPUI's +//! `.occlude()` + `capture_any_mouse_up` primitive works correctly in +//! `TestAppContext`. If this regresses, the harness itself is broken. +//! 2. `baseline_overlay_with_full_listener_combo_receives_click` — proves +//! the exact listener combo `render_input_overlay` uses (on_mouse_down + +//! capture_any_mouse_up + on_mouse_move + on_scroll_wheel on a single +//! `.occlude()` div) works in isolation. +//! 3. `ely_shell_external_canvas_lays_out_inside_window` — boots a real +//! `ElyShell`, navigates to an external URL, and asserts the +//! input_overlay's measured viewport bounds fit inside the visible +//! window. This was the original ship-blocker: the overlay was being +//! positioned at `y = window_height - 17`, entirely below the visible +//! region, so every user click hit empty space above the overlay. + +use std::cell::RefCell; +use std::rc::Rc; + +use ely_domain::{TabId, UrlText}; +use gpui::{ + Bounds, Context, IntoElement, Modifiers, MouseButton, ParentElement, Pixels, Render, + Styled, TestAppContext, Window, div, point, px, +}; +use gpui::InteractiveElement; + +use super::ShellState; +use super::web_surface::WebSurfaceStore; + +#[cfg(test)] +impl super::ElyShell { + pub(super) fn web_surfaces_for_test(&self) -> &WebSurfaceStore { + &self.web_surfaces + } +} + +#[gpui::test] +async fn ely_shell_external_canvas_lays_out_inside_window(cx: &mut TestAppContext) { + cx.update(|cx| gpui_component::init(cx)); + + let (shell, cx) = cx.add_window_view(|window, cx| super::ElyShell::new(window, cx)); + cx.run_until_parked(); + + cx.update(|window, app_cx| { + shell.update(app_cx, |shell, ctx| { + shell.navigate_active_tab( + UrlText::parse("https://example.com/".to_string()).expect("valid URL"), + window, + ctx, + ); + }); + }); + cx.run_until_parked(); + + let (_active_tab_id, active_tab_url, viewport_bounds) = active_tab_overlay_state(&shell, cx); + assert!( + active_tab_url.starts_with("https://"), + "active tab URL must be external https for render_external_web_canvas \ + to render the input_overlay (got {active_tab_url:?})." + ); + let bounds = viewport_bounds.unwrap_or_else(|| { + panic!( + "viewport_bounds for the active tab is None. The canvas tracker \ + in render_input_overlay's sibling never fired its layout \ + callback — render_external_web_canvas was not reached." + ) + }); + let window_size = cx.update(|window, _| window.bounds().size); + assert!( + bounds.origin.y + bounds.size.height <= window_size.height + px(1.0) + && bounds.origin.x + bounds.size.width <= window_size.width + px(1.0), + "Layout regression: input_overlay viewport_bounds {bounds:?} extend \ + outside the {window_size:?} window. The canvas tracker measured a \ + layout that escapes the visible region — every user click in the \ + visible area now lands above (or beside) the overlay's hitbox. \ + The original ship-blocker was bounds.origin.y == window_height-17 \ + caused by content (the rendered web image) being a non-absolute \ + child of the relative wrapper, which doubled the parent's height \ + and pushed the overlay off-screen." + ); +} + +#[gpui::test] +async fn baseline_overlay_with_full_listener_combo_receives_click( + cx: &mut TestAppContext, +) { + let click_count = Rc::new(RefCell::new(0u32)); + let counter_for_render = click_count.clone(); + + struct ComboProbe { + on_up_counter: Rc>, + } + impl Render for ComboProbe { + fn render(&mut self, _window: &mut Window, _cx: &mut Context) -> impl IntoElement { + let counter = self.on_up_counter.clone(); + div().relative().size_full().child( + div() + .absolute() + .size_full() + .occlude() + .on_mouse_down(MouseButton::Left, |_event, _window, _cx| {}) + .capture_any_mouse_up(move |_event, _window, _cx| { + *counter.borrow_mut() += 1; + }) + .on_mouse_move(|_event, _window, _cx| {}) + .on_scroll_wheel(|_event, _window, _cx| {}), + ) + } + } + + let (_probe, cx) = cx.add_window_view(|_window, _cx| ComboProbe { + on_up_counter: counter_for_render, + }); + cx.run_until_parked(); + + cx.simulate_mouse_move(point(px(100.0), px(100.0)), None, Modifiers::default()); + cx.simulate_click(point(px(100.0), px(100.0)), Modifiers::default()); + cx.run_until_parked(); + + assert_eq!( + *click_count.borrow(), + 1, + "GPUI baseline with input_overlay's full listener combo: click should \ + reach capture_any_mouse_up. If this fails, the listener combo itself \ + is the problem, not the surrounding shell." + ); +} + +#[gpui::test] +async fn baseline_overlay_div_receives_simulated_click(cx: &mut TestAppContext) { + let click_count = Rc::new(RefCell::new(0u32)); + let counter_for_render = click_count.clone(); + + struct Probe { + on_up_counter: Rc>, + } + impl Render for Probe { + fn render(&mut self, _window: &mut Window, _cx: &mut Context) -> impl IntoElement { + let counter = self.on_up_counter.clone(); + div().relative().size_full().child( + div().absolute().size_full().occlude().capture_any_mouse_up( + move |_event, _window, _cx| { + *counter.borrow_mut() += 1; + }, + ), + ) + } + } + + let (_probe, cx) = cx.add_window_view(|_window, _cx| Probe { + on_up_counter: counter_for_render, + }); + cx.run_until_parked(); + + cx.simulate_mouse_move(point(px(100.0), px(100.0)), None, Modifiers::default()); + cx.simulate_click(point(px(100.0), px(100.0)), Modifiers::default()); + cx.run_until_parked(); + + assert_eq!( + *click_count.borrow(), + 1, + "GPUI baseline: a div with .absolute().size_full().occlude() and \ + capture_any_mouse_up never received the simulated click. The test \ + harness or GPUI primitive is broken — ElyShell test results are \ + meaningless until this passes." + ); +} + +fn active_tab_overlay_state( + shell: &gpui::Entity, + cx: &mut gpui::VisualTestContext, +) -> (TabId, String, Option>) { + shell.read_with(cx, |shell, _cx| { + let tab = match &shell.state { + ShellState::Ready(core) => core.active_tab().expect("active tab exists"), + ShellState::StartupError(message) => { + panic!("ElyShell failed to start in test: {message}") + } + }; + let tab_id = tab.id().clone(); + let url = tab.url().as_str().to_string(); + let bounds = shell + .web_surfaces_for_test() + .surface_for_test(&tab_id) + .and_then(|surface| surface.viewport_bounds); + (tab_id, url, bounds) + }) +} diff --git a/crates/ely_app/src/shell/mod.rs b/crates/ely_app/src/shell/mod.rs index 58c960c..363a435 100644 --- a/crates/ely_app/src/shell/mod.rs +++ b/crates/ely_app/src/shell/mod.rs @@ -33,6 +33,9 @@ mod web_surface_runtime; mod web_surface_state; mod web_surface_view; +#[cfg(test)] +mod gpui_harness_tests; + use std::time::Duration; use ely_browser_core::{BrowserCore, InitialBrowserConfig}; diff --git a/crates/ely_app/src/shell/web_surface.rs b/crates/ely_app/src/shell/web_surface.rs index 304d2f2..9dd3211 100644 --- a/crates/ely_app/src/shell/web_surface.rs +++ b/crates/ely_app/src/shell/web_surface.rs @@ -315,6 +315,11 @@ impl WebSurfaceStore { fn surface_mut(&mut self, tab_id: &TabId) -> &mut PerTabSurface { self.surfaces.entry(tab_id.clone()).or_insert_with(PerTabSurface::new) } + + #[cfg(test)] + pub(super) fn surface_for_test(&self, tab_id: &TabId) -> Option<&PerTabSurface> { + self.surfaces.get(tab_id) + } } pub(super) fn is_external_web_url(url: &str) -> bool { diff --git a/crates/ely_app/src/shell/web_surface_tests.rs b/crates/ely_app/src/shell/web_surface_tests.rs index 9457071..9f13c86 100644 --- a/crates/ely_app/src/shell/web_surface_tests.rs +++ b/crates/ely_app/src/shell/web_surface_tests.rs @@ -213,6 +213,120 @@ fn zero_wheel_delta_reports_zero_delta() -> Result<(), Box> { Ok(()) } +/// Pinning the per-tab isolation invariant. A click recorded against +/// tab A must not be drained by, dropped by, or overwritten by any +/// state mutation routed to tab B. The store keys every click on its +/// owning `TabId` via `PerTabSurface`, but the singleton +/// `keyboard_focus` cross-cuts tabs — so a regression that +/// accidentally entangled them (e.g. dropping A's `click_point` when +/// B took focus) would surface here. +#[test] +fn click_on_tab_a_survives_click_on_tab_b() -> Result<(), Box> { + let mut store = WebSurfaceStore::new(); + let tab_a = web_tab("https://example.com/a")?; + let tab_b = web_tab("https://example.com/b")?; + + assert_applied(store.record_viewport_size(tab_a.id(), web_bounds(), 1.0)); + assert_applied(store.record_viewport_size(tab_b.id(), web_bounds(), 1.0)); + + assert_applied(store.record_click_point( + tab_a.id(), + tab_a.url().as_str(), + point(px(40.0), px(40.0)), + 1.0, + )); + assert_applied(store.record_click_point( + tab_b.id(), + tab_b.url().as_str(), + point(px(200.0), px(200.0)), + 1.0, + )); + + let input_a = store.take_pending_input(tab_a.id(), tab_a.url().as_str()); + assert_eq!( + input_a.click_point.map(|p| (p.x(), p.y())), + Some((40, 40)), + "tab A's click must survive a subsequent click on tab B — \ + per-tab surfaces are independent owners of `click_point`", + ); + + let input_b = store.take_pending_input(tab_b.id(), tab_b.url().as_str()); + assert_eq!( + input_b.click_point.map(|p| (p.x(), p.y())), + Some((200, 200)), + "tab B's click must drain into B's pending input, not A's", + ); + Ok(()) +} + +/// Pinning the zero-delta short-circuit's non-effect on a buffered +/// click. `record_scroll_delta` wipes `click_point` (post-scroll +/// coords would target the wrong DOM node), but the early `None` +/// return for `DroppedZeroDelta` must short-circuit *before* the wipe. +/// A future refactor that moved the wipe above the delta check would +/// silently eat clicks whenever a precision-mouse wheel reported a +/// sub-device-pixel delta. +#[test] +fn zero_wheel_delta_must_not_erase_buffered_click() -> Result<(), Box> { + let mut store = WebSurfaceStore::new(); + let tab = web_tab("https://example.com/form")?; + let url = tab.url().as_str(); + + 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_eq!( + store.record_scroll_delta(tab.id(), url, point(px(0.0), px(0.0)), 1.0), + WebSurfaceInputOutcome::DroppedZeroDelta, + ); + + let input = store.take_pending_input(tab.id(), url); + assert_eq!( + input.click_point.map(|p| (p.x(), p.y())), + Some((160, 120)), + "a zero-delta wheel event reports DroppedZeroDelta and must not \ + take the `click_point` wipe path — the early return guards it", + ); + Ok(()) +} + +/// Pinning the bounds-vs-drain decoupling. A viewport resize between +/// click and drain leaves `viewport_bounds` mutated but does not +/// touch `click_point`, `scroll_offset`, or `requested_url` — the +/// three keys the drain filter checks. The click's stored device-px +/// coordinates remain valid against the new bounds because GPUI +/// re-renders before any new click can arrive. +/// +/// If a future refactor stored raw window-relative coords on +/// `click_point` and converted them at drain time, a resize between +/// record and drain would shift the result; this test would still +/// drain "something" but the coordinates would change, surfacing the +/// drift. +#[test] +fn click_survives_viewport_bounds_change_before_drain() -> Result<(), Box> { + let mut store = WebSurfaceStore::new(); + let tab = web_tab("https://example.com/resize")?; + let url = tab.url().as_str(); + + 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)); + + // Resize: first measurement is buffered (requires confirmation). + assert_eq!( + store.record_viewport_size(tab.id(), resized_once_bounds(), 1.0), + WebSurfaceInputOutcome::Buffered, + ); + + let input = store.take_pending_input(tab.id(), url); + assert_eq!( + input.click_point.map(|p| (p.x(), p.y())), + Some((160, 120)), + "a resize-in-progress must not steal the buffered click — \ + the drain filter checks url+scroll_offset, not bounds", + ); + Ok(()) +} + fn web_bounds() -> Bounds { Bounds::new(point(px(0.0), px(0.0)), size(px(640.0), px(480.0))) } diff --git a/crates/ely_app/src/shell/web_surface_view.rs b/crates/ely_app/src/shell/web_surface_view.rs index 11f20ab..7380923 100644 --- a/crates/ely_app/src/shell/web_surface_view.rs +++ b/crates/ely_app/src/shell/web_surface_view.rs @@ -69,19 +69,18 @@ fn render_web_surface( let tracker_entity = state_entity; div() - .flex_1() - .h_full() + .relative() + .size_full() .min_w_0() .overflow_hidden() .child( div() - .relative() - .size_full() - .overflow_hidden() - .child(content) - .child(render_viewport_tracker(tab.id().clone(), tracker_entity)) - .child(render_input_overlay(input_tab_id, input_url, input_entity)), + .absolute() + .inset_0() + .child(content), ) + .child(render_viewport_tracker(tab.id().clone(), tracker_entity)) + .child(render_input_overlay(input_tab_id, input_url, input_entity)) .into_any_element() }