diff --git a/AGENTS.md b/AGENTS.md index 8b6fbea..86d3c03 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -77,8 +77,8 @@ Ely GPUI Component. A component library for GPUI, in light and dark. - gpui clicks a focused element when Enter or Space is released. An overlay that hands focus back picks on release too; a pick on press returns focus first, and the release clicks the opener again. An edit in place that hands focus back keeps its text on Enter's release (`forms::InlineEdit`). The press itself bubbles, so a container that takes Space leaves a focused button its own: `overlays::media_button` stops it, and a button in a `SelectableList` row stops Space and Enter at its box (`mail::MailItem`'s star, `messaging::PinnedMessages`' Unpin). - A focus handle a component hands back lives across all its modes: take it with `tab_stop` before any early return, or keyed state drops it while the other mode draws and the handed-back focus lands nowhere. A thread's message keeps one handle folded and open, a Tab stop only while folded (`mail::MailThreadView`). - A button whose words and action change with a mode keeps one handle through `Button::focus_handle`, and the component moves focus to it or back to its field as the mode turns (`project::TimeTracker`). -- A panel that opens from something other than a button goes through `overlays::Popover::with_opener`, which hands the opener its toggle; focus, placement and every way out stay in the popover (`calendar::EventPopover`'s chip). -- A flow in steps goes through `navigation::Wizard`: Back, Next, Finish and the step's motion live there. `onboarding::OnboardingWizard` leaves out its numbered steps (`headless`) and shows a count over a bar in as many parts. +- A panel that opens from something other than a button goes through `overlays::Popover::with_opener`, which hands the opener its toggle; focus, placement and every way out stay in the popover (`calendar::EventPopover`'s chip). Its content gets a close that hands focus back: a press that ends the panel's work closes it before the owner acts, or a panel whose host leaves the tree strands focus (`onboarding::Hotspot`'s Got it, EventPopover's Edit and Delete). +- A flow in steps goes through `navigation::Wizard`: Back, Next, Finish and the step's motion live there. Next and Finish share one focus handle, and focus moves to it when Back reaches the first step or a finished step is picked. `onboarding::OnboardingWizard` leaves out its numbered steps (`headless`) and shows a count over a bar in as many parts. - A dialog goes through `overlays::Dialog`: scrim, focus trap, Escape and focus return live there. Its close runs on every way out, so a dialog with two answers marks the chosen one before it closes (`messaging::IncomingCallDialog`). AlertDialog, ConfirmDialog and PromptDialog build on it; a fullscreen dialog fills the window below its title bar. A dialog's card fits the window, a title bar's height clear at each end; its body scrolls and its actions stay. - Controls on a dark layer over pictures are `overlays::media_button`, light icons that dim when they have nowhere to go, for the lightbox and a video player's bar alike. - A component's letter keys step aside while Command or Control is held, so the app's shortcuts reach the app (`media::ImageAnnotator`, `VideoPlayer`, `VideoThumbnailStrip`). diff --git a/TASKS.md b/TASKS.md index 8d751d1..aaf5493 100644 --- a/TASKS.md +++ b/TASKS.md @@ -409,3 +409,4 @@ Per-component lines live in `tasks/`. Tags there: | T36b | 2 | PASS | | | T36c | 1 | FAIL | a long quota name pushed its count past 280px; a month past 12 read as passed; the Amex shape, the address clearing, the clamped share and the paywall's benefits untested | | 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 | diff --git a/src/calendar/popover.rs b/src/calendar/popover.rs index 698aecf..f8e8682 100644 --- a/src/calendar/popover.rs +++ b/src/calendar/popover.rs @@ -14,7 +14,7 @@ use super::{ use crate::{ buttons::{Button, ButtonVariant}, data_display::color_mark, - forms::OnValue, + forms::{OnValue, Run}, overlays::Popover, primitives::{Icon, IconName}, theme::{ActiveTheme, ControlSize, IconSize, TextSize}, @@ -107,17 +107,18 @@ impl RenderOnce for EventPopover { .unwrap_or_else(|| format::system_zone("event popover")); let (event, chip_zone) = (self.event.clone(), zone.clone()); let (id, chip_id) = (self.id.clone(), (self.id.clone(), "chip")); - let details = move |_: &mut Window, cx: &mut App| { + let details = move |close: Run, _: &mut Window, cx: &mut App| { let theme = cx.theme(); let key = self.event.key.clone(); let action = |label: &'static str, variant, handler: Option| { handler.map(|handler| { - let key = key.clone(); + let (key, close) = (key.clone(), close.clone()); Button::new((self.id.clone(), label), label) .variant(variant) .size(ControlSize::Sm) .on_click(move |_, window, cx| { log::info!("event popover: {label} {key}"); + close(window, cx); handler(&key, window, cx) }) }) diff --git a/src/calendar/tests/people.rs b/src/calendar/tests/people.rs index 19620ab..b5b9dda 100644 --- a/src/calendar/tests/people.rs +++ b/src/calendar/tests/people.rs @@ -90,6 +90,7 @@ fn popover(_: &Planning, owner: Entity) -> gpui::AnyElement { fn a_chip_opens_its_details_and_edit_names_it(cx: &mut TestAppContext) { let (host, cx) = planning(popover, cx); tab_to(1, cx); + let chip = cx.update(|window, cx| window.focused(cx)); press("space", cx); assert!( cx.debug_bounds("event-popover review").is_some(), @@ -105,4 +106,9 @@ fn a_chip_opens_its_details_and_edit_names_it(cx: &mut TestAppContext) { ["edit review"], "Tab reaches Delete, then Edit" ); + let focused = cx.update(|window, cx| window.focused(cx)); + assert_eq!( + focused, chip, + "Edit closes the details and hands focus back to the chip" + ); } diff --git a/src/canvas/panel.rs b/src/canvas/panel.rs index f5352cb..0a70f21 100644 --- a/src/canvas/panel.rs +++ b/src/canvas/panel.rs @@ -87,6 +87,6 @@ pub(crate) fn color_well( .tooltip("Change the color") .on_click(move |window, cx| toggle(window, cx)) }, - move |_, _| ColorPicker::new((id, "picker"), color).on_change(on_pick), + move |_, _, _| ColorPicker::new((id, "picker"), color).on_change(on_pick), ) } diff --git a/src/motion/progress.rs b/src/motion/progress.rs index 2e83a00..63e716b 100644 --- a/src/motion/progress.rs +++ b/src/motion/progress.rs @@ -151,7 +151,11 @@ impl RenderOnce for ProgressBar { .when_some(buffer, |lane, buffer| { lane.child(bar(share(buffer)).bg(faint)) }) - .child(bar(share(at)).bg(fill)) + .child( + bar(share(at)) + .bg(fill) + .debug_selector(move || format!("progress-fill-{ix}")), + ) }) }; div() diff --git a/src/navigation/steps.rs b/src/navigation/steps.rs index 1474fe1..f4eb484 100644 --- a/src/navigation/steps.rs +++ b/src/navigation/steps.rs @@ -10,7 +10,7 @@ use crate::{ buttons::{Button, ButtonVariant}, forms::{Choice, Run}, motion, - primitives::{FocusRing, Icon, IconName}, + primitives::{FocusRing, Icon, IconName, tab_stop}, theme::{ActiveTheme, ControlSize, IconSize, TextSize}, }; @@ -208,7 +208,7 @@ impl RenderOnce for Steps { } } -/// Steps, the current step's content, then Back and Next. Next becomes Finish on the last step. +/// Steps, the current step's content, then Back and Next. Next becomes Finish on the last step, keeping its focus; focus moves to it when Back reaches the first step or a finished step is picked. #[derive(IntoElement)] pub struct Wizard { id: ElementId, @@ -293,7 +293,9 @@ impl RenderOnce for Wizard { on_step(to, window, cx); }) }; + let advance = tab_stop((id.clone(), "advance").into(), true, window, cx); let (back, next, jump) = (step.clone(), step.clone(), step); + let (to_advance, jumped) = (advance.clone(), advance.clone()); let finish = { let id = id.clone(); move |window: &mut Window, cx: &mut App| { @@ -315,17 +317,17 @@ impl RenderOnce for Wizard { }, ) }); - let advance = if current == last { - Button::new((id.clone(), "finish"), "Finish") - .primary() - .disabled(!self.ready) - .on_click(move |_, window, cx| finish(window, cx)) - } else { - Button::new((id.clone(), "next"), "Next") - .primary() - .disabled(!self.ready) - .on_click(move |_, window, cx| next(current + 1, window, cx)) - }; + let forward = Button::new( + (id.clone(), "advance"), + if current == last { "Finish" } else { "Next" }, + ) + .primary() + .disabled(!self.ready) + .focus_handle(&advance) + .on_click(move |_, window, cx| match current == last { + true => finish(window, cx), + false => next(current + 1, window, cx), + }); div() .id(self.id.clone()) .flex() @@ -334,8 +336,12 @@ impl RenderOnce for Wizard { .w_full() .when(!self.headless, |wizard| { wizard.child( - Steps::new((id.clone(), "steps"), self.steps, current) - .on_select(move |to, window, cx| jump(to, window, cx)), + Steps::new((id.clone(), "steps"), self.steps, current).on_select( + move |to, window, cx| { + window.focus(&jumped); + jump(to, window, cx) + }, + ), ) }) .children(content) @@ -348,9 +354,14 @@ impl RenderOnce for Wizard { Button::new((id.clone(), "back"), "Back") .variant(ButtonVariant::Ghost) .disabled(current == 0) - .on_click(move |_, window, cx| back(current - 1, window, cx)), + .on_click(move |_, window, cx| { + if current == 1 { + window.focus(&to_advance); + } + back(current - 1, window, cx) + }), ) - .child(advance), + .child(forward), ) } } diff --git a/src/navigation/tests.rs b/src/navigation/tests.rs index 36bbe5f..05927ba 100644 --- a/src/navigation/tests.rs +++ b/src/navigation/tests.rs @@ -155,6 +155,24 @@ fn the_wizard_walks_forward_back_and_finishes(cx: &mut TestAppContext) { assert!(view.read_with(cx, |view, _| view.done)); } +/// On the last step the first stop is the first finished step; picking it hands focus to Next, so Enter walks on from there. +#[gpui::test] +fn a_picked_step_hands_focus_to_next(cx: &mut TestAppContext) { + setup(cx); + let (view, cx) = cx.add_window_view(|_, _| Flow { + step: 2, + done: false, + }); + cx.update(|window, _| { + window.blur(); + window.focus_next(); + }); + press("enter", cx); + assert_eq!(view.read_with(cx, |view, _| view.step), 0); + press("enter", cx); + assert_eq!(view.read_with(cx, |view, _| view.step), 1); +} + struct Docs { chosen: SharedString, clicked: Option, diff --git a/src/onboarding/checklist.rs b/src/onboarding/checklist.rs index 275d0d0..755a534 100644 --- a/src/onboarding/checklist.rs +++ b/src/onboarding/checklist.rs @@ -82,6 +82,7 @@ impl RenderOnce for SetupChecklist { self.tasks.iter().filter(|task| task.done).count(), self.tasks.len(), ); + let tally = format!("{done} of {all} done"); let turns: Vec = self .tasks .iter() @@ -203,11 +204,14 @@ impl RenderOnce for SetupChecklist { ) .child( tabular(div()) - .debug_selector(move || format!("setup-{done}-of-{all}")) + .debug_selector({ + let tally = tally.clone(); + move || format!("setup-{tally}") + }) .flex_none() .text_size(theme.text_size(TextSize::Sm)) .text_color(theme.colors.fg_muted) - .child(format!("{done} of {all} done")), + .child(tally), ), ) .child( diff --git a/src/onboarding/hotspot.rs b/src/onboarding/hotspot.rs index d19c814..5eed758 100644 --- a/src/onboarding/hotspot.rs +++ b/src/onboarding/hotspot.rs @@ -73,7 +73,7 @@ impl Hotspot { } } - /// Runs on Got it; the owner stops showing the hotspot. + /// Runs on Got it, once the tip has closed and handed focus back; the owner stops showing the hotspot. pub fn on_dismiss(mut self, handler: impl Fn(&mut Window, &mut App) + 'static) -> Self { self.on_dismiss = Some(Rc::new(handler)); self @@ -94,7 +94,7 @@ impl RenderOnce for Hotspot { id: dot.into(), toggle, }, - move |_, cx| { + move |close: Run, _: &mut Window, cx: &mut App| { let theme = cx.theme(); let prose = |text: SharedString| div().flex().child(div().flex_1().min_w_0().child(text)); @@ -114,6 +114,7 @@ impl RenderOnce for Hotspot { .size(ControlSize::Sm) .on_click(move |_, window, cx| { log::info!("hotspot: dismissed"); + close(window, cx); dismiss(window, cx) }), ), diff --git a/src/onboarding/tests.rs b/src/onboarding/tests.rs index 2372baf..7f33652 100644 --- a/src/onboarding/tests.rs +++ b/src/onboarding/tests.rs @@ -1,23 +1,26 @@ use gpui::{ - AnyElement, App, Context, Entity, IntoElement, KeyBinding, ParentElement, Render, Styled, - TestAppContext, VisualTestContext, Window, div, px, + AnyElement, App, Context, Entity, FocusHandle, IntoElement, KeyBinding, ParentElement, Render, + Styled, TestAppContext, VisualTestContext, Window, div, px, }; use super::{ FeatureHighlight, Hotspot, OnboardingStep, OnboardingWizard, SetupChecklist, SetupTask, }; use crate::{ + buttons::Button, forms, - primitives::{FocusNext, IconName}, + primitives::{FocusNext, FocusScope, IconName}, theme::Theme, }; type Part = fn(&Bench, Entity) -> AnyElement; -/// A view that shows one onboarding part, keeps what it heard, and holds the owner's step and readiness. +/// A view that shows one onboarding part, keeps what it heard, and holds the owner's step, readiness and what it hid, with the root focus an app keeps. struct Bench { part: Part, said: Vec, + root: FocusHandle, + hidden: bool, step: usize, ready: bool, all_done: bool, @@ -39,9 +42,11 @@ fn bench(part: Part, cx: &mut TestAppContext) -> (Entity, &mut VisualTest forms::bind_keys(cx); cx.bind_keys([KeyBinding::new("tab", FocusNext, None)]); }); - let (host, cx) = cx.add_window_view(|_, _| Bench { + let (host, cx) = cx.add_window_view(|_, cx| Bench { part, said: Vec::new(), + root: cx.focus_handle(), + hidden: false, step: 0, ready: true, all_done: false, @@ -120,13 +125,23 @@ fn wizard(bench: &Bench, owner: Entity) -> AnyElement { #[gpui::test] fn the_wizard_walks_on_and_back_and_finishes(cx: &mut TestAppContext) { let (host, cx) = bench(wizard, cx); - assert!(cx.debug_bounds("onboarding-step-1-of-3").is_some()); + assert!(cx.debug_bounds("onboarding-Step 1 of 3").is_some()); + let (first, second) = ( + cx.debug_bounds("progress-fill-0") + .expect("the bar's first part"), + cx.debug_bounds("progress-fill-1") + .expect("the bar's second part"), + ); + assert!( + first.size.width > px(0.0) && second.size.width == px(0.0), + "the first of three parts is full: {first:?}, {second:?}" + ); tab(2, cx); tap("space", cx); tab(2, cx); tap("space", cx); edit(&host, cx, |bench| bench.step = 2); - assert!(cx.debug_bounds("onboarding-step-3-of-3").is_some()); + assert!(cx.debug_bounds("onboarding-Step 3 of 3").is_some()); tab(3, cx); tap("space", cx); tab(1, cx); @@ -144,6 +159,28 @@ fn next_rests_until_the_step_is_ready(cx: &mut TestAppContext) { assert_eq!(said(&host, cx), ["skip"]); } +/// On the step before last, the stops are Skip, Back and Next: Next turns to Finish under focus, so a second press finishes. +#[gpui::test] +fn next_keeps_focus_as_it_turns_to_finish(cx: &mut TestAppContext) { + let (host, cx) = bench(wizard, cx); + edit(&host, cx, |bench| bench.step = 1); + tab(3, cx); + tap("space", cx); + tap("space", cx); + assert_eq!(said(&host, cx), ["step 2", "finish"]); +} + +/// Back from the second step leaves Back at rest on the first, so focus moves to Next, and a press walks on again. +#[gpui::test] +fn back_to_the_first_step_hands_focus_to_next(cx: &mut TestAppContext) { + let (host, cx) = bench(wizard, cx); + edit(&host, cx, |bench| bench.step = 1); + tab(2, cx); + tap("space", cx); + tap("space", cx); + assert_eq!(said(&host, cx), ["step 0", "step 1"]); +} + fn hotspot(_: &Bench, owner: Entity) -> AnyElement { Hotspot::new("pins", "Pin a view", "Keep it one press away.") .on_dismiss(move |_, cx| say(&owner, "seen".into(), cx)) @@ -172,6 +209,40 @@ fn a_hotspot_opens_its_tip_from_the_keyboard_and_is_dismissed(cx: &mut TestAppCo ); } +fn hidden_hotspot(bench: &Bench, owner: Entity) -> AnyElement { + let after = owner.clone(); + let spot = (!bench.hidden).then(|| { + Hotspot::new("pins", "Pin a view", "Keep it one press away.").on_dismiss(move |_, cx| { + owner.update(cx, |bench, cx| { + bench.said.push("seen".into()); + bench.hidden = true; + cx.notify(); + }) + }) + }); + FocusScope::new(&bench.root) + .root() + .children(spot) + .child( + Button::new("after", "After").on_click(move |_, _, cx| say(&after, "after".into(), cx)), + ) + .into_any_element() +} + +/// Got it closes the tip before the owner hides the dot, so the root keeps focus and Tab reaches the next stop. +#[gpui::test] +fn after_got_it_tab_reaches_the_next_stop(cx: &mut TestAppContext) { + let (host, cx) = bench(hidden_hotspot, cx); + cx.update(|window, _| window.activate_window()); + tab(1, cx); + tap("enter", cx); + tap("tab", cx); + tap("space", cx); + tap("tab", cx); + tap("space", cx); + assert_eq!(said(&host, cx), ["seen", "after"]); +} + fn highlight(_: &Bench, owner: Entity) -> AnyElement { let later = owner.clone(); FeatureHighlight::new( @@ -222,13 +293,13 @@ fn checklist(bench: &Bench, owner: Entity) -> AnyElement { #[gpui::test] fn a_setup_checklist_starts_what_is_left_and_hides_when_done(cx: &mut TestAppContext) { let (host, cx) = bench(checklist, cx); - assert!(cx.debug_bounds("setup-1-of-3").is_some()); + assert!(cx.debug_bounds("setup-1 of 3 done").is_some()); tab(2, cx); tap("space", cx); tab(3, cx); tap("space", cx); edit(&host, cx, |bench| bench.all_done = true); - assert!(cx.debug_bounds("setup-3-of-3").is_some()); + assert!(cx.debug_bounds("setup-3 of 3 done").is_some()); tab(1, cx); tap("space", cx); assert_eq!(said(&host, cx), ["start connect", "start invite", "hide"]); diff --git a/src/onboarding/wizard.rs b/src/onboarding/wizard.rs index 814fa20..e57ffa5 100644 --- a/src/onboarding/wizard.rs +++ b/src/onboarding/wizard.rs @@ -106,6 +106,8 @@ impl RenderOnce for OnboardingWizard { .unwrap_or_else(|| panic!("onboarding wizard {id:?} has no on_finish")); let theme = cx.theme(); let (current, count) = (self.current, self.steps.len()); + let reached = current + 1; + let place = format!("Step {reached} of {count}"); let skip = self.on_skip.map(|run| { Button::new((id.clone(), "skip"), "Skip for now") .variant(ButtonVariant::Ghost) @@ -123,10 +125,13 @@ impl RenderOnce for OnboardingWizard { .gap_2() .child( div() - .debug_selector(move || format!("onboarding-step-{}-of-{count}", current + 1)) + .debug_selector({ + let place = place.clone(); + move || format!("onboarding-{place}") + }) .text_size(theme.text_size(TextSize::Sm)) .text_color(theme.colors.fg_muted) - .child(format!("Step {} of {count}", current + 1)), + .child(place), ) .children(skip.map(|skip| div().flex_none().child(skip))); let step = &self.steps[current]; @@ -172,11 +177,8 @@ impl RenderOnce for OnboardingWizard { .gap_6() .child( div().flex().flex_col().gap_2().child(head).child( - ProgressBar::new( - (id.clone(), "progress"), - (current + 1) as f32 / count as f32, - ) - .segments(count), + ProgressBar::new((id.clone(), "progress"), reached as f32 / count as f32) + .segments(count), ), ) .child(wizard) diff --git a/src/overlays/popover.rs b/src/overlays/popover.rs index 2b78e98..34616b0 100644 --- a/src/overlays/popover.rs +++ b/src/overlays/popover.rs @@ -12,7 +12,7 @@ use crate::{ theme::ControlSize, }; -type Content = Box AnyElement>; +type Content = Box AnyElement>; type Own = Box AnyElement>; /// A popover's own button: its label and look. @@ -73,20 +73,22 @@ impl Popover { variant: ButtonVariant::Secondary, size: ControlSize::default(), }), - content: Box::new(move |window, cx| content(window, cx).into_any_element()), + content: Box::new(move |_, window, cx| content(window, cx).into_any_element()), } } - /// A panel that opens from an element of the owner's: `opener` builds it with the run that opens and closes the panel. + /// A panel that opens from an element of the owner's: `opener` builds it with the run that opens and closes the panel, and `content` gets the run that closes it and hands focus back, for a press that ends the panel's work. pub fn with_opener( id: impl Into, opener: impl FnOnce(Run) -> T + 'static, - content: impl FnOnce(&mut Window, &mut App) -> E + 'static, + content: impl FnOnce(Run, &mut Window, &mut App) -> E + 'static, ) -> Self { Self { id: id.into(), opener: Opener::Own(Box::new(move |toggle| opener(toggle).into_any_element())), - content: Box::new(move |window, cx| content(window, cx).into_any_element()), + content: Box::new(move |close, window, cx| { + content(close, window, cx).into_any_element() + }), } } @@ -179,9 +181,14 @@ impl RenderOnce for Popover { close(&state, window, cx); return host; } - let (escape, out, tall) = (state.clone(), state.clone(), state.clone()); + let (escape, out, tall, done) = + (state.clone(), state.clone(), state.clone(), state.clone()); let (anchor, height) = (state.read(cx).host, state.read(cx).height); - let body = (self.content)(window, cx); + let body = (self.content)( + Rc::new(move |window, cx| close(&done, window, cx)), + window, + cx, + ); let panel = div() .id((self.id.clone(), "panel")) .relative() diff --git a/src/overlays/tests/mod.rs b/src/overlays/tests/mod.rs index 5e33e14..70d1732 100644 --- a/src/overlays/tests/mod.rs +++ b/src/overlays/tests/mod.rs @@ -430,5 +430,5 @@ fn a_tall_dialog_body_keeps_its_height_and_scrolls(cx: &mut TestAppContext) { #[test] #[should_panic(expected = "an icon is for its own button")] fn a_popover_opened_by_its_owner_takes_no_icon() { - let _ = Popover::with_opener("own", |_| div(), |_, _| div()).icon(IconName::Clock); + let _ = Popover::with_opener("own", |_| div(), |_, _, _| div()).icon(IconName::Clock); } diff --git a/src/project/labels.rs b/src/project/labels.rs index 1a567b2..6eed50e 100644 --- a/src/project/labels.rs +++ b/src/project/labels.rs @@ -8,7 +8,7 @@ use gpui::{ use crate::{ buttons::{ButtonVariant, IconButton}, data_display::color_mark, - forms::{ColorPalette, Enter, FormError, InlineEdit, Input, OnValue, TextInput}, + forms::{ColorPalette, Enter, FormError, InlineEdit, Input, OnValue, Run, TextInput}, mail::Label, overlays::Popover, primitives::{Icon, IconName}, @@ -149,7 +149,7 @@ impl RenderOnce for LabelManager { let swatch_id = (id.clone(), format!("hue-{key}")); let palette = { let (id, key, on_recolor) = (id.clone(), key.clone(), self.on_recolor.clone()); - move |_: &mut Window, _: &mut App| { + move |_: Run, _: &mut Window, _: &mut App| { let colors = HUE_NAMES .iter() .zip(chart)