Onboarding · Help: articles in a list the keyboard walks, a long address cut

HelpPanel lists its articles in lists::SelectableList: the list is one
stop whose cursor scrolls into view, and Enter or a double press opens
an article, focus going to All articles as before. SelectableList now
activates on Enter's release, armed by its own press, as the palette
and menus pick: an activation that moves focus left the release to
press what took it, here the way back. ContactSupport cuts a long
address in the middle and puts a Copy beside it. The lists tests move
into a folder, the release tests on their own.

AGENTS.md

@@ -140,7 +140,7 @@ Ely GPUI Component. A component library for GPUI, in light and dark.
 - A mode that leaves on Escape holds its own focus through `primitives::hold_focus`: turned on while focus is elsewhere, it takes focus so Escape reaches it, and `hand_back` returns it (`documents::ZenMode`, `collab::FollowMode`).
 - A chat holds at its newest message through gpui's `list` with `ListAlignment::Bottom`: it sticks while scrolled to the end, and `scroll_to` one past the last item sticks it again; that call sends no scroll event, so the owner clears its own away state (`chat::MessageList`).
 - Anything pressable is a Tab stop: a row, card or toggle takes `tab_index(0)` and `focus_ring`, with a 1px border that stays transparent where it shows none; gpui presses it on the release of Enter or Space.
-- A list of pressable rows goes through `lists::SelectableList`, whose cursor reaches them by key: Up and Down move, Enter or a double press activates (`media::Playlist`). A bare `ListItem` press is the pointer's only. Titled sections of such lists go through `lists::Sections`, for ChannelList, MemberList and MailboxList; each owner builds its rows and keeps its own checks.
+- A list of pressable rows goes through `lists::SelectableList`, whose cursor reaches them by key: Up and Down move, Enter or a double press activates (`media::Playlist`). Enter activates on its release, armed by its own press, so an activation that moves focus leaves the release nothing to press (`onboarding::HelpPanel`). A bare `ListItem` press is the pointer's only. Titled sections of such lists go through `lists::Sections`, for ChannelList, MemberList and MailboxList; each owner builds its rows and keeps its own checks.
 - Columns that must line up across rows sit absolutely at fractions of their row, as the bars over them do (`calendar::CalendarMonthView`): with real text, gpui 0.2.2 lets a `flex_1` cell's text widen it, and the test platform's fixed advances hide it.
 - A row that holds text beside fixed parts lets the text give way: the text takes `flex_1().min_w_0()` and an `Ellipsis`; counts, icons, dates and chevrons take `flex_none`. A label that must stay whole wraps in that block instead, as `forms::Checkbox`'s does.
 - A mark over a text field reads the field at paint time, from a canvas laid over it (`collab::RemoteCursor`): by then `TextInput::bounds_for` holds this frame's layout, and a still page never renders again to catch up.

TASKS.md

@@ -411,3 +411,4 @@ Per-component lines live in `tasks/`. Tags there:
 | T36c | 2 | PASS | |
 | T37a | 1 | FAIL | the wizard's Next and Back dropped focus as the flow turned; Got it stranded focus on a closed tip; the words read and the bar's share untested apart from their selectors |
 | T37a | 2 | PASS | |
+| T37b | 1 | FAIL | the help panel's rows were bare Tab stops, not a list the keyboard walks; a long support address drew past 280px |

examples/gallery/pages/onboarding/mod.rs

@@ -30,6 +30,7 @@ const SCRIPT: &[Step] = &[
     Step::Key("escape"),
     Step::DownAt("onboarding-help-panel", 120.0, 124.0),
     Step::UpAt("onboarding-help-panel", 120.0, 124.0),
+    Step::Key("enter"),
     Step::Wait(200),
     Step::Shot("help-article"),
     Step::DownAt("onboarding-shortcuts", 20.0, 16.0),

src/lists/select.rs

@@ -47,13 +47,14 @@ struct Cursor {
     scroll: ScrollHandle,
     seen: Option<Vec<SharedString>>,
     sent: Option<Vec<SharedString>>,
+    armed: bool,
 }
 
 type OnSelect = Rc<dyn Fn(&[SharedString], &mut Window, &mut App)>;
 type OnActivate = Rc<dyn Fn(&SharedString, &mut Window, &mut App)>;
 type Picker = Rc<dyn Fn(usize, Pick, &mut Window, &mut App)>;
 
-/// Rows you select. A press picks one; with `multiple`, Cmd-press adds or drops one and Shift-press takes the range from the last pick. Up and Down move, Shift with them extends, Space toggles, Cmd-A takes all. Keys pass over disabled rows, and no pick adds one.
+/// Rows you select. A press picks one; with `multiple`, Cmd-press adds or drops one and Shift-press takes the range from the last pick. Up and Down move, Shift with them extends, Space toggles, Cmd-A takes all, and Enter activates the row on its release, so an activation that moves focus leaves the release nothing to press. Keys pass over disabled rows, and no pick adds one.
 #[derive(IntoElement)]
 pub struct SelectableList {
     id: ElementId,
@@ -235,6 +236,8 @@ impl RenderOnce for SelectableList {
                     })
             })
             .collect();
+        let (lifted, released, barred_rows, keyed) =
+            (cursor.clone(), activate.clone(), off.clone(), keys.clone());
         self.base
             .id(self.id)
             .track_focus(&focus)
@@ -243,6 +246,22 @@ impl RenderOnce for SelectableList {
             .flex()
             .flex_col()
             .gap_0p5()
+            .on_key_up(move |event, window, cx| {
+                if event.keystroke.key != "enter" || count == 0 {
+                    return;
+                }
+                let (armed, at) = lifted.update(cx, |cursor, _| {
+                    (std::mem::take(&mut cursor.armed), cursor.at.min(count - 1))
+                });
+                if !armed {
+                    return;
+                }
+                cx.stop_propagation();
+                if barred_rows[at] {
+                    return;
+                }
+                released(&keyed[at], window, cx);
+            })
             .on_key_down(move |event, window, cx| {
                 if count == 0 {
                     return;
@@ -259,9 +278,7 @@ impl RenderOnce for SelectableList {
                     "space" => (Some(at).filter(open), Pick::Toggle),
                     "enter" => {
                         cx.stop_propagation();
-                        if open(&at) {
-                            activate(&keys[at], window, cx);
-                        }
+                        cursor.update(cx, |cursor, _| cursor.armed = !held.modified());
                         return;
                     }
                     "a" if held.platform => (Some(at), Pick::All),

src/lists/tests/mod.rs

@@ -18,6 +18,8 @@ use crate::{
     theme::Theme,
 };
 
+mod release;
+
 fn setup(cx: &mut TestAppContext) {
     cx.update(|cx| {
         Theme::init(cx);
@@ -322,6 +324,14 @@ fn a_long_list_builds_only_what_is_near_the_view(cx: &mut TestAppContext) {
     assert!((9..60).contains(&most), "built up to row {most} of 1000");
 }
 
+/// Enter, pressed and let go: a row opens on the release.
+fn enter(cx: &mut VisualTestContext) {
+    cx.simulate_keystrokes("enter");
+    cx.simulate_event(gpui::KeyUpEvent {
+        keystroke: gpui::Keystroke::parse("enter").expect("a key"),
+    });
+}
+
 #[gpui::test]
 fn enter_or_a_double_press_opens_a_row(cx: &mut TestAppContext) {
     setup(cx);
@@ -329,7 +339,7 @@ fn enter_or_a_double_press_opens_a_row(cx: &mut TestAppContext) {
     settle(cx);
     cx.simulate_click(row(1), Modifiers::none());
     settle(cx);
-    cx.simulate_keystrokes("enter");
+    enter(cx);
     settle(cx);
     for count in [1, 2] {
         cx.simulate_event(gpui::MouseDownEvent {
@@ -363,7 +373,9 @@ fn keys_pass_over_a_disabled_row_and_never_pick_or_open_it(cx: &mut TestAppConte
     cx.simulate_keystrokes("down");
     settle(cx);
     assert_eq!(picks(&view, cx), ["c"], "down steps over the disabled row");
-    cx.simulate_keystrokes("up enter cmd-a");
+    cx.simulate_keystrokes("up");
+    enter(cx);
+    cx.simulate_keystrokes("cmd-a");
     settle(cx);
     assert_eq!(
         picks(&view, cx),
@@ -392,7 +404,8 @@ fn the_cursor_stays_on_its_row_when_the_rows_reorder(cx: &mut TestAppContext) {
         cx.notify();
     });
     settle(cx);
-    cx.simulate_keystrokes("enter up");
+    enter(cx);
+    cx.simulate_keystrokes("up");
     settle(cx);
     let opened: Vec<String> = view.read_with(cx, |picks, _| {
         picks.1.iter().map(|key| key.to_string()).collect()

src/lists/tests/release.rs

@@ -0,0 +1,94 @@
+use gpui::{
+    AppContext, Context, Entity, FocusHandle, InteractiveElement, IntoElement, ParentElement,
+    Render, TestAppContext, Window, div,
+};
+
+use super::{enter, settle, setup};
+use crate::{
+    buttons::Button,
+    forms::{Enter, Input, TextInput},
+    lists::{ListItem, SelectableList},
+};
+
+/// A list whose open hands focus to a button beside it, and what each heard.
+struct Handoff {
+    button: FocusHandle,
+    heard: Vec<&'static str>,
+}
+
+impl Render for Handoff {
+    fn render(&mut self, _: &mut Window, cx: &mut Context<Self>) -> impl IntoElement {
+        let (to, opened, clicked) = (self.button.clone(), cx.entity(), cx.entity());
+        div()
+            .child(
+                SelectableList::new("handoff")
+                    .row("a", ListItem::new("handoff-a", "A"))
+                    .on_activate(move |_, window, cx| {
+                        window.focus(&to);
+                        opened.update(cx, |view, _| view.heard.push("open"));
+                    }),
+            )
+            .child(
+                Button::new("after", "After")
+                    .focus_handle(&self.button)
+                    .on_click(move |_, _, cx| {
+                        clicked.update(cx, |view, _| view.heard.push("click"))
+                    }),
+            )
+    }
+}
+
+/// Enter opens on its release, so an open that moves focus to a button leaves the button unpressed.
+#[gpui::test]
+fn an_open_that_moves_focus_leaves_the_release_alone(cx: &mut TestAppContext) {
+    setup(cx);
+    let (view, cx) = cx.add_window_view(|_, cx| Handoff {
+        button: cx.focus_handle().tab_stop(true),
+        heard: Vec::new(),
+    });
+    settle(cx);
+    cx.update(|window, _| window.focus_next());
+    settle(cx);
+    enter(cx);
+    settle(cx);
+    assert_eq!(view.read_with(cx, |view, _| view.heard.clone()), ["open"]);
+}
+
+/// A field whose Enter moves focus on to a list, and what the list opened.
+struct Relay {
+    field: Entity<TextInput>,
+    heard: Vec<&'static str>,
+}
+
+impl Render for Relay {
+    fn render(&mut self, _: &mut Window, cx: &mut Context<Self>) -> impl IntoElement {
+        let opened = cx.entity();
+        div()
+            .capture_action(|_: &Enter, window, _| window.focus_next())
+            .child(Input::new(&self.field))
+            .child(
+                SelectableList::new("relay")
+                    .row("a", ListItem::new("relay-a", "A"))
+                    .on_activate(move |_, _, cx| {
+                        opened.update(cx, |view, _| view.heard.push("open"))
+                    }),
+            )
+    }
+}
+
+/// A release whose press fell elsewhere opens nothing.
+#[gpui::test]
+fn a_release_that_began_elsewhere_opens_nothing(cx: &mut TestAppContext) {
+    setup(cx);
+    cx.update(crate::forms::bind_keys);
+    let (view, cx) = cx.add_window_view(|window, cx| Relay {
+        field: cx.new(|cx| TextInput::new(window, cx)),
+        heard: Vec::new(),
+    });
+    settle(cx);
+    cx.update(|window, _| window.focus_next());
+    settle(cx);
+    enter(cx);
+    settle(cx);
+    assert!(view.read_with(cx, |view, _| view.heard.is_empty()));
+}

src/onboarding/help.rs

@@ -10,9 +10,9 @@ use crate::{
     documents::MarkdownRenderer,
     forms::{Input, Run, TextInput},
     layout::on_axis,
-    primitives::{FocusRing, Icon, IconName, tab_stop},
-    theme::{ActiveTheme, ControlSize, IconSize, Radius, TextSize},
-    typography::Ellipsis,
+    lists::{ListItem, SelectableList},
+    primitives::{Icon, IconName, tab_stop},
+    theme::{ActiveTheme, ControlSize, IconSize, TextSize},
 };
 
 /// A help article: its key, its title, a line on it, and its body in Markdown. A link to `help:<key>` in a body opens that article.
@@ -53,7 +53,7 @@ fn link(url: &str) -> Link<'_> {
 
 type OnOpen = Rc<dyn Fn(Option<&SharedString>, &mut Window, &mut App)>;
 
-/// Help beside the work: a search over the articles and their list, or one article with the way back to all. Focus follows: an article opened takes it to All articles, and the way back takes it to the search. Contact support and Keyboard shortcuts wait at its foot when the owner handles them. It fills its box, which the host sizes.
+/// Help beside the work: a search over the articles and their list, where Enter or a double press opens one, or one article with the way back to all. Focus follows: an article opened takes it to All articles, and the way back takes it to the search. Contact support and Keyboard shortcuts wait at its foot when the owner handles them. It fills its box, which the host sizes.
 #[derive(IntoElement)]
 pub struct HelpPanel {
     id: ElementId,
@@ -182,7 +182,7 @@ impl RenderOnce for HelpPanel {
         let body = match shown {
             Some(article) => {
                 let (back, follow) = (on_open.clone(), on_open.clone());
-                div()
+                let page = div()
                     .flex()
                     .flex_col()
                     .gap_3()
@@ -219,57 +219,52 @@ impl RenderOnce for HelpPanel {
                             }
                             Link::Web(url) => cx.open_url(url),
                         }),
-                    )
-                    .into_any_element()
+                    );
+                on_axis(
+                    div()
+                        .id((id.clone(), "body"))
+                        .flex_1()
+                        .min_h_0()
+                        .overflow_y_scroll()
+                        .p_4()
+                        .child(page),
+                )
+                .into_any_element()
             }
             None => {
                 let query = self.search.read(cx).text().trim().to_string();
                 let listed = found(&self.articles, &query);
                 let none = listed.is_empty();
-                let rows = listed.iter().map(|article| {
-                    let (open, key) = (on_open.clone(), article.key.clone());
-                    div()
-                        .id((id.clone(), format!("row-{}", article.key)))
-                        .flex()
-                        .child(
-                            div()
-                                .flex_1()
-                                .min_w_0()
-                                .child(
-                                    div()
-                                        .font_weight(FontWeight::MEDIUM)
-                                        .child(Ellipsis::new(article.title.clone())),
-                                )
-                                .child(
-                                    div()
-                                        .text_size(theme.text_size(TextSize::Sm))
-                                        .text_color(theme.colors.fg_muted)
-                                        .child(article.summary.clone()),
-                                ),
+                let list = listed.iter().fold(
+                    SelectableList::new((id.clone(), "articles")),
+                    |list, article| {
+                        list.row(
+                            article.key.clone(),
+                            ListItem::new(
+                                (id.clone(), format!("row-{}", article.key)),
+                                article.title.clone(),
+                            )
+                            .description(article.summary.clone()),
                         )
-                        .px_2()
-                        .py_1p5()
-                        .rounded(theme.radius(Radius::Md))
-                        .border_1()
-                        .border_color(gpui::transparent_black())
-                        .cursor_pointer()
-                        .hover(|style| style.bg(theme.colors.hover))
-                        .tab_index(0)
-                        .focus_ring(cx)
-                        .on_click(move |_, window, cx| {
-                            log::info!("help panel: open {key}");
-                            open(Some(&key), window, cx)
-                        })
-                });
+                    },
+                );
                 div()
+                    .flex_1()
+                    .min_h_0()
                     .flex()
                     .flex_col()
                     .gap_3()
+                    .p_4()
                     .child(
                         Input::new(&self.search)
                             .prefix(Icon::new(IconName::Search).size(IconSize::Sm)),
                     )
-                    .child(div().flex().flex_col().gap_0p5().children(rows))
+                    .children((!none).then(|| {
+                        list.on_activate(move |key, window, cx| {
+                            log::info!("help panel: open {key}");
+                            on_open(Some(key), window, cx)
+                        })
+                    }))
                     .children(none.then(|| {
                         div()
                             .text_size(theme.text_size(TextSize::Sm))
@@ -299,6 +294,7 @@ impl RenderOnce for HelpPanel {
         });
         let foot = (contact.is_some() || shortcuts.is_some()).then(|| {
             div()
+                .debug_selector(|| "help-foot".into())
                 .flex()
                 .flex_none()
                 .flex_wrap()
@@ -315,15 +311,7 @@ impl RenderOnce for HelpPanel {
             .flex()
             .flex_col()
             .child(head)
-            .child(on_axis(
-                div()
-                    .id((id.clone(), "body"))
-                    .flex_1()
-                    .min_h_0()
-                    .overflow_y_scroll()
-                    .p_4()
-                    .child(body),
-            ))
+            .child(body)
             .children(foot)
     }
 }

src/onboarding/support.rs

@@ -1,20 +1,21 @@
 use std::rc::Rc;
 
 use gpui::{
-    App, ElementId, IntoElement, ParentElement, RenderOnce, SharedString, Styled, Window, div,
+    App, ElementId, InteractiveElement, IntoElement, ParentElement, RenderOnce, SharedString,
+    Styled, Window, div,
 };
 
 use crate::{
-    buttons::{Button, ButtonVariant},
+    buttons::{Button, ButtonVariant, CopyButton},
     forms::{Choice, FormField, Input, Run, Select, TextInput},
     primitives::{Icon, IconName},
     theme::{ActiveTheme, IconSize, TextSize},
-    typography::literal,
+    typography::{MiddleEllipsis, literal},
 };
 
 type OnMessage = Rc<dyn Fn(&SharedString, &str, &mut Window, &mut App)>;
 
-/// A way to reach people: a topic and a message, and Send once both are given. It rests while the owner sends, and empties the message once sent, with focus back in it. With `address`, the address to write to instead shows under it.
+/// A way to reach people: a topic and a message, and Send once both are given. It rests while the owner sends, and empties the message once sent, with focus back in it. With `address`, the address to write to instead shows under it, cut in the middle when narrow, with a Copy.
 #[derive(IntoElement)]
 pub struct ContactSupport {
     id: ElementId,
@@ -116,13 +117,20 @@ impl RenderOnce for ContactSupport {
             .children(self.address.map(|address| {
                 div()
                     .flex()
-                    .flex_wrap()
                     .items_center()
-                    .gap_x_1()
+                    .gap_1()
                     .text_size(theme.text_size(TextSize::Sm))
                     .text_color(theme.colors.fg_muted)
-                    .child("Or write to")
-                    .child(literal(div()).text_color(theme.colors.fg).child(address))
+                    .child(div().flex_none().child("Or write to"))
+                    .child(
+                        literal(div())
+                            .debug_selector(|| "support-address".into())
+                            .flex_1()
+                            .min_w_0()
+                            .text_color(theme.colors.fg)
+                            .child(MiddleEllipsis::new(address.clone())),
+                    )
+                    .child(CopyButton::new((id.clone(), "copy-address"), address))
             }))
     }
 }

src/onboarding/tests/help.rs

@@ -48,16 +48,16 @@ fn panel(bench: &Bench, owner: Entity<Bench>) -> AnyElement {
         .into_any_element()
 }
 
-/// Stops on the list: close, the search, each article, then the foot; on an article: close, All articles, then the foot. An article opened by key takes focus to All articles, and the way back takes it to the search, where words narrow the list.
+/// Stops on the list: close, the search, the list, then the foot; on an article: close, All articles, then the foot. Enter on the list's cursor, on the first row at first, opens an article and takes focus to All articles, which the release leaves alone; the way back takes it to the search, where words narrow the list.
 #[gpui::test]
 fn an_article_opens_from_the_list_and_focus_follows(cx: &mut TestAppContext) {
     let (host, cx) = bench(panel, cx);
     tab(3, cx);
-    tap("space", cx);
+    tap("enter", cx);
     tap("space", cx);
     write("email", cx);
     tab(3, cx);
-    tap("space", cx);
+    tap("enter", cx);
     tab(3, cx);
     tap("space", cx);
     tab(4, cx);
@@ -77,6 +77,47 @@ fn an_article_opens_from_the_list_and_focus_follows(cx: &mut TestAppContext) {
     );
 }
 
+fn many(bench: &Bench, owner: Entity<Bench>) -> AnyElement {
+    let articles = (1..=12).map(|n| article(&format!("a{n}"), &format!("Article {n}"), "Words."));
+    let (opener, contact) = (owner.clone(), owner);
+    div()
+        .w(px(280.0))
+        .h(px(300.0))
+        .child(
+            HelpPanel::new("help", articles, &bench.search, bench.open.clone())
+                .on_open(move |key, _, cx| {
+                    let key = key.cloned();
+                    opener.update(cx, |bench, cx| {
+                        bench.said.push(format!(
+                            "open {}",
+                            key.as_ref().map_or("none", |key| key.as_ref())
+                        ));
+                        bench.open = key;
+                        cx.notify();
+                    })
+                })
+                .on_contact(move |_, cx| say(&contact, "contact".into(), cx)),
+        )
+        .into_any_element()
+}
+
+/// Twelve articles in a 300px panel scroll inside the list, so the foot stays in the box, and the cursor walks down to the eighth.
+#[gpui::test]
+fn a_long_list_scrolls_inside_the_panel(cx: &mut TestAppContext) {
+    let (host, cx) = bench(many, cx);
+    let foot = cx.debug_bounds("help-foot").expect("the foot");
+    assert!(
+        foot.bottom() <= px(316.0),
+        "the foot ends inside the 300px panel, which starts 16px down: {foot:?}"
+    );
+    tab(2, cx);
+    for _ in 0..7 {
+        tap("down", cx);
+    }
+    tap("enter", cx);
+    assert_eq!(said(&host, cx), ["open a8"]);
+}
+
 fn cheatsheet(bench: &Bench, owner: Entity<Bench>) -> AnyElement {
     let shortcut = |name: &str, keys: &str| Shortcut {
         group: "General".into(),
@@ -216,6 +257,30 @@ fn a_message_waits_while_busy(cx: &mut TestAppContext) {
     assert_eq!(said(&host, cx), ["bug: It froze"]);
 }
 
+fn long_address(_: &Bench, owner: Entity<Bench>) -> AnyElement {
+    div()
+        .w(px(280.0))
+        .child(
+            ContactSupport::new("support", [Choice::new("bug", "Something broke")])
+                .address("support-and-billing-questions@example-company.com")
+                .on_send(move |topic, message, _, cx| {
+                    say(&owner, format!("{topic}: {message}"), cx)
+                }),
+        )
+        .into_any_element()
+}
+
+/// A long address gives way in the middle, so it ends inside a 280px box, which starts 16px in.
+#[gpui::test]
+fn a_long_address_is_cut_inside_a_narrow_box(cx: &mut TestAppContext) {
+    let (_, cx) = bench(long_address, cx);
+    let address = cx.debug_bounds("support-address").expect("the address");
+    assert!(
+        address.right() <= px(296.0),
+        "the address ends inside the box: {address:?}"
+    );
+}
+
 fn inline(_: &Bench, owner: Entity<Bench>) -> AnyElement {
     InlineHelp::new("hint", "Only admins can change billing.")
         .on_more(move |_, cx| say(&owner, "more".into(), cx))