From 0e919bfbd5cb10f73b4c1115c651662d77c8c825 Mon Sep 17 00:00:00 2001 From: Johann Date: Tue, 20 Oct 2026 19:06:15 +0800 Subject: [PATCH 1/2] Fix macOS copy announcements Route Command+C in the focused document reader through the shared copy handler before Cocoa activates the Copy menu item. VoiceOver can then announce the number of copied characters instead of only the menu label. Enable the shared selection and whole-document copy path on macOS, remove the old Mac-only widening helper, and preserve native copying in other controls. Add native keyboard regression coverage for copying, selection preservation, keyboard-source cleanup, and editable-field copying. --- .../src/ui/document_manager/window.rs | 29 ---------- crates/paperback/src/ui/mac_shortcuts.rs | 21 +++++-- crates/paperback/src/ui/main_window.rs | 8 ++- .../paperback/src/ui/main_window/menu_edit.rs | 14 ++++- .../src/ui/main_window/menu_events.rs | 17 +++--- crates/paperback/src/ui/reader_input.rs | 2 +- .../paperback/tests/macos_menu_shortcuts.rs | 57 +++++++++++++++++-- 7 files changed, 94 insertions(+), 54 deletions(-) diff --git a/crates/paperback/src/ui/document_manager/window.rs b/crates/paperback/src/ui/document_manager/window.rs index 940aafcc..9ccbaea9 100644 --- a/crates/paperback/src/ui/document_manager/window.rs +++ b/crates/paperback/src/ui/document_manager/window.rs @@ -1,7 +1,5 @@ //! The loaded-window management for a document: keeping the text control's slice of a large document ahead of a forward read, compacting it when the caret runs out of text behind it or the layout changes, copying the whole document when a windowed Select All only reached a slice, and jumping to the true document edges rather than the loaded window's. Split out of the main `DocumentManager` impl; see [`super`] for the windowing model in `text_window`. -#[cfg(target_os = "macos")] -use wxdragon::clipboard::Clipboard; use wxdragon::prelude::*; use super::{DocumentManager, DocumentTab}; @@ -97,36 +95,9 @@ impl DocumentManager { tab.text_ctrl.show_position(local); } - /// Copies the current selection, widening it to the whole document when everything loaded is selected but only part of the document is. Select All can only ever reach what the control holds, which for a windowed document is a slice of the book rather than the book, so a selection covering all of it means all of it in the only sense the user has. Keyed off the selection rather than a remembered Select All, so clicking anywhere collapses it and this stops applying on its own with no flag to keep in step. - /// - /// macOS only, where the copy is the native one and this is what widens it. Elsewhere the copy runs through `menu_edit::handle_copy`, which reaches the whole document through `text_to_copy` instead. Returns whether it copied; false means the caller should let the control's own copy run. - #[cfg(target_os = "macos")] - pub fn copy_whole_document_if_all_selected(&self) -> bool { - let Some(tab) = self.active_tab() else { - return false; - }; - let doc_len = tab.session.document_len(); - // Only the widened case is handled here. Anything else is left to the control's own copy, - // which is better tested than anything this could put in its place. - let source = copy_source( - tab.text_ctrl.get_selection(), - tab.text_ctrl.get_last_position(), - tab.window.is_whole_document(doc_len), - ); - if source != CopySource::WholeDocument { - return false; - } - let text = tab.session.content(); - if text.is_empty() { - return false; - } - Clipboard::get().set_text(&text) - } - /// The text Edit > Copy puts on the clipboard: the control's selection, or the whole document /// when everything loaded is selected but only part of the document is loaded. `None` when /// nothing is selected. - #[cfg(not(target_os = "macos"))] pub fn text_to_copy(&self) -> Option { let tab = self.active_tab()?; let doc_len = tab.session.document_len(); diff --git a/crates/paperback/src/ui/mac_shortcuts.rs b/crates/paperback/src/ui/mac_shortcuts.rs index c50e5419..e020266a 100644 --- a/crates/paperback/src/ui/mac_shortcuts.rs +++ b/crates/paperback/src/ui/mac_shortcuts.rs @@ -72,7 +72,12 @@ fn shortcut_action(shortcuts: &ShortcutsConfig, key: i32, modifiers: usize) -> O }) } -pub fn install(frame: Frame, config: Rc>, from_keyboard: Rc>) { +pub fn install( + frame: Frame, + config: Rc>, + from_keyboard: Rc>, + reader_has_focus: impl Fn() -> bool + 'static, +) { let monitor = ConcreteBlock::new(move |event: *mut Object| -> *mut Object { if !frame.is_valid() || !frame.is_enabled() { return event; @@ -88,12 +93,20 @@ pub fn install(frame: Frame, config: Rc>, from_keyboard: Rc } let Some((modifiers, key)) = event_key(event) else { return event }; let action = config.try_lock().ok().and_then(|cfg| shortcut_action(&cfg.get_shortcuts(), key, modifiers)); - let Some(action) = action else { return event }; // Percentage announcements have no menu item and already run in the reader's key handler. - if action == ActionId::AnnouncePercent { + if action == Some(ActionId::AnnouncePercent) { return event; } - let id = if action == ActionId::Options { menu_ids::PREFERENCES } else { menu_ids::action_to_menu_id(action) }; + let copy_key = key == 'C' as i32 && modifiers & (COMMAND | CONTROL | OPTION | SHIFT) == COMMAND; + let id = if let Some(action) = action { + if action == ActionId::Options { menu_ids::PREFERENCES } else { menu_ids::action_to_menu_id(action) } + } else if copy_key && reader_has_focus() { + // Copy is a stock wx command, outside ActionId. Catch it before Cocoa reads + // "Copy" and let the app announce the character count instead. + menu_ids::COPY + } else { + return event; + }; let item = frame.get_menu_bar().and_then(|bar| bar.find_item(id)); if !item.is_some_and(|item| item.is_enabled()) { return event; diff --git a/crates/paperback/src/ui/main_window.rs b/crates/paperback/src/ui/main_window.rs index 8d02d444..7cf04efd 100644 --- a/crates/paperback/src/ui/main_window.rs +++ b/crates/paperback/src/ui/main_window.rs @@ -25,7 +25,6 @@ use crate::{ #[cfg(any(target_os = "linux", target_os = "windows"))] mod ipc; mod links; -#[cfg(not(target_os = "macos"))] mod menu_edit; mod menu_events; mod menu_file; @@ -132,7 +131,12 @@ impl MainWindow { ); menu_events::bind_key_source(&frame, &config, Rc::clone(&from_keyboard)); #[cfg(target_os = "macos")] - super::mac_shortcuts::install(frame, Rc::clone(&config), Rc::clone(&from_keyboard)); + { + let dm = Rc::clone(&doc_manager); + super::mac_shortcuts::install(frame, Rc::clone(&config), Rc::clone(&from_keyboard), move || { + dm.try_lock().is_ok_and(|dm| dm.active_tab().is_some_and(|tab| tab.text_ctrl.has_focus())) + }); + } #[cfg(not(target_os = "windows"))] window_events::bind_tab_cycling(&frame, &doc_manager); window_events::bind_document_events(frame, &doc_manager, live_region_label); diff --git a/crates/paperback/src/ui/main_window/menu_edit.rs b/crates/paperback/src/ui/main_window/menu_edit.rs index 0a7c315c..479d7472 100644 --- a/crates/paperback/src/ui/main_window/menu_edit.rs +++ b/crates/paperback/src/ui/main_window/menu_edit.rs @@ -1,13 +1,17 @@ -//! Edit > Copy and Edit > Select All on Windows and Linux. +//! Edit > Copy on every desktop platform, and Edit > Select All on Windows and Linux. use std::{rc::Rc, sync::Mutex}; -use patois::{nt, t}; +#[cfg(not(target_os = "macos"))] +use patois::nt; +use patois::t; use wxdragon::prelude::*; use super::DocumentManager; +#[cfg(not(target_os = "macos"))] +use crate::ui::navigation::announce_for_selection_command; use crate::ui::{ - navigation::{announce_for_command, announce_for_selection_command}, + navigation::announce_for_command, selection::{copied_announcement, copy_to_clipboard}, }; @@ -56,6 +60,7 @@ fn copy_message(outcome: CopyOutcome) -> String { /// The shortcut waits as well, which the copy's does not. Selecting is not silent: the change is /// itself reported, and a message raised the moment the command runs is spoken first and leaves that /// report hanging on the end. +#[cfg(not(target_os = "macos"))] pub fn handle_select_all(dm: &Rc>, live_region_label: StaticText, from_keyboard: bool) { let Some(count) = dm.lock().unwrap().select_all_and_count() else { return; @@ -64,6 +69,7 @@ pub fn handle_select_all(dm: &Rc>, live_region_label: Sta } /// Its own function of the count so the wording can be pinned without a document, which a test cannot build. +#[cfg(not(target_os = "macos"))] fn selected_message(count: usize) -> String { // TRANSLATORS: Announced after Select All, from the Edit menu or with Ctrl+A. The %d placeholder is replaced with the number of characters in the document. Plural form is chosen by that count. nt("%d character selected.", "%d characters selected.", u64::try_from(count).unwrap_or(0)).replacen( @@ -101,12 +107,14 @@ mod tests { } #[test] + #[cfg(not(target_os = "macos"))] fn a_select_all_says_how_many_characters_the_document_has() { assert_eq!(selected_message(42), "42 characters selected."); } /// One is read aloud by a person, not measured by a buffer. #[test] + #[cfg(not(target_os = "macos"))] fn a_select_all_of_one_character_is_not_plural() { assert_eq!(selected_message(1), "1 character selected."); } diff --git a/crates/paperback/src/ui/main_window/menu_events.rs b/crates/paperback/src/ui/main_window/menu_events.rs index 9fdb40a3..811caeda 100644 --- a/crates/paperback/src/ui/main_window/menu_events.rs +++ b/crates/paperback/src/ui/main_window/menu_events.rs @@ -11,11 +11,9 @@ use wxdragon::{prelude::*, timer::Timer}; #[cfg(target_os = "windows")] use super::HotkeyHandle; -#[cfg(not(target_os = "macos"))] -use super::menu_edit; use super::{ DocumentManager, FindDialogState, MainWindow, background, commands, dialogs, find, get_update_channel, help, menu, - menu_file, menu_go, menu_ids, menu_tools, menu_view, sleep_timer, update_title_from_manager, updater, + menu_edit, menu_file, menu_go, menu_ids, menu_tools, menu_view, sleep_timer, update_title_from_manager, updater, }; use crate::ui::navigation::announce_for_command; @@ -217,15 +215,14 @@ impl MainWindow { help::handle_donate(&frame_copy); } menu_ids::COPY => { - // On macOS the native copy runs unless the copy widened to the whole document. + // Other macOS controls keep Cocoa's responder-chain copy. Only the reader + // needs our document-window handling and character-count announcement. #[cfg(target_os = "macos")] - { - let widened = dm.lock().unwrap().copy_whole_document_if_all_selected(); - if !widened { - event.skip(true); - } + if !dm.lock().unwrap().active_tab().is_some_and(|tab| tab.text_ctrl.has_focus()) { + event.skip(true); + return; } - #[cfg(not(target_os = "macos"))] + event.skip(false); menu_edit::handle_copy(&dm, live_region_label, from_keyboard); } #[cfg(not(target_os = "macos"))] diff --git a/crates/paperback/src/ui/reader_input.rs b/crates/paperback/src/ui/reader_input.rs index c52492ef..6a1dd2db 100644 --- a/crates/paperback/src/ui/reader_input.rs +++ b/crates/paperback/src/ui/reader_input.rs @@ -132,7 +132,7 @@ pub(super) fn build_text_ctrl( } // Ctrl+C and Ctrl+A are intercepted here because the reading control consumes them in its own window procedure, before wx consults the accelerator table, so neither ever becomes a menu command and the handlers Edit > Copy and Edit > Select All reach never run for the keystroke. Raising the same command a click produces is what makes the two ways of doing either behave identically - for the copy that is the announcement and the widening to the whole document, and for the select all it is the announcement alone, without which the command says nothing at all. // The mark is set here rather than by `bind_key_source`'s CHAR_HOOK, which resolves a key through the ActionIds and finds nothing for a wxWidgets stock command; without it the announcement is deferred as though a menu had been clicked, and read over by the focus chain returning to the book. It is cleared again when the command goes unhandled, so a chord that reaches nothing cannot leave the mark set for the next click. - // macOS leaves both to the native Edit menu, which is reached before this handler. + // macOS handles Copy in `mac_shortcuts` before Cocoa's Edit menu; Select All stays native. #[cfg(not(target_os = "macos"))] if let Some(stock_id) = stock_edit_command(key, kbd.control_down(), kbd.alt_down(), kbd.shift_down()) { kbd.event.skip(false); diff --git a/crates/paperback/tests/macos_menu_shortcuts.rs b/crates/paperback/tests/macos_menu_shortcuts.rs index 57a7ff1e..88ef8c83 100644 --- a/crates/paperback/tests/macos_menu_shortcuts.rs +++ b/crates/paperback/tests/macos_menu_shortcuts.rs @@ -85,6 +85,16 @@ mod native { pub const CONTROL: usize = 1 << 18; pub const OPTION: usize = 1 << 19; + pub fn activate(frame: Frame) { + unsafe { + let app: *mut Object = msg_send![class!(NSApplication), sharedApplication]; + let _: () = msg_send![app, activateIgnoringOtherApps: true]; + let view = frame.get_handle().cast::(); + let window: *mut Object = msg_send![view, window]; + let _: () = msg_send![window, makeKeyAndOrderFront: std::ptr::null_mut::()]; + } + } + pub fn send_key(frame: impl WxWidget + Copy, text: &str, modifiers: usize, hardware_key: u16) { let text = CString::new(text).unwrap(); unsafe { @@ -133,7 +143,7 @@ fn main() { sync::Mutex, }; - use native::{COMMAND, CONTROL, OPTION, SHIFT, expect_command, send_key}; + use native::{COMMAND, CONTROL, OPTION, SHIFT, activate, expect_command, send_key}; use paperback_core::config::{ActionId, ConfigManager, KeyChord}; use wxdragon::prelude::*; @@ -181,7 +191,16 @@ fn main() { let received_for_menu = Rc::clone(&received); let source_for_menu = Rc::clone(&from_keyboard); frame.on_menu(move |event| { + if event.get_id() == menu_ids::COPY && !text.has_focus() { + event.skip(true); + return; + } received_for_menu.borrow_mut().push((event.get_id(), source_for_menu.replace(false))); + if event.get_id() == menu_ids::COPY { + event.skip(false); + assert!(wxdragon::clipboard::Clipboard::get().set_text(&text.get_string_selection())); + return; + } let position = text.get_insertion_point(); let result = match event.get_id() { menu_ids::NEXT_LINK => Some(session_for_menu.navigate_link(position, false, true)), @@ -195,6 +214,7 @@ fn main() { } }); let menu = Menu::builder() + .append_item(menu_ids::COPY, "Copy\tCtrl+C", "") .append_item(menu_ids::FIND_NEXT, "Find Next\tCtrl+G", "") .append_item(menu_ids::FIND_PREVIOUS, "Find Previous\tCtrl+Shift+G", "") .append_item(menu_ids::NEXT_HEADING, "Next Heading\tH", "") @@ -205,7 +225,7 @@ fn main() { .append_item(menu_ids::PLAY_PAUSE_AUDIO, "Play/Pause\tRawCtrl+Space", "") .build(); frame.set_menu_bar(MenuBar::builder().append(menu, "Commands").build()); - mac_shortcuts::install(frame, Rc::clone(&config), Rc::clone(&from_keyboard)); + mac_shortcuts::install(frame, Rc::clone(&config), Rc::clone(&from_keyboard), move || text.has_focus()); frame.show(true); text.set_focus(); expect_command(&received, frame, "g", COMMAND, 5, menu_ids::FIND_NEXT); @@ -341,9 +361,36 @@ fn main() { config.lock().unwrap().set_shortcuts(&shortcuts); expect_command(&received, frame, "?", COMMAND | SHIFT, 44, menu_ids::NEXT_HEADING); assert_eq!(bar.find_item(menu_ids::FIND_NEXT).unwrap().get_label(), "Find Next\tCtrl+G"); - passed_for_app.set(true); - frame.close(true); - wxdragon::call_after(Box::new(|| wxdragon::app::get_app_instance().unwrap().exit_main_loop())); + // Copy needs real focus, which AppKit establishes after application initialization. + let timer_holder = Rc::new(RefCell::new(None)); + let holder = Rc::clone(&timer_holder); + let timer = wxdragon::timer::Timer::new(&frame); + timer.on_tick(move |_| { + holder.borrow_mut().take(); + activate(frame); + text.set_focus(); + text.set_selection(0, 5); + assert!(text.has_focus(), "the reader must have focus before testing Copy"); + assert!(frame.get_menu_bar().unwrap().find_item(menu_ids::COPY).unwrap().is_enabled()); + expect_command(&received, frame, "c", COMMAND, 8, menu_ids::COPY); + assert_eq!(wxdragon::clipboard::Clipboard::get().get_text().as_deref(), Some("Start")); + assert_eq!(text.get_selection(), (0, 5), "copy must preserve the reader's selection"); + assert!(!from_keyboard.get(), "copy must clear its keyboard source"); + let edit = TextCtrl::builder(&panel).with_value("Native copy").build(); + edit.set_focus(); + edit.set_selection(0, 6); + received.borrow_mut().clear(); + send_key(frame, "c", COMMAND, 8); + assert!(received.borrow().is_empty(), "editable text must keep native copy"); + assert_eq!(wxdragon::clipboard::Clipboard::get().get_text().as_deref(), Some("Native")); + edit.destroy(); + text.set_focus(); + passed_for_app.set(true); + frame.close(true); + wxdragon::call_after(Box::new(|| wxdragon::app::get_app_instance().unwrap().exit_main_loop())); + }); + assert!(timer.start(100, true)); + *timer_holder.borrow_mut() = Some(timer); }) .unwrap(); std::fs::remove_file(config_path).unwrap(); From b9df5973f65c7d12cd8cc48ddddc1c75736acd83 Mon Sep 17 00:00:00 2001 From: Quin Gillespie Date: Wed, 21 Oct 2026 13:28:01 -0600 Subject: [PATCH 2/2] Test macOS Copy routing without needing real window focus, which CI runners never give --- .../paperback/tests/macos_menu_shortcuts.rs | 66 +++++++------------ 1 file changed, 23 insertions(+), 43 deletions(-) diff --git a/crates/paperback/tests/macos_menu_shortcuts.rs b/crates/paperback/tests/macos_menu_shortcuts.rs index 88ef8c83..94994e95 100644 --- a/crates/paperback/tests/macos_menu_shortcuts.rs +++ b/crates/paperback/tests/macos_menu_shortcuts.rs @@ -85,16 +85,6 @@ mod native { pub const CONTROL: usize = 1 << 18; pub const OPTION: usize = 1 << 19; - pub fn activate(frame: Frame) { - unsafe { - let app: *mut Object = msg_send![class!(NSApplication), sharedApplication]; - let _: () = msg_send![app, activateIgnoringOtherApps: true]; - let view = frame.get_handle().cast::(); - let window: *mut Object = msg_send![view, window]; - let _: () = msg_send![window, makeKeyAndOrderFront: std::ptr::null_mut::()]; - } - } - pub fn send_key(frame: impl WxWidget + Copy, text: &str, modifiers: usize, hardware_key: u16) { let text = CString::new(text).unwrap(); unsafe { @@ -143,7 +133,7 @@ fn main() { sync::Mutex, }; - use native::{COMMAND, CONTROL, OPTION, SHIFT, activate, expect_command, send_key}; + use native::{COMMAND, CONTROL, OPTION, SHIFT, expect_command, send_key}; use paperback_core::config::{ActionId, ConfigManager, KeyChord}; use wxdragon::prelude::*; @@ -188,10 +178,13 @@ fn main() { } activated_for_key.borrow_mut().push((enter, result)); }); + // CI runners never make the test window key, so the reader's focus is stood in for rather than read from AppKit. + let reader_focused = Rc::new(Cell::new(true)); + let reader_focused_for_menu = Rc::clone(&reader_focused); let received_for_menu = Rc::clone(&received); let source_for_menu = Rc::clone(&from_keyboard); frame.on_menu(move |event| { - if event.get_id() == menu_ids::COPY && !text.has_focus() { + if event.get_id() == menu_ids::COPY && !reader_focused_for_menu.get() { event.skip(true); return; } @@ -225,7 +218,10 @@ fn main() { .append_item(menu_ids::PLAY_PAUSE_AUDIO, "Play/Pause\tRawCtrl+Space", "") .build(); frame.set_menu_bar(MenuBar::builder().append(menu, "Commands").build()); - mac_shortcuts::install(frame, Rc::clone(&config), Rc::clone(&from_keyboard), move || text.has_focus()); + let reader_focused_for_shortcuts = Rc::clone(&reader_focused); + mac_shortcuts::install(frame, Rc::clone(&config), Rc::clone(&from_keyboard), move || { + reader_focused_for_shortcuts.get() + }); frame.show(true); text.set_focus(); expect_command(&received, frame, "g", COMMAND, 5, menu_ids::FIND_NEXT); @@ -361,36 +357,20 @@ fn main() { config.lock().unwrap().set_shortcuts(&shortcuts); expect_command(&received, frame, "?", COMMAND | SHIFT, 44, menu_ids::NEXT_HEADING); assert_eq!(bar.find_item(menu_ids::FIND_NEXT).unwrap().get_label(), "Find Next\tCtrl+G"); - // Copy needs real focus, which AppKit establishes after application initialization. - let timer_holder = Rc::new(RefCell::new(None)); - let holder = Rc::clone(&timer_holder); - let timer = wxdragon::timer::Timer::new(&frame); - timer.on_tick(move |_| { - holder.borrow_mut().take(); - activate(frame); - text.set_focus(); - text.set_selection(0, 5); - assert!(text.has_focus(), "the reader must have focus before testing Copy"); - assert!(frame.get_menu_bar().unwrap().find_item(menu_ids::COPY).unwrap().is_enabled()); - expect_command(&received, frame, "c", COMMAND, 8, menu_ids::COPY); - assert_eq!(wxdragon::clipboard::Clipboard::get().get_text().as_deref(), Some("Start")); - assert_eq!(text.get_selection(), (0, 5), "copy must preserve the reader's selection"); - assert!(!from_keyboard.get(), "copy must clear its keyboard source"); - let edit = TextCtrl::builder(&panel).with_value("Native copy").build(); - edit.set_focus(); - edit.set_selection(0, 6); - received.borrow_mut().clear(); - send_key(frame, "c", COMMAND, 8); - assert!(received.borrow().is_empty(), "editable text must keep native copy"); - assert_eq!(wxdragon::clipboard::Clipboard::get().get_text().as_deref(), Some("Native")); - edit.destroy(); - text.set_focus(); - passed_for_app.set(true); - frame.close(true); - wxdragon::call_after(Box::new(|| wxdragon::app::get_app_instance().unwrap().exit_main_loop())); - }); - assert!(timer.start(100, true)); - *timer_holder.borrow_mut() = Some(timer); + text.set_selection(0, 5); + assert!(bar.find_item(menu_ids::COPY).unwrap().is_enabled()); + expect_command(&received, frame, "c", COMMAND, 8, menu_ids::COPY); + assert_eq!(wxdragon::clipboard::Clipboard::get().get_text().as_deref(), Some("Start")); + assert_eq!(text.get_selection(), (0, 5), "copy must preserve the reader's selection"); + assert!(!from_keyboard.get(), "copy must clear its keyboard source"); + reader_focused.set(false); + received.borrow_mut().clear(); + send_key(frame, "c", COMMAND, 8); + assert!(received.borrow().is_empty(), "copy outside the reader must stay native"); + assert!(!from_keyboard.get(), "a native copy must not leave the keyboard source set"); + passed_for_app.set(true); + frame.close(true); + wxdragon::call_after(Box::new(|| wxdragon::app::get_app_instance().unwrap().exit_main_loop())); }) .unwrap(); std::fs::remove_file(config_path).unwrap();