From d7f8ba8b57541552d1337c82867996af7a30367c Mon Sep 17 00:00:00 2001 From: Marco Arnulfo Date: Wed, 14 Oct 2026 20:13:56 +0200 Subject: [PATCH] fix(forms): Tab and Space reach check boxes and radio buttons, and a click focuses them (fixes #2223) --- crates/ui-egui/src/canvas.rs | 8 +- crates/ui-egui/src/forms_ui.rs | 137 +++++++++++++++++++++++++---- crates/ui-egui/tests/forms.rs | 155 +++++++++++++++++++++++++++++++-- parity/acrobat-features.toml | 4 +- 4 files changed, 274 insertions(+), 30 deletions(-) diff --git a/crates/ui-egui/src/canvas.rs b/crates/ui-egui/src/canvas.rs index 00ff0eb3a5..bab4d5d476 100644 --- a/crates/ui-egui/src/canvas.rs +++ b/crates/ui-egui/src/canvas.rs @@ -2206,9 +2206,13 @@ pub fn shortcuts(view: &mut DocView, ctx: &egui::Context, speed: crate::scroll_s view.find_step(true); } } - if ctx.egui_wants_keyboard_input() { + // A focused check box or radio button holds egui's keyboard focus only for Space, and a + // radio button for the arrows that move within its group: the other page keys still work. + let form_box = view.forms.keys.filter(|(id, _)| ctx.memory(|m| m.focused()) == Some(*id)); + if ctx.egui_wants_keyboard_input() && form_box.is_none() { return; } + let arrows = !form_box.is_some_and(|(_, radio)| radio); // Rotation first: egui matches ⌘+ loosely with respect to Shift. if pressed(KeyboardShortcut::new(Modifiers::COMMAND | Modifiers::SHIFT, Key::Plus)) || pressed(KeyboardShortcut::new(Modifiers::COMMAND | Modifiers::SHIFT, Key::Equals)) @@ -2255,7 +2259,7 @@ pub fn shortcuts(view: &mut DocView, ctx: &egui::Context, speed: crate::scroll_s if pressed(cmd(Key::CloseBracket)) { view.view_history(true); } - let key = |k| ctx.input(|i| i.key_pressed(k)); + let key = |k| (arrows || !matches!(k, Key::ArrowLeft | Key::ArrowRight | Key::ArrowUp | Key::ArrowDown)) && ctx.input(|i| i.key_pressed(k)); if key(Key::Escape) { if view.camera.is_some() || view.camera_done.is_some() { view.pending_action = Some(ViewAction::CancelCamera); diff --git a/crates/ui-egui/src/forms_ui.rs b/crates/ui-egui/src/forms_ui.rs index 8020e868b2..bf2edf5d70 100644 --- a/crates/ui-egui/src/forms_ui.rs +++ b/crates/ui-egui/src/forms_ui.rs @@ -1,10 +1,10 @@ //! Filling in forms on the page (execution plan M6; Acrobat fills fields in every viewing mode). //! //! Text fields open an in-place editor over the widget (Enter or clicking away commits, Escape -//! cancels, Tab moves to the next field). Check boxes and radio buttons toggle on click. List -//! boxes are directly selectable in-place on the page (clicking picks an option; Ctrl-click -//! toggles on multi-select lists). Combo boxes open a list of their options. Every change is one -//! undoable engine edit. +//! cancels, Tab moves to the next field). Check boxes and radio buttons toggle on click, or on +//! Space once Tab or a click has focused them. List boxes are directly selectable in-place on the +//! page (clicking picks an option; Ctrl-click toggles on multi-select lists). Combo boxes open a +//! list of their options. Every change is one undoable engine edit. use std::collections::HashSet; @@ -57,6 +57,13 @@ pub struct FormView { pub notice: Option, /// A push button was clicked: (its field name, what it does). pub button: Option<(String, pdfcraft_engine::form_scripts::ButtonAction)>, + /// The focused check box or radio button was clicked. It toggles once no other edit is + /// queued: the text the click committed goes first, and the toggle is dropped if that text + /// is refused and its field reopens. + pub(crate) toggle: bool, + /// The widget holding egui's keyboard focus for the focused check box or radio button, and + /// whether it takes the arrows (radio buttons). The page keys still work around it. + pub(crate) keys: Option<(egui::Id, bool)>, } /// A form message for the app to show, with document data kept separate so the visible @@ -93,6 +100,29 @@ fn open_focus(f: &FormField, widget: usize) -> Focus { } } +/// Select a radio button, if it isn't selected already. +fn select_edit(f: &FormField, w: &FormWidget) -> Option { + (w.on_state.as_ref() != f.value.first()).then(|| Edit::SetFieldValue { name: f.name.clone(), value: FieldValue::Radio(w.on_state.clone()) }) +} + +/// What clicking a check box or radio button does. +fn toggle_edit(f: &FormField, w: &FormWidget) -> Option { + let value = match f.kind { + FormFieldKind::CheckBox => FieldValue::Check(f.value.is_empty()), + FormFieldKind::Radio => { + let on = w.on_state.clone(); + // Clicking the selected button again turns it off unless the group forbids it. + let value = if f.value.first() == on.as_ref() && !f.has(field_flags::NO_TOGGLE_TO_OFF) { None } else { on }; + if value.as_ref() == f.value.first() { + return None; + } + FieldValue::Radio(value) + } + _ => return None, + }; + Some(Edit::SetFieldValue { name: f.name.clone(), value }) +} + /// Where the pointer is relative to one page's form widgets. #[derive(Clone, Copy, Debug, PartialEq, Eq)] pub(crate) enum FieldPointer { @@ -159,18 +189,14 @@ pub(crate) fn page_input( None => view.forms.notice = Some(FormNotice::NoAction(f.name.clone())), }, FormFieldKind::Signature => view.sign.field = Some(f.name.clone()), - FormFieldKind::CheckBox => { - view.forms.focus = None; - view.pending_edit = Some(Edit::SetFieldValue { name: f.name.clone(), value: FieldValue::Check(f.value.is_empty()) }); - } - FormFieldKind::Radio => { - view.forms.focus = None; - let on = w.on_state.clone(); - // Clicking the selected button again turns it off unless the group forbids it. - let value = if f.value.first() == on.as_ref() && !f.has(field_flags::NO_TOGGLE_TO_OFF) { None } else { on }; - if value.as_ref() != f.value.first() { - view.pending_edit = Some(Edit::SetFieldValue { name: f.name.clone(), value: FieldValue::Radio(value) }); + FormFieldKind::CheckBox | FormFieldKind::Radio => { + // The box takes the focus, as in Acrobat, so Tab goes on from it (#2223). The overlay + // toggles it, after the edit for any text this commits. + if view.forms.focus.as_ref().is_none_or(|x| x.name != f.name || x.widget != wi) { + commit(view, form); + view.forms.focus = Some(open_focus(f, wi)); } + view.forms.toggle = true; } // The press already committed the previous field and opened this one (above), unless // press and release came in one frame. Committing again would re-run a validation that @@ -468,6 +494,8 @@ pub(crate) fn overlay(ctx: &egui::Context, view: &mut DocView, info: &DocInfo, f if !ctx.input(|i| i.pointer.primary_down()) { view.forms.pressed = false; } + let toggle = std::mem::take(&mut view.forms.toggle); + view.forms.keys = None; let focus = view.forms.focus.clone()?; let Some(f) = form.iter().find(|f| f.name == focus.name) else { view.forms.focus = None; @@ -475,7 +503,15 @@ pub(crate) fn overlay(ctx: &egui::Context, view: &mut DocView, info: &DocInfo, f }; let w = f.widgets.get(focus.widget)?; let page = w.page?; - let xf = view.page_xform(page)?; + let Some(xf) = view.page_xform(page) else { + // A check box or radio button scrolled out of view lets go (it has no draft to keep): + // taking the keyboard back when its page returns would turn scrolling arrows into + // choices, or pull the keyboard from wherever the user went meanwhile. + if matches!(f.kind, FormFieldKind::CheckBox | FormFieldKind::Radio) { + view.forms.focus = None; + } + return None; + }; let rect = widget_rect(&xf, info, page, w.rect); let zoom = xf.rect.width() / xf.pw.max(1.0); let t = Tokens::get(ctx); @@ -628,6 +664,69 @@ pub(crate) fn overlay(ctx: &egui::Context, view: &mut DocView, info: &DocInfo, f close = true; } } + FormFieldKind::CheckBox | FormFieldKind::Radio => { + // Nothing is drawn over the box, but a widget there holds egui's keyboard focus, so + // Space, Tab and the arrows come here instead of turning or scrolling the page. + let id = egui::Id::new(("form-field", view.id.0, &f.name, focus.widget)); + let mut has_focus = false; + // Not interactable, so clicks still reach the page under it. + let area = egui::Area::new(egui::Id::new(("form-toggle", view.id.0))).order(egui::Order::Foreground).interactable(false); + area.fixed_pos(rect.min).show(ctx, |ui| { + let r = ui.interact(rect, id, egui::Sense::focusable_noninteractive()); + if let Some(fx) = view.forms.focus.as_mut() + && std::mem::take(&mut fx.request_focus) + { + r.request_focus(); + } + let filter = egui::EventFilter { tab: true, horizontal_arrows: true, vertical_arrows: true, escape: false }; + ui.memory_mut(|m| m.set_focus_lock_filter(id, filter)); + has_focus = r.has_focus(); + }); + view.forms.keys = Some((id, f.kind == FormFieldKind::Radio)); + let (esc, tab, shift) = ctx.input(|i| (i.key_pressed(egui::Key::Escape), i.key_pressed(egui::Key::Tab), i.modifiers.shift)); + let space = has_focus && ctx.input_mut(|i| i.consume_key(egui::Modifiers::NONE, egui::Key::Space)); + let arrow = has_focus && f.kind == FormFieldKind::Radio; + let step = ctx.input(|i| { + if !arrow { + None + } else if i.key_pressed(egui::Key::ArrowRight) || i.key_pressed(egui::Key::ArrowDown) { + Some(true) + } else if i.key_pressed(egui::Key::ArrowLeft) || i.key_pressed(egui::Key::ArrowUp) { + Some(false) + } else { + None + } + }); + let clicked_away = ctx.input(|i| i.pointer.any_click() && !i.pointer.hover_pos().is_some_and(|p| rect.contains(p))); + if esc || clicked_away { + close = true; + } else if tab { + next = Some(!shift); + } else if toggle && view.pending_edit.is_some() { + view.forms.toggle = true; + } else if toggle { + edit = toggle_edit(f, w); + } else if space { + // Space toggles a check box and selects a radio button, as in browsers: it never + // turns the selected button off. + edit = if f.kind == FormFieldKind::Radio { select_edit(f, w) } else { toggle_edit(f, w) }; + } else if let Some(forward) = step { + // The arrows move to the group's next or previous button and select it, wrapping. + let stops: Vec = f.widgets.iter().enumerate().filter(|(_, w)| w.page.is_some() && !w.hidden).map(|(i, _)| i).collect(); + if let Some(pos) = stops.iter().position(|&i| i == focus.widget) { + let k = if forward { (pos + 1) % stops.len() } else { (pos + stops.len() - 1) % stops.len() }; + if let Some((wi, target)) = stops.get(k).and_then(|&wi| Some((wi, f.widgets.get(wi)?))) { + view.forms.focus = Some(open_focus(f, wi)); + edit = select_edit(f, target); + if let Some(p) = target.page + && view.page_screen_rect(p).is_none() + { + view.go_to_page(p); + } + } + } + } + } _ => close = true, } if close { @@ -636,11 +735,11 @@ pub(crate) fn overlay(ctx: &egui::Context, view: &mut DocView, info: &DocInfo, f if let Some(forward) = next { commit(view, form); let pending = view.pending_edit.take(); - // The next widget that takes typing or a choice, in the document's tab order (each - // page's /Tabs: rows, columns or structure), wrapping. + // The next field that can be filled in, check boxes and radio buttons included (#2223), in + // the document's tab order (each page's /Tabs: rows, columns or structure), wrapping. let mut order: Vec<(usize, &FormField, usize)> = form .iter() - .filter(|x| fillable(x) && matches!(x.kind, FormFieldKind::Text | FormFieldKind::Combo | FormFieldKind::List)) + .filter(|x| fillable(x)) .flat_map(|x| x.widgets.iter().enumerate().filter(|(_, w)| w.page.is_some() && !w.hidden).map(move |(wi, w)| (w.tab, x, wi))) .collect(); order.sort_by_key(|o| o.0); diff --git a/crates/ui-egui/tests/forms.rs b/crates/ui-egui/tests/forms.rs index 96a0a6a11e..fa9732c481 100644 --- a/crates/ui-egui/tests/forms.rs +++ b/crates/ui-egui/tests/forms.rs @@ -54,12 +54,8 @@ fn typing_into_a_text_field_and_tabbing_on() { h.key_press(egui::Key::Tab); h.run_steps(4); assert_eq!(value(&h, "name"), ["Ada Lovelace"]); - assert_eq!( - h.state().views[0].forms.focus.as_ref().map(|f| f.name.as_str()), - Some("country"), - "Tab moves to the next field that takes typing or a choice" - ); - // The country list is open; Escape closes it. Escape also abandons a draft. + assert_eq!(h.state().views[0].forms.focus.as_ref().map(|f| f.name.as_str()), Some("agree"), "Tab moves to the next field"); + // Escape leaves the check box. It also abandons a draft. h.key_press(egui::Key::Escape); h.run_steps(2); click_field(&mut h, "city", 0); @@ -96,6 +92,144 @@ fn check_boxes_radios_and_choices() { assert_eq!(value(&h, "country"), ["fr"]); } +fn focused(h: &Harness<'static, PdfCraftApp>) -> Option<(String, usize)> { + h.state().views[0].forms.focus.as_ref().map(|f| (f.name.clone(), f.widget)) +} + +/// A text field, a check box, another text field and a pair of radio buttons, one under the +/// other, then two empty pages. +fn check_box_harness() -> Harness<'static, PdfCraftApp> { + let bytes = pdf_from_objects(&[ + "<< /Type /Catalog /Pages 2 0 R /AcroForm << /Fields [4 0 R 5 0 R 6 0 R 10 0 R] /DA (/Helv 0 Tf 0 g) /DR << /Font << /Helv 7 0 R >> >> >> >>", + "<< /Type /Pages /Kids [3 0 R 9 0 R 13 0 R] /Count 3 /MediaBox [0 0 300 400] >>", + "<< /Type /Page /Parent 2 0 R /Annots [4 0 R 5 0 R 6 0 R 11 0 R 12 0 R] >>", + "<< /Type /Annot /Subtype /Widget /FT /Tx /T (first) /Rect [20 350 280 372] /P 3 0 R /F 4 >>", + "<< /Type /Annot /Subtype /Widget /FT /Btn /T (agree) /V /Off /AS /Off /Rect [20 310 36 326] /P 3 0 R /F 4 /AP << /N << /Yes 8 0 R /Off 8 0 R >> >> >>", + "<< /Type /Annot /Subtype /Widget /FT /Tx /T (last) /Rect [20 270 280 292] /P 3 0 R /F 4 >>", + "<< /Type /Font /Subtype /Type1 /BaseFont /Helvetica /Encoding /WinAnsiEncoding >>", + "<< /Type /XObject /Subtype /Form /BBox [0 0 16 16] /Length 0 >>\nstream\n\nendstream", + "<< /Type /Page /Parent 2 0 R >>", + "<< /FT /Btn /T (pick) /Ff 32768 /V /Off /Kids [11 0 R 12 0 R] >>", + "<< /Type /Annot /Subtype /Widget /Parent 10 0 R /AS /Off /Rect [20 230 36 246] /P 3 0 R /F 4 /AP << /N << /A 8 0 R /Off 8 0 R >> >> >>", + "<< /Type /Annot /Subtype /Widget /Parent 10 0 R /AS /Off /Rect [60 230 76 246] /P 3 0 R /F 4 /AP << /N << /B 8 0 R /Off 8 0 R >> >> >>", + "<< /Type /Page /Parent 2 0 R >>", + ]); + let mut h = Harness::builder().with_size(egui::vec2(1400.0, 900.0)).build_eframe(|_cc| { + let mut app = PdfCraftApp::new(); + app.set_option("language", "en").unwrap(); + app.open_bytes("agree.pdf", None, bytes).unwrap(); + app.set_option("left", "closed").unwrap(); + app.set_option("zoom", "150").unwrap(); + app + }); + h.run_steps(6); + h +} + +#[test] +fn tab_and_space_fill_in_a_check_box_between_text_fields() { + // Issue #2223: Tab went from `first` straight to `last`, and Space could not tick `agree`. + let mut h = check_box_harness(); + click_field(&mut h, "first", 0); + h.event(egui::Event::Text("Ada".into())); + h.run_steps(1); + h.key_press(egui::Key::Tab); + h.run_steps(3); + assert_eq!(value(&h, "first"), ["Ada"]); + assert_eq!(focused(&h), Some(("agree".into(), 0)), "Tab stops on the check box"); + assert!(h.ctx.egui_wants_keyboard_input(), "the check box has the keyboard, not the page"); + h.key_press(egui::Key::Space); + h.run_steps(3); + assert_eq!(value(&h, "agree"), ["Yes"], "Space ticks it"); + assert_eq!(focused(&h), Some(("agree".into(), 0)), "and it keeps the focus"); + h.key_press(egui::Key::Space); + h.run_steps(3); + assert!(value(&h, "agree").is_empty(), "Space again clears it"); + h.key_press(egui::Key::Tab); + h.run_steps(3); + assert_eq!(focused(&h), Some(("last".into(), 0))); + h.key_press_modifiers(egui::Modifiers::SHIFT, egui::Key::Tab); + h.run_steps(3); + assert_eq!(focused(&h), Some(("agree".into(), 0)), "Shift+Tab comes back to it"); +} + +#[test] +fn clicking_a_check_box_while_typing_keeps_the_text_and_focuses_the_box() { + // Issue #2223: the click ticked the box but left no field focused, so Tab started over. + let mut h = check_box_harness(); + click_field(&mut h, "first", 0); + h.event(egui::Event::Text("Ada".into())); + h.run_steps(1); + click_field(&mut h, "agree", 0); + assert_eq!(value(&h, "agree"), ["Yes"]); + assert_eq!(value(&h, "first"), ["Ada"], "the text typed before the click is kept"); + assert_eq!(focused(&h), Some(("agree".into(), 0)), "the click focuses the check box"); + h.key_press(egui::Key::Tab); + h.run_steps(3); + assert_eq!(focused(&h), Some(("last".into(), 0)), "so Tab goes on from there"); +} + +#[test] +fn page_keys_still_work_with_a_check_box_or_radio_button_focused() { + // The widget that takes Space for a focused box must not swallow the page keys. + let mut h = check_box_harness(); + click_field(&mut h, "agree", 0); + assert_eq!(focused(&h), Some(("agree".into(), 0))); + h.key_press(egui::Key::End); + h.run_steps(4); + assert_eq!(h.state().views[0].current, 2, "End goes to the last page"); + assert!(h.state().views[0].page_screen_rect(0).is_none(), "where the check box is out of view"); + // Scrolled away, the box lets go: back on its page, Space no longer toggles it. + h.key_press(egui::Key::Home); + h.run_steps(4); + assert_eq!(h.state().views[0].current, 0); + assert_eq!(focused(&h), None, "the box let go of the focus"); + h.key_press(egui::Key::Space); + h.run_steps(3); + assert_eq!(value(&h, "agree"), ["Yes"]); + // A radio button keeps the arrows for its group, so → selects B instead of turning the page. + click_field(&mut h, "pick", 0); + h.key_press(egui::Key::ArrowRight); + h.run_steps(3); + assert_eq!(value(&h, "pick"), ["B"]); + assert_eq!(h.state().views[0].current, 0, "the arrow stayed in the group"); + // Once scrolled away, an arrow scrolls again instead of changing the choice. + h.key_press(egui::Key::End); + h.run_steps(4); + h.key_press(egui::Key::Home); + h.run_steps(4); + h.key_press(egui::Key::ArrowUp); + h.run_steps(3); + assert_eq!((focused(&h), value(&h, "pick")), (None, vec!["B".to_string()])); +} + +#[test] +fn radio_buttons_take_tab_space_and_arrows() { + let mut h = harness(); + click_field(&mut h, "agree", 0); + h.key_press(egui::Key::Tab); + h.run_steps(3); + assert_eq!(focused(&h), Some(("size".into(), 0))); + h.key_press(egui::Key::Space); + h.run_steps(3); + assert_eq!(value(&h, "size"), ["S"], "Space selects the focused button"); + h.key_press(egui::Key::Space); + h.run_steps(3); + assert_eq!(value(&h, "size"), ["S"], "and doesn't turn it off again"); + h.key_press(egui::Key::ArrowRight); + h.run_steps(3); + assert_eq!(focused(&h), Some(("size".into(), 1)), "an arrow moves within the group"); + assert_eq!(value(&h, "size"), ["L"], "and selects the button it lands on"); + h.key_press(egui::Key::ArrowLeft); + h.run_steps(3); + assert_eq!((focused(&h), value(&h, "size")), (Some(("size".into(), 0)), vec!["S".to_string()])); + h.key_press(egui::Key::Tab); + h.run_steps(3); + h.key_press(egui::Key::Tab); + h.run_steps(3); + assert_eq!(focused(&h), Some(("country".into(), 0)), "past the group's second button"); +} + #[test] fn tabbing_into_a_filled_field_selects_it_so_typing_replaces() { let mut h = harness(); @@ -108,10 +242,17 @@ fn tabbing_into_a_filled_field_selects_it_so_typing_replaces() { h.key_press(egui::Key::Enter); h.run_steps(3); assert_eq!(value(&h, "city"), ["Paris"]); - // name → (Tab) country → (Tab) city, whose text is selected on entry. + // name → (Tab) agree → (Tab) size, both buttons → (Tab) country → (Tab) city, whose text is + // selected on entry. click_field(&mut h, "name", 0); h.key_press(egui::Key::Tab); h.run_steps(3); + assert_eq!(focused(&h), Some(("agree".into(), 0))); + for _ in 0..3 { + h.key_press(egui::Key::Tab); + h.run_steps(3); + } + assert_eq!(focused(&h), Some(("country".into(), 0))); h.key_press(egui::Key::Tab); h.run_steps(4); assert_eq!(h.state().views[0].forms.focus.as_ref().map(|f| f.name.as_str()), Some("city")); diff --git a/parity/acrobat-features.toml b/parity/acrobat-features.toml index d8fbe77ddf..db514b6da4 100644 --- a/parity/acrobat-features.toml +++ b/parity/acrobat-features.toml @@ -4428,8 +4428,8 @@ milestone = "M6" status = "shipped" commands = ["form.fields"] tools = ["form_fill"] -evidence = ["crates/ui-egui/tests/forms.rs::check_boxes_radios_and_choices", "crates/forms/src/tests.rs::check_boxes_and_radios_switch_states"] -notes = "NoToggleToOff honoured; widgets without appearances get our own" +evidence = ["crates/ui-egui/tests/forms.rs::check_boxes_radios_and_choices", "crates/forms/src/tests.rs::check_boxes_and_radios_switch_states", "crates/ui-egui/tests/forms.rs::tab_and_space_fill_in_a_check_box_between_text_fields", "crates/ui-egui/tests/forms.rs::clicking_a_check_box_while_typing_keeps_the_text_and_focuses_the_box", "crates/ui-egui/tests/forms.rs::radio_buttons_take_tab_space_and_arrows"] +notes = "NoToggleToOff honoured; widgets without appearances get our own. Tab and Shift+Tab stop on check boxes and on each radio button, Space toggles the focused one, and the arrows move within a radio group (#2223); a click focuses the box too" [[feature]] id = "form.fill-choice"