From a859704185614d6d1ff6824504d6b848bb4132c3 Mon Sep 17 00:00:00 2001 From: Ren Amamiya Date: Sat, 7 Nov 2026 19:30:04 +0700 Subject: [PATCH] audit select --- crates/core/src/menu.rs | 70 +++++++++++------------- crates/core/src/popover.rs | 66 ++++++++-------------- crates/core/src/select.rs | 32 +++++++++-- crates/core/tests/popover.rs | 103 +++++++++++++++++++++++------------ crates/core/tests/select.rs | 84 +++++++++++++++++++++++++++- crates/theme/src/lib.rs | 16 +++--- crates/theme/src/resolve.rs | 15 ++++- docs/CHECKLIST.md | 14 +++-- 8 files changed, 262 insertions(+), 138 deletions(-) diff --git a/crates/core/src/menu.rs b/crates/core/src/menu.rs index 48fe83e..43fd3ff 100644 --- a/crates/core/src/menu.rs +++ b/crates/core/src/menu.rs @@ -127,6 +127,7 @@ impl MenuItem { struct MenuChrome { surface: gpui::Hsla, border: gpui::Hsla, + panel_border: gpui::Hsla, foreground: gpui::Hsla, muted_foreground: gpui::Hsla, primary: gpui::Hsla, @@ -242,6 +243,11 @@ impl RenderOnce for Menu { MenuChrome { surface: theme.colors.surface, border: theme.colors.border, + panel_border: if theme.borders.overlays { + theme.colors.border + } else { + theme.colors.surface + }, foreground: theme.colors.foreground, muted_foreground: theme.colors.muted_foreground, primary: theme.ramps.primary.base, @@ -259,7 +265,6 @@ impl RenderOnce for Menu { }; let trigger_bounds: Rc>>> = Rc::new(RefCell::new(None)); - let panel_bounds: Rc>>> = Rc::new(RefCell::new(None)); let selection = window.use_keyed_state("menu-selection", cx, |_, _| 0usize); let was_open = window.use_keyed_state("menu-was-open", cx, |_, _| self.open); @@ -363,6 +368,7 @@ impl RenderOnce for Menu { let panel = v_flex() .id("menu-panel") + .occlude() .track_focus(&focus) .role(Role::Menu) .aria_label(label) @@ -373,9 +379,25 @@ impl RenderOnce for Menu { .text_color(chrome.foreground) .rounded(chrome.radius_overlay) .border_1() - .border_color(chrome.border) + .border_color(chrome.panel_border) .shadow(chrome.shadow.clone()) .overflow_hidden() + .on_mouse_down_out({ + let on_change = on_change.clone(); + let trigger_bounds = trigger_bounds.clone(); + + move |event, window, cx| { + if trigger_bounds + .borrow() + .is_some_and(|bounds| bounds.contains(&event.position)) + { + return; + } + if let Some(on_change) = &on_change { + on_change(false, window, cx); + } + } + }) .on_key_down({ let items = items.clone(); let on_change = on_change.clone(); @@ -392,44 +414,14 @@ impl RenderOnce for Menu { .children(rows), ) .child( - canvas( - { - let cell = panel_bounds.clone(); - move |bounds, _window, _cx| *cell.borrow_mut() = Some(bounds) - }, - { - let on_change = on_change.clone(); - move |_, _, window, cx| { - if !focus.contains_focused(window, cx) { - window.focus(&focus, cx); - } - - window.on_mouse_event({ - let on_change = on_change.clone(); - move |event: &MouseDownEvent, phase, window, cx| { - if phase != DispatchPhase::Bubble { - return; - } - - let on_panel = panel_bounds - .borrow() - .is_some_and(|bounds| bounds.contains(&event.position)); - let on_trigger = trigger_bounds - .borrow() - .is_some_and(|bounds| bounds.contains(&event.position)); - if on_panel || on_trigger { - return; - } - - if let Some(on_change) = &on_change { - on_change(false, window, cx); - } - cx.stop_propagation(); - } - }); + canvas(|_, _, _| {}, { + let focus = focus.clone(); + move |_, _, window, cx| { + if !focus.contains_focused(window, cx) { + window.focus(&focus, cx); } - }, - ) + } + }) .absolute() .inset_0(), ); diff --git a/crates/core/src/popover.rs b/crates/core/src/popover.rs index 9e5b95c..97d2be1 100644 --- a/crates/core/src/popover.rs +++ b/crates/core/src/popover.rs @@ -102,7 +102,6 @@ impl RenderOnce for Popover { }; let trigger_bounds: BoundsCell = Rc::new(RefCell::new(None)); - let panel_bounds: BoundsCell = Rc::new(RefCell::new(None)); let fallback_label = self .a11y_label @@ -166,6 +165,7 @@ impl RenderOnce for Popover { let panel = v_flex() .id("popover-panel") + .occlude() .track_focus(&focus) .role(Role::Dialog) .aria_label(label) @@ -178,6 +178,21 @@ impl RenderOnce for Popover { .border_color(colors.border) .shadow(shadows) .overflow_hidden() + .on_mouse_down_out({ + let on_change = on_change.clone(); + let trigger_bounds = trigger_bounds.clone(); + move |event, window, cx| { + if trigger_bounds + .borrow() + .is_some_and(|bounds| bounds.contains(&event.position)) + { + return; + } + if let Some(on_change) = &on_change { + on_change(false, window, cx); + } + } + }) .on_key_down({ let on_change = on_change.clone(); move |event: &KeyDownEvent, window, cx| { @@ -191,49 +206,14 @@ impl RenderOnce for Popover { }) .when_some(content, |panel, content| panel.child(content)) .child( - canvas( - { - let cell = panel_bounds.clone(); - move |bounds, _window, _cx| *cell.borrow_mut() = Some(bounds) - }, - { - let on_change = on_change.clone(); - let focus = focus; - move |_, _, window, cx| { - if !focus.contains_focused(window, cx) { - window.focus(&focus, cx); - } - window.on_mouse_event({ - let on_change = on_change.clone(); - move |event: &MouseDownEvent, phase, window, cx| { - if phase != DispatchPhase::Bubble - || event.button != MouseButton::Left - { - return; - } - - let on_panel = panel_bounds - .borrow() - .is_some_and(|bounds| bounds.contains(&event.position)); - - let on_trigger = trigger_bounds - .borrow() - .is_some_and(|bounds| bounds.contains(&event.position)); - - if on_panel || on_trigger { - return; - } - - if let Some(on_change) = &on_change { - on_change(false, window, cx); - } - - cx.stop_propagation(); - } - }); + canvas(|_, _, _| {}, { + let focus = focus; + move |_, _, window, cx| { + if !focus.contains_focused(window, cx) { + window.focus(&focus, cx); } - }, - ) + } + }) .absolute() .inset_0(), ); diff --git a/crates/core/src/select.rs b/crates/core/src/select.rs index 132bb35..2acea6c 100644 --- a/crates/core/src/select.rs +++ b/crates/core/src/select.rs @@ -6,7 +6,8 @@ use gpui::{ SharedString, StatefulInteractiveElement as _, StyleRefinement, Styled as _, Window, }; use gpui_native_icons::{IconRole, icon}; -use gpui_native_theme::{ActiveTheme as _, Size}; +use gpui_native_theme::{ActiveTheme as _, BORDER_WIDTH, Size}; +use gpui_native_utils::focus_ring; use crate::common::{Sizable, StyledExt, h_flex}; use crate::menu::{Menu, MenuItem}; @@ -169,6 +170,8 @@ where let muted_foreground = theme.colors.muted_foreground; let radius = theme.radius.control; let icon_size = metrics.dense_icon_size; + let fields_bordered = theme.borders.fields; + let focus_ring_on = theme.borders.focus_ring; let label = self .a11y_label @@ -180,7 +183,22 @@ where .use_keyed_state("select-open", cx, |_, _| false) .clone() }); + let is_open = *open.read(cx); + let open_for_items = open.clone(); + + let (rest_border, hover_border) = if fields_bordered { + ( + if is_open && !focus_ring_on { + ring + } else { + border + }, + border, + ) + } else { + (input, muted) + }; let selected_index = self.selected.as_ref().and_then(|value| { self.options @@ -194,6 +212,7 @@ where let trigger_id = self.id.clone(); let trigger_label = label.clone(); let trigger_style = self.style; + let trigger = move |_window: &mut Window, _cx: &mut App| { let text = trigger_selected .as_ref() @@ -207,17 +226,18 @@ where h_flex() .id((trigger_id.clone(), "select-trigger")) + .debug_selector(|| "select-trigger".to_string()) .role(Role::ComboBox) .aria_label(trigger_label.clone()) .cursor_pointer() .justify_between() .gap_2() .w_full() - .h(metrics.input_height) + .h(metrics.input_height + BORDER_WIDTH * 2.0) .px(metrics.dense_padding_x) .rounded(radius) .border_1() - .border_color(if is_open { ring } else { border }) + .border_color(rest_border) .bg(input) .text_size(metrics.text_size) .text_color(if trigger_selected.is_some() { @@ -225,7 +245,10 @@ where } else { muted_foreground }) - .hover(|style| style.bg(muted)) + .when(is_open && focus_ring_on, |trigger| { + trigger.shadow(focus_ring(ring)) + }) + .hover(|style| style.bg(muted).border_color(hover_border)) .child(text) .child( icon(IconRole::ChevronDown) @@ -236,7 +259,6 @@ where .into_any_element() }; - let open_for_items = open.clone(); let items = move |_window: &mut Window, _cx: &mut App| { self.options .iter() diff --git a/crates/core/tests/popover.rs b/crates/core/tests/popover.rs index ec91f26..adaf33a 100644 --- a/crates/core/tests/popover.rs +++ b/crates/core/tests/popover.rs @@ -1,8 +1,9 @@ use std::{cell::RefCell, rc::Rc}; use gpui::{ - Context, IntoElement, Modifiers, MouseButton, ParentElement as _, Render, Styled as _, - TestAppContext, VisualTestContext, Window, WindowHandle, div, point, px, rgba, + Context, InteractiveElement as _, IntoElement, Modifiers, MouseButton, ParentElement as _, + Render, StatefulInteractiveElement as _, Styled as _, TestAppContext, VisualTestContext, + Window, WindowHandle, div, point, px, rgba, }; use gpui_native_core::popover::Popover; use gpui_native_theme::Theme; @@ -10,37 +11,53 @@ use gpui_native_theme::Theme; struct Harness { open: bool, changes: Rc>>, + target_clicks: Rc>, } impl Render for Harness { fn render(&mut self, _window: &mut Window, cx: &mut Context) -> impl IntoElement { let changes = self.changes.clone(); + let target_clicks = self.target_clicks.clone(); let view = cx.entity(); - div().size_full().child( - Popover::new("test-popover") - .open(self.open) - .on_change(move |next, _, cx| { - changes.borrow_mut().push(next); - view.update(cx, |harness, cx| { - harness.open = next; - cx.notify(); - }); - }) - .trigger(|_, _| { + div() + .size_full() + .child( + div().absolute().left(px(400.)).top(px(300.)).child( div() - .w(px(100.)) - .h(px(32.)) + .id("popover-target") + .size(px(40.)) .bg(rgba(0x000000ff)) - .into_any_element() - }) - .content(|_, _| { - div() - .w(px(200.)) - .h(px(80.)) - .child("Popover content") - .into_any_element() - }), - ) + .on_click(move |_, _, cx| { + *target_clicks.borrow_mut() += 1; + cx.stop_propagation(); + }), + ), + ) + .child( + Popover::new("test-popover") + .open(self.open) + .on_change(move |next, _, cx| { + changes.borrow_mut().push(next); + view.update(cx, |harness, cx| { + harness.open = next; + cx.notify(); + }); + }) + .trigger(|_, _| { + div() + .w(px(100.)) + .h(px(32.)) + .bg(rgba(0x000000ff)) + .into_any_element() + }) + .content(|_, _| { + div() + .w(px(200.)) + .h(px(80.)) + .child("Popover content") + .into_any_element() + }), + ) } } @@ -48,27 +65,31 @@ impl Render for Harness { /// hangs below it around `(0..200, 36..116)`, so clicks at `(10, 16)` land /// on the trigger, `(100, 70)` on the panel, and anything past `(300, 300)` /// is outside both. -fn popover_window( - open: bool, -) -> ( +type PopoverWindow = ( TestAppContext, VisualTestContext, WindowHandle, Rc>>, -) { + Rc>, +); + +fn popover_window(open: bool) -> PopoverWindow { let mut cx = TestAppContext::single(); cx.update(Theme::init); let changes = Rc::new(RefCell::new(Vec::new())); let harness_changes = changes.clone(); + let target_clicks = Rc::new(RefCell::new(0)); + let harness_clicks = target_clicks.clone(); let window = cx.add_window(move |_, _| Harness { open, changes: harness_changes, + target_clicks: harness_clicks, }); let mut visual = VisualTestContext::from_window(window.into(), &cx); visual.update(|window, cx| { window.draw(cx).clear(cx); }); - (cx, visual, window, changes) + (cx, visual, window, changes, target_clicks) } fn draw(visual: &mut VisualTestContext) { @@ -84,7 +105,7 @@ fn click(visual: &mut VisualTestContext, position: gpui::Point) { #[test] fn clicking_outside_dismisses_and_reports_closed() { - let (_cx, mut visual, _window, changes) = popover_window(true); + let (_cx, mut visual, _window, changes, _clicks) = popover_window(true); click(&mut visual, point(px(600.), px(600.))); assert_eq!(*changes.borrow(), vec![false]); @@ -98,7 +119,7 @@ fn clicking_outside_dismisses_and_reports_closed() { #[test] fn clicking_the_trigger_toggles_and_clicking_the_panel_does_nothing() { - let (_cx, mut visual, _window, changes) = popover_window(true); + let (_cx, mut visual, _window, changes, _clicks) = popover_window(true); // On the open panel's trigger: toggles closed. click(&mut visual, point(px(10.), px(16.))); @@ -111,7 +132,7 @@ fn clicking_the_trigger_toggles_and_clicking_the_panel_does_nothing() { #[test] fn clicking_the_trigger_opens_a_closed_popover() { - let (_cx, mut visual, _window, changes) = popover_window(false); + let (_cx, mut visual, _window, changes, _clicks) = popover_window(false); click(&mut visual, point(px(10.), px(16.))); assert_eq!(*changes.borrow(), vec![true]); @@ -119,7 +140,7 @@ fn clicking_the_trigger_opens_a_closed_popover() { #[test] fn escape_dismisses_while_the_panel_holds_focus() { - let (_cx, mut visual, _window, changes) = popover_window(true); + let (_cx, mut visual, _window, changes, _clicks) = popover_window(true); draw(&mut visual); // The panel claims focus on open, so escape reaches its key listener. @@ -134,7 +155,7 @@ fn escape_dismisses_while_the_panel_holds_focus() { #[test] fn the_controlled_loop_reopens_from_the_app_side() { - let (mut cx, mut visual, window, changes) = popover_window(true); + let (mut cx, mut visual, window, changes, _clicks) = popover_window(true); // Dismiss once. click(&mut visual, point(px(600.), px(600.))); @@ -155,3 +176,15 @@ fn the_controlled_loop_reopens_from_the_app_side() { visual.simulate_keystrokes("escape"); assert_eq!(*changes.borrow(), vec![false, false]); } + +/// Clicking another control while the popover is open dismisses it and still +/// acts on that control: dismissing does not claim the event. +#[test] +fn clicking_another_control_dismisses_and_reaches_it() { + let (_cx, mut visual, _window, changes, clicks) = popover_window(true); + + click(&mut visual, point(px(420.), px(320.))); + + assert_eq!(*changes.borrow(), vec![false]); + assert_eq!(*clicks.borrow(), 1); +} diff --git a/crates/core/tests/select.rs b/crates/core/tests/select.rs index 5f7f77b..8926deb 100644 --- a/crates/core/tests/select.rs +++ b/crates/core/tests/select.rs @@ -1,11 +1,12 @@ use std::{cell::RefCell, rc::Rc}; use gpui::{ - Context, IntoElement, MouseButton, ParentElement as _, Render, Styled as _, TestAppContext, - VisualTestContext, Window, div, point, px, + Context, Entity, IntoElement, MouseButton, ParentElement as _, Render, Styled as _, + TestAppContext, VisualTestContext, Window, div, point, px, }; +use gpui_native_core::input::{Input, InputState}; use gpui_native_core::select::Select; -use gpui_native_theme::Theme; +use gpui_native_theme::{ActiveTheme as _, BORDER_WIDTH, Theme}; #[derive(Clone, Copy, Debug, PartialEq, Eq)] enum Strategy { @@ -200,3 +201,80 @@ fn two_selects_keep_their_popups_independent() { assert_eq!(*first_changes.borrow(), vec![]); assert_eq!(*second_changes.borrow(), vec![Some(Strategy::Curated)]); } + +#[test] +fn a_click_outside_closes_the_popup() { + let (_cx, mut visual, changes) = select_window(); + + click(&mut visual, point(px(100.), px(31.))); + draw(&mut visual); + click(&mut visual, point(px(100.), px(300.))); + draw(&mut visual); + + // The popup held keyboard focus while open; a keystroke after the outside + // click answers nothing if the popup really closed. + visual.simulate_keystrokes("enter"); + draw(&mut visual); + assert_eq!(*changes.borrow(), Vec::>::new()); +} + +/// Clicking another control while the popup is open dismisses it and still +/// acts on the control: dismissing does not claim the event. +#[test] +fn a_click_on_another_control_dismisses_and_reaches_it() { + let (_cx, mut visual, first_changes, second_changes) = pair_window(); + + click(&mut visual, point(px(100.), px(31.))); + draw(&mut visual); + + // The second select's trigger, below the first — at x=300, past the first + // popup's 182px-wide panel: this click must close the first popup AND open + // the second. + click(&mut visual, point(px(300.), px(69.))); + draw(&mut visual); + visual.simulate_keystrokes("enter"); + draw(&mut visual); + + assert_eq!(*first_changes.borrow(), Vec::>::new()); + assert_eq!(*second_changes.borrow(), vec![Some(Strategy::Curated)]); +} + +struct HeightsHarness { + state: Entity, +} + +impl Render for HeightsHarness { + fn render(&mut self, _window: &mut Window, _cx: &mut Context) -> impl IntoElement { + div() + .flex() + .flex_col() + .gap_4() + .child(Select::new("heights").options(STRATEGIES)) + .child(Input::new("heights-field", self.state.clone())) + } +} + +/// The trigger renders at the same height as the input field: the stated +/// metric plus the always-painted hairline border, whatever the platform. +#[test] +fn the_trigger_matches_the_input_height() { + let mut cx = TestAppContext::single(); + cx.update(Theme::init); + let state = cx.update(InputState::new); + let state_for_window = state.clone(); + let window = cx.add_window(move |_, _| HeightsHarness { + state: state_for_window, + }); + let mut visual = VisualTestContext::from_window(window.into(), &cx); + draw(&mut visual); + + let trigger = visual + .debug_bounds("select-trigger") + .expect("trigger rendered"); + let field = visual.debug_bounds("input-field").expect("input rendered"); + + // Snapped bounds: allow half a pixel. + assert!((trigger.size.height - field.size.height).abs() <= px(0.5)); + let expected = cx.update(|cx| cx.theme().sizes.medium.input_height) + BORDER_WIDTH * 2.0; + assert!((trigger.size.height - expected).abs() <= px(0.5)); +} diff --git a/crates/theme/src/lib.rs b/crates/theme/src/lib.rs index 63ab0e3..26f8eb9 100644 --- a/crates/theme/src/lib.rs +++ b/crates/theme/src/lib.rs @@ -101,18 +101,18 @@ impl Default for Radius { pub struct Borders { /// Filled actions keep a border (Windows, KDE). pub buttons: bool, - /// Text fields and selects keep a border (Windows, KDE; the macOS - /// hairline bezel). + /// Text fields and selects keep a border. pub fields: bool, - /// Floating layers keep a border (Windows, KDE). + /// Floating layers keep a border. pub overlays: bool, - /// Keyboard focus paints an accent ring around the control (macOS, - /// GNOME); otherwise focus re-colors the border (Windows, KDE). + /// Keyboard focus paints an accent ring around the control. pub focus_ring: bool, } impl Borders { - pub(crate) fn for_platform(source: SourceKind) -> Self { + pub(crate) fn for_platform(source: SourceKind, variant: ThemeVariant) -> Self { + let overlays = variant == ThemeVariant::Dark; + match source { SourceKind::Windows | SourceKind::Kde => Self { buttons: true, @@ -123,13 +123,13 @@ impl Borders { SourceKind::Macos => Self { buttons: false, fields: true, - overlays: false, + overlays, focus_ring: true, }, SourceKind::Omarchy | SourceKind::Gnome => Self { buttons: false, fields: false, - overlays: false, + overlays, focus_ring: true, }, SourceKind::Fallback => Self { diff --git a/crates/theme/src/resolve.rs b/crates/theme/src/resolve.rs index 389b2a7..035d5b2 100644 --- a/crates/theme/src/resolve.rs +++ b/crates/theme/src/resolve.rs @@ -275,7 +275,7 @@ pub(crate) fn resolve( }, colors, radius, - borders: Borders::for_platform(system.source), + borders: Borders::for_platform(system.source, variant), spacing: Spacing { unit: system.source.spacing_unit(), }, @@ -535,6 +535,19 @@ mod tests { assert_eq!(unit, px(4.)); } + #[test] + fn dark_mode_separates_overlays_by_border() { + for source in [SourceKind::Macos, SourceKind::Gnome, SourceKind::Omarchy] { + let mut stated = system(ThemeVariant::Dark, PartialPalette::default(), None); + stated.source = source; + let resolved = resolve(&stated, None, None); + assert!( + resolved.borders.overlays, + "{source:?} frames floating layers in dark" + ); + } + } + #[test] fn overlay_is_translucent_and_overridable() { let light = resolve( diff --git a/docs/CHECKLIST.md b/docs/CHECKLIST.md index bfa2743..3ef5043 100644 --- a/docs/CHECKLIST.md +++ b/docs/CHECKLIST.md @@ -9,9 +9,6 @@ is the first downstream consumer; its screens drive the order. ## Next -- [ ] **Select platform chrome** — the input treatment on the trigger; the - popup border behind `Theme::borders.overlays`. - ## Queue — to follow Platform metric adoption continues one component at a time; each stop goes @@ -74,7 +71,9 @@ Gaps surfaced by the signed comparison; ordered by signed's usage. metrics and the full radius ladder per platform, Linux root fonts (GNOME gsettings `font-name`, KDE `kdeglobals font=`), per-source border + focus chrome (`Theme::borders`), and a per-platform spacing - unit (`Theme::spacing`). Components adopt the tokens one by one + unit (`Theme::spacing`); overlay borders engage in dark mode, where + the elevation shadow barely reads and floating layers separate by + border instead. Components adopt the tokens one by one (see Next and Queue) ### icons (`crates/icons`) @@ -103,6 +102,13 @@ Gaps surfaced by the signed comparison; ordered by signed's usage. typically small ghost buttons; a docked click activates the control without reaching the field's caret handler, and the text region shrinks to make room +- [x] Select platform chrome — the trigger takes the input treatment: the + hairline border always painted, sized outside the stated field height + and taking the fill's color where the platform paints none; open + recolors the border on Windows / KDE and paints the accent ring on + 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] Button, Checkbox, Switch, Radio + RadioGroup, Toggle, Link, Slider, Progress, Avatar (image + seeded pixel-art fallback) - [x] Spinner — painted ring + rotating-icon variant, indeterminate and