Add Qt accessibility enforcement tooling for src/gui/ (AGENTS.md + CI check)#3289
Conversation
- AGENTS.md: new "Accessibility Enforcement" section with canonical Qt patterns for setAccessibleName, QAccessibleValueChangeEvent, QAccessibleInterface, focus policy, and the interactive-QLabel anti-pattern. All six AI tools that read AGENTS.md pick this up automatically at the point they touch src/gui/ files. - tools/check_a11y.py: static checker (pure Python, no Qt toolchain) that emits GitHub Actions inline annotations for four patterns: interactive QLabel without accessible role, value-change methods missing updateAccessibility, widget constructors with no setAccessibleName calls, and paintEvent overrides without a QAccessibleInterface companion. Exits 0 always -- warning-only. - .github/workflows/a11y-check.yml: runs check_a11y.py on every PR against the changed src/gui/ files only. Completes in seconds. Fixes and structural items are tracked in aethersdr#3288.
…aethersdr#3288). When check_a11y.py finds issues, the workflow now: - Applies the aetherclaude-eligible label so AetherClaude generates remediation patches automatically (mechanical fixes: setAccessibleName, QAccessibleValueChangeEvent, focusPolicy). - Posts a PR comment summarising the finding count and explaining what happens next. Both steps use continue-on-error: true so fork PRs (read-only token) degrade gracefully -- inline diff annotations still appear regardless.
|
@ten9876 — wanted to reach out directly and share the thinking behind this PR, since it might look like "meta tooling for its own sake" without context. I'm a blind ham radio operator (AI5OS) and I use AetherSDR with VoiceOver on macOS. Phase 1 (#899, merged) was a great start — named controls make a real difference. But the Phase 2 audit (#3288) turned up something more fundamental: That's fixable, and I've scoped the fixes in #3288. But what I really wanted to avoid is landing those fixes and then watching them quietly regress over time as new code is added — which is what tends to happen when accessibility isn't part of the normal contribution workflow. This PR tries to solve that problem without adding friction:
I'm happy to adjust any of this — if you'd prefer the AGENTS.md changes go somewhere else, or if the linter checks are too noisy, just say the word. The structural fixes in #3288 can land independently; this is just the scaffolding to keep them from drifting. Thanks for the work on this project — it's one of the most capable Qt6 radio clients out there and I'd love to see it be the accessible one too. — Justin / AI5OS |
|
Holding this PR until I can discuss AGENTS.md changes with Jeremy. |
Two adjustments to the accessibility enforcement scaffolding so it can land cleanly on `main`: 1. Move the 81-line "Accessibility Enforcement" prose out of AGENTS.md and into a new `docs/a11y.md` so AGENTS.md stays the tight architectural index it already is (every other section is brief with pointers to a canonical file — MeterSmoother, CwDecodeSettings.h, etc.). AGENTS.md retains a 4-line "Accessibility" sub-section linking to `docs/a11y.md`, so all six AI tools that read AGENTS.md still pick up the rules at the point they touch `src/gui/`. The relocated doc also gains the Phase 1 → Phase 2 background as opening context so a first-time reader understands *why* the rules exist. 2. Drop the "Label PR for AetherClaude auto-remediation" step from the workflow. Per AGENTS.md and CONSTITUTION.md (Principle X), the `aetherclaude-eligible` label is the **issue-level** claim mechanism that gates AetherClaude implementation work. Auto-applying it to every PR with `src/gui/` findings would (a) repurpose the label off-spec, and (b) trigger AetherClaude to write remediation patches on top of community contributors' open PRs without their consent. The summary comment now points readers to docs/a11y.md and the tracking issue (aethersdr#3288) instead, so the remediation path is opt-in. The static checker (`tools/check_a11y.py`) and its CI workflow (inline GitHub annotations, exit 0 always, ~10 s runtime) are unchanged otherwise. Author's intent — informational, never build-blocking — is preserved. Principle XI.
|
Hi @w9fyi — coordination update. Pushed What changed: 1. Relocated the AGENTS.md prose to 2. Dropped the "Label PR for AetherClaude auto-remediation" workflow step. Everything else is untouched: Real-talk on the work itself: the lived-experience framing in your direct comment to @ten9876 — "the S-meter reads 'S-Meter' and never updates, the VFO frequency never changes as far as my screen reader is concerned" — is the most valuable single bit of context this project has gotten on accessibility, and the grep confirms it (zero @ten9876 — flagging for your review on the relocation + the auto-label removal; both align with the existing AGENTS.md / CONSTITUTION.md semantics, but they're meaningful changes to a meaningful PR so I want you to see them before merge. If you approve, this is ready to land and unblocks the Phase 2a quick-wins from #3288. Principle X. (self-assigned before pushing.) |
Followup to 4034535. Empirical run on src/gui/ before this change emitted 899 findings — 841 of them from Check 2 (value-method-missing-a11y-update) matching every `m_button->setText("Cancel")` call site inside a constructor body. Signal drowned in button labels. After this change: 13 findings, all genuine targets (VfoWidget::updateFreqLabel, SMeterWidget paint surface, the 4 interactive-QLabel hits Justin's audit already flagged). What changed: 1. Check 2 scoped to method DEFINITIONS, not call sites. Regex now requires `ClassName::method(` — so call-site invocations like `m_btn->setText("Cancel")` inside a constructor body no longer match. The watched-name list drops `setValue` and `setText` (too universal to be a useful signal) and adds domain-specific names that show intent at the call site too: `setDbm`, `setFrequency`, `updateReadout`, `updateLabel`, `updateLevel`, `updateValue`. Out-of-line definitions in the .cpp are where the high-impact gaps live (SMeterWidget::setLevel, VfoWidget::updateFreqLabel), and that's what the lint now catches. 2. Check 4 narrowed to data-bearing widgets. A `paintEvent` override on a purely decorative widget (custom badge, gradient backdrop) has no data to announce and no payoff in a QAccessibleInterface subclass. Check 4 now requires the file to ALSO declare or define one of the watched value-change methods before flagging. Drops 52 → 4 findings; the 4 remaining are SMeterWidget, VfoWidget, and LevelBar — exactly the high-impact custom renders. 3. Suppression comments — first-class escape hatch. - `// a11y-check: skip-file` anywhere in the file → skip all checks for that file (decorative widgets, third-party subclasses, work-in-progress files). - `// a11y-check: skip` on a value-method definition line or in its body → skip Check 2 for that one method (legitimate cases: an internal cache setter where the public-facing setter already announces, or a method that delegates to an announcer). Doc explicitly states these are not "lint debt": there is no review pressure to remove them, and other AI agents are instructed to leave them alone unless asked to revisit. The point of the lint is to surface genuine misses, not to coerce ceremony onto widgets that don't need it. 4. Throttling guidance for high-rate updaters. A frequency-label setText driven at 30+ Hz during continuous tuning would spam VoiceOver into uselessness — and bother sighted users on the same machine because the SR overlay paints over the UI. New doc section explains the three patterns that work (MeterSmoother gate, sliderReleased instead of valueChanged, ~10 Hz debounce) and explicitly says "noisy updateAccessibility is worse than no updateAccessibility — prefer slightly stale to spammy." 5. Doc tone softened. - Title: "Accessibility Enforcement — Rules" → "Accessibility — Patterns for `src/gui/`". CI exits 0; "Enforcement / Rules" framed a nudge as a gate. - "Required on every QWidget subclass constructor" → "Naming interactive and informational widgets." Layout-only containers (QFrame wrappers, decorative spacers, static "Hz" suffix labels) are explicitly called out as not needing a name. - setAccessibleDescription moved from required to optional ("add only when the name alone doesn't convey purpose; a redundant description becomes noise on a screen reader"). The Phase 1 → Phase 2 background paragraph is preserved verbatim. Author intent (warning-only, inline annotations, no build block, no surprise to community contributors) is preserved. The lint now hits 13 genuine targets across 282 files instead of drowning the diff annotation panel in setText() noise. Principle XI.
|
Second fix-up pushed ( The breakdown is the interesting bit:
The 13 remaining findings are exactly the high-impact targets @w9fyi called out in #3288: Three additional pieces beyond the regex tightening:
@w9fyi — your Phase 1 → Phase 2 background paragraph and the actual canonical patterns are preserved verbatim. The changes are about how the lint behaves and how the doc frames the patterns, not the patterns themselves. If you'd like a different threshold or a different watched-name list, happy to adjust — these are tuning knobs, not architecture. @ten9876 — second adjustment for your review on top of the relocation + auto-label-drop from Principle XI. |
…t link Two small tightenings before merge: 1. docs/a11y.md — "the rules below are how we keep it fixed" → "the patterns below". Matches the deliberately softened "patterns, not gates" framing in the rest of the doc; "rules" was a leftover from the earlier prescriptive wording. 2. .github/workflows/a11y-check.yml — fix the canonical-doc link in the auto-posted summary comment. The previous "../blob/main/docs/a11y.md" resolved from the PR comment URL ".../pull/N" to ".../pull/blob/main/..." which 404s. Plain "docs/a11y.md" works because GitHub resolves relative paths in PR/issue comments against the repository root. No behavior change to the linter or workflow logic. Empirical noise floor still 13 findings across 282 src/gui/ files (unchanged). Principle XI.
Two related fixes to TitleBar's new PanLock accessors so all four platforms compile again: 1. signals: vs. public: — MOC parses anything inside a `signals:` block as a signal declaration, so the inline `bool isPanFollowChecked() const` and `void setPanFollowChecked(bool)` were rejected at moc time with "Not a signal declaration" (TitleBar.h:56). Both methods are accessors, not signals — they belong in `public:`. 2. Out-of-line over inline. Even after the section move, the inline bodies referenced `m_panFollowBtn->isChecked()` and `QSignalBlocker`, which need the full QPushButton type. The header only forward-declares QPushButton, so inline bodies couldn't compile in callers either (the moc error masked this until now). Moving the definitions to TitleBar.cpp keeps the header light and matches the style of neighbours like `isSystemMoveAreaAt`. Build verified locally on Arch Linux x86 — 632/632 clean (only the pre-existing unrelated macDaxDriverInstalled warning). Same maintainer fix-up pattern as aethersdr#3279/aethersdr#3286/aethersdr#3289/aethersdr#3381/aethersdr#3398/aethersdr#3417/aethersdr#3439/aethersdr#3441. Principle XI.
…tions Follow-up to #3289 closing five gaps surfaced by reviewing our doc against the WCAG/POUR-grounded coverage at accessibilitychecker.org. Our existing doc is materially deeper than that article on Qt-specific patterns (live value announcements, throttling, custom-painted widgets, lint mechanics), but it was missing five concrete things every Qt-desktop a11y guide should have. All five land as additions; no existing section is changed. 1. "What we're following" — three-line POUR (Perceivable / Operable / Understandable / Robust) grounding right after the Background. Cites WCAG 2.1 quick-ref. Future contributors asking "why does this rule exist" now get "WCAG 2.1 says so" rather than "because the project says so." 2. Form-field instructions — added to the existing "Accessible description" bullet. For QSpinBox / QLineEdit / QComboBox, the accessible name answers "what is this?" and the description answers "what do I type?". Tooltips don't reach the screen reader; the description does — that's the right home for input semantics. 3. Keyboard-only test — added to the existing "Tab focus" bullet. Tab to the widget from a sibling, activate with Space/Return, no mouse. If you can't reach it the focus policy is wrong; if Space/Return doesn't fire it the widget needs a keyPressEvent (or it should have been a QPushButton — links to the QLabel anti-pattern section). 4. Colour contrast — new section between "Live value updates" and "Custom-painted widgets." WCAG 4.5:1 normal text / 3:1 large text and non-text components. Cites #3441 (Reboot Radio button) as the canonical disabled-state-blends-into-background case so future readers see why the section exists. WebAIM Contrast Checker + the OS-level "Increase Contrast" mode as the practical verification workflow. 5. Manual verification — new section between "Suppressing the lint" and "CI enforcement." Names the three platform screen readers (VoiceOver, NVDA/Narrator, Orca) with the actual keystrokes to launch them, and tells contributors to walk the widget the way a screen-reader user would. Lint = static; this = real test. Explicit that the lint catches what it can detect statically and nothing more. Doc grows 166 → 254 lines (+88). Section order flows motivation → grounding → patterns → escape valves → verification → mechanism. Principle XI.
Two related fixes to TitleBar's new PanLock accessors so all four platforms compile again: 1. signals: vs. public: — MOC parses anything inside a `signals:` block as a signal declaration, so the inline `bool isPanFollowChecked() const` and `void setPanFollowChecked(bool)` were rejected at moc time with "Not a signal declaration" (TitleBar.h:56). Both methods are accessors, not signals — they belong in `public:`. 2. Out-of-line over inline. Even after the section move, the inline bodies referenced `m_panFollowBtn->isChecked()` and `QSignalBlocker`, which need the full QPushButton type. The header only forward-declares QPushButton, so inline bodies couldn't compile in callers either (the moc error masked this until now). Moving the definitions to TitleBar.cpp keeps the header light and matches the style of neighbours like `isSystemMoveAreaAt`. Build verified locally on Arch Linux x86 — 632/632 clean (only the pre-existing unrelated macDaxDriverInstalled warning). Same maintainer fix-up pattern as aethersdr#3279/aethersdr#3286/aethersdr#3289/aethersdr#3381/aethersdr#3398/aethersdr#3417/aethersdr#3439/aethersdr#3441. Principle XI.
…tions (#3443) ## Summary Follow-up to #3289 closing five gaps surfaced by reviewing `docs/a11y.md` against the WCAG/POUR-grounded coverage at [accessibilitychecker.org/blog/a11y/](https://www.accessibilitychecker.org/blog/a11y/). Our existing doc is materially deeper than that article on Qt-specific patterns (live value announcements, throttling, custom-painted widgets, lint mechanics) — but it was missing five concrete things every Qt-desktop a11y guide should have. All five land as pure additions; no existing section is modified. ## What's added 1. **"What we're following" section** — three-line POUR (Perceivable / Operable / Understandable / Robust) grounding right after the Background. Cites WCAG 2.1 quick-ref. Future contributors asking *"why does this rule exist"* now get *"WCAG 2.1 says so"* rather than *"because the project says so."* 2. **Form-field instructions guidance** — added to the existing "Accessible description" bullet. For `QSpinBox` / `QLineEdit` / `QComboBox`, the accessible name answers *"what is this?"* and the description answers *"what do I type?"*. Tooltips don't reach the screen reader; the description does. 3. **Keyboard-only test** — added to the existing "Tab focus" bullet. Tab to the widget from a sibling, activate with `Space`/`Return`, no mouse. Links to the existing QLabel anti-pattern section as the structural escape. 4. **Colour contrast section** — between "Live value updates" and "Custom-painted widgets." WCAG **4.5:1 normal text / 3:1 large text** and non-text components. Cites #3441 (Reboot Radio button) as the canonical disabled-state-blends-into-background case so future readers see *why* the section exists. WebAIM Contrast Checker + the OS-level "Increase Contrast" mode as the practical verification workflow. 5. **Manual verification section** — between "Suppressing the lint" and "CI enforcement." Names the three platform screen readers ([VoiceOver](https://www.apple.com/accessibility/), [NVDA](https://www.nvaccess.org/) / Narrator, [Orca](https://help.gnome.org/users/orca/stable/)) with the actual keystrokes to launch them. Lint = static; this = real test. Explicit that *"the lint catches what it can detect statically — this is the real test."* ## Doc shape | | Before | After | |---|---|---| | Lines | 166 | 254 | | Sections | 7 | 9 | Section order now flows: motivation → grounding → patterns → escape valves → verification → mechanism. ## What I *didn't* import from the article - **Legal framing** ("legally mandated… legal action or fines"). Out of voice. AetherSDR's a11y motivation is "a blind ham operator filed #3288 and we want AetherSDR usable for him" — that's a stronger frame than fear of lawsuits. - **POUR as the organizing structure of the whole doc**. The article is structured around POUR; we'd lose clarity if we reshuffled our concrete Qt sections under POUR headings. Mention WCAG/POUR once near the top, keep our pragmatic structure. - **Alt text on images / captions on video**. We have ~zero of either in AetherSDR — no help text videos, the only "images" are panadapter renders (covered by our custom-painted-widgets section). ## Test plan - [x] `docs/a11y.md` parses as valid markdown (9 `##` sections, no orphaned formatting). - [x] Links checked: WCAG 2.1 quickref, WebAIM contrast checker + articles/contrast, NVDA, Apple Accessibility, Orca docs — all resolve. - [x] No code changes — `tools/check_a11y.py` and the workflow are untouched, so no CI behaviour change. Refs #3288 #3289
… check) (aethersdr#3289) Companion to aethersdr#3288 (Phase 2 accessibility audit). This PR makes accessibility **automatic and invisible** — AI agents and human contributors working on `src/gui/` get guidance and inline CI annotations without any visual changes to the app. ## What's in this PR ### `AGENTS.md` — new Accessibility Enforcement section All six AI tools that read this repo's canonical agent guide (`CLAUDE.md`, `copilot-instructions.md`, `GEMINI.md`, `CONVENTIONS.md`, etc. all point here) will now see the accessibility rules whenever they touch `src/gui/`. The section covers: - `setAccessibleName` / `setAccessibleDescription` in every widget constructor - `QAccessibleValueChangeEvent` in value-change methods (`setLevel`, `setValue`, `updateFreqLabel`, `setText`) - `setFocusPolicy(Qt::TabFocus)` for interactive widgets - `QAccessibleInterface` subclass requirement for `paintEvent` overrides - The interactive `QLabel` anti-pattern (with correct fix path) - Common misconception corrected: `Qt::WA_AcceptTouchEvents` does **not** hide a widget from AT tools ### `tools/check_a11y.py` — static accessibility linter Pure Python, no Qt toolchain required. Four checks, each operating on file text: 1. `QLabel` used as an interactive element (has `mousePressEvent` or appears in `eventFilter`) — flag as missing accessible role 2. Value-change method defined without `QAccessible::updateAccessibility` in its body 3. `QWidget` subclass constructor that creates child widgets but calls `setAccessibleName` zero times 4. `paintEvent` override without a `QAccessibleInterface` subclass or `*Accessible.h` companion Output format is GitHub Actions `::warning file=...,line=...,title=...::` so findings appear as **inline PR annotations** on the diff. Exit code is always 0 — accessibility is informational, never a build-blocker. ### `.github/workflows/a11y-check.yml` Runs on every PR. Diffs against the base branch, filters to changed `src/gui/` files, passes them to `check_a11y.py`. Completes in seconds (Python only — no compile step). ## What this does NOT do - No changes to any file in `src/` — the quick-win `setAccessibleName` additions and structural fixes are tracked in aethersdr#3288 and will follow as separate PRs - No new dependencies - No visual changes to the application ## Verification ```sh # Should emit warnings for SMeterWidget::setLevel() and SpectrumWidget paint override python3 tools/check_a11y.py src/gui/SMeterWidget.cpp src/gui/SpectrumWidget.cpp ``` — AI5OS (@w9fyi) --------- Co-authored-by: Jeremy [KK7GWY] <kk7gwy@aethersdr.com>
…tions (aethersdr#3443) ## Summary Follow-up to aethersdr#3289 closing five gaps surfaced by reviewing `docs/a11y.md` against the WCAG/POUR-grounded coverage at [accessibilitychecker.org/blog/a11y/](https://www.accessibilitychecker.org/blog/a11y/). Our existing doc is materially deeper than that article on Qt-specific patterns (live value announcements, throttling, custom-painted widgets, lint mechanics) — but it was missing five concrete things every Qt-desktop a11y guide should have. All five land as pure additions; no existing section is modified. ## What's added 1. **"What we're following" section** — three-line POUR (Perceivable / Operable / Understandable / Robust) grounding right after the Background. Cites WCAG 2.1 quick-ref. Future contributors asking *"why does this rule exist"* now get *"WCAG 2.1 says so"* rather than *"because the project says so."* 2. **Form-field instructions guidance** — added to the existing "Accessible description" bullet. For `QSpinBox` / `QLineEdit` / `QComboBox`, the accessible name answers *"what is this?"* and the description answers *"what do I type?"*. Tooltips don't reach the screen reader; the description does. 3. **Keyboard-only test** — added to the existing "Tab focus" bullet. Tab to the widget from a sibling, activate with `Space`/`Return`, no mouse. Links to the existing QLabel anti-pattern section as the structural escape. 4. **Colour contrast section** — between "Live value updates" and "Custom-painted widgets." WCAG **4.5:1 normal text / 3:1 large text** and non-text components. Cites aethersdr#3441 (Reboot Radio button) as the canonical disabled-state-blends-into-background case so future readers see *why* the section exists. WebAIM Contrast Checker + the OS-level "Increase Contrast" mode as the practical verification workflow. 5. **Manual verification section** — between "Suppressing the lint" and "CI enforcement." Names the three platform screen readers ([VoiceOver](https://www.apple.com/accessibility/), [NVDA](https://www.nvaccess.org/) / Narrator, [Orca](https://help.gnome.org/users/orca/stable/)) with the actual keystrokes to launch them. Lint = static; this = real test. Explicit that *"the lint catches what it can detect statically — this is the real test."* ## Doc shape | | Before | After | |---|---|---| | Lines | 166 | 254 | | Sections | 7 | 9 | Section order now flows: motivation → grounding → patterns → escape valves → verification → mechanism. ## What I *didn't* import from the article - **Legal framing** ("legally mandated… legal action or fines"). Out of voice. AetherSDR's a11y motivation is "a blind ham operator filed aethersdr#3288 and we want AetherSDR usable for him" — that's a stronger frame than fear of lawsuits. - **POUR as the organizing structure of the whole doc**. The article is structured around POUR; we'd lose clarity if we reshuffled our concrete Qt sections under POUR headings. Mention WCAG/POUR once near the top, keep our pragmatic structure. - **Alt text on images / captions on video**. We have ~zero of either in AetherSDR — no help text videos, the only "images" are panadapter renders (covered by our custom-painted-widgets section). ## Test plan - [x] `docs/a11y.md` parses as valid markdown (9 `##` sections, no orphaned formatting). - [x] Links checked: WCAG 2.1 quickref, WebAIM contrast checker + articles/contrast, NVDA, Apple Accessibility, Orca docs — all resolve. - [x] No code changes — `tools/check_a11y.py` and the workflow are untouched, so no CI behaviour change. Refs aethersdr#3288 aethersdr#3289
Companion to #3288 (Phase 2 accessibility audit).
This PR makes accessibility automatic and invisible — AI agents and human contributors working on
src/gui/get guidance and inline CI annotations without any visual changes to the app.What's in this PR
AGENTS.md— new Accessibility Enforcement sectionAll six AI tools that read this repo's canonical agent guide (
CLAUDE.md,copilot-instructions.md,GEMINI.md,CONVENTIONS.md, etc. all point here) will now see the accessibility rules whenever they touchsrc/gui/. The section covers:setAccessibleName/setAccessibleDescriptionin every widget constructorQAccessibleValueChangeEventin value-change methods (setLevel,setValue,updateFreqLabel,setText)setFocusPolicy(Qt::TabFocus)for interactive widgetsQAccessibleInterfacesubclass requirement forpaintEventoverridesQLabelanti-pattern (with correct fix path)Qt::WA_AcceptTouchEventsdoes not hide a widget from AT toolstools/check_a11y.py— static accessibility linterPure Python, no Qt toolchain required. Four checks, each operating on file text:
QLabelused as an interactive element (hasmousePressEventor appears ineventFilter) — flag as missing accessible roleQAccessible::updateAccessibilityin its bodyQWidgetsubclass constructor that creates child widgets but callssetAccessibleNamezero timespaintEventoverride without aQAccessibleInterfacesubclass or*Accessible.hcompanionOutput format is GitHub Actions
::warning file=...,line=...,title=...::so findings appear as inline PR annotations on the diff. Exit code is always 0 — accessibility is informational, never a build-blocker..github/workflows/a11y-check.ymlRuns on every PR. Diffs against the base branch, filters to changed
src/gui/files, passes them tocheck_a11y.py. Completes in seconds (Python only — no compile step).What this does NOT do
src/— the quick-winsetAccessibleNameadditions and structural fixes are tracked in Phase 2 accessibility: live-region announcements + structural fixes for custom widgets #3288 and will follow as separate PRsVerification
# Should emit warnings for SMeterWidget::setLevel() and SpectrumWidget paint override python3 tools/check_a11y.py src/gui/SMeterWidget.cpp src/gui/SpectrumWidget.cpp— AI5OS (@w9fyi)