gpui: Ensure first tab index is selected on first focus (#35247)

Finn Evers and Jason Lee created 3 months ago

This fixes an issue with tab indices where we would actually focus the
second focus handle on first focus instead of the first one. The test
was updated accordingly.

Release Notes:

- N/A

---------

Co-authored-by: Jason Lee <[e-mail]>

Change summary

crates/gpui/examples/tab_stop.rs | 15 ++++++++++++---
crates/gpui/src/tab_stop.rs      | 32 ++++++++++++++++++--------------
2 files changed, 30 insertions(+), 17 deletions(-)

Detailed changes

crates/gpui/examples/tab_stop.rs

@@ -6,6 +6,7 @@ use gpui::{
 actions!(example, [Tab, TabPrev]);
 
 struct Example {
+    focus_handle: FocusHandle,
     items: Vec<FocusHandle>,
     message: SharedString,
 }
@@ -20,8 +21,11 @@ impl Example {
             cx.focus_handle().tab_index(2).tab_stop(true),
         ];
 
-        window.focus(items.first().unwrap());
+        let focus_handle = cx.focus_handle();
+        window.focus(&focus_handle);
+
         Self {
+            focus_handle,
             items,
             message: SharedString::from("Press `Tab`, `Shift-Tab` to switch focus."),
         }
@@ -40,6 +44,10 @@ impl Example {
 
 impl Render for Example {
     fn render(&mut self, window: &mut Window, cx: &mut Context<Self>) -> impl IntoElement {
+        fn tab_stop_style<T: Styled>(this: T) -> T {
+            this.border_3().border_color(gpui::blue())
+        }
+
         fn button(id: impl Into<ElementId>) -> Stateful<Div> {
             div()
                 .id(id)
@@ -52,12 +60,13 @@ impl Render for Example {
                 .border_color(gpui::black())
                 .bg(gpui::black())
                 .text_color(gpui::white())
-                .focus(|this| this.border_color(gpui::blue()))
+                .focus(tab_stop_style)
                 .shadow_sm()
         }
 
         div()
             .id("app")
+            .track_focus(&self.focus_handle)
             .on_action(cx.listener(Self::on_tab))
             .on_action(cx.listener(Self::on_tab_prev))
             .size_full()
@@ -86,7 +95,7 @@ impl Render for Example {
                             .border_color(gpui::black())
                             .when(
                                 item_handle.tab_stop && item_handle.is_focused(window),
-                                |this| this.border_color(gpui::blue()),
+                                tab_stop_style,
                             )
                             .map(|this| match item_handle.tab_stop {
                                 true => this

crates/gpui/src/tab_stop.rs

@@ -32,20 +32,18 @@ impl TabHandles {
         self.handles.clear();
     }
 
-    fn current_index(&self, focused_id: Option<&FocusId>) -> usize {
-        self.handles
-            .iter()
-            .position(|h| Some(&h.id) == focused_id)
-            .unwrap_or_default()
+    fn current_index(&self, focused_id: Option<&FocusId>) -> Option<usize> {
+        self.handles.iter().position(|h| Some(&h.id) == focused_id)
     }
 
     pub(crate) fn next(&self, focused_id: Option<&FocusId>) -> Option<FocusHandle> {
-        let ix = self.current_index(focused_id);
-
-        let mut next_ix = ix + 1;
-        if next_ix + 1 > self.handles.len() {
-            next_ix = 0;
-        }
+        let next_ix = self
+            .current_index(focused_id)
+            .and_then(|ix| {
+                let next_ix = ix + 1;
+                (next_ix < self.handles.len()).then_some(next_ix)
+            })
+            .unwrap_or_default();
 
         if let Some(next_handle) = self.handles.get(next_ix) {
             Some(next_handle.clone())
@@ -55,7 +53,7 @@ impl TabHandles {
     }
 
     pub(crate) fn prev(&self, focused_id: Option<&FocusId>) -> Option<FocusHandle> {
-        let ix = self.current_index(focused_id);
+        let ix = self.current_index(focused_id).unwrap_or_default();
         let prev_ix;
         if ix == 0 {
             prev_ix = self.handles.len().saturating_sub(1);
@@ -108,8 +106,14 @@ mod tests {
             ]
         );
 
-        // next
-        assert_eq!(tab.next(None), Some(tab.handles[1].clone()));
+        // Select first tab index if no handle is currently focused.
+        assert_eq!(tab.next(None), Some(tab.handles[0].clone()));
+        // Select last tab index if no handle is currently focused.
+        assert_eq!(
+            tab.prev(None),
+            Some(tab.handles[tab.handles.len() - 1].clone())
+        );
+
         assert_eq!(
             tab.next(Some(&tab.handles[0].id)),
             Some(tab.handles[1].clone())