From 3b673de430e5bec20ebf20c4ba79dd1572469f7e Mon Sep 17 00:00:00 2001 From: clover caruso Date: Sun, 11 Oct 2026 18:57:52 -0700 Subject: [PATCH] fix: offer notebook sources in one Open Notebook menu Share the sidebar and welcome menu, routing filesystem, SMB and Live Share choices through the existing opening flows. Preserve keyboard focus and disabled platform choices, with real accessibility-action replay coverage. fixes #130 Assisted-by: gpt-6.1-sol --- crates/snowbound/src/main.rs | 37 +++++- crates/snowbound/src/sidebar.rs | 205 +++++++++++++++++++------------ crates/snowbound/tests/replay.rs | 163 +++++++++++++++++++----- 3 files changed, 299 insertions(+), 106 deletions(-) diff --git a/crates/snowbound/src/main.rs b/crates/snowbound/src/main.rs index 0a70c368bc141d260a68e01a2c3663a9e2ba9873..1f6d525c84d5e1262628be84beaa71e6b07de9d8 100644 --- a/crates/snowbound/src/main.rs +++ b/crates/snowbound/src/main.rs @@ -390,6 +390,7 @@ impl std::task::Wake for Redraw { #[derive(Debug)] enum Replay { Input(ui::Event), + Access(accesskit::Action, String), /// A trackpad pinch scaling the page by the factor. Pinch(f32), /// Paints the next frame into a PNG as well as the window. @@ -6408,6 +6409,31 @@ impl State { }; match replay { Replay::Input(event) => self.input(event), + Replay::Access(action, label) => { + let tree = self.ui.accessibility_tree("", self.window.scale_factor()); + let mut nodes = tree + .nodes + .iter() + .filter(|(_, node)| node.label() == Some(label.as_str())); + let (id, node) = nodes + .next() + .unwrap_or_else(|| panic!("Replay label {label:?} is missing")); + assert!( + nodes.next().is_none(), + "Replay label {label:?} is ambiguous" + ); + assert!( + node.supports_action(action), + "Replay label {label:?} does not support {action:?}" + ); + self.access_action(accesskit::ActionRequest { + action, + target_tree: accesskit::TreeId::ROOT, + target_node: *id, + data: None, + }) + .expect("Replay accessibility action"); + } Replay::Pinch(factor) => { if let Err(error) = self.pinch(factor) { eprintln!("{error}"); @@ -6794,7 +6820,7 @@ fn write_png(path: &Path, size: [u32; 2], pixels: &[u8]) -> Result<(), Box) -> Result<(), Box> { let (settle, settled) = std::sync::mpsc::channel(); @@ -6868,6 +6894,15 @@ fn replay(script: String, proxy: EventLoopProxy) -> Result<(), Box Ok(Replay::Snapshot(rest.into())), "settle" => Ok(Replay::Settle(None, settle.clone())), "accessibility" => Ok(Replay::Settle(Some(rest.into()), settle.clone())), + "access" => { + let (action, label) = rest.split_once(' ').ok_or("access takes ACTION LABEL")?; + let action = match action { + "click" => accesskit::Action::Click, + "focus" => accesskit::Action::Focus, + _ => return Err(format!("Unknown accessibility action: {action}").into()), + }; + Ok(Replay::Access(action, label.to_owned())) + } "renderer" => Ok(Replay::Renderer(settings::Backend::named(rest)?)), "appearance" => Ok(Replay::Appearance(match rest { "light" => winit::window::Theme::Light, diff --git a/crates/snowbound/src/sidebar.rs b/crates/snowbound/src/sidebar.rs index c69bd7c4b3a128bad3be860c0fb0ab7d42af5eb7..5c4b0f2eccb5a986daa6181df9782a8fbca78175 100644 --- a/crates/snowbound/src/sidebar.rs +++ b/crates/snowbound/src/sidebar.rs @@ -29,7 +29,7 @@ pub const NAV: f32 = 2.0 * ui::shell::TOOL + 1.0 + (RAIL - ui::shell::TOOL); const INDENT: f32 = 16.0; /// What the sidebar was asked to do. -pub enum Action { +enum Action { /// Opens a notebook's section by catalog path. Open { notebook: usize, @@ -37,10 +37,7 @@ pub enum Action { }, /// Folds or unfolds a notebook's or section group's rows, by `Library::key`. Fold(String), - NewNotebook, - OpenNotebook, - #[cfg(feature = "live")] - OpenShared, + Command(crate::Command), /// Signs in again to the notebook opened from its server at this location. SignIn(String), Options, @@ -56,7 +53,7 @@ pub enum Action { } /// What the tree's rows read and what they were asked. -pub struct Tree<'a> { +struct Tree<'a> { theme: &'a Theme, /// The open section's notebook index and catalog path. open: Option<(usize, &'a str)>, @@ -331,34 +328,8 @@ fn sidebar( ..Spec::default() }, ); - for (part, icon, label, chosen) in [ - ( - "new", - Leading::Icon(art::PLUS), - "New Notebook", - Action::NewNotebook, - ), - ( - "open", - Leading::Notebook(art::NOTEBOOK, None), - "Open Notebook", - Action::OpenNotebook, - ), - #[cfg(feature = "live")] - ( - "shared", - Leading::Icon(art::OPEN_SHARED), - "Open Shared", - Action::OpenShared, - ), - ( - "options", - Leading::Icon(art::OPTIONS), - "Options", - Action::Options, - ), - ] { - let (row, _) = tree_row( + let row = |ui: &mut Ui, tree: &mut Tree, part, icon, label| { + tree_row( ui, tree, ui.id(part), @@ -372,10 +343,27 @@ fn sidebar( renamed: false, unread: false, }, - ); - if row.clicked { - tree.action = Some(chosen); - } + ) + .0 + }; + if row(ui, tree, "new", Leading::Icon(art::PLUS), "New Notebook").clicked { + tree.action = Some(Action::Command(crate::Command::NewNotebook)); + } + if let Some(command) = open_notebook( + ui, + Spec { + size: [fill(), px(ROW)], + hover_fill: Some(ui::mix(theme.sidebar, theme.hover(), 0.6)), + radius: 4.0, + pad: [ROW_PAD, 0.0], + gap: ROW_PAD, + ..Spec::default() + }, + ) { + tree.action = Some(Action::Command(command)); + } + if row(ui, tree, "options", Leading::Icon(art::OPTIONS), "Options").clicked { + tree.action = Some(Action::Options); } ui.close(); } @@ -839,6 +827,77 @@ fn tree_row(ui: &mut Ui, tree: &mut Tree, id: Id, row: Row) -> (Signal, bool) { (ui.signal(id), folded) } +fn open_notebook(ui: &mut Ui, spec: Spec) -> Option { + let button = ui.id("open"); + let menu = button.child("menu"); + let center = spec.center; + let theme = ui.theme.clone(); + ui.open_as( + button, + Spec { + flags: Flags::CLICKABLE, + role: Some(accesskit::Role::Button), + ..spec + }, + ); + ui.leaf( + "label", + Spec { + size: [fill(), fill()], + icon: Some(art::NOTEBOOK), + tint: Some(crate::notebook_color(&theme, None)), + text: Some(crate::commands::command(crate::commands::Id::OpenNotebook).title), + center, + ..Spec::default() + }, + ); + ui.leaf( + "arrow", + Spec { + size: [px(ICON), fill()], + icon: Some(ui::shell::CHEVRON), + color: Some(theme.text_dim), + ..Spec::default() + }, + ); + ui.close(); + if ui.signal(button).pressed { + ui.set_focus(Some(button)); + ui.open_popup(menu); + } + let open = ui.popup_open(menu); + if let Some(node) = ui.access(button) { + node.set_has_popup(accesskit::HasPopup::Menu); + node.set_expanded(open); + } + let items = [ + ui::popup::Item { + text: "From File System", + icon: Some(art::NOTEBOOK), + ..Default::default() + }, + ui::popup::Item { + text: "From SMB Share", + icon: Some(art::SERVER), + disabled: cfg!(target_arch = "wasm32"), + ..Default::default() + }, + ui::popup::Item { + text: "Connect to Live Share", + icon: Some(art::OPEN_SHARED), + disabled: !cfg!(feature = "live"), + ..Default::default() + }, + ]; + match ui::popup::menu(ui, menu, ui::Anchor::Below(button), &items, None) { + Some(0) => Some(crate::Command::OpenNotebook), + Some(1) => Some(crate::Command::OpenFromServer(None)), + #[cfg(feature = "live")] + Some(2) => Some(crate::Command::OpenShared), + _ => None, + } +} + impl crate::State { /// How wide the sidebar stands this frame as it eases open and shut. pub(crate) fn sidebar_width(&mut self) -> f32 { @@ -969,10 +1028,7 @@ impl crate::State { self.rename(target); } Some(Action::Renamed(keep)) => self.finish_renaming(keep), - Some(Action::NewNotebook) => self.commands.push(crate::Command::NewNotebook), - Some(Action::OpenNotebook) => self.commands.push(crate::Command::OpenNotebook), - #[cfg(feature = "live")] - Some(Action::OpenShared) => self.open_shared(), + Some(Action::Command(command)) => self.commands.push(command), Some(Action::SignIn(location)) => self .commands .push(crate::Command::OpenFromServer(Some(location))), @@ -1454,22 +1510,9 @@ impl crate::State { ( "open", Leading::Notebook(art::NOTEBOOK, None), - "Open Notebook", + crate::commands::command(crate::commands::Id::OpenNotebook).title, crate::Command::OpenNotebook, ), - #[cfg(feature = "live")] - ( - "shared", - Leading::Icon(art::OPEN_SHARED), - "Open Shared Notebook", - crate::Command::OpenShared, - ), - ( - "server", - Leading::Icon(art::SERVER), - "Open Notebook from Server", - crate::Command::OpenFromServer(None), - ), ] .into_iter() .chain(crate::guide::OFFERED.then_some(( @@ -1572,26 +1615,36 @@ impl crate::State { ); let mut chosen = None; for (part, icon, label, command) in buttons { - let (icon, tint) = icon.art(&theme); - let button = self.ui.leaf( - part, - Spec { - flags: Flags::CLICKABLE, - size: [fill(), px(BUTTON[1])], - icon: Some(icon), - tint: Some(tint), - text: Some(label), - fill: Some(theme.chip), - hover_fill: Some(theme.hover()), - hover_border: Some(theme.accent), - radius: 6.0, - center: true, - role: Some(accesskit::Role::Button), - ..Spec::default() - }, - ); - if button.clicked { - chosen = Some(command); + let spec = Spec { + flags: Flags::CLICKABLE, + size: [fill(), px(BUTTON[1])], + fill: Some(theme.chip), + hover_fill: Some(theme.hover()), + hover_border: Some(theme.accent), + radius: 6.0, + center: true, + role: Some(accesskit::Role::Button), + ..Spec::default() + }; + if matches!(command, crate::Command::OpenNotebook) { + chosen = open_notebook(&mut self.ui, spec).or(chosen); + } else { + let (icon, tint) = icon.art(&theme); + if self + .ui + .leaf( + part, + Spec { + icon: Some(icon), + tint: Some(tint), + text: Some(label), + ..spec + }, + ) + .clicked + { + chosen = Some(command); + } } } self.commands.extend(chosen); diff --git a/crates/snowbound/tests/replay.rs b/crates/snowbound/tests/replay.rs index 723fa558140a8d827c6c53b3bfa11caeefa726a0..e8ae0b042a1ba2a2661a52c48fd15e0925851917 100644 --- a/crates/snowbound/tests/replay.rs +++ b/crates/snowbound/tests/replay.rs @@ -796,37 +796,142 @@ fn stop_sharing_ends_the_share() { assert!(!kept.contains("code"), "still kept: {kept}"); } -/// Open Shared Notebook is where people look with notebooks open: in the sidebar's footer, -/// and in the palette, which opens it. -#[cfg(feature = "live")] #[test] -fn open_shared_notebook_is_offered_with_notebooks_open() { - let scratch = Scratch::new("open-shared"); - std::fs::write( - scratch.0.join("settings.json"), - r#"{"user_name": "Snowbound Test", "sidebar": true}"#, - ) - .unwrap(); - let notebook = - Path::new(env!("CARGO_MANIFEST_DIR")).join("../../corpus/cross-container/candidate"); - let mut steps = vec!["accessibility sidebar", "modifiers command shift", "key p"]; - steps.extend([ - "modifiers", - "settle", - "type Open Shared", - "settle", - "key Enter", - ]); - steps.extend(["settle", "accessibility dialog"]); - let variables = [("SNOWBOUND_LIVE", "off"), ("SNOWBOUND_LIVE_RELAY", "off")]; - let [sidebar, dialog] = replay_with(&scratch, Some(¬ebook), &steps, &variables) - .try_into() +fn open_notebook_routes_from_the_sidebar_and_welcome() { + for welcome in [false, true] { + let scratch = Scratch::new(if welcome { + "welcome-open" + } else { + "sidebar-open" + }); + std::fs::write( + scratch.0.join("settings.json"), + r#"{"user_name": "Snowbound Test", "sidebar": true}"#, + ) .unwrap(); - assert!(sidebar.contains(r#""Open Shared""#), "{sidebar}"); - assert!( - dialog.contains(r#"Dialog "Open Shared Notebook""#), - "{dialog}" - ); + let notebook = + Path::new(env!("CARGO_MANIFEST_DIR")).join("../../corpus/cross-container/candidate"); + let mut steps = Vec::new(); + if welcome { + steps.extend([ + "modifiers command shift", + "key p", + "modifiers", + "settle", + "type Close This Notebook", + "key Enter", + "settle", + ]); + } + steps.extend([ + "accessibility initial", + "access focus Open Notebook", + "key Enter", + "accessibility menu", + "key Escape", + "accessibility escaped", + "key Space", + "accessibility space-menu", + "move 1170 10", + "press", + "release", + "accessibility outside", + "key Enter", + "key Down", + "key Down", + "key Enter", + "accessibility keyboard-smb", + "key Escape", + "access click Open Notebook", + "access click From SMB Share", + "accessibility access-smb", + "key Escape", + "accessibility cancelled", + ]); + #[cfg(feature = "live")] + steps.extend([ + "access click Open Notebook", + "access click Connect to Live Share", + "accessibility live", + "key Escape", + "accessibility live-cancelled", + "modifiers command shift", + "key p", + "modifiers", + "settle", + "type Open Shared", + "key Enter", + "accessibility palette-live", + ]); + let variables = [("SNOWBOUND_LIVE", "off"), ("SNOWBOUND_LIVE_RELAY", "off")]; + let trees = replay_with(&scratch, Some(¬ebook), &steps, &variables); + assert_eq!( + trees[0].matches(r#"Button "Open Notebook""#).count(), + 1, + "{}", + trees[0] + ); + assert_eq!( + trees[0].contains("No notebooks open"), + welcome, + "{}", + trees[0] + ); + for tree in [&trees[1], &trees[3]] { + assert_eq!( + menu_items(tree), + [ + "From File System", + "From SMB Share", + "Connect to Live Share" + ] + ); + assert!( + tree.contains(r#"Button "Open Notebook" [expanded]"#), + "{tree}" + ); + } + let live = trees[1] + .lines() + .find(|line| line.contains(r#"MenuItem "Connect to Live Share""#)) + .unwrap(); + assert_eq!( + live.contains("[disabled]"), + !cfg!(feature = "live"), + "{live}" + ); + for tree in [&trees[2], &trees[4], &trees[7]] { + assert!( + tree.contains(r#"Button "Open Notebook" [focused]"#), + "{tree}" + ); + assert!(!tree.contains("MenuItem"), "{tree}"); + } + for tree in &trees[5..7] { + assert_eq!( + tree.matches(r#"Dialog "Open Notebook from Server""#) + .count(), + 1, + "{tree}" + ); + assert!(!tree.contains("MenuItem"), "{tree}"); + } + #[cfg(feature = "live")] + { + for tree in [&trees[8], &trees[10]] { + assert_eq!( + tree.matches(r#"Dialog "Open Shared Notebook""#).count(), + 1, + "{tree}" + ); + } + assert!( + trees[9].contains(r#"Button "Open Notebook" [focused]"#), + "{}", + trees[9] + ); + } + } } /// Switching between Metal and OpenGL, again and again, leaves the page, its edits and the -- 2.54.0