From 66961a1026dcdeee14304fc82de1d765227546d2 Mon Sep 17 00:00:00 2001 From: Ren Amamiya Date: Sat, 7 Nov 2026 19:42:10 +0700 Subject: [PATCH] audit menu --- crates/core/src/menu.rs | 22 ++++++++++----- crates/core/tests/menu.rs | 22 +++++++++++++++ crates/core/tests/select_pointer.rs | 42 +++++++++++++++++++++-------- docs/CHECKLIST.md | 8 +++--- 4 files changed, 74 insertions(+), 20 deletions(-) diff --git a/crates/core/src/menu.rs b/crates/core/src/menu.rs index 43fd3ff..1cae710 100644 --- a/crates/core/src/menu.rs +++ b/crates/core/src/menu.rs @@ -7,7 +7,7 @@ use gpui::{ Entity, InteractiveElement as _, IntoElement, KeyDownEvent, MouseButton, MouseDownEvent, ParentElement as _, Pixels, Point, RenderOnce, Role, ScrollHandle, SharedString, StatefulInteractiveElement as _, Styled as _, Window, anchored, canvas, deferred, div, - ease_out_quint, px, + ease_out_quint, px, relative, }; use gpui_native_icons::{IconRole, icon as icon_glyph}; use gpui_native_theme::ActiveTheme as _; @@ -138,6 +138,9 @@ struct MenuChrome { radius_sm: Pixels, icon_size: Pixels, dense_icon_size: Pixels, + menu_item_height: Pixels, + spacing_unit: Pixels, + text_size: Pixels, shadow: Vec, } @@ -258,6 +261,9 @@ impl RenderOnce for Menu { radius_sm: theme.radius.sm, icon_size: theme.sizes.medium.icon_size, dense_icon_size: theme.sizes.medium.dense_icon_size, + menu_item_height: theme.sizes.medium.menu_item_height, + spacing_unit: theme.spacing.unit, + text_size: theme.sizes.medium.text_size, shadow: theme.elevations.popover.shadows(), }, theme.motion.overlay, @@ -374,7 +380,7 @@ impl RenderOnce for Menu { .aria_label(label) .mt_1() .min_w_56() - .p_1() + .p(chrome.spacing_unit) .bg(chrome.surface) .text_color(chrome.foreground) .rounded(chrome.radius_overlay) @@ -454,8 +460,9 @@ fn render_item( match item.kind { MenuItemKind::Separator => div() .h(px(1.)) - .mx_2() - .my_1() + .debug_selector(move || format!("menu-separator-{index}")) + .mx(chrome.spacing_unit * 2.0) + .my(chrome.spacing_unit) .bg(chrome.border) .into_any_element(), MenuItemKind::Action => { @@ -468,11 +475,14 @@ fn render_item( div() .id(("menu-item", index)) + .debug_selector(move || format!("menu-item-{index}")) .flex() .items_center() .gap_2() - .px_2() - .py_1() + .min_h(chrome.menu_item_height) + .px(chrome.spacing_unit * 2.0) + .text_size(chrome.text_size) + .line_height(relative(1.0)) .rounded(chrome.radius_sm) .map(|row| { if disabled { diff --git a/crates/core/tests/menu.rs b/crates/core/tests/menu.rs index 6021dc0..dcf94c0 100644 --- a/crates/core/tests/menu.rs +++ b/crates/core/tests/menu.rs @@ -112,6 +112,28 @@ fn click(visual: &mut VisualTestContext, position: gpui::Point) { visual.simulate_mouse_up(position, MouseButton::Left, Modifiers::default()); } +/// Every action row renders at the platform's stated menu row height, +/// whatever its content — the metric is the whole row, not a floor under +/// padding. Item 1 of the harness menu is a separator, so it is skipped. +#[test] +fn rows_render_at_the_stated_menu_row_height() { + let (cx, mut visual, _window, _changes, _activated) = menu_window(true); + + let stated = cx.update(|cx| Theme::global(cx).sizes.medium.menu_item_height); + for index in [0, 2, 3, 4] { + let selector: &'static str = Box::leak(format!("menu-item-{index}").into_boxed_str()); + let row = visual + .debug_bounds(selector) + .unwrap_or_else(|| panic!("row {index} should have been laid out")); + // Painted bounds are snapped to device pixels; allow half a pixel. + assert!( + (row.size.height - stated).abs() <= px(0.5), + "row {index} is {:?} tall, stated menu row height is {stated:?}", + row.size.height + ); + } +} + #[test] fn arrow_navigation_skips_separators_and_disabled_items() { let (_cx, mut visual, _window, changes, activated) = menu_window(true); diff --git a/crates/core/tests/select_pointer.rs b/crates/core/tests/select_pointer.rs index c24c4d9..f0dd044 100644 --- a/crates/core/tests/select_pointer.rs +++ b/crates/core/tests/select_pointer.rs @@ -67,25 +67,45 @@ fn click(visual: &mut VisualTestContext, position: gpui::Point) { visual.simulate_mouse_up(position, MouseButton::Left, gpui::Modifiers::default()); } -/// The trigger band is `16..46`; the panel hangs 4 px below it with a 4 px -/// inner padding, so the three rows ride `~58..90`, `~90..122`, `~122..154`. -/// Every option must answer a pointer press — a popup that clips its last -/// item never does. +/// The three option rows must answer a pointer press wherever they were laid +/// out — a popup that clips its last item never does. Row positions come from +/// the rendered bounds, so the stated menu row metric can change freely. #[test] fn every_option_row_answers_the_pointer() { let (_cx, mut visual, changes) = level_window(); - let expected = [ - (px(74.), Level::Low), - (px(106.), Level::Mid), - (px(138.), Level::High), - ]; - for (y, value) in expected { + // Open once to measure where the rows were actually drawn. + click(&mut visual, point(px(100.), px(31.))); + visual.update(|window, cx| { + window.draw(cx).clear(cx); + }); + let centers: Vec> = (0..3) + .map(|index| { + let selector: &'static str = Box::leak(format!("menu-item-{index}").into_boxed_str()); + let row = visual + .debug_bounds(selector) + .unwrap_or_else(|| panic!("row {index} should have been laid out")); + point( + row.left() + row.size.width / 2.0, + row.top() + row.size.height / 2.0, + ) + }) + .collect(); + + // Close again; each iteration below opens the popup fresh. + click(&mut visual, point(px(100.), px(31.))); + visual.update(|window, cx| { + window.draw(cx).clear(cx); + }); + + let expected = [(Level::Low, 0), (Level::Mid, 1), (Level::High, 2)]; + for (value, index) in expected { + // Choosing the previous row closed the popup; reopen it. click(&mut visual, point(px(100.), px(31.))); visual.update(|window, cx| { window.draw(cx).clear(cx); }); - click(&mut visual, point(px(100.), y)); + click(&mut visual, centers[index]); visual.update(|window, cx| { window.draw(cx).clear(cx); }); diff --git a/docs/CHECKLIST.md b/docs/CHECKLIST.md index 3ef5043..9fa77e1 100644 --- a/docs/CHECKLIST.md +++ b/docs/CHECKLIST.md @@ -14,9 +14,6 @@ is the first downstream consumer; its screens drive the order. Platform metric adoption continues one component at a time; each stop goes through the review gate and a gallery pass. -- [ ] **Menu platform chrome** — overlay border gate; paddings, separators, - and the row min-height through `Theme::spacing` and the stated menu - row height. - [ ] **Popover platform chrome** — overlay border gate; the anchor gap through `Theme::spacing`. - [ ] **Dialog platform chrome** — overlay border gate. @@ -109,6 +106,11 @@ Gaps surfaced by the signed comparison; ordered by signed's usage. macOS / GNOME, so the trigger matches a focused field; the menu popup's border sits behind `Theme::borders.overlays`, disappearing into the surface where the platform paints none +- [x] Menu platform chrome — the popup border behind `Theme::borders.overlays` + (and dark mode); the panel padding and separator margins come from + `Theme::spacing`, and every action row renders at the stated + `menu_item_height` with content centered, so the stated metric is the + whole row; rows carry debug selectors for headless geometry tests - [x] Button, Checkbox, Switch, Radio + RadioGroup, Toggle, Link, Slider, Progress, Avatar (image + seeded pixel-art fallback) - [x] Spinner — painted ring + rotating-icon variant, indeterminate and