button: Show keyboard focus on borderless variants when focus_ring is off

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<T: Styled + Sized> 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,64 +210,75 @@ impl<T: Styled + Sized> ThemeStyled for T {
     }
 }
 
+fn border_widths(style: &StyleRefinement, rem_size: Pixels) -> Edges<Pixels> {
+    let width = |value: Option<gpui::AbsoluteLength>| {
+        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<Pixels> {
+    let radius = |value: Option<gpui::AbsoluteLength>| {
+        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<Pixels>) -> 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<T: Styled + ParentElement>(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<T: Styled + ParentElement>(
     mut element: T,
     window: &Window,
     color: Hsla,
 ) -> T {
     let rem_size = window.rem_size();
-    let style = element.style();
-    let border_widths = Edges::<Pixels> {
-        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::<Pixels> {
-        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(

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,

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<Self>) -> 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<u8>,
+    }
+    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<Pixels>, 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<Pixels>) -> 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)");
     }
 }