From d575a199b362515f8ddba9b8659c101720dcd870 Mon Sep 17 00:00:00 2001 From: Tryanks Date: Tue, 6 Oct 2026 15:06:29 +0800 Subject: [PATCH 1/2] fix(ui): end the text selection before a click stops propagation gpui-base begins a selection gesture on any left MouseDown and ends it from a bubble-phase MouseUp listener on the root. A click handler that only called cx.stop_propagation() swallowed that MouseUp, so the selection kept following the pointer across Markdown until the next release, and the next press could not clear it. One owner, widgets::stop_click_propagation, lets the selection see the release first. Every on_click that stops propagation goes through it, including the Markdown link handlers that already did this by hand. Regression test: a swallowing click above selectable Markdown, then a pointer move over the text, must leave no selection. --- crates/ui/src/composer/components/images.rs | 4 +- crates/ui/src/composer/components/pickers.rs | 8 +-- .../ui/src/composer/components/user_input.rs | 12 ++-- crates/ui/src/markdown/image_link.rs | 3 +- crates/ui/src/markdown/inline.rs | 3 +- crates/ui/src/markdown/inline_flow.rs | 3 +- crates/ui/src/markdown/render.rs | 3 +- crates/ui/src/markdown/view.rs | 63 ++++++++++++++++++- crates/ui/src/orchestrate_settings.rs | 2 +- crates/ui/src/overlay/notification.rs | 2 +- crates/ui/src/settings_page.rs | 2 +- crates/ui/src/sidebar.rs | 10 +-- crates/ui/src/widgets/mod.rs | 10 +++ crates/ui/src/widgets/popover.rs | 2 +- 14 files changed, 97 insertions(+), 30 deletions(-) diff --git a/crates/ui/src/composer/components/images.rs b/crates/ui/src/composer/components/images.rs index 41902ea0d..08e2404df 100644 --- a/crates/ui/src/composer/components/images.rs +++ b/crates/ui/src/composer/components/images.rs @@ -508,8 +508,8 @@ impl Composer { .xsmall() .text_color(cx.theme().muted_foreground), ) - .on_click(cx.listener(move |this, _, _, cx| { - cx.stop_propagation(); + .on_click(cx.listener(move |this, _, window, cx| { + crate::widgets::stop_click_propagation(window, cx); this.remove_image(index, cx); })), ) diff --git a/crates/ui/src/composer/components/pickers.rs b/crates/ui/src/composer/components/pickers.rs index cf669d173..c648f0589 100644 --- a/crates/ui/src/composer/components/pickers.rs +++ b/crates/ui/src/composer/components/pickers.rs @@ -1196,8 +1196,8 @@ fn render_model_row( muted }), ) - .on_click(move |_, _, cx| { - cx.stop_propagation(); + .on_click(move |_, window, cx| { + crate::widgets::stop_click_propagation(window, cx); let fav_id = fav_id.clone(); store_fav.update(cx, |store, _cx| store.toggle_favorite_model(fav_id)); // Refresh the open popover so the star + ordering update. @@ -1681,8 +1681,8 @@ fn render_fast_mode_bolt( compact, ) .debug_selector(|| "traits-fast-mode".into()) - .on_click(move |_, _, cx| { - cx.stop_propagation(); + .on_click(move |_, window, cx| { + crate::widgets::stop_click_propagation(window, cx); let value = fast.value(on != Some(true)); store.update(cx, |store, _cx| { store.set_active_option(fast.option_id.clone(), Some(value)); diff --git a/crates/ui/src/composer/components/user_input.rs b/crates/ui/src/composer/components/user_input.rs index bb95aa744..f73741ccc 100644 --- a/crates/ui/src/composer/components/user_input.rs +++ b/crates/ui/src/composer/components/user_input.rs @@ -387,8 +387,8 @@ impl Composer { } else { crate::tr!("userinput.answer") }) - .on_click(cx.listener(|this, _, _, cx| { - cx.stop_propagation(); + .on_click(cx.listener(|this, _, window, cx| { + crate::widgets::stop_click_propagation(window, cx); this.ui_async_expanded = !this.ui_async_expanded; cx.notify(); })); @@ -402,8 +402,8 @@ impl Composer { }) .icon(IconName::Close) .tooltip(crate::tr!("userinput.dismiss")) - .on_click(cx.listener(move |this, _, _, cx| { - cx.stop_propagation(); + .on_click(cx.listener(move |this, _, window, cx| { + crate::widgets::stop_click_propagation(window, cx); this.ui_dismissed_request_id = Some(request_dismiss.clone()); cx.notify(); })); @@ -454,7 +454,7 @@ impl Composer { .disabled(index == 0) .tooltip(crate::tr!("userinput.previous")) .on_click(cx.listener(move |this, _, window, cx| { - cx.stop_propagation(); + crate::widgets::stop_click_propagation(window, cx); this.ui_go(-1, &questions_previous, window, cx); })), ) @@ -481,7 +481,7 @@ impl Composer { .disabled(index + 1 >= total) .tooltip(crate::tr!("userinput.next_question")) .on_click(cx.listener(move |this, _, window, cx| { - cx.stop_propagation(); + crate::widgets::stop_click_propagation(window, cx); this.ui_go(1, &questions_next, window, cx); })), ) diff --git a/crates/ui/src/markdown/image_link.rs b/crates/ui/src/markdown/image_link.rs index d71dabaa7..1ac100a71 100644 --- a/crates/ui/src/markdown/image_link.rs +++ b/crates/ui/src/markdown/image_link.rs @@ -73,8 +73,7 @@ pub(super) fn badge( .tooltip(move |window, cx| Tooltip::new(tooltip_url.clone()).build(window, cx)) .child(div().min_w_0().truncate().child(label.clone())) .on_click(move |_, window, cx| { - gpui_base::TextSelection::end(window, cx); - cx.stop_propagation(); + crate::widgets::stop_click_propagation(window, cx); let source = view.read(cx).image_source(&url.to_string().into()); crate::attachments::open_image_lightbox(source, label.to_string(), window, cx); }); diff --git a/crates/ui/src/markdown/inline.rs b/crates/ui/src/markdown/inline.rs index f0d2e44d8..7a8e11df8 100644 --- a/crates/ui/src/markdown/inline.rs +++ b/crates/ui/src/markdown/inline.rs @@ -411,8 +411,7 @@ impl Element for Inline { return; } if let Some(link) = Self::link_for_position(&layout, &links, event.position) { - gpui_base::TextSelection::end(window, cx); - cx.stop_propagation(); + crate::widgets::stop_click_propagation(window, cx); match view.read(cx).resolve_link(&link.url) { LinkTarget::Web(url) => cx.open_url(&url), LinkTarget::Local(path) => cx.open_with_system(&path), diff --git a/crates/ui/src/markdown/inline_flow.rs b/crates/ui/src/markdown/inline_flow.rs index 207f81e60..3ada998ce 100644 --- a/crates/ui/src/markdown/inline_flow.rs +++ b/crates/ui/src/markdown/inline_flow.rs @@ -192,8 +192,7 @@ impl InlineFlow { }); }) .on_click(move |_, window, cx| { - gpui_base::TextSelection::end(window, cx); - cx.stop_propagation(); + crate::widgets::stop_click_propagation(window, cx); if let Some(link) = &link { match view.read(cx).resolve_link(&link.url) { LinkTarget::Web(url) => cx.open_url(&url), diff --git a/crates/ui/src/markdown/render.rs b/crates/ui/src/markdown/render.rs index 2349c1aa3..3e161edde 100644 --- a/crates/ui/src/markdown/render.rs +++ b/crates/ui/src/markdown/render.rs @@ -644,8 +644,7 @@ fn render_paragraph( }); }) .on_click(move |_, window, cx| { - gpui_base::TextSelection::end(window, cx); - cx.stop_propagation(); + crate::widgets::stop_click_propagation(window, cx); if let Some(link) = &link { match view.read(cx).resolve_link(&link.url) { LinkTarget::Web(url) => cx.open_url(&url), diff --git a/crates/ui/src/markdown/view.rs b/crates/ui/src/markdown/view.rs index 3b8f8278e..f51e878f8 100644 --- a/crates/ui/src/markdown/view.rs +++ b/crates/ui/src/markdown/view.rs @@ -349,7 +349,8 @@ mod tests { use gpui::{ AppContext as _, Context, Entity, IntoElement, ListAlignment, ListState, Modifiers, - MouseButton, Render, TestAppContext, VisualTestContext, Window, div, point, px, + MouseButton, Render, StatefulInteractiveElement as _, TestAppContext, VisualTestContext, + Window, div, point, px, }; use super::*; @@ -482,6 +483,32 @@ mod tests { } } + struct SwallowedClickRoot { + markdown: Entity, + } + + impl Render for SwallowedClickRoot { + fn render(&mut self, _: &mut Window, _: &mut Context) -> impl IntoElement { + div() + .size_full() + .child(TextSelectionLayer) + .child( + div() + .id("swallowing-button") + .w_full() + .h(px(52.)) + .on_click(|_, window, cx| { + crate::widgets::stop_click_propagation(window, cx) + }), + ) + .child( + div() + .h(px(40.)) + .child(MarkdownView::new(&self.markdown).selectable(true)), + ) + } + } + impl Render for CrossViewRoot { fn render(&mut self, _: &mut Window, _: &mut Context) -> impl IntoElement { div() @@ -986,6 +1013,40 @@ mod tests { }); } + #[gpui::test] + fn click_that_stops_propagation_does_not_leave_selection_following_pointer( + cx: &mut TestAppContext, + ) { + cx.update(crate::theme::init); + cx.update(crate::markdown::init); + let (_, cx) = cx.add_window_view(|_, cx| SwallowedClickRoot { + markdown: cx.new(|cx| MarkdownState::new("Hello world", cx)), + }); + let cx: &mut VisualTestContext = cx; + cx.update(|window, cx| { + let _ = window.draw(cx); + }); + + cx.simulate_mouse_down( + point(px(10.), px(20.)), + MouseButton::Left, + Modifiers::default(), + ); + cx.simulate_mouse_up( + point(px(10.), px(20.)), + MouseButton::Left, + Modifiers::default(), + ); + cx.simulate_mouse_move(point(px(80.), px(70.)), None, Modifiers::default()); + cx.update(|window, cx| { + let _ = window.draw(cx); + assert!( + gpui_base::TextSelection::selected_text(window, cx).is_empty(), + "the released pointer kept selecting text" + ); + }); + } + #[gpui::test] fn cross_view_drag_copies_in_document_order(cx: &mut TestAppContext) { cx.update(crate::theme::init); diff --git a/crates/ui/src/orchestrate_settings.rs b/crates/ui/src/orchestrate_settings.rs index e528f80f3..de331ea07 100644 --- a/crates/ui/src/orchestrate_settings.rs +++ b/crates/ui/src/orchestrate_settings.rs @@ -352,7 +352,7 @@ impl OrchestrateSettingsPanel { .tooltip(label.clone()) .aria_label(label) .on_click(cx.listener(move |this, _, window, cx| { - cx.stop_propagation(); + crate::widgets::stop_click_propagation(window, cx); on_reset(this, window, cx); })) .into_any_element() diff --git a/crates/ui/src/overlay/notification.rs b/crates/ui/src/overlay/notification.rs index affc1f20c..13c1648e8 100644 --- a/crates/ui/src/overlay/notification.rs +++ b/crates/ui/src/overlay/notification.rs @@ -342,7 +342,7 @@ impl Render for Notification { .xsmall() .icon(IconName::Close) .on_click(cx.listener(|this, _, window, cx| { - cx.stop_propagation(); + crate::widgets::stop_click_propagation(window, cx); this.dismiss(window, cx); })), ), diff --git a/crates/ui/src/settings_page.rs b/crates/ui/src/settings_page.rs index 14fab0940..feae075ef 100644 --- a/crates/ui/src/settings_page.rs +++ b/crates/ui/src/settings_page.rs @@ -2654,7 +2654,7 @@ impl SettingsPage { .on_click(cx.listener(move |this, _, window, cx| { // Toggle rows are clickable as a whole; a click that lands // on this button must not also flip the switch. - cx.stop_propagation(); + crate::widgets::stop_click_propagation(window, cx); on_reset(this, window, cx); })) .into_any_element(), diff --git a/crates/ui/src/sidebar.rs b/crates/ui/src/sidebar.rs index 8fba1e7b0..0ec422641 100644 --- a/crates/ui/src/sidebar.rs +++ b/crates/ui/src/sidebar.rs @@ -2051,8 +2051,8 @@ impl SessionsSidebar { .tooltip(|window, cx| { Tooltip::new(crate::tr!("sidebar.create_thread").into_owned()).build(window, cx) }) - .on_click(cx.listener(move |this, _, _, cx| { - cx.stop_propagation(); + .on_click(cx.listener(move |this, _, window, cx| { + crate::widgets::stop_click_propagation(window, cx); let cwd = plus_cwd.clone(); let project_id = plus_project_id.clone(); this.store.update(cx, |store, cx| { @@ -2670,7 +2670,7 @@ impl SessionsSidebar { Tooltip::new(crate::tr!("sidebar.archive").into_owned()).build(window, cx) }) .on_click(cx.listener(move |this, _, window, cx| { - cx.stop_propagation(); + crate::widgets::stop_click_propagation(window, cx); this.archive_thread(&archive_id, &archive_title, window, cx); })) .child( @@ -3634,8 +3634,8 @@ impl SessionsSidebar { .gap(px(2.)) .text_size(px(12.)) .text_color(cx.theme().muted_foreground) - .on_click(cx.listener(move |this, _, _, cx| { - cx.stop_propagation(); + .on_click(cx.listener(move |this, _, window, cx| { + crate::widgets::stop_click_propagation(window, cx); this.store.update(cx, |store, _| { store.set_thread_collapsed(disclosure_id.clone(), !children_collapsed) }); diff --git a/crates/ui/src/widgets/mod.rs b/crates/ui/src/widgets/mod.rs index 471f816de..a62507f76 100644 --- a/crates/ui/src/widgets/mod.rs +++ b/crates/ui/src/widgets/mod.rs @@ -23,3 +23,13 @@ pub use progress::Progress; pub use spinner::Spinner; pub use switch::Switch; pub use tooltip::Tooltip; + +/// Stops a click's propagation after letting the window text selection see +/// the release. gpui-base ends a selection gesture from a bubble-phase +/// MouseUp listener on the root, and a MouseDown anywhere begins one, so a +/// click handler that only stops propagation leaves the selection following +/// the pointer until the next release. +pub(crate) fn stop_click_propagation(window: &mut gpui::Window, cx: &mut gpui::App) { + gpui_base::TextSelection::end(window, cx); + cx.stop_propagation(); +} diff --git a/crates/ui/src/widgets/popover.rs b/crates/ui/src/widgets/popover.rs index d3b094005..28ddf74d7 100644 --- a/crates/ui/src/widgets/popover.rs +++ b/crates/ui/src/widgets/popover.rs @@ -229,7 +229,7 @@ impl Popover { let mut root = div() .id(self.id) .on_click(move |_, window, cx| { - cx.stop_propagation(); + crate::widgets::stop_click_propagation(window, cx); toggle.update(cx, |state, cx| state.toggle_open(window, cx)); cx.notify(parent); }) From c40a90db67d0b546395e885e711611aeb2d11a3e Mon Sep 17 00:00:00 2001 From: Tryanks Date: Tue, 6 Oct 2026 15:14:33 +0800 Subject: [PATCH 2/2] test(traverse): stop the host's refresh loop before driving a refresh a_self_hosted_machine_starts_without_its_manifest_and_applies_it_when_it_arrives reset the pacing stamp while the host's own refresh loop was about to run its first pass. On a slow runner that pass landed after the reset, fetched the manifest itself, and left the test's refresh with nothing to return. The test now aborts the loop first; the contract it proves, applying a manifest that arrives after startup, drives apply_manifest directly and never needed the loop. --- crates/traverse/src/host.rs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/crates/traverse/src/host.rs b/crates/traverse/src/host.rs index 95a7ff764..d29594dfc 100644 --- a/crates/traverse/src/host.rs +++ b/crates/traverse/src/host.rs @@ -1168,7 +1168,7 @@ mod tests { let manifest_path = dir.join("relays.json"); let (to_host, _host_rx) = async_channel::unbounded::(); let (_host_tx, from_host) = async_channel::unbounded::(); - let host = TraverseHost::start( + let mut host = TraverseHost::start( HostMux::new(to_host, from_host), HostConfig { host_name: "Late".into(), @@ -1193,7 +1193,12 @@ mod tests { ) .unwrap(); let loader = host.loader.clone().unwrap(); - // The failed startup fetch paces the next attempt; the loop would wait it out. + // The failed startup fetch paces the next attempt; the loop would wait + // it out. The loop is stopped before the stamp is reset: its first + // pass runs as soon as the runtime schedules it, and once unpaced it + // would fetch the written manifest itself, leaving this refresh with + // nothing to return. + host.refresh.take().unwrap().abort(); loader.state().last_attempt_ms = None; let manifest = block_on(loader.refresh()).expect("the manifest arrived"); block_on(host.shared.apply_manifest(manifest));