From cdb7c88e0c5a3f337fbd0f3f929c7f2be1fee771 Mon Sep 17 00:00:00 2001 From: kirillDevPro <113171057+kirillDevPro@users.noreply.github.com> Date: Sun, 20 Sep 2026 16:29:32 +0200 Subject: [PATCH] fix(tooltip): render popover-hosted tooltips above their parent A tooltip on a button inside a MoonPopover never appears. Every settings popup is built from glyph buttons whose meaning lives in the tooltip, so those controls read as unlabeled squares. GPUI sorts every deferred draw into one global ascending order by priority and paints in that order, regardless of nesting. The managed tooltip overlay deferred at priority 2 while a MoonPopover defers at 30_000, so the popover always painted over the tooltip. A tooltip is the topmost transient surface in any interface, so it now defers above every other band unconditionally rather than opting in per host. The bands themselves move into one module, src/layer.rs, which is the only place they are numbered: LAYER_OVERLAY (1), LAYER_MOON_POPOVER (30_000), LAYER_MOON_POPOVER_MENU (31_000) and LAYER_TOOLTIP (100_000). Every call site that carried a bare literal now names its band. Only the tooltip's value changes behaviour; the one other renumber, the popover-hosted select menu from 30_001 to 31_000, widens the gap without changing any ordering, and the existing test there asserts the relation rather than the number. src/time/date_picker.rs keeps its bare literal 2: that file is a Mirror component with a zero donor-drift budget, so any byte change there fails the donor-mirror guardrail. The gallery's popover gains a tooltip-bearing button so the behaviour has somewhere to be seen. Two tests pin the result: a band-relation test over the constants, and a headless scene-quad probe asserting the tooltip paints above its hosting popover in both themes. --- crates/moon-ui-components/src/combobox.rs | 2 +- crates/moon-ui-components/src/layer.rs | 27 +++ crates/moon-ui-components/src/layer/tests.rs | 20 ++ crates/moon-ui-components/src/lib.rs | 1 + .../src/menu/context_menu.rs | 2 +- .../src/moon/dropdown/popup.rs | 7 +- .../src/moon/dropdown/trigger.rs | 2 +- crates/moon-ui-components/src/moon/popover.rs | 4 +- .../src/moon/popover/tests.rs | 217 ++++++++++++++++++ crates/moon-ui-components/src/moon/select.rs | 2 +- .../src/native_menu/fallback.rs | 2 +- crates/moon-ui-components/src/popover.rs | 4 +- crates/moon-ui-components/src/select.rs | 2 +- crates/moon-ui-components/src/tooltip.rs | 11 +- crates/moon-ui-gallery/src/gallery.rs | 1 + docs/component-mirror-baseline.json | 24 +- 16 files changed, 303 insertions(+), 25 deletions(-) create mode 100644 crates/moon-ui-components/src/layer.rs create mode 100644 crates/moon-ui-components/src/layer/tests.rs diff --git a/crates/moon-ui-components/src/combobox.rs b/crates/moon-ui-components/src/combobox.rs index 2206385..f3cac61 100644 --- a/crates/moon-ui-components/src/combobox.rs +++ b/crates/moon-ui-components/src/combobox.rs @@ -741,7 +741,7 @@ where dismiss_handler, cx, )) - .with_priority(1), + .with_priority(crate::layer::LAYER_OVERLAY), ) }) } diff --git a/crates/moon-ui-components/src/layer.rs b/crates/moon-ui-components/src/layer.rs new file mode 100644 index 0000000..1694cbd --- /dev/null +++ b/crates/moon-ui-components/src/layer.rs @@ -0,0 +1,27 @@ +//! Named deferred-draw priority bands for MoonUI overlay surfaces. +//! +//! GPUI deferred draws are sorted into one global ascending order by `priority` and painted in +//! that order (`moon-gpui` `window.rs`, `deferred_draw_traversal_order`). This file is the only +//! place these bands are numbered. A higher number paints later and therefore on top. +//! +//! The band is absolute, not relative to the hosting surface: a child overlay inside a +//! higher-band parent must opt into a band above that parent or it is painted underneath. +//! `MoonSelect::in_popover()` (`moon/select.rs:378`) is the existing opt-in; the managed +//! tooltip avoids the problem by sitting above every band unconditionally. + +/// Longbridge default overlay band: plain popovers, select menus, comboboxes, context menus, +/// native-menu fallback, and dropdown submenus. +pub(crate) const LAYER_OVERLAY: usize = 1; + +/// Moon popover and Moon dropdown surfaces. +pub(crate) const LAYER_MOON_POPOVER: usize = 30_000; + +/// A menu opened from inside a Moon popover (`MoonSelect::in_popover`). +pub(crate) const LAYER_MOON_POPOVER_MENU: usize = 31_000; + +/// The managed tooltip overlay. The tooltip is the topmost transient surface and must outrank +/// every other deferred band. +pub(crate) const LAYER_TOOLTIP: usize = 100_000; + +#[cfg(test)] +mod tests; diff --git a/crates/moon-ui-components/src/layer/tests.rs b/crates/moon-ui-components/src/layer/tests.rs new file mode 100644 index 0000000..ddc5780 --- /dev/null +++ b/crates/moon-ui-components/src/layer/tests.rs @@ -0,0 +1,20 @@ +//! Band-ordering tests for the deferred-draw layer constants live here and are authored by the +//! breakage prover. + +use super::{LAYER_MOON_POPOVER, LAYER_MOON_POPOVER_MENU, LAYER_OVERLAY, LAYER_TOOLTIP}; + +/// `layer.rs` is the only place these bands are numbered. Collapsing two bands together or +/// renumbering `LAYER_TOOLTIP` under `LAYER_MOON_POPOVER_MENU` lets a Moon popover or its +/// in-popover select menu repaint over a tooltip again, reproducing MoonUI #628. +#[test] +fn tooltip_outranks_every_other_deferred_band() { + assert!(LAYER_TOOLTIP > LAYER_MOON_POPOVER_MENU); + assert!(LAYER_MOON_POPOVER_MENU > LAYER_MOON_POPOVER); + assert!(LAYER_MOON_POPOVER > LAYER_OVERLAY); + + // Independently derived from the frozen literal at + // crates/moon-ui-components/src/time/date_picker.rs:512 (`.with_priority(2)`), a + // component-mirror donor file this ordering must never regress below. + let date_picker_calendar_priority: usize = 2; + assert!(LAYER_TOOLTIP > date_picker_calendar_priority); +} diff --git a/crates/moon-ui-components/src/lib.rs b/crates/moon-ui-components/src/lib.rs index d7d70f5..205e41c 100644 --- a/crates/moon-ui-components/src/lib.rs +++ b/crates/moon-ui-components/src/lib.rs @@ -12,6 +12,7 @@ mod icon; mod index_path; #[cfg(any(feature = "inspector", debug_assertions))] mod inspector; +mod layer; pub mod moon; mod root; mod styled; diff --git a/crates/moon-ui-components/src/menu/context_menu.rs b/crates/moon-ui-components/src/menu/context_menu.rs index 73b5a55..8b0b652 100644 --- a/crates/moon-ui-components/src/menu/context_menu.rs +++ b/crates/moon-ui-components/src/menu/context_menu.rs @@ -205,7 +205,7 @@ impl Element for ContextMenu< ), ), ) - .with_priority(1) + .with_priority(crate::layer::LAYER_OVERLAY) .into_any(), ); } diff --git a/crates/moon-ui-components/src/moon/dropdown/popup.rs b/crates/moon-ui-components/src/moon/dropdown/popup.rs index 9116c5c..77ab92a 100644 --- a/crates/moon-ui-components/src/moon/dropdown/popup.rs +++ b/crates/moon-ui-components/src/moon/dropdown/popup.rs @@ -888,7 +888,12 @@ impl MoonPopupMenu { } .into_any_element(), }) - .with_priority(1), + // Deliberately the base band even though the parent dropdown sits at + // LAYER_MOON_POPOVER (moon/dropdown/trigger.rs:654, set unconditionally), + // so an overlapping submenu paints under its parent. Pre-existing, not + // fixed here; do not "correct" it to LAYER_MOON_POPOVER without checking + // the paint-order consequences. + .with_priority(crate::layer::LAYER_OVERLAY), ); } diff --git a/crates/moon-ui-components/src/moon/dropdown/trigger.rs b/crates/moon-ui-components/src/moon/dropdown/trigger.rs index 4ffc0cc..14f1d71 100644 --- a/crates/moon-ui-components/src/moon/dropdown/trigger.rs +++ b/crates/moon-ui-components/src/moon/dropdown/trigger.rs @@ -651,7 +651,7 @@ impl RenderOnce for MoonDropdown { let mut popover = CorePopover::new(ElementId::from(self.id.clone())) .appearance(false) .anchor(Anchor::TopLeft) - .deferred_priority(30_000) + .deferred_priority(crate::layer::LAYER_MOON_POPOVER) .open(open) .trigger_any(trigger) .content(move |_, window, cx| { diff --git a/crates/moon-ui-components/src/moon/popover.rs b/crates/moon-ui-components/src/moon/popover.rs index 037c855..2a7d1b8 100644 --- a/crates/moon-ui-components/src/moon/popover.rs +++ b/crates/moon-ui-components/src/moon/popover.rs @@ -7,8 +7,6 @@ use super::{ tokens::{MoonPalette, MoonRect, rgba_from}, }; -/// Shared layer boundary for popovers and controls opening menus above them. -pub(super) const MOON_POPOVER_PRIORITY: usize = 30_000; const POPOVER_PADDING: f32 = 6.0; const POPOVER_BORDER: f32 = 1.0; @@ -368,7 +366,7 @@ impl RenderOnce for MoonPopover { let mut popover = CorePopover::new(ElementId::from(self.id.clone())) .anchor(anchor_for(self.placement)) .appearance(false) - .deferred_priority(MOON_POPOVER_PRIORITY) + .deferred_priority(crate::layer::LAYER_MOON_POPOVER) .overlay_closable(self.overlay_closable) .open(open) .trigger_any(trigger) diff --git a/crates/moon-ui-components/src/moon/popover/tests.rs b/crates/moon-ui-components/src/moon/popover/tests.rs index dec2297..90b0d50 100644 --- a/crates/moon-ui-components/src/moon/popover/tests.rs +++ b/crates/moon-ui-components/src/moon/popover/tests.rs @@ -100,3 +100,220 @@ fn intrinsic_popover_shrink_wraps_its_rendered_child(cx: &mut gpui::TestAppConte assert_eq!(child.size.width, px(73.0)); assert_eq!(popup.size.width, child.size.width + px(chrome)); } + +// ---- Scene-order proof: managed tooltip must outrank its hosting Moon popover ---- + +use crate::ElementExt as _; +use crate::Root; +use crate::moon::{MoonButton, MoonThemeConfig}; +use crate::tooltip::{TooltipContent, TooltipOverlay}; +use gpui::{ + AnyWindowHandle, AppContext as _, AtlasKey, AtlasTile, Bounds, DevicePixels, + HeadlessAppContext, NoopTextSystem, ParentElement as _, Pixels, PlatformAtlas, + PlatformHeadlessRenderer, Quad, Scene, Size, size, +}; +use std::{ + borrow::Cow, + cell::{Cell, RefCell}, + rc::Rc, + sync::Arc, + time::Duration, +}; + +/// Fresh copy of moon/select/tests.rs's module-private recorder pair: exporting it would mean +/// rewriting a test file that already carries a passing test, out of scope for a proof dispatch. +struct HostedTooltipSceneRecorder(Rc>>); +impl PlatformHeadlessRenderer for HostedTooltipSceneRecorder { + fn render_scene_to_image( + &mut self, + scene: &Scene, + size: Size, + ) -> anyhow::Result { + self.render_scene(scene, size)?; + anyhow::bail!("scene recorder does not rasterize images") + } + fn render_scene(&mut self, scene: &Scene, _size: Size) -> anyhow::Result<()> { + self.0.replace(scene.quads.clone()); + Ok(()) + } + fn sprite_atlas(&self) -> Arc { + Arc::new(HostedTooltipQuadOnlyAtlas) + } +} + +struct HostedTooltipQuadOnlyAtlas; +impl PlatformAtlas for HostedTooltipQuadOnlyAtlas { + fn get_or_insert_with<'a>( + &self, + _key: &AtlasKey, + _build: &mut dyn FnMut() -> anyhow::Result, Cow<'a, [u8]>)>>, + ) -> anyhow::Result> { + Ok(None) + } + fn remove(&self, _key: &AtlasKey) {} +} + +const HOSTED_TOOLTIP_PROBE_W: f32 = 96.0; +const HOSTED_TOOLTIP_PROBE_H: f32 = 28.0; + +/// Fixed-size tooltip body so its scene quad cannot be mistaken for the popover chrome. +struct HostedTooltipProbeBody; +impl Render for HostedTooltipProbeBody { + fn render(&mut self, _window: &mut Window, _cx: &mut Context) -> impl IntoElement { + div() + .w(px(HOSTED_TOOLTIP_PROBE_W)) + .h(px(HOSTED_TOOLTIP_PROBE_H)) + .bg(gpui::black()) + } +} + +/// Always-open Moon popover hosting a button positioned well inside the popover box on every side. +struct HostedTooltipHarness { + button_bounds: Rc>>, +} +impl Render for HostedTooltipHarness { + fn render(&mut self, _window: &mut Window, _cx: &mut Context) -> impl IntoElement { + let button_bounds = self.button_bounds.clone(); + MoonPopover::new("hosted-tooltip") + .open(true) + .width(400.0) + .trigger(div().w(px(20.0)).h(px(20.0))) + .content( + div().relative().w(px(360.0)).h(px(240.0)).child( + div() + .id("hosted-tooltip:button") + .absolute() + .left(px(140.0)) + .top(px(100.0)) + .w(px(80.0)) + .h(px(32.0)) + .on_prepaint(move |bounds, _, _| button_bounds.set(bounds)) + .child(MoonButton::new("hosted-button").tooltip("Hosted tooltip")), + ), + ) + } +} + +/// Pumps N draw frames so prepaint capture and animation state settle deterministically. +fn hosted_tooltip_pump(cx: &mut HeadlessAppContext, window: AnyWindowHandle, frames: usize) { + for _ in 0..frames { + cx.update_window(window, |_, window, cx| { + window.refresh(); + window.draw(cx).clear(); + }) + .expect("headless frame must draw"); + cx.run_until_parked(); + } +} + +/// tooltip.rs TooltipOverlay::render .with_priority(LAYER_TOOLTIP) must keep the managed tooltip +/// above its hosting Moon popover (LAYER_TOOLTIP == 100_000 vs LAYER_MOON_POPOVER == 30_000). +/// Lowering it back to a literal, or pointing it at LAYER_OVERLAY, reintroduces MoonUI #628. +/// layer::tests pins the constants RELATION; this pins that the overlay actually USES it. +#[test] +fn popover_hosted_tooltip_paints_above_its_popover() { + for (theme, config) in [ + ("dark", MoonThemeConfig::moon_terminal()), + ("light", MoonThemeConfig::moon_light()), + ] { + let quads = Rc::new(RefCell::new(Vec::new())); + let recorded = quads.clone(); + let mut cx = HeadlessAppContext::with_platform( + Arc::new(NoopTextSystem::new()), + Arc::new(()), + move || Some(Box::new(HostedTooltipSceneRecorder(recorded.clone()))), + ); + cx.update(crate::init); + cx.update(|cx| MoonTheme::install_config(config.clone(), cx)); + + let button_bounds = Rc::new(Cell::new(Bounds::default())); + let window = cx + .open_window(size(px(800.0), px(600.0)), { + let button_bounds = button_bounds.clone(); + move |window, cx| { + let view = cx.new(|_| HostedTooltipHarness { button_bounds }); + cx.new(|cx| Root::new(view, window, cx)) + } + }) + .expect("headless window must open"); + + hosted_tooltip_pump(&mut cx, window.into(), 4); + let trigger_bounds = button_bounds.get(); + + cx.update_window(window.into(), |_, window, cx| { + let overlay = + Root::tooltip_overlay(window, cx).expect("Root must own a tooltip overlay"); + overlay.update(cx, |overlay: &mut TooltipOverlay, cx| { + let content = TooltipContent { + build: Rc::new(|_, cx| cx.new(|_| HostedTooltipProbeBody).into()), + trigger_bounds, + }; + overlay.request_show(content, window, cx); + }); + }) + .expect("headless window must accept the direct request_show drive"); + + // Cold-start timer (tooltip.rs:409-434) never fires under run_until_parked alone + // (dispatcher.rs:76-78); advance the simulated clock by hand. + cx.advance_clock(Duration::from_millis(500)); + let scale = cx + .update_window(window.into(), |_, window, _| window.scale_factor()) + .expect("headless window must retain its scale"); + hosted_tooltip_pump(&mut cx, window.into(), 8); + // The recorder only fires on an actual render pass; a plain draw() never reaches it. + let _ = cx.capture_screenshot(window.into()); + + let quads = quads.borrow(); + let popovers: Vec<_> = quads + .iter() + .filter(|q| !q.background.is_transparent() && q.bounds.size.width.0 == 400.0 * scale) + .collect(); + let tooltips: Vec<_> = quads + .iter() + .filter(|q| { + !q.background.is_transparent() + && q.bounds.size.width.0 == HOSTED_TOOLTIP_PROBE_W * scale + && q.bounds.size.height.0 == HOSTED_TOOLTIP_PROBE_H * scale + }) + .collect(); + + assert_eq!( + popovers.len(), + 1, + "{theme}: unique hosting popover: {quads:?}" + ); + // Must exist BEFORE any order comparison -- a "not found" path that skips the compare + // could never redden under the named mutation. + assert_eq!( + tooltips.len(), + 1, + "{theme}: tooltip quad must exist: {quads:?}" + ); + let popover = popovers[0]; + let tooltip = tooltips[0]; + let target = tooltip.bounds.center(); + + assert!( + popover.bounds.contains(&target), + "{theme}: tooltip must render inside its popover" + ); + assert!( + popover.content_mask.bounds.contains(&target), + "{theme}: popover must not be clipped at the probe point" + ); + assert!( + tooltip.content_mask.bounds.contains(&target), + "{theme}: tooltip must not be clipped at the probe point" + ); + assert_ne!( + tooltip.order, popover.order, + "{theme}: overlapping surfaces must have distinct draw orders" + ); + assert!( + tooltip.order > popover.order, + "{theme}: tooltip must paint above its hosting popover; popover={}, tooltip={}", + popover.order, + tooltip.order + ); + } +} diff --git a/crates/moon-ui-components/src/moon/select.rs b/crates/moon-ui-components/src/moon/select.rs index 99986f9..275c845 100644 --- a/crates/moon-ui-components/src/moon/select.rs +++ b/crates/moon-ui-components/src/moon/select.rs @@ -408,7 +408,7 @@ where .with_size(size_for(trigger_size, menu_size)); if self.in_popover { - select = select.menu_priority(super::popover::MOON_POPOVER_PRIORITY + 1); + select = select.menu_priority(crate::layer::LAYER_MOON_POPOVER_MENU); } if let Some(trigger_variant) = self.trigger_variant { diff --git a/crates/moon-ui-components/src/native_menu/fallback.rs b/crates/moon-ui-components/src/native_menu/fallback.rs index 52aaf55..71b2454 100644 --- a/crates/moon-ui-components/src/native_menu/fallback.rs +++ b/crates/moon-ui-components/src/native_menu/fallback.rs @@ -107,7 +107,7 @@ impl Render for FallbackMenuOverlay { .snap_to_window_with_margin(px(8.)) .child(active.menu.clone()), ) - .with_priority(1), + .with_priority(crate::layer::LAYER_OVERLAY), ); } root diff --git a/crates/moon-ui-components/src/popover.rs b/crates/moon-ui-components/src/popover.rs index f75c834..f667635 100644 --- a/crates/moon-ui-components/src/popover.rs +++ b/crates/moon-ui-components/src/popover.rs @@ -59,7 +59,7 @@ impl Popover { appearance: true, overlay_closable: true, close_on_content_click: false, - deferred_priority: 1, + deferred_priority: crate::layer::LAYER_OVERLAY, default_open: false, open: None, on_open_change: None, @@ -153,7 +153,7 @@ impl Popover { self } - /// Override deferred overlay priority. + /// Override deferred overlay priority. The default is `LAYER_OVERLAY`. pub fn deferred_priority(mut self, priority: usize) -> Self { self.deferred_priority = priority; self diff --git a/crates/moon-ui-components/src/select.rs b/crates/moon-ui-components/src/select.rs index a2ef2af..01c6523 100644 --- a/crates/moon-ui-components/src/select.rs +++ b/crates/moon-ui-components/src/select.rs @@ -92,7 +92,7 @@ impl Default for SelectOptions { title_prefix: None, menu_width: Length::Auto, menu_max_h: rems(20.).into(), - menu_priority: 1, + menu_priority: crate::layer::LAYER_OVERLAY, disabled: false, appearance: true, search_placeholder: None, diff --git a/crates/moon-ui-components/src/tooltip.rs b/crates/moon-ui-components/src/tooltip.rs index 9671512..7424260 100644 --- a/crates/moon-ui-components/src/tooltip.rs +++ b/crates/moon-ui-components/src/tooltip.rs @@ -12,6 +12,7 @@ use crate::{ animation::{Transition, ease_in_out_cubic, ease_out_cubic}, h_flex, kbd::Kbd, + layer::LAYER_TOOLTIP, root::Root, text::Text, }; @@ -498,6 +499,14 @@ impl Render for TooltipOverlay { let is_switching = self.is_switching; let prev_trigger_bounds = self.prev_trigger_bounds; + // The managed tooltip overlay must outrank every other deferred band, including + // LAYER_MOON_POPOVER, because a tooltip is the topmost transient surface. The matching + // literal 2 in src/time/date_picker.rs:512 is deliberately left alone: that file is a + // Mirror component with a zero donor-drift budget, and any byte change there fails + // `cargo xtask component-mirror`. That literal is the date picker's calendar overlay, so + // it is still subject to this bug: a MoonDatePicker inside a MoonPopover has its calendar + // painted underneath. Fixing it needs a donor-side change or a component-class change; + // it is a follow-up, not resolved. deferred( tooltip_overlay_positioner(trigger_bounds).child(div().child(content_view).map(|el| { if is_switching { @@ -538,7 +547,7 @@ impl Render for TooltipOverlay { } })), ) - .with_priority(2) + .with_priority(LAYER_TOOLTIP) .into_any_element() } } diff --git a/crates/moon-ui-gallery/src/gallery.rs b/crates/moon-ui-gallery/src/gallery.rs index 3afd912..5d619d9 100644 --- a/crates/moon-ui-gallery/src/gallery.rs +++ b/crates/moon-ui-gallery/src/gallery.rs @@ -1229,6 +1229,7 @@ impl Gallery { MoonButton::new("popover-action") .label("Action") .variant(MoonButtonVariant::Blue) + .tooltip("Runs the popover action") .render(), ), ), diff --git a/docs/component-mirror-baseline.json b/docs/component-mirror-baseline.json index cd10992..4c46351 100644 --- a/docs/component-mirror-baseline.json +++ b/docs/component-mirror-baseline.json @@ -227,12 +227,12 @@ "local_paths": [ "combobox.rs" ], - "local_hash": "e645b6303a418133", + "local_hash": "4c7d616e46a4fae8", "local_files": [ { "path": "combobox.rs", - "hash": "709e82aad68cabcb", - "bytes": 52680 + "hash": "b87ebd3a62f14230", + "bytes": 52706 } ], "donor_hash": "2324438aee108e17", @@ -928,12 +928,12 @@ "local_paths": [ "native_menu" ], - "local_hash": "d8df00f5803a0e99", + "local_hash": "8d4cd7ecde3da41b", "local_files": [ { "path": "native_menu/fallback.rs", - "hash": "ca4beeae7b3c113f", - "bytes": 4341 + "hash": "d7aa4e43ed0816e6", + "bytes": 4367 }, { "path": "native_menu/macos.rs", @@ -1040,12 +1040,12 @@ "local_paths": [ "popover.rs" ], - "local_hash": "24b2a32c0455c8f0", + "local_hash": "529dd018e2c7f81b", "local_files": [ { "path": "popover.rs", - "hash": "0404b56976204327", - "bytes": 23015 + "hash": "b3316c5b02f1d758", + "bytes": 23073 } ], "donor_hash": "da9409c2c18e2a14", @@ -1214,12 +1214,12 @@ "local_paths": [ "select.rs" ], - "local_hash": "ce7472eac2cb6c8a", + "local_hash": "8da0aea74824b10f", "local_files": [ { "path": "select.rs", - "hash": "1f0377cf08137e57", - "bytes": 32216 + "hash": "f068cbebb4147784", + "bytes": 32242 } ], "donor_hash": "245acdb36368c661",