From cced83419063633cad835d9d72f9e9eadeb0bebe Mon Sep 17 00:00:00 2001 From: Marco Arnulfo <103679419+marcoarnulfo@users.noreply.github.com> Date: Tue, 13 Oct 2026 06:36:15 +0200 Subject: [PATCH] fix(ui): a rule skipped by hand stays with its document (#1301) (#1445) Rules skipped in the accessibility results were kept in one set for the whole app, and every check applied it. With the options dialog turned off, skipping Tagged PDF in one document made the next document's check report it as Skipped, its findings cleared, in the panel and the saved report. The set is now keyed by document: Skip Rule records the rule for the document whose report is shown, and a check applies only that document's skips. Start Checking in the options dialog still starts over, for the document it checks only. Adds a_rule_skipped_in_one_document_is_not_skipped_in_another in tests/accessibility.rs, which fails before this change. --- crates/ui-egui/src/a11y_ui.rs | 14 +++---- crates/ui-egui/src/dialogs.rs | 4 +- crates/ui-egui/src/lib.rs | 3 +- crates/ui-egui/tests/accessibility.rs | 54 +++++++++++++++++++++++++++ 4 files changed, 66 insertions(+), 9 deletions(-) diff --git a/crates/ui-egui/src/a11y_ui.rs b/crates/ui-egui/src/a11y_ui.rs index 0fc8e2b2..70d14d0d 100644 --- a/crates/ui-egui/src/a11y_ui.rs +++ b/crates/ui-egui/src/a11y_ui.rs @@ -364,7 +364,7 @@ impl PdfCraftApp { }; // Rules skipped by hand stay skipped when checking again. let mut report = report; - for r in report.results.iter_mut().filter(|r| self.a11y_skipped.contains(&r.rule)) { + for r in report.results.iter_mut().filter(|r| self.a11y_skipped.contains(&(id, r.rule))) { r.status = Status::Skipped; r.findings.clear(); } @@ -448,12 +448,12 @@ impl PdfCraftApp { } } PanelAction::Skip(rule) => { - self.a11y_skipped.insert(rule); - if let Some((_, r)) = self.a11y.report.as_mut() - && let Some(x) = r.results.iter_mut().find(|x| x.rule == rule) - { - x.status = Status::Skipped; - x.findings.clear(); + if let Some((id, r)) = self.a11y.report.as_mut() { + self.a11y_skipped.insert((*id, rule)); + if let Some(x) = r.results.iter_mut().find(|x| x.rule == rule) { + x.status = Status::Skipped; + x.findings.clear(); + } } } PanelAction::Fix(rule) => { diff --git a/crates/ui-egui/src/dialogs.rs b/crates/ui-egui/src/dialogs.rs index 9de94341..308a88c1 100644 --- a/crates/ui-egui/src/dialogs.rs +++ b/crates/ui-egui/src/dialogs.rs @@ -1333,7 +1333,9 @@ fn dialogs(app: &mut PdfCraftApp, ctx: &egui::Context) { app.run_compare(); } if a11y_now { - app.a11y_skipped.clear(); + if let Some((_, id)) = app.active_ids() { + app.a11y_skipped.retain(|(doc, _)| *doc != id); + } app.run_accessibility_check(); } } diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index ff84c3ee..fe861eda 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -647,7 +647,8 @@ pub struct PdfCraftApp { pub doc_js: js_ui::DocJsDraft, pub a11y: a11y_ui::A11yState, pub(crate) tags_state: tags_ui::State, - pub a11y_skipped: std::collections::BTreeSet, + /// Rules skipped by hand, for the document they were skipped in. + pub a11y_skipped: std::collections::HashSet<(DocId, pdfcraft_engine::a11y::Rule)>, pub alt_draft: a11y_ui::AltDraft, /// List the OS key store's signing identities among the digital IDs (the desktop app). pub os_key_store_ids: bool, diff --git a/crates/ui-egui/tests/accessibility.rs b/crates/ui-egui/tests/accessibility.rs index cc235c9f..b050dd07 100644 --- a/crates/ui-egui/tests/accessibility.rs +++ b/crates/ui-egui/tests/accessibility.rs @@ -92,6 +92,60 @@ fn checking_fixing_skipping_and_reporting() { assert!(html.contains("Accessibility Report") && html.contains("Skipped")); } +/// #1301: a rule skipped by hand is skipped for that document only. Another document checked +/// without the options dialog reports its own result, and the first keeps its skip, even after +/// the other is checked again through the dialog. +#[test] +fn a_rule_skipped_in_one_document_is_not_skipped_in_another() { + let mut h = Harness::builder().with_size(egui::vec2(1400.0, 900.0)).build_eframe(move |_cc| { + let mut app = PdfCraftApp::new(); + app.set_option("language", "en").unwrap(); + app.open_bytes("a.pdf", None, FIXTURE.to_vec()).unwrap(); + app.open_bytes("b.pdf", None, FIXTURE.to_vec()).unwrap(); + app.active = Some(0); + app.a11y_options.show_dialog = false; + app + }); + h.run_steps(4); + let tagged = |h: &Harness<'static, PdfCraftApp>| { + let (id, report) = h.state().a11y.report.as_ref().unwrap(); + assert_eq!(Some(*id), h.state().active_ids().map(|(_, id)| id), "the report is the active document's"); + let r = report.result(Rule::TaggedPdf).unwrap(); + (r.status, r.findings.len()) + }; + assert!(h.state_mut().execute("a11y.check")); + h.run_steps(3); + assert_eq!(h.state().dialog, None); + right_click(&mut h, "Tagged PDF - Failed"); + h.get_by_label("Skip Rule").click(); + h.run_steps(2); + assert_eq!(tagged(&h), (Status::Skipped, 0)); + + h.state_mut().active = Some(1); + h.run_steps(2); + assert!(h.state_mut().execute("a11y.check")); + h.run_steps(3); + assert_eq!(h.state().dialog, None); + assert_eq!(tagged(&h), (Status::Failed, 2), "b.pdf's own result"); + // Starting again from the options dialog forgets b.pdf's skips, not a.pdf's. + right_click(&mut h, "Tagged PDF - Failed"); + h.get_by_label("Skip Rule").click(); + h.run_steps(2); + assert_eq!(tagged(&h), (Status::Skipped, 0)); + h.state_mut().a11y_options.show_dialog = true; + assert!(h.state_mut().execute("a11y.check")); + h.run_steps(2); + h.get_by_label("Start Checking").click(); + h.run_steps(3); + assert_eq!(tagged(&h), (Status::Failed, 2)); + + h.state_mut().active = Some(0); + h.run_steps(2); + h.state_mut().run_accessibility_check(); + h.run_steps(2); + assert_eq!(tagged(&h), (Status::Skipped, 0), "a.pdf keeps its skip"); +} + #[test] fn checker_summary_does_not_hide_manual_review() { let mut h = Harness::builder().with_size(egui::vec2(1400.0, 900.0)).build_eframe(move |_cc| {