From 84446022e530fbc428ad2fe6c1ad9d00a9b64c44 Mon Sep 17 00:00:00 2001 From: Lander Brandt Date: Mon, 24 Aug 2026 13:19:18 -0700 Subject: [PATCH 1/7] fix(input): tolerate a window whose window_handle() panics gpui's TestWindow panics (unimplemented!) in its HasWindowHandle impl instead of returning Err, so the graceful `.ok()?` bail in the input and accessibility ns_view helpers never fires -- a downstream crate's tests that build a Root or render an Input over a test window panic. Probe the handle under catch_unwind and no-op when there is no backing AppKit window. --- crates/base/src/input/base/native.rs | 11 ++++++++++- crates/base/src/macos_accessibility.rs | 10 ++++++++-- 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/crates/base/src/input/base/native.rs b/crates/base/src/input/base/native.rs index f0cb5a208c..b4cc064957 100644 --- a/crates/base/src/input/base/native.rs +++ b/crates/base/src/input/base/native.rs @@ -44,7 +44,16 @@ mod macos { } fn ns_view(window: &Window) -> Option<&AnyObject> { - let handle = HasWindowHandle::window_handle(window).ok()?; + // A headless/test window's `window_handle()` panics (`unimplemented!`) + // instead of returning Err, so the graceful `.ok()?` never fires. + // Probe under catch_unwind and treat a panic as "no view" (the Input + // then skips native text-content wiring) so downstream crates' tests + // that render an Input over a test window don't panic here. + let handle = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { + HasWindowHandle::window_handle(window).ok() + })) + .ok() + .flatten()?; let RawWindowHandle::AppKit(handle) = handle.as_raw() else { return None; }; diff --git a/crates/base/src/macos_accessibility.rs b/crates/base/src/macos_accessibility.rs index 75e942c54e..e7b66fbb93 100644 --- a/crates/base/src/macos_accessibility.rs +++ b/crates/base/src/macos_accessibility.rs @@ -14,8 +14,14 @@ use objc2_foundation::NSPoint; use raw_window_handle::{HasWindowHandle, RawWindowHandle}; pub fn install_window_hit_test_forwarder(window: &Window) { - let Some(view) = ns_view(window) else { - return; + // A headless/test window's `window_handle()` panics (`unimplemented!`) + // instead of returning `Err`, so `ns_view`'s graceful `.ok()?` never + // gets to fire. Probe under `catch_unwind` and no-op when there is no + // backing AppKit window (gpui's test platform), so downstream crates' + // tests -- which build a `Root` over a test window -- don't panic here. + let view = match std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| ns_view(window))) { + Ok(Some(view)) => view, + _ => return, }; let Some(window) = view.window() else { return; From 5695747717ac89cdae4fc73de210d0f5bb42fea8 Mon Sep 17 00:00:00 2001 From: Lander Brandt Date: Mon, 24 Aug 2026 13:19:18 -0700 Subject: [PATCH 2/7] feat(dock): expose remove_panel_id publicly on DockArea Lets a consumer close a panel it only holds a PanelId for (e.g. an unresolved/InvalidPanel leaf from a restored layout), where remove_panel would need a live Entity

. --- crates/base/src/dock/dock_area.rs | 27 ++++++++++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-) diff --git a/crates/base/src/dock/dock_area.rs b/crates/base/src/dock/dock_area.rs index 0c114d04d8..20d35a5b25 100644 --- a/crates/base/src/dock/dock_area.rs +++ b/crates/base/src/dock/dock_area.rs @@ -681,7 +681,10 @@ impl DockArea { self.commit(result, window, cx); } - fn remove_panel_id(&mut self, panel: PanelId, window: &mut Window, cx: &mut Context) { + /// Remove a panel by id. Unlike [`Self::remove_panel`] this needs no live + /// `Entity`, so it can close a panel held only by `PanelId` (e.g. an + /// unresolved `InvalidPanel` leaf from a restored layout). + pub fn remove_panel_id(&mut self, panel: PanelId, window: &mut Window, cx: &mut Context) { let Some(region) = self.placement_of_panel(panel) else { return; }; @@ -4709,4 +4712,26 @@ mod tests { "the area must not fill itself with a group that never zoomed" ); } + + /// `remove_panel_id` drops the panel named only by its `PanelId`. + #[gpui::test] + fn remove_panel_id_drops_the_panel_it_names(cx: &mut TestAppContext) { + let log = Log::default(); + let (area, alpha, cx) = two_groups(&log, cx); + let alpha_id = panel_id_of(&alpha); + assert!( + cx.read(|cx| area.read(cx).panel(alpha_id).is_some()), + "alpha starts owned by the area" + ); + + cx.update(|window, cx| { + area.update(cx, |area, cx| area.remove_panel_id(alpha_id, window, cx)); + }); + cx.run_until_parked(); + + assert!( + cx.read(|cx| area.read(cx).panel(alpha_id).is_none()), + "remove_panel_id removes the panel identified only by its id" + ); + } } From e1aaa56bb4147719a6a7f6a8879688eea30c6265 Mon Sep 17 00:00:00 2001 From: Lander Brandt Date: Mon, 24 Aug 2026 13:19:18 -0700 Subject: [PATCH 3/7] fix(dock): ignore move_panel of a panel this area does not own A cross-DockArea drag -- dragging a tab between an outer dock and a nested dock hosted inside one of its own panels -- would insert a PanelId with no backing entity into the target tree while the panel stayed registered in its real owner: a ghost tab in one, a duplicate in the other. Guard move_panel to no-op when the panel is not owned here, so the panel stays in its source. Covered by a_move_of_an_unowned_panel_is_ignored. --- crates/base/src/dock/dock_area.rs | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/crates/base/src/dock/dock_area.rs b/crates/base/src/dock/dock_area.rs index 20d35a5b25..ad4e2d3f4e 100644 --- a/crates/base/src/dock/dock_area.rs +++ b/crates/base/src/dock/dock_area.rs @@ -598,6 +598,11 @@ impl DockArea { window: &mut Window, cx: &mut Context, ) { + // A panel this area does not own (e.g. dropped from a nested dock) has + // no backing entity here; inserting it would strand a ghost tab. + if self.panel(panel).is_none() { + return; + } let Some(destination) = self.placement_of_node(target_node(&target)) else { return; }; @@ -2656,6 +2661,21 @@ mod tests { ); } + /// Moving a panel this area does not own is a no-op, not a ghost insert. + #[gpui::test] + fn a_move_of_an_unowned_panel_is_ignored(cx: &mut TestAppContext) { + let log = Log::default(); + let (area, _panels, cx) = one_group(&log, &["Alpha", "Beta"], None, cx); + let group = child_node(&area, 0, cx); + let before = cx.read(|cx| area.read(cx).dump(cx)); + + // A PanelId from nowhere, as if dropped from another DockArea. + move_panel_into(&area, PanelId::from_u64(9_999_999), group, None, true, cx); + + let after = cx.read(|cx| area.read(cx).dump(cx)); + assert_eq!(before, after, "an unowned panel move must not touch the tree"); + } + /// The other drop geometry: a placement whose axis differs from the /// parent's wraps the target in a fresh split, so the sizes are decided /// by a `ResizableState` that has never been measured. From 3f9a790d7ca703ae86f964fd9e77e19b86aacd75 Mon Sep 17 00:00:00 2001 From: Lander Brandt Date: Mon, 24 Aug 2026 13:19:18 -0700 Subject: [PATCH 4/7] feat(dock): expose per-node bounds for spatial overlays Record each tab-group leaf's on-screen rect during render (via on_prepaint) into a node_bounds map, exposed as DockArea::node_bounds. Lets a host paint spatial overlays over panes -- e.g. a vimium-style pane picker badging each pane -- which the pure-data tree cannot express. The wrapper that captures the rect carries no sizing (that stays on the parent resizable_panel), so split layout is unchanged. --- crates/base/src/dock/dock_area.rs | 32 ++++++++++++++- crates/component/src/dock/dock.rs | 68 ++++++++++++++++++++++++++++++- 2 files changed, 98 insertions(+), 2 deletions(-) diff --git a/crates/base/src/dock/dock_area.rs b/crates/base/src/dock/dock_area.rs index ad4e2d3f4e..89cadef4bd 100644 --- a/crates/base/src/dock/dock_area.rs +++ b/crates/base/src/dock/dock_area.rs @@ -137,6 +137,13 @@ pub struct DockArea { tiles: HashMap>, panels: HashMap>, + /// Each tab-group leaf's on-screen rectangle, recorded during render + /// (via `on_prepaint`). Lets a host paint spatial overlays -- e.g. a + /// vimium-style pane picker badging each pane -- which the pure-data + /// tree cannot express. Stale entries for removed nodes are harmless: + /// node ids are unique, so a gone node is simply never queried. + node_bounds: HashMap>, + locked: bool, zoomed: Option, focus_handle: FocusHandle, @@ -170,6 +177,7 @@ impl DockArea { splits: HashMap::new(), tiles: HashMap::new(), panels: HashMap::new(), + node_bounds: HashMap::new(), locked: false, zoomed: None, focus_handle: cx.focus_handle(), @@ -209,6 +217,12 @@ impl DockArea { self.bounds } + /// Last-rendered rect of tab-group leaf `node`, or `None` if it is not a + /// rendered tab group. + pub fn node_bounds(&self, node: NodeId) -> Option> { + self.node_bounds.get(&node).copied() + } + /// The tree for one region, or `None` for a dock that does not exist. /// /// The `Option` is in the signature rather than hidden behind a panic @@ -1507,7 +1521,23 @@ impl DockArea { .into_any_element() } PaneRef::Tabs { .. } => match self.groups.get(&node.id()) { - Some(cached) => cached.entity.clone().into_any_element(), + Some(cached) => { + // Wrapper records the group's rect for spatial overlays; it + // only holds bounds, so sizing stays on `resizable_panel`. + let node_id = node.id(); + let area = self.this.clone(); + // The probe must precede the content child so it measures + // the wrapper's origin, not a point below it. + div() + .size_full() + .on_prepaint(move |bounds, _, cx| { + _ = area.update(cx, |area, _| { + area.node_bounds.insert(node_id, bounds); + }); + }) + .child(cached.entity.clone()) + .into_any_element() + } None => Empty.into_any_element(), }, PaneRef::Tiles { .. } => match self.tiles.get(&node.id()) { diff --git a/crates/component/src/dock/dock.rs b/crates/component/src/dock/dock.rs index 37f4b38722..de7c6c8c80 100644 --- a/crates/component/src/dock/dock.rs +++ b/crates/component/src/dock/dock.rs @@ -252,7 +252,7 @@ mod tests { use gpui_base::dock::DockAreaRenderer; use crate::dock::{ - DockArea, DockLayout, DockPlacement, DockSkin, + DockArea, DockLayout, DockPlacement, DockSkin, PaneRef, test_support::{MeasuredProbe, SizedProbe}, }; @@ -455,4 +455,70 @@ mod tests { ); assert_eq!(left, Some(px(240.)), "the left dock follows the pointer"); } + + /// `node_bounds` records each center leaf's rect for host-painted overlays. + /// Regression guard for the `on_prepaint` probe order: the probe must + /// precede the pane content, or `origin.y` picks up the content's height and + /// any overlay lands off-screen. Asserts the two leaves are top-anchored and + /// tiled. + #[gpui::test] + fn node_bounds_capture_leaf_rects_at_the_docks_top(cx: &mut TestAppContext) { + cx.update(|cx| crate::init(cx)); + let (area, cx) = cx.add_window_view(|window, cx| { + DockArea::new("test", None, window, cx).with_renderer(DockSkin::new(cx)) + }); + cx.simulate_resize(size(px(800.), px(600.))); + cx.update(|window, cx| { + area.update(cx, |area, cx| { + area.set_center( + DockLayout::h_split() + .child(DockLayout::tabs().panel(MeasuredProbe::new(Rc::default(), cx)), None) + .child(DockLayout::tabs().panel(MeasuredProbe::new(Rc::default(), cx)), None), + window, + cx, + ); + }); + }); + cx.run_until_parked(); + // Force a real paint so the leaves' `on_prepaint` probes fire. + cx.update(|window, cx| window.draw(cx).clear(cx)); + + let (left_id, right_id) = cx.update(|_, cx| { + let area = area.read(cx); + let tree = area.layout(DockPlacement::Center).expect("a center tree"); + let PaneRef::Split { children, .. } = tree.root().kind() else { + panic!("the center root is a horizontal split"); + }; + (children[0].id(), children[1].id()) + }); + + let (left, right) = cx.update(|_, cx| { + let area = area.read(cx); + ( + area.node_bounds(left_id).expect("left leaf rect captured during paint"), + area.node_bounds(right_id).expect("right leaf rect captured during paint"), + ) + }); + + // Two leaves side by side: a shared top edge and height, tiled along x. + assert_eq!(left.origin.y, right.origin.y, "the two leaves share a top edge"); + assert_eq!(left.size.height, right.size.height, "the two leaves are the same height"); + assert!(left.size.height > px(0.), "the leaf has a real height"); + assert!(left.origin.x < right.origin.x, "the left leaf sits left of the right"); + + // Top-anchored: the probe recorded the wrapper's origin, not a static + // position below the content. The regression set `origin.y` to the pane + // height, pushing the rect (and any overlay) off the bottom of the view. + assert!( + left.origin.y < px(1.), + "the leaf rect starts at the dock's top; got origin.y {:?}", + left.origin.y, + ); + assert!( + left.origin.y + left.size.height <= px(601.), + "the leaf rect fits inside the 600px window; got origin.y {:?} + height {:?}", + left.origin.y, + left.size.height, + ); + } } From 4edffc0748e813ddcb8ec2a23af1731b69c6f23c Mon Sep 17 00:00:00 2001 From: Lander Brandt Date: Mon, 24 Aug 2026 13:19:18 -0700 Subject: [PATCH 5/7] feat(dock): customizable tabs via render_tab, with a built-in close button Panels can override Panel::render_tab to have the final say over their own tab -- restyle it, swap the label, add a prefix icon, or add/drop a suffix -- while the tab bar keeps its layout, drag/drop, and activation. The tab group builds the fully wired Tab and routes it through the panel; the default returns it unchanged. Closable panels get a built-in close (X) button suffix. It stops click propagation so closing never also selects the tab, and closes by panel id so it targets its own tab regardless of which is active. --- crates/component/src/dock/panel.rs | 18 ++- crates/component/src/dock/tab_panel.rs | 154 ++++++++++++++++++++++++- 2 files changed, 168 insertions(+), 4 deletions(-) diff --git a/crates/component/src/dock/panel.rs b/crates/component/src/dock/panel.rs index dbd562ba33..34eaa28998 100644 --- a/crates/component/src/dock/panel.rs +++ b/crates/component/src/dock/panel.rs @@ -25,7 +25,7 @@ use gpui::{ use gpui_base::dock::{PanelId, PanelState, TabGroup}; use rust_i18n::t; -use crate::{button::Button, menu::PopupMenu}; +use crate::{button::Button, menu::PopupMenu, tab::Tab}; #[derive(Clone, Copy, Debug, Default, PartialEq, Eq)] pub enum PanelStyle { @@ -137,6 +137,17 @@ pub trait Panel: gpui_base::dock::Panel { fn inner_padding(&self, cx: &App) -> bool { true } + + /// Customize this panel's own tab in the tab bar. + /// + /// The tab group builds a fully wired [`Tab`] -- its index, label, + /// selected state, click-to-activate, and drag/drop are already set -- + /// and hands it here for the panel to do final tweaks to e.g. styling. + /// Returns the (possibly customized) tab, so the tab bar's own layout + /// and behavior are preserved. The default returns it untouched. + fn render_tab(&mut self, tab: Tab, window: &mut Window, cx: &mut Context) -> Tab { + tab + } } /// Object-safe counterpart of [`Panel`], and the presentation half of the @@ -150,6 +161,7 @@ pub trait PanelView: gpui_base::dock::PanelView { fn dropdown_menu(&self, menu: PopupMenu, window: &mut Window, cx: &mut App) -> PopupMenu; fn zoom_control(&self, cx: &App) -> Option; fn inner_padding(&self, cx: &App) -> bool; + fn render_tab(&self, tab: Tab, window: &mut Window, cx: &mut App) -> Tab; } impl PanelView for Entity { @@ -187,6 +199,10 @@ impl PanelView for Entity { fn inner_padding(&self, cx: &App) -> bool { self.read(cx).inner_padding(cx) } + + fn render_tab(&self, tab: Tab, window: &mut Window, cx: &mut App) -> Tab { + self.update(cx, |this, cx| this.render_tab(tab, window, cx)) + } } /// The panel handle `gpui-base` holds on this crate's behalf. diff --git a/crates/component/src/dock/tab_panel.rs b/crates/component/src/dock/tab_panel.rs index 14c71fc098..d36a171ee9 100644 --- a/crates/component/src/dock/tab_panel.rs +++ b/crates/component/src/dock/tab_panel.rs @@ -40,6 +40,9 @@ use crate::{ /// a really-drawn frame whether the control was offered. const ZOOM_CONTROL_SELECTOR: &str = "dock-tab-bar-zoom-control"; +/// Debug-bounds selector for a tab's close (X) button, for tests. +const CLOSE_BUTTON_SELECTOR: &str = "dock-tab-close-button"; + /// The size the styled drag preview occupies, reported to base so a drop /// placeholder knows where to fly in from. const DRAG_PREVIEW_SIZE: gpui::Size = gpui::size(px(96.), px(30.)); @@ -492,13 +495,36 @@ impl TabGroupSkin { let handle = PanelHandle::of(panel); let drag = tab_drag(group, ix, cx); - Tab::new() + let tab = Tab::new() .ix(ix) .tab_bar_prefix(has_leading) .map(|this| match handle.and_then(|handle| handle.tab_name(cx)) { Some(tab_name) => this.child(tab_name), None => this.child(panel_title(panel, window, cx)), }) + // A per-tab close (X) button, shown only for a + // panel that reports itself closable. Stops + // propagation so the click never also selects the + // tab, and closes by id so it targets this tab + // regardless of which one is active. + .when(panel.closable(cx), |this| { + this.suffix( + Button::new(("close-tab", ix)) + .icon(IconName::Close) + .xsmall() + .ghost() + .tab_stop(false) + .debug_selector(|| CLOSE_BUTTON_SELECTOR.to_string()) + .on_click({ + let group = group.clone(); + let panel_id = panel.panel_id(cx); + move |_, window, cx| { + cx.stop_propagation(); + group.close(panel_id, window, cx); + } + }), + ) + }) // A collapsed group shows no tab as active: the // strip is a way back in, not a selection. The // comparison is against the panel on screen, not @@ -568,7 +594,14 @@ impl TabGroupSkin { } }) }) - }) + }); + + // Let the panel tweak its finished tab; no handle keeps + // the default. + match handle { + Some(handle) => handle.render_tab(tab, window, cx), + None => tab, + } }) .collect::>(), ) @@ -777,7 +810,7 @@ impl TabGroupRenderer for TabGroupSkin { #[cfg(test)] mod tests { - use std::cell::RefCell; + use std::cell::{Cell, RefCell}; use gpui::{ Entity, EventEmitter, FocusHandle, Focusable, Pixels, TestAppContext, VisualTestContext, @@ -1460,4 +1493,119 @@ mod tests { "a collapsed group installs no action handler" ); } + + /// A panel that reports a chosen `closable` and records whether the skin + /// routed its tab through [`Panel::render_tab`]. + struct TabProbe { + focus_handle: FocusHandle, + closable: bool, + rendered_tab: Rc>, + } + + impl TabProbe { + fn new(closable: bool, rendered_tab: Rc>, cx: &mut App) -> Entity { + cx.new(|cx| Self { + focus_handle: cx.focus_handle(), + closable, + rendered_tab, + }) + } + } + + impl gpui_base::dock::Panel for TabProbe { + fn panel_name(&self) -> &'static str { + "TabProbe" + } + + fn closable(&self, _: &App) -> bool { + self.closable + } + } + + impl Panel for TabProbe { + fn render_tab(&mut self, tab: Tab, _: &mut Window, _: &mut Context) -> Tab { + self.rendered_tab.set(true); + tab + } + } + + impl EventEmitter for TabProbe {} + + impl Focusable for TabProbe { + fn focus_handle(&self, _: &App) -> FocusHandle { + self.focus_handle.clone() + } + } + + impl Render for TabProbe { + fn render(&mut self, _: &mut Window, _: &mut Context) -> impl IntoElement { + Empty + } + } + + /// Render a two-tab group through the real [`DockSkin`] and report whether + /// the first tab drew a close button. The second tab is a non-closable + /// filler (a lone panel draws no tab bar) that never draws one, so the + /// probe is unambiguous. + fn drew_close_button(cx: &mut TestAppContext, closable: bool) -> bool { + cx.update(|cx| crate::init(cx)); + let (area, cx) = cx.add_window_view(|window, cx| { + DockArea::new("skin", None, window, cx).with_renderer(DockSkin::new(cx)) + }); + cx.update(|window, cx| { + let under_test = TabProbe::new(closable, Rc::new(Cell::new(false)), cx); + let filler = TabProbe::new(false, Rc::new(Cell::new(false)), cx); + let layout = DockLayout::tabs() + .panel_view(panel_handle(under_test), cx) + .panel_view(panel_handle(filler), cx); + area.update(cx, |area, cx| area.set_center(layout, window, cx)); + }); + cx.run_until_parked(); + cx.update(|window, cx| window.draw(cx).clear(cx)); + cx.debug_bounds(CLOSE_BUTTON_SELECTOR).is_some() + } + + /// A closable panel's tab carries a close (X) button. + #[gpui::test] + fn a_closable_panel_gets_a_close_button(cx: &mut TestAppContext) { + assert!( + drew_close_button(cx, true), + "a closable panel's tab must draw a close button" + ); + } + + /// A non-closable panel draws no close button (the gate is real). + #[gpui::test] + fn a_non_closable_panel_gets_no_close_button(cx: &mut TestAppContext) { + assert!( + !drew_close_button(cx, false), + "a panel that reports itself non-closable must draw no close button" + ); + } + + /// The skin routes each tab through [`Panel::render_tab`]. + #[gpui::test] + fn render_tab_routes_each_tab_through_its_panel(cx: &mut TestAppContext) { + cx.update(|cx| crate::init(cx)); + let rendered = Rc::new(Cell::new(false)); + let (area, cx) = cx.add_window_view(|window, cx| { + DockArea::new("skin", None, window, cx).with_renderer(DockSkin::new(cx)) + }); + let flag = rendered.clone(); + cx.update(|window, cx| { + let probe = TabProbe::new(true, flag, cx); + let filler = TabProbe::new(false, Rc::new(Cell::new(false)), cx); + let layout = DockLayout::tabs() + .panel_view(panel_handle(probe), cx) + .panel_view(panel_handle(filler), cx); + area.update(cx, |area, cx| area.set_center(layout, window, cx)); + }); + cx.run_until_parked(); + cx.update(|window, cx| window.draw(cx).clear(cx)); + + assert!( + rendered.get(), + "the skin must route the panel's tab through Panel::render_tab" + ); + } } From dfb736d6eaff191885bff6fe28b298ff8dd78e65 Mon Sep 17 00:00:00 2001 From: Lander Brandt Date: Wed, 26 Aug 2026 10:37:53 -0700 Subject: [PATCH 6/7] fix(dock): address review feedback on node bounds, close button, and a11y scope - Prune node_bounds on reconcile so a removed leaf's NodeId reports None instead of a stale rect, matching the documented contract - Gate the per-tab close button on group.is_draggable(), the same check TabGroup::close_panel runs, so it never draws when a click could not close the panel (locked layout or a group's last visible panel) - Narrow the macOS accessibility catch_unwind to window_handle(), as the native Input does, so failures in handle matching or pointer conversion still surface - Add tests covering node removal and a non-active tab close-button click --- crates/base/src/dock/dock_area.rs | 9 ++- crates/base/src/macos_accessibility.rs | 19 +++--- crates/component/src/dock/dock.rs | 51 ++++++++++++++++ crates/component/src/dock/tab_panel.rs | 85 +++++++++++++++++++++++--- 4 files changed, 141 insertions(+), 23 deletions(-) diff --git a/crates/base/src/dock/dock_area.rs b/crates/base/src/dock/dock_area.rs index 89cadef4bd..ac37a0e514 100644 --- a/crates/base/src/dock/dock_area.rs +++ b/crates/base/src/dock/dock_area.rs @@ -137,11 +137,8 @@ pub struct DockArea { tiles: HashMap>, panels: HashMap>, - /// Each tab-group leaf's on-screen rectangle, recorded during render - /// (via `on_prepaint`). Lets a host paint spatial overlays -- e.g. a - /// vimium-style pane picker badging each pane -- which the pure-data - /// tree cannot express. Stale entries for removed nodes are harmless: - /// node ids are unique, so a gone node is simply never queried. + /// Tab-group leaf rects, recorded in `on_prepaint`, for host-painted + /// spatial overlays. Pruned to live nodes on `reconcile`. node_bounds: HashMap>, locked: bool, @@ -1111,6 +1108,8 @@ impl DockArea { self.groups.retain(|node, _| live_nodes.contains(node)); self.splits.retain(|node, _| live_nodes.contains(node)); self.tiles.retain(|node, _| live_nodes.contains(node)); + // A removed leaf must report no bounds, not a stale rect. + self.node_bounds.retain(|node, _| live_nodes.contains(node)); let departed: Vec> = self .panels diff --git a/crates/base/src/macos_accessibility.rs b/crates/base/src/macos_accessibility.rs index e7b66fbb93..2d0d1e342a 100644 --- a/crates/base/src/macos_accessibility.rs +++ b/crates/base/src/macos_accessibility.rs @@ -14,14 +14,8 @@ use objc2_foundation::NSPoint; use raw_window_handle::{HasWindowHandle, RawWindowHandle}; pub fn install_window_hit_test_forwarder(window: &Window) { - // A headless/test window's `window_handle()` panics (`unimplemented!`) - // instead of returning `Err`, so `ns_view`'s graceful `.ok()?` never - // gets to fire. Probe under `catch_unwind` and no-op when there is no - // backing AppKit window (gpui's test platform), so downstream crates' - // tests -- which build a `Root` over a test window -- don't panic here. - let view = match std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| ns_view(window))) { - Ok(Some(view)) => view, - _ => return, + let Some(view) = ns_view(window) else { + return; }; let Some(window) = view.window() else { return; @@ -46,7 +40,14 @@ extern "C" fn hit_test_forwarder(this: &NSWindow, _cmd: Sel, point: NSPoint) -> } fn ns_view(window: &Window) -> Option<&NSView> { - let handle = HasWindowHandle::window_handle(window).ok()?; + // A test window's `window_handle()` panics instead of returning `Err`. + // Probe only that call under `catch_unwind` (the forwarder is then not + // installed); kept narrow so a real failure below still surfaces. + let handle = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { + HasWindowHandle::window_handle(window).ok() + })) + .ok() + .flatten()?; let RawWindowHandle::AppKit(handle) = handle.as_raw() else { return None; }; diff --git a/crates/component/src/dock/dock.rs b/crates/component/src/dock/dock.rs index de7c6c8c80..ee345b1563 100644 --- a/crates/component/src/dock/dock.rs +++ b/crates/component/src/dock/dock.rs @@ -521,4 +521,55 @@ mod tests { left.size.height, ); } + + /// A removed leaf reports `None`, not the rect it was last drawn with. + #[gpui::test] + fn node_bounds_drops_a_removed_leaf(cx: &mut TestAppContext) { + cx.update(|cx| crate::init(cx)); + let (area, cx) = cx.add_window_view(|window, cx| { + DockArea::new("test", None, window, cx).with_renderer(DockSkin::new(cx)) + }); + cx.simulate_resize(size(px(800.), px(600.))); + cx.update(|window, cx| { + area.update(cx, |area, cx| { + area.set_center( + DockLayout::h_split() + .child(DockLayout::tabs().panel(MeasuredProbe::new(Rc::default(), cx)), None) + .child(DockLayout::tabs().panel(MeasuredProbe::new(Rc::default(), cx)), None), + window, + cx, + ); + }); + }); + cx.run_until_parked(); + cx.update(|window, cx| window.draw(cx).clear(cx)); + + let (right_id, right_panel) = cx.update(|_, cx| { + let area = area.read(cx); + let tree = area.layout(DockPlacement::Center).expect("a center tree"); + let PaneRef::Split { children, .. } = tree.root().kind() else { + panic!("the center root is a horizontal split"); + }; + let right = &children[1]; + let PaneRef::Tabs { panels, .. } = right.kind() else { + panic!("the right child is a tab group"); + }; + (right.id(), panels[0]) + }); + + assert!( + cx.update(|_, cx| area.read(cx).node_bounds(right_id).is_some()), + "the right leaf's rect is captured while it is on screen" + ); + + cx.update(|window, cx| { + area.update(cx, |area, cx| area.remove_panel_id(right_panel, window, cx)); + }); + cx.run_until_parked(); + + assert!( + cx.update(|_, cx| area.read(cx).node_bounds(right_id).is_none()), + "a removed leaf reports no bounds, not the rect it was last drawn with" + ); + } } diff --git a/crates/component/src/dock/tab_panel.rs b/crates/component/src/dock/tab_panel.rs index d36a171ee9..fc48bec83e 100644 --- a/crates/component/src/dock/tab_panel.rs +++ b/crates/component/src/dock/tab_panel.rs @@ -502,12 +502,16 @@ impl TabGroupSkin { Some(tab_name) => this.child(tab_name), None => this.child(panel_title(panel, window, cx)), }) - // A per-tab close (X) button, shown only for a - // panel that reports itself closable. Stops - // propagation so the click never also selects the - // tab, and closes by id so it targets this tab - // regardless of which one is active. - .when(panel.closable(cx), |this| { + // A per-tab close (X) button, shown only when the + // panel reports itself closable and the group can + // actually close it. `is_draggable` is the same + // gate `TabGroup::close_panel` checks -- false for a + // locked layout or a group's last visible panel -- + // so the button never draws when a click would do + // nothing. Stops propagation so the click never also + // selects the tab, and closes by id so it targets + // this tab regardless of which one is active. + .when(group.is_draggable() && panel.closable(cx), |this| { this.suffix( Button::new(("close-tab", ix)) .icon(IconName::Close) @@ -813,7 +817,8 @@ mod tests { use std::cell::{Cell, RefCell}; use gpui::{ - Entity, EventEmitter, FocusHandle, Focusable, Pixels, TestAppContext, VisualTestContext, + Entity, EventEmitter, FocusHandle, Focusable, Modifiers, MouseButton, Pixels, + TestAppContext, VisualTestContext, }; use gpui_base::dock::{ DockArea, DockAreaRenderer, DockLayout, DockPlacement, PanelEvent, TileContext, @@ -1494,12 +1499,13 @@ mod tests { ); } - /// A panel that reports a chosen `closable` and records whether the skin - /// routed its tab through [`Panel::render_tab`]. + /// Panel with a chosen `closable` that records whether `render_tab` ran + /// and whether it was ever made active (i.e. a tab-select fired). struct TabProbe { focus_handle: FocusHandle, closable: bool, rendered_tab: Rc>, + activated: Rc>, } impl TabProbe { @@ -1508,6 +1514,7 @@ mod tests { focus_handle: cx.focus_handle(), closable, rendered_tab, + activated: Rc::new(Cell::new(false)), }) } } @@ -1520,6 +1527,12 @@ mod tests { fn closable(&self, _: &App) -> bool { self.closable } + + fn set_active(&mut self, active: bool, _: &mut Window, _: &mut Context) { + if active { + self.activated.set(true); + } + } } impl Panel for TabProbe { @@ -1608,4 +1621,58 @@ mod tests { "the skin must route the panel's tab through Panel::render_tab" ); } + + /// Clicking a non-active tab's close button removes that panel by id and + /// does not select it (`stop_propagation`). The displayed tab is a + /// non-closable filler, so the only close button is the one under test. + #[gpui::test] + fn clicking_a_close_button_removes_only_that_tab(cx: &mut TestAppContext) { + cx.update(|cx| crate::init(cx)); + let (area, cx) = cx.add_window_view(|window, cx| { + DockArea::new("skin", None, window, cx).with_renderer(DockSkin::new(cx)) + }); + + let (closable_id, filler_id, activated) = cx.update(|window, cx| { + let filler = TabProbe::new(false, Rc::new(Cell::new(false)), cx); + let closable = TabProbe::new(true, Rc::new(Cell::new(false)), cx); + let activated = closable.read(cx).activated.clone(); + let filler_handle = panel_handle(filler); + let closable_handle = panel_handle(closable); + let ids = (closable_handle.panel_id(cx), filler_handle.panel_id(cx)); + // Filler at ix 0 is the displayed tab; the closable tab at ix 1 is + // never active, so its close button is the only one drawn. + let layout = DockLayout::tabs() + .panel_view(filler_handle, cx) + .panel_view(closable_handle, cx); + area.update(cx, |area, cx| area.set_center(layout, window, cx)); + (ids.0, ids.1, activated) + }); + cx.run_until_parked(); + cx.update(|window, cx| window.draw(cx).clear(cx)); + + assert!( + cx.update(|_, cx| area.read(cx).panel(closable_id).is_some()), + "the closable tab starts owned by the area" + ); + + let button = cx + .debug_bounds(CLOSE_BUTTON_SELECTOR) + .expect("the non-active closable tab draws a close button"); + cx.simulate_mouse_down(button.center(), MouseButton::Left, Modifiers::none()); + cx.simulate_mouse_up(button.center(), MouseButton::Left, Modifiers::none()); + cx.run_until_parked(); + + assert!( + cx.update(|_, cx| area.read(cx).panel(closable_id).is_none()), + "the close click removes exactly the tab it belongs to" + ); + assert!( + cx.update(|_, cx| area.read(cx).panel(filler_id).is_some()), + "the displayed filler tab is untouched" + ); + assert!( + !activated.get(), + "stop_propagation keeps the close click from also selecting the tab" + ); + } } From 753f1545d9b57a2ed9f78df50c984a9f3da7e726 Mon Sep 17 00:00:00 2001 From: Lander Brandt Date: Sat, 5 Sep 2026 06:16:33 -0700 Subject: [PATCH 7/7] fix(dock): address second review round on close button and a11y probe - Gate the per-tab close button on the full close_panel condition: add TabGroupContext::is_close_permitted (the container-level closable bit) and require !collapsed, so the X never draws on a locked or last-panel group, nor on a collapsed dock strip where a click would destroy a panel instead of reopening the dock. Correct the comment that wrongly called is_draggable the same gate close_panel checks. - Make Panel::render_tab take &self/&App instead of &mut self, removing the mutable panel-entity borrow held across the tab bar's render. - Give the close button a Dock.Close tooltip. - Cache the macOS window-handle probe per window and warn once on a panic, so ns_view no longer pays a catch_unwind plus unwind on every focused frame over a test window, and a genuine failure on a real window is not silently swallowed. - Test the collapsed-group close-button gate. --- crates/base/src/dock/tab_group.rs | 14 +++- crates/base/src/input/base/native.rs | 46 ++++++++--- crates/component/src/dock/panel.rs | 18 ++--- crates/component/src/dock/tab_panel.rs | 105 ++++++++++++++++++------- 4 files changed, 131 insertions(+), 52 deletions(-) diff --git a/crates/base/src/dock/tab_group.rs b/crates/base/src/dock/tab_group.rs index 33f0e2fdcb..172466eea8 100644 --- a/crates/base/src/dock/tab_group.rs +++ b/crates/base/src/dock/tab_group.rs @@ -280,6 +280,7 @@ impl TabGroup { zoomed: self.zoomed, collapsed: self.constraints.is_collapsed(), closable: self.is_closable(cx), + close_permitted: self.constraints.is_closable(), locked: self.is_locked(), draggable: self.draggable(cx), droppable: self.droppable(), @@ -784,6 +785,7 @@ pub struct TabGroupContext { draggable: bool, droppable: bool, closable: bool, + close_permitted: bool, drop_indicator: Option, on_select_tab: SelectTabHandler, on_close: ClosePanelHandler, @@ -827,12 +829,20 @@ impl TabGroupContext { self.collapsed } - /// Whether closing the displayed panel is allowed at all, so a skin knows - /// whether to offer a Close control. + /// Whether the *active* panel can be closed. For a per-tab control use + /// [`Self::is_close_permitted`] with the tab's own [`PanelView::closable`]. pub fn is_closable(&self) -> bool { self.closable } + /// Whether the container permits closing panels at all -- the group-level + /// half of [`TabGroup::close_panel`]'s gate, before the per-panel check. + /// False for a dock's last group. Combine with [`Self::is_draggable`] and + /// the tab's own `closable` for a per-tab close control. + pub fn is_close_permitted(&self) -> bool { + self.close_permitted + } + pub fn is_locked(&self) -> bool { self.locked } diff --git a/crates/base/src/input/base/native.rs b/crates/base/src/input/base/native.rs index b4cc064957..05164e7c98 100644 --- a/crates/base/src/input/base/native.rs +++ b/crates/base/src/input/base/native.rs @@ -1,6 +1,11 @@ #[cfg(target_os = "macos")] mod macos { - use std::{cell::RefCell, collections::HashMap, mem, ptr, sync::Once}; + use std::{ + cell::RefCell, + collections::{HashMap, HashSet}, + mem, ptr, + sync::Once, + }; use gpui::Window; use objc2::{ @@ -17,6 +22,10 @@ mod macos { thread_local! { static CONTENT_TYPES: RefCell>> = RefCell::new(HashMap::new()); + + /// Windows whose raw `window_handle()` has no view, cached so the + /// panicking probe below runs once per window, not once per frame. + static HANDLE_UNAVAILABLE: RefCell> = RefCell::new(HashSet::new()); } pub fn set_text_content_type(window: &Window, content_type: Option<&str>) { @@ -44,16 +53,31 @@ mod macos { } fn ns_view(window: &Window) -> Option<&AnyObject> { - // A headless/test window's `window_handle()` panics (`unimplemented!`) - // instead of returning Err, so the graceful `.ok()?` never fires. - // Probe under catch_unwind and treat a panic as "no view" (the Input - // then skips native text-content wiring) so downstream crates' tests - // that render an Input over a test window don't panic here. - let handle = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { - HasWindowHandle::window_handle(window).ok() - })) - .ok() - .flatten()?; + // A test window's raw `window_handle()` panics instead of returning + // `Err`. Probe under `catch_unwind` once per window, caching failures; + // this runs every focused frame. `Window::window_handle` is a separate, + // non-panicking call, safe as the cache key. + let window_id = window.window_handle().window_id(); + if HANDLE_UNAVAILABLE.with(|seen| seen.borrow().contains(&window_id)) { + return None; + } + let handle = match std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { + HasWindowHandle::window_handle(window) + })) { + Ok(Ok(handle)) => handle, + // A panic is expected on a test window; on a real one it is a + // genuine failure, so warn once instead of swallowing it. + other => { + let first = HANDLE_UNAVAILABLE.with(|seen| seen.borrow_mut().insert(window_id)); + if first && matches!(other, Err(_)) { + tracing::warn!( + ?window_id, + "window handle probe panicked; skipping native text-content wiring" + ); + } + return None; + } + }; let RawWindowHandle::AppKit(handle) = handle.as_raw() else { return None; }; diff --git a/crates/component/src/dock/panel.rs b/crates/component/src/dock/panel.rs index 34eaa28998..8a9ae1457d 100644 --- a/crates/component/src/dock/panel.rs +++ b/crates/component/src/dock/panel.rs @@ -138,14 +138,10 @@ pub trait Panel: gpui_base::dock::Panel { true } - /// Customize this panel's own tab in the tab bar. - /// - /// The tab group builds a fully wired [`Tab`] -- its index, label, - /// selected state, click-to-activate, and drag/drop are already set -- - /// and hands it here for the panel to do final tweaks to e.g. styling. - /// Returns the (possibly customized) tab, so the tab bar's own layout - /// and behavior are preserved. The default returns it untouched. - fn render_tab(&mut self, tab: Tab, window: &mut Window, cx: &mut Context) -> Tab { + /// Final tweak to this panel's fully-wired tab (styling, label, suffix). + /// The default returns it untouched. `&self`: it runs inside the tab bar's + /// render, so `&mut self` would risk re-entrancy on the panel entity. + fn render_tab(&self, tab: Tab, window: &mut Window, cx: &App) -> Tab { tab } } @@ -161,7 +157,7 @@ pub trait PanelView: gpui_base::dock::PanelView { fn dropdown_menu(&self, menu: PopupMenu, window: &mut Window, cx: &mut App) -> PopupMenu; fn zoom_control(&self, cx: &App) -> Option; fn inner_padding(&self, cx: &App) -> bool; - fn render_tab(&self, tab: Tab, window: &mut Window, cx: &mut App) -> Tab; + fn render_tab(&self, tab: Tab, window: &mut Window, cx: &App) -> Tab; } impl PanelView for Entity { @@ -200,8 +196,8 @@ impl PanelView for Entity { self.read(cx).inner_padding(cx) } - fn render_tab(&self, tab: Tab, window: &mut Window, cx: &mut App) -> Tab { - self.update(cx, |this, cx| this.render_tab(tab, window, cx)) + fn render_tab(&self, tab: Tab, window: &mut Window, cx: &App) -> Tab { + self.read(cx).render_tab(tab, window, cx) } } diff --git a/crates/component/src/dock/tab_panel.rs b/crates/component/src/dock/tab_panel.rs index fc48bec83e..dd60f02ff3 100644 --- a/crates/component/src/dock/tab_panel.rs +++ b/crates/component/src/dock/tab_panel.rs @@ -502,33 +502,38 @@ impl TabGroupSkin { Some(tab_name) => this.child(tab_name), None => this.child(panel_title(panel, window, cx)), }) - // A per-tab close (X) button, shown only when the - // panel reports itself closable and the group can - // actually close it. `is_draggable` is the same - // gate `TabGroup::close_panel` checks -- false for a - // locked layout or a group's last visible panel -- - // so the button never draws when a click would do - // nothing. Stops propagation so the click never also - // selects the tab, and closes by id so it targets - // this tab regardless of which one is active. - .when(group.is_draggable() && panel.closable(cx), |this| { - this.suffix( - Button::new(("close-tab", ix)) - .icon(IconName::Close) - .xsmall() - .ghost() - .tab_stop(false) - .debug_selector(|| CLOSE_BUTTON_SELECTOR.to_string()) - .on_click({ - let group = group.clone(); - let panel_id = panel.panel_id(cx); - move |_, window, cx| { - cx.stop_propagation(); - group.close(panel_id, window, cx); - } - }), - ) - }) + // Per-tab close (X) button. The gate mirrors + // `TabGroup::close_panel`: container permits closing + // (not a dock's last group), group is draggable, and + // the panel is closable; plus `!collapsed`, since a + // collapsed strip is a way back in, not a place to + // close. Stops propagation so the click closes by id + // without also selecting the tab. + .when( + !collapsed + && group.is_close_permitted() + && group.is_draggable() + && panel.closable(cx), + |this| { + this.suffix( + Button::new(("close-tab", ix)) + .icon(IconName::Close) + .xsmall() + .ghost() + .tab_stop(false) + .tooltip(t!("Dock.Close")) + .debug_selector(|| CLOSE_BUTTON_SELECTOR.to_string()) + .on_click({ + let group = group.clone(); + let panel_id = panel.panel_id(cx); + move |_, window, cx| { + cx.stop_propagation(); + group.close(panel_id, window, cx); + } + }), + ) + }, + ) // A collapsed group shows no tab as active: the // strip is a way back in, not a selection. The // comparison is against the panel on screen, not @@ -1536,7 +1541,7 @@ mod tests { } impl Panel for TabProbe { - fn render_tab(&mut self, tab: Tab, _: &mut Window, _: &mut Context) -> Tab { + fn render_tab(&self, tab: Tab, _: &mut Window, _: &App) -> Tab { self.rendered_tab.set(true); tab } @@ -1675,4 +1680,48 @@ mod tests { "stop_propagation keeps the close click from also selecting the tab" ); } + + /// A collapsed group offers no close button, though the same group does + /// while open. Before/after collapse isolates `!collapsed`; nothing else + /// changes. + #[gpui::test] + fn a_collapsed_group_draws_no_close_button(cx: &mut TestAppContext) { + cx.update(|cx| crate::init(cx)); + let (area, cx) = cx.add_window_view(|window, cx| { + DockArea::new("skin", None, window, cx).with_renderer(DockSkin::new(cx)) + }); + // Two closable panels so the group is draggable (not on its last + // visible panel) and offers close buttons while open. + cx.update(|window, cx| { + let a = TabProbe::new(true, Rc::new(Cell::new(false)), cx); + let b = TabProbe::new(true, Rc::new(Cell::new(false)), cx); + let layout = DockLayout::tabs() + .panel_view(panel_handle(a), cx) + .panel_view(panel_handle(b), cx); + area.update(cx, |area, cx| { + area.set_dock(DockPlacement::Bottom, layout, window, cx); + if !area.is_dock_open(DockPlacement::Bottom) { + area.toggle_dock(DockPlacement::Bottom, window, cx); + } + }); + }); + cx.run_until_parked(); + cx.update(|window, cx| window.draw(cx).clear(cx)); + assert!( + cx.debug_bounds(CLOSE_BUTTON_SELECTOR).is_some(), + "an open group with two closable panels offers a close button" + ); + + // Collapse the bottom dock: its strip stays clickable, but the close + // buttons on it must not. + cx.update(|window, cx| { + area.update(cx, |area, cx| area.toggle_dock(DockPlacement::Bottom, window, cx)); + }); + cx.run_until_parked(); + cx.update(|window, cx| window.draw(cx).clear(cx)); + assert!( + cx.debug_bounds(CLOSE_BUTTON_SELECTOR).is_none(), + "a collapsed group must not offer a close button" + ); + } }