From fa1dcd73e2a032fd93722dd05e9cba56f66de787 Mon Sep 17 00:00:00 2001 From: neilcawse <8085959+neilcawse@users.noreply.github.com> Date: Sun, 11 Oct 2026 20:57:04 -0400 Subject: [PATCH] UI: Paste button and menus paste the system clipboard; freeze regression test (#273) In 0.5.0 a click anywhere on the Home ribbon's Paste button (label, icon or dropdown arrow) froze the app for good. home() called paste.secondary_clicked() inside ctx.data_mut(); after any click egui answers that with ctx.input(), which takes the context lock the closure already holds, so the UI thread waited on itself. Sampling the hung 0.5.0 process on macOS shows the main thread parked in RawRwLock::lock_exclusive_slow under Context::write <- data_mut <- ribbon::home. #272 moved the query out of the closure; it landed after the 0.5.0 release, which is the build both reporters used. No test ever clicked the Paste button itself, which is how this shipped. The new test clicks it, then opens its menu with a secondary click and picks Paste Values, on a worker thread with a timeout so a regression fails instead of hanging the run. With #272 reverted it fails (egui's debug lock check panics after 10 s); with it, it passes. The Paste button gets an accessible "Paste" name so the test (and screen readers) can find it, the same way Pictures and Border presets are named. With the freeze gone, the button still did not do what it says: it pasted GridCraft's own copy only (GridState::system_clipboard was never assigned), so text copied in another app gave "Nothing to paste." The ribbon button, its options menu's Paste, the cell and header context menus and the macOS Edit menu now read the system clipboard through a new paste_text host service and paste it like Cmd/Ctrl+V does, falling back to GridCraft's own copy when there is no text (or no service, as on the web). Asking egui for the clipboard instead (RequestPaste) was tried and stalled the macOS event loop until the next input event. Co-Authored-By: Claude Opus 5.5 --- apps/gridcraft/src/main.rs | 23 +++-- apps/gridcraft/src/native_menu.rs | 2 + crates/ui-egui/src/grid.rs | 3 +- crates/ui-egui/src/lib.rs | 15 ++++ crates/ui-egui/src/ribbon.rs | 9 +- crates/ui-egui/tests/clipboard.rs | 135 +++++++++++++++++++++++++++++- docs/ui-parity.md | 3 +- 7 files changed, 173 insertions(+), 17 deletions(-) diff --git a/apps/gridcraft/src/main.rs b/apps/gridcraft/src/main.rs index 8093205..4fbac1e 100644 --- a/apps/gridcraft/src/main.rs +++ b/apps/gridcraft/src/main.rs @@ -135,10 +135,22 @@ fn save_prefs(app: &SheetApp) { } } +/// Read the system clipboard through the shared handle, creating it on first use. +fn read_clipboard( + handle: &RefCell>, + read: impl FnOnce(&mut arboard::Clipboard) -> Result, +) -> Option { + let mut clipboard = handle.try_borrow_mut().ok()?; + if clipboard.is_none() { + *clipboard = arboard::Clipboard::new().ok(); + } + read(clipboard.as_mut()?).ok() +} + fn services() -> Services { // Keep the owner alive: X11/Wayland serve clipboard data from this handle. let clipboard: Rc>> = Rc::default(); - let paste_clipboard = clipboard.clone(); + let (html_clipboard, text_clipboard) = (clipboard.clone(), clipboard.clone()); Services { copy_html: Some(Box::new(move |html, text| { let mut clipboard = clipboard.borrow_mut(); @@ -147,13 +159,8 @@ fn services() -> Services { } clipboard.as_mut().ok_or("clipboard unavailable")?.set_html(html, Some(text)).map_err(|e| e.to_string()) })), - paste_html: Some(Box::new(move || { - let mut clipboard = paste_clipboard.try_borrow_mut().ok()?; - if clipboard.is_none() { - *clipboard = arboard::Clipboard::new().ok(); - } - clipboard.as_mut()?.get().html().ok() - })), + paste_html: Some(Box::new(move || read_clipboard(&html_clipboard, |clipboard| clipboard.get().html()))), + paste_text: Some(Box::new(move || read_clipboard(&text_clipboard, |clipboard| clipboard.get_text()))), pick_open: Some(Box::new(|| { rfd::FileDialog::new() .add_filter("Spreadsheets", &["xlsx", "xlsm", "xls", "xlsb", "ods", "csv", "tsv", "txt", "json"]) diff --git a/apps/gridcraft/src/native_menu.rs b/apps/gridcraft/src/native_menu.rs index b127543..f4b6e80 100644 --- a/apps/gridcraft/src/native_menu.rs +++ b/apps/gridcraft/src/native_menu.rs @@ -298,6 +298,8 @@ impl NativeMenu { app.open_dialog(name, params); } else if id == "edit.copy" || id == "edit.cut" { app.copy_to_clipboard(ctx, &id); + } else if id == "edit.paste" { + app.paste_from_clipboard(); } else { app.run_or_alert(&id, json!({})); } diff --git a/crates/ui-egui/src/grid.rs b/crates/ui-egui/src/grid.rs index d0eef85..ece47cd 100644 --- a/crates/ui-egui/src/grid.rs +++ b/crates/ui-egui/src/grid.rs @@ -84,7 +84,6 @@ pub struct GridState { pub last_border: String, pub last_fill: String, pub last_font_color: String, - pub system_clipboard: Option, pub trace: Option, pub renaming_tab: Option, pub rename_text: String, @@ -2651,6 +2650,7 @@ fn context_menu(app: &mut SheetApp, ui: &mut egui::Ui) { "edit.copy" | "edit.cut" => { app.copy_to_clipboard(ui.ctx(), id); } + "edit.paste" => app.paste_from_clipboard(), other => app.run_or_alert(other, json!({})), } } @@ -2893,6 +2893,7 @@ fn header_menu(app: &mut SheetApp, ui: &mut egui::Ui, was_open: bool) { "edit.copy" | "edit.cut" => { app.copy_to_clipboard(ui.ctx(), id); } + "edit.paste" => app.paste_from_clipboard(), other => app.run_or_alert(other, json!({})), } } diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index 0b19b25..8655089 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -179,6 +179,10 @@ pub struct Services { /// source spreadsheet recorded as numbers stay numbers ("Rp10,000.00" → 10000). `None` from /// the host pastes the plain text alone. pub paste_html: Option Option>>, + /// Read the system clipboard's plain text for the ribbon and menu Paste commands (Cmd/Ctrl+V + /// gets it from egui's `Event::Paste`). `None` from the host, or no service (web), pastes + /// GridCraft's own clipboard. + pub paste_text: Option Option>>, /// Web: deliver a file to the user (name, bytes). pub download: Option>, /// Web: start an asynchronous file pick; the bytes arrive later in `inbox`. @@ -392,6 +396,17 @@ impl SheetApp { self.run_or_alert("edit.paste", params); } + /// Ribbon and menu Paste: paste the text on the system clipboard, as Cmd/Ctrl+V does. With no + /// text there, or no host clipboard (web), GridCraft's own clipboard is pasted. + pub fn paste_from_clipboard(&mut self) { + let text = self.services.paste_text.as_mut().and_then(|read| read()).filter(|t| !t.is_empty()); + match text { + // Same line endings as egui's `Event::Paste` text. + Some(text) => self.paste_external(text.replace("\r\n", "\n")), + None => self.run_or_alert("edit.paste", json!({})), + } + } + /// Web: `file.exportPdf` without a path returns the PDF as base64; hand it to the browser as a download. fn deliver_pdf(&self, result: &Json) { if result.get("path").is_some() { diff --git a/crates/ui-egui/src/ribbon.rs b/crates/ui-egui/src/ribbon.rs index e8291ac..e02316c 100644 --- a/crates/ui-egui/src/ribbon.rs +++ b/crates/ui-egui/src/ribbon.rs @@ -483,6 +483,8 @@ fn menu_items(app: &mut SheetApp, ui: &mut Ui, items: &[(&str, &str, serde_json: if response.clicked() { if let Some(d) = id.strip_prefix("dialog:") { app.open_dialog(d, p.clone()); + } else if *id == "edit.paste" { + app.paste_from_clipboard(); } else { act(app, id, p.clone()); } @@ -495,17 +497,14 @@ fn home(app: &mut SheetApp, ui: &mut Ui) { let st = active_style(app); // Clipboard let paste = big_button(ui, Icon::Paste, "Paste", "Paste (⌘V)", true); + paste.widget_info(|| egui::WidgetInfo::labeled(egui::WidgetType::Button, ui.is_enabled(), "Paste")); for sequence in keytip_table::group_extension(app.keytips.prefix().unwrap_or_default(), &["edit.paste", "edit.pasteSpecial", "dialog:pasteSpecial"]) { keytip(app, ui, sequence, paste.rect.center_bottom()); } if paste.clicked() { - if let Some(text) = app.grid.system_clipboard.take() { - app.paste_external(text); - } else { - act(app, "edit.paste", json!({})); - } + app.paste_from_clipboard(); } let paste_keys = matches!(app.keytips.prefix(), Some("HV" | "E")); // Keep a keyboard-opened menu anchored to the button when a pointer click diff --git a/crates/ui-egui/tests/clipboard.rs b/crates/ui-egui/tests/clipboard.rs index 71fe336..1e1181b 100644 --- a/crates/ui-egui/tests/clipboard.rs +++ b/crates/ui-egui/tests/clipboard.rs @@ -3,9 +3,12 @@ use std::cell::RefCell; use std::rc::Rc; -use egui::{Event, OutputCommand}; +use egui::accesskit::Role; +use egui::{Event, Modifiers, OutputCommand, PointerButton}; +use egui_kittest::kittest::Queryable as _; use gridcraft_engine::Session; -use gridcraft_ui_egui::{Services, SheetApp}; +use gridcraft_engine::core::{CellRef, Value}; +use gridcraft_ui_egui::{Services, SheetApp, grid}; use serde_json::json; fn harness(services: Services) -> egui_kittest::Harness<'static, SheetApp> { @@ -116,3 +119,131 @@ fn copying_multiple_areas_copies_lined_up_areas_and_explains_the_rest() { assert_eq!(h.state().message.as_ref().map(|(_, text)| text.as_str()), Some("This action won't work on multiple selections.")); assert_eq!(h.state().session.clipboard.as_ref().map(|c| c.areas.len()), Some(2), "the earlier copy stays"); } + +#[test] +fn clicking_the_ribbon_paste_button_or_its_menu_does_not_freeze() { + // #273: a click on Paste deadlocked the UI thread (a click-state query inside the egui context + // lock), so the frame never returned. Run the clicks on a worker and fail on a timeout instead of + // hanging the test run if that comes back. + let (done, finished) = std::sync::mpsc::channel(); + let worker = std::thread::spawn(move || { + let cell = value_at; + let mut h = harness(Services::default()); + h.state_mut().run("edit.copy", json!({"range": "A1:B2"})).unwrap(); + h.state_mut().run("selection.set", json!({"cell": "D4"})).unwrap(); + // No host clipboard here (as on the web): GridCraft's own copy is pasted. + h.get_by_role_and_label(Role::Button, "Paste").click(); + h.run_steps(3); + assert_eq!(cell(&h, "D4"), Some(Value::from("Name"))); + assert_eq!(cell(&h, "E5"), Some(Value::number(42.0))); + + // The secondary click opens the paste options menu; its items paste too. + h.state_mut().run("selection.set", json!({"cell": "G1"})).unwrap(); + h.get_by_role_and_label(Role::Button, "Paste").click_secondary(); + h.run_steps(3); + h.get_by_label("Paste Values").click(); + h.run_steps(3); + assert_eq!(cell(&h, "G1"), Some(Value::from("Name"))); + assert_eq!(cell(&h, "H2"), Some(Value::number(42.0))); + let _ = done.send(()); + }); + match finished.recv_timeout(std::time::Duration::from_secs(60)) { + Ok(()) => assert!(worker.join().is_ok()), + Err(std::sync::mpsc::RecvTimeoutError::Disconnected) => { + if let Err(panic) = worker.join() { + std::panic::resume_unwind(panic); + } + } + Err(std::sync::mpsc::RecvTimeoutError::Timeout) => panic!("clicking Paste froze the UI thread"), + } +} + +type Harness = egui_kittest::Harness<'static, SheetApp>; + +fn value_at(h: &Harness, a1: &str) -> Option { + let at = gridcraft_engine::core::CellRef::parse(a1).unwrap(); + h.state().session.active().and_then(|d| d.wb.active().map(|s| s.value(at))) +} + +/// A host clipboard holding `text`, and how many times the app read it. +fn system_clipboard(text: &str) -> (Rc>, Services) { + let clipboard = Rc::new(RefCell::new((text.to_string(), 0))); + let read = clipboard.clone(); + let services = Services { + paste_text: Some(Box::new(move || { + let mut clipboard = read.borrow_mut(); + clipboard.1 += 1; + Some(clipboard.0.clone()) + })), + ..Default::default() + }; + (clipboard, services) +} + +#[test] +fn ribbon_paste_pastes_the_system_clipboard_like_cmd_v() { + // Text copied in another app: the Paste button pastes it, as Cmd/Ctrl+V does. + let (clipboard, services) = system_clipboard("x\t1\r\ny\t2\r\n"); + let mut h = harness(services); + h.state_mut().run("selection.set", json!({"cell": "D4"})).unwrap(); + h.get_by_role_and_label(Role::Button, "Paste").click(); + h.run_steps(2); + assert_eq!(clipboard.borrow().1, 1, "one clipboard read per click"); + assert_eq!(value_at(&h, "D4"), Some(Value::from("x"))); + assert_eq!(value_at(&h, "E4"), Some(Value::number(1.0))); + assert_eq!(value_at(&h, "D5"), Some(Value::from("y"))); + assert_eq!(value_at(&h, "E5"), Some(Value::number(2.0))); + assert!(h.state().message.is_none(), "{:?}", h.state().message); + + // The clipboard text is GridCraft's own copy: the copy is pasted with its formulas. + h.state_mut().run("cell.set", json!({"cell": "A3", "input": "=B2*2"})).unwrap(); + let copied = h.state_mut().run("edit.copy", json!({"range": "A3"})).unwrap(); + clipboard.borrow_mut().0 = copied["text"].as_str().unwrap().to_string(); + h.state_mut().run("selection.set", json!({"cell": "B9"})).unwrap(); + h.get_by_role_and_label(Role::Button, "Paste").click(); + h.run_steps(2); + let sh = h.state().session.active().unwrap().wb.active().unwrap(); + assert_eq!(sh.cell(CellRef::parse("B9").unwrap()).unwrap().formula.as_ref().unwrap().text, "C8*2"); + + // Nothing on the system clipboard (an image, say): GridCraft's own copy is pasted. + clipboard.borrow_mut().0.clear(); + h.state_mut().run("selection.set", json!({"cell": "B12"})).unwrap(); + h.get_by_role_and_label(Role::Button, "Paste").click(); + h.run_steps(2); + let sh = h.state().session.active().unwrap().wb.active().unwrap(); + assert_eq!(sh.cell(CellRef::parse("B12").unwrap()).unwrap().formula.as_ref().unwrap().text, "C11*2"); +} + +#[test] +fn paste_menu_items_paste_the_system_clipboard() { + let (clipboard, services) = system_clipboard("from menu"); + let mut h = harness(services); + let ribbon = h.get_by_role_and_label(Role::Button, "Paste").rect(); + let click_menu_paste = |h: &Harness| h.get_all_by_role_and_label(Role::Button, "Paste").find(|n| n.rect() != ribbon).unwrap().click(); + + // Home › Paste options menu › Paste. + h.state_mut().run("selection.set", json!({"cell": "D4"})).unwrap(); + h.get_by_role_and_label(Role::Button, "Paste").click_secondary(); + h.run_steps(2); + click_menu_paste(&h); + h.run_steps(2); + assert_eq!(value_at(&h, "D4"), Some(Value::from("from menu"))); + + // The cell's context menu › Paste. + clipboard.borrow_mut().0 = "from cell menu".into(); + h.state_mut().run("selection.set", json!({"cell": "F6"})).unwrap(); + let app = h.state(); + let sh = app.session.active().unwrap().wb.active().unwrap(); + let geo = grid::Geo::new(sh, app.grid.rect.unwrap(), app.view().scroll); + let pos = geo.cell_rect(sh, CellRef::parse("F6").unwrap()).center(); + h.input_mut().events.push(Event::PointerMoved(pos)); + for pressed in [true, false] { + h.input_mut().events.push(Event::PointerButton { pos, button: PointerButton::Secondary, pressed, modifiers: Modifiers::NONE }); + h.run_steps(2); + } + assert!(h.state().grid.context_menu.is_some()); + click_menu_paste(&h); + h.run_steps(2); + assert_eq!(value_at(&h, "F6"), Some(Value::from("from cell menu"))); + assert_eq!(clipboard.borrow().1, 2); +} diff --git a/docs/ui-parity.md b/docs/ui-parity.md index 10abcbc..065f335 100644 --- a/docs/ui-parity.md +++ b/docs/ui-parity.md @@ -1,6 +1,6 @@ # UI parity -> **Last reviewed:** 2026-10-10 · **Last updated:** 2026-10-11 · **Change:** minor (AutoFit reserves filter dropdown space, observed against Windows Excel) · **Target:** Microsoft Excel (Microsoft 365), Excel for Mac 16.113.4 +> **Last reviewed:** 2026-10-10 · **Last updated:** 2026-10-12 · **Change:** minor (ribbon and menu Paste paste the system clipboard like Cmd/Ctrl+V; regression test for the Paste button freeze, #273; Paste button gets an accessible name) · **Target:** Microsoft Excel (Microsoft 365), Excel for Mac 16.113.4 Interaction, controls and feel, against Excel. Part of [target-app-parity.md](target-app-parity.md). **UI/UX fidelity: ~55% ready, 50–80 h** (estimated). Ribbon command coverage is measured @@ -160,6 +160,7 @@ scrollbar bugs 2–4 · pixel/feel pass against observed Excel 7–12. | Date | Change | Summary | |---|---|---| +| 2026-10-12 | minor | Paste from the ribbon (button and options menu), the cell and header context menus and the macOS menu bar's Edit menu pastes what is on the system clipboard, as Cmd/Ctrl+V does, instead of only GridCraft's own copy (the web build, which can't read the clipboard on request, still pastes GridCraft's copy). The desktop app reads the clipboard off the UI thread and gives up after 5 s, so a clipboard source that never answers can't freeze it. Regression test for the Paste button click freeze in 0.5.0 (#273, fixed by #272); the Paste button gets an accessible "Paste" name for screen readers | | 2026-10-11 | minor | AutoFit accounts for table and ordinary filter dropdowns, including low zoom; header text excludes the button area | | 2026-10-11 | minor | Outline band for grouped rows and columns, Show / Hide Detail on the Data tab, numpad navigation keys in shortcuts (#373) | | 2026-10-11 | minor | Shift+Space / Ctrl+Space no longer start an edit; Control+Space selects the column on Mac (#338) |