From 7aa8b1b3d23386c5af00c94bd02803fdb394406c Mon Sep 17 00:00:00 2001 From: Floyd Wang Date: Mon, 28 Sep 2026 21:55:25 +0800 Subject: [PATCH] button: Show keyboard focus on borderless variants when `focus_ring` is off With `Theme::focus_ring` off, `focus_ring_style` only tinted the border, so Ghost / Text / Link and filled buttons without `.outline()` showed no focus at all. Elements without a border now get a 1px ring inside their edge instead: no layout change, and nothing an ancestor can clip. Closes #3298 Co-Authored-By: Claude Opus 5.5 --- crates/component/src/styled.rs | 130 ++++++++++++---------- crates/component/src/theme/mod.rs | 3 +- crates/kit/tests/rendering.rs | 177 +++++++++++++++++++++++++++++- 3 files changed, 250 insertions(+), 60 deletions(-) diff --git a/crates/component/src/styled.rs b/crates/component/src/styled.rs index cf4577ac3f..ce6162228b 100644 --- a/crates/component/src/styled.rs +++ b/crates/component/src/styled.rs @@ -130,7 +130,9 @@ pub trait ThemeStyled: Styled + Sized { /// /// The ring is dropped when [`crate::Theme::focus_ring`] is off, leaving /// the tinted border — an application whose layout clips its containers can - /// turn it off rather than finding room for the ring in each of them. + /// turn it off rather than finding room for the ring in each of them. An + /// element without a border draws a 1px ring just inside its edge instead, + /// so borderless controls still show focus. /// /// Calling this turns the ring on; gate it with `when` for the conditions /// that decide whether the control shows one at all — its focus state, @@ -172,15 +174,22 @@ impl ThemeStyled for T { /// The ring sits outside the element's border, so an ancestor that clips /// its content will cut it off — leave it a few pixels of room, or don't /// clip. - fn focus_ring_style(self, window: &Window, cx: &App) -> Self + fn focus_ring_style(mut self, window: &Window, cx: &App) -> Self where Self: ParentElement, { // The ring is painted outside the border, so a clipping ancestor cuts // it off. An application whose layout clips heavily turns it off in the - // theme and keeps the tinted border, which takes no space. + // theme and keeps the tinted border, which takes no space. Without a + // border to tint, draw the same 1px line inside the edge. if !cx.theme().focus_ring { - return self.border_color(cx.theme().ring); + let rem_size = window.rem_size(); + let has_border = + border_widths(self.style(), rem_size).any(|width| *width > Pixels::ZERO); + if has_border { + return self.border_color(cx.theme().ring); + } + return inset_focus_ring(self, window, cx.theme().ring); } focus_ring( @@ -201,6 +210,65 @@ impl ThemeStyled for T { } } +fn border_widths(style: &StyleRefinement, rem_size: Pixels) -> Edges { + let width = |value: Option| { + value.map(|v| v.to_pixels(rem_size)).unwrap_or_default() + }; + let widths = &style.border_widths; + Edges { + top: width(widths.top), + bottom: width(widths.bottom), + left: width(widths.left), + right: width(widths.right), + } +} + +fn corner_radii(style: &StyleRefinement, rem_size: Pixels) -> Corners { + let radius = |value: Option| { + value.map(|v| v.to_pixels(rem_size)).unwrap_or_default() + }; + let radii = &style.corner_radii; + Corners { + top_left: radius(radii.top_left), + top_right: radius(radii.top_right), + bottom_left: radius(radii.bottom_left), + bottom_right: radius(radii.bottom_right), + } +} + +fn corner_radii_refinement(radius: Corners) -> StyleRefinement { + let mut style = StyleRefinement::default(); + style.corner_radii.top_left = Some(radius.top_left.into()); + style.corner_radii.top_right = Some(radius.top_right.into()); + style.corner_radii.bottom_left = Some(radius.bottom_left.into()); + style.corner_radii.bottom_right = Some(radius.bottom_right.into()); + style +} + +/// Draw a 1px ring just inside a borderless element's edge. +/// +/// It sits within the element's bounds, so no clipping ancestor can cut it, +/// and it is an absolute child, so it changes no layout. With no border, the +/// element's own radius is already concentric with its edge. +fn inset_focus_ring(mut element: T, window: &Window, color: Hsla) -> T { + let radius = corner_radii(element.style(), window.rem_size()); + element.child( + div() + .when(cfg!(test), |this| { + this.debug_selector(|| "focus-ring".into()) + }) + .flex_none() + .absolute() + .top_0() + .left_0() + .right_0() + .bottom_0() + .border_1() + .border_color(color) + .refine_style(&corner_radii_refinement(radius)), + ) +} + /// Paint only the outside band, preserving translucent control backgrounds. pub(crate) fn focus_ring( mut element: T, @@ -208,57 +276,9 @@ pub(crate) fn focus_ring( color: Hsla, ) -> T { let rem_size = window.rem_size(); - let style = element.style(); - let border_widths = Edges:: { - top: style - .border_widths - .top - .map(|v| v.to_pixels(rem_size)) - .unwrap_or_default(), - bottom: style - .border_widths - .bottom - .map(|v| v.to_pixels(rem_size)) - .unwrap_or_default(), - left: style - .border_widths - .left - .map(|v| v.to_pixels(rem_size)) - .unwrap_or_default(), - right: style - .border_widths - .right - .map(|v| v.to_pixels(rem_size)) - .unwrap_or_default(), - }; - let radius = Corners:: { - top_left: style - .corner_radii - .top_left - .map(|v| v.to_pixels(rem_size)) - .unwrap_or_default(), - top_right: style - .corner_radii - .top_right - .map(|v| v.to_pixels(rem_size)) - .unwrap_or_default(), - bottom_left: style - .corner_radii - .bottom_left - .map(|v| v.to_pixels(rem_size)) - .unwrap_or_default(), - bottom_right: style - .corner_radii - .bottom_right - .map(|v| v.to_pixels(rem_size)) - .unwrap_or_default(), - } - .map(|value| *value + FOCUS_RING_WIDTH); - let mut ring_style = StyleRefinement::default(); - ring_style.corner_radii.top_left = Some(radius.top_left.into()); - ring_style.corner_radii.top_right = Some(radius.top_right.into()); - ring_style.corner_radii.bottom_left = Some(radius.bottom_left.into()); - ring_style.corner_radii.bottom_right = Some(radius.bottom_right.into()); + let border_widths = border_widths(element.style(), rem_size); + let radius = corner_radii(element.style(), rem_size).map(|value| *value + FOCUS_RING_WIDTH); + let ring_style = corner_radii_refinement(radius); let inset = FOCUS_RING_WIDTH; element.child( diff --git a/crates/component/src/theme/mod.rs b/crates/component/src/theme/mod.rs index 83a53cbc53..6f4660a5ec 100644 --- a/crates/component/src/theme/mod.rs +++ b/crates/component/src/theme/mod.rs @@ -172,7 +172,8 @@ pub struct Theme { /// The ring is painted outside the element, so any ancestor that clips its /// content will cut it off. An application whose layout clips heavily can /// turn it off here: focused controls then show only their tinted border, - /// which costs no space and cannot be clipped. + /// or a 1px ring just inside their edge when they have none. Neither costs + /// space or can be clipped. #[serde(default = "default_true")] pub focus_ring: bool, pub transparent: Hsla, diff --git a/crates/kit/tests/rendering.rs b/crates/kit/tests/rendering.rs index a2ace672b4..ec13934408 100644 --- a/crates/kit/tests/rendering.rs +++ b/crates/kit/tests/rendering.rs @@ -10,11 +10,12 @@ fn main() { #[cfg(target_os = "macos")] mod macos { use gpui_kit::{ - AppContext, AssetSource, Context, Entity, HeadlessAppContext, Render, Result, SharedString, - Window, + AppContext, AssetSource, Bounds, Context, Entity, HeadlessAppContext, Pixels, Render, + Result, Rgba, SharedString, Window, assets::Assets, component::{ - ActiveTheme, Theme, + ActiveTheme, IconName, Theme, + button::{Button, ButtonVariants as _}, checkbox::Checkbox, input::{Input, InputState}, text::TextView, @@ -245,6 +246,168 @@ mod macos { } } + const BUTTON_IDS: [&str; 4] = ["ghost", "text", "link", "primary"]; + + struct Buttons; + impl Render for Buttons { + fn render(&mut self, _: &mut Window, cx: &mut Context) -> impl IntoElement { + // Lowercase labels without ascenders keep glyph ink away from the + // top edge, where the ring is sampled. + div() + .size_full() + .bg(cx.theme().background) + .text_color(cx.theme().foreground) + .p_4() + .flex() + .items_center() + .gap_4() + .child(Button::new("ghost").ghost().icon(IconName::Copy)) + .child(Button::new("text").text().label("ocean")) + .child(Button::new("link").link().label("ocean")) + .child(Button::new("primary").primary().label("ocean")) + } + } + + const BUTTONS_SIZE: (f32, f32) = (320., 64.); + + fn buttons_window(cx: &mut HeadlessAppContext) -> gpui_kit::AnyWindowHandle { + cx.update(|cx| Theme::update(cx, |theme| theme.focus_ring = false)); + let (handle, _) = cx + .update(|cx| { + gpui_kit::open_window( + gpui_kit::WindowOptions { + window_bounds: Some(gpui_kit::WindowBounds::Windowed(gpui_kit::Bounds { + origin: Default::default(), + size: size(px(BUTTONS_SIZE.0), px(BUTTONS_SIZE.1)), + })), + focus: false, + show: false, + ..Default::default() + }, + cx, + |_, cx| cx.new(|_| Buttons), + ) + }) + .unwrap(); + cx.update_window(handle, |_, window, cx| window.render_frame(cx)) + .unwrap(); + handle + } + + /// A window capture as raw RGBA rows. + struct Capture { + width: u32, + height: u32, + raw: Vec, + } + impl Capture { + fn take(cx: &mut HeadlessAppContext, handle: gpui_kit::AnyWindowHandle) -> Self { + let image = cx.capture_screenshot(handle).unwrap(); + let (width, height) = image.dimensions(); + Self { + width, + height, + raw: image.into_raw(), + } + } + fn scale(&self) -> f32 { + self.width as f32 / BUTTONS_SIZE.0 + } + fn get_pixel(&self, x: u32, y: u32) -> &[u8] { + let start = ((y * self.width + x) * 4) as usize; + &self.raw[start..start + 4] + } + } + + /// Ring-coloured pixels along the top edge of `bounds`, away from the corners. + fn ring_pixels(image: &Capture, bounds: Bounds, ring: Rgba) -> usize { + let scale = image.scale(); + let device = |value: Pixels| (f32::from(value) * scale).round() as u32; + let ring = [ring.r, ring.g, ring.b].map(|channel| (channel * 255.).round() as i32); + let (left, right) = (device(bounds.left()), device(bounds.right())); + let top = device(bounds.top()); + let quarter = (right - left) / 4; + ((left + quarter)..(right - quarter)) + .flat_map(|x| (top..top + 2).map(move |y| (x, y))) + .filter(|&(x, y)| { + let pixel = image.get_pixel(x, y); + (0..3).all(|i| (pixel[i] as i32 - ring[i]).abs() <= 8) + }) + .count() + } + + /// Device pixels that differ between two captures outside `bounds`. + fn changes_outside(before: &Capture, after: &Capture, bounds: Bounds) -> usize { + let scale = before.scale(); + let inside = |x: u32, y: u32| { + let point = gpui_kit::point(px(x as f32 / scale), px(y as f32 / scale)); + bounds.contains(&point) + }; + (0..before.height) + .flat_map(|y| (0..before.width).map(move |x| (x, y))) + .filter(|&(x, y)| !inside(x, y) && after.get_pixel(x, y) != before.get_pixel(x, y)) + .count() + } + + fn borderless_buttons_show_keyboard_focus_inside_when_focus_ring_is_off() { + let mut cx = context(Arc::new(Assets)); + let handle = buttons_window(&mut cx); + let ring = cx.update(|cx| Rgba::from(cx.theme().ring)); + let idle = Capture::take(&mut cx, handle); + + for id in BUTTON_IDS { + let bounds = cx + .update_window(handle, |_, window, cx| { + // Tab only reaches Root's binding once something inside + // it holds focus, so the first stop is what Tab does. + if window.focused(cx).is_none() { + window.focus_next(cx); + } else { + window.press("tab", cx); + } + window.render_frame(cx); + window.find(id).bounds() + }) + .unwrap(); + let focused = Capture::take(&mut cx, handle); + assert!( + ring_pixels(&idle, bounds, ring) == 0, + "an unfocused `{id}` button must draw no ring" + ); + assert!( + ring_pixels(&focused, bounds, ring) > 0, + "a keyboard-focused `{id}` button must draw a ring inside its bounds" + ); + let outside = changes_outside(&idle, &focused, bounds); + assert_eq!( + outside, 0, + "focusing `{id}` changed {outside} device pixels outside its bounds" + ); + } + } + + fn clicking_a_button_draws_no_focus_ring() { + let mut cx = context(Arc::new(Assets)); + let handle = buttons_window(&mut cx); + let ring = cx.update(|cx| Rgba::from(cx.theme().ring)); + for id in BUTTON_IDS { + let bounds = cx + .update_window(handle, |_, window, cx| { + window.click(id, cx); + window.render_frame(cx); + assert!(window.focused(cx).is_none(), "clicking `{id}` took focus"); + window.find(id).bounds() + }) + .unwrap(); + let clicked = Capture::take(&mut cx, handle); + assert_eq!( + ring_pixels(&clicked, bounds, ring), + 0, + "a clicked `{id}` button must draw no ring" + ); + } + } + pub fn run() { println!("running pixels_detect_missing_check_even_when_checked_state_is_correct"); pixels_detect_missing_check_even_when_checked_state_is_correct(); @@ -255,6 +418,12 @@ mod macos { println!("running wrapped_cjk_with_inline_code_stays_within_the_wrap_width"); wrapped_cjk_with_inline_code_stays_within_the_wrap_width(); println!("passed wrapped_cjk_with_inline_code_stays_within_the_wrap_width"); - println!("rendering: 3 passed (Metal)"); + println!("running borderless_buttons_show_keyboard_focus_inside_when_focus_ring_is_off"); + borderless_buttons_show_keyboard_focus_inside_when_focus_ring_is_off(); + println!("passed borderless_buttons_show_keyboard_focus_inside_when_focus_ring_is_off"); + println!("running clicking_a_button_draws_no_focus_ring"); + clicking_a_button_draws_no_focus_ring(); + println!("passed clicking_a_button_draws_no_focus_ring"); + println!("rendering: 5 passed (Metal)"); } }