settings_ui: Make arrow keys up and down activate page content (#40106)

Danilo Leal created

Release Notes:

- settings ui: Navigating the settings navbar with arrow keys up and
down now also activates the page, allowing users to more quickly see the
content for a given page before moving focus to the page itself.

Change summary

assets/keymaps/default-linux.json     |   3 
assets/keymaps/default-macos.json     |   3 
assets/keymaps/default-windows.json   |   3 
crates/settings_ui/src/settings_ui.rs | 106 +++++++++++++++++++++++++---
4 files changed, 104 insertions(+), 11 deletions(-)

Detailed changes

assets/keymaps/default-linux.json 🔗

@@ -1249,6 +1249,7 @@
       "escape": "workspace::CloseWindow",
       "ctrl-m": "settings_editor::Minimize",
       "ctrl-f": "search::FocusSearch",
+      "left": "settings_editor::ToggleFocusNav",
       "ctrl-shift-e": "settings_editor::ToggleFocusNav",
       // todo(settings_ui): cut this down based on the max files and overflow UI
       "ctrl-1": ["settings_editor::FocusFile", 0],
@@ -1269,6 +1270,8 @@
     "context": "SettingsWindow > NavigationMenu",
     "use_key_equivalents": true,
     "bindings": {
+      "up": "settings_editor::FocusPreviousNavEntry",
+      "down": "settings_editor::FocusNextNavEntry",
       "right": "settings_editor::ExpandNavEntry",
       "left": "settings_editor::CollapseNavEntry",
       "pageup": "settings_editor::FocusPreviousRootNavEntry",

assets/keymaps/default-macos.json 🔗

@@ -1354,6 +1354,7 @@
       "escape": "workspace::CloseWindow",
       "cmd-m": "settings_editor::Minimize",
       "cmd-f": "search::FocusSearch",
+      "left": "settings_editor::ToggleFocusNav",
       "cmd-shift-e": "settings_editor::ToggleFocusNav",
       // todo(settings_ui): cut this down based on the max files and overflow UI
       "ctrl-1": ["settings_editor::FocusFile", 0],
@@ -1374,6 +1375,8 @@
     "context": "SettingsWindow > NavigationMenu",
     "use_key_equivalents": true,
     "bindings": {
+      "up": "settings_editor::FocusPreviousNavEntry",
+      "down": "settings_editor::FocusNextNavEntry",
       "right": "settings_editor::ExpandNavEntry",
       "left": "settings_editor::CollapseNavEntry",
       "pageup": "settings_editor::FocusPreviousRootNavEntry",

assets/keymaps/default-windows.json 🔗

@@ -1270,6 +1270,7 @@
       "escape": "workspace::CloseWindow",
       "ctrl-m": "settings_editor::Minimize",
       "ctrl-f": "search::FocusSearch",
+      "left": "settings_editor::ToggleFocusNav",
       "ctrl-shift-e": "settings_editor::ToggleFocusNav",
       // todo(settings_ui): cut this down based on the max files and overflow UI
       "ctrl-1": ["settings_editor::FocusFile", 0],
@@ -1290,6 +1291,8 @@
     "context": "SettingsWindow > NavigationMenu",
     "use_key_equivalents": true,
     "bindings": {
+      "up": "settings_editor::FocusPreviousNavEntry",
+      "down": "settings_editor::FocusNextNavEntry",
       "right": "settings_editor::ExpandNavEntry",
       "left": "settings_editor::CollapseNavEntry",
       "pageup": "settings_editor::FocusPreviousRootNavEntry",

crates/settings_ui/src/settings_ui.rs 🔗

@@ -75,7 +75,11 @@ actions!(
         /// Focuses the first navigation entry.
         FocusFirstNavEntry,
         /// Focuses the last navigation entry.
-        FocusLastNavEntry
+        FocusLastNavEntry,
+        /// Focuses and opens the next navigation entry without moving focus to content.
+        FocusNextNavEntry,
+        /// Focuses and opens the previous navigation entry without moving focus to content.
+        FocusPreviousNavEntry
     ]
 );
 
@@ -459,6 +463,7 @@ pub struct SettingsWindow {
     /// [page_index][page_item_index] will be false
     /// when the item is filtered out either by searches
     /// or by the current file
+    navbar_focus_subscriptions: Vec<gpui::Subscription>,
     filter_table: Vec<Vec<bool>>,
     has_query: bool,
     content_handles: Vec<Vec<Entity<NonFocusableHandle>>>,
@@ -889,6 +894,7 @@ impl SettingsWindow {
                 window,
                 cx,
             ),
+            navbar_focus_subscriptions: vec![],
             content_focus_handle: NonFocusableHandle::new(
                 CONTENT_CONTAINER_TAB_INDEX,
                 false,
@@ -934,7 +940,8 @@ impl SettingsWindow {
     }
 
     fn build_navbar(&mut self, cx: &App) {
-        let mut navbar_entries = Vec::with_capacity(self.navbar_entries.len());
+        let mut navbar_entries = Vec::new();
+
         for (page_index, page) in self.pages.iter().enumerate() {
             navbar_entries.push(NavBarEntry {
                 title: page.title,
@@ -963,6 +970,30 @@ impl SettingsWindow {
         self.navbar_entries = navbar_entries;
     }
 
+    fn setup_navbar_focus_subscriptions(
+        &mut self,
+        window: &mut Window,
+        cx: &mut Context<SettingsWindow>,
+    ) {
+        let mut focus_subscriptions = Vec::new();
+
+        for entry_index in 0..self.navbar_entries.len() {
+            let focus_handle = self.navbar_entries[entry_index].focus_handle.clone();
+
+            let subscription = cx.on_focus(
+                &focus_handle,
+                window,
+                move |this: &mut SettingsWindow,
+                      window: &mut Window,
+                      cx: &mut Context<SettingsWindow>| {
+                    this.open_and_scroll_to_navbar_entry(entry_index, window, cx, false);
+                },
+            );
+            focus_subscriptions.push(subscription);
+        }
+        self.navbar_focus_subscriptions = focus_subscriptions;
+    }
+
     fn visible_navbar_entries(&self) -> impl Iterator<Item = (usize, &NavBarEntry)> {
         let mut index = 0;
         let entries = &self.navbar_entries;
@@ -1258,6 +1289,7 @@ impl SettingsWindow {
         if self.pages.is_empty() {
             self.pages = page_data::settings_data();
             self.build_navbar(cx);
+            self.setup_navbar_focus_subscriptions(window, cx);
             self.build_content_handles(window, cx);
         }
         sub_page_stack_mut().clear();
@@ -1355,7 +1387,7 @@ impl SettingsWindow {
             .visible_navbar_entries()
             .any(|(index, _)| index == self.navbar_entry)
         {
-            self.open_and_scroll_to_navbar_entry(self.navbar_entry, window, cx);
+            self.open_and_scroll_to_navbar_entry(self.navbar_entry, window, cx, true);
         } else {
             self.open_first_nav_page();
         };
@@ -1571,6 +1603,38 @@ impl SettingsWindow {
                     this.focus_and_scroll_to_nav_entry(last_entry_index, window, cx);
                 }
             }))
+            .on_action(cx.listener(|this, _: &FocusNextNavEntry, window, cx| {
+                let entry_index = this
+                    .focused_nav_entry(window, cx)
+                    .unwrap_or(this.navbar_entry);
+                let mut next_index = None;
+                for (index, _) in this.visible_navbar_entries() {
+                    if index > entry_index {
+                        next_index = Some(index);
+                        break;
+                    }
+                }
+                let Some(next_entry_index) = next_index else {
+                    return;
+                };
+                this.open_and_scroll_to_navbar_entry(next_entry_index, window, cx, false);
+            }))
+            .on_action(cx.listener(|this, _: &FocusPreviousNavEntry, window, cx| {
+                let entry_index = this
+                    .focused_nav_entry(window, cx)
+                    .unwrap_or(this.navbar_entry);
+                let mut prev_index = None;
+                for (index, _) in this.visible_navbar_entries() {
+                    if index >= entry_index {
+                        break;
+                    }
+                    prev_index = Some(index);
+                }
+                let Some(prev_entry_index) = prev_index else {
+                    return;
+                };
+                this.open_and_scroll_to_navbar_entry(prev_entry_index, window, cx, false);
+            }))
             .border_color(cx.theme().colors().border)
             .bg(cx.theme().colors().panel_background)
             .child(self.render_search(window, cx))
@@ -1602,6 +1666,9 @@ impl SettingsWindow {
                                                 .on_toggle(cx.listener(
                                                     move |this, _, window, cx| {
                                                         this.toggle_navbar_entry(ix);
+                                                        // Update selection state immediately before cx.notify
+                                                        // to prevent double selection flash
+                                                        this.navbar_entry = ix;
                                                         window.focus(
                                                             &this.navbar_entries[ix].focus_handle,
                                                         );
@@ -1612,7 +1679,7 @@ impl SettingsWindow {
                                         .on_click(
                                             cx.listener(move |this, _, window, cx| {
                                                 this.open_and_scroll_to_navbar_entry(
-                                                    ix, window, cx,
+                                                    ix, window, cx, true,
                                                 );
                                             }),
                                         )
@@ -1651,6 +1718,7 @@ impl SettingsWindow {
         navbar_entry_index: usize,
         window: &mut Window,
         cx: &mut Context<Self>,
+        focus_content: bool,
     ) {
         self.open_navbar_entry_page(navbar_entry_index);
         cx.notify();
@@ -1658,12 +1726,17 @@ impl SettingsWindow {
         if self.navbar_entries[navbar_entry_index].is_root
             || !self.is_nav_entry_visible(navbar_entry_index)
         {
-            let Some(first_item_index) = self.visible_page_items().next().map(|(index, _)| index)
-            else {
-                return;
-            };
-            self.focus_content_element(first_item_index, window, cx);
             self.page_scroll_handle.set_offset(point(px(0.), px(0.)));
+            if focus_content {
+                let Some(first_item_index) =
+                    self.visible_page_items().next().map(|(index, _)| index)
+                else {
+                    return;
+                };
+                self.focus_content_element(first_item_index, window, cx);
+            } else {
+                window.focus(&self.navbar_entries[navbar_entry_index].focus_handle);
+            }
         } else {
             let entry_item_index = self.navbar_entries[navbar_entry_index]
                 .item_index
@@ -1676,7 +1749,12 @@ impl SettingsWindow {
             };
             self.page_scroll_handle
                 .scroll_to_top_of_item(selected_item_index);
-            self.focus_content_element(entry_item_index, window, cx);
+
+            if focus_content {
+                self.focus_content_element(entry_item_index, window, cx);
+            } else {
+                window.focus(&self.navbar_entries[navbar_entry_index].focus_handle);
+            }
         }
 
         // Page scroll handle updates the active item index
@@ -2132,7 +2210,12 @@ impl Render for SettingsWindow {
                                 .focus_handle(cx)
                                 .contains_focused(window, cx)
                             {
-                                this.open_and_scroll_to_navbar_entry(this.navbar_entry, window, cx);
+                                this.open_and_scroll_to_navbar_entry(
+                                    this.navbar_entry,
+                                    window,
+                                    cx,
+                                    true,
+                                );
                             } else {
                                 this.focus_and_scroll_to_nav_entry(this.navbar_entry, window, cx);
                             }
@@ -2507,6 +2590,7 @@ mod test {
             navbar_entry: selected_idx.expect("Must have a selected navbar entry"),
             navbar_entries: Vec::default(),
             navbar_scroll_handle: UniformListScrollHandle::default(),
+            navbar_focus_subscriptions: vec![],
             filter_table: vec![],
             has_query: false,
             content_handles: vec![],