Anthony Eid and Ben Kunkle created
Closes #40608
This fixes tabbing in both the settings ui nav bar and page content
going off screen instead of scrolling the focused element into a visible
view.
The bug occurred because `gpui::list` and `gpui::uniform_list` only
render visible elements, preventing non visible elements in a view from
having their focus handle added to the element tree. Thus making the tab
stop map skip over those elements because they weren't present.
The fix for this is scrolling to reveal non visible elements and then
focus the selected element on the next frame.
Release Notes:
- settings ui: Auto scroll to reveal items in navigation bar and window
when tabbing
---------
Co-authored-by: Ben Kunkle <ben@zed.dev>assets/keymaps/default-linux.json | 2
assets/keymaps/default-macos.json | 2
assets/keymaps/default-windows.json | 2
crates/gpui/src/window.rs | 10
crates/settings_ui/src/settings_ui.rs | 164 +++++++++++++++++++++++-----
5 files changed, 144 insertions(+), 36 deletions(-)
@@ -1280,7 +1280,9 @@
"use_key_equivalents": true,
"bindings": {
"up": "settings_editor::FocusPreviousNavEntry",
+ "shift-tab": "settings_editor::FocusPreviousNavEntry",
"down": "settings_editor::FocusNextNavEntry",
+ "tab": "settings_editor::FocusNextNavEntry",
"right": "settings_editor::ExpandNavEntry",
"left": "settings_editor::CollapseNavEntry",
"pageup": "settings_editor::FocusPreviousRootNavEntry",
@@ -1386,7 +1386,9 @@
"use_key_equivalents": true,
"bindings": {
"up": "settings_editor::FocusPreviousNavEntry",
+ "shift-tab": "settings_editor::FocusPreviousNavEntry",
"down": "settings_editor::FocusNextNavEntry",
+ "tab": "settings_editor::FocusNextNavEntry",
"right": "settings_editor::ExpandNavEntry",
"left": "settings_editor::CollapseNavEntry",
"pageup": "settings_editor::FocusPreviousRootNavEntry",
@@ -1309,7 +1309,9 @@
"use_key_equivalents": true,
"bindings": {
"up": "settings_editor::FocusPreviousNavEntry",
+ "shift-tab": "settings_editor::FocusPreviousNavEntry",
"down": "settings_editor::FocusNextNavEntry",
+ "tab": "settings_editor::FocusNextNavEntry",
"right": "settings_editor::ExpandNavEntry",
"left": "settings_editor::CollapseNavEntry",
"pageup": "settings_editor::FocusPreviousRootNavEntry",
@@ -4313,14 +4313,14 @@ impl Window {
}
/// Returns a generic handler that invokes the given handler with the view and context associated with the given view handle.
- pub fn handler_for<V: Render, Callback: Fn(&mut V, &mut Window, &mut Context<V>) + 'static>(
+ pub fn handler_for<E: 'static, Callback: Fn(&mut E, &mut Window, &mut Context<E>) + 'static>(
&self,
- view: &Entity<V>,
+ entity: &Entity<E>,
f: Callback,
- ) -> impl Fn(&mut Window, &mut App) + use<V, Callback> {
- let view = view.downgrade();
+ ) -> impl Fn(&mut Window, &mut App) + 'static {
+ let entity = entity.downgrade();
move |window: &mut Window, cx: &mut App| {
- view.update(cx, |view, cx| f(view, window, cx)).ok();
+ entity.update(cx, |entity, cx| f(entity, window, cx)).ok();
}
}
@@ -1052,7 +1052,7 @@ impl SettingsWindow {
};
// high overdraw value so the list scrollbar len doesn't change too much
- let list_state = gpui::ListState::new(0, gpui::ListAlignment::Top, px(100.0)).measure_all();
+ let list_state = gpui::ListState::new(0, gpui::ListAlignment::Top, px(0.0)).measure_all();
list_state.set_scroll_handler(|_, _, _| {});
let mut this = Self {
@@ -1184,7 +1184,7 @@ impl SettingsWindow {
move |this: &mut SettingsWindow,
window: &mut Window,
cx: &mut Context<SettingsWindow>| {
- this.open_and_scroll_to_navbar_entry(entry_index, window, cx, false);
+ this.open_and_scroll_to_navbar_entry(entry_index, None, false, window, cx);
},
);
focus_subscriptions.push(subscription);
@@ -1622,7 +1622,7 @@ impl SettingsWindow {
.visible_navbar_entries()
.any(|(index, _)| index == self.navbar_entry)
{
- self.open_and_scroll_to_navbar_entry(self.navbar_entry, window, cx, true);
+ self.open_and_scroll_to_navbar_entry(self.navbar_entry, None, true, window, cx);
} else {
self.open_first_nav_page();
};
@@ -1921,7 +1921,13 @@ impl SettingsWindow {
let Some(next_entry_index) = next_index else {
return;
};
- this.open_and_scroll_to_navbar_entry(next_entry_index, window, cx, false);
+ this.open_and_scroll_to_navbar_entry(
+ next_entry_index,
+ Some(gpui::ScrollStrategy::Bottom),
+ false,
+ window,
+ cx,
+ );
}))
.on_action(cx.listener(|this, _: &FocusPreviousNavEntry, window, cx| {
let entry_index = this
@@ -1937,7 +1943,13 @@ impl SettingsWindow {
let Some(prev_entry_index) = prev_index else {
return;
};
- this.open_and_scroll_to_navbar_entry(prev_entry_index, window, cx, false);
+ this.open_and_scroll_to_navbar_entry(
+ prev_entry_index,
+ Some(gpui::ScrollStrategy::Top),
+ false,
+ window,
+ cx,
+ );
}))
.border_color(cx.theme().colors().border)
.bg(cx.theme().colors().panel_background)
@@ -1957,21 +1969,22 @@ impl SettingsWindow {
this.visible_navbar_entries()
.skip(range.start.saturating_sub(1))
.take(range.len())
- .map(|(ix, entry)| {
+ .map(|(entry_index, entry)| {
TreeViewItem::new(
- ("settings-ui-navbar-entry", ix),
+ ("settings-ui-navbar-entry", entry_index),
entry.title,
)
.track_focus(&entry.focus_handle)
.root_item(entry.is_root)
- .toggle_state(this.is_navbar_entry_selected(ix))
+ .toggle_state(this.is_navbar_entry_selected(entry_index))
.when(entry.is_root, |item| {
item.expanded(entry.expanded || this.has_query)
.on_toggle(cx.listener(
move |this, _, window, cx| {
- this.toggle_navbar_entry(ix);
+ this.toggle_navbar_entry(entry_index);
window.focus(
- &this.navbar_entries[ix].focus_handle,
+ &this.navbar_entries[entry_index]
+ .focus_handle,
);
cx.notify();
},
@@ -1980,7 +1993,11 @@ impl SettingsWindow {
.on_click(
cx.listener(move |this, _, window, cx| {
this.open_and_scroll_to_navbar_entry(
- ix, window, cx, true,
+ entry_index,
+ None,
+ true,
+ window,
+ cx,
);
}),
)
@@ -2017,13 +2034,16 @@ impl SettingsWindow {
fn open_and_scroll_to_navbar_entry(
&mut self,
navbar_entry_index: usize,
+ scroll_strategy: Option<gpui::ScrollStrategy>,
+ focus_content: bool,
window: &mut Window,
cx: &mut Context<Self>,
- focus_content: bool,
) {
self.open_navbar_entry_page(navbar_entry_index);
cx.notify();
+ let mut handle_to_focus = None;
+
if self.navbar_entries[navbar_entry_index].is_root
|| !self.is_nav_entry_visible(navbar_entry_index)
{
@@ -2035,9 +2055,17 @@ impl SettingsWindow {
else {
return;
};
- self.focus_content_element(first_item_index, window, cx);
+ handle_to_focus = Some(self.focus_handle_for_content_element(first_item_index, cx));
+ } else if !self.is_nav_entry_visible(navbar_entry_index) {
+ let Some(first_visible_nav_entry_index) =
+ self.visible_navbar_entries().next().map(|(index, _)| index)
+ else {
+ return;
+ };
+ self.focus_and_scroll_to_nav_entry(first_visible_nav_entry_index, window, cx);
} else {
- window.focus(&self.navbar_entries[navbar_entry_index].focus_handle);
+ handle_to_focus =
+ Some(self.navbar_entries[navbar_entry_index].focus_handle.clone());
}
} else {
let entry_item_index = self.navbar_entries[navbar_entry_index]
@@ -2055,18 +2083,33 @@ impl SettingsWindow {
offset_in_item: px(0.),
});
if focus_content {
- self.focus_content_element(entry_item_index, window, cx);
+ handle_to_focus = Some(self.focus_handle_for_content_element(entry_item_index, cx));
} else {
- window.focus(&self.navbar_entries[navbar_entry_index].focus_handle);
+ handle_to_focus =
+ Some(self.navbar_entries[navbar_entry_index].focus_handle.clone());
}
}
+ if let Some(scroll_strategy) = scroll_strategy
+ && let Some(logical_entry_index) = self
+ .visible_navbar_entries()
+ .into_iter()
+ .position(|(index, _)| index == navbar_entry_index)
+ {
+ self.navbar_scroll_handle
+ .scroll_to_item(logical_entry_index + 1, scroll_strategy);
+ }
+
// Page scroll handle updates the active item index
// in it's next paint call after using scroll_handle.scroll_to_top_of_item
// The call after that updates the offset of the scroll handle. So to
// ensure the scroll handle doesn't lag behind we need to render three frames
// back to back.
- cx.on_next_frame(window, |_, window, cx| {
+ cx.on_next_frame(window, move |_, window, cx| {
+ if let Some(handle) = handle_to_focus.as_ref() {
+ window.focus(handle);
+ }
+
cx.on_next_frame(window, |_, _, cx| {
cx.notify();
});
@@ -2133,7 +2176,7 @@ impl SettingsWindow {
fn render_page_items(
&mut self,
- page_index: Option<usize>,
+ page_index: usize,
_window: &mut Window,
cx: &mut Context<SettingsWindow>,
) -> impl IntoElement {
@@ -2197,16 +2240,14 @@ impl SettingsWindow {
.unwrap_or(false);
let is_last = Some(actual_item_index) == last_non_header_index;
+ let item_focus_handle =
+ this.content_handles[page_index][actual_item_index].focus_handle(cx);
+
v_flex()
.id(("settings-page-item", actual_item_index))
.w_full()
.min_w_0()
- .when_some(page_index, |element, page_index| {
- element.track_focus(
- &this.content_handles[page_index][actual_item_index]
- .focus_handle(cx),
- )
- })
+ .track_focus(&item_focus_handle)
.child(item.render(
this,
actual_item_index,
@@ -2327,7 +2368,7 @@ impl SettingsWindow {
page_header = self.render_files_header(window, cx).into_any_element();
page_content = self
- .render_page_items(Some(self.current_page_index()), window, cx)
+ .render_page_items(self.current_page_index(), window, cx)
.into_any_element();
} else {
page_header = h_flex()
@@ -2356,6 +2397,65 @@ impl SettingsWindow {
.pb_8()
.px_8()
.bg(cx.theme().colors().editor_background)
+ .on_action(cx.listener(|this, _: &menu::SelectNext, window, cx| {
+ if !sub_page_stack().is_empty() {
+ window.focus_next();
+ return;
+ }
+ for (logical_index, (actual_index, _)) in this.visible_page_items().enumerate() {
+ let handle = this.content_handles[this.current_page_index()][actual_index]
+ .focus_handle(cx);
+ let mut offset = 1; // for page header
+
+ if let Some((_, next_item)) = this.visible_page_items().nth(logical_index + 1)
+ && matches!(next_item, SettingsPageItem::SectionHeader(_))
+ {
+ offset += 1;
+ }
+ if handle.contains_focused(window, cx) {
+ let next_logical_index = logical_index + offset + 1;
+ this.list_state.scroll_to_reveal_item(next_logical_index);
+ // We need to render the next item to ensure it's focus handle is in the element tree
+ cx.on_next_frame(window, |_, window, cx| {
+ window.focus_next();
+ cx.notify();
+ });
+ cx.notify();
+ return;
+ }
+ }
+ window.focus_next();
+ }))
+ .on_action(cx.listener(|this, _: &menu::SelectPrevious, window, cx| {
+ if !sub_page_stack().is_empty() {
+ window.focus_prev();
+ return;
+ }
+ let mut prev_was_header = false;
+ for (logical_index, (actual_index, item)) in this.visible_page_items().enumerate() {
+ let is_header = matches!(item, SettingsPageItem::SectionHeader(_));
+ let handle = this.content_handles[this.current_page_index()][actual_index]
+ .focus_handle(cx);
+ let mut offset = 1; // for page header
+
+ if prev_was_header {
+ offset -= 1;
+ }
+ if handle.contains_focused(window, cx) {
+ let next_logical_index = logical_index + offset - 1;
+ this.list_state.scroll_to_reveal_item(next_logical_index);
+ // We need to render the next item to ensure it's focus handle is in the element tree
+ cx.on_next_frame(window, |_, window, cx| {
+ window.focus_prev();
+ cx.notify();
+ });
+ cx.notify();
+ return;
+ }
+ prev_was_header = is_header;
+ }
+ window.focus_prev();
+ }))
.child(page_header)
.when(sub_page_stack().is_empty(), |this| {
this.vertical_scrollbar_for(self.list_state.clone(), window, cx)
@@ -2543,12 +2643,13 @@ impl SettingsWindow {
0
}
- fn focus_content_element(&self, item_index: usize, window: &mut Window, cx: &mut App) {
- if !sub_page_stack().is_empty() {
- return;
- }
+ fn focus_handle_for_content_element(
+ &self,
+ actual_item_index: usize,
+ cx: &Context<Self>,
+ ) -> FocusHandle {
let page_index = self.current_page_index();
- window.focus(&self.content_handles[page_index][item_index].focus_handle(cx));
+ self.content_handles[page_index][actual_item_index].focus_handle(cx)
}
fn focused_nav_entry(&self, window: &Window, cx: &App) -> Option<usize> {
@@ -2609,9 +2710,10 @@ impl Render for SettingsWindow {
{
this.open_and_scroll_to_navbar_entry(
this.navbar_entry,
+ None,
+ true,
window,
cx,
- true,
);
} else {
this.focus_and_scroll_to_nav_entry(this.navbar_entry, window, cx);